Fee implementation passing all tests #94

enriquefynn · 2021-05-20T19:40:49Z

Implements Ruud's proposal for fee management.
Moves the variadic accounts to Lido's account, simplifying handling.
Modifies the distribution of the validator's fees, the new approach is done in one transaction per validator
Modifies tests for the new structures

naomijub · 2021-05-20T20:08:59Z

Why CI did not run for this branch?

naomijub · 2021-05-20T20:14:34Z

program/tests/initialize.rs

-    assert_eq!(lido.owner, id());
+    // let lido = get_account(&mut banks_client, &lido_accounts.lido.pubkey()).await;
+    // assert_eq!(lido.data.len(), get_packed_len::<state::Lido>());
+    // assert_eq!(lido.owner, id());


are these asserts supposed to be commented?

ruuda · 2021-05-21T07:23:52Z

Why CI did not run for this branch?

It’s targeted at fee-management-initial, but our config only lists main:

solido/.github/workflows/rust.yml

Lines 6 to 7 in c798716

    
           pull_request: 
        
             branches: [ main ]

The docs for pull_request.branches are a bit cryptic:

For a pull_request event, only branches and tags on the base are evaluated.

I think this means that if the base branch for the pull request is listed in branches, it will get evaluated. In this case the base branch is not main but fee-management-initial. I ran into this at #90 (comment) as well so I fixed it there.

ruuda · 2021-05-21T07:30:22Z

$ git diff --shortstat origin/fee-management-initial..origin/fee-management-new-struct
 11 files changed, 506 insertions(+), 521 deletions(-)

It’s a net negative, nice! 😎

ruuda

Very nice! This makes everything simpler, more readable, and safer 🎉

My only substantial comment is that we should not mix stSOL and stake pool tokens in the code, if only to not confuse the reader. Let's add a new type for stake pool tokens as well. Aside from that I only have a few minor comments.

program/src/instruction.rs

ruuda · 2021-05-21T08:21:10Z

program/src/state.rs

-        if insurance_account.key != &self.insurance_account
-            || treasury_account.key != &self.treasury_account
-            || manager_accounts.key != &self.manager_account
+        if &spl_token::state::Account::unpack_from_slice(&insurance_account_info.data.borrow())


Instead of repeating the check three times, we could make the signature

pub fn check_recipient_valid(&self, minter_program: &Pubkey, recipient: &AccountInfo) -> Result<(), LidoError>;

Then at the call site it would look almost the same:

new_fee.check_recipient_valid(&lido.st_sol_mint_program, accounts.insurance_account)?; new_fee.check_recipient_valid(&lido.st_sol_mint_program, accounts.treasury_account)?; new_fee.check_recipient_valid(&lido.st_sol_mint_program, accounts.manager_accounts)?;

and this makes it a lot more obvious that these accounts are all treated in the same way, and it makes it harder to have copy-paste bugs such as accidentally checking one account twice and leaving one unchecked in the code below. (I didn’t see such copy-paste mistake here, but steering clear of them is nicer than carefully checking for them.)

ruuda · 2021-05-21T08:36:25Z

program/src/process_management.rs

