fix: suggestions from @ntn-x2 review

pallets/parachain-staking/src/benchmarking.rs

@@ -590,7 +590,7 @@ benchmarks! {
 		assert_eq!(pallet_balances::Pallet::<T>::usable_balance(&beneficiary), (amount + amount).into());
 	}
 
-	execute_pending_reward_change {
+	exectue_scheduled_reward_change {
 		// we need at least 1 collators
 		let n in 0 .. T::MaxTopCandidates::get();
 		// we need at least 1 delegator

pallets/parachain-staking/src/default_weights.rs

@@ -69,7 +69,7 @@ pub trait WeightInfo {
 	fn increment_delegator_rewards() -> Weight;
 	fn increment_collator_rewards(_m: u32, ) -> Weight;
 	fn claim_rewards_for() -> Weight;
-	fn execute_pending_reward_change(n: u32, m: u32, ) -> Weight;
+	fn exectue_scheduled_reward_change(n: u32, m: u32, ) -> Weight;
 }
 
 /// Weights for parachain_staking using the Substrate node and recommended hardware.
@@ -421,7 +421,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	// Storage: ParachainStaking CounterForCandidatePool (r:1 w:0)
 	/// The range of component `n` is `[0, 75]`.
 	/// The range of component `m` is `[0, 35]`.
-	fn execute_pending_reward_change(n: u32, m: u32, ) -> Weight {
+	fn exectue_scheduled_reward_change(n: u32, m: u32, ) -> Weight {
 		(0 as Weight)
 			// Standard Error: 5_730_000
 			.saturating_add((202_623_000 as Weight).saturating_mul(n as Weight))
@@ -782,7 +782,7 @@ impl WeightInfo for () {
 	// Storage: ParachainStaking CounterForCandidatePool (r:1 w:0)
 	/// The range of component `n` is `[0, 75]`.
 	/// The range of component `m` is `[0, 35]`.
-	fn execute_pending_reward_change(n: u32, m: u32, ) -> Weight {
+	fn exectue_scheduled_reward_change(n: u32, m: u32, ) -> Weight {
 		(0 as Weight)
 			// Standard Error: 5_730_000
 			.saturating_add((202_623_000 as Weight).saturating_mul(n as Weight))

pallets/parachain-staking/src/lib.rs

@@ -647,10 +647,17 @@ pub mod pallet {
 	#[pallet::getter(fn last_reward_reduction)]
 	pub(crate) type LastRewardReduction<T: Config> = StorageValue<_, T::BlockNumber, ValueQuery>;
 
-	/// The counter of accumulated rewards for a collator candidate.
+	/// The counter of accumulated rewards for an account.
 	///
-	/// It maps from collator accounts to the number of block authored by them
-	/// since the last payout.
+	/// For collators, it reflects the number of authored blocks since the last
+	/// reward raise. Thus, everytime a collator authors a block, the
+	/// counter is increased. It is reset, when the collator increments their
+	/// rewards.
+	///
+	/// For delegators, it is used to determine the difference between the
+	/// delegator and corresponding collator when incrementing the delegator's
+	/// rewards. In this case, the counter is never incremented but reset to the
+	/// collator one when the delegator reward increment happens.
 	// TODO: Maybe rather use u64. Assuming 30 validators, u32 would suffice for 27 years of constant 12s blocktime.
 	#[pallet::storage]
 	#[pallet::getter(fn reward_count)]
@@ -1778,8 +1785,8 @@ pub mod pallet {
 		/// before adjusting the inflation.
 		///
 		/// Emits `RoundInflationSet`.
-		#[pallet::weight(<T as Config>::WeightInfo::execute_pending_reward_change(T::MaxTopCandidates::get(), T::MaxDelegatorsPerCollator::get()))]
-		pub fn execute_pending_reward_change(origin: OriginFor<T>) -> DispatchResultWithPostInfo {
+		#[pallet::weight(<T as Config>::WeightInfo::exectue_scheduled_reward_change(T::MaxTopCandidates::get(), T::MaxDelegatorsPerCollator::get()))]
+		pub fn exectue_scheduled_reward_change(origin: OriginFor<T>) -> DispatchResultWithPostInfo {
 			ensure_signed(origin)?;
 
 			let now = frame_system::Pallet::<T>::block_number();
@@ -1827,7 +1834,7 @@ pub mod pallet {
 				new_inflation.delegator.reward_rate.per_block,
 			));
 
-			Ok(Some(<T as Config>::WeightInfo::execute_pending_reward_change(
+			Ok(Some(<T as Config>::WeightInfo::exectue_scheduled_reward_change(
 				CandidatePool::<T>::count(),
 				max_num_delegators.saturated_into(),
 			))
@@ -2469,6 +2476,8 @@ pub mod pallet {
 				Rewards::<T>::mutate(acc, |reward| {
 					*reward = reward.saturating_add(Self::calc_block_rewards_delegator(stake, diff.into()));
 				});
+				// align with collator counter such that incrementing would
+				// lead to 0 rewards until the collator counter increases
 				RewardCount::<T>::insert(acc, col_reward_count);
 
 				// 4 reads from reward calc

pallets/parachain-staking/src/mock.rs

@@ -355,6 +355,15 @@ pub(crate) fn almost_equal(left: Balance, right: Balance, precision: Perbill) ->
 	left.max(right) - left.min(right) <= err
 }
 
+/// Incrementelly traverses from the current block to the provided one and
+/// potentially sets a block authors.
+///
+/// If for a block `i` the corresponding index of the authors input is set, this
+/// account is regarded to be the block author and thus gets noted.
+///
+/// NOTE: At most, this updates the RewardCount of the block author but does not
+/// increment rewards or claim them. Please use `roll_to_claim_rewards` in that
+/// case.
 pub(crate) fn roll_to(n: BlockNumber, authors: Vec<Option<AccountId>>) {
 	while System::block_number() < n {
 		if let Some(Some(author)) = authors.get((System::block_number()) as usize) {
@@ -367,6 +376,14 @@ pub(crate) fn roll_to(n: BlockNumber, authors: Vec<Option<AccountId>>) {
 }
 
 #[allow(unused_must_use)]
+/// Incrementelly traverses from the current block to the provided one and
+/// potentially sets a block authors.
+///
+///  If existent, rewards of the block author and their delegators are
+/// incremented and claimed.
+///
+/// If for a block `i` the corresponding index of the authors input is set, this
+/// account is regarded to be the block author and thus gets noted.
 pub(crate) fn roll_to_claim_rewards(n: BlockNumber, authors: Vec<Option<AccountId>>) {
 	while System::block_number() < n {
 		if let Some(Some(author)) = authors.get((System::block_number()) as usize) {

pallets/parachain-staking/src/tests.rs

@@ -2757,7 +2757,7 @@ fn adjust_reward_rates() {
 			roll_to_claim_rewards(<Test as Config>::BLOCKS_PER_YEAR + 1, vec![]);
 			// reward reduction should not happen automatically anymore
 			assert_eq!(StakePallet::last_reward_reduction(), 0u64);
-			assert_ok!(StakePallet::execute_pending_reward_change(Origin::signed(1)));
+			assert_ok!(StakePallet::exectue_scheduled_reward_change(Origin::signed(1)));
 			assert_eq!(StakePallet::last_reward_reduction(), 1u64);
 			let inflation_1 = InflationInfo::new(
 				<Test as Config>::BLOCKS_PER_YEAR,
@@ -2788,7 +2788,7 @@ fn adjust_reward_rates() {
 			roll_to_claim_rewards(2 * <Test as Config>::BLOCKS_PER_YEAR + 1, vec![]);
 			// reward reduction should not happen automatically anymore
 			assert_eq!(StakePallet::last_reward_reduction(), 1u64);
-			assert_ok!(StakePallet::execute_pending_reward_change(Origin::signed(1)));
+			assert_ok!(StakePallet::exectue_scheduled_reward_change(Origin::signed(1)));
 			assert_eq!(StakePallet::last_reward_reduction(), 2u64);
 			let inflation_2 = InflationInfo::new(
 				<Test as Config>::BLOCKS_PER_YEAR,
@@ -4175,7 +4175,7 @@ fn rewards_yearly_inflation_adjustment() {
 			});
 
 			// execute to trigger reward increment
-			assert_ok!(StakePallet::execute_pending_reward_change(Origin::signed(1)));
+			assert_ok!(StakePallet::exectue_scheduled_reward_change(Origin::signed(1)));
 			(1..=5).for_each(|id| {
 				assert!(StakePallet::reward_count(id).is_zero(), "acc_id {:?}", id);
 				assert!(!StakePallet::rewards(id).is_zero(), "acc_id {:?}", id);

pallets/parachain-staking/src/types.rs

@@ -20,7 +20,7 @@ use frame_support::traits::{Currency, Get};
 use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
 use scale_info::TypeInfo;
 use sp_runtime::{
-	traits::{AtLeast32BitUnsigned, Saturating, Zero},
+	traits::{AtLeast32BitUnsigned, CheckedSub, Saturating, Zero},
 	RuntimeDebug,
 };
 use sp_staking::SessionIndex;
@@ -213,7 +213,7 @@ pub type Delegator<AccountId, Balance> = Stake<Option<AccountId>, Balance>;
 impl<AccountId, Balance> Delegator<AccountId, Balance>
 where
 	AccountId: Eq + Ord + Clone + Debug,
-	Balance: Copy + Add<Output = Balance> + Saturating + PartialOrd + Eq + Ord + Debug + Zero + Default,
+	Balance: Copy + Add<Output = Balance> + Saturating + PartialOrd + Eq + Ord + Debug + Zero + Default + CheckedSub,
 {
 	/// Adds a new delegation.
 	///
@@ -255,12 +255,10 @@ where
 	/// was not found and Ok(None) if delegated stake would underflow.
 	pub fn dec_delegation(&mut self, collator: AccountId, less: Balance) -> Result<Option<Balance>, ()> {
 		if self.owner == Some(collator) {
-			if self.amount > less {
-				self.amount = self.amount.saturating_sub(less);
-				Ok(Some(self.amount))
-			} else {
-				Ok(None)
-			}
+			Ok(self.amount.checked_sub(&less).map(|new| {
+				self.amount = new;
+				self.amount
+			}))
 		} else {
 			Err(())
 		}

runtimes/peregrine/src/weights/parachain_staking.rs

@@ -394,7 +394,7 @@ impl<T: frame_system::Config> parachain_staking::WeightInfo for WeightInfo<T> {
 	// Storage: ParachainStaking CounterForCandidatePool (r:1 w:0)
 	/// The range of component `n` is `[0, 75]`.
 	/// The range of component `m` is `[0, 35]`.
-	fn execute_pending_reward_change(n: u32, m: u32, ) -> Weight {
+	fn exectue_scheduled_reward_change(n: u32, m: u32, ) -> Weight {
 		(0 as Weight)
 			// Standard Error: 5_730_000
 			.saturating_add((202_623_000 as Weight).saturating_mul(n as Weight))

runtimes/spiritnet/src/weights/parachain_staking.rs

@@ -394,7 +394,7 @@ impl<T: frame_system::Config> parachain_staking::WeightInfo for WeightInfo<T> {
 	// Storage: ParachainStaking CounterForCandidatePool (r:1 w:0)
 	/// The range of component `n` is `[0, 75]`.
 	/// The range of component `m` is `[0, 35]`.
-	fn execute_pending_reward_change(n: u32, m: u32, ) -> Weight {
+	fn exectue_scheduled_reward_change(n: u32, m: u32, ) -> Weight {
 		(0 as Weight)
 			// Standard Error: 5_730_000
 			.saturating_add((202_623_000 as Weight).saturating_mul(n as Weight))