From 34427113f7ce12daff71932f70b65a137bbd308e Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 1 Apr 2024 16:35:36 +0700 Subject: [PATCH] Pools: Make `PermissionlessWithdraw` the default claim permission (#3438) Related Issue https://github.com/paritytech/polkadot-sdk/issues/3398 This PR makes permissionless withdrawing the default option, giving any network participant access to claim pool rewards on member's behalf. Of course, members can still opt out of this by setting a `Permissioned` claim permission. Permissionless claiming has been a part of the nomination pool pallet for around 9 months now, with very limited uptake (~4% of total pool members). 1.6% of pool members are using `PermissionlessAll`, strongly suggesting it is not wanted - it is too ambiguous and doesn't provide guidance to claimers. Stakers expect rewards to be claimed on their behalf by default - I have expanded upon this in detail within the [accompanying issue's discussion](https://github.com/paritytech/polkadot-sdk/issues/3398). Other protocols have this behaviour, whereby staking rewards are received without the staker having to take any action. From this perspective, permissionless claiming is not intuitive for pool members. As evidence of this, over 150,000 DOT is currently unclaimed on Polkadot, and is growing at a non-linear rate. --- prdoc/pr_3438.prdoc | 13 +++++++ .../nomination-pools/benchmarking/src/lib.rs | 4 +- substrate/frame/nomination-pools/src/lib.rs | 31 ++++++++------- substrate/frame/nomination-pools/src/tests.rs | 39 +++++++------------ 4 files changed, 45 insertions(+), 42 deletions(-) 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..5f4a0e3d57af7 --- /dev/null +++ b/prdoc/pr_3438.prdoc @@ -0,0 +1,13 @@ +title: "Pools: Make PermissionlessWithdraw the default claim permission" + +doc: + - 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. + +crates: + - name: pallet-nomination-pools + bump: minor + - name: pallet-nomination-pools-benchmarking + bump: minor diff --git a/substrate/frame/nomination-pools/benchmarking/src/lib.rs b/substrate/frame/nomination-pools/benchmarking/src/lib.rs index 48d7dae29ef03..f7df173ec04ea 100644 --- a/substrate/frame/nomination-pools/benchmarking/src/lib.rs +++ b/substrate/frame/nomination-pools/benchmarking/src/lib.rs @@ -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 f29a49a2b1b3d..23501cd89d209 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -461,22 +461,26 @@ pub enum ClaimPermission { 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`, or permissionless. fn can_bond_extra(&self) -> bool { matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessCompound) } + /// Permissionless payout claiming is allowed if the current permission is + /// `PermissionlessWithdraw`, or permissionless. fn can_claim_payout(&self) -> bool { matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessWithdraw) } } -impl Default for ClaimPermission { - fn default() -> Self { - Self::Permissioned - } -} - /// A member in a pool. #[derive( Encode, @@ -2630,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 - /// `PermissionlessAll` or `PermissionlessCompound`. + /// `PermissionlessCompound` or `PermissionlessAll`. #[pallet::call_index(14)] #[pallet::weight( T::WeightInfo::bond_extra_transfer() @@ -2648,15 +2652,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( @@ -2666,16 +2665,18 @@ pub mod pallet { let who = ensure_signed(origin)?; ensure!(PoolMembers::::contains_key(&who), Error::::PoolMemberNotFound); + ClaimPermissions::::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` 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 8fb2b41b88a19..32b3e9af3cd6d 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -2441,16 +2441,10 @@ 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 - )); + // 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 @@ -2489,7 +2483,6 @@ mod unbond { ); // Make permissionless - assert_eq!(ClaimPermissions::::get(10), ClaimPermission::Permissioned); assert_ok!(Pools::set_claim_permission( RuntimeOrigin::signed(20), ClaimPermission::PermissionlessAll @@ -4563,12 +4556,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 +4784,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 +4803,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); }); } @@ -5224,7 +5213,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);