Feat/check unclaimed rewards 1969 #1972

shannonwells · 2024-05-16T00:41:20Z

Goal

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.

Checklist

Feature implemented
Tests added
Weights updated

shannonwells · 2024-05-16T00:51:03Z

pallets/capacity/src/tests/mock.rs

@@ -208,7 +208,7 @@ impl pallet_capacity::Config for Test {
 	type CapacityPerToken = TestCapacityPerToken;
 	type RewardEra = TestRewardEra;
 	type EraLength = ConstU32<10>;
-	type StakingRewardsPastErasMax = ConstU32<5>;
+	type StakingRewardsPastErasMax = ConstU32<6>; // 5 for claiming rewards, 1 for current reward era


This change was needed, and it broke some other tests.

shannonwells · 2024-05-16T00:51:41Z

pallets/capacity/src/tests/mock.rs

@@ -235,7 +235,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
 	let mut ext = sp_io::TestExternalities::new(t);
 	ext.execute_with(|| {
 		System::set_block_number(1);
-		set_era_and_reward_pool_at_block(1, 0, 0);
+		set_era_and_reward_pool(1, 1, 0);


it made more sense to start at era 1 and have that start at block 1 instead of block 0, because block number is also 1. This broke different tests.

shannonwells · 2024-05-16T00:53:10Z

pallets/capacity/src/types.rs

@@ -361,11 +362,33 @@ impl<T: Config> ProviderBoostHistory<T> {
 		Some(self.count())
 	}

-	/// Return how much is staked for the given era.  If there is no entry for that era, return None.
-	pub fn get_staking_amount_for_era(&self, reward_era: &T::RewardEra) -> Option<&BalanceOf<T>> {
+	/// A wrapper for the key/value retrieval of the BoundedBTreeMap.


better name, better explanation.

aramikm

Some comments and questions!

pallets/capacity/src/lib.rs

pallets/capacity/src/types.rs

wilwade

Some quick thoughts

pallets/capacity/src/lib.rs

wilwade · 2024-05-21T20:28:46Z

pallets/capacity/src/lib.rs

-		InsufficientStakingAmount,
-		/// Staker is attempting to stake a zero amount.
+		StakingAmountBelowMinimum,
+		/// Staker is attempting to stake a zero amount.  DEPRECATED


It is still used by ensure_can_stake, but you can tag it with a rust deprecated attribute as well

Suggested change

/// Staker is attempting to stake a zero amount. DEPRECATED

/// Staker is attempting to stake a zero amount.

/// #[deprecated(since = "1.13.0", note = "Use ?? instead")]

wilwade

I'm a bit lost in what is new and what is from main, but I think I looked at it all. I think it is good to merge once the conflicts are resolved.

wilwade · 2024-05-21T21:33:33Z

pallets/capacity/src/lib.rs

+					current_era_info.era_index.saturating_sub(past_eras_max.into()).add(One::one());
+				StakingRewardPool::<T>::remove(earliest_era); // 1w
+			}
+			CurrentEraInfo::<T>::set(new_era_info); // 1w


Not for this PR, but this line feels like it should go up by the creation of the new_era_info. I kept trying to figure out why it mattered about past_eras_max

I'll move it anyway since I'm adding the deprecation line.

enddynayn · 2024-05-21T22:28:11Z

pallets/README.template.md

+
+| Name      | Description         | Query                    | Runtime Added |
+| --------- | ------------------- | ------------------------ | ------------- |
+| {Query 1} | {Query Desctiption} | `{queryCallInCamelCase}` | 1             |


Desctiption -> Description

this wasn't my change, sorry, it was from conflicting stuff from main.

@enddynayn I'll fix it in my upcoming docs PR

enddynayn · 2024-05-21T22:32:05Z

pallets/capacity/src/types.rs

 		Some(())
 	}

 	/// Decrease an MSA target Staking total and Capacity amount.
-	pub fn withdraw(&mut self, amount: Balance, capacity: Balance) {
+	/// If the amount would put you below the minimum, zero out the amount.


…o old position for backward compatibility * FIX: Implement eligible reward amounts as originally designed, only for amounts staked the entire era. * Better moments

…f the account boosted earlier than the first era in the range. * On an unstake, Use upsert_boost_history and update reward pool in the same place, which is where the unstaking balance is decreased, not the capacity. * Rerun capacity benchmarks

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. Closes #1969 Closes #1578

shannonwells force-pushed the feat/check-unclaimed-rewards-1969 branch from 148705a to 75cc0ff Compare May 16, 2024 00:49

shannonwells commented May 16, 2024

View reviewed changes

shannonwells marked this pull request as ready for review May 16, 2024 23:24

shannonwells requested a review from wilwade as a code owner May 16, 2024 23:24

shannonwells marked this pull request as draft May 17, 2024 19:43

aramikm reviewed May 17, 2024

View reviewed changes

pallets/capacity/src/lib.rs Show resolved Hide resolved

pallets/capacity/src/types.rs Show resolved Hide resolved

wilwade reviewed May 20, 2024

View reviewed changes

pallets/capacity/src/lib.rs Show resolved Hide resolved

pallets/capacity/src/lib.rs Show resolved Hide resolved

shannonwells marked this pull request as ready for review May 21, 2024 18:28

shannonwells force-pushed the feat/capacity-staking-rewards-impl branch from 1699361 to 5a0ebce Compare May 21, 2024 18:44

wilwade reviewed May 21, 2024

View reviewed changes

wilwade approved these changes May 21, 2024

View reviewed changes

enddynayn reviewed May 21, 2024

View reviewed changes

shannonwells added 9 commits May 21, 2024 16:28

get_amount_staked_for_era working w/ tests

af5ecc5

WIP debugging in test

356c62f

tests passing

7a97e06

passing tests with unstake failing if unclaimed rewards

050446f

best time-boxed effort

0af1059

a rename and regenerate weights

0876646

fix a benchmark, and fix a bug.

42766c4

* FIX: Move enum from main back to old po* Move enum from main back t…

8ca9758

…o old position for backward compatibility * FIX: Implement eligible reward amounts as originally designed, only for amounts staked the entire era. * Better moments

shannonwells force-pushed the feat/check-unclaimed-rewards-1969 branch from 23dcbd7 to 3187a83 Compare May 21, 2024 23:29

address PR comments

d08853a

shannonwells merged commit aeaec44 into feat/capacity-staking-rewards-impl May 21, 2024

shannonwells deleted the feat/check-unclaimed-rewards-1969 branch May 21, 2024 23:46

shannonwells mentioned this pull request May 21, 2024

Better optimize storage for StakingRewardPool #1976

Closed

5 tasks

shannonwells mentioned this pull request Jul 10, 2024

claim staking rewards 1747 | will be split up into multiple PRs #2065

Closed

3 tasks

shannonwells added the Provider Boosting label Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/check unclaimed rewards 1969 #1972

Feat/check unclaimed rewards 1969 #1972

shannonwells commented May 16, 2024 •

edited

Loading

shannonwells May 16, 2024

shannonwells May 16, 2024 •

edited

Loading

shannonwells May 16, 2024

aramikm left a comment

wilwade left a comment

wilwade May 21, 2024

wilwade left a comment

wilwade May 21, 2024

shannonwells May 21, 2024

enddynayn May 21, 2024

shannonwells May 21, 2024

wilwade May 22, 2024

enddynayn May 21, 2024

Feat/check unclaimed rewards 1969 #1972

Feat/check unclaimed rewards 1969 #1972

Conversation

shannonwells commented May 16, 2024 • edited Loading

Goal

Discussion

Checklist

Choose a reason for hiding this comment

shannonwells May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aramikm left a comment

Choose a reason for hiding this comment

wilwade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilwade 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

Choose a reason for hiding this comment

shannonwells commented May 16, 2024 •

edited

Loading

shannonwells May 16, 2024 •

edited

Loading