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-program: MoveStake and MoveLamports #1415

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

2501babe
Copy link
Member

@2501babe 2501babe commented May 17, 2024

Problem

due to split destinations requiring rent-exemption, there isnt a good way to move value between stake accounts without holding Withdrawer authority, because split/merge cycles require new lamports to fund split when then results in unreclaimable undelegated lamports accumulating in merge destinations

Summary of Changes

we implement two new instructions, MoveStake and MoveLamports, which move stake and lamports between stake accounts with the same authorized/lockup, using Staker authority. further detail can be found in (simd148)[https://github.com/solana-foundation/solana-improvement-documents/pull/148], which this intends to implement

Feature Gate Issue: #1610

@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 82.7%. Comparing base (20ee70c) to head (06c50ca).
Report is 55 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #1415     +/-   ##
=========================================
- Coverage    82.7%    82.7%   -0.1%     
=========================================
  Files         871      871             
  Lines      369894   370084    +190     
=========================================
+ Hits       306158   306222     +64     
- Misses      63736    63862    +126     

@2501babe 2501babe force-pushed the 20240502_newstakeixns branch 3 times, most recently from 67aaa8d to 060122b Compare June 4, 2024 21:11
@2501babe
Copy link
Member Author

2501babe commented Jun 4, 2024

note to self this should be done except i need to make and put the feature gate issue in the feature text. i need to do a test run with a panic in the new tests first tho to make sure they actually run

@2501babe 2501babe force-pushed the 20240502_newstakeixns branch 2 times, most recently from 9d5aa08 to 43a839c Compare June 5, 2024 05:32
@2501babe 2501babe added the feature-gate Pull Request adds or modifies a runtime feature gate label Jun 5, 2024
@2501babe 2501babe force-pushed the 20240502_newstakeixns branch 2 times, most recently from f38ceda to 797f2a4 Compare June 5, 2024 05:43
@2501babe 2501babe marked this pull request as ready for review June 5, 2024 22:18
@2501babe
Copy link
Member Author

2501babe commented Jun 5, 2024

ok! my last force push was just to add instruction descriptions and delete some dead test code, previously it passed ci so this version should be final

ive split the pr into five commits for ease of reviewing:

  • 6105c77: this is the most important one and contains the full impl of the new instructions
  • 316215e: this just adds the instructions to transaction-status
  • 2b24676: this adds the instruction to a few existing smoke tests in stake_instruction.rs
  • a8e39ad: this wraps the instruction in a feature gate, using the same pattern as redelegate
  • 1871f45: this is all the new tests for the new instruction, including the neostake test helpers needed to run them. worth skimming but this is the bulk of the new lines of code

@2501babe 2501babe requested a review from joncinque June 5, 2024 23:43
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Really great work! The core logic is sound, and most of my comments are minor

programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
context: &mut ProgramTestContext,
vote_account: &Pubkey,
staked_amount: u64,
) -> (Keypair, Keypair, Keypair) {

Choose a reason for hiding this comment

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

nit: not really important, but typically a helper struct is preferable to a 3-tuple

programs/stake-tests/tests/test_move_stake_and_lamports.rs Outdated Show resolved Hide resolved
@2501babe
Copy link
Member Author

2501babe commented Jun 7, 2024

note to future reviewers that 6105c77 is functionally identical to the tip of the branch but there was a structural change in a later commit, but all the important changes are in stake_instruction.rs, stake_state.rs, and stake/instruction.rs, and total ~400 lines, the other ~1500 are just tests and transaction parser code

Copy link

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

I haven't looked at stake-tests at all yet, but here's a first set of comments.

sdk/program/src/stake/instruction.rs Outdated Show resolved Hide resolved
sdk/program/src/stake/instruction.rs Outdated Show resolved Hide resolved
sdk/program/src/stake/instruction.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_instruction.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
programs/stake/src/stake_state.rs Show resolved Hide resolved
programs/stake/src/stake_state.rs Show resolved Hide resolved
programs/stake/src/stake_state.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants