nomination-pools: re-bonding unclaimed rewards. #11767

Szegoo · 2022-07-02T12:57:13Z

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 · 2022-07-05T08:46:28Z

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

cc @kianenigma

frame/nomination-pools/src/lib.rs

kianenigma · 2022-07-06T11:10:58Z

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 · 2022-07-06T11:28:25Z

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

…/substrate into refactorred-extra-bonding

Szegoo · 2022-07-16T10:43:04Z

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

frame/nomination-pools/src/lib.rs

kianenigma

Needs tests + less duplciate code.

Szegoo · 2022-07-23T11:35:57Z

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?

kianenigma · 2022-07-26T13:14:30Z

frame/nomination-pools/src/lib.rs

-			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)?;


What we need to avoid all this duplicate code is:

break down the current do_reward_payout to calculate_and_payout_rewards.

internally, this is composed of two functions: calculate_rewards and payout_rewards.

In here, if it is BondExtra::Rewards, only calculate, and use the same try_bond_funds.

If it is BondExtra::FreeBalance, use the existing calculate_and_payout_rewards.

Thanks for specifying this to me, I have cleaned up the code, so could you check if the changes I made are reasonable?

Szegoo · 2022-07-27T11:56:10Z

frame/nomination-pools/src/tests.rs

@@ -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 },


The field member inside the PaidOut should probably be changed to recipient, because the receiver of the payout, doesn't have to be the member account anymore. @kianenigma Do you agree with this? Also, should we keep the member field, and make the recipient a new field?

@kianenigma I have kept the member field that stores the member account and I made a new recipient field in the PaidOut event. Could you please take a look at the code to check if this makes sense?

Szegoo · 2022-08-02T11:19:01Z

@ggwpez @kianenigma review?

Szegoo · 2022-08-07T15:06:49Z

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

kianenigma · 2022-08-16T07:50:55Z

frame/nomination-pools/src/lib.rs

+/// Indicates whether the the member account or the bonded accound is going
+/// to receive the payout.
+#[derive(Encode, Decode)]
+pub enum PayoutRecipient<T: Config> {


Doesn't look like this needs to be public.

kianenigma · 2022-08-16T07:55:01Z

frame/nomination-pools/src/lib.rs

@@ -358,6 +358,17 @@ enum AccountType {
 	Reward,
 }

+/// Indicates whether the the member account or the bonded accound is going
+/// to receive the payout.
+#[derive(Encode, Decode)]


are you sure this is needed?

kianenigma · 2022-08-16T07:58:04Z

frame/nomination-pools/src/lib.rs

+	/// Stores the `AccountId` of the member account.
+	MemberAccount(T::AccountId),
+	/// Stores the `AccountId` of the bonded account and the member account.
+	/// (member_account, bonded_account)
+	BondedAccount(T::AccountId, T::AccountId),


Suggested change

/// Stores the `AccountId` of the member account.

MemberAccount(T::AccountId),

/// Stores the `AccountId` of the bonded account and the member account.

/// (member_account, bonded_account)

BondedAccount(T::AccountId, T::AccountId),

/// Pending rewards should go to the member.

MemberAccount(T::AccountId),

/// Pending rewards should become bonded.

BondedAccount(T::AccountId, T::AccountId),

kianenigma · 2022-08-16T07:59:05Z

frame/nomination-pools/src/lib.rs

-	/// 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


The correct line wrapping is 100, please avoid wrapping lines too short as well.

kianenigma · 2022-08-16T08:20:37Z

@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 · 2022-08-16T08:47:20Z

@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 · 2022-08-19T09:48:24Z

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 · 2022-08-19T09:48:29Z

/tip small

substrate-tip-bot · 2022-08-19T09:48:34Z

@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

extra bonding refactorred

b61a765

Merge branch 'paritytech:master' into refactorred-extra-bonding

9ac37d9

kianenigma reviewed Jul 6, 2022

View reviewed changes

frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved

Szegoo and others added 6 commits July 6, 2022 13:30

ty is always Later

514ee3c

Merge branch 'refactorred-extra-bonding' of https://github.com/Szegoo…

5ff050d

…/substrate into refactorred-extra-bonding

fmt

34d090b

Merge branch 'paritytech:master' into refactorred-extra-bonding

82ede4a

Merge branch 'master' into refactorred-extra-bonding

ef50ccf

update for the new np reward scheme

ac1a516

kianenigma reviewed Jul 23, 2022

View reviewed changes

frame/nomination-pools/src/lib.rs Show resolved Hide resolved

kianenigma reviewed Jul 23, 2022

View reviewed changes

frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved

kianenigma reviewed Jul 23, 2022

View reviewed changes

Szegoo added 2 commits July 23, 2022 14:06

update comments

319fb18

small fix in comment

75dacb0

kianenigma reviewed Jul 26, 2022

View reviewed changes

Szegoo and others added 2 commits July 27, 2022 12:35

Merge branch 'paritytech:master' into refactorred-extra-bonding

3619884

remove duplications

93bb5b0

Szegoo commented Jul 27, 2022

View reviewed changes

Szegoo and others added 2 commits August 5, 2022 14:07

Merge branch 'paritytech:master' into refactorred-extra-bonding

c2d9028

test calculate_rewards

adacec8

Szegoo added 2 commits August 7, 2022 16:31

test try_bond_funds_from_rewards

312d3da

test payout_rewards

d0c845a

Szegoo requested a review from kianenigma August 8, 2022 05:51

Szegoo added 2 commits August 11, 2022 08:56

modified the PaidOut event

4a2a64c

update test

9edba42

kianenigma reviewed Aug 16, 2022

View reviewed changes

remove unnecessary

ef27da8

kianenigma closed this Aug 19, 2022

kianenigma mentioned this pull request Aug 19, 2022

nomination-pools: optimize re-bonding unclaimed rewards. #11671

Closed

Szegoo mentioned this pull request Oct 7, 2022

Member Request polkadot-fellows/seeding#36

Merged

louismerlin removed the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Oct 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nomination-pools: re-bonding unclaimed rewards. #11767

nomination-pools: re-bonding unclaimed rewards. #11767

Szegoo commented Jul 2, 2022

Szegoo commented Jul 5, 2022

kianenigma commented Jul 6, 2022

Szegoo commented Jul 6, 2022

Szegoo commented Jul 16, 2022

kianenigma left a comment

Szegoo commented Jul 23, 2022

kianenigma Jul 26, 2022

Szegoo Jul 27, 2022 •

edited

Loading

Szegoo Jul 27, 2022 •

edited

Loading

Szegoo Aug 11, 2022

Szegoo commented Aug 2, 2022 •

edited

Loading

Szegoo commented Aug 7, 2022

kianenigma Aug 16, 2022

kianenigma Aug 16, 2022

kianenigma Aug 16, 2022

kianenigma Aug 16, 2022

kianenigma commented Aug 16, 2022

Szegoo commented Aug 16, 2022 •

edited

Loading

kianenigma commented Aug 19, 2022

kianenigma commented Aug 19, 2022

substrate-tip-bot bot commented Aug 19, 2022

nomination-pools: re-bonding unclaimed rewards. #11767

nomination-pools: re-bonding unclaimed rewards. #11767

Conversation

Szegoo commented Jul 2, 2022

Szegoo commented Jul 5, 2022

kianenigma commented Jul 6, 2022

Szegoo commented Jul 6, 2022

Szegoo commented Jul 16, 2022

kianenigma left a comment

Choose a reason for hiding this comment

Szegoo commented Jul 23, 2022

kianenigma Jul 26, 2022

Choose a reason for hiding this comment

Szegoo Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Szegoo Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Szegoo Aug 11, 2022

Choose a reason for hiding this comment

Szegoo commented Aug 2, 2022 • edited Loading

Szegoo commented Aug 7, 2022

kianenigma Aug 16, 2022

Choose a reason for hiding this comment

kianenigma Aug 16, 2022

Choose a reason for hiding this comment

kianenigma Aug 16, 2022

Choose a reason for hiding this comment

kianenigma Aug 16, 2022

Choose a reason for hiding this comment

kianenigma commented Aug 16, 2022

Szegoo commented Aug 16, 2022 • edited Loading

kianenigma commented Aug 19, 2022

kianenigma commented Aug 19, 2022

substrate-tip-bot bot commented Aug 19, 2022

Szegoo Jul 27, 2022 •

edited

Loading

Szegoo Jul 27, 2022 •

edited

Loading

Szegoo commented Aug 2, 2022 •

edited

Loading

Szegoo commented Aug 16, 2022 •

edited

Loading