[NPoS] Fix for Reward Deficit in the pool (#1255)

closes #158. partially addresses #226. Instead of fragile calculation of current balance by looking at `free balance - ED`, Nomination Pool now freezes ED in the pool reward account to restrict an account from going below minimum balance. This also has a nice side effect that if ED changes, we know how much is the imbalance in ED frozen in the pool and the current required ED. A pool operator can diligently top up the pool with the deficit in ED or vice versa, withdraw the excess they transferred to the pool. ## Notable changes - New call `adjust_pool_deposit`: Allows to top up the deficit or withdraw the excess deposited funds to the pool. - Uses Fungible trait (instead of Currency trait). Since NP was not doing any locking/reserving previously, no migration is needed for this. - One time migration of freezing ED from each of the existing pools (not very PoV friendly but fine for relay chain).
paritytech · Sep 29, 2023 · f820dc0 · f820dc0
1 parent 0691c91
commit f820dc0
Show file tree

Hide file tree

Showing 13 changed files with 2,355 additions and 1,833 deletions.
diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs
@@ -292,9 +292,9 @@ impl pallet_balances::Config for Runtime {
 	type ReserveIdentifier = [u8; 8];
 	type WeightInfo = weights::pallet_balances::WeightInfo<Runtime>;
 	type RuntimeHoldReason = RuntimeHoldReason;
-	type FreezeIdentifier = ();
+	type FreezeIdentifier = RuntimeFreezeReason;
+	type MaxFreezes = ConstU32<1>;
 	type MaxHolds = ConstU32<1>;
-	type MaxFreezes = ConstU32<0>;
 }
 
 parameter_types! {
@@ -1311,6 +1311,7 @@ impl pallet_nomination_pools::Config for Runtime {
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = weights::pallet_nomination_pools::WeightInfo<Self>;
 	type Currency = Balances;
+	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type RewardCounter = FixedU128;
 	type BalanceToU256 = BalanceToU256;
 	type U256ToBalance = U256ToBalance;
@@ -1398,7 +1399,7 @@ construct_runtime! {
 		VoterList: pallet_bags_list::<Instance1>::{Pallet, Call, Storage, Event<T>} = 25,
 
 		// Nomination pools for staking.
-		NominationPools: pallet_nomination_pools::{Pallet, Call, Storage, Event<T>, Config<T>} = 29,
+		NominationPools: pallet_nomination_pools::{Pallet, Call, Storage, Event<T>, Config<T>, FreezeReason} = 29,
 
 		// Fast unstake pallet: extension to staking.
 		FastUnstake: pallet_fast_unstake = 30,
@@ -1505,6 +1506,7 @@ pub mod migrations {
 		UpgradeSessionKeys,
 		parachains_configuration::migration::v9::MigrateToV9<Runtime>,
 		paras_registrar::migration::VersionCheckedMigrateToV1<Runtime, ()>,
+		pallet_nomination_pools::migration::versioned_migrations::V5toV6<Runtime>,
 		pallet_referenda::migration::v1::MigrateV0ToV1<Runtime, ()>,
 	);
 }

diff --git a/polkadot/runtime/westend/src/weights/pallet_nomination_pools.rs b/polkadot/runtime/westend/src/weights/pallet_nomination_pools.rs
diff --git a/prdoc/pr_1255.prdoc b/prdoc/pr_1255.prdoc
@@ -0,0 +1,22 @@
+# Schema: Parity PR Documentation Schema (prdoc)
+# See doc at https://github.com/paritytech/prdoc
+
+title: Fix for Reward Deficit in the pool
+
+doc:
+  - audience: Core Dev
+    description: Instead of fragile calculation of current balance by looking at free balance - ED, Nomination Pool now freezes ED in the pool reward account to restrict an account from going below minimum balance. This also has a nice side effect that if ED changes, we know how much is the imbalance in ED frozen in the pool and the current required ED. A pool operator can diligently top up the pool with the deficit in ED or vice versa, withdraw the excess they transferred to the pool.
+    notes:
+      - Introduces new call `adjust_pool_deposit` that allows to top up the deficit or withdraw the excess deposit for the pool.
+      - Switch to using Fungible trait from Currency trait.
+
+migrations:
+  db: []
+
+  runtime:
+    - { pallet: "pallet-nomination-pools", description: "One time migration of freezing ED from each of the existing pools."}
+
+crates:
+  - name: pallet-nomination-pools
+
+host_functions: []
diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs
@@ -521,8 +521,8 @@ impl pallet_balances::Config for Runtime {
 	type ExistentialDeposit = ExistentialDeposit;
 	type AccountStore = frame_system::Pallet<Runtime>;
 	type WeightInfo = pallet_balances::weights::SubstrateWeight<Runtime>;
-	type FreezeIdentifier = ();
-	type MaxFreezes = ();
+	type FreezeIdentifier = RuntimeFreezeReason;
+	type MaxFreezes = ConstU32<1>;
 	type RuntimeHoldReason = RuntimeHoldReason;
 	type MaxHolds = ConstU32<2>;
 }
@@ -882,6 +882,7 @@ impl pallet_nomination_pools::Config for Runtime {
 	type WeightInfo = ();
 	type RuntimeEvent = RuntimeEvent;
 	type Currency = Balances;
+	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type RewardCounter = FixedU128;
 	type BalanceToU256 = BalanceToU256;
 	type U256ToBalance = U256ToBalance;

diff --git a/substrate/frame/nomination-pools/benchmarking/src/lib.rs b/substrate/frame/nomination-pools/benchmarking/src/lib.rs
@@ -27,7 +27,10 @@ use frame_benchmarking::v1::{account, whitelist_account};
 use frame_election_provider_support::SortedListProvider;
 use frame_support::{
 	assert_ok, ensure,
-	traits::{Currency, Get},
+	traits::{
+		fungible::{Inspect, Mutate, Unbalanced},
+		Get,
+	},
 };
 use frame_system::RawOrigin as RuntimeOrigin;
 use pallet_nomination_pools::{
@@ -67,7 +70,7 @@ fn create_funded_user_with_balance<T: pallet_nomination_pools::Config>(
 	balance: BalanceOf<T>,
 ) -> T::AccountId {
 	let user = account(string, n, USER_SEED);
-	T::Currency::make_free_balance_be(&user, balance);
+	T::Currency::set_balance(&user, balance);
 	user
 }
 
@@ -148,8 +151,7 @@ impl<T: Config> ListScenario<T> {
 		);
 
 		// Burn the entire issuance.
-		let i = CurrencyOf::<T>::burn(CurrencyOf::<T>::total_issuance());
-		sp_std::mem::forget(i);
+		CurrencyOf::<T>::set_total_issuance(Zero::zero());
 
 		// Create accounts with the origin weight
 		let (pool_creator1, pool_origin1) =
@@ -206,7 +208,7 @@ impl<T: Config> ListScenario<T> {
 
 		let joiner: T::AccountId = account("joiner", USER_SEED, 0);
 		self.origin1_member = Some(joiner.clone());
-		CurrencyOf::<T>::make_free_balance_be(&joiner, amount * 2u32.into());
+		CurrencyOf::<T>::set_balance(&joiner, amount * 2u32.into());
 
 		let original_bonded = T::Staking::active_stake(&self.origin1).unwrap();
 
@@ -254,7 +256,7 @@ frame_benchmarking::benchmarks! {
 		whitelist_account!(joiner);
 	}: _(RuntimeOrigin::Signed(joiner.clone()), max_additional, 1)
 	verify {
-		assert_eq!(CurrencyOf::<T>::free_balance(&joiner), joiner_free - max_additional);
+		assert_eq!(CurrencyOf::<T>::balance(&joiner), joiner_free - max_additional);
 		assert_eq!(
 			T::Staking::active_stake(&scenario.origin1).unwrap(),
 			scenario.dest_weight
@@ -289,7 +291,7 @@ frame_benchmarking::benchmarks! {
 		// transfer exactly `extra` to the depositor of the src pool (1),
 		let reward_account1 = Pools::<T>::create_reward_account(1);
 		assert!(extra >= CurrencyOf::<T>::minimum_balance());
-		CurrencyOf::<T>::deposit_creating(&reward_account1, extra);
+		let _ = CurrencyOf::<T>::mint_into(&reward_account1, extra);
 
 	}: _(RuntimeOrigin::Signed(claimer), T::Lookup::unlookup(scenario.creator1.clone()), BondExtra::Rewards)
 	verify {
@@ -309,27 +311,27 @@ frame_benchmarking::benchmarks! {
 		let reward_account = Pools::<T>::create_reward_account(1);
 
 		// Send funds to the reward account of the pool
-		CurrencyOf::<T>::make_free_balance_be(&reward_account, ed + origin_weight);
+		CurrencyOf::<T>::set_balance(&reward_account, ed + origin_weight);
 
 		// set claim preferences to `PermissionlessAll` so any account can claim rewards on member's
 		// behalf.
 		let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessAll);
 
 		// Sanity check
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&depositor),
+			CurrencyOf::<T>::balance(&depositor),
 			origin_weight
 		);
 
 		whitelist_account!(depositor);
 	}:claim_payout_other(RuntimeOrigin::Signed(claimer), depositor.clone())
 	verify {
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&depositor),
+			CurrencyOf::<T>::balance(&depositor),
 			origin_weight + commission * origin_weight
 		);
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&reward_account),
+			CurrencyOf::<T>::balance(&reward_account),
 			ed + commission * origin_weight
 		);
 	}
@@ -383,7 +385,7 @@ frame_benchmarking::benchmarks! {
 			T::Staking::active_stake(&pool_account).unwrap(),
 			min_create_bond + min_join_bond
 		);
-		assert_eq!(CurrencyOf::<T>::free_balance(&joiner), min_join_bond);
+		assert_eq!(CurrencyOf::<T>::balance(&joiner), min_join_bond);
 
 		// Unbond the new member
 		Pools::<T>::fully_unbond(RuntimeOrigin::Signed(joiner.clone()).into(), joiner.clone()).unwrap();
@@ -403,7 +405,7 @@ frame_benchmarking::benchmarks! {
 	}: _(RuntimeOrigin::Signed(pool_account.clone()), 1, s)
 	verify {
 		// The joiners funds didn't change
-		assert_eq!(CurrencyOf::<T>::free_balance(&joiner), min_join_bond);
+		assert_eq!(CurrencyOf::<T>::balance(&joiner), min_join_bond);
 		// The unlocking chunk was removed
 		assert_eq!(pallet_staking::Ledger::<T>::get(pool_account).unwrap().unlocking.len(), 0);
 	}
@@ -426,7 +428,7 @@ frame_benchmarking::benchmarks! {
 			T::Staking::active_stake(&pool_account).unwrap(),
 			min_create_bond + min_join_bond
 		);
-		assert_eq!(CurrencyOf::<T>::free_balance(&joiner), min_join_bond);
+		assert_eq!(CurrencyOf::<T>::balance(&joiner), min_join_bond);
 
 		// Unbond the new member
 		pallet_staking::CurrentEra::<T>::put(0);
@@ -447,8 +449,7 @@ frame_benchmarking::benchmarks! {
 	}: withdraw_unbonded(RuntimeOrigin::Signed(joiner.clone()), joiner_lookup, s)
 	verify {
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&joiner),
-			min_join_bond * 2u32.into()
+			CurrencyOf::<T>::balance(&joiner), min_join_bond * 2u32.into()
 		);
 		// The unlocking chunk was removed
 		assert_eq!(pallet_staking::Ledger::<T>::get(&pool_account).unwrap().unlocking.len(), 0);
@@ -485,7 +486,7 @@ frame_benchmarking::benchmarks! {
 			Zero::zero()
 		);
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&pool_account),
+			CurrencyOf::<T>::balance(&pool_account),
 			min_create_bond
 		);
 		assert_eq!(pallet_staking::Ledger::<T>::get(&pool_account).unwrap().unlocking.len(), 1);
@@ -515,7 +516,7 @@ frame_benchmarking::benchmarks! {
 
 		// Funds where transferred back correctly
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&depositor),
+			CurrencyOf::<T>::balance(&depositor),
 			// gets bond back + rewards collecting when unbonding
 			min_create_bond * 2u32.into() + CurrencyOf::<T>::minimum_balance()
 		);
@@ -527,7 +528,7 @@ frame_benchmarking::benchmarks! {
 		let depositor_lookup = T::Lookup::unlookup(depositor.clone());
 
 		// Give the depositor some balance to bond
-		CurrencyOf::<T>::make_free_balance_be(&depositor, min_create_bond * 2u32.into());
+		CurrencyOf::<T>::set_balance(&depositor, min_create_bond * 2u32.into());
 
 		// Make sure no Pools exist at a pre-condition for our verify checks
 		assert_eq!(RewardPools::<T>::count(), 0);
@@ -782,7 +783,7 @@ frame_benchmarking::benchmarks! {
 		let ed = CurrencyOf::<T>::minimum_balance();
 		let (depositor, pool_account) = create_pool_account::<T>(0, origin_weight, Some(commission));
 		let reward_account = Pools::<T>::create_reward_account(1);
-		CurrencyOf::<T>::make_free_balance_be(&reward_account, ed + origin_weight);
+		CurrencyOf::<T>::set_balance(&reward_account, ed + origin_weight);
 
 		// member claims a payout to make some commission available.
 		let _ = Pools::<T>::claim_payout(RuntimeOrigin::Signed(claimer).into());
@@ -791,15 +792,29 @@ frame_benchmarking::benchmarks! {
 	}:_(RuntimeOrigin::Signed(depositor.clone()), 1u32.into())
 	verify {
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&depositor),
+			CurrencyOf::<T>::balance(&depositor),
 			origin_weight + commission * origin_weight
 		);
 		assert_eq!(
-			CurrencyOf::<T>::free_balance(&reward_account),
+			CurrencyOf::<T>::balance(&reward_account),
 			ed + commission * origin_weight
 		);
 	}
 