+    let token_shares = distribute_fees(
+        &lido.fee_spec,
+        lido.validator_credit_accounts.validator_accounts.len() as u64,
+        StLamports(stake_pool_fee_account.amount),


Technically, the stake pool fee account contains stake pool tokens, not stSOL, and distribute_fees converts between the two. I’m not sure how much we will be dealing with stake pool tokens in computations, but it would be good to have separate types for them, so we can prevent unit mistakes like adding stake pool tokens to stSOL. If we call them StLamports and SptLamports that’s only one character difference, that sounds like a recipe for subtle mistakes, maybe we should call the stake pool token type StakePoolTokenLamports to be sure.

To convert between the two, I think the safest thing to do would be to put that in distribute_fees. It would then look something like

pub fn distribute_fees(spec: &FeeSpec, num_validators: u64, amount_spt: StakePoolTokenLamports) -> Option<Fees> { // The process of distributing fees restores the SPT <-> stSOL peg, // we have as many new stSOL to mint, as we received in SPT fees. let amount = StLamports(amount_spt.0); // ... function body unchanged }

The StakePoolTokenLamports type doesn’t need any of the std::ops implementations for now, because we are never doing arithmetic with it.

ruuda · 2021-05-21T08:38:36Z

program/src/process_management.rs

-        validator_credit_accounts.validator_accounts[idx].amount +=
-            token_shares.each_validator_amount;
+    for vc in lido.validator_credit_accounts.validator_accounts.iter_mut() {
+        vc.st_sol_amount = (vc.st_sol_amount + token_shares.reward_per_validator)


Hah, the type system caught a hypothetical overflow bug in the old code :D

ruuda · 2021-05-21T08:41:22Z

program/src/processor.rs

@@ -235,8 +218,6 @@ pub fn process_deposit(
        .calc_pool_tokens_for_deposit(amount, total_lamports)
        .ok_or(LidoError::CalculationFailure)?;

-    let total_st_sol = lido.st_sol_total_shares + st_sol_amount;


Another hypothetical overflow prevented by the new types!

ruuda · 2021-05-21T08:44:11Z

program/src/state.rs

+/// Constant size of fee struct: 3 public keys + 4 u32
+pub const LIDO_CONSTANT_FEE_SIZE: usize = 3 * 32 + 4 * 4;
+/// Constant size of Lido
+pub const LIDO_CONSTANT_SIZE: usize = LIDO_CONSTANT_HEADER_SIZE + LIDO_CONSTANT_FEE_SIZE;


What we could do to make this a bit harder to go out of sync, is to define an inner struct for the constant-size part, and use mem::size_of, instead of computing it manually. The downside is that we need to touch all the places that refer to lido to reference this inner struct, so I’m not sure it’s worth it.

mem::size_of will take the vec as if it had 0 elements?

I mean something like this

// No variable-size things in this struct. struct LidoFixed { foo: u64, bar: Pubkey, } // All of the variable-size things after the fixed-size part. // (We should confirm that Borsh respects field order.) struct Lido { fixed: LidoFixed, variable: Vec<Quux>, } // The size of the fixed-size prefix. mem::size_of::<LidoFixed>()

ruuda · 2021-05-21T08:46:30Z

program/src/state.rs

-        return buffer_size.saturating_sub(8) / 40;
+        // 8 bytes: 4 bytes for `max_validators` + 4 bytes for number of validators in vec
+        // 40 bytes for each validator = 32 bytes + 8 bytes for st_sol_amount
+        // TODO: Make a test for this


We should probably add this test now, otherwise it will likely never happen.

I added it ^^

solido/program/src/state.rs

Line 317 in c07cd2f

fn test_validator_credit_size() {

ruuda · 2021-05-21T08:48:34Z

program/src/state.rs

+
+    // The actual amount that goes to validation can be a tiny bit lower
+    // than the target amount, when the number of validators does not divide
+    // the target amount. The loss is at most `num_validators` μstSOL.


μstSOL should now be stLamports.

ruuda · 2021-05-21T08:51:04Z

program/src/state.rs

+            },
+            distribute_fees(&spec_equal, 1, StLamports(1_000)).unwrap()
+        );
+        // Validation fee of 250 μstSOL is not a multiple of the number of


s/μstSOL/stLamports/

Also, the validation fee is no longer 250 stLamports, it’s 2000/9. I think a more interesting test would be to make the reward 999 lamports, because then at least among the different fee posts the division has no rounding error (222 stLamports would go to validation fees), but the validation fees themselves are not a multiple of 4, so we get this re-distribution of the rounding error either way.

ruuda · 2021-05-21T08:52:17Z

program/src/state.rs

-            manager_fee_numerator: 1,
-            denominator: 9,
-
+        let spec_equal = FeeSpec {


The shares for this spec are not actually equal. I think we can just call it spec.

Corrected writable accounts Add function to check token minter Use StakePoolTokenLamports instead of u64 for stake pool lamports

ruuda

Looks almost good to me, only the consistency checks for the fee accounts were lost now, so we might end up checking an account that we are never going to use 🙊

It would be good if we could somehow generate these checks automatically and make them part of from_slice, but I don't have a clear idea for how to do that yet.

ruuda · 2021-05-21T13:49:01Z

program/src/processor.rs

-    )?;
+    Lido::check_valid_minter_program(&accounts.mint_program.key, accounts.insurance_account)?;
+    Lido::check_valid_minter_program(&accounts.mint_program.key, accounts.treasury_account)?;
+    Lido::check_valid_minter_program(&accounts.mint_program.key, accounts.manager_fee_account)?;


Ooh, there are these &self.insurance_account == accounts.insurance_account.key checks that were present previously, but that are now lost.

What we might do is to just use the accounts from self.*. If accounts.* does not match, the runtime will throw an error for accessing an unspecified account.

I think it's better to leave it static but add an argument for the public key and check if it's equal to the account_info one.
Since we have to pass which one is the public key anyway.

ruuda · 2021-05-21T13:50:47Z

program/src/process_management.rs

-    )?;
+    Lido::check_valid_minter_program(&lido.st_sol_mint_program, accounts.insurance_account)?;
+    Lido::check_valid_minter_program(&lido.st_sol_mint_program, accounts.treasury_account)?;
+    Lido::check_valid_minter_program(&lido.st_sol_mint_program, accounts.manager_fee_account)?;


Also here, see my comment below about checking the consistency of the accounts.

ruuda

LGTM!

Fee implementation passing all tests

ff6eb88

enriquefynn requested review from naomijub, ruuda and malikankit and removed request for naomijub May 20, 2021 19:41

naomijub reviewed May 20, 2021

View reviewed changes

Uncomment asserts, set max validators to 10_000

da7f98f

ruuda reviewed May 21, 2021

View reviewed changes

Changes based on review

c07cd2f

Corrected writable accounts Add function to check token minter Use StakePoolTokenLamports instead of u64 for stake pool lamports

ruuda reviewed May 21, 2021

View reviewed changes

check for fee accounts to be the same as passed by accounts_info

1002a46

ruuda approved these changes May 21, 2021

View reviewed changes

naomijub approved these changes May 21, 2021

View reviewed changes

enriquefynn merged commit b1d44d4 into fee-management-initial May 21, 2021

enriquefynn deleted the fee-management-new-struct branch May 21, 2021 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fee implementation passing all tests #94

Fee implementation passing all tests #94

enriquefynn commented May 20, 2021 •

edited

Loading

naomijub commented May 20, 2021

naomijub May 20, 2021

ruuda commented May 21, 2021 •

edited

Loading

ruuda commented May 21, 2021

ruuda left a comment

ruuda May 21, 2021

ruuda May 21, 2021

ruuda May 21, 2021

ruuda May 21, 2021

ruuda May 21, 2021

enriquefynn May 21, 2021 •

edited

Loading

ruuda May 21, 2021

ruuda May 21, 2021

enriquefynn May 21, 2021 •

edited

Loading

ruuda May 21, 2021

ruuda May 21, 2021

ruuda May 21, 2021

ruuda left a comment

ruuda May 21, 2021

enriquefynn May 21, 2021

ruuda May 21, 2021

ruuda left a comment

Fee implementation passing all tests #94

Fee implementation passing all tests #94

Conversation

enriquefynn commented May 20, 2021 • edited Loading

naomijub commented May 20, 2021

Choose a reason for hiding this comment

ruuda commented May 21, 2021 • edited Loading

ruuda commented May 21, 2021

ruuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enriquefynn May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enriquefynn May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruuda left a comment

Choose a reason for hiding this comment

enriquefynn commented May 20, 2021 •

edited

Loading

ruuda commented May 21, 2021 •

edited

Loading

enriquefynn May 21, 2021 •

edited

Loading

enriquefynn May 21, 2021 •

edited

Loading