From 0edf4d6a9fdaaba5939f8d554966e7ffcf2f8ef3 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Mon, 3 Oct 2022 13:05:07 +0100 Subject: [PATCH 1/9] initial --- frame/nomination-pools/src/lib.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 28d10ce573401..565078757234d 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -28,7 +28,7 @@ //! //! ## Key terms //! -//! * pool id: A unique identifier of each pool. Set to u12 +//! * pool id: A unique identifier of each pool. Set to u32 //! * bonded pool: Tracks the distribution of actively staked funds. See [`BondedPool`] and //! [`BondedPoolInner`]. //! * reward pool: Tracks rewards earned by actively staked funds. See [`RewardPool`] and @@ -47,7 +47,7 @@ //! exactly the same rules and conditions as a normal staker. Its bond increases or decreases as //! members join, it can `nominate` or `chill`, and might not even earn staking rewards if it is //! not nominating proper validators. -//! * reward account: A similar key-less account, that is set as the `Payee` account fo the bonded +//! * reward account: A similar key-less account, that is set as the `Payee` account for the bonded //! account for all staking rewards. //! //! ## Usage @@ -1943,6 +1943,26 @@ pub mod pallet { Ok(()) } + /* + /// Create a new delegation pool with a previously used pool id + /// + /// # Arguments + /// + /// same as `create` with the inclusion of + /// * `pool_id` - `Option`, if `None` this is similar to `create`. + /// if `Some(claim)` the caller is claiming that `claim` A.K.A PoolId is not in use. + pub fn create_with_pool_id() { + origin: OriginFor, + #[pallet::compact] amount: BalanceOf, + root: AccountIdLookupOf, + nominator: AccountIdLookupOf, + state_toggler: AccountIdLookupOf, + pool_id: Option, + } -> DispatchResult { + + } + */ + /// Nominate on behalf of the pool. /// /// The dispatch origin of this call must be signed by the pool nominator or the pool From 94f61708c5ebf7e2c793ee975772fd08fa33f407 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Wed, 5 Oct 2022 13:09:53 +0100 Subject: [PATCH 2/9] logic check --- frame/nomination-pools/src/lib.rs | 99 +++++++++++++++++++++++++++-- frame/nomination-pools/src/mock.rs | 2 +- frame/nomination-pools/src/tests.rs | 61 +++++++++++++++++- 3 files changed, 153 insertions(+), 9 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 565078757234d..11136ebada738 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1451,6 +1451,10 @@ pub mod pallet { Defensive(DefensiveError), /// Partial unbonding now allowed permissionlessly. PartialUnbondNotAllowedPermissionlessly, + /// Pool id currently in use. + PoolIdInUse, + /// Claim exceeds the last pool id. + PoolIdCountExceeded } #[derive(Encode, Decode, PartialEq, TypeInfo, frame_support::PalletError, RuntimeDebug)] @@ -1943,7 +1947,9 @@ pub mod pallet { Ok(()) } - /* + + // TODO: Benchmark? + /// Doesn't mean that the poolId has been removed from the BondedPools, just means that there is no active depositor and no stake in the pool /// Create a new delegation pool with a previously used pool id /// /// # Arguments @@ -1951,17 +1957,98 @@ pub mod pallet { /// same as `create` with the inclusion of /// * `pool_id` - `Option`, if `None` this is similar to `create`. /// if `Some(claim)` the caller is claiming that `claim` A.K.A PoolId is not in use. - pub fn create_with_pool_id() { + #[pallet::weight(T::WeightInfo::create())] + #[transactional] + pub fn create_with_pool_id( origin: OriginFor, #[pallet::compact] amount: BalanceOf, root: AccountIdLookupOf, nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, - pool_id: Option, - } -> DispatchResult { - + state_claim: Option, + ) -> DispatchResult { + let who = ensure_signed(origin)?; + let root = T::Lookup::lookup(root)?; + let nominator = T::Lookup::lookup(nominator)?; + let state_toggler = T::Lookup::lookup(state_toggler)?; + + ensure!(amount >= Pallet::::depositor_min_bond(), Error::::MinimumBondNotMet); + ensure!( + MaxPools::::get() + .map_or(true, |max_pools| BondedPools::::count() < max_pools), + Error::::MaxPools + ); + ensure!(!PoolMembers::::contains_key(&who), Error::::AccountBelongsToOtherPool); + + let pool_id = match state_claim { + Some(claim) => { + ensure!(!BondedPools::::contains_key(claim), Error::::PoolIdInUse); // create custom Error? + ensure!(claim < LastPoolId::::get(), Error::::PoolIdCountExceeded); + claim + }, + None => { + let inc_pool_id = LastPoolId::::try_mutate::<_, Error, _>(|id| { + *id = id.checked_add(1).ok_or(Error::::OverflowRisk)?; + Ok(*id) + })?; + inc_pool_id + }, + }; + + let mut bonded_pool = BondedPool::::new( + pool_id, + PoolRoles { + root: Some(root), + nominator: Some(nominator), + state_toggler: Some(state_toggler), + depositor: who.clone(), + }, + ); + + bonded_pool.try_inc_members()?; + let points = bonded_pool.try_bond_funds(&who, amount, BondType::Create)?; + + T::Currency::transfer( + &who, + &bonded_pool.reward_account(), + T::Currency::minimum_balance(), + ExistenceRequirement::AllowDeath, + )?; + + PoolMembers::::insert( + who.clone(), + PoolMember:: { + pool_id, + points, + last_recorded_reward_counter: Zero::zero(), + unbonding_eras: Default::default(), + }, + ); + RewardPools::::insert( + pool_id, + RewardPool:: { + last_recorded_reward_counter: Zero::zero(), + last_recorded_total_payouts: Zero::zero(), + total_rewards_claimed: Zero::zero(), + }, + ); + ReversePoolIdLookup::::insert(bonded_pool.bonded_account(), pool_id); + + Self::deposit_event(Event::::Created { depositor: who.clone(), pool_id }); + + Self::deposit_event(Event::::Bonded { + member: who, + pool_id, + bonded: amount, + joined: true, + }); + bonded_pool.put(); + + Ok(()) + // ensure bondedpoools contains that pool_id, this needs a custom error. // THIS should be inside the match statement + + // let result = match Option ( if Some(claim), if None then increment the last pool Id) } - */ /// Nominate on behalf of the pool. /// diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 1b3372dae56ee..4ddbe5a2575dd 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -275,7 +275,7 @@ impl ExtBuilder { let _ = crate::GenesisConfig:: { min_join_bond: MinJoinBondConfig::get(), min_create_bond: 2, - max_pools: Some(2), + max_pools: Some(3), max_members_per_pool: self.max_members_per_pool, max_members: self.max_members, } diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 5074a7ffa695a..cceb0cf1250c0 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4170,9 +4170,21 @@ mod create { }, } .put(); - assert_eq!(MaxPools::::get(), Some(2)); + assert_eq!(MaxPools::::get(), Some(3)); assert_eq!(BondedPools::::count(), 2); + BondedPool:: { + id: 3, + inner: BondedPoolInner { + state: PoolState::Open, + points: 10, + member_counter: 1, + roles: DEFAULT_ROLES, + }, + } + .put(); + assert_eq!(BondedPools::::count(), 3); + // Then assert_noop!( Pools::create(RuntimeOrigin::signed(11), 20, 123, 456, 789), @@ -4181,7 +4193,7 @@ mod create { // Given assert_eq!(PoolMembers::::count(), 1); - MaxPools::::put(3); + MaxPools::::put(4); MaxPoolMembers::::put(1); Balances::make_free_balance_be(&11, 5 + 20); @@ -4198,6 +4210,51 @@ mod create { ); }); } + + #[test] + fn create_with_pool_id_works() { + ExtBuilder::default().build_and_execute(|| { + let ed = Balances::minimum_balance(); + + Balances::make_free_balance_be(&11, StakingMock::minimum_bond() + ed); + assert_ok!(Pools::create( + RuntimeOrigin::signed(11), + StakingMock::minimum_bond(), + 123, + 456, + 789 + )); + + assert_eq!(Balances::free_balance(&11), 0); + // delete the initial pool created, then pool_Id `1` will be free + + assert_noop!(Pools::create_with_pool_id( + RuntimeOrigin::signed(12), + 20, + 234, + 654, + 783, + Some(1) + ), Error::::PoolIdInUse); + + // start dismantling the pool. + assert_ok!(Pools::set_state(RuntimeOrigin::signed(902), 1, PoolState::Destroying)); + assert_ok!(fully_unbond_permissioned(10)); + + CurrentEra::set(3); + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 10)); + + assert_ok!(Pools::create_with_pool_id( + RuntimeOrigin::signed(10), + 20, + 234, + 654, + 783, + Some(1) + )); + }); + + } } mod nominate { From d7b3ad34439f63703c63ba8cb1ae19ad23dde1b4 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Thu, 6 Oct 2022 11:30:32 +0100 Subject: [PATCH 3/9] do_create --- frame/nomination-pools/src/lib.rs | 210 +++++++++++------------------- 1 file changed, 77 insertions(+), 133 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 11136ebada738..d3f032b54ddad 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -279,6 +279,7 @@ use frame_support::{ }, transactional, CloneNoBound, DefaultNoBound, RuntimeDebugNoBound, }; +use frame_system::ensure_signed; use scale_info::TypeInfo; use sp_core::U256; use sp_runtime::{ @@ -1878,78 +1879,14 @@ pub mod pallet { nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, ) -> DispatchResult { - let who = ensure_signed(origin)?; - let root = T::Lookup::lookup(root)?; - let nominator = T::Lookup::lookup(nominator)?; - let state_toggler = T::Lookup::lookup(state_toggler)?; - - ensure!(amount >= Pallet::::depositor_min_bond(), Error::::MinimumBondNotMet); - ensure!( - MaxPools::::get() - .map_or(true, |max_pools| BondedPools::::count() < max_pools), - Error::::MaxPools - ); - ensure!(!PoolMembers::::contains_key(&who), Error::::AccountBelongsToOtherPool); - let pool_id = LastPoolId::::try_mutate::<_, Error, _>(|id| { *id = id.checked_add(1).ok_or(Error::::OverflowRisk)?; Ok(*id) })?; - let mut bonded_pool = BondedPool::::new( - pool_id, - PoolRoles { - root: Some(root), - nominator: Some(nominator), - state_toggler: Some(state_toggler), - depositor: who.clone(), - }, - ); - - bonded_pool.try_inc_members()?; - let points = bonded_pool.try_bond_funds(&who, amount, BondType::Create)?; - - T::Currency::transfer( - &who, - &bonded_pool.reward_account(), - T::Currency::minimum_balance(), - ExistenceRequirement::AllowDeath, - )?; - - PoolMembers::::insert( - who.clone(), - PoolMember:: { - pool_id, - points, - last_recorded_reward_counter: Zero::zero(), - unbonding_eras: Default::default(), - }, - ); - RewardPools::::insert( - pool_id, - RewardPool:: { - last_recorded_reward_counter: Zero::zero(), - last_recorded_total_payouts: Zero::zero(), - total_rewards_claimed: Zero::zero(), - }, - ); - ReversePoolIdLookup::::insert(bonded_pool.bonded_account(), pool_id); - Self::deposit_event(Event::::Created { depositor: who.clone(), pool_id }); - - Self::deposit_event(Event::::Bonded { - member: who, - pool_id, - bonded: amount, - joined: true, - }); - bonded_pool.put(); - - Ok(()) + Self::do_create(origin, amount, root, nominator, state_toggler, pool_id) } - - // TODO: Benchmark? - /// Doesn't mean that the poolId has been removed from the BondedPools, just means that there is no active depositor and no stake in the pool /// Create a new delegation pool with a previously used pool id /// /// # Arguments @@ -1966,23 +1903,10 @@ pub mod pallet { nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, state_claim: Option, - ) -> DispatchResult { - let who = ensure_signed(origin)?; - let root = T::Lookup::lookup(root)?; - let nominator = T::Lookup::lookup(nominator)?; - let state_toggler = T::Lookup::lookup(state_toggler)?; - - ensure!(amount >= Pallet::::depositor_min_bond(), Error::::MinimumBondNotMet); - ensure!( - MaxPools::::get() - .map_or(true, |max_pools| BondedPools::::count() < max_pools), - Error::::MaxPools - ); - ensure!(!PoolMembers::::contains_key(&who), Error::::AccountBelongsToOtherPool); - + ) -> DispatchResult { let pool_id = match state_claim { Some(claim) => { - ensure!(!BondedPools::::contains_key(claim), Error::::PoolIdInUse); // create custom Error? + ensure!(!BondedPools::::contains_key(claim), Error::::PoolIdInUse); ensure!(claim < LastPoolId::::get(), Error::::PoolIdCountExceeded); claim }, @@ -1995,59 +1919,7 @@ pub mod pallet { }, }; - let mut bonded_pool = BondedPool::::new( - pool_id, - PoolRoles { - root: Some(root), - nominator: Some(nominator), - state_toggler: Some(state_toggler), - depositor: who.clone(), - }, - ); - - bonded_pool.try_inc_members()?; - let points = bonded_pool.try_bond_funds(&who, amount, BondType::Create)?; - - T::Currency::transfer( - &who, - &bonded_pool.reward_account(), - T::Currency::minimum_balance(), - ExistenceRequirement::AllowDeath, - )?; - - PoolMembers::::insert( - who.clone(), - PoolMember:: { - pool_id, - points, - last_recorded_reward_counter: Zero::zero(), - unbonding_eras: Default::default(), - }, - ); - RewardPools::::insert( - pool_id, - RewardPool:: { - last_recorded_reward_counter: Zero::zero(), - last_recorded_total_payouts: Zero::zero(), - total_rewards_claimed: Zero::zero(), - }, - ); - ReversePoolIdLookup::::insert(bonded_pool.bonded_account(), pool_id); - - Self::deposit_event(Event::::Created { depositor: who.clone(), pool_id }); - - Self::deposit_event(Event::::Bonded { - member: who, - pool_id, - bonded: amount, - joined: true, - }); - bonded_pool.put(); - - Ok(()) - // ensure bondedpoools contains that pool_id, this needs a custom error. // THIS should be inside the match statement - - // let result = match Option ( if Some(claim), if None then increment the last pool Id) + Self::do_create(origin, amount, root, nominator, state_toggler, pool_id) } /// Nominate on behalf of the pool. @@ -2471,6 +2343,78 @@ impl Pallet { Ok(pending_rewards) } + fn do_create( + origin: T::RuntimeOrigin, + amount: BalanceOf, + root: AccountIdLookupOf, + nominator: AccountIdLookupOf, + state_toggler: AccountIdLookupOf, + pool_id: PoolId, + ) -> DispatchResult { + let who = ensure_signed(origin)?; + let root = T::Lookup::lookup(root)?; + let nominator = T::Lookup::lookup(nominator)?; + let state_toggler = T::Lookup::lookup(state_toggler)?; + + ensure!(amount >= Pallet::::depositor_min_bond(), Error::::MinimumBondNotMet); + ensure!( + MaxPools::::get() + .map_or(true, |max_pools| BondedPools::::count() < max_pools), + Error::::MaxPools + ); + ensure!(!PoolMembers::::contains_key(&who), Error::::AccountBelongsToOtherPool); + let mut bonded_pool = BondedPool::::new( + pool_id, + PoolRoles { + root: Some(root), + nominator: Some(nominator), + state_toggler: Some(state_toggler), + depositor: who.clone(), + }, + ); + + bonded_pool.try_inc_members()?; + let points = bonded_pool.try_bond_funds(&who, amount, BondType::Create)?; + + T::Currency::transfer( + &who, + &bonded_pool.reward_account(), + T::Currency::minimum_balance(), + ExistenceRequirement::AllowDeath, + )?; + + PoolMembers::::insert( + who.clone(), + PoolMember:: { + pool_id, + points, + last_recorded_reward_counter: Zero::zero(), + unbonding_eras: Default::default(), + }, + ); + RewardPools::::insert( + pool_id, + RewardPool:: { + last_recorded_reward_counter: Zero::zero(), + last_recorded_total_payouts: Zero::zero(), + total_rewards_claimed: Zero::zero(), + }, + ); + ReversePoolIdLookup::::insert(bonded_pool.bonded_account(), pool_id); + + Self::deposit_event(Event::::Created { depositor: who.clone(), pool_id }); + + Self::deposit_event(Event::::Bonded { + member: who, + pool_id, + bonded: amount, + joined: true, + }); + bonded_pool.put(); + + Ok(()) + } + /// Ensure the correctness of the state of this pallet. /// /// This should be valid before or after each state transition of this pallet. From f282d905104e6f6dceed88e2cacee3c97b443e19 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Fri, 7 Oct 2022 18:03:41 +0100 Subject: [PATCH 4/9] cargo fmt --- frame/nomination-pools/src/lib.rs | 17 ++++++++--------- frame/nomination-pools/src/tests.rs | 27 +++++++++++---------------- 2 files changed, 19 insertions(+), 25 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index ca40762763e83..f133f2724da28 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1455,7 +1455,7 @@ pub mod pallet { /// Pool id currently in use. PoolIdInUse, /// Claim exceeds the last pool id. - PoolIdCountExceeded + PoolIdCountExceeded, } #[derive(Encode, Decode, PartialEq, TypeInfo, frame_support::PalletError, RuntimeDebug)] @@ -1888,12 +1888,12 @@ pub mod pallet { } /// Create a new delegation pool with a previously used pool id - /// + /// /// # Arguments - /// + /// /// same as `create` with the inclusion of - /// * `pool_id` - `Option`, if `None` this is similar to `create`. - /// if `Some(claim)` the caller is claiming that `claim` A.K.A PoolId is not in use. + /// * `pool_id` - `Option`, if `None` this is similar to `create`. if `Some(claim)` + /// the caller is claiming that `claim` A.K.A PoolId is not in use. #[pallet::weight(T::WeightInfo::create())] #[transactional] pub fn create_with_pool_id( @@ -1903,7 +1903,7 @@ pub mod pallet { nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, state_claim: Option, - ) -> DispatchResult { + ) -> DispatchResult { let pool_id = match state_claim { Some(claim) => { ensure!(!BondedPools::::contains_key(claim), Error::::PoolIdInUse); @@ -2349,7 +2349,7 @@ impl Pallet { root: AccountIdLookupOf, nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, - pool_id: PoolId, + pool_id: PoolId, ) -> DispatchResult { let who = ensure_signed(origin)?; let root = T::Lookup::lookup(root)?; @@ -2358,8 +2358,7 @@ impl Pallet { ensure!(amount >= Pallet::::depositor_min_bond(), Error::::MinimumBondNotMet); ensure!( - MaxPools::::get() - .map_or(true, |max_pools| BondedPools::::count() < max_pools), + MaxPools::::get().map_or(true, |max_pools| BondedPools::::count() < max_pools), Error::::MaxPools ); ensure!(!PoolMembers::::contains_key(&who), Error::::AccountBelongsToOtherPool); diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index cceb0cf1250c0..b489d9b61d038 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4224,18 +4224,14 @@ mod create { 456, 789 )); - + assert_eq!(Balances::free_balance(&11), 0); // delete the initial pool created, then pool_Id `1` will be free - assert_noop!(Pools::create_with_pool_id( - RuntimeOrigin::signed(12), - 20, - 234, - 654, - 783, - Some(1) - ), Error::::PoolIdInUse); + assert_noop!( + Pools::create_with_pool_id(RuntimeOrigin::signed(12), 20, 234, 654, 783, Some(1)), + Error::::PoolIdInUse + ); // start dismantling the pool. assert_ok!(Pools::set_state(RuntimeOrigin::signed(902), 1, PoolState::Destroying)); @@ -4245,16 +4241,15 @@ mod create { assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 10)); assert_ok!(Pools::create_with_pool_id( - RuntimeOrigin::signed(10), - 20, - 234, - 654, - 783, + RuntimeOrigin::signed(10), + 20, + 234, + 654, + 783, Some(1) )); }); - - } + } } mod nominate { From 1a11c5943a1a7a1a995042804d53aabb39346313 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Tue, 11 Oct 2022 22:55:00 +0100 Subject: [PATCH 5/9] fixes --- frame/nomination-pools/src/lib.rs | 27 +++++++-------------------- frame/nomination-pools/src/tests.rs | 16 +++++++--------- 2 files changed, 14 insertions(+), 29 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index f133f2724da28..fd88e3009bf50 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -28,7 +28,7 @@ //! //! ## Key terms //! -//! * pool id: A unique identifier of each pool. Set to u32 +//! * pool id: A unique identifier of each pool. Set to u32. //! * bonded pool: Tracks the distribution of actively staked funds. See [`BondedPool`] and //! [`BondedPoolInner`]. //! * reward pool: Tracks rewards earned by actively staked funds. See [`RewardPool`] and @@ -1454,8 +1454,8 @@ pub mod pallet { PartialUnbondNotAllowedPermissionlessly, /// Pool id currently in use. PoolIdInUse, - /// Claim exceeds the last pool id. - PoolIdCountExceeded, + /// Pool id provided is not correct/usable. + InvalidPoolId, } #[derive(Encode, Decode, PartialEq, TypeInfo, frame_support::PalletError, RuntimeDebug)] @@ -1892,8 +1892,7 @@ pub mod pallet { /// # Arguments /// /// same as `create` with the inclusion of - /// * `pool_id` - `Option`, if `None` this is similar to `create`. if `Some(claim)` - /// the caller is claiming that `claim` A.K.A PoolId is not in use. + /// * `pool_id` - `A valid PoolId. #[pallet::weight(T::WeightInfo::create())] #[transactional] pub fn create_with_pool_id( @@ -1902,22 +1901,10 @@ pub mod pallet { root: AccountIdLookupOf, nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, - state_claim: Option, + pool_id: PoolId, ) -> DispatchResult { - let pool_id = match state_claim { - Some(claim) => { - ensure!(!BondedPools::::contains_key(claim), Error::::PoolIdInUse); - ensure!(claim < LastPoolId::::get(), Error::::PoolIdCountExceeded); - claim - }, - None => { - let inc_pool_id = LastPoolId::::try_mutate::<_, Error, _>(|id| { - *id = id.checked_add(1).ok_or(Error::::OverflowRisk)?; - Ok(*id) - })?; - inc_pool_id - }, - }; + ensure!(!BondedPools::::contains_key(pool_id), Error::::PoolIdInUse); + ensure!(pool_id < LastPoolId::::get(), Error::::InvalidPoolId); Self::do_create(origin, amount, root, nominator, state_toggler, pool_id) } diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index b489d9b61d038..e01dd63dc5e0a 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4229,10 +4229,15 @@ mod create { // delete the initial pool created, then pool_Id `1` will be free assert_noop!( - Pools::create_with_pool_id(RuntimeOrigin::signed(12), 20, 234, 654, 783, Some(1)), + Pools::create_with_pool_id(RuntimeOrigin::signed(12), 20, 234, 654, 783, 1), Error::::PoolIdInUse ); + assert_noop!( + Pools::create_with_pool_id(RuntimeOrigin::signed(12), 20, 234, 654, 783, 3), + Error::::InvalidPoolId + ); + // start dismantling the pool. assert_ok!(Pools::set_state(RuntimeOrigin::signed(902), 1, PoolState::Destroying)); assert_ok!(fully_unbond_permissioned(10)); @@ -4240,14 +4245,7 @@ mod create { CurrentEra::set(3); assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(10), 10, 10)); - assert_ok!(Pools::create_with_pool_id( - RuntimeOrigin::signed(10), - 20, - 234, - 654, - 783, - Some(1) - )); + assert_ok!(Pools::create_with_pool_id(RuntimeOrigin::signed(10), 20, 234, 654, 783, 1)); }); } } From 77c43d1df80d1ac3318072bbe227cc6189f5f8f3 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Mon, 24 Oct 2022 10:47:52 +0100 Subject: [PATCH 6/9] ensure_signed --- frame/nomination-pools/src/lib.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index fd88e3009bf50..3957b6edcedcb 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1879,12 +1879,14 @@ pub mod pallet { nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, ) -> DispatchResult { + let depositor = ensure_signed(origin)?; + let pool_id = LastPoolId::::try_mutate::<_, Error, _>(|id| { *id = id.checked_add(1).ok_or(Error::::OverflowRisk)?; Ok(*id) })?; - Self::do_create(origin, amount, root, nominator, state_toggler, pool_id) + Self::do_create(depositor, amount, root, nominator, state_toggler, pool_id) } /// Create a new delegation pool with a previously used pool id @@ -1903,10 +1905,12 @@ pub mod pallet { state_toggler: AccountIdLookupOf, pool_id: PoolId, ) -> DispatchResult { + let depositor = ensure_signed(origin)?; + ensure!(!BondedPools::::contains_key(pool_id), Error::::PoolIdInUse); ensure!(pool_id < LastPoolId::::get(), Error::::InvalidPoolId); - Self::do_create(origin, amount, root, nominator, state_toggler, pool_id) + Self::do_create(depositor, amount, root, nominator, state_toggler, pool_id) } /// Nominate on behalf of the pool. @@ -2331,14 +2335,13 @@ impl Pallet { } fn do_create( - origin: T::RuntimeOrigin, + who: T::AccountId, amount: BalanceOf, root: AccountIdLookupOf, nominator: AccountIdLookupOf, state_toggler: AccountIdLookupOf, pool_id: PoolId, ) -> DispatchResult { - let who = ensure_signed(origin)?; let root = T::Lookup::lookup(root)?; let nominator = T::Lookup::lookup(nominator)?; let state_toggler = T::Lookup::lookup(state_toggler)?; From 643db1c1357e57ec6caa52cd0360162d679fb088 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Tue, 25 Oct 2022 23:44:12 +0100 Subject: [PATCH 7/9] update --- frame/nomination-pools/src/mock.rs | 2 +- frame/nomination-pools/src/tests.rs | 20 +++++--------------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 4ddbe5a2575dd..1b3372dae56ee 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -275,7 +275,7 @@ impl ExtBuilder { let _ = crate::GenesisConfig:: { min_join_bond: MinJoinBondConfig::get(), min_create_bond: 2, - max_pools: Some(3), + max_pools: Some(2), max_members_per_pool: self.max_members_per_pool, max_members: self.max_members, } diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index e01dd63dc5e0a..b800acbf41852 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4170,21 +4170,9 @@ mod create { }, } .put(); - assert_eq!(MaxPools::::get(), Some(3)); + assert_eq!(MaxPools::::get(), Some(2)); assert_eq!(BondedPools::::count(), 2); - BondedPool:: { - id: 3, - inner: BondedPoolInner { - state: PoolState::Open, - points: 10, - member_counter: 1, - roles: DEFAULT_ROLES, - }, - } - .put(); - assert_eq!(BondedPools::::count(), 3); - // Then assert_noop!( Pools::create(RuntimeOrigin::signed(11), 20, 123, 456, 789), @@ -4193,7 +4181,7 @@ mod create { // Given assert_eq!(PoolMembers::::count(), 1); - MaxPools::::put(4); + MaxPools::::put(3); MaxPoolMembers::::put(1); Balances::make_free_balance_be(&11, 5 + 20); @@ -4215,7 +4203,7 @@ mod create { fn create_with_pool_id_works() { ExtBuilder::default().build_and_execute(|| { let ed = Balances::minimum_balance(); - + Balances::make_free_balance_be(&11, StakingMock::minimum_bond() + ed); assert_ok!(Pools::create( RuntimeOrigin::signed(11), @@ -4228,6 +4216,8 @@ mod create { assert_eq!(Balances::free_balance(&11), 0); // delete the initial pool created, then pool_Id `1` will be free + + assert_noop!( Pools::create_with_pool_id(RuntimeOrigin::signed(12), 20, 234, 654, 783, 1), Error::::PoolIdInUse From 260932c9a6a5e741d940739d2a5e9a02b73bc296 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Tue, 25 Oct 2022 23:54:00 +0100 Subject: [PATCH 8/9] cargo fmt --- frame/nomination-pools/src/tests.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index b800acbf41852..562371c856b7d 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -4203,7 +4203,7 @@ mod create { fn create_with_pool_id_works() { ExtBuilder::default().build_and_execute(|| { let ed = Balances::minimum_balance(); - + Balances::make_free_balance_be(&11, StakingMock::minimum_bond() + ed); assert_ok!(Pools::create( RuntimeOrigin::signed(11), @@ -4216,8 +4216,6 @@ mod create { assert_eq!(Balances::free_balance(&11), 0); // delete the initial pool created, then pool_Id `1` will be free - - assert_noop!( Pools::create_with_pool_id(RuntimeOrigin::signed(12), 20, 234, 654, 783, 1), Error::::PoolIdInUse From 268eb8f1e46f851ea8dbbc3be6f6879ae11f845a Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Wed, 26 Oct 2022 00:44:46 +0100 Subject: [PATCH 9/9] remove unused import --- frame/nomination-pools/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 3957b6edcedcb..778aac7c4ede0 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -279,7 +279,6 @@ use frame_support::{ }, transactional, CloneNoBound, DefaultNoBound, RuntimeDebugNoBound, }; -use frame_system::ensure_signed; use scale_info::TypeInfo; use sp_core::U256; use sp_runtime::{