Pools: Add ability to configure commission claiming permissions (#2474)

Addresses #409.

This request has been raised by multiple community members - the ability
for the nomination pool root role to configure permissionless commission
claiming:

> Would it be possible to have a claim_commission_other extrinsic for
claiming commission of nomination pools permissionless?

This PR does not quite introduce this additional call, but amends
`do_claim_commission` to check a new `claim_permission` field in the
`Commission` struct, configured by an enum:

```
enum CommissionClaimPermission {
   Permissionless,
   Account(AccountId),
}
```
This can be optionally set in a bonded pool's
`commission.claim_permission` field:

```
struct BondedPool {
   commission: {
      <snip>
      claim_permission: Option<CommissionClaimPermission<T::AccountId>>,
   },
   <snip>
}
```

This is a new field and requires a migration to add it to existing
pools. This will be `None` on pool creation, falling back to the `root`
role having sole access to claim commission if it is not set; this is
the behaviour as it is today. Once set, the field _can_ be set to `None`
again.

#### Changes
- [x] Add `commision.claim_permission` field.
- [x] Add `can_claim_commission` and amend `do_claim_commission`.
- [x] Add `set_commission_claim_permission` call.
- [x] Test to cover new configs and call.
- [x] Add and amend benchmarks.
- [x] Generate new weights + slot into call
`set_commission_claim_permission`.
- [x] Add migration to introduce `commission.claim_permission`, bump
storage version.
- [x] Update Westend weights.
- [x] Migration working.

---------

Co-authored-by: command-bot <>

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

@@ -676,6 +676,13 @@ pub struct PoolRoles<AccountId> {
 	pub bouncer: Option<AccountId>,
 }
 
+// A pool's possible commission claiming permissions.
+#[derive(PartialEq, Eq, Copy, Clone, Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)]
+pub enum CommissionClaimPermission<AccountId> {
+	Permissionless,
+	Account(AccountId),
+}
+
 /// Pool commission.
 ///
 /// The pool `root` can set commission configuration after pool creation. By default, all commission
@@ -705,6 +712,9 @@ pub struct Commission<T: Config> {
 	/// The block from where throttling should be checked from. This value will be updated on all
 	/// commission updates and when setting an initial `change_rate`.
 	pub throttle_from: Option<BlockNumberFor<T>>,
+	// Whether commission can be claimed permissionlessly, or whether an account can claim
+	// commission. `Root` role can always claim.
+	pub claim_permission: Option<CommissionClaimPermission<T::AccountId>>,
 }
 
 impl<T: Config> Commission<T> {
@@ -1078,6 +1088,17 @@ impl<T: Config> BondedPool<T> {
 		self.is_root(who)
 	}
 
+	fn can_claim_commission(&self, who: &T::AccountId) -> bool {
+		if let Some(permission) = self.commission.claim_permission.as_ref() {
+			match permission {
+				CommissionClaimPermission::Permissionless => true,
+				CommissionClaimPermission::Account(account) => account == who || self.is_root(who),
+			}
+		} else {
+			self.is_root(who)
+		}
+	}
+
 	fn is_destroying(&self) -> bool {
 		matches!(self.state, PoolState::Destroying)
 	}
@@ -1572,7 +1593,7 @@ pub mod pallet {
 	use sp_runtime::Perbill;
 
 	/// The current storage version.
-	const STORAGE_VERSION: StorageVersion = StorageVersion::new(7);
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(8);
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
@@ -1850,6 +1871,11 @@ pub mod pallet {
 			pool_id: PoolId,
 			change_rate: CommissionChangeRate<BlockNumberFor<T>>,
 		},
+		/// Pool commission claim permission has been updated.
+		PoolCommissionClaimPermissionUpdated {
+			pool_id: PoolId,
+			permission: Option<CommissionClaimPermission<T::AccountId>>,
+		},
 		/// Pool commission has been claimed.
 		PoolCommissionClaimed { pool_id: PoolId, commission: BalanceOf<T> },
 		/// Topped up deficit in frozen ED of the reward pool.
@@ -2742,6 +2768,32 @@ pub mod pallet {
 			let who = ensure_signed(origin)?;
 			Self::do_adjust_pool_deposit(who, pool_id)
 		}
+
+		/// Set or remove a pool's commission claim permission.
+		///
+		/// Determines who can claim the pool's pending commission. Only the `Root` role of the pool
+		/// is able to conifigure commission claim permissions.
+		#[pallet::call_index(22)]
+		#[pallet::weight(T::WeightInfo::set_commission_claim_permission())]
+		pub fn set_commission_claim_permission(
+			origin: OriginFor<T>,
+			pool_id: PoolId,
+			permission: Option<CommissionClaimPermission<T::AccountId>>,
+		) -> DispatchResult {
+			let who = ensure_signed(origin)?;
+			let mut bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
+			ensure!(bonded_pool.can_manage_commission(&who), Error::<T>::DoesNotHavePermission);
+
+			bonded_pool.commission.claim_permission = permission.clone();
+			bonded_pool.put();
+
+			Self::deposit_event(Event::<T>::PoolCommissionClaimPermissionUpdated {
+				pool_id,
+				permission,
+			});
+
+			Ok(())
+		}
 	}
 
 	#[pallet::hooks]
@@ -3106,12 +3158,12 @@ impl<T: Config> Pallet<T> {
 
 	fn do_claim_commission(who: T::AccountId, pool_id: PoolId) -> DispatchResult {
 		let bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
-		ensure!(bonded_pool.can_manage_commission(&who), Error::<T>::DoesNotHavePermission);
+		ensure!(bonded_pool.can_claim_commission(&who), Error::<T>::DoesNotHavePermission);
 
 		let mut reward_pool = RewardPools::<T>::get(pool_id)
 			.defensive_ok_or::<Error<T>>(DefensiveError::RewardPoolNotFound.into())?;
 
-		// IMPORTANT: make sure that any newly pending commission not yet processed is added to
+		// IMPORTANT: ensure newly pending commission not yet processed is added to
 		// `total_commission_pending`.
 		reward_pool.update_records(
 			pool_id,

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

@@ -27,6 +27,15 @@ use sp_runtime::TryRuntimeError;
 pub mod versioned {
 	use super::*;
 
+	/// v8: Adds commission claim permissions to `BondedPools`.
+	pub type V7ToV8<T> = frame_support::migrations::VersionedMigration<
+		7,
+		8,
+		v8::VersionUncheckedMigrateV7ToV8<T>,
+		crate::pallet::Pallet<T>,
+		<T as frame_system::Config>::DbWeight,
+	>;
+
 	/// Migration V6 to V7 wrapped in a [`frame_support::migrations::VersionedMigration`], ensuring
 	/// the migration is only performed when on-chain version is 6.
 	pub type V6ToV7<T> = frame_support::migrations::VersionedMigration<
@@ -47,6 +56,74 @@ pub mod versioned {
 	>;
 }
 
+pub mod v8 {
+	use super::*;
+
+	#[derive(Decode)]
+	pub struct OldCommission<T: Config> {
+		pub current: Option<(Perbill, T::AccountId)>,
+		pub max: Option<Perbill>,
+		pub change_rate: Option<CommissionChangeRate<BlockNumberFor<T>>>,
+		pub throttle_from: Option<BlockNumberFor<T>>,
+	}
+
+	#[derive(Decode)]
+	pub struct OldBondedPoolInner<T: Config> {
+		pub commission: OldCommission<T>,
+		pub member_counter: u32,
+		pub points: BalanceOf<T>,
+		pub roles: PoolRoles<T::AccountId>,
+		pub state: PoolState,
+	}
+
+	impl<T: Config> OldBondedPoolInner<T> {
+		fn migrate_to_v8(self) -> BondedPoolInner<T> {
+			BondedPoolInner {
+				commission: Commission {
+					current: self.commission.current,
+					max: self.commission.max,
+					change_rate: self.commission.change_rate,
+					throttle_from: self.commission.throttle_from,
+					// `claim_permission` is a new field.
+					claim_permission: None,
+				},
+				member_counter: self.member_counter,
+				points: self.points,
+				roles: self.roles,
+				state: self.state,
+			}
+		}
+	}
+
+	pub struct VersionUncheckedMigrateV7ToV8<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateV7ToV8<T> {
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
+			Ok(Vec::new())
+		}
+
+		fn on_runtime_upgrade() -> Weight {
+			let mut translated = 0u64;
+			BondedPools::<T>::translate::<OldBondedPoolInner<T>, _>(|_key, old_value| {
+				translated.saturating_inc();
+				Some(old_value.migrate_to_v8())
+			});
+			T::DbWeight::get().reads_writes(translated, translated + 1)
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(_: Vec<u8>) -> Result<(), TryRuntimeError> {
+			// Check new `claim_permission` field is present.
+			ensure!(
+				BondedPools::<T>::iter()
+					.all(|(_, inner)| inner.commission.claim_permission.is_none()),
+				"`claim_permission` value has not been set correctly."
+			);
+			Ok(())
+		}
+	}
+}
+
 /// This migration accumulates and initializes the [`TotalValueLocked`] for all pools.
 ///
 /// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated

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

@@ -5761,7 +5761,13 @@ mod commission {
 			// Then:
 			assert_eq!(
 				BondedPool::<Runtime>::get(1).unwrap().commission,
-				Commission { current: None, max: None, change_rate: None, throttle_from: Some(1) }
+				Commission {
+					current: None,
+					max: None,
+					change_rate: None,
+					throttle_from: Some(1),
+					claim_permission: None,
+				}
 			);
 			assert_eq!(
 				pool_events_since_last_call(),
@@ -5956,6 +5962,7 @@ mod commission {
 						min_delay: 2_u64
 					}),
 					throttle_from: Some(1_u64),
+					claim_permission: None,
 				}
 			);
 			assert_eq!(
@@ -6007,6 +6014,7 @@ mod commission {
 						min_delay: 2_u64
 					}),
 					throttle_from: Some(3_u64),
+					claim_permission: None,
 				}
 			);
 			assert_eq!(
@@ -6082,7 +6090,8 @@ mod commission {
 						max_increase: Perbill::from_percent(1),
 						min_delay: 2
 					}),
-					throttle_from: Some(7)
+					throttle_from: Some(7),
+					claim_permission: None,
 				}
 			);
 			assert_eq!(
@@ -6183,6 +6192,7 @@ mod commission {
 					max: Some(Perbill::from_percent(50)),
 					change_rate: None,
 					throttle_from: Some(1),
+					claim_permission: None,
 				}
 			);
 
@@ -6409,6 +6419,7 @@ mod commission {
 						min_delay: 10_u64
 					}),
 					throttle_from: Some(11),
+					claim_permission: None,
 				}
 			);
 
@@ -6502,7 +6513,8 @@ mod commission {
 						max_increase: Perbill::from_percent(1),
 						min_delay: 0
 					}),
-					throttle_from: Some(1)
+					throttle_from: Some(1),
+					claim_permission: None,
 				}
 			);
 
@@ -6885,6 +6897,13 @@ mod commission {
 	#[test]
 	fn claim_commission_works() {
 		ExtBuilder::default().build_and_execute(|| {
+			/// Deposit rewards into the pool and claim payout. This will set up pending commission
+			/// to be tested in various scenarios.
+			fn deposit_rewards_and_claim_payout(caller: AccountId, points: u128) {
+				deposit_rewards(points);
+				assert_ok!(Pools::claim_payout(RuntimeOrigin::signed(caller)));
+			}
+
 			let pool_id = 1;
 
 			let _ = Currency::set_balance(&900, 5);
@@ -6905,21 +6924,9 @@ mod commission {
 				]
 			);
 
-			// Pool earns 80 points, payout is triggered.
-			deposit_rewards(80);
-			assert_eq!(
-				PoolMembers::<Runtime>::get(10).unwrap(),
-				PoolMember::<Runtime> { pool_id, points: 10, ..Default::default() }
-			);
-
-			assert_ok!(Pools::claim_payout(RuntimeOrigin::signed(10)));
-			assert_eq!(
-				pool_events_since_last_call(),
-				vec![Event::PaidOut { member: 10, pool_id, payout: 40 }]
-			);
-
 			// Given:
-			assert_eq!(RewardPool::<Runtime>::current_balance(pool_id), 40);
+			deposit_rewards_and_claim_payout(10, 100);
+			assert_eq!(RewardPool::<Runtime>::current_balance(pool_id), 50);
 
 			// Pool does not exist
 			assert_noop!(
@@ -6944,6 +6951,176 @@ mod commission {
 				Pools::claim_commission(RuntimeOrigin::signed(900), pool_id,),
 				Error::<Runtime>::NoPendingCommission
 			);
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::PaidOut { member: 10, pool_id, payout: 50 },
+					Event::PoolCommissionClaimed { pool_id: 1, commission: 50 }
+				]
+			);
+
+			// The pool commission's claim_permission field is updated to `Permissionless` by the
+			// root member, which means anyone can now claim commission for the pool.
+
+			// Given:
+			// Some random non-pool member to claim commission.
+			let non_pool_member = 1001;
+			let _ = Currency::set_balance(&non_pool_member, 5);
+
+			// Set up pending commission.
+			deposit_rewards_and_claim_payout(10, 100);
+			assert_ok!(Pools::set_commission_claim_permission(
+				RuntimeOrigin::signed(900),
+				pool_id,
+				Some(CommissionClaimPermission::Permissionless)
+			));
+
+			// When:
+			assert_ok!(Pools::claim_commission(RuntimeOrigin::signed(non_pool_member), pool_id));
+
+			// Then:
+			assert_eq!(RewardPool::<Runtime>::current_balance(pool_id), 0);
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::PaidOut { member: 10, pool_id, payout: 50 },
+					Event::PoolCommissionClaimPermissionUpdated {
+						pool_id: 1,
+						permission: Some(CommissionClaimPermission::Permissionless)
+					},
+					Event::PoolCommissionClaimed { pool_id: 1, commission: 50 },
+				]
+			);
+
+			// The pool commission's claim_permission is updated to an adhoc account by the root
+			// member, which means now only that account (in addition to the root role) can claim
+			// commission for the pool.
+
+			// Given:
+			// The account designated to claim commission.
+			let designated_commission_claimer = 2001;
+			let _ = Currency::set_balance(&designated_commission_claimer, 5);
+
+			// Set up pending commission.
+			deposit_rewards_and_claim_payout(10, 100);
+			assert_ok!(Pools::set_commission_claim_permission(
+				RuntimeOrigin::signed(900),
+				pool_id,
+				Some(CommissionClaimPermission::Account(designated_commission_claimer))
+			));
+
+			// When:
+			// Previous claimer can no longer claim commission.
+			assert_noop!(
+				Pools::claim_commission(RuntimeOrigin::signed(1001), pool_id,),
+				Error::<Runtime>::DoesNotHavePermission
+			);
+			// Designated claimer can claim commission.
+			assert_ok!(Pools::claim_commission(
+				RuntimeOrigin::signed(designated_commission_claimer),
+				pool_id
+			));
+
+			// Then:
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::PaidOut { member: 10, pool_id, payout: 50 },
+					Event::PoolCommissionClaimPermissionUpdated {
+						pool_id: 1,
+						permission: Some(CommissionClaimPermission::Account(2001))
+					},
+					Event::PoolCommissionClaimed { pool_id: 1, commission: 50 },
+				]
+			);
+
+			// Even with an Account claim permission set, the `root` role of the pool can still
+			// claim commission.
+
+			// Given:
+			deposit_rewards_and_claim_payout(10, 100);
+
+			// When:
+			assert_ok!(Pools::claim_commission(RuntimeOrigin::signed(900), pool_id));
+
+			// Then:
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::PaidOut { member: 10, pool_id, payout: 50 },
+					Event::PoolCommissionClaimed { pool_id: 1, commission: 50 },
+				]
+			);
+
+			// The root role updates commission's claim_permission back to `None`, which results in
+			// only the root member being able to claim commission for the pool.
+
+			// Given:
+			deposit_rewards_and_claim_payout(10, 100);
+
+			// When:
+			assert_ok!(Pools::set_commission_claim_permission(
+				RuntimeOrigin::signed(900),
+				pool_id,
+				None
+			));
+			// Previous claimer can no longer claim commission.
+			assert_noop!(
+				Pools::claim_commission(
+					RuntimeOrigin::signed(designated_commission_claimer),
+					pool_id,
+				),
+				Error::<Runtime>::DoesNotHavePermission
+			);
+			// Root can claim commission.
+			assert_ok!(Pools::claim_commission(RuntimeOrigin::signed(900), pool_id));
+
+			// Then:
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::PaidOut { member: 10, pool_id, payout: 50 },
+					Event::PoolCommissionClaimPermissionUpdated { pool_id: 1, permission: None },
+					Event::PoolCommissionClaimed { pool_id: 1, commission: 50 },
+				]
+			);
+		})
+	}
+
+	#[test]
+	fn set_commission_claim_permission_handles_errors() {
+		ExtBuilder::default().build_and_execute(|| {
+			let pool_id = 1;
+
+			let _ = Currency::set_balance(&900, 5);
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::Created { depositor: 10, pool_id },
+					Event::Bonded { member: 10, pool_id, bonded: 10, joined: true },
+				]
+			);
+
+			// Cannot operate on a non-existing pool.
+			assert_noop!(
+				Pools::set_commission_claim_permission(
+					RuntimeOrigin::signed(10),
+					90,
+					Some(CommissionClaimPermission::Permissionless)
+				),
+				Error::<Runtime>::PoolNotFound
+			);
+
+			// Only the root role can change the commission claim permission.
+			assert_noop!(
+				Pools::set_commission_claim_permission(
+					RuntimeOrigin::signed(10),
+					pool_id,
+					Some(CommissionClaimPermission::Permissionless)
+				),
+				Error::<Runtime>::DoesNotHavePermission
+			);
 		})
 	}
 }