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: remove useless setters of dBFT interfaces #102

Merged
merged 6 commits into from
Mar 5, 2024

Conversation

AnnaShaleva
Copy link
Member

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

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

Project coverage is 63.60%. Comparing base (f3c9fd4) to head (a2fdfa4).

Files Patch % Lines
internal/payload/constructors.go 58.06% 13 Missing ⚠️
internal/simulation/main.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
- Coverage   63.80%   63.60%   -0.21%     
==========================================
  Files          26       26              
  Lines        1575     1547      -28     
==========================================
- Hits         1005      984      -21     
+ Misses        502      495       -7     
  Partials       68       68              

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

@AnnaShaleva
Copy link
Member Author

AnnaShaleva commented Mar 5, 2024

@roman-khimov, the only question I have is whether we need to remove all these setters from the default payloads implementations? It's kind of internal thing, thus I'm not sure it's worth to refactor it.

UPD: removed, they not needed anymore.

New question: do we need to remove almost all setters and use constructors instead? Like this:

-               commit := c.Config.NewCommit()
-               commit.SetSignature(sign)
+               commit := c.Config.NewCommit(sign)

@AnnaShaleva AnnaShaleva marked this pull request as draft March 5, 2024 15:05
@AnnaShaleva AnnaShaleva added this to the v0.2.0 milestone Mar 5, 2024
@AnnaShaleva AnnaShaleva marked this pull request as ready for review March 5, 2024 15:12
@roman-khimov
Copy link
Member

Like this

Like that. I see no reason for our

s := NewSmth()
s.SetThis()
s.SetThat()

pattern. It's just a bad interface.

@AnnaShaleva
Copy link
Member Author

@roman-khimov, can we keep two setters:

  • (Block).SetTransactions([]Transaction[H]), it's needed because block can be constructed prior to all transactions receiving;
  • (ConsensusPayload).SetValidatorIndex, it's needed to avoid passing the same argument to all constructors. For code shortness.

@roman-khimov
Copy link
Member

can we keep two setters

Sure, if they're justified. SetTransactions() is special, OK. SetValidatorIndex() seems to be required for recoveries.

@roman-khimov roman-khimov merged commit 1db32ec into master Mar 5, 2024
8 of 10 checks passed
@roman-khimov roman-khimov deleted the simplify-payload-setters branch March 5, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants