Pools: Make PermissionlessWithdraw the default claim permission

polkadot/runtime/westend/src/lib.rs

@@ -1707,6 +1707,7 @@ pub mod migrations {
 			crate::xcm_config::XcmRouter,
 			GetLegacyLeaseImpl,
 		>,
+		pallet_nomination_pools::migration::versioned::V8ToV9<Runtime>,
 	);
 }
 

prdoc/pr_3438.prdoc

@@ -0,0 +1,13 @@
+title: "Pools: Make PermissionlessWithdraw the default claim permission"
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Makes permissionless withdrawing the default claim permission, giving any network participant
+      access to claim pool rewards on member's behalf, by default.
+
+      Removes the PermissionlessAll variant, and adds a migration to migrate PermissionlessAll to
+      PermissionlessWithdraw.
+
+crates:
+  - name: pallet-nomination-pools

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

@@ -285,8 +285,8 @@ frame_benchmarking::benchmarks! {
 		let scenario = ListScenario::<T>::new(origin_weight, true)?;
 		let extra = (scenario.dest_weight - origin_weight).max(CurrencyOf::<T>::minimum_balance());
 
-		// set claim preferences to `PermissionlessAll` to any account to bond extra on member's behalf.
-		let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(scenario.creator1.clone()).into(), ClaimPermission::PermissionlessAll);
+		// set claim preferences to `PermissionlessCompound` to any account to bond extra on member's behalf.
+		let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(scenario.creator1.clone()).into(), ClaimPermission::PermissionlessCompound);
 
 		// transfer exactly `extra` to the depositor of the src pool (1),
 		let reward_account1 = Pools::<T>::create_reward_account(1);
@@ -313,9 +313,9 @@ frame_benchmarking::benchmarks! {
 		// Send funds to the reward account of the pool
 		CurrencyOf::<T>::set_balance(&reward_account, ed + origin_weight);
 
-		// set claim preferences to `PermissionlessAll` so any account can claim rewards on member's
+		// set claim preferences to `PermissionlessWithdraw` so any account can claim rewards on member's
 		// behalf.
-		let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessAll);
+		let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessWithdraw);
 
 		// Sanity check
 		assert_eq!(
@@ -795,9 +795,9 @@ frame_benchmarking::benchmarks! {
 			T::Staking::active_stake(&pool_account).unwrap(),
 			min_create_bond + min_join_bond
 		);
-	}:_(RuntimeOrigin::Signed(joiner.clone()), ClaimPermission::PermissionlessAll)
+	}:_(RuntimeOrigin::Signed(joiner.clone()), ClaimPermission::Permissioned)
 	verify {
-		assert_eq!(ClaimPermissions::<T>::get(joiner), ClaimPermission::PermissionlessAll);
+		assert_eq!(ClaimPermissions::<T>::get(joiner), ClaimPermission::Permissioned);
 	}
 
 	claim_commission {

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

@@ -457,23 +457,25 @@ pub enum ClaimPermission {
 	PermissionlessCompound,
 	/// Anyone can withdraw rewards on a pool member's behalf.
 	PermissionlessWithdraw,
-	/// Anyone can withdraw and compound rewards on a pool member's behalf.
-	PermissionlessAll,
+}
+
+impl Default for ClaimPermission {
+	fn default() -> Self {
+		Self::PermissionlessWithdraw
+	}
 }
 
 impl ClaimPermission {
+	/// Permissionless compounding of pool rewards is allowed if the current permission is
+	/// `PermissionlessCompound`.
 	fn can_bond_extra(&self) -> bool {
-		matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessCompound)
+		matches!(self, ClaimPermission::PermissionlessCompound)
 	}
 
+	/// Permissionless payout claiming is allowed if the current permission is
+	/// `PermissionlessWithdraw`.
 	fn can_claim_payout(&self) -> bool {
-		matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessWithdraw)
-	}
-}
-
-impl Default for ClaimPermission {
-	fn default() -> Self {
-		Self::Permissioned
+		matches!(self, ClaimPermission::PermissionlessWithdraw)
 	}
 }
 
@@ -1577,7 +1579,7 @@ pub mod pallet {
 	use sp_runtime::Perbill;
 
 	/// The in-code storage version.
-	const STORAGE_VERSION: StorageVersion = StorageVersion::new(8);
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(9);
 
 	#[pallet::pallet]
 	#[pallet::storage_version(STORAGE_VERSION)]
@@ -2595,7 +2597,7 @@ pub mod pallet {
 		///
 		/// In the case of `origin != other`, `origin` can only bond extra pending rewards of
 		/// `other` members assuming set_claim_permission for the given member is
-		/// `PermissionlessAll` or `PermissionlessCompound`.
+		/// `PermissionlessCompound`.
 		#[pallet::call_index(14)]
 		#[pallet::weight(
 			T::WeightInfo::bond_extra_transfer()
@@ -2613,15 +2615,10 @@ pub mod pallet {
 		/// Allows a pool member to set a claim permission to allow or disallow permissionless
 		/// bonding and withdrawing.
 		///
-		/// By default, this is `Permissioned`, which implies only the pool member themselves can
-		/// claim their pending rewards. If a pool member wishes so, they can set this to
-		/// `PermissionlessAll` to allow any account to claim their rewards and bond extra to the
-		/// pool.
-		///
 		/// # Arguments
 		///
 		/// * `origin` - Member of a pool.
-		/// * `actor` - Account to claim reward. // improve this
+		/// * `permission` - The permission to be applied.
 		#[pallet::call_index(15)]
 		#[pallet::weight(T::DbWeight::get().reads_writes(1, 1))]
 		pub fn set_claim_permission(
@@ -2631,16 +2628,18 @@ pub mod pallet {
 			let who = ensure_signed(origin)?;
 
 			ensure!(PoolMembers::<T>::contains_key(&who), Error::<T>::PoolMemberNotFound);
+
 			ClaimPermissions::<T>::mutate(who, |source| {
 				*source = permission;
 			});
+
 			Ok(())
 		}
 
 		/// `origin` can claim payouts on some pool member `other`'s behalf.
 		///
-		/// Pool member `other` must have a `PermissionlessAll` or `PermissionlessWithdraw` in order
-		/// for this call to be successful.
+		/// Pool member `other` must have a `PermissionlessWithdraw` claim permission for this call
+		/// to be successful.
 		#[pallet::call_index(16)]
 		#[pallet::weight(T::WeightInfo::claim_payout())]
 		pub fn claim_payout_other(origin: OriginFor<T>, other: T::AccountId) -> DispatchResult {

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

@@ -27,6 +27,15 @@ use sp_runtime::TryRuntimeError;
 pub mod versioned {
 	use super::*;
 
+	/// v9: Migrates claim permissions with a `PermissionlessAll` value to `PermissionlessWithdraw`.
+	pub type V8ToV9<T> = frame_support::migrations::VersionedMigration<
+		8,
+		9,
+		v9::VersionUncheckedMigrateV8ToV9<T>,
+		crate::pallet::Pallet<T>,
+		<T as frame_system::Config>::DbWeight,
+	>;
+
 	/// v8: Adds commission claim permissions to `BondedPools`.
 	pub type V7ToV8<T> = frame_support::migrations::VersionedMigration<
 		7,
@@ -109,6 +118,65 @@ pub mod unversioned {
 	}
 }
 
+mod v9 {
+	use super::*;
+
+	#[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)]
+	pub enum OldClaimPermission {
+		Permissioned,
+		PermissionlessCompound,
+		PermissionlessWithdraw,
+		PermissionlessAll,
+	}
+
+	impl OldClaimPermission {
+		fn migrate_to_v9(self) -> Option<ClaimPermission> {
+			match self {
+				OldClaimPermission::Permissioned => Some(ClaimPermission::Permissioned),
+				OldClaimPermission::PermissionlessCompound =>
+					Some(ClaimPermission::PermissionlessCompound),
+				OldClaimPermission::PermissionlessWithdraw =>
+					Some(ClaimPermission::PermissionlessWithdraw),
+				OldClaimPermission::PermissionlessAll =>
+					Some(ClaimPermission::PermissionlessWithdraw),
+			}
+		}
+	}
+
+	pub struct VersionUncheckedMigrateV8ToV9<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config> OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9<T> {
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<Vec<u8>, TryRuntimeError> {
+			let claim_permission_count = ClaimPermissions::<T>::iter_keys().count();
+			Ok((claim_permission_count as u64).encode())
+		}
+
+		fn on_runtime_upgrade() -> Weight {
+			let mut translates = 0u64;
+			ClaimPermissions::<T>::translate::<OldClaimPermission, _>(|_key, current_val| {
+				translates.saturating_inc();
+				current_val.migrate_to_v9()
+			});
+
+			T::DbWeight::get().reads_writes(translates, translates)
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(data: Vec<u8>) -> Result<(), TryRuntimeError> {
+			let old_claim_permission_count: u64 = Decode::decode(&mut &data[..]).unwrap();
+			let new_claim_permission_count = ClaimPermissions::<T>::iter_keys().count() as u64;
+
+			// There should no longer be `PermissionlessAll` claim
+			// permissions in storage.
+			ensure!(
+				old_claim_permission_count == new_claim_permission_count,
+				"Not all`ClaimPermission` values have been migrated."
+			);
+			Ok(())
+		}
+	}
+}
+
 pub mod v8 {
 	use super::{v7::V7BondedPoolInner, *};
 

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

@@ -2441,16 +2441,9 @@ mod claim_payout {
 			// given
 			assert_eq!(Currency::free_balance(&10), 35);
 
-			// Permissioned by default
-			assert_noop!(
-				Pools::claim_payout_other(RuntimeOrigin::signed(80), 10),
-				Error::<Runtime>::DoesNotHavePermission
-			);
+			// when
 
-			assert_ok!(Pools::set_claim_permission(
-				RuntimeOrigin::signed(10),
-				ClaimPermission::PermissionlessWithdraw
-			));
+			// Claim permission of `PermissionlessWithdraw` allows payout claiming as default.
 			assert_ok!(Pools::claim_payout_other(RuntimeOrigin::signed(80), 10));
 
 			// then
@@ -2488,21 +2481,10 @@ mod unbond {
 					Error::<T>::MinimumBondNotMet
 				);
 
-				// Make permissionless
-				assert_eq!(ClaimPermissions::<Runtime>::get(10), ClaimPermission::Permissioned);
-				assert_ok!(Pools::set_claim_permission(
-					RuntimeOrigin::signed(20),
-					ClaimPermission::PermissionlessAll
-				));
-
 				// but can go to 0
 				assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 15));
 				assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().active_points(), 0);
 				assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().unbonding_points(), 20);
-				assert_eq!(
-					ClaimPermissions::<Runtime>::get(20),
-					ClaimPermission::PermissionlessAll
-				);
 			})
 	}
 
@@ -4563,12 +4545,11 @@ mod withdraw_unbonded {
 			CurrentEra::set(1);
 			assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().points, 20);
 
-			assert_ok!(Pools::set_claim_permission(
-				RuntimeOrigin::signed(20),
-				ClaimPermission::PermissionlessAll
-			));
 			assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 20));
-			assert_eq!(ClaimPermissions::<Runtime>::get(20), ClaimPermission::PermissionlessAll);
+			assert_eq!(
+				ClaimPermissions::<Runtime>::get(20),
+				ClaimPermission::PermissionlessWithdraw
+			);
 
 			assert_eq!(
 				pool_events_since_last_call(),
@@ -4792,7 +4773,7 @@ mod create {
 }
 
 #[test]
-fn set_claimable_actor_works() {
+fn set_claim_permission_works() {
 	ExtBuilder::default().build_and_execute(|| {
 		// Given
 		Currency::set_balance(&11, ExistentialDeposit::get() + 2);
@@ -4811,22 +4792,19 @@ fn set_claimable_actor_works() {
 			]
 		);
 
-		// Make permissionless
-		assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::Permissioned);
+		// Make permissioned
+		assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::PermissionlessWithdraw);
 		assert_noop!(
-			Pools::set_claim_permission(
-				RuntimeOrigin::signed(12),
-				ClaimPermission::PermissionlessAll
-			),
+			Pools::set_claim_permission(RuntimeOrigin::signed(12), ClaimPermission::Permissioned),
 			Error::<T>::PoolMemberNotFound
 		);
 		assert_ok!(Pools::set_claim_permission(
 			RuntimeOrigin::signed(11),
-			ClaimPermission::PermissionlessAll
+			ClaimPermission::Permissioned
 		));
 
 		// then
-		assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::PermissionlessAll);
+		assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::Permissioned);
 	});
 }
 
@@ -5212,7 +5190,7 @@ mod bond_extra {
 
 			assert_ok!(Pools::set_claim_permission(
 				RuntimeOrigin::signed(10),
-				ClaimPermission::PermissionlessAll
+				ClaimPermission::PermissionlessCompound
 			));
 			assert_ok!(Pools::bond_extra_other(RuntimeOrigin::signed(50), 10, BondExtra::Rewards));
 			assert_eq!(Currency::free_balance(&default_reward_account()), 7);