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

stake-pool: Update tests for 1.17 #4913

Merged
merged 4 commits into from
Aug 6, 2023
Merged

Conversation

joncinque
Copy link
Contributor

Problem

The SPL downstream tests were disabled in solana-labs/solana#32524, but we need to re-enable the downstream tests to avoid breaking people in the future, per solana-labs/solana#32655.

After doing the work to reinstate the old StakeState and re-enabling the downstream tests, there were errors in the stake pools tests.

Solution

The biggest change relates to the async rewards payout being implemented for solana-foundation/solana-improvement-documents#15 -- we have to warp to slot + 1 in order to get out of the rewards payout time.

After that, there was one issue where we weren't serializing 200 bytes for stake accounts in a test.

And a little extra thing, I noticed that retain in the stake pool program was using a function pointer instead of a closure, so I changed to be a closure, same as all the other big_vec functions. This should improve compute-unit performance because the Rust compiler can inline closures, but not calling into function pointers.

@joncinque joncinque requested a review from 2501babe August 1, 2023 21:01
Comment on lines 2382 to 2386
let mut data = bincode::serialize::<stake::state::StakeState>(
&stake::state::StakeState::Stake(meta, stake),
)
.unwrap();
data.extend_from_slice(&[0, 0, 0, 0]);
Copy link
Member

Choose a reason for hiding this comment

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

i havent run the code to see what happens but shouldnt this have a check that the length is 196 before extending? otherwise when we move to 1.17 we would extend to 204

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I'll instead allocate a bigger vec and then write into it

2501babe
2501babe previously approved these changes Aug 6, 2023
@2501babe
Copy link
Member

2501babe commented Aug 6, 2023

otherwise lgtm

@mergify mergify bot dismissed 2501babe’s stale review August 6, 2023 15:05

Pull request has been modified.

@joncinque joncinque merged commit abc9604 into solana-labs:master Aug 6, 2023
@joncinque joncinque deleted the testup branch August 6, 2023 16:03
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