From 7f6761a226bfadfc063c9a8127e54633a37f2587 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 3 Jul 2023 13:08:13 +0100 Subject: [PATCH 1/4] add CommissionExceedsGlobalMaximum --- frame/nomination-pools/src/lib.rs | 10 ++++++++ frame/nomination-pools/src/tests.rs | 39 +++++++++++++++++++---------- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 66c5f786f1e25..fbee831a7217d 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -749,6 +749,10 @@ impl Commission { None => None, Some((commission, payee)) => { ensure!(!self.throttling(commission), Error::::CommissionChangeThrottled); + ensure!( + commission <= &GlobalMaxCommission::::get().unwrap_or(Bounded::max_value()), + Error::::CommissionExceedsGlobalMaximum + ); ensure!( self.max.map_or(true, |m| commission <= &m), Error::::CommissionExceedsMaximum @@ -773,6 +777,10 @@ impl Commission { /// updated to the new maximum. This will also register a `throttle_from` update. /// A `PoolCommissionUpdated` event is triggered if `current.0` is updated. fn try_update_max(&mut self, pool_id: PoolId, new_max: Perbill) -> DispatchResult { + ensure!( + new_max <= GlobalMaxCommission::::get().unwrap_or(Bounded::max_value()), + Error::::CommissionExceedsGlobalMaximum + ); if let Some(old) = self.max.as_mut() { if new_max > *old { return Err(Error::::MaxCommissionRestricted.into()) @@ -1824,6 +1832,8 @@ pub mod pallet { MaxCommissionRestricted, /// The supplied commission exceeds the max allowed commission. CommissionExceedsMaximum, + /// The supplied commission exceeds global maximum commission. + CommissionExceedsGlobalMaximum, /// Not enough blocks have surpassed since the last commission update. CommissionChangeThrottled, /// The submitted changes to commission change rate are not allowed. diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 49a0596587706..576be3e14928a 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -6601,24 +6601,31 @@ mod commission { // top up the commission payee account to existential deposit let _ = Balances::deposit_creating(&2, 5); - // Set a commission pool 1 to 100%, with a payee set to `2` - assert_ok!(Pools::set_commission( - RuntimeOrigin::signed(900), - bonded_pool.id, - Some((Perbill::from_percent(100), 2)), - )); + // Set a commission pool 1 to 100% fails. + assert_noop!( + Pools::set_commission( + RuntimeOrigin::signed(900), + bonded_pool.id, + Some((Perbill::from_percent(100), 2)), + ), + Error::::CommissionExceedsGlobalMaximum + ); assert_eq!( pool_events_since_last_call(), vec![ Event::Created { depositor: 10, pool_id: 1 }, Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, - Event::PoolCommissionUpdated { - pool_id: 1, - current: Some((Perbill::from_percent(100), 2)) - } ] ); + // Set pool commission to 90% and then set global max commission to 80%. + assert_ok!(Pools::set_commission( + RuntimeOrigin::signed(900), + bonded_pool.id, + Some((Perbill::from_percent(90), 2)), + )); + GlobalMaxCommission::::set(Some(Perbill::from_percent(80))); + // The pool earns 10 points deposit_rewards(10); @@ -6630,11 +6637,17 @@ mod commission { &mut reward_pool )); - // Confirm the commission was only 9 points out of 10 points, and the payout was 1 out - // of 10 points, reflecting the 90% global max commission. + // Confirm the commission was only 8 points out of 10 points, and the payout was 2 out + // of 10 points, reflecting the 80% global max commission. assert_eq!( pool_events_since_last_call(), - vec![Event::PaidOut { member: 10, pool_id: 1, payout: 1 },] + vec![ + Event::PoolCommissionUpdated { + pool_id: 1, + current: Some((Perbill::from_percent(90), 2)) + }, + Event::PaidOut { member: 10, pool_id: 1, payout: 2 }, + ] ); }) } From df3b059d5cee6cd78c5b198f9d45a760e41c8e02 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 3 Jul 2023 13:10:47 +0100 Subject: [PATCH 2/4] rename test --- frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 576be3e14928a..0e31773d62db5 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -6591,7 +6591,7 @@ mod commission { } #[test] - fn global_max_prevents_100_percent_commission_payout() { + fn global_max_caps_max_commission_payout() { ExtBuilder::default().build_and_execute(|| { // Note: GlobalMaxCommission is set at 90%. From 844748692504fefd01758bf6a9ccbd1dbdf33aa5 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 3 Jul 2023 14:38:57 +0100 Subject: [PATCH 3/4] amend set_commission_max_works_with_error_tests --- frame/nomination-pools/src/tests.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 0e31773d62db5..afca3d715b86f 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5915,6 +5915,13 @@ mod commission { Error::::DoesNotHavePermission ); + // Cannot set max commission above GlobalMaxCommission + assert_noop!(Pools::set_commission_max( + RuntimeOrigin::signed(900), + 1, + Perbill::from_percent(100) + ), Error::::CommissionExceedsGlobalMaximum); + // Set a max commission commission pool 1 to 80% assert_ok!(Pools::set_commission_max( RuntimeOrigin::signed(900), From 459166278a4b5e9eea1a9a4d412e67c236625e5d Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 3 Jul 2023 14:40:44 +0100 Subject: [PATCH 4/4] fmt --- frame/nomination-pools/src/tests.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index afca3d715b86f..ac8fa5c4dbc0c 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -5916,11 +5916,14 @@ mod commission { ); // Cannot set max commission above GlobalMaxCommission - assert_noop!(Pools::set_commission_max( - RuntimeOrigin::signed(900), - 1, - Perbill::from_percent(100) - ), Error::::CommissionExceedsGlobalMaximum); + assert_noop!( + Pools::set_commission_max( + RuntimeOrigin::signed(900), + 1, + Perbill::from_percent(100) + ), + Error::::CommissionExceedsGlobalMaximum + ); // Set a max commission commission pool 1 to 80% assert_ok!(Pools::set_commission_max(