[NPoS] Fix for Reward Deficit in the pool

substrate/frame/nomination-pools/src/lib.rs

@@ -1300,13 +1300,22 @@ impl<T: Config> RewardPool<T> {
 		self.total_commission_pending =
 			self.total_commission_pending.saturating_add(new_pending_commission);
 
-		// Store the total payouts at the time of this update. Total payouts are essentially the
-		// entire historical balance of the reward pool, equating to the current balance + the total
-		// rewards that have left the pool + the total commission that has left the pool.
-		self.last_recorded_total_payouts = balance
+		// Total payouts are essentially the entire historical balance of the reward pool, equating
+		// to the current balance + the total rewards that have left the pool + the total commission
+		// that has left the pool.
+		let last_recorded_total_payouts = balance
 			.checked_add(&self.total_rewards_claimed.saturating_add(self.total_commission_claimed))
 			.ok_or(Error::<T>::OverflowRisk)?;
 
+		// Store the total payouts at the time of this update.
+		//
+		// An increase in ED could cause `last_recorded_total_payouts` to decrease but we should not
+		// allow that to happen since an already paid out reward cannot decrease. The reward account
+		// might go in deficit temporarily in this exceptional case but it will be corrected once
+		// new rewards are added to the pool.
+		self.last_recorded_total_payouts =
+			self.last_recorded_total_payouts.max(last_recorded_total_payouts);
+
 		Ok(())
 	}
 
@@ -1487,6 +1496,7 @@ impl<T: Config> SubPools<T> {
 /// `no_era` pool. This is guaranteed to at least be equal to the staking `UnbondingDuration`. For
 /// improved UX [`Config::PostUnbondingPoolsWindow`] should be configured to a non-zero value.
 pub struct TotalUnbondingPools<T: Config>(PhantomData<T>);
+
 impl<T: Config> Get<u32> for TotalUnbondingPools<T> {
 	fn get() -> u32 {
 		// NOTE: this may be dangerous in the scenario bonding_duration gets decreased because
@@ -1770,6 +1780,8 @@ pub mod pallet {
 		},
 		/// Pool commission has been claimed.
 		PoolCommissionClaimed { pool_id: PoolId, commission: BalanceOf<T> },
+		/// Pool topped up in case of a reward deficit.
+		PoolToppedUp { pool_id: PoolId, top_up_value: BalanceOf<T>, deficit: BalanceOf<T> },
 	}
 
 	#[pallet::error]
@@ -1845,6 +1857,8 @@ pub mod pallet {
 		InvalidPoolId,
 		/// Bonding extra is restricted to the exact pending reward amount.
 		BondExtraRestricted,
+		/// No reward deficit to top up.
+		NoRewardDeficit,
 	}
 
 	#[derive(Encode, Decode, PartialEq, TypeInfo, PalletError, RuntimeDebug)]
@@ -2631,6 +2645,24 @@ pub mod pallet {
 			let who = ensure_signed(origin)?;
 			Self::do_claim_commission(who, pool_id)
 		}
+
+		/// Top up the reward deficit of a pool permissionlessly.
+		///
+		/// This can happen in situations where ED has increased from the time the pool was created.
+		/// The increased ED eats up from the available rewards of the pool, and the pool can end up
+		/// with a net deficit.
+		#[pallet::call_index(21)]
+		// FIXME(ank4n): bench + tests
+		#[pallet::weight(T::WeightInfo::claim_commission())]
+		pub fn top_up_reward_deficit(
+			origin: OriginFor<T>,
+			pool_id: PoolId,
+			#[pallet::compact] max_transfer: BalanceOf<T>,
+		) -> DispatchResult {
+			let who = ensure_signed(origin)?;
+			let caller_balance = T::Currency::free_balance(&who);
+			Self::do_top_up_reward_deficit(who, pool_id, max_transfer.min(caller_balance))
+		}
 	}
 
 	#[pallet::hooks]
@@ -3029,6 +3061,62 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
+	fn pool_pending_rewards(pool: PoolId) -> Result<BalanceOf<T>, sp_runtime::DispatchError> {
+		let bonded_pool = BondedPools::<T>::get(pool).ok_or(Error::<T>::PoolNotFound)?;
+		let reward_pool = RewardPools::<T>::get(pool).ok_or(Error::<T>::PoolNotFound)?;
+
+		let current_rc = if !bonded_pool.points.is_zero() {
+			let commission = bonded_pool.commission.current();
+			reward_pool.current_reward_counter(pool, bonded_pool.points, commission)?.0
+		} else {
+			Default::default()
+		};
+
+		Ok(PoolMembers::<T>::iter()
+			.filter(|(_, d)| d.pool_id == pool)
+			.map(|(_, d)| d.pending_rewards(current_rc).unwrap_or_default())
+			.fold(0u32.into(), |acc: BalanceOf<T>, x| acc.saturating_add(x)))
+	}
+
+	fn do_top_up_reward_deficit(
+		who: T::AccountId,
+		pool: PoolId,
+		max_transfer: BalanceOf<T>,
+	) -> DispatchResult {
+		let pool_pending_rewards = Self::pool_pending_rewards(pool)?;
+		let reward_balance = RewardPool::<T>::current_balance(pool);
+		let deficit = pool_pending_rewards.saturating_sub(reward_balance);
+
+		ensure!(!deficit.is_zero(), Error::<T>::NoRewardDeficit);
+
+		// do not top up beyond the max transfer amount desired by the caller.
+		let top_up_amount = deficit.min(max_transfer);
+		T::Currency::transfer(
+			&who,
+			&Self::create_reward_account(pool),
+			top_up_amount,
+			ExistenceRequirement::KeepAlive,
+		)?;
+
+		// The topped up amount should not be claimable by delegators.
+		RewardPools::<T>::mutate(pool, |maybe_reward_pool| {
+			if let Some(pool) = maybe_reward_pool {
+				pool.last_recorded_total_payouts.saturating_accrue(top_up_amount);
+				Ok(())
+			} else {
+				Err(Error::<T>::PoolNotFound)
+			}
+		})?;
+
+		Self::deposit_event(Event::<T>::PoolToppedUp {
+			pool_id: pool,
+			top_up_value: top_up_amount,
+			deficit: deficit.saturating_sub(top_up_amount),
+		});
+
+		Ok(())
+	}
+
 	/// Ensure the correctness of the state of this pallet.
 	///
 	/// This should be valid before or after each state transition of this pallet.

substrate/frame/nomination-pools/src/mock.rs

@@ -423,6 +423,45 @@ pub fn fully_unbond_permissioned(member: AccountId) -> DispatchResult {
 	Pools::unbond(RuntimeOrigin::signed(member), member, points)
 }
 
+pub fn pending_rewards_for_pool(pool: PoolId) -> Balance {
+	Pools::pool_pending_rewards(pool).expect("pool should exist")
+}
+
+pub fn pending_rewards_for_delegator(delegator: AccountId) -> Balance {
+	let member = PoolMembers::<T>::get(delegator).unwrap();
+	let bonded_pool = BondedPools::<T>::get(member.pool_id).unwrap();
+	let reward_pool = RewardPools::<T>::get(member.pool_id).unwrap();
+
+	assert!(!bonded_pool.points.is_zero());
+
+	let commission = bonded_pool.commission.current();
+	let current_rc = reward_pool
+		.current_reward_counter(member.pool_id, bonded_pool.points, commission)
+		.unwrap()
+		.0;
+
+	member.pending_rewards(current_rc).unwrap_or_default()
+}
+
+#[derive(PartialEq, Debug)]
+pub enum RewardImbalance {
+	// There is no reward deficit.
+	Surplus(Balance),
+	// There is a reward deficit.
+	Deficit(Balance),
+}
+
+pub fn reward_imbalance(pool: PoolId) -> RewardImbalance {
+	let pending_rewards = pending_rewards_for_pool(pool);
+	let current_balance = RewardPool::<Runtime>::current_balance(pool);
+
+	if pending_rewards > current_balance {
+		RewardImbalance::Deficit(pending_rewards - current_balance)
+	} else {
+		RewardImbalance::Surplus(current_balance - pending_rewards)
+	}
+}
+
 #[cfg(test)]
 mod test {
 	use super::*;

substrate/frame/nomination-pools/src/tests.rs

@@ -298,10 +298,11 @@ mod bonded_pool {
 }
 
 mod reward_pool {
+	use super::*;
+	use crate::mock::RewardImbalance::{Deficit, Surplus};
+
 	#[test]
 	fn current_balance_only_counts_balance_over_existential_deposit() {
-		use super::*;
-
 		ExtBuilder::default().build_and_execute(|| {
 			let reward_account = Pools::create_reward_account(2);
 
@@ -324,6 +325,158 @@ mod reward_pool {
 			assert_eq!(RewardPool::<Runtime>::current_balance(2), 1);
 		});
 	}
+
+	#[test]
+	fn ed_change_causes_reward_deficit() {
+		ExtBuilder::default().max_members_per_pool(Some(5)).build_and_execute(|| {
+			// original ED
+			ExistentialDeposit::set(5);
+
+			// 11 joins the pool
+			Balances::make_free_balance_be(&11, 500);
+			assert_ok!(Pools::join(RuntimeOrigin::signed(11), 90, 1));
+
+			// new delegator does not have any pending rewards
+			assert_eq!(pending_rewards_for_delegator(11), 0);
+
+			// give the pool some rewards
+			deposit_rewards(100);
+
+			// all existing delegator has pending rewards
+			assert_eq!(pending_rewards_for_delegator(11), 90);
+			assert_eq!(pending_rewards_for_delegator(10), 10);
+			assert_eq!(reward_imbalance(1), Surplus(0));
+
+			// 12 joins the pool.
+			Balances::make_free_balance_be(&12, 500);
+			assert_ok!(Pools::join(RuntimeOrigin::signed(12), 100, 1));
+
+			// Current reward balance is committed to last recorded reward counter of
+			// the pool before the increase in ED.
+			let bonded_pool = BondedPools::<Runtime>::get(1).unwrap();
+			let reward_pool = RewardPools::<Runtime>::get(1).unwrap();
+			assert_eq!(
+				reward_pool.last_recorded_reward_counter,
+				reward_pool
+					.current_reward_counter(1, bonded_pool.points, Perbill::zero())
+					.unwrap()
+					.0
+			);
+
+			// reward pool before ED increase and reward counter getting committed.
+			let reward_pool_1 = RewardPools::<Runtime>::get(1).unwrap();
+
+			// increase ED from 5 to 50
+			ExistentialDeposit::set(50);
+
+			// There is now an expected deficit of ed_diff
+			assert_eq!(reward_imbalance(1), Deficit(45));
+
+			// 13 joins the pool which commits the reward counter to reward pool.
+			Balances::make_free_balance_be(&13, 500);
+			assert_ok!(Pools::join(RuntimeOrigin::signed(13), 100, 1));
+
+			// still a deficit
+			assert_eq!(reward_imbalance(1), Deficit(45));
+
+			// reward pool after ED increase
+			let reward_pool_2 = RewardPools::<Runtime>::get(1).unwrap();
+
+			// last recorded total payout does not decrease even as ED increases.
+			assert_eq!(
+				reward_pool_1.last_recorded_total_payouts,
+				reward_pool_2.last_recorded_total_payouts
+			);
+
+			// Topping up pool decreases deficit
+			deposit_rewards(10);
+			assert_eq!(reward_imbalance(1), Deficit(35));
+
+			// top up the pool to remove the deficit
+			deposit_rewards(35);
+			// No deficit anymore
+			assert_eq!(reward_imbalance(1), Surplus(0));
+		});
+	}
+
+	#[test]
+	fn top_up_fixes_reward_deficit() {
+		ExtBuilder::default().max_members_per_pool(Some(5)).build_and_execute(|| {
+			// Given: pool has a reward deficit
+
+			// original ED
+			ExistentialDeposit::set(5);
+
+			// 11 joins the pool
+			Balances::make_free_balance_be(&11, 500);
+			assert_ok!(Pools::join(RuntimeOrigin::signed(11), 90, 1));
+
+			// Pool some rewards
+			deposit_rewards(100);
+
+			// 12 joins the pool.
+			Balances::make_free_balance_be(&12, 500);
+			assert_ok!(Pools::join(RuntimeOrigin::signed(12), 10, 1));
+
+			// When: pool ends up in reward deficit
+
+			// increase ED
+			ExistentialDeposit::set(50);
+			assert_eq!(reward_imbalance(1), Deficit(45));
+
+			// clear events
+			pool_events_since_last_call();
+
+			// Then: top up reduces the deficit
+			Balances::make_free_balance_be(&99, 1000);
+			// caller can set safe ceiling for top up.
+			let max_top_up: Balance = 20;
+			assert_ok!(Pools::top_up_reward_deficit(RuntimeOrigin::signed(99), 1, max_top_up));
+			// only upto max_transfer is topped up.
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![Event::PoolToppedUp {
+					pool_id: 1,
+					top_up_value: max_top_up,
+					deficit: 45 - max_top_up
+				},]
+			);
+			assert_eq!(reward_imbalance(1), Deficit(25));
+
+			// Top up the remaining deficit
+			assert_ok!(Pools::top_up_reward_deficit(RuntimeOrigin::signed(99), 1, 1000));
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![Event::PoolToppedUp { pool_id: 1, top_up_value: 25, deficit: 0 },]
+			);
+			assert_eq!(reward_imbalance(1), Surplus(0));
+
+			// Trying to top up again does not work
+			assert_err!(
+				Pools::top_up_reward_deficit(RuntimeOrigin::signed(99), 1, 1000),
+				Error::<T>::NoRewardDeficit
+			);
+		});
+	}
+
+	#[test]
+	fn topping_up_does_not_work_for_pools_with_no_deficit() {
+		ExtBuilder::default().max_members_per_pool(Some(5)).build_and_execute(|| {
+			// 11 joins the pool
+			Balances::make_free_balance_be(&11, 500);
+			assert_ok!(Pools::join(RuntimeOrigin::signed(11), 90, 1));
+
+			// Pool some rewards
+			deposit_rewards(100);
+			assert_eq!(reward_imbalance(1), Surplus(0));
+
+			// Topping up fails
+			assert_err!(
+				Pools::top_up_reward_deficit(RuntimeOrigin::signed(11), 1, 100),
+				Error::<T>::NoRewardDeficit
+			);
+		});
+	}
 }
 
 mod unbond_pool {