*: add Hash/Address generic parameters to dBFT #94
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roman-khimov
requested changes
Feb 13, 2024
| @@ -3,13 +3,14 @@ package dbft | |||
| import ( | |||
| "testing" | |||
|
|
|||
| "github.com/nspcc-dev/neo-go/pkg/util" | |||
| "github.com/stretchr/testify/require" | |||
|
|
|||
There was a problem hiding this comment.
Surprising that we have this in code.
There was a problem hiding this comment.
We have this due to the order of commits. First commit adds generics to dBFT, and thus, it should be properly instantiated in all places. Next commit adds custom hash/address implementation, so this dependency is removed in the latter commit anyway.
There was a problem hiding this comment.
I mean the whitespace line, but it's not added, so can stay for a while.
|
BTW, likely README should be adjusted a bit as well. |
AnnaShaleva
force-pushed
the
generic-dbft
branch
from
5d5eacb
to
513537d
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #94 +/- ##
==========================================
- Coverage 73.18% 73.13% -0.06%
==========================================
Files 24 24
Lines 1365 1381 +16
==========================================
+ Hits 999 1010 +11
- Misses 298 303 +5
Partials 68 68 ☔ View full report in Codecov by Sentry. |
AnnaShaleva
force-pushed
the
generic-dbft
branch
from
513537d
to
dc662ea
Compare
A part of #2. Use generics instead of util.Uint160 and util.Uint256 types for DBFT and related components. Keep util.Uint160 and util.Uint256 only for specific DBFT implementation in testing code. The following regressions/behaviour changes were made to properly apply generics: 1. `dbft.Option` alias is removed since type parameters can't be defined on aliases (generic type aliases are prohibited). Ref. golang/go#46477. 2. Default dBFT configuration is reduced: all payload-specific defaults are removed, as described in #91. It is done because default dBFT configuration should not depend on any implementation-specific hash type. 3. DBFT configuration validation check is extended wrt point 2. 4. The check if generic `dbft.DBFT` type implements generic `dbft.Service` interface is removed since such check should be performed on particular (non-generic) DBFT implementation. Signed-off-by: Anna Shaleva <[email protected]>
A part of #2. Use custom Hash/Address implementation, get rid of util.Uint256/util.Uint160 NeoGo dependency. Signed-off-by: Anna Shaleva <[email protected]>
A part of #2, not needed anymore. Signed-off-by: Anna Shaleva <[email protected]>
AnnaShaleva
force-pushed
the
generic-dbft
branch
from
dc662ea
to
87f247c
Compare
roman-khimov
approved these changes
Feb 13, 2024
| @@ -3,13 +3,14 @@ package dbft | |||
| import ( | |||
| "testing" | |||
|
|
|||
| "github.com/nspcc-dev/neo-go/pkg/util" | |||
| "github.com/stretchr/testify/require" | |||
|
|
|||
There was a problem hiding this comment.
I mean the whitespace line, but it's not added, so can stay for a while.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Spread the generic Hash/Adress disease, add custom Hash/Address implementation and get rid of NeoGo dependency.
Close #2. @roman-khimov, let's firstly agree and merge this PR, and after that I'll move all payloads/block/tx implementation to the testing code together with custom Hash/Address implementation (#91). Now I clearly see that we need this.