nomination-pools: re-bonding unclaimed rewards.

[Szegoo]

Currently, it is possible to bond the funds from the rewards pool, but it is not done in the cleanest way. As mentioned in #11671 the rewards are first transferred to the account, and then transferred to the bonded account.

This PR fixes the unnecessary transfer from the reward pool to the account that is made here. Instead, the rewards are now directly bonded to the bonded account.

I will check if there are some docs that need to be updated when I am assured that the try_bond_funds_from_rewards is correct. I don't believe that additional tests should be added, because the bond_extra is already tested, and this PR doesn't change the results of the call to bond_extra.

Closes #11671

Polkadot address: 126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA

[Szegoo]

I checked, and there are no additional docs that need to be changed. This PR is ready for review.

cc @kianenigma

[kianenigma]

Looks overall in the right direction, but should we wait until #11669 is merged? that one is critical and will cause some changes to this as well.

[Szegoo]

Looks overall in the right direction, but should we wait until #11669 is merged? that one is critical and will cause some changes to this as well.

Yeah, we should probably wait so that we don't cause some conflicts with #11669

[Szegoo]

@kianenigma I have adapted the code so that it works now correctly with the changes made in #11669

[Szegoo]

Needs tests + less duplciate code.

Not quite sure what the new tests would be good for. Isn't this already tested in the bond_extra tests?

[Szegoo]

@ggwpez @kianenigma review?

[Szegoo]

@kianenigma I forgot to add tests, but I have added them now, could you please review this?

[kianenigma]

@Szegoo this is how I would go about doing this. I still think your approach is a bit over complicated:

diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs
index 09f1746b8e5..63b69cc9562 100644
--- a/frame/nomination-pools/src/lib.rs
+++ b/frame/nomination-pools/src/lib.rs
@@ -857,21 +857,20 @@ 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.
 	///
 	/// Returns `Ok(points_issues)`, `Err` otherwise.
 	fn try_bond_funds(
 		&mut self,
-		who: &T::AccountId,
+		from: &T::AccountId,
 		amount: BalanceOf<T>,
 		ty: BondType,
 	) -> Result<BalanceOf<T>, DispatchError> {
 		// Cache the value
 		let bonded_account = self.bonded_account();
 		T::Currency::transfer(
-			&who,
+			&from,
 			&bonded_account,
 			amount,
 			match ty {
@@ -1551,18 +1550,28 @@ 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 =
-				Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?;
+
+			// transfer the payouts either to the member account or the bonded account.
+			let bonded_account = bonded_pool.bonded_account();
+			let claimed = Self::do_reward_payout(
+				match extra {
+					BondExtra::FreeBalance(_) => &who,
+					BondExtra::Rewards => &bonded_account,
+				},
+				&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),
-				BondExtra::Rewards =>
-					(bonded_pool.try_bond_funds(&who, claimed, BondType::Later)?, claimed),
+				BondExtra::Rewards => (
+					// NOTE: this will trigger a transfer from `bonded_account` to itself under the
+					// hood, which is totally harmless.
+					bonded_pool.try_bond_funds(&bonded_account, claimed, BondType::Later)?,
+					claimed,
+				),
 			};
 
 			bonded_pool.ok_to_be_open(bonded)?;
@@ -2302,10 +2311,11 @@ impl<T: Config> Pallet<T> {
 	}
 
 	/// 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.
+	///
+	/// 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,
+		transfer_recipient: &T::AccountId,
 		member: &mut PoolMember<T>,
 		bonded_pool: &mut BondedPool<T>,
 		reward_pool: &mut RewardPool<T>,
@@ -2330,13 +2340,13 @@ impl<T: Config> Pallet<T> {
 		// Transfer payout to the member.
 		T::Currency::transfer(
 			&bonded_pool.reward_account(),
-			&member_account,
+			transfer_recipient,
 			pending_rewards,
 			ExistenceRequirement::AllowDeath,
 		)?;
 
 		Self::deposit_event(Event::<T>::PaidOut {
-			member: member_account.clone(),
+			member: transfer_recipient.clone(),
 			pool_id: member.pool_id,
 			payout: pending_rewards,
 		});

But even with my approach, I am starting to think that fixing this is not really worth the fix. Transferring things back to the user account, and taking it out again in the same block is slightly more inefficient, but it is simplifying the code a lot, and sadly I only came to this conclusion by seeing the RP. Also, note that as long as only a member can trigger a payout for themselves, their balances is already read in that block (to pay the fees), so it is a very cheap operation.

I am curious to know what you think. I am still not certain yet, but for now my opinion is leaning toward closing this PR.

[Szegoo]

@kianenigma Yes, my approach does add complexity to the code. Your approach seems simpler but still makes the code a bit more complex. And also if we implement this we should probably add a new transfer_recipient field to the PaidOut event because it isn't implied anymore that the receiver is the member_account.

Maybe I could try implementing your suggestion, but I am not sure how much would that make the code more efficient, and if it would be worthy of the added complexity.

EDIT: Actually, after implementing your solution locally, I believe that it is not adding too much complexity and it doesn't make the code confusing, so it may be worth doing this. It may even be confusing for someone new reading the code that we are not paying out to the bonded_account directly, but that we are firstly moving the rewards to the member_account.

I am interested to hear your final decision.

[kianenigma]

I am still of the opinion that this is not needed at the time being and I rather keep the current working code in-place and re-visit the issue later. I will close the PR, keep the issue open for the sake of posterity. Thanks for helping us explore the opportunities here, even though the PR is not merged, the outcome is valuable.

[kianenigma]

/tip small

[substrate-tip-bot[bot]]

@kianenigma A small tip was successfully submitted for Szegoo (126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips