stake-pool: Add withdraw-sol command + CLI + docs #2475

joncinque · 2021-09-30T22:51:19Z

Problem

People want to withdraw SOL directly from the reserve, but this is currently only permitted if there's nowhere else to withdraw from. There are many valid usecases for someone to withdraw SOL directly from the reserve, especially for restricted pools that want easier control over all the SOL in the pool.

Solution

Add a withdraw-sol instruction! The concept is very similar to deposit-sol, except in reverse. Other additions:

sol withdrawal fee: allow managers to specify a different fee for sol withdrawals. this makes sense if you want to encourage stake withdraws
sol withdraw authority: forces sol withdrawals to be signed by a particular key
add CLI support for withdrawing sol, setting the sol withdrawal fee, and setting the sol withdraw authority
update the docs for the new instructions
rename fee -> epoch_fee for clarity
force not inlining some processor functions because we were blowing the stack size with the addition of the new instruction (who knew?)
remove the clock and rent accounts wherever they weren't needed during CPIs to the stake program

@jon-chuang could you take a look at the program changes in c6f1f34 and 819c76c?
@c3pko could you look at the doc changes in bd14d67 ?

jon-chuang

I've checked the changes to the struct and the processor.

I haven't done such a thorough check on the tests, which are pretty boilerplate.

The processor changes mainly followed the template of withdraw_stake.

The worst thing I could have missed is a missing check of some sort. I've only done a once over to cross reference and all the checks appear to be there.

jon-chuang · 2021-10-05T15:57:30Z

stake-pool/program/src/instruction.rs

+        AccountMeta::new_readonly(*user_transfer_authority, true),
+        AccountMeta::new(*pool_tokens_from, false),
+        AccountMeta::new(*reserve_stake_account, false),
+        AccountMeta::new(*lamports_to, true),


why is this a signer?

copy-pasta error! thank you for spotting this

jon-chuang · 2021-10-05T15:58:42Z

stake-pool/program/src/processor.rs

        withdrawal_fee: Fee,
-        stake_deposit_fee: Fee,
-        stake_referral_fee: u8,
+        deposit_fee: Fee,


prefer if the names were kept as they were to prevent confusion

The idea is that this is changing the behavior, so that the deposit fee set at init is actually for both stake and sol deposit. it seemed unclear to me that only the stake deposit fee is set at init, but not the sol deposit fee. this way all of them are set the same on init, to minimize surprise to a pool creator. And I didn't want to have to specify each of the fees separately on init, just to keep things simpler.

Ah I missed that... Yeah it's probably an ok compromise

jon-chuang · 2021-10-05T15:58:50Z

stake-pool/program/src/processor.rs

-        stake_deposit_fee: Fee,
-        stake_referral_fee: u8,
+        deposit_fee: Fee,
+        referral_fee: u8,


prefer if the names were kept as they were to prevent confusion

overall, it's quite inconsistent which params we include in the initialize instruction...

Yeah it's true, thankfully it's all configurable afterwards

jon-chuang · 2021-10-05T16:00:18Z

stake-pool/program/src/processor.rs

        stake_pool.check_manager(manager_info)?;

        if fee.can_only_change_next_epoch() && stake_pool.last_update_epoch < clock.epoch {
            return Err(StakePoolError::StakeListAndPoolOutOfDate.into());
        }

        fee.check_too_high()?;
-        fee.check_withdrawal(&stake_pool.withdrawal_fee)?;


did this check go inside update_fee?

yes, exactly, i'll flag it for you

jon-chuang · 2021-10-05T16:02:21Z

stake-pool/program/src/state.rs

        match fee {
            FeeType::SolReferral(new_fee) => self.sol_referral_fee = *new_fee,
            FeeType::StakeReferral(new_fee) => self.stake_referral_fee = *new_fee,
            FeeType::Epoch(new_fee) => self.next_epoch_fee = Some(*new_fee),
-            FeeType::Withdrawal(new_fee) => self.next_withdrawal_fee = Some(*new_fee),
+            FeeType::StakeWithdrawal(new_fee) => {
+                new_fee.check_withdrawal(&self.stake_withdrawal_fee)?;


ok, I guess it got moved

jon-chuang · 2021-10-05T16:05:08Z

stake-pool/program/tests/set_funding_authority.rs

@@ -17,7 +17,7 @@ use {
    },


tangential discussion but since when was github able to do this with recognizing file name changes? 😍

Huh, that's a good point, i'm not sure, but it's very welcome 😄

joncinque · 2021-10-06T02:27:52Z

@jon-chuang thanks a ton for the review! And you got it all right with regards to withdrawing from the reserve as a stake account vs withdraw sol. the two are meant to be totally equivalent, but the old withdraw_stake version has to use a stake account, to keep things consistent

joncinque added 7 commits September 29, 2021 01:54

stake-pool: plumb sol withdraw fee and authority

29cb032

Add sol withdraw instruction and processor

c6f1f34

Cleanup sysvar usage

905887e

Fix stack size violation

b3acb0d

Add tests

819c76c

Add CLI support

c4d8709

Add docs for new command

bd14d67

jon-chuang reviewed Oct 5, 2021

View reviewed changes

Integrate review feedback

19d428b

joncinque merged commit b80c10f into solana-labs:master Oct 6, 2021

joncinque deleted the sp-withdraw branch October 6, 2021 02:44

stake-pool: Add withdraw-sol command + CLI + docs #2475

stake-pool: Add withdraw-sol command + CLI + docs #2475

Uh oh!

Conversation

joncinque commented Sep 30, 2021

Problem

Solution

Uh oh!

jon-chuang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joncinque commented Oct 6, 2021

Uh oh!

Uh oh!