Conversation
2501babe
force-pushed
the
20260417_depositsol
branch
3 times, most recently
from
fa0b257 to
1e0b60b
Compare
2501babe
force-pushed
the
20260417_depositsol
branch
5 times, most recently
from
5ff8680 to
310dc5c
Compare
2501babe
force-pushed
the
20260417_depositsol
branch
from
310dc5c to
5542960
Compare
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Looks really great! Just a couple of small things
| /// Lamports to deposit into pool | ||
| pub lamports: u64, | ||
|
|
||
| /// The pool to withdraw from |
There was a problem hiding this comment.
nit
| /// The pool to withdraw from | |
| /// The pool to deposit into |
| #[clap(short, long = "pool", value_parser = |p: &str| parse_address(p, "pool_address"))] | ||
| pub pool_address: Option<Pubkey>, | ||
|
|
||
| /// The vote account corresponding to the pool to withdraw from |
There was a problem hiding this comment.
nit:
| /// The vote account corresponding to the pool to withdraw from | |
| /// The vote account corresponding to the pool to deposit into |
| .is_none() | ||
| { | ||
| return Err(format!( | ||
| "Pool {} onramp {} does not exist", |
There was a problem hiding this comment.
nit: is it worth telling people what to do? ie "Run spl-single-pool create-onramp ..."
| /// Fee charged for the `DepositSol` instruction. | ||
| pub const DEPOSIT_SOL_FEE_BPS: u64 = 100; |
There was a problem hiding this comment.
Makes sense to me! I think we could get away with 50 bps, especially considering inflation going down over time, but that could also be done in another upgrade
| } | ||
|
|
||
| interface DepositSolParams { | ||
| rpc: Rpc<GetAccountInfoApi & GetMinimumBalanceForRentExemptionApi & GetStakeMinimumDelegationApi>; |
There was a problem hiding this comment.
nit: I think this only needs GetAccountInfoApi
| rpc: Rpc<GetAccountInfoApi & GetMinimumBalanceForRentExemptionApi & GetStakeMinimumDelegationApi>; | |
| rpc: Rpc<GetAccountInfoApi>; |
| // we round division down and reject deposits too small to generate a fee | ||
| // this is to avoid pathological cases where eg someone deposits 2 lamps and pays 50% | ||
| let deposit_sol_fee = raw_tokens | ||
| .checked_mul(DEPOSIT_SOL_FEE_BPS) | ||
| .and_then(|n| n.checked_div(MAX_BPS)) | ||
| .ok_or(SinglePoolError::UnexpectedMathError)?; |
There was a problem hiding this comment.
I would recommend a ceil_div on this calculation so that we always take a little bit more.
Because of the 0 check later, it's impossible for someone to deposit less than 100 lamports anyway, which is a slick touch
| // replenish to delegate the deposit. this safely returns Ok if onramp doesnt meet minimum delegation | ||
| invoke( | ||
| &svsp_instruction::replenish_pool(program_id, vote_account_info.key), | ||
| &[ | ||
| vote_account_info.clone(), | ||
| pool_info.clone(), | ||
| pool_stake_info.clone(), | ||
| pool_onramp_info.clone(), | ||
| pool_stake_authority_info.clone(), | ||
| clock_info.clone(), | ||
| stake_history_info.clone(), | ||
| stake_config_info.clone(), | ||
| stake_program_info.clone(), | ||
| ], | ||
| )?; |
There was a problem hiding this comment.
We don't typically perform re-entrancy in our programs, so just flagging the behavior, but I can't see a reason why it would be a problem
| // deposit source must be a system account for transfer to succeed | ||
| if !user_lamport_account_info.is_signer | ||
| || user_lamport_account_info.owner != &system_program::id() | ||
| || user_lamport_account_info.lamports() < deposit_amount | ||
| { | ||
| return Err(SinglePoolError::InvalidDepositSolSource.into()); | ||
| } |
There was a problem hiding this comment.
I wasn't sure about these checks, but it's probably good to make sure one of the program accounts isn't weirdly compromised through the operation
| AccountMeta::new_readonly(system_program::id(), false), | ||
| AccountMeta::new_readonly(spl_token::id(), false), | ||
| AccountMeta::new_readonly(stake::program::id(), false), | ||
| AccountMeta::new_readonly(*program_id, false), |
There was a problem hiding this comment.
Is this actually needed for re-entrancy? Since the JS client seem ok without it, I'm going to guess not
2 participants
the latest. the greatest. it needs no further introduction. god knows ive talked about it too much already. deposit sol
closes #46