+	adjust_pool_deposit {
+		// Create a pool
+		let (depositor, _) = create_pool_account::<T>(0, Pools::<T>::depositor_min_bond() * 2u32.into(), None);
+
+		// Remove ed freeze to create a scenario where the ed deposit needs to be adjusted.
+		let _ = Pools::<T>::unfreeze_pool_deposit(&Pools::<T>::create_reward_account(1));
+		assert!(&Pools::<T>::check_ed_imbalance().is_err());
+
+		whitelist_account!(depositor);
+	}:_(RuntimeOrigin::Signed(depositor), 1)
+	verify {
+		assert!(&Pools::<T>::check_ed_imbalance().is_ok());
+	}
+
 	impl_benchmark_test_suite!(
 		Pallet,
 		crate::mock::new_test_ext(),

diff --git a/substrate/frame/nomination-pools/benchmarking/src/mock.rs b/substrate/frame/nomination-pools/benchmarking/src/mock.rs
@@ -74,8 +74,8 @@ impl pallet_balances::Config for Runtime {
 	type ExistentialDeposit = ExistentialDeposit;
 	type AccountStore = System;
 	type WeightInfo = ();
-	type FreezeIdentifier = ();
-	type MaxFreezes = ();
+	type FreezeIdentifier = RuntimeFreezeReason;
+	type MaxFreezes = ConstU32<1>;
 	type RuntimeHoldReason = ();
 	type MaxHolds = ();
 }
@@ -160,6 +160,7 @@ impl pallet_nomination_pools::Config for Runtime {
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = ();
 	type Currency = Balances;
+	type RuntimeFreezeReason = RuntimeFreezeReason;
 	type RewardCounter = FixedU128;
 	type BalanceToU256 = BalanceToU256;
 	type U256ToBalance = U256ToBalance;
@@ -183,7 +184,7 @@ frame_support::construct_runtime!(
 		Balances: pallet_balances::{Pallet, Call, Storage, Config<T>, Event<T>},
 		Staking: pallet_staking::{Pallet, Call, Config<T>, Storage, Event<T>},
 		VoterList: pallet_bags_list::<Instance1>::{Pallet, Call, Storage, Event<T>},
-		Pools: pallet_nomination_pools::{Pallet, Call, Storage, Event<T>},
+		Pools: pallet_nomination_pools::{Pallet, Call, Storage, Event<T>, FreezeReason},
 	}
 );