nomination-pools: allow pool-ids to be reused

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
@@ -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::{
@@ -1451,6 +1452,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)]
@@ -1874,73 +1879,47 @@ pub mod pallet {
 			nominator: AccountIdLookupOf<T>,
 			state_toggler: AccountIdLookupOf<T>,
 		) -> 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::<T>::depositor_min_bond(), Error::<T>::MinimumBondNotMet);
-			ensure!(
-				MaxPools::<T>::get()
-					.map_or(true, |max_pools| BondedPools::<T>::count() < max_pools),
-				Error::<T>::MaxPools
-			);
-			ensure!(!PoolMembers::<T>::contains_key(&who), Error::<T>::AccountBelongsToOtherPool);
-
 			let pool_id = LastPoolId::<T>::try_mutate::<_, Error<T>, _>(|id| {
 				*id = id.checked_add(1).ok_or(Error::<T>::OverflowRisk)?;
 				Ok(*id)
 			})?;
-			let mut bonded_pool = BondedPool::<T>::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,
-			)?;
+			Self::do_create(origin, amount, root, nominator, state_toggler, pool_id)
+		}
 
-			PoolMembers::<T>::insert(
-				who.clone(),
-				PoolMember::<T> {
-					pool_id,
-					points,
-					last_recorded_reward_counter: Zero::zero(),
-					unbonding_eras: Default::default(),
+		/// Create a new delegation pool with a previously used pool id
+		///
+		/// # Arguments
+		///
+		/// same as `create` with the inclusion of
+		/// * `pool_id` - `Option<PoolId>`, 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(
+			origin: OriginFor<T>,
+			#[pallet::compact] amount: BalanceOf<T>,
+			root: AccountIdLookupOf<T>,
+			nominator: AccountIdLookupOf<T>,
+			state_toggler: AccountIdLookupOf<T>,
+			state_claim: Option<PoolId>,
+		) -> DispatchResult {
+			let pool_id = match state_claim {
+				Some(claim) => {
+					ensure!(!BondedPools::<T>::contains_key(claim), Error::<T>::PoolIdInUse);
+					ensure!(claim < LastPoolId::<T>::get(), Error::<T>::PoolIdCountExceeded);
+					claim
 				},
-			);
-			RewardPools::<T>::insert(
-				pool_id,
-				RewardPool::<T> {
-					last_recorded_reward_counter: Zero::zero(),
-					last_recorded_total_payouts: Zero::zero(),
-					total_rewards_claimed: Zero::zero(),
+				None => {
+					let inc_pool_id = LastPoolId::<T>::try_mutate::<_, Error<T>, _>(|id| {
+						*id = id.checked_add(1).ok_or(Error::<T>::OverflowRisk)?;
+						Ok(*id)
+					})?;
+					inc_pool_id
 				},
-			);
-			ReversePoolIdLookup::<T>::insert(bonded_pool.bonded_account(), pool_id);
-
-			Self::deposit_event(Event::<T>::Created { depositor: who.clone(), pool_id });
-
-			Self::deposit_event(Event::<T>::Bonded {
-				member: who,
-				pool_id,
-				bonded: amount,
-				joined: true,
-			});
-			bonded_pool.put();
+			};
 
-			Ok(())
+			Self::do_create(origin, amount, root, nominator, state_toggler, pool_id)
 		}
 
 		/// Nominate on behalf of the pool.
@@ -2364,6 +2343,77 @@ impl<T: Config> Pallet<T> {
 		Ok(pending_rewards)
 	}
 
+	fn do_create(
+		origin: T::RuntimeOrigin,
+		amount: BalanceOf<T>,
+		root: AccountIdLookupOf<T>,
+		nominator: AccountIdLookupOf<T>,
+		state_toggler: AccountIdLookupOf<T>,
+		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::<T>::depositor_min_bond(), Error::<T>::MinimumBondNotMet);
+		ensure!(
+			MaxPools::<T>::get().map_or(true, |max_pools| BondedPools::<T>::count() < max_pools),
+			Error::<T>::MaxPools
+		);
+		ensure!(!PoolMembers::<T>::contains_key(&who), Error::<T>::AccountBelongsToOtherPool);
+		let mut bonded_pool = BondedPool::<T>::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::<T>::insert(
+			who.clone(),
+			PoolMember::<T> {
+				pool_id,
+				points,
+				last_recorded_reward_counter: Zero::zero(),
+				unbonding_eras: Default::default(),
+			},
+		);
+		RewardPools::<T>::insert(
+			pool_id,
+			RewardPool::<T> {
+				last_recorded_reward_counter: Zero::zero(),
+				last_recorded_total_payouts: Zero::zero(),
+				total_rewards_claimed: Zero::zero(),
+			},
+		);
+		ReversePoolIdLookup::<T>::insert(bonded_pool.bonded_account(), pool_id);
+
+		Self::deposit_event(Event::<T>::Created { depositor: who.clone(), pool_id });
+
+		Self::deposit_event(Event::<T>::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.

frame/nomination-pools/src/mock.rs

@@ -275,7 +275,7 @@ impl ExtBuilder {
 		let _ = crate::GenesisConfig::<Runtime> {
 			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,
 		}

frame/nomination-pools/src/tests.rs

@@ -4170,9 +4170,21 @@ mod create {
 				},
 			}
 			.put();
-			assert_eq!(MaxPools::<Runtime>::get(), Some(2));
+			assert_eq!(MaxPools::<Runtime>::get(), Some(3));
 			assert_eq!(BondedPools::<Runtime>::count(), 2);
 
+			BondedPool::<Runtime> {
+				id: 3,
+				inner: BondedPoolInner {
+					state: PoolState::Open,
+					points: 10,
+					member_counter: 1,
+					roles: DEFAULT_ROLES,
+				},
+			}
+			.put();
+			assert_eq!(BondedPools::<Runtime>::count(), 3);
+
 			// Then
 			assert_noop!(
 				Pools::create(RuntimeOrigin::signed(11), 20, 123, 456, 789),
@@ -4181,7 +4193,7 @@ mod create {
 
 			// Given
 			assert_eq!(PoolMembers::<Runtime>::count(), 1);
-			MaxPools::<Runtime>::put(3);
+			MaxPools::<Runtime>::put(4);
 			MaxPoolMembers::<Runtime>::put(1);
 			Balances::make_free_balance_be(&11, 5 + 20);
 
@@ -4198,6 +4210,46 @@ 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::<Runtime>::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 {