Sanity checks on initialization and prevention of claiming money if reward vec not fully initialized #10
Conversation
…iming if reward vec is not initialized and make some associated types constants
girazoki
changed the title
…imed by now vs how much it already claimed
… the vesting period
src/lib.rs
Outdated
| &PALLET_ID.into_account(), | ||
| &payee, | ||
| payable_amount, | ||
| KeepAlive, |
There was a problem hiding this comment.
I believe KeepAlive will not allow the transfer to go through if it would take the account below the existential deposit. In this case, the ideal scenario is that every single reward token will be claimed and paid out. So if we require KeepAlive the last claim will not succeed because it would have killed the pallet's pot account.
There was a problem hiding this comment.
I see, I missread it then. Will change it
src/lib.rs
Outdated
|
|
||
| let pay_period_as_balance: BalanceOf<T> = payable_period | ||
| .saturated_into::<u128>() | ||
| .try_into() |
There was a problem hiding this comment.
I'm not sure I understand the need for this conversation.
To know the should_have_claimed, we need:
payable_period (number of blocks) / vesting_period (number of blocks) * (info.total_reward (balance) - first_paid (balance))
There was a problem hiding this comment.
The main reason is that I need all these parameters to have the same type to do the arithmetic computation. And I cannot divide, then make the conversion, then multiply (the division would already be truncated). I might be able to do this in a more elegant way, let me take a closer look, but this was the most straight-forward way I found
There was a problem hiding this comment.
I agree that converting a number of blocks to a balance is a bit odd. My inclination would be to convert everything to an integer (e.g. u128) when they need a common type, but that might be my C++ background talking :)
There was a problem hiding this comment.
I tried minimizing conversions but I can do that too definitely
There was a problem hiding this comment.
I ended up using relay_chain::BlockNumber from cumulus_primitives_core. I think this is what makes most sense as we are relying on this type when we count vested blocks. This type is a u32 and convertible automatically to Balance thanks to the atLeast32BitsUnsigned trait. Let me know if this looks better
|
|
||
| // If the period is bigger than whats missing to pay, then return whats missing to pay | ||
| let payable_amount = if should_have_claimed >= info.total_reward { | ||
| info.total_reward.saturating_sub(info.claimed_reward) |
There was a problem hiding this comment.
This case is supposed to be when the entire amount is vested but we haven't claimed it all, right? Is this going to be slightly off because we subtract first_paid above?
There was a problem hiding this comment.
Right, I need to substract first_paid from the comparison. Good catch! I will also add a test for that ( I have one already but I am gonna make it more precise)
| let payable_amount = if should_have_claimed >= info.total_reward { | ||
| info.total_reward.saturating_sub(info.claimed_reward) | ||
| } else { | ||
| should_have_claimed + first_paid - info.claimed_reward |
There was a problem hiding this comment.
Why do we add back in first_paid here?
There was a problem hiding this comment.
Because claimed_reward also contains first_paid.
src/lib.rs
Outdated
| /// The total vesting period. Ideally this should be less than the lease period to ensure | ||
| /// there is no overlap between contributors from two different auctions |
There was a problem hiding this comment.
Outdated comment. There is no notion of different auctions
src/lib.rs
Outdated
| ensure!( | ||
| info.free_claim_done == false, | ||
| Error::<T>::FirstClaimAlreadyDone | ||
| initialized == true, |
There was a problem hiding this comment.
| initialized == true, | |
| initialized, |
This PR adds:
Closes #9