Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dbft: move payloads/block/tx/crypto interfaces to dbft package #97

Merged
merged 5 commits into from
Mar 4, 2024

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented Feb 19, 2024

Two parts of dBFT structure refactoring: close #90, close #91.

TODO:

  • Improve documentation.
  • Move all default implementations to testing code or separate place.
  • Update changelog.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 66.82028% with 72 lines in your changes are missing coverage. Please review.

Project coverage is 72.30%. Comparing base (e4fad43) to head (2f4e90f).

Files Patch % Lines
internal/payload/consensus_message.go 66.66% 17 Missing and 2 partials ⚠️
consensus_message_type.go 0.00% 16 Missing ⚠️
config.go 72.50% 11 Missing ⚠️
internal/payload/recovery_message.go 35.29% 11 Missing ⚠️
dbft.go 82.14% 2 Missing and 3 partials ⚠️
internal/payload/change_view.go 0.00% 3 Missing ⚠️
internal/payload/constructors.go 57.14% 3 Missing ⚠️
send.go 87.50% 1 Missing and 1 partial ⚠️
check.go 50.00% 1 Missing ⚠️
internal/block/block.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   73.22%   72.30%   -0.93%     
==========================================
  Files          24       25       +1     
  Lines        1393     1390       -3     
==========================================
- Hits         1020     1005      -15     
- Misses        305      318      +13     
+ Partials       68       67       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Mar 1, 2024

@roman-khimov, I need some initial assessment of the resulting packages structure from your side. If the direction is OK, then I'll update the documentation. The resulting built binary is smaller than the one got from master:

-rw-rw-r--  1 anna anna 189040 мар  1 22:12 build.branch
-rw-rw-r--  1 anna anna 622930 мар  1 22:11 build.master

I'm not sure how to deal with internal package. Currently dbft package doesn't depend on internal in any way, is it enough? I'm not sure if internal is excluded from build in this case.

block/block_test.go Outdated Show resolved Hide resolved
crypto/ecdsa.go Outdated Show resolved Hide resolved
payload/change_view.go Show resolved Hide resolved
payload/message.go Outdated Show resolved Hide resolved
payload/message_test.go Outdated Show resolved Hide resolved
payload/recovery_request.go Show resolved Hide resolved
simulation/main.go Outdated Show resolved Hide resolved
helpers.go Outdated Show resolved Hide resolved
internal/block/block_test.go Outdated Show resolved Hide resolved
internal/crypto/ecdsa.go Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Member

It looks OK except for some new exports. I'd then want to simplify payload interfaces (getters/setters suck), but that can wait.

@AnnaShaleva AnnaShaleva force-pushed the tidy-dbft branch 2 times, most recently from f5818b9 to 2640fae Compare March 4, 2024 10:36
@AnnaShaleva AnnaShaleva marked this pull request as ready for review March 4, 2024 10:39
To avoid cyclic dependency during tests and completely untie dBFT from
non-generic payloads implementation.

Signed-off-by: Anna Shaleva <[email protected]>
Noone needs them, they are used only for tests and as examples.

Signed-off-by: Anna Shaleva <[email protected]>
Signed-off-by: Anna Shaleva <[email protected]>
@AnnaShaleva
Copy link
Member Author

I'd then want to simplify payload interfaces (getters/setters suck), but that can wait.

We may do this in #84.

@AnnaShaleva AnnaShaleva mentioned this pull request Mar 4, 2024
@roman-khimov roman-khimov merged commit 4f3a7fe into master Mar 4, 2024
6 of 8 checks passed
@roman-khimov roman-khimov deleted the tidy-dbft branch March 4, 2024 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants