From ac22eaf6775b6b7083d056116768ece2b7eb5861 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 12:15:42 +0700 Subject: [PATCH 01/29] add v9 migration --- .../frame/nomination-pools/src/migration.rs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 6887fcfa7ecad..a0f125df59219 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/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 = frame_support::migrations::VersionedMigration< + 8, + 9, + v9::VersionUncheckedMigrateV8ToV9, + crate::pallet::Pallet, + ::DbWeight, + >; + /// v8: Adds commission claim permissions to `BondedPools`. pub type V7ToV8 = frame_support::migrations::VersionedMigration< 7, @@ -109,6 +118,47 @@ pub mod unversioned { } } +mod v9 { + use super::*; + + pub struct VersionUncheckedMigrateV8ToV9(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9 { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + fn on_runtime_upgrade() -> Weight { + let mut reads = 0u64; + let mut translated = 0u64; + + ClaimPermissions::::translate::(|_key, val| { + reads.saturating_inc(); + if self == ClaimPermission.PermissionlessAll { + translated.saturating_inc(); + ClaimPermission::PermissionlessWithdraw + } else { + val + }}); + + // reads: translated + onchain version. + // writes: translated. + T::DbWeight::get().reads_writes(reads + 1, translated) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { + // There should no longer be `PermissionlessAll` claim permissions in storage. + ensure!( + ClaimPermissions::::iter() + .all(|val| val !== ClaimPermission.PermissionlessAll), + "A `ClaimPermission` value has not been migrated." + ); + Ok(()) + } + } +} + pub mod v8 { use super::{v7::V7BondedPoolInner, *}; From a3c99b2a6195f1825804a3090dac5684aa076f7c Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 12:18:02 +0700 Subject: [PATCH 02/29] Revert "add v9 migration" This reverts commit ac22eaf6775b6b7083d056116768ece2b7eb5861. --- .../frame/nomination-pools/src/migration.rs | 50 ------------------- 1 file changed, 50 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index a0f125df59219..6887fcfa7ecad 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -27,15 +27,6 @@ use sp_runtime::TryRuntimeError; pub mod versioned { use super::*; - /// v9: Migrates claim permissions with a `PermissionlessAll` value to `PermissionlessWithdraw`. - pub type V8ToV9 = frame_support::migrations::VersionedMigration< - 8, - 9, - v9::VersionUncheckedMigrateV8ToV9, - crate::pallet::Pallet, - ::DbWeight, - >; - /// v8: Adds commission claim permissions to `BondedPools`. pub type V7ToV8 = frame_support::migrations::VersionedMigration< 7, @@ -118,47 +109,6 @@ pub mod unversioned { } } -mod v9 { - use super::*; - - pub struct VersionUncheckedMigrateV8ToV9(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9 { - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) - } - - fn on_runtime_upgrade() -> Weight { - let mut reads = 0u64; - let mut translated = 0u64; - - ClaimPermissions::::translate::(|_key, val| { - reads.saturating_inc(); - if self == ClaimPermission.PermissionlessAll { - translated.saturating_inc(); - ClaimPermission::PermissionlessWithdraw - } else { - val - }}); - - // reads: translated + onchain version. - // writes: translated. - T::DbWeight::get().reads_writes(reads + 1, translated) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { - // There should no longer be `PermissionlessAll` claim permissions in storage. - ensure!( - ClaimPermissions::::iter() - .all(|val| val !== ClaimPermission.PermissionlessAll), - "A `ClaimPermission` value has not been migrated." - ); - Ok(()) - } - } -} - pub mod v8 { use super::{v7::V7BondedPoolInner, *}; From aef43288b12addb90d3a7c8673f7052215f7314c Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 12:23:34 +0700 Subject: [PATCH 03/29] v9 migration --- .../frame/nomination-pools/src/migration.rs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 6887fcfa7ecad..0c225952f8f24 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/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 = frame_support::migrations::VersionedMigration< + 8, + 9, + v9::VersionUncheckedMigrateV8ToV9, + crate::pallet::Pallet, + ::DbWeight, + >; + /// v8: Adds commission claim permissions to `BondedPools`. pub type V7ToV8 = frame_support::migrations::VersionedMigration< 7, @@ -109,6 +118,45 @@ pub mod unversioned { } } +mod v9 { + use super::*; + + pub struct VersionUncheckedMigrateV8ToV9(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9 { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + fn on_runtime_upgrade() -> Weight { + let mut translates = 0u64; + ClaimPermissions::::translate::(|_key, val| { + translates.saturating_inc(); + translates.saturating_inc(); + + if self == ClaimPermission.PermissionlessAll { + ClaimPermission::PermissionlessWithdraw + } else { + val + } + }); + + T::DbWeight::get().reads_writes(translates, translates) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { + // There should no longer be `PermissionlessAll` claim permissions in storage. + ensure!( + ClaimPermissions::::iter() + .all(|val| val != ClaimPermission.PermissionlessAll), + "A `ClaimPermission` value has not been migrated." + ); + Ok(()) + } + } +} + pub mod v8 { use super::{v7::V7BondedPoolInner, *}; From ee7d671c399ab482622d8e37e455804702b15738 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 12:30:02 +0700 Subject: [PATCH 04/29] use OldClaimPermission --- .../frame/nomination-pools/src/migration.rs | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 0c225952f8f24..ce0ab47b12f1a 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -121,6 +121,18 @@ pub mod unversioned { mod v9 { use super::*; + #[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)] + pub enum OldClaimPermission { + Permissioned, + PermissionlessCompound, + PermissionlessWithdraw, + PermissionlessAll, + } + + #[frame_support::storage_alias] + pub type ClaimPermissions = + StorageMap<_, Twox64Concat, T::AccountId, OldClaimPermission, ValueQuery>; + pub struct VersionUncheckedMigrateV8ToV9(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9 { #[cfg(feature = "try-runtime")] @@ -130,14 +142,14 @@ mod v9 { fn on_runtime_upgrade() -> Weight { let mut translates = 0u64; - ClaimPermissions::::translate::(|_key, val| { + ClaimPermissions::::translate::(|_key, val| { translates.saturating_inc(); translates.saturating_inc(); if self == ClaimPermission.PermissionlessAll { - ClaimPermission::PermissionlessWithdraw + Some(ClaimPermission::PermissionlessWithdraw) } else { - val + Some(val) } }); @@ -148,8 +160,7 @@ mod v9 { fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { // There should no longer be `PermissionlessAll` claim permissions in storage. ensure!( - ClaimPermissions::::iter() - .all(|val| val != ClaimPermission.PermissionlessAll), + ClaimPermissions::::iter().all(|val| val != ClaimPermission.PermissionlessAll), "A `ClaimPermission` value has not been migrated." ); Ok(()) From 2022bb6aa40b6c7b7db7b544c9c4f38d51f909ea Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 13:03:20 +0700 Subject: [PATCH 05/29] make `PermissionlessWithdraw` default --- .../nomination-pools/benchmarking/src/lib.rs | 12 ++--- substrate/frame/nomination-pools/src/lib.rs | 21 +++----- .../frame/nomination-pools/src/migration.rs | 25 +++++----- substrate/frame/nomination-pools/src/tests.rs | 48 +++++-------------- 4 files changed, 40 insertions(+), 66 deletions(-) diff --git a/substrate/frame/nomination-pools/benchmarking/src/lib.rs b/substrate/frame/nomination-pools/benchmarking/src/lib.rs index 48d7dae29ef03..933104eff93a9 100644 --- a/substrate/frame/nomination-pools/benchmarking/src/lib.rs +++ b/substrate/frame/nomination-pools/benchmarking/src/lib.rs @@ -285,8 +285,8 @@ frame_benchmarking::benchmarks! { let scenario = ListScenario::::new(origin_weight, true)?; let extra = (scenario.dest_weight - origin_weight).max(CurrencyOf::::minimum_balance()); - // set claim preferences to `PermissionlessAll` to any account to bond extra on member's behalf. - let _ = Pools::::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::::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::::create_reward_account(1); @@ -313,9 +313,9 @@ frame_benchmarking::benchmarks! { // Send funds to the reward account of the pool CurrencyOf::::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::::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessAll); + let _ = Pools::::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::::get(joiner), ClaimPermission::PermissionlessAll); + assert_eq!(ClaimPermissions::::get(joiner), ClaimPermission::Permissioned); } claim_commission { diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 074d59931ade7..65a8e5112f265 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -457,23 +457,21 @@ 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 ClaimPermission { fn can_bond_extra(&self) -> bool { - matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessCompound) + matches!(self, ClaimPermission::PermissionlessCompound) } fn can_claim_payout(&self) -> bool { - matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessWithdraw) + matches!(self, ClaimPermission::PermissionlessWithdraw) } } impl Default for ClaimPermission { fn default() -> Self { - Self::Permissioned + Self::PermissionlessWithdraw } } @@ -2595,7 +2593,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 +2611,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( @@ -2639,8 +2632,8 @@ pub mod pallet { /// `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, other: T::AccountId) -> DispatchResult { diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index ce0ab47b12f1a..6606c63afd018 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -129,9 +129,18 @@ mod v9 { PermissionlessAll, } - #[frame_support::storage_alias] - pub type ClaimPermissions = - StorageMap<_, Twox64Concat, T::AccountId, OldClaimPermission, ValueQuery>; + impl OldClaimPermission { + fn migrate_to_v9(self) -> ClaimPermission { + match self { + OldClaimPermission::Permissioned => ClaimPermission::Permissioned, + OldClaimPermission::PermissionlessCompound => + ClaimPermission::PermissionlessCompound, + OldClaimPermission::PermissionlessWithdraw => + ClaimPermission::PermissionlessWithdraw, + OldClaimPermission::PermissionlessAll => ClaimPermission::PermissionlessWithdraw, + } + } + } pub struct VersionUncheckedMigrateV8ToV9(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9 { @@ -142,15 +151,9 @@ mod v9 { fn on_runtime_upgrade() -> Weight { let mut translates = 0u64; - ClaimPermissions::::translate::(|_key, val| { + ClaimPermissions::::translate::(|_key, current_val| { translates.saturating_inc(); - translates.saturating_inc(); - - if self == ClaimPermission.PermissionlessAll { - Some(ClaimPermission::PermissionlessWithdraw) - } else { - Some(val) - } + Some(current_val.migrate_to_v9()) }); T::DbWeight::get().reads_writes(translates, translates) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 13298fa64f54c..dd49feada7871 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/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::::DoesNotHavePermission - ); + // when - assert_ok!(Pools::set_claim_permission( - RuntimeOrigin::signed(10), - ClaimPermission::PermissionlessWithdraw - )); + // Claim permission of `PermissionlessWithdraw` allows payout claiming be default. assert_ok!(Pools::claim_payout_other(RuntimeOrigin::signed(80), 10)); // then @@ -2488,21 +2481,10 @@ mod unbond { Error::::MinimumBondNotMet ); - // Make permissionless - assert_eq!(ClaimPermissions::::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::::get(20).unwrap().active_points(), 0); assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 20); - assert_eq!( - ClaimPermissions::::get(20), - ClaimPermission::PermissionlessAll - ); }) } @@ -4563,12 +4545,11 @@ mod withdraw_unbonded { CurrentEra::set(1); assert_eq!(PoolMembers::::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::::get(20), ClaimPermission::PermissionlessAll); + assert_eq!( + ClaimPermissions::::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::::get(11), ClaimPermission::Permissioned); + // Make permissioned + assert_eq!(ClaimPermissions::::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::::PoolMemberNotFound ); assert_ok!(Pools::set_claim_permission( RuntimeOrigin::signed(11), - ClaimPermission::PermissionlessAll + ClaimPermission::Permissioned )); // then - assert_eq!(ClaimPermissions::::get(11), ClaimPermission::PermissionlessAll); + assert_eq!(ClaimPermissions::::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); From d767c179f5458ca267cf02c93515b17b1b8dd514 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 14:34:31 +0700 Subject: [PATCH 06/29] don't store default --- substrate/frame/nomination-pools/src/lib.rs | 40 ++++++++++++++----- .../frame/nomination-pools/src/migration.rs | 15 +++---- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 65a8e5112f265..c34e102d5d067 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -459,22 +459,36 @@ pub enum ClaimPermission { PermissionlessWithdraw, } +impl Default for ClaimPermission { + fn default() -> Self { + Self::PermissionlessWithdraw + } +} + impl ClaimPermission { + /// If the current permission is `PermissionlessWithdraw`, return `None`, otherwise return + /// `Some(self)`. + fn default_as_none(self) -> Option { + if self == ClaimPermission::default() { + None + } else { + Some(self) + } + } + + /// Permissionless compounding of pool rewards is alllowed if the current permission is + /// `PermissionlessCompound`. fn can_bond_extra(&self) -> bool { matches!(self, ClaimPermission::PermissionlessCompound) } + /// Permissionless payout claiming is alllowed if the current permission is + /// `PermissionlessWithdraw`. fn can_claim_payout(&self) -> bool { matches!(self, ClaimPermission::PermissionlessWithdraw) } } -impl Default for ClaimPermission { - fn default() -> Self { - Self::PermissionlessWithdraw - } -} - /// A member in a pool. #[derive( Encode, @@ -2624,9 +2638,17 @@ pub mod pallet { let who = ensure_signed(origin)?; ensure!(PoolMembers::::contains_key(&who), Error::::PoolMemberNotFound); - ClaimPermissions::::mutate(who, |source| { - *source = permission; - }); + + // If the permission is `None`, then remove the claim permission. + match permission.default_as_none() { + Some(claim_permission) => { + ClaimPermissions::::insert(&who, claim_permission); + }, + None => { + ClaimPermissions::::remove(&who); + }, + } + Ok(()) } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 6606c63afd018..173be7288944a 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -130,14 +130,15 @@ mod v9 { } impl OldClaimPermission { - fn migrate_to_v9(self) -> ClaimPermission { + // NOTE: As `PermissionlessWithraw` is now default, we do not need these entries in storage. + // `PermissionlessAll` entries are also removed. + fn migrate_to_v9(self) -> Option { match self { - OldClaimPermission::Permissioned => ClaimPermission::Permissioned, + OldClaimPermission::Permissioned => Some(ClaimPermission::Permissioned), OldClaimPermission::PermissionlessCompound => - ClaimPermission::PermissionlessCompound, - OldClaimPermission::PermissionlessWithdraw => - ClaimPermission::PermissionlessWithdraw, - OldClaimPermission::PermissionlessAll => ClaimPermission::PermissionlessWithdraw, + Some(ClaimPermission::PermissionlessCompound), + OldClaimPermission::PermissionlessWithdraw => None, + OldClaimPermission::PermissionlessAll => None, } } } @@ -153,7 +154,7 @@ mod v9 { let mut translates = 0u64; ClaimPermissions::::translate::(|_key, current_val| { translates.saturating_inc(); - Some(current_val.migrate_to_v9()) + current_val.migrate_to_v9() }); T::DbWeight::get().reads_writes(translates, translates) From 5bc03561ccaba5b4290f624e2727deef9443c0a5 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 15:05:15 +0700 Subject: [PATCH 07/29] fix typo --- substrate/frame/nomination-pools/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index c34e102d5d067..2aa6c24307d49 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -476,13 +476,13 @@ impl ClaimPermission { } } - /// Permissionless compounding of pool rewards is alllowed if the current permission is + /// Permissionless compounding of pool rewards is allowed if the current permission is /// `PermissionlessCompound`. fn can_bond_extra(&self) -> bool { matches!(self, ClaimPermission::PermissionlessCompound) } - /// Permissionless payout claiming is alllowed if the current permission is + /// Permissionless payout claiming is allowed if the current permission is /// `PermissionlessWithdraw`. fn can_claim_payout(&self) -> bool { matches!(self, ClaimPermission::PermissionlessWithdraw) From 36c6c977da7862fe93ece532dd8ffccc18f545d4 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 15:06:31 +0700 Subject: [PATCH 08/29] fix comment --- substrate/frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index dd49feada7871..e74b45e8f61d4 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -2443,7 +2443,7 @@ mod claim_payout { // when - // Claim permission of `PermissionlessWithdraw` allows payout claiming be default. + // Claim permission of `PermissionlessWithdraw` allows payout claiming as default. assert_ok!(Pools::claim_payout_other(RuntimeOrigin::signed(80), 10)); // then From cf38a3f49bf95cbe5cafac9f773de5267ce817f8 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 15:11:47 +0700 Subject: [PATCH 09/29] add migration --- polkadot/runtime/westend/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index bbb010f60bffe..460da06235ef1 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1711,6 +1711,7 @@ pub mod migrations { crate::xcm_config::XcmRouter, GetLegacyLeaseImpl, >, + pallet_nomination_pools::migration::versioned::V8ToV9, ); } From c305ddaf22115db8402d1ba9bed2dd8fe8d2a8b4 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 15:18:00 +0700 Subject: [PATCH 10/29] add prdoc --- prdoc/pr_3438.prdoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 prdoc/pr_3438.prdoc diff --git a/prdoc/pr_3438.prdoc b/prdoc/pr_3438.prdoc new file mode 100644 index 0000000000000..9b84692a87512 --- /dev/null +++ b/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 remove PermissionlessAll from + ClaimPermission. Also removes default values from storage. + +crates: + - name: pallet-nomination-pools From 4077e3fc103b8b1fff154e572ce340c7346cf786 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 15:31:56 +0700 Subject: [PATCH 11/29] amend post_upgrade --- substrate/frame/nomination-pools/src/migration.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 173be7288944a..9316f281f478c 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -162,9 +162,13 @@ mod v9 { #[cfg(feature = "try-runtime")] fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { - // There should no longer be `PermissionlessAll` claim permissions in storage. + // There should no longer be `PermissionlessAll` or `PermissionlessWithdraw` claim + // permissions in storage. ensure!( - ClaimPermissions::::iter().all(|val| val != ClaimPermission.PermissionlessAll), + ClaimPermissions::::iter().all(|val| match val { + ClaimPermission::Permissioned | ClaimPermission::PermissionlessCompound => true, + _ => false, + }), "A `ClaimPermission` value has not been migrated." ); Ok(()) From af6a34744dc466ac1a2413842307757fcb1a0867 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 22 Feb 2024 15:36:40 +0700 Subject: [PATCH 12/29] fix --- substrate/frame/nomination-pools/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 9316f281f478c..ccf33e29ac197 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -165,7 +165,7 @@ mod v9 { // There should no longer be `PermissionlessAll` or `PermissionlessWithdraw` claim // permissions in storage. ensure!( - ClaimPermissions::::iter().all(|val| match val { + ClaimPermissions::::iter().all(|(_, val)| match val { ClaimPermission::Permissioned | ClaimPermission::PermissionlessCompound => true, _ => false, }), From a8d72d4b75458577d24043f16007e7051249f184 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 28 Feb 2024 15:17:41 +0700 Subject: [PATCH 13/29] action feedback --- prdoc/pr_3438.prdoc | 4 ++-- substrate/frame/nomination-pools/src/lib.rs | 22 +++---------------- .../frame/nomination-pools/src/migration.rs | 8 +++---- 3 files changed, 9 insertions(+), 25 deletions(-) diff --git a/prdoc/pr_3438.prdoc b/prdoc/pr_3438.prdoc index 9b84692a87512..93e46032e3ef3 100644 --- a/prdoc/pr_3438.prdoc +++ b/prdoc/pr_3438.prdoc @@ -6,8 +6,8 @@ doc: 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 remove PermissionlessAll from - ClaimPermission. Also removes default values from storage. + Removes the PermissionlessAll variant, and adds a migration to migrate PermissionlessAll to + PermissionlessWithdraw. crates: - name: pallet-nomination-pools diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 73f764fac1c1d..c814ecc4a9a30 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -466,16 +466,6 @@ impl Default for ClaimPermission { } impl ClaimPermission { - /// If the current permission is `PermissionlessWithdraw`, return `None`, otherwise return - /// `Some(self)`. - fn default_as_none(self) -> Option { - if self == ClaimPermission::default() { - None - } else { - Some(self) - } - } - /// Permissionless compounding of pool rewards is allowed if the current permission is /// `PermissionlessCompound`. fn can_bond_extra(&self) -> bool { @@ -2639,15 +2629,9 @@ pub mod pallet { ensure!(PoolMembers::::contains_key(&who), Error::::PoolMemberNotFound); - // If the permission is `None`, then remove the claim permission. - match permission.default_as_none() { - Some(claim_permission) => { - ClaimPermissions::::insert(&who, claim_permission); - }, - None => { - ClaimPermissions::::remove(&who); - }, - } + ClaimPermissions::::mutate(who, |source| { + *source = permission; + }); Ok(()) } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index ffecb52f1d151..56de1a4e9b7ce 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -130,15 +130,15 @@ mod v9 { } impl OldClaimPermission { - // NOTE: As `PermissionlessWithraw` is now default, we do not need these entries in storage. - // `PermissionlessAll` entries are also removed. fn migrate_to_v9(self) -> Option { match self { OldClaimPermission::Permissioned => Some(ClaimPermission::Permissioned), OldClaimPermission::PermissionlessCompound => Some(ClaimPermission::PermissionlessCompound), - OldClaimPermission::PermissionlessWithdraw => None, - OldClaimPermission::PermissionlessAll => None, + OldClaimPermission::PermissionlessWithdraw => + Some(ClaimPermission::PermissionlessWithdraw), + OldClaimPermission::PermissionlessAll => + Some(ClaimPermission::PermissionlessWithdraw), } } } From 2e3274744077302e9926e6d3cf5c4f2775e0fc01 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 28 Feb 2024 15:35:57 +0700 Subject: [PATCH 14/29] bump storage version --- substrate/frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index c814ecc4a9a30..8335dd0184ecf 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1579,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)] From b51c28d4232701032ecc9940940c318cf8a37f79 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 28 Feb 2024 16:01:33 +0700 Subject: [PATCH 15/29] amend post_upgrade --- substrate/frame/nomination-pools/src/migration.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 56de1a4e9b7ce..74ffaa2fddbc2 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -166,7 +166,9 @@ mod v9 { // permissions in storage. ensure!( ClaimPermissions::::iter().all(|(_, val)| match val { - ClaimPermission::Permissioned | ClaimPermission::PermissionlessCompound => true, + ClaimPermission::Permissioned | + ClaimPermission::PermissionlessCompound | + ClaimPermission::PermissionlessWithdraw => true, _ => false, }), "A `ClaimPermission` value has not been migrated." From 425e7258519bb7575f7de6723db191f184d0093f Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 28 Feb 2024 16:32:14 +0700 Subject: [PATCH 16/29] post_upgrade checks count --- .../frame/nomination-pools/src/migration.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 74ffaa2fddbc2..855899efbd349 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -147,7 +147,8 @@ mod v9 { impl OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) + let claim_permission_count = ClaimPermissions::::iter_keys().count(); + Ok((claim_permission_count as u64).encode()) } fn on_runtime_upgrade() -> Weight { @@ -161,17 +162,15 @@ mod v9 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { - // There should no longer be `PermissionlessAll` or `PermissionlessWithdraw` claim + fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { + let old_claim_permission_count: u64 = Decode::decode(&mut &data[..]).unwrap(); + let new_claim_permission_count = ClaimPermissions::::iter_keys().count(); + + // There should no longer be `PermissionlessAll` claim // permissions in storage. ensure!( - ClaimPermissions::::iter().all(|(_, val)| match val { - ClaimPermission::Permissioned | - ClaimPermission::PermissionlessCompound | - ClaimPermission::PermissionlessWithdraw => true, - _ => false, - }), - "A `ClaimPermission` value has not been migrated." + old_claim_permission_count == new_claim_permission_count, + "Not all`ClaimPermission` values have been migrated." ); Ok(()) } From 32c8ed3720d55c127c41069152e5543f775e23f9 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 28 Feb 2024 16:41:10 +0700 Subject: [PATCH 17/29] u64 --- substrate/frame/nomination-pools/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 855899efbd349..e67dc74755385 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -164,7 +164,7 @@ mod v9 { #[cfg(feature = "try-runtime")] fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { let old_claim_permission_count: u64 = Decode::decode(&mut &data[..]).unwrap(); - let new_claim_permission_count = ClaimPermissions::::iter_keys().count(); + let new_claim_permission_count: u64 = ClaimPermissions::::iter_keys().count(); // There should no longer be `PermissionlessAll` claim // permissions in storage. From 0c89e3461ed2b8d3e366115fb90d3b7f359a4e9d Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 28 Feb 2024 17:38:53 +0700 Subject: [PATCH 18/29] cast --- substrate/frame/nomination-pools/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index e67dc74755385..1c3e8993a0fd1 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -164,7 +164,7 @@ mod v9 { #[cfg(feature = "try-runtime")] fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { let old_claim_permission_count: u64 = Decode::decode(&mut &data[..]).unwrap(); - let new_claim_permission_count: u64 = ClaimPermissions::::iter_keys().count(); + let new_claim_permission_count = ClaimPermissions::::iter_keys().count() as u64; // There should no longer be `PermissionlessAll` claim // permissions in storage. From f5a75b19cf59918ce94f8533706a8baa23a19d44 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 29 Feb 2024 12:56:39 +0700 Subject: [PATCH 19/29] audence update --- prdoc/pr_3438.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_3438.prdoc b/prdoc/pr_3438.prdoc index 93e46032e3ef3..da3e0baff73c4 100644 --- a/prdoc/pr_3438.prdoc +++ b/prdoc/pr_3438.prdoc @@ -1,7 +1,7 @@ title: "Pools: Make PermissionlessWithdraw the default claim permission" doc: - - audience: Runtime Dev + - audience: Runtime User description: | Makes permissionless withdrawing the default claim permission, giving any network participant access to claim pool rewards on member's behalf, by default. From 34d4807e93538a4a246fd9034e308d88ee96f9b1 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 13 Mar 2024 10:45:37 +0700 Subject: [PATCH 20/29] remove migration --- .../frame/nomination-pools/src/migration.rs | 68 ------------------- 1 file changed, 68 deletions(-) diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 1c3e8993a0fd1..14410339f59a4 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -27,15 +27,6 @@ use sp_runtime::TryRuntimeError; pub mod versioned { use super::*; - /// v9: Migrates claim permissions with a `PermissionlessAll` value to `PermissionlessWithdraw`. - pub type V8ToV9 = frame_support::migrations::VersionedMigration< - 8, - 9, - v9::VersionUncheckedMigrateV8ToV9, - crate::pallet::Pallet, - ::DbWeight, - >; - /// v8: Adds commission claim permissions to `BondedPools`. pub type V7ToV8 = frame_support::migrations::VersionedMigration< 7, @@ -118,65 +109,6 @@ 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 { - 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(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for VersionUncheckedMigrateV8ToV9 { - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - let claim_permission_count = ClaimPermissions::::iter_keys().count(); - Ok((claim_permission_count as u64).encode()) - } - - fn on_runtime_upgrade() -> Weight { - let mut translates = 0u64; - ClaimPermissions::::translate::(|_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) -> Result<(), TryRuntimeError> { - let old_claim_permission_count: u64 = Decode::decode(&mut &data[..]).unwrap(); - let new_claim_permission_count = ClaimPermissions::::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, *}; From e0891a0890d1b25f44eb3eff087fbfc213d4594c Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 13 Mar 2024 11:01:00 +0700 Subject: [PATCH 21/29] roll back storage version --- substrate/frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index a6a5ff052abce..e24d298e6d928 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1579,7 +1579,7 @@ pub mod pallet { use sp_runtime::Perbill; /// The in-code storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(9); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(8); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] From e39539a5931d67f6879686a82cdacb9d62369bff Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 14 Mar 2024 08:37:38 +0700 Subject: [PATCH 22/29] rm --- polkadot/runtime/westend/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 803d8fdaac23f..93d3ec8f76186 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1710,8 +1710,7 @@ pub mod migrations { Runtime, crate::xcm_config::XcmRouter, GetLegacyLeaseImpl, - >, - pallet_nomination_pools::migration::versioned::V8ToV9, + > ); } From 682b33441a70ccfab3708d39593f3982259966be Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 14 Mar 2024 16:55:04 +0700 Subject: [PATCH 23/29] fmt --- polkadot/runtime/westend/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 93d3ec8f76186..45bbd0260e5e6 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1710,7 +1710,7 @@ pub mod migrations { Runtime, crate::xcm_config::XcmRouter, GetLegacyLeaseImpl, - > + >, ); } From d800fccd8819a0c59b4b3fc654fb345f30bf44fe Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Fri, 15 Mar 2024 12:50:59 +0700 Subject: [PATCH 24/29] amend prdoc --- prdoc/pr_3438.prdoc | 3 --- 1 file changed, 3 deletions(-) diff --git a/prdoc/pr_3438.prdoc b/prdoc/pr_3438.prdoc index da3e0baff73c4..6ceb8b968d96a 100644 --- a/prdoc/pr_3438.prdoc +++ b/prdoc/pr_3438.prdoc @@ -6,8 +6,5 @@ doc: 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 From 35505c65012969dbc1f27a760ff10371eb2d0c64 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 21 Mar 2024 11:39:59 +0700 Subject: [PATCH 25/29] bring back PermissionlessAll --- .../nomination-pools/benchmarking/src/lib.rs | 8 ++++---- substrate/frame/nomination-pools/src/lib.rs | 16 +++++++++------- substrate/frame/nomination-pools/src/tests.rs | 10 ++++++++++ 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/substrate/frame/nomination-pools/benchmarking/src/lib.rs b/substrate/frame/nomination-pools/benchmarking/src/lib.rs index 933104eff93a9..f7df173ec04ea 100644 --- a/substrate/frame/nomination-pools/benchmarking/src/lib.rs +++ b/substrate/frame/nomination-pools/benchmarking/src/lib.rs @@ -285,8 +285,8 @@ frame_benchmarking::benchmarks! { let scenario = ListScenario::::new(origin_weight, true)?; let extra = (scenario.dest_weight - origin_weight).max(CurrencyOf::::minimum_balance()); - // set claim preferences to `PermissionlessCompound` to any account to bond extra on member's behalf. - let _ = Pools::::set_claim_permission(RuntimeOrigin::Signed(scenario.creator1.clone()).into(), ClaimPermission::PermissionlessCompound); + // set claim preferences to `PermissionlessAll` to any account to bond extra on member's behalf. + let _ = Pools::::set_claim_permission(RuntimeOrigin::Signed(scenario.creator1.clone()).into(), ClaimPermission::PermissionlessAll); // transfer exactly `extra` to the depositor of the src pool (1), let reward_account1 = Pools::::create_reward_account(1); @@ -313,9 +313,9 @@ frame_benchmarking::benchmarks! { // Send funds to the reward account of the pool CurrencyOf::::set_balance(&reward_account, ed + origin_weight); - // set claim preferences to `PermissionlessWithdraw` so any account can claim rewards on member's + // set claim preferences to `PermissionlessAll` so any account can claim rewards on member's // behalf. - let _ = Pools::::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessWithdraw); + let _ = Pools::::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessAll); // Sanity check assert_eq!( diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index e24d298e6d928..2382d77ab0007 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -457,6 +457,8 @@ 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 { @@ -467,15 +469,15 @@ impl Default for ClaimPermission { impl ClaimPermission { /// Permissionless compounding of pool rewards is allowed if the current permission is - /// `PermissionlessCompound`. + /// `PermissionlessCompound`, or permissionless. fn can_bond_extra(&self) -> bool { - matches!(self, ClaimPermission::PermissionlessCompound) + matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessCompound) } /// Permissionless payout claiming is allowed if the current permission is - /// `PermissionlessWithdraw`. + /// `PermissionlessWithdraw`, or permissionless. fn can_claim_payout(&self) -> bool { - matches!(self, ClaimPermission::PermissionlessWithdraw) + matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessWithdraw) } } @@ -2632,7 +2634,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 - /// `PermissionlessCompound`. + /// `PermissionlessCompound` or `PermissionlessAll`. #[pallet::call_index(14)] #[pallet::weight( T::WeightInfo::bond_extra_transfer() @@ -2673,8 +2675,8 @@ pub mod pallet { /// `origin` can claim payouts on some pool member `other`'s behalf. /// - /// Pool member `other` must have a `PermissionlessWithdraw` claim permission for this call - /// to be successful. + /// Pool member `other` must have a `PermissionlessWithdraw` or `PermissionlessAll` 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, other: T::AccountId) -> DispatchResult { diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index f0bfd6da2988e..5ebdeea77b9e2 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -2481,10 +2481,20 @@ mod unbond { Error::::MinimumBondNotMet ); + // Make permissionless + 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::::get(20).unwrap().active_points(), 0); assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 20); + assert_eq!( + ClaimPermissions::::get(20), + ClaimPermission::PermissionlessAll + ); }) } From 892e7cab2bb0e197b23d671b6b13967c56769161 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Thu, 21 Mar 2024 11:42:47 +0700 Subject: [PATCH 26/29] add comment --- substrate/frame/nomination-pools/src/tests.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 5ebdeea77b9e2..32b3e9af3cd6d 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -2443,7 +2443,8 @@ mod claim_payout { // when - // Claim permission of `PermissionlessWithdraw` allows payout claiming as default. + // NOTE: Claim permission of `PermissionlessWithdraw` allows payout claiming as default, + // so a claim permission does not need to be set for non-pool members prior to claiming. assert_ok!(Pools::claim_payout_other(RuntimeOrigin::signed(80), 10)); // then From 80499a6d5278b9a79dafa32e4fba8a0ce49345ed Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 1 Apr 2024 16:11:50 +0700 Subject: [PATCH 27/29] add bump --- prdoc/pr_3438.prdoc | 1 + 1 file changed, 1 insertion(+) diff --git a/prdoc/pr_3438.prdoc b/prdoc/pr_3438.prdoc index 6ceb8b968d96a..1ae120e996b47 100644 --- a/prdoc/pr_3438.prdoc +++ b/prdoc/pr_3438.prdoc @@ -8,3 +8,4 @@ doc: crates: - name: pallet-nomination-pools + - bump: minor From abe04caeb284a7dc1e38404085b98af13ac7c0e1 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 1 Apr 2024 16:14:29 +0700 Subject: [PATCH 28/29] prdoc --- prdoc/pr_3438.prdoc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/prdoc/pr_3438.prdoc b/prdoc/pr_3438.prdoc index 1ae120e996b47..8aaf1eb18f190 100644 --- a/prdoc/pr_3438.prdoc +++ b/prdoc/pr_3438.prdoc @@ -8,4 +8,6 @@ doc: crates: - name: pallet-nomination-pools - - bump: minor + bump: minor + - name pallet-nomination-pools-benchmarking + bump: minor From 0b1d09ef26a5e13b39d1c6ad879b3158fd7c83db Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 1 Apr 2024 16:15:26 +0700 Subject: [PATCH 29/29] fix --- prdoc/pr_3438.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_3438.prdoc b/prdoc/pr_3438.prdoc index 8aaf1eb18f190..5f4a0e3d57af7 100644 --- a/prdoc/pr_3438.prdoc +++ b/prdoc/pr_3438.prdoc @@ -9,5 +9,5 @@ doc: crates: - name: pallet-nomination-pools bump: minor - - name pallet-nomination-pools-benchmarking + - name: pallet-nomination-pools-benchmarking bump: minor