The goal of this PR is to implement list_unclaimed_rewards, and also one that is lighter weight, has_unclaimed_rewards, which returns a bool and which unstake extrinsic uses. Unstake now fails if there are any unclaimed rewards.
Update: put back to draft since I realized I need to update the unstake benchmark for worst case data storage. Ok to review otherwise.
Closes #1969
Closes #1578
Discussion
Two issues
The smaller issue, originally in the design doc, one could not unstake without claiming all rewards. Now I am questioning whether we should require that, but I can still see reasons to support it, if only as a nudge to make sure people get all they're due.
With that said, I think the lightweight has_unclaimed_rewards ("You've Got Unclaimed Rewards!") would be very useful to providers, and we should make a custom RPC out of it.
I looked at different ways of making this more efficient in terms of storage reads/writes.
The current way stores Reward Pool history as a StorageMap by RewardEra. This means on getting a list unclaimed rewards or actually claiming the rewards - if there are any - it's possible that all the past RewardPoolInfos will be accessed up to the history limit (31). In addition there are 4 more reads needed. The size of the entire pool history is about 12k bits since it's 3 u128 (in production) 31 items. If someone boosted and then left it for the limit of RewardEras without claiming anything, then checked for unclaimed or claimed rewards, the number of reads would be the maximum.
However, at every boost and unstake, only a single RewardPoolInfo record is updated.
If to save only the number of read/writes for checking and claiming rewards, the entire Reward Pool history is stored as a single record, that is a lot of data to read and write on every boost/unstake. This is also a big lift when a new Reward Era begins in on_initialize, because instead of 2 smaller records (the new one and the oldest one) the entire data set would also be read/written.
With the current Provider Boost economics, the Reward Pool amount is a constant, so total_reward_pool isn't needed. Also, nobody has asked for any feature for the unclaimed_rewards field. That field was put in the the design last year, well before the economics model was decided and there had to be some guesswork about it.
I think the middle ground solution is, RewardPool storage should be changed in another PR to have only the one value of total_staked, but keep it as key/value StorageMap instead of changing it to a StorageValue for a BoundedVec, i.e. the entire Reward Pool history.
Goal
The goal of this PR is to implement
list_unclaimed_rewards, and also one that is lighter weight,has_unclaimed_rewards, which returns abooland whichunstakeextrinsic uses. Unstake now fails if there are any unclaimed rewards.Update: put back to draft since I realized I need to update the unstake benchmark for worst case data storage. Ok to review otherwise.
Closes #1969 Closes #1578
Discussion
Two issues
The smaller issue, originally in the design doc, one could not unstake without claiming all rewards. Now I am questioning whether we should require that, but I can still see reasons to support it, if only as a nudge to make sure people get all they're due.
With that said, I think the lightweight
has_unclaimed_rewards("You've Got Unclaimed Rewards!") would be very useful to providers, and we should make a custom RPC out of it.I looked at different ways of making this more efficient in terms of storage reads/writes.
The current way stores Reward Pool history as a
StorageMapbyRewardEra. This means on getting a list unclaimed rewards or actually claiming the rewards - if there are any - it's possible that all the past RewardPoolInfos will be accessed up to the history limit (31). In addition there are 4 more reads needed. The size of the entire pool history is about 12k bits since it's 3 u128 (in production) 31 items. If someone boosted and then left it for the limit ofRewardEraswithout claiming anything, then checked for unclaimed or claimed rewards, the number of reads would be the maximum.However, at every boost and unstake, only a single
RewardPoolInforecord is updated.If to save only the number of read/writes for checking and claiming rewards, the entire Reward Pool history is stored as a single record, that is a lot of data to read and write on every boost/unstake. This is also a big lift when a new Reward Era begins in
on_initialize, because instead of 2 smaller records (the new one and the oldest one) the entire data set would also be read/written.With the current Provider Boost economics, the Reward Pool amount is a constant, so
total_reward_poolisn't needed. Also, nobody has asked for any feature for theunclaimed_rewardsfield. That field was put in the the design last year, well before the economics model was decided and there had to be some guesswork about it.I think the middle ground solution is, RewardPool storage should be changed in another PR to have only the one value of
total_staked, but keep it as key/valueStorageMapinstead of changing it to aStorageValuefor aBoundedVec, i.e. the entire Reward Pool history.Checklist