`claim_staking_rewards` extrinsic #2080

shannonwells · 2024-07-15T22:52:09Z

Goal

The goal of this PR is to implement the claim_staking_rewards extrinsic, its benchmarks, and tests.

Closes #1970

Discussion

Also removes a trait function that was complicating things and didn't really belong there.

Checklist

Design doc(s) updated
Tests added
Benchmarks added
Weights updated

…l refactor

shannonwells · 2024-07-15T23:28:25Z

pallets/capacity/src/tests/change_staking_target_tests.rs

@@ -40,7 +40,7 @@ fn do_retarget_happy_path() {
 		let from_amount = 600u64;
 		let to_amount = 300u64;
 		let to_msa: MessageSourceId = 2;
-		let staking_type = ProviderBoost;
+		let staking_type = StakingType::ProviderBoost;


This was about silencing linter warnings.

shannonwells · 2024-07-16T23:41:41Z

pallets/capacity/src/benchmarking.rs

-			assert_ok!(new_chunk.try_insert(era.into(), (1000u32 * era).into()));
-		}
-		ProviderBoostRewardPools::<T>::set(i, Some(new_chunk));
+fn fill_reward_pool_chunks<T: Config>(current_era: RewardEra) {


The previous version of this function, while it worked for the provider_boost benchmark, did not correctly fill up the reward_pool_chunks, so it caused an EraOutOfRange error in the claim_staking_reward benchmark.

shannonwells · 2024-07-16T23:42:54Z

pallets/capacity/src/lib.rs

@@ -402,6 +410,10 @@ pub mod pallet {
 		MaxRetargetsExceeded,
 		/// Tried to exceed bounds of a some Bounded collection
 		CollectionBoundExceeded,
+		/// This origin has nothing staked for ProviderBoost.
+		NotAProviderBoostAccount,


NotAStakingAccount was getting a bit overloaded.

shannonwells · 2024-07-16T23:50:02Z

common/primitives/src/capacity.rs

+	/// When this reward expires, i.e. can no longer be claimed
+	pub expires_at_block: BlockNumber,
+	/// The total staked in this era as of the current block
+	pub staked_amount: Balance,


It makes sense to have the amount they staked in each Reward Info.

shannonwells · 2024-07-16T23:53:07Z

pallets/capacity/src/types.rs

-		// have to save this because calling self.count() after self.0.get_mut produces compiler error
-		let current_count = self.count();
+
+		let current_staking_amount = self.get_last_staking_amount();


some code golf.

shannonwells · 2024-07-16T23:53:34Z

runtime/common/src/fee.rs

@@ -77,7 +77,7 @@ mod test {

 		assert_eq!(p, 100_000);

-		assert!(q >= 65_000_000);
+		assert!(q >= 50_000_000);


pulled in from main b/c benchmarks were failing due to too low weight.

shannonwells · 2024-07-17T17:00:47Z

pallets/capacity/src/weights.rs

+		// Proof Size summary in bytes:
+		//  Measured:  `1592`
+		//  Estimated: `20591`
+		// Minimum execution time: 90_000_000 picoseconds.


This is worst case scenario. Claiming rewards may be able to have some tuning, but also the benchmark should include variation. Created issue #2082 to address this

aramikm

Overall looks good, just some nits and questions!

common/primitives/src/capacity.rs

pallets/capacity/src/lib.rs

aramikm

Read through the code and looked good to me!

enddynayn · 2024-07-18T14:11:14Z

pallets/capacity/src/lib.rs

+			);
+			let total_to_mint = Self::do_claim_rewards(&staker)?;
+			Self::deposit_event(Event::ProviderBoostRewardClaimed {
+				account: staker.clone(),


Can you please check if the clone can be removed from here? It seems at first sight that we might not need to do a clone.

enddynayn · 2024-07-18T14:16:13Z

pallets/capacity/src/lib.rs

@@ -1024,37 +1053,42 @@ impl<T: Config> Pallet<T> {
 			Some(provider_boost_history) => {
 				match provider_boost_history.count() {
 					0usize => false,
-					// they staked before the current era, so they have unclaimed rewards.
-					1usize => provider_boost_history.get_entry_for_era(&current_era).is_none(),
+					1usize => {


I think removing a match statement makes the code a bit more readable:

pub(crate) fn has_unclaimed_rewards(account: &T::AccountId) -> bool { let current_era = Self::get_current_era().era_index; if let Some(provider_boost_history) = Self::get_staking_history_for(account) { let entry_count = provider_boost_history.count(); match entry_count { 0 => false, 1 => { let previous_era = current_era.saturating_sub(1); provider_boost_history.get_entry_for_era(&previous_era).is_none() && provider_boost_history.get_entry_for_era(&current_era).is_none() }, _ => true } } else { false } }

enddynayn · 2024-07-18T14:18:55Z

pallets/capacity/src/lib.rs

+			.fold(zero_balance, |acc, reward_info| acc.saturating_add(reward_info.earned_amount))
+			.into();
+		ensure!(total_to_mint.gt(&Zero::zero()), Error::<T>::NothingToClaim);
+		let _minted_unused = T::Currency::mint_into(&staker, total_to_mint)?;


Do we need let _minted_unused if we are not using it?

enddynayn

👍 amazing!

The goal of this PR is to implement the claim_staking_rewards extrinsic, its benchmarks, and tests. Closes #1970

claim_staking_rewards extrinsic + tests plus UnclaimedRewardInfo smal…

cb36af5

…l refactor

shannonwells changed the base branch from main to feat/capacity-staking-rewards-impl July 15, 2024 23:26

shannonwells commented Jul 15, 2024

View reviewed changes

claim_staking_rewards benchmark + updated weights.rs

c299b37

shannonwells commented Jul 16, 2024

View reviewed changes

shannonwells added 2 commits July 16, 2024 17:06

cleanup for PR

bf7f004

weights file

8c89ce5

shannonwells commented Jul 17, 2024

View reviewed changes

shannonwells marked this pull request as ready for review July 17, 2024 17:02

shannonwells requested a review from wilwade as a code owner July 17, 2024 17:02

shannonwells requested review from a team, mattheworris, enddynayn, aramikm, claireolmstead and JoeCap08055 and removed request for a team July 17, 2024 20:01

aramikm reviewed Jul 17, 2024

View reviewed changes

common/primitives/src/capacity.rs Outdated Show resolved Hide resolved

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

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

shannonwells added 2 commits July 17, 2024 14:17

correct benchmark

57b22f1

address PR comments

c5d713b

shannonwells requested a review from aramikm July 17, 2024 23:12

aramikm approved these changes Jul 17, 2024

View reviewed changes

enddynayn reviewed Jul 18, 2024

View reviewed changes

enddynayn approved these changes Jul 18, 2024

View reviewed changes

shannonwells merged commit ad63363 into feat/capacity-staking-rewards-impl Jul 18, 2024

shannonwells deleted the extrinsic-claim-rewards branch July 18, 2024 16:42

This was referenced Jul 18, 2024

Implement claim_staking_reward extrinsic #1571

Closed

Integrate economic model into StakingRewardsProvider trait implementation #1747

Closed

shannonwells added a commit that referenced this pull request Jul 18, 2024

claim_staking_rewards extrinsic (#2080)

ec2b7ac

The goal of this PR is to implement the claim_staking_rewards extrinsic, its benchmarks, and tests. Closes #1970

shannonwells added the Provider Boosting label Jul 19, 2024

shannonwells added a commit that referenced this pull request Jul 23, 2024

claim_staking_rewards extrinsic (#2080)

6ed6217

The goal of this PR is to implement the claim_staking_rewards extrinsic, its benchmarks, and tests. Closes #1970

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`claim_staking_rewards` extrinsic #2080

`claim_staking_rewards` extrinsic #2080

shannonwells commented Jul 15, 2024 •

edited

Loading

shannonwells Jul 15, 2024

shannonwells Jul 16, 2024

shannonwells Jul 16, 2024

shannonwells Jul 16, 2024

shannonwells Jul 16, 2024

shannonwells Jul 16, 2024

shannonwells Jul 17, 2024 •

edited

Loading

aramikm left a comment

aramikm left a comment

enddynayn Jul 18, 2024 •

edited

Loading

enddynayn Jul 18, 2024 •

edited

Loading

enddynayn Jul 18, 2024

enddynayn left a comment

claim_staking_rewards extrinsic #2080

claim_staking_rewards extrinsic #2080

Conversation

shannonwells commented Jul 15, 2024 • edited Loading

Goal

Discussion

Checklist

shannonwells Jul 15, 2024

Choose a reason for hiding this comment

shannonwells Jul 16, 2024

Choose a reason for hiding this comment

shannonwells Jul 16, 2024

Choose a reason for hiding this comment

shannonwells Jul 16, 2024

Choose a reason for hiding this comment

shannonwells Jul 16, 2024

Choose a reason for hiding this comment

shannonwells Jul 16, 2024

Choose a reason for hiding this comment

shannonwells Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

aramikm left a comment

Choose a reason for hiding this comment

aramikm left a comment

Choose a reason for hiding this comment

enddynayn Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

enddynayn Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

enddynayn Jul 18, 2024

Choose a reason for hiding this comment

enddynayn left a comment

Choose a reason for hiding this comment

`claim_staking_rewards` extrinsic #2080

`claim_staking_rewards` extrinsic #2080

shannonwells commented Jul 15, 2024 •

edited

Loading

shannonwells Jul 17, 2024 •

edited

Loading

enddynayn Jul 18, 2024 •

edited

Loading

enddynayn Jul 18, 2024 •

edited

Loading