Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Convert tuples to structs in Stake rewards-calculation functions#24681

Merged
mergify[bot] merged 2 commits intosolana-labs:masterfrom
CriesofCarrots:stake-tuples
Apr 27, 2022
Merged

Convert tuples to structs in Stake rewards-calculation functions#24681
mergify[bot] merged 2 commits intosolana-labs:masterfrom
CriesofCarrots:stake-tuples

Conversation

@CriesofCarrots
Copy link
Copy Markdown
Contributor

@CriesofCarrots CriesofCarrots commented Apr 26, 2022

Problem

It's difficult to remember what the return values from the Stake program's internal rewards-calculation functions signify. This one is especially awful:

fn calculate_stake_points_and_credits( ...) -> Option<(u64, u64, u64)> 

Summary of Changes

Replace opaque tuples with helper structs
No behavior changes
Open to bikeshedding on struct field names.

@CriesofCarrots CriesofCarrots requested a review from t-nelson April 26, 2022 05:02
Comment thread programs/stake/src/stake_state.rs Outdated
Comment thread programs/stake/src/stake_state.rs Outdated
Comment thread programs/stake/src/stake_state.rs Outdated
Comment thread programs/stake/src/stake_state.rs Outdated
Comment thread programs/stake/src/stake_state.rs Outdated
ryoqun
ryoqun previously approved these changes Apr 26, 2022
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with nits. really appreciate spending time for this cleanup for benefits of all future readers.

@CriesofCarrots
Copy link
Copy Markdown
Contributor Author

Thanks for all the good ideas, @ryoqun !

@mergify mergify Bot dismissed ryoqun’s stale review April 26, 2022 16:27

Pull request has been modified.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 26, 2022

Codecov Report

Merging #24681 (ecf9603) into master (8ba003a) will increase coverage by 0.2%.
The diff coverage is 91.4%.

@@            Coverage Diff            @@
##           master   #24681     +/-   ##
=========================================
+ Coverage    81.8%    82.0%   +0.2%     
=========================================
  Files         632      596     -36     
  Lines      167499   165357   -2142     
  Branches      322        0    -322     
=========================================
- Hits       137169   135748   -1421     
+ Misses      30217    29609    -608     
+ Partials      113        0    -113     

t-nelson
t-nelson previously approved these changes Apr 26, 2022
Copy link
Copy Markdown
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aw. when did we draw straws? 🙂

lgtm! thanks for being the one to finally bite the bullet!

@mergify mergify Bot dismissed t-nelson’s stale review April 27, 2022 04:31

Pull request has been modified.

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Apr 27, 2022
@mergify mergify Bot merged commit db32549 into solana-labs:master Apr 27, 2022
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Apr 27, 2022
…ana-labs#24681)

* Replace opaque tuples with structs in Stake rewards calculation

* Fixup struct and field names, and remove one cumbersome destructuring
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
…ana-labs#24681)

* Replace opaque tuples with structs in Stake rewards calculation

* Fixup struct and field names, and remove one cumbersome destructuring
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jun 29, 2022
…ana-labs#24681)

* Replace opaque tuples with structs in Stake rewards calculation

* Fixup struct and field names, and remove one cumbersome destructuring
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants