From fe1473983bdb98aab0ee32e341211d845e28ec4d Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 9 May 2022 12:40:28 +0100 Subject: [PATCH 01/10] add MinPointsToBalance to pools --- .../nomination-pools/benchmarking/src/lib.rs | 4 +- frame/nomination-pools/src/lib.rs | 18 +++++++- frame/nomination-pools/src/mock.rs | 1 + frame/nomination-pools/src/tests.rs | 44 ++++++++++++------- 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 7ae6338e6304a..86882e55dbe36 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -28,7 +28,7 @@ use frame_support::{ensure, traits::Get}; use frame_system::RawOrigin as Origin; use pallet_nomination_pools::{ BalanceOf, BondExtra, BondedPoolInner, BondedPools, ConfigOp, MaxPoolMembers, - MaxPoolMembersPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, Pallet as Pools, + MaxPoolMembersPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, MinPointsToBalance, Pallet as Pools, PoolMembers, PoolRoles, PoolState, RewardPools, SubPoolsStorage, }; use sp_runtime::traits::{Bounded, Zero}; @@ -622,6 +622,7 @@ frame_benchmarking::benchmarks! { ConfigOp::Set(BalanceOf::::max_value()), ConfigOp::Set(u32::MAX), ConfigOp::Set(u32::MAX), + ConfigOp::Set(u32::MAX), ConfigOp::Set(u32::MAX) ) verify { assert_eq!(MinJoinBond::::get(), BalanceOf::::max_value()); @@ -629,6 +630,7 @@ frame_benchmarking::benchmarks! { assert_eq!(MaxPools::::get(), Some(u32::MAX)); assert_eq!(MaxPoolMembers::::get(), Some(u32::MAX)); assert_eq!(MaxPoolMembersPerPool::::get(), Some(u32::MAX)); + assert_eq!(MinPointsToBalance::::get(), u32::MAX); } impl_benchmark_test_suite!( diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 8b26bad6c7df8..456d2b3184b1d 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -709,11 +709,15 @@ impl BondedPool { // Pool points can inflate relative to balance, but only if the pool is slashed. // If we cap the ratio of points:balance so one cannot join a pool that has been slashed // 90%, - ensure!(points_to_balance_ratio_floor < 10u32.into(), Error::::OverflowRisk); + ensure!( + points_to_balance_ratio_floor < MinPointsToBalance::::get().into(), + Error::::OverflowRisk + ); // while restricting the balance to 1/10th of max total issuance, let next_bonded_balance = bonded_balance.saturating_add(new_funds); ensure!( - next_bonded_balance < BalanceOf::::max_value().div(10u32.into()), + next_bonded_balance < + BalanceOf::::max_value().div(MinPointsToBalance::::get().into()), Error::::OverflowRisk ); // then we can be decently confident the bonding pool points will not overflow @@ -1136,6 +1140,10 @@ pub mod pallet { #[pallet::storage] pub type SubPoolsStorage = CountedStorageMap<_, Twox64Concat, PoolId, SubPools>; + /// The minimum points to balance ratio for members to join a pool. + #[pallet::storage] + pub type MinPointsToBalance = StorageValue<_, u32, ValueQuery>; + /// Metadata for the pool. #[pallet::storage] pub type Metadata = @@ -1155,6 +1163,7 @@ pub mod pallet { pub max_pools: Option, pub max_members_per_pool: Option, pub max_members: Option, + pub min_points_to_balance: u32, } #[cfg(feature = "std")] @@ -1166,6 +1175,7 @@ pub mod pallet { max_pools: Some(16), max_members_per_pool: Some(32), max_members: Some(16 * 32), + min_points_to_balance: 10, } } } @@ -1184,6 +1194,7 @@ pub mod pallet { if let Some(max_members) = self.max_members { MaxPoolMembers::::put(max_members); } + MinPointsToBalance::::put(self.min_points_to_balance); } } @@ -1798,6 +1809,7 @@ pub mod pallet { /// * `max_pools` - Set [`MaxPools`]. /// * `max_members` - Set [`MaxPoolMembers`]. /// * `max_members_per_pool` - Set [`MaxPoolMembersPerPool`]. + /// * `min_points_to_balance` - Set [`MinPointsToBalance`], #[pallet::weight(T::WeightInfo::set_configs())] pub fn set_configs( origin: OriginFor, @@ -1806,6 +1818,7 @@ pub mod pallet { max_pools: ConfigOp, max_members: ConfigOp, max_members_per_pool: ConfigOp, + min_points_to_balance: ConfigOp, ) -> DispatchResult { ensure_root(origin)?; @@ -1824,6 +1837,7 @@ pub mod pallet { config_op_exp!(MaxPools::, max_pools); config_op_exp!(MaxPoolMembers::, max_members); config_op_exp!(MaxPoolMembersPerPool::, max_members_per_pool); + config_op_exp!(MinPointsToBalance::, min_points_to_balance); Ok(()) } diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 5498496965adb..3f20f8c870515 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -222,6 +222,7 @@ impl ExtBuilder { max_pools: Some(2), max_members_per_pool: Some(3), max_members: Some(4), + min_points_to_balance: 10, } .assimilate_storage(&mut storage); diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index ecda162c6bdbc..8e87ef47d0ff5 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -186,23 +186,31 @@ mod bonded_pool { }, }; + let min_points_to_balance: u128 = MinPointsToBalance::::get().into(); + // Simulate a 100% slashed pool StakingMock::set_bonded_balance(pool.bonded_account(), 0); assert_noop!(pool.ok_to_join(0), Error::::OverflowRisk); - // Simulate a 89% - StakingMock::set_bonded_balance(pool.bonded_account(), 11); + // Simulate a slashed pool at `MinPointsToBalance` + 1 slashed pool + StakingMock::set_bonded_balance( + pool.bonded_account(), + min_points_to_balance.saturating_add(1).into(), + ); assert_ok!(pool.ok_to_join(0)); - // Simulate a 90% slashed pool - StakingMock::set_bonded_balance(pool.bonded_account(), 10); + // Simulate a slashed pool at `MinPointsToBalance` + StakingMock::set_bonded_balance( + pool.bonded_account(), + min_points_to_balance, + ); assert_noop!(pool.ok_to_join(0), Error::::OverflowRisk); - StakingMock::set_bonded_balance(pool.bonded_account(), Balance::MAX / 10); - // New bonded balance would be over 1/10th of Balance type + StakingMock::set_bonded_balance(pool.bonded_account(), Balance::MAX / min_points_to_balance); + // New bonded balance would be over threshold of Balance type assert_noop!(pool.ok_to_join(0), Error::::OverflowRisk); // and a sanity check - StakingMock::set_bonded_balance(pool.bonded_account(), Balance::MAX / 10 - 1); + StakingMock::set_bonded_balance(pool.bonded_account(), Balance::MAX / min_points_to_balance - 1); assert_ok!(pool.ok_to_join(0)); }); } @@ -488,22 +496,24 @@ mod join { }, ); - // Force the points:balance ratio to 100/10 - StakingMock::set_bonded_balance(Pools::create_bonded_account(123), 10); + // Force the points:balance ratio to `MinPointsToBalance` (100/10) + let min_points_to_balance: u128 = MinPointsToBalance::::get().into(); + + StakingMock::set_bonded_balance(Pools::create_bonded_account(123), min_points_to_balance); assert_noop!(Pools::join(Origin::signed(11), 420, 123), Error::::OverflowRisk); - StakingMock::set_bonded_balance(Pools::create_bonded_account(123), Balance::MAX / 10); - // Balance is gt 1/10 of Balance::MAX + StakingMock::set_bonded_balance(Pools::create_bonded_account(123), Balance::MAX / min_points_to_balance); + // Balance needs to be gt Balance::MAX / `MinPointsToBalance` assert_noop!(Pools::join(Origin::signed(11), 5, 123), Error::::OverflowRisk); - StakingMock::set_bonded_balance(Pools::create_bonded_account(1), 10); + StakingMock::set_bonded_balance(Pools::create_bonded_account(1), min_points_to_balance); // Cannot join a pool that isn't open unsafe_set_state(123, PoolState::Blocked).unwrap(); - assert_noop!(Pools::join(Origin::signed(11), 10, 123), Error::::NotOpen); + assert_noop!(Pools::join(Origin::signed(11), min_points_to_balance, 123), Error::::NotOpen); unsafe_set_state(123, PoolState::Destroying).unwrap(); - assert_noop!(Pools::join(Origin::signed(11), 10, 123), Error::::NotOpen); + assert_noop!(Pools::join(Origin::signed(11), min_points_to_balance, 123), Error::::NotOpen); // Given MinJoinBond::::put(100); @@ -3331,12 +3341,14 @@ mod set_configs { ConfigOp::Set(3u32), ConfigOp::Set(4u32), ConfigOp::Set(5u32), + ConfigOp::Set(10u32), )); assert_eq!(MinJoinBond::::get(), 1); assert_eq!(MinCreateBond::::get(), 2); assert_eq!(MaxPools::::get(), Some(3)); assert_eq!(MaxPoolMembers::::get(), Some(4)); assert_eq!(MaxPoolMembersPerPool::::get(), Some(5)); + assert_eq!(MinPointsToBalance::::get(), 10); // Noop does nothing assert_storage_noop!(assert_ok!(Pools::set_configs( @@ -3346,6 +3358,7 @@ mod set_configs { ConfigOp::Noop, ConfigOp::Noop, ConfigOp::Noop, + ConfigOp::Noop, ))); // Removing works @@ -3355,7 +3368,8 @@ mod set_configs { ConfigOp::Remove, ConfigOp::Remove, ConfigOp::Remove, - ConfigOp::Remove + ConfigOp::Remove, + ConfigOp::Remove, )); assert_eq!(MinJoinBond::::get(), 0); assert_eq!(MinCreateBond::::get(), 0); From 5a75504c740c9bd1f4392764f75e838d49a959c3 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 9 May 2022 12:52:10 +0100 Subject: [PATCH 02/10] add min_points_to_balance to benchmark mock --- frame/nomination-pools/benchmarking/src/mock.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index d98bcb542f514..2fa0b215c2439 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -194,6 +194,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { max_pools: Some(3), max_members_per_pool: Some(3), max_members: Some(3 * 3), + min_points_to_balance: 10, } .assimilate_storage(&mut storage); sp_io::TestExternalities::from(storage) From 3c76e56c1b04846640126091254c50517210b915 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 9 May 2022 12:53:33 +0100 Subject: [PATCH 03/10] fmt --- .../nomination-pools/benchmarking/src/lib.rs | 4 +-- frame/nomination-pools/src/tests.rs | 33 ++++++++++++++----- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 86882e55dbe36..2dfd300273bfc 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -28,8 +28,8 @@ use frame_support::{ensure, traits::Get}; use frame_system::RawOrigin as Origin; use pallet_nomination_pools::{ BalanceOf, BondExtra, BondedPoolInner, BondedPools, ConfigOp, MaxPoolMembers, - MaxPoolMembersPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, MinPointsToBalance, Pallet as Pools, - PoolMembers, PoolRoles, PoolState, RewardPools, SubPoolsStorage, + MaxPoolMembersPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, MinPointsToBalance, + Pallet as Pools, PoolMembers, PoolRoles, PoolState, RewardPools, SubPoolsStorage, }; use sp_runtime::traits::{Bounded, Zero}; use sp_staking::{EraIndex, StakingInterface}; diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 8e87ef47d0ff5..aaaaa1ac86a23 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -200,17 +200,20 @@ mod bonded_pool { assert_ok!(pool.ok_to_join(0)); // Simulate a slashed pool at `MinPointsToBalance` + StakingMock::set_bonded_balance(pool.bonded_account(), min_points_to_balance); + assert_noop!(pool.ok_to_join(0), Error::::OverflowRisk); + StakingMock::set_bonded_balance( pool.bonded_account(), - min_points_to_balance, + Balance::MAX / min_points_to_balance, ); - assert_noop!(pool.ok_to_join(0), Error::::OverflowRisk); - - StakingMock::set_bonded_balance(pool.bonded_account(), Balance::MAX / min_points_to_balance); // New bonded balance would be over threshold of Balance type assert_noop!(pool.ok_to_join(0), Error::::OverflowRisk); // and a sanity check - StakingMock::set_bonded_balance(pool.bonded_account(), Balance::MAX / min_points_to_balance - 1); + StakingMock::set_bonded_balance( + pool.bonded_account(), + Balance::MAX / min_points_to_balance - 1, + ); assert_ok!(pool.ok_to_join(0)); }); } @@ -499,10 +502,16 @@ mod join { // Force the points:balance ratio to `MinPointsToBalance` (100/10) let min_points_to_balance: u128 = MinPointsToBalance::::get().into(); - StakingMock::set_bonded_balance(Pools::create_bonded_account(123), min_points_to_balance); + StakingMock::set_bonded_balance( + Pools::create_bonded_account(123), + min_points_to_balance, + ); assert_noop!(Pools::join(Origin::signed(11), 420, 123), Error::::OverflowRisk); - StakingMock::set_bonded_balance(Pools::create_bonded_account(123), Balance::MAX / min_points_to_balance); + StakingMock::set_bonded_balance( + Pools::create_bonded_account(123), + Balance::MAX / min_points_to_balance, + ); // Balance needs to be gt Balance::MAX / `MinPointsToBalance` assert_noop!(Pools::join(Origin::signed(11), 5, 123), Error::::OverflowRisk); @@ -510,10 +519,16 @@ mod join { // Cannot join a pool that isn't open unsafe_set_state(123, PoolState::Blocked).unwrap(); - assert_noop!(Pools::join(Origin::signed(11), min_points_to_balance, 123), Error::::NotOpen); + assert_noop!( + Pools::join(Origin::signed(11), min_points_to_balance, 123), + Error::::NotOpen + ); unsafe_set_state(123, PoolState::Destroying).unwrap(); - assert_noop!(Pools::join(Origin::signed(11), min_points_to_balance, 123), Error::::NotOpen); + assert_noop!( + Pools::join(Origin::signed(11), min_points_to_balance, 123), + Error::::NotOpen + ); // Given MinJoinBond::::put(100); From c33d16a14b72faa594ea5662dee5f2000f60caa9 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 9 May 2022 14:06:41 +0100 Subject: [PATCH 04/10] comments Co-authored-by: Oliver Tale-Yazdi --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 456d2b3184b1d..87c307b3e5245 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1140,7 +1140,7 @@ pub mod pallet { #[pallet::storage] pub type SubPoolsStorage = CountedStorageMap<_, Twox64Concat, PoolId, SubPools>; - /// The minimum points to balance ratio for members to join a pool. + /// The minimum points to balance ratio that must be maintained for the pool to be `open`. #[pallet::storage] pub type MinPointsToBalance = StorageValue<_, u32, ValueQuery>; From 4f2776388488888e94e0c6b1faf99662308fbb82 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 9 May 2022 14:52:23 +0100 Subject: [PATCH 05/10] check min_points_to_balance.is_zero --- frame/nomination-pools/src/lib.rs | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 456d2b3184b1d..b35e4b4757fb0 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -706,20 +706,24 @@ impl BondedPool { // We checked for zero above .div(bonded_balance); + let min_points_to_balance = MinPointsToBalance::::get(); + // Pool points can inflate relative to balance, but only if the pool is slashed. // If we cap the ratio of points:balance so one cannot join a pool that has been slashed - // 90%, - ensure!( - points_to_balance_ratio_floor < MinPointsToBalance::::get().into(), - Error::::OverflowRisk - ); - // while restricting the balance to 1/10th of max total issuance, - let next_bonded_balance = bonded_balance.saturating_add(new_funds); - ensure!( - next_bonded_balance < - BalanceOf::::max_value().div(MinPointsToBalance::::get().into()), - Error::::OverflowRisk - ); + // by `min_points_to_balance`%, if not zero. + + if !min_points_to_balance.is_zero() { + ensure!( + points_to_balance_ratio_floor < min_points_to_balance.into(), + Error::::OverflowRisk + ); + // while restricting the balance to `min_points_to_balance` of max total issuance, + let next_bonded_balance = bonded_balance.saturating_add(new_funds); + ensure!( + next_bonded_balance < BalanceOf::::max_value().div(min_points_to_balance.into()), + Error::::OverflowRisk + ); + } // then we can be decently confident the bonding pool points will not overflow // `BalanceOf`. Note that these are just heuristics. @@ -1851,7 +1855,6 @@ pub mod pallet { 2 * sp_std::mem::size_of::>(), "bit-length of the reward points must be at least twice as much as balance" ); - assert!( T::StakingInterface::bonding_duration() < TotalUnbondingPools::::get(), "There must be more unbonding pools then the bonding duration / From eb64fb42fa0ca525628aebc2a0a1c746ca03a557 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 9 May 2022 17:14:43 +0100 Subject: [PATCH 06/10] comment --- frame/nomination-pools/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index fecb5d09af3bd..fea7b1e41a476 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1145,6 +1145,10 @@ pub mod pallet { pub type SubPoolsStorage = CountedStorageMap<_, Twox64Concat, PoolId, SubPools>; /// The minimum points to balance ratio that must be maintained for the pool to be `open`. + /// This is important in the event slashing takes place and the pools points to balance ratio + /// becomes disproportional. + /// For a value of 10, the threshold would be a 10:1 ratio of pool points to pool balance. + /// such a scenario would also be the equivalent of the pool being slashed 90%. #[pallet::storage] pub type MinPointsToBalance = StorageValue<_, u32, ValueQuery>; From 53f9ad60c3875c8bd98587866b4e887cba9fa4b2 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Mon, 9 May 2022 17:16:33 +0100 Subject: [PATCH 07/10] comment --- frame/nomination-pools/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index fea7b1e41a476..5a8c4c5289016 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1145,10 +1145,10 @@ pub mod pallet { pub type SubPoolsStorage = CountedStorageMap<_, Twox64Concat, PoolId, SubPools>; /// The minimum points to balance ratio that must be maintained for the pool to be `open`. - /// This is important in the event slashing takes place and the pools points to balance ratio + /// This is important in the event slashing takes place and the pool points to balance ratio /// becomes disproportional. - /// For a value of 10, the threshold would be a 10:1 ratio of pool points to pool balance. - /// such a scenario would also be the equivalent of the pool being slashed 90%. + /// For a value of 10, the threshold would be a pool points to pool balance ratio of 10:1. + /// Such a scenario would also be the equivalent of the pool being 90% slashed. #[pallet::storage] pub type MinPointsToBalance = StorageValue<_, u32, ValueQuery>; From 2759aba5f6bd9a6eafa20f71659472c95b38349a Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 11 May 2022 17:50:04 +0100 Subject: [PATCH 08/10] storage to constant --- bin/node/runtime/src/lib.rs | 2 + .../nomination-pools/benchmarking/src/lib.rs | 6 +-- .../nomination-pools/benchmarking/src/mock.rs | 3 +- frame/nomination-pools/src/lib.rs | 54 +++++++++---------- frame/nomination-pools/src/mock.rs | 3 +- frame/nomination-pools/src/tests.rs | 8 +-- 6 files changed, 34 insertions(+), 42 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index fee288cb2b7f8..b7cfed5ee63ab 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -708,6 +708,7 @@ impl pallet_bags_list::Config for Runtime { parameter_types! { pub const PostUnbondPoolsWindow: u32 = 4; pub const NominationPoolsPalletId: PalletId = PalletId(*b"py/nopls"); + pub const MinPointsToBalance: u32 = 10; } use sp_runtime::traits::Convert; @@ -735,6 +736,7 @@ impl pallet_nomination_pools::Config for Runtime { type MaxMetadataLen = ConstU32<256>; type MaxUnbonding = ConstU32<8>; type PalletId = NominationPoolsPalletId; + type MinPointsToBalance = MinPointsToBalance; } parameter_types! { diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 2dfd300273bfc..a0dee5986ae55 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -28,8 +28,8 @@ use frame_support::{ensure, traits::Get}; use frame_system::RawOrigin as Origin; use pallet_nomination_pools::{ BalanceOf, BondExtra, BondedPoolInner, BondedPools, ConfigOp, MaxPoolMembers, - MaxPoolMembersPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, MinPointsToBalance, - Pallet as Pools, PoolMembers, PoolRoles, PoolState, RewardPools, SubPoolsStorage, + MaxPoolMembersPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, Pallet as Pools, + PoolMembers, PoolRoles, PoolState, RewardPools, SubPoolsStorage, }; use sp_runtime::traits::{Bounded, Zero}; use sp_staking::{EraIndex, StakingInterface}; @@ -623,14 +623,12 @@ frame_benchmarking::benchmarks! { ConfigOp::Set(u32::MAX), ConfigOp::Set(u32::MAX), ConfigOp::Set(u32::MAX), - ConfigOp::Set(u32::MAX) ) verify { assert_eq!(MinJoinBond::::get(), BalanceOf::::max_value()); assert_eq!(MinCreateBond::::get(), BalanceOf::::max_value()); assert_eq!(MaxPools::::get(), Some(u32::MAX)); assert_eq!(MaxPoolMembers::::get(), Some(u32::MAX)); assert_eq!(MaxPoolMembersPerPool::::get(), Some(u32::MAX)); - assert_eq!(MinPointsToBalance::::get(), u32::MAX); } impl_benchmark_test_suite!( diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index 2fa0b215c2439..eb884869f6d32 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -144,6 +144,7 @@ impl Convert for U256ToBalance { parameter_types! { pub static PostUnbondingPoolsWindow: u32 = 10; pub const PoolsPalletId: PalletId = PalletId(*b"py/nopls"); + pub const MinPointsToBalance: u32 = 10; } impl pallet_nomination_pools::Config for Runtime { @@ -157,6 +158,7 @@ impl pallet_nomination_pools::Config for Runtime { type MaxMetadataLen = ConstU32<256>; type MaxUnbonding = ConstU32<8>; type PalletId = PoolsPalletId; + type MinPointsToBalance = MinPointsToBalance; } impl crate::Config for Runtime {} @@ -194,7 +196,6 @@ pub fn new_test_ext() -> sp_io::TestExternalities { max_pools: Some(3), max_members_per_pool: Some(3), max_members: Some(3 * 3), - min_points_to_balance: 10, } .assimilate_storage(&mut storage); sp_io::TestExternalities::from(storage) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 5a8c4c5289016..649feec24acd0 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -706,24 +706,22 @@ impl BondedPool { // We checked for zero above .div(bonded_balance); - let min_points_to_balance = MinPointsToBalance::::get(); + let min_points_to_balance = T::MinPointsToBalance::get(); // Pool points can inflate relative to balance, but only if the pool is slashed. // If we cap the ratio of points:balance so one cannot join a pool that has been slashed // by `min_points_to_balance`%, if not zero. + ensure!( + points_to_balance_ratio_floor < min_points_to_balance.into(), + Error::::OverflowRisk + ); + // while restricting the balance to `min_points_to_balance` of max total issuance, + let next_bonded_balance = bonded_balance.saturating_add(new_funds); + ensure!( + next_bonded_balance < BalanceOf::::max_value().div(min_points_to_balance.into()), + Error::::OverflowRisk + ); - if !min_points_to_balance.is_zero() { - ensure!( - points_to_balance_ratio_floor < min_points_to_balance.into(), - Error::::OverflowRisk - ); - // while restricting the balance to `min_points_to_balance` of max total issuance, - let next_bonded_balance = bonded_balance.saturating_add(new_funds); - ensure!( - next_bonded_balance < BalanceOf::::max_value().div(min_points_to_balance.into()), - Error::::OverflowRisk - ); - } // then we can be decently confident the bonding pool points will not overflow // `BalanceOf`. Note that these are just heuristics. @@ -1071,6 +1069,14 @@ pub mod pallet { #[pallet::constant] type PalletId: Get; + /// The minimum points to balance ratio that must be maintained for the pool to be `open`. + /// This is important in the event slashing takes place and the pool points to balance ratio + /// becomes disproportional. + /// For a value of 10, the threshold would be a pool points to pool balance ratio of 10:1. + /// Such a scenario would also be the equivalent of the pool being 90% slashed. + #[pallet::constant] + type MinPointsToBalance: Get; + /// Infallible method for converting `Currency::Balance` to `U256`. type BalanceToU256: Convert, U256>; @@ -1144,14 +1150,6 @@ pub mod pallet { #[pallet::storage] pub type SubPoolsStorage = CountedStorageMap<_, Twox64Concat, PoolId, SubPools>; - /// The minimum points to balance ratio that must be maintained for the pool to be `open`. - /// This is important in the event slashing takes place and the pool points to balance ratio - /// becomes disproportional. - /// For a value of 10, the threshold would be a pool points to pool balance ratio of 10:1. - /// Such a scenario would also be the equivalent of the pool being 90% slashed. - #[pallet::storage] - pub type MinPointsToBalance = StorageValue<_, u32, ValueQuery>; - /// Metadata for the pool. #[pallet::storage] pub type Metadata = @@ -1171,7 +1169,6 @@ pub mod pallet { pub max_pools: Option, pub max_members_per_pool: Option, pub max_members: Option, - pub min_points_to_balance: u32, } #[cfg(feature = "std")] @@ -1183,7 +1180,6 @@ pub mod pallet { max_pools: Some(16), max_members_per_pool: Some(32), max_members: Some(16 * 32), - min_points_to_balance: 10, } } } @@ -1202,7 +1198,6 @@ pub mod pallet { if let Some(max_members) = self.max_members { MaxPoolMembers::::put(max_members); } - MinPointsToBalance::::put(self.min_points_to_balance); } } @@ -1807,7 +1802,7 @@ pub mod pallet { Ok(()) } - /// Update configurations for the nomination pools. The origin must for this call must be + /// Update configurations for the nomination pools. The origin for this call must be /// Root. /// /// # Arguments @@ -1817,7 +1812,6 @@ pub mod pallet { /// * `max_pools` - Set [`MaxPools`]. /// * `max_members` - Set [`MaxPoolMembers`]. /// * `max_members_per_pool` - Set [`MaxPoolMembersPerPool`]. - /// * `min_points_to_balance` - Set [`MinPointsToBalance`], #[pallet::weight(T::WeightInfo::set_configs())] pub fn set_configs( origin: OriginFor, @@ -1826,7 +1820,6 @@ pub mod pallet { max_pools: ConfigOp, max_members: ConfigOp, max_members_per_pool: ConfigOp, - min_points_to_balance: ConfigOp, ) -> DispatchResult { ensure_root(origin)?; @@ -1845,8 +1838,6 @@ pub mod pallet { config_op_exp!(MaxPools::, max_pools); config_op_exp!(MaxPoolMembers::, max_members); config_op_exp!(MaxPoolMembersPerPool::, max_members_per_pool); - config_op_exp!(MinPointsToBalance::, min_points_to_balance); - Ok(()) } } @@ -1854,6 +1845,10 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { fn integrity_test() { + assert!( + T::MinPointsToBalance::get() > 0, + "Minimum points to balance ratio must be greater than 0" + ); assert!( sp_std::mem::size_of::() >= 2 * sp_std::mem::size_of::>(), @@ -2217,7 +2212,6 @@ impl Pallet { sum_unbonding_balance ); } - Ok(()) } diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 3f20f8c870515..1dadd7e00c528 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -162,6 +162,7 @@ parameter_types! { pub static MaxMetadataLen: u32 = 2; pub static CheckLevel: u8 = 255; pub const PoolsPalletId: PalletId = PalletId(*b"py/nopls"); + pub const MinPointsToBalance: u32 = 10; } impl pools::Config for Runtime { type Event = Event; @@ -174,6 +175,7 @@ impl pools::Config for Runtime { type PalletId = PoolsPalletId; type MaxMetadataLen = MaxMetadataLen; type MaxUnbonding = MaxUnbonding; + type MinPointsToBalance = MinPointsToBalance; } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; @@ -222,7 +224,6 @@ impl ExtBuilder { max_pools: Some(2), max_members_per_pool: Some(3), max_members: Some(4), - min_points_to_balance: 10, } .assimilate_storage(&mut storage); diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index aaaaa1ac86a23..807019dbab20a 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -186,7 +186,7 @@ mod bonded_pool { }, }; - let min_points_to_balance: u128 = MinPointsToBalance::::get().into(); + let min_points_to_balance: u128 = MinPointsToBalance::get().into(); // Simulate a 100% slashed pool StakingMock::set_bonded_balance(pool.bonded_account(), 0); @@ -500,7 +500,7 @@ mod join { ); // Force the points:balance ratio to `MinPointsToBalance` (100/10) - let min_points_to_balance: u128 = MinPointsToBalance::::get().into(); + let min_points_to_balance: u128 = MinPointsToBalance::get().into(); StakingMock::set_bonded_balance( Pools::create_bonded_account(123), @@ -3356,14 +3356,12 @@ mod set_configs { ConfigOp::Set(3u32), ConfigOp::Set(4u32), ConfigOp::Set(5u32), - ConfigOp::Set(10u32), )); assert_eq!(MinJoinBond::::get(), 1); assert_eq!(MinCreateBond::::get(), 2); assert_eq!(MaxPools::::get(), Some(3)); assert_eq!(MaxPoolMembers::::get(), Some(4)); assert_eq!(MaxPoolMembersPerPool::::get(), Some(5)); - assert_eq!(MinPointsToBalance::::get(), 10); // Noop does nothing assert_storage_noop!(assert_ok!(Pools::set_configs( @@ -3373,7 +3371,6 @@ mod set_configs { ConfigOp::Noop, ConfigOp::Noop, ConfigOp::Noop, - ConfigOp::Noop, ))); // Removing works @@ -3384,7 +3381,6 @@ mod set_configs { ConfigOp::Remove, ConfigOp::Remove, ConfigOp::Remove, - ConfigOp::Remove, )); assert_eq!(MinJoinBond::::get(), 0); assert_eq!(MinCreateBond::::get(), 0); From 96c431227b7b35e1cce9ea27fcc8ad0f16a3bc36 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Wed, 11 May 2022 18:03:19 +0100 Subject: [PATCH 09/10] fix --- frame/nomination-pools/benchmarking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index b4ec7337b2ad2..aa4c093dcf0d4 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -622,7 +622,7 @@ frame_benchmarking::benchmarks! { ConfigOp::Set(BalanceOf::::max_value()), ConfigOp::Set(u32::MAX), ConfigOp::Set(u32::MAX), - ConfigOp::Set(u32::MAX), + ConfigOp::Set(u32::MAX) ) verify { assert_eq!(MinJoinBond::::get(), BalanceOf::::max_value()); assert_eq!(MinCreateBond::::get(), BalanceOf::::max_value()); From f92074953f3d58957177e29fe4ba4065d4bb6ec4 Mon Sep 17 00:00:00 2001 From: Ross Bulat Date: Fri, 13 May 2022 12:27:24 +0100 Subject: [PATCH 10/10] comment --- frame/nomination-pools/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 2e774a8bc1bd7..7665843d301df 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1073,10 +1073,10 @@ pub mod pallet { #[pallet::constant] type PalletId: Get; - /// The minimum points to balance ratio that must be maintained for the pool to be `open`. - /// This is important in the event slashing takes place and the pool points to balance ratio - /// becomes disproportional. - /// For a value of 10, the threshold would be a pool points to pool balance ratio of 10:1. + /// The minimum pool points-to-balance ratio that must be maintained for it to be `open`. + /// This is important in the event slashing takes place and the pool's points-to-balance + /// ratio becomes disproportional. + /// For a value of 10, the threshold would be a pool points-to-balance ratio of 10:1. /// Such a scenario would also be the equivalent of the pool being 90% slashed. #[pallet::constant] type MinPointsToBalance: Get;