nomination-pools: re-bonding unclaimed rewards.

frame/nomination-pools/src/lib.rs

@@ -358,6 +358,16 @@ enum AccountType {
 	Reward,
 }
 
+/// Indicates whether the the member account or the bonded accound is going
+/// to receive the payout.
+#[derive(Encode, Decode)]
+enum PayoutRecipient<T: Config> {
+	/// Stores the `AccountId` of the member account.
+	MemberAccount(T::AccountId),
+	/// Stores the `AccountId` of the bonded account.
+	BondedAccount(T::AccountId),
+}
+
 /// A member in a pool.
 #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, CloneNoBound)]
 #[cfg_attr(feature = "std", derive(frame_support::PartialEqNoBound, DefaultNoBound))]
@@ -857,28 +867,36 @@ impl<T: Config> BondedPool<T> {
 
 	/// Bond exactly `amount` from `who`'s funds into this pool.
 	///
-	/// If the bond type is `Create`, `StakingInterface::bond` is called, and `who`
-	/// is allowed to be killed. Otherwise, `StakingInterface::bond_extra` is called and `who`
-	/// cannot be killed.
+	/// If the bond type is `Create`, `StakingInterface::bond` is called, and
+	/// `who` is allowed to be killed. Otherwise, `StakingInterface::bond_extra`
+	/// is called and `who` cannot be killed.
+	///
+	/// The `bond_from_member` argument is a flag that indicates whether the
+	/// `amount` should be transfered from `who`, or the `bonded_account`
+	/// already has the `amount`, but just needs to bond it.
 	///
 	/// Returns `Ok(points_issues)`, `Err` otherwise.
 	fn try_bond_funds(
 		&mut self,
 		who: &T::AccountId,
 		amount: BalanceOf<T>,
 		ty: BondType,
+		bond_from_member: bool,
 	) -> Result<BalanceOf<T>, DispatchError> {
 		// Cache the value
 		let bonded_account = self.bonded_account();
-		T::Currency::transfer(
-			&who,
-			&bonded_account,
-			amount,
-			match ty {
-				BondType::Create => ExistenceRequirement::AllowDeath,
-				BondType::Later => ExistenceRequirement::KeepAlive,
-			},
-		)?;
+
+		if bond_from_member {
+			T::Currency::transfer(
+				&who,
+				&bonded_account,
+				amount,
+				match ty {
+					BondType::Create => ExistenceRequirement::AllowDeath,
+					BondType::Later => ExistenceRequirement::KeepAlive,
+				},
+			)?;
+		}
 		// We must calculate the points issued *before* we bond who's funds, else points:balance
 		// ratio will be wrong.
 		let points_issued = self.issue(amount);
@@ -899,6 +917,35 @@ impl<T: Config> BondedPool<T> {
 		Ok(points_issued)
 	}
 
+	/// Bond all of the pending_rewards from `member` into this pool.
+	///
+	/// Transfers the member's rewards directly to the bonded account of the
+	/// member. Makes a call to the `StakingInterface::bond_extra` to bond the
+	/// newly claimed rewards.
+	///
+	/// Emits the `PaidOut` event.
+	///
+	/// Returns `Ok((points_issues, bonded))`, `Err` otherwise.
+	fn try_bond_funds_from_rewards(
+		&mut self,
+		member: &mut PoolMember<T>,
+		reward_pool: &mut RewardPool<T>,
+	) -> Result<(BalanceOf<T>, BalanceOf<T>), DispatchError> {
+		let bonded_account = self.bonded_account();
+
+		let pending_rewards = Pallet::<T>::calculate_and_payout_rewards(
+			member,
+			self,
+			reward_pool,
+			PayoutRecipient::<T>::BondedAccount(bonded_account.clone()),
+		)?;
+
+		let points_issued =
+			self.try_bond_funds(&bonded_account, pending_rewards, BondType::Later, false)?;
+
+		Ok((points_issued, pending_rewards))
+	}
+
 	// Set the state of `self`, and deposit an event if the state changed. State should never be set
 	// directly in in order to ensure a state change event is always correctly deposited.
 	fn set_state(&mut self, state: PoolState) {
@@ -1503,7 +1550,7 @@ pub mod pallet {
 			reward_pool.update_records(pool_id, bonded_pool.points)?;
 
 			bonded_pool.try_inc_members()?;
-			let points_issued = bonded_pool.try_bond_funds(&who, amount, BondType::Later)?;
+			let points_issued = bonded_pool.try_bond_funds(&who, amount, BondType::Later, true)?;
 
 			PoolMembers::insert(
 				who.clone(),
@@ -1532,13 +1579,13 @@ pub mod pallet {
 
 		/// Bond `extra` more funds from `origin` into the pool to which they already belong.
 		///
-		/// Additional funds can come from either the free balance of the account, of from the
+		/// Additional funds can come from either the free balance of the account, or from the
 		/// accumulated rewards, see [`BondExtra`].
 		///
 		/// Bonding extra funds implies an automatic payout of all pending rewards as well.
 		// NOTE: this transaction is implemented with the sole purpose of readability and
 		// correctness, not optimization. We read/write several storage items multiple times instead
-		// of just once, in the spirit reusing code.
+		// of just once, in the spirit of reusing code.
 		#[pallet::weight(
 			T::WeightInfo::bond_extra_transfer()
 			.max(T::WeightInfo::bond_extra_reward())
@@ -1551,18 +1598,17 @@ pub mod pallet {
 			// payout related stuff: we must claim the payouts, and updated recorded payout data
 			// before updating the bonded pool points, similar to that of `join` transaction.
 			reward_pool.update_records(bonded_pool.id, bonded_pool.points)?;
-			// TODO: optimize this to not touch the free balance of `who ` at all in benchmarks.
-			// Currently, bonding rewards is like a batch. In the same PR, also make this function
-			// take a boolean argument that make it either 100% pure (no storage update), or make it
-			// also emit event and do the transfer. #11671
-			let claimed =
+
+			// we have to claim the pending_rewards every time we change the bonded amount.
+			if matches!(extra, BondExtra::FreeBalance(_)) {
 				Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?;
+			}
 
 			let (points_issued, bonded) = match extra {
 				BondExtra::FreeBalance(amount) =>
-					(bonded_pool.try_bond_funds(&who, amount, BondType::Later)?, amount),
+					(bonded_pool.try_bond_funds(&who, amount, BondType::Later, true)?, amount),
 				BondExtra::Rewards =>
-					(bonded_pool.try_bond_funds(&who, claimed, BondType::Later)?, claimed),
+					bonded_pool.try_bond_funds_from_rewards(&mut member, &mut reward_pool)?,
 			};
 
 			bonded_pool.ok_to_be_open(bonded)?;
@@ -1580,7 +1626,7 @@ pub mod pallet {
 		}
 
 		/// A bonded member can use this to claim their payout based on the rewards that the pool
-		/// has accumulated since their last claimed payout (OR since joining if this is there first
+		/// has accumulated since their last claimed payout (OR since joining if this is their first
 		/// time claiming rewards). The payout will be transferred to the member's account.
 		///
 		/// The member will earn rewards pro rata based on the members stake vs the sum of the
@@ -1892,7 +1938,7 @@ pub mod pallet {
 			);
 
 			bonded_pool.try_inc_members()?;
-			let points = bonded_pool.try_bond_funds(&who, amount, BondType::Create)?;
+			let points = bonded_pool.try_bond_funds(&who, amount, BondType::Create, true)?;
 
 			T::Currency::transfer(
 				&who,
@@ -2301,24 +2347,64 @@ impl<T: Config> Pallet<T> {
 			.div(current_points)
 	}
 
-	/// If the member has some rewards, transfer a payout from the reward pool to the member.
-	// Emits events and potentially modifies pool state if any arithmetic saturates, but does
-	// not persist any of the mutable inputs to storage.
-	fn do_reward_payout(
-		member_account: &T::AccountId,
+	/// Calculates the rewards for the given member, and makes a payout to the
+	/// defined `payout_recipient` account.
+	///
+	/// Returns the amount of pending_rewards that the member has accumulated,
+	/// that are now transfered to the `payout_recipient`.
+	fn calculate_and_payout_rewards(
 		member: &mut PoolMember<T>,
 		bonded_pool: &mut BondedPool<T>,
 		reward_pool: &mut RewardPool<T>,
+		payout_recipient: PayoutRecipient<T>,
 	) -> Result<BalanceOf<T>, DispatchError> {
-		debug_assert_eq!(member.pool_id, bonded_pool.id);
-
-		// a member who has no skin in the game anymore cannot claim any rewards.
-		ensure!(!member.active_points().is_zero(), Error::<T>::FullyUnbonding);
+		let (pending_rewards, current_reward_counter) =
+			Self::calculate_rewards(member, bonded_pool, reward_pool)?;
+		Self::payout_rewards(
+			member,
+			bonded_pool,
+			reward_pool,
+			pending_rewards,
+			current_reward_counter,
+			payout_recipient,
+		)?;
+		Ok(pending_rewards)
+	}
 
+	/// Only calculates the rewards for the given member without changing any
+	/// data.
+	///
+	/// Returns the pending rewards of the member and the
+	/// `current_reward_counter` of the reward pool
+	fn calculate_rewards(
+		member: &PoolMember<T>,
+		bonded_pool: &BondedPool<T>,
+		reward_pool: &RewardPool<T>,
+	) -> Result<(BalanceOf<T>, T::RewardCounter), DispatchError> {
 		let current_reward_counter =
 			reward_pool.current_reward_counter(bonded_pool.id, bonded_pool.points)?;
 		let pending_rewards = member.pending_rewards(current_reward_counter)?;
 
+		Ok((pending_rewards, current_reward_counter))
+	}
+
+	/// Does the actual payout of the accumulated rewards by the member.
+	///
+	/// Returns the amount of pending rewards that got paid out.
+	/// Emits the `PaidOut` event.
+	fn payout_rewards(
+		member: &mut PoolMember<T>,
+		bonded_pool: &mut BondedPool<T>,
+		reward_pool: &mut RewardPool<T>,
+		pending_rewards: BalanceOf<T>,
+		current_reward_counter: T::RewardCounter,
+		payout_recipient: PayoutRecipient<T>,
+	) -> Result<BalanceOf<T>, DispatchError> {
+		debug_assert_eq!(member.pool_id, bonded_pool.id);
+
+		// a member who has no skin in the game anymore cannot claim any rewards.
+		ensure!(!member.active_points().is_zero(), Error::<T>::FullyUnbonding);
+
 		if pending_rewards.is_zero() {
 			return Ok(pending_rewards)
 		}
@@ -2327,23 +2413,45 @@ impl<T: Config> Pallet<T> {
 		member.last_recorded_reward_counter = current_reward_counter;
 		reward_pool.register_claimed_reward(pending_rewards);
 
-		// Transfer payout to the member.
+		let recipient = match payout_recipient {
+			PayoutRecipient::<T>::MemberAccount(member_account) => member_account,
+			PayoutRecipient::<T>::BondedAccount(bonded_account) => bonded_account,
+		};
+
+		// Transfer payout to the recipient.
 		T::Currency::transfer(
 			&bonded_pool.reward_account(),
-			&member_account,
+			&recipient,
 			pending_rewards,
 			ExistenceRequirement::AllowDeath,
 		)?;
 
 		Self::deposit_event(Event::<T>::PaidOut {
-			member: member_account.clone(),
+			member: recipient.clone(),
 			pool_id: member.pool_id,
 			payout: pending_rewards,
 		});
 
 		Ok(pending_rewards)
 	}
 
+	/// If the member has some rewards, transfer a payout from the reward pool to the member.
+	// Emits events and potentially modifies pool state if any arithmetic saturates, but does
+	// not persist any of the mutable inputs to storage.
+	fn do_reward_payout(
+		member_account: &T::AccountId,
+		member: &mut PoolMember<T>,
+		bonded_pool: &mut BondedPool<T>,
+		reward_pool: &mut RewardPool<T>,
+	) -> Result<BalanceOf<T>, DispatchError> {
+		Self::calculate_and_payout_rewards(
+			member,
+			bonded_pool,
+			reward_pool,
+			PayoutRecipient::<T>::MemberAccount(member_account.clone()),
+		)
+	}
+
 	/// Ensure the correctness of the state of this pallet.
 	///
 	/// This should be valid before or after each state transition of this pallet.

frame/nomination-pools/src/tests.rs

@@ -1481,7 +1481,7 @@ mod claim_payout {
 			// and 20 bonds more -- they should not have more share of this reward.
 			assert_ok!(Pools::bond_extra(Origin::signed(20), BondExtra::FreeBalance(10)));
 
-			// everyone claim.
+			// everyone claims.
 			assert_ok!(Pools::claim_payout(Origin::signed(10)));
 			assert_ok!(Pools::claim_payout(Origin::signed(20)));
 
@@ -4164,7 +4164,11 @@ mod bond_extra {
 				vec![
 					Event::Created { depositor: 10, pool_id: 1 },
 					Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
-					Event::PaidOut { member: 10, pool_id: 1, payout: claimable_reward },
+					Event::PaidOut {
+						member: default_bonded_account(),
+						pool_id: 1,
+						payout: claimable_reward
+					},
 					Event::Bonded {
 						member: 10,
 						pool_id: 1,
@@ -4184,7 +4188,7 @@ mod bond_extra {
 			Balances::make_free_balance_be(&default_reward_account(), 8);
 			// ... if which only 3 is claimable to make sure the reward account does not die.
 			let claimable_reward = 8 - ExistentialDeposit::get();
-			// NOTE: easier to read of we use 3, so let's use the number instead of variable.
+			// NOTE: easier to read if we use 3, so let's use the number instead of variable.
 			assert_eq!(claimable_reward, 3, "test is correct if rewards are divisible by 3");
 
 			// given
@@ -4219,9 +4223,9 @@ mod bond_extra {
 					Event::Created { depositor: 10, pool_id: 1 },
 					Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
 					Event::Bonded { member: 20, pool_id: 1, bonded: 20, joined: true },
-					Event::PaidOut { member: 10, pool_id: 1, payout: 1 },
+					Event::PaidOut { member: default_bonded_account(), pool_id: 1, payout: 1 },
 					Event::Bonded { member: 10, pool_id: 1, bonded: 1, joined: false },
-					Event::PaidOut { member: 20, pool_id: 1, payout: 2 },
+					Event::PaidOut { member: default_bonded_account(), pool_id: 1, payout: 2 },
 					Event::Bonded { member: 20, pool_id: 1, bonded: 2, joined: false }
 				]
 			);