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
@@ -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,6 +1947,109 @@ 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
+		/// 
+		/// 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 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 = match state_claim {
+				Some(claim) => {
+					ensure!(!BondedPools::<T>::contains_key(claim), Error::<T>::PoolIdInUse); // create custom Error?
+					ensure!(claim < LastPoolId::<T>::get(), Error::<T>::PoolIdCountExceeded);
+					claim
+				},
+				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
+				},
+			};
+
+			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 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.
 		///
 		/// The dispatch origin of this call must be signed by the pool nominator or the pool

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,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::<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 {