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

Cli stake-split: adjust transfer amount if recipient has lamports #266

Merged
merged 8 commits into from
Mar 21, 2024

Conversation

CriesofCarrots
Copy link

Problem

The #162 cli change permits split recipients that carry a lamports balance. However, the message builder below always funds the account with the full rent-exempt reserve regardless of such an existing balance.

Summary of Changes

Some refactoring to make cases easier to follow (review by commit will be best)
Subtract (saturating) any existing lamports balance from the transfer amount

@CriesofCarrots
Copy link
Author

CriesofCarrots commented Mar 15, 2024

Sidenote: this fix allows using the CLI with the existing ledger-solana app to split without blind signing by using 2 transactions, one to transfer in the rent-exempt reserve, and the other to split.

This makes me a little inclined to backport to v1.18

@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 81.8%. Comparing base (dcc6f1e) to head (33cedb9).

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #266     +/-   ##
=========================================
- Coverage    81.9%    81.8%   -0.1%     
=========================================
  Files         837      837             
  Lines      226805   226809      +4     
=========================================
- Hits       185843   185756     -87     
- Misses      40962    41053     +91     

t-nelson
t-nelson previously approved these changes Mar 20, 2024
Copy link

@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.

ok... very after lunch 😅

lgtm!

cli/src/stake.rs Show resolved Hide resolved
owner if owner == system_program::id() => {
if !account.data.is_empty() {
Err(CliError::BadParameter(format!(
"Account {split_stake_account_address} has data and cannot be used to split stake"

Choose a reason for hiding this comment

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

i wonder if we should spruce up these error messages to be a little more normie-friendly? maybe in follow up

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll make a follow-up. If you have recommended verbiage, please do share.

cli/src/stake.rs Show resolved Hide resolved
Copy link
Author

@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.

@t-nelson Would you approve a v1.18 backport of this?

owner if owner == system_program::id() => {
if !account.data.is_empty() {
Err(CliError::BadParameter(format!(
"Account {split_stake_account_address} has data and cannot be used to split stake"
Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll make a follow-up. If you have recommended verbiage, please do share.

cli/src/stake.rs Show resolved Hide resolved
cli/src/stake.rs Show resolved Hide resolved
Copy link

mergify bot commented Mar 20, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

@CriesofCarrots CriesofCarrots merged commit dff99d0 into anza-xyz:master Mar 21, 2024
37 checks passed
mergify bot pushed a commit that referenced this pull request Mar 21, 2024
* Remove incorrect check

* Move to closure

* Use match statement instead

* Adjust rent_exempt_reserve by existing balance

* Only transfer lamports if rent_exempt_reserve needs are greater than 0

* Rename variable for clarity

* Add minimum-delegation check

* Bump test split amount to meet arbitrary mock minimum-delegation amount

(cherry picked from commit dff99d0)

# Conflicts:
#	cli/src/stake.rs
CriesofCarrots added a commit that referenced this pull request Mar 21, 2024
* Remove incorrect check

* Move to closure

* Use match statement instead

* Adjust rent_exempt_reserve by existing balance

* Only transfer lamports if rent_exempt_reserve needs are greater than 0

* Rename variable for clarity

* Add minimum-delegation check

* Bump test split amount to meet arbitrary mock minimum-delegation amount

(cherry picked from commit dff99d0)

# Conflicts:
#	cli/src/stake.rs
CriesofCarrots added a commit that referenced this pull request Mar 21, 2024
* Remove incorrect check

* Move to closure

* Use match statement instead

* Adjust rent_exempt_reserve by existing balance

* Only transfer lamports if rent_exempt_reserve needs are greater than 0

* Rename variable for clarity

* Add minimum-delegation check

* Bump test split amount to meet arbitrary mock minimum-delegation amount

(cherry picked from commit dff99d0)

# Conflicts:
#	cli/src/stake.rs
CriesofCarrots added a commit that referenced this pull request Mar 22, 2024
…rts (backport of #266) (#369)

Cli stake-split: adjust transfer amount if recipient has lamports (#266)

* Remove incorrect check

* Move to closure

* Use match statement instead

* Adjust rent_exempt_reserve by existing balance

* Only transfer lamports if rent_exempt_reserve needs are greater than 0

* Rename variable for clarity

* Add minimum-delegation check

* Bump test split amount to meet arbitrary mock minimum-delegation amount

(cherry picked from commit dff99d0)

# Conflicts:
#	cli/src/stake.rs

Co-authored-by: Tyera <[email protected]>
anwayde pushed a commit to firedancer-io/agave that referenced this pull request Jul 23, 2024
…rts (backport of anza-xyz#266) (anza-xyz#369)

Cli stake-split: adjust transfer amount if recipient has lamports (anza-xyz#266)

* Remove incorrect check

* Move to closure

* Use match statement instead

* Adjust rent_exempt_reserve by existing balance

* Only transfer lamports if rent_exempt_reserve needs are greater than 0

* Rename variable for clarity

* Add minimum-delegation check

* Bump test split amount to meet arbitrary mock minimum-delegation amount

(cherry picked from commit dff99d0)

# Conflicts:
#	cli/src/stake.rs

Co-authored-by: Tyera <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants