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

Withdraw available stake #441

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
17 changes: 17 additions & 0 deletions clap-utils/src/input_validators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,23 @@ where
}
}

pub fn is_amount_or_all_or_available<T>(amount: T) -> Result<(), String>
CriesofCarrots marked this conversation as resolved.
Show resolved Hide resolved
where
T: AsRef<str> + Display,
{
if amount.as_ref().parse::<u64>().is_ok()
|| amount.as_ref().parse::<f64>().is_ok()
|| amount.as_ref() == "ALL"
|| amount.as_ref() == "AVAILABLE"
{
Ok(())
} else {
Err(format!(
"Unable to parse input amount as integer or float, provided: {amount}"
))
}
}

pub fn is_rfc3339_datetime<T>(value: T) -> Result<(), String>
where
T: AsRef<str> + Display,
Expand Down
37 changes: 33 additions & 4 deletions cli/src/spend_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use {
crate::{
checks::{check_account_for_balance_with_commitment, get_fee_for_messages},
cli::CliError,
stake,
},
clap::ArgMatches,
solana_clap_utils::{input_parsers::lamports_of_sol, offline::SIGN_ONLY_ARG},
Expand All @@ -15,6 +16,7 @@ use {
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum SpendAmount {
All,
Available,
Some(u64),
RentExempt,
}
Expand All @@ -35,9 +37,16 @@ impl SpendAmount {
}

pub fn new_from_matches(matches: &ArgMatches<'_>, name: &str) -> Self {
let amount = lamports_of_sol(matches, name);
let sign_only = matches.is_present(SIGN_ONLY_ARG.name);
SpendAmount::new(amount, sign_only)
let amount = lamports_of_sol(matches, name);
if amount.is_some() {
return SpendAmount::new(amount, sign_only);
}
match matches.value_of(name).unwrap_or("ALL") {
"ALL" if !sign_only => SpendAmount::All,
"AVAILABLE" if !sign_only => SpendAmount::Available,
_ => panic!("Only specific amounts are supported for sign-only operations"),
}
}
}

Expand Down Expand Up @@ -96,7 +105,7 @@ where
)?;
Ok((message, spend))
} else {
let from_balance = rpc_client
let mut from_balance = rpc_client
.get_balance_with_commitment(from_pubkey, commitment)?
.value;
let from_rent_exempt_minimum = if amount == SpendAmount::RentExempt {

Choose a reason for hiding this comment

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

Here

Copy link
Author

Choose a reason for hiding this comment

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

I made change to get the from_rent_exempt_minimum with SpendAmount::Available case.
Then I subtract from the balance 👍

Choose a reason for hiding this comment

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

Is this issue still open intentionally? I'm looking for ways to pitch in, but don't want to duplicate efforts.

Expand All @@ -105,6 +114,26 @@ where
} else {
0
};
if amount == SpendAmount::Available {
if let Some(account) = rpc_client
.get_account_with_commitment(from_pubkey, commitment)?
.value
{
if account.owner == solana_sdk::stake::program::id() {
let state = stake::get_account_stake_state(
rpc_client,
from_pubkey,
account,
true,
None,
false,
)?;
if let Some(active_stake) = state.active_stake {
from_balance = from_balance.saturating_sub(active_stake);

Choose a reason for hiding this comment

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

Unfortunately, there's one more way this is broken.
Currently, the stake program takes a conservative approach to partially activated accounts.

// Assume full stake if the stake account hasn't been
// de-activated, because in the future the exposed stake
// might be higher than stake.stake() due to warmup
stake.delegation.stake

In other words, if a stake account has some active stake, the entire delegation at the point when it started delegating is locked to withdrawals, even if some of it is still activating over multiple epochs.

The fix here is to also subtract state.activating_stake.unwrap_or_default()

Sorry for not catching this sooner; in truth, we ought to work up a series of withdrawal integration tests here, using SpendAmount::Available (and SpendAmount::All) to catch all these edge cases. Please let me know if that's something you're interested/willing to work on.

Copy link
Author

Choose a reason for hiding this comment

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

No problem !
So when I have Some state.active_stake I need to also subtract activating_stake at L134?

Regarding the stake account state I see other fields like delegated_stake deactivating_stake. Is it something to take into consideration?

About the integration tests, should it be done in this PR or in another one?

Choose a reason for hiding this comment

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

So when I have Some state.active_stake I need to also subtract activating_stake at L134?

No, when you have any activating_stake it needs to be subtracted. So this needs to happen outside of the if let Some(active_stake) case.

Regarding the stake account state I see other fields like delegated_stake deactivating_stake. Is it something to take into consideration?

No, deactivating_stake is already included in active_stake, and delegated_stake doesn't reflect activation or deactivation history.
However, to understand all the fields better, I recommend you start with this method, and read upstream to Delegation::stake_activating_and_deactivating()

About the integration tests, should it be done in this PR or in another one?

If you are up for it in this PR, that would be best. That way we can be confident we are not merging any edge-case bugs.

Thank you!

}
}
}
}
let (message, SpendAndFee { spend, fee }) = resolve_spend_message(
rpc_client,
amount,
Expand Down Expand Up @@ -172,7 +201,7 @@ where
fee,
},
)),
SpendAmount::All => {
SpendAmount::All | SpendAmount::Available => {
let lamports = if from_pubkey == fee_pubkey {
from_balance.saturating_sub(fee)
} else {
Expand Down
67 changes: 58 additions & 9 deletions cli/src/stake.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ impl StakeSubCommands for App<'_, '_> {
.index(2)
.value_name("AMOUNT")
.takes_value(true)
.validator(is_amount_or_all)
.validator(is_amount_or_all_or_available)
CriesofCarrots marked this conversation as resolved.
Show resolved Hide resolved
.required(true)
.help(
"The amount to send to the stake account, in SOL; accepts keyword ALL",
Expand Down Expand Up @@ -249,7 +249,7 @@ impl StakeSubCommands for App<'_, '_> {
.index(2)
.value_name("AMOUNT")
.takes_value(true)
.validator(is_amount_or_all)
.validator(is_amount_or_all_or_available)
CriesofCarrots marked this conversation as resolved.
Show resolved Hide resolved
.required(true)
.help(
"The amount to send to the stake account, in SOL; accepts keyword ALL",
Expand Down Expand Up @@ -606,11 +606,11 @@ impl StakeSubCommands for App<'_, '_> {
.index(3)
.value_name("AMOUNT")
.takes_value(true)
.validator(is_amount_or_all)
.validator(is_amount_or_all_or_available)
.required(true)
.help(
"The amount to withdraw from the stake account, in SOL; accepts \
keyword ALL",
keyword ALL and AVAILABLE",
),
)
.arg(
Expand Down Expand Up @@ -2554,11 +2554,29 @@ pub fn process_show_stake_account(
use_csv: bool,
) -> ProcessResult {
let stake_account = rpc_client.get_account(stake_account_address)?;
let state = get_account_stake_state(
rpc_client,
stake_account_address,
stake_account,
use_lamports_unit,
with_rewards,
use_csv,
)?;
Ok(config.output_format.formatted_string(&state))
}

pub fn get_account_stake_state(
rpc_client: &RpcClient,
stake_account_address: &Pubkey,
stake_account: solana_sdk::account::Account,
use_lamports_unit: bool,
with_rewards: Option<usize>,
use_csv: bool,
) -> Result<CliStakeState, CliError> {
if stake_account.owner != stake::program::id() {
return Err(CliError::RpcRequestError(format!(
"{stake_account_address:?} is not a stake account",
))
.into());
)));
}
match stake_account.state() {
Ok(stake_state) => {
Expand Down Expand Up @@ -2597,12 +2615,11 @@ pub fn process_show_stake_account(
});
state.epoch_rewards = epoch_rewards;
}
Ok(config.output_format.formatted_string(&state))
Ok(state)
}
Err(err) => Err(CliError::RpcRequestError(format!(
"Account data could not be deserialized to stake state: {err}"
))
.into()),
))),
}
}

Expand Down Expand Up @@ -4490,6 +4507,38 @@ mod tests {
}
);

// Test WithdrawStake Subcommand w/ AVAILABLE amount
let test_withdraw_stake = test_commands.clone().get_matches_from(vec![
"test",
"withdraw-stake",
&stake_account_string,
&stake_account_string,
"AVAILABLE",
]);

assert_eq!(
parse_command(&test_withdraw_stake, &default_signer, &mut None).unwrap(),
CliCommandInfo {
command: CliCommand::WithdrawStake {
stake_account_pubkey,
destination_account_pubkey: stake_account_pubkey,
amount: SpendAmount::Available,
withdraw_authority: 0,
custodian: None,
sign_only: false,
dump_transaction_message: false,
blockhash_query: BlockhashQuery::All(blockhash_query::Source::Cluster),
nonce_account: None,
nonce_authority: 0,
memo: None,
seed: None,
fee_payer: 0,
compute_unit_price: None,
},
signers: vec![Box::new(read_keypair_file(&default_keypair_file).unwrap())],
}
);

// Test WithdrawStake Subcommand w/ ComputeUnitPrice
let test_withdraw_stake = test_commands.clone().get_matches_from(vec![
"test",
Expand Down
Loading