From 11a0c44e8dc13e80ba4bd677beffe481c1f342d9 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 18 Jun 2024 18:10:27 +0200 Subject: [PATCH 01/57] migration test --- polkadot/runtime/westend/src/lib.rs | 77 ++++++++++++++++++++- substrate/frame/nomination-pools/src/lib.rs | 58 ++++++++-------- 2 files changed, 107 insertions(+), 28 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 5b50a078539e..398ccf35d8d5 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1007,7 +1007,8 @@ impl InstanceFilter for ProxyType { matches!( c, RuntimeCall::Staking(..) | - RuntimeCall::Session(..) | RuntimeCall::Utility(..) | + RuntimeCall::Session(..) | + RuntimeCall::Utility(..) | RuntimeCall::FastUnstake(..) | RuntimeCall::VoterList(..) | RuntimeCall::NominationPools(..) @@ -2667,6 +2668,80 @@ mod remote_tests { .unwrap(); ext.execute_with(|| Runtime::on_runtime_upgrade(UpgradeCheckSelect::PreAndPost)); } + + #[tokio::test] + async fn delegate_stake_migration() { + use frame_support::{migrations::SteppedMigration, traits::TryState}; + sp_tracing::try_init_simple(); + let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); + let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); + let mut ext = Builder::::default() + .mode(if let Some(state_snapshot) = maybe_state_snapshot { + Mode::OfflineOrElseOnline( + OfflineConfig { state_snapshot: state_snapshot.clone() }, + OnlineConfig { + transport, + state_snapshot: Some(state_snapshot), + pallets: vec![ + "staking".into(), + "system".into(), + "balances".into(), + "nomination-pools".into(), + "delegated-staking".into(), + ], + ..Default::default() + }, + ) + } else { + Mode::Online(OnlineConfig { transport, ..Default::default() }) + }) + .build() + .await + .unwrap(); + ext.execute_with(|| { + let mut needs_migration = 0; + let mut success = 0; + + // iterate over all pools + pallet_nomination_pools::BondedPools::::iter_keys().for_each(|k| { + if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration( + k.clone(), + ) { + needs_migration = needs_migration + 1; + pallet_nomination_pools::Pallet::::migrate_to_delegate_stake(k) + .map(|_| success = success + 1) + .map_err(|e| log::error!("Failed to migrate pool {}: {:?}", k, e)); + } + }); + + // log summary + log::info!( + "Migration summary: {} pools needed migration, {} pools successfully migrated", + needs_migration, + success + ); + + // reset counters + needs_migration = 0; + success = 0; + // iterate over all pool members + pallet_nomination_pools::PoolMembers::::iter_keys().for_each(|k| { + if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration(k.clone()) { + needs_migration = needs_migration + 1; + pallet_nomination_pools::Pallet::::migrate_member_to_delegate_stake(k.clone()) + .map(|_| success = success + 1) + .map_err(|e| log::error!("Failed to migrate member {}: {:?}", k, e)); + } + }); + + // log summary + log::info!( + "Migration summary: {} members needed migration, {} members successfully migrated", + needs_migration, + success + ); + }); + } } mod clean_state_migration { diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 2aaea0446366..d155797fb6c1 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2916,32 +2916,7 @@ pub mod pallet { ); let member_account = T::Lookup::lookup(member_account)?; - let member = - PoolMembers::::get(&member_account).ok_or(Error::::PoolMemberNotFound)?; - - // ensure pool is migrated. - ensure!( - T::StakeAdapter::pool_strategy(Pool::from(Self::generate_bonded_account( - member.pool_id - ))) == adapter::StakeStrategyType::Delegate, - Error::::NotMigrated - ); - - let pool_contribution = member.total_balance(); - ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); - // the member must have some contribution to be migrated. - ensure!(pool_contribution > Zero::zero(), Error::::AlreadyMigrated); - - let delegation = - T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); - // delegation should not exist. - ensure!(delegation.is_none(), Error::::AlreadyMigrated); - - T::StakeAdapter::migrate_delegation( - Pool::from(Pallet::::generate_bonded_account(member.pool_id)), - Member::from(member_account), - pool_contribution, - )?; + Self::migrate_member_to_delegate_stake(member_account)?; // if successful, we refund the fee. Ok(Pays::No.into()) @@ -3094,13 +3069,42 @@ impl Pallet { T::PalletId::get().into_sub_account_truncating((AccountType::Bonded, id)) } - fn migrate_to_delegate_stake(id: PoolId) -> DispatchResult { + pub fn migrate_to_delegate_stake(id: PoolId) -> DispatchResult { T::StakeAdapter::migrate_nominator_to_agent( Pool::from(Self::generate_bonded_account(id)), &Self::generate_reward_account(id), ) } + pub fn migrate_member_to_delegate_stake(member_account: T::AccountId) -> DispatchResult { + let member = + PoolMembers::::get(&member_account).ok_or(Error::::PoolMemberNotFound)?; + + // ensure pool is migrated. + ensure!( + T::StakeAdapter::pool_strategy(Pool::from(Self::generate_bonded_account( + member.pool_id + ))) == adapter::StakeStrategyType::Delegate, + Error::::NotMigrated + ); + + let pool_contribution = member.total_balance(); + ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); + // the member must have some contribution to be migrated. + ensure!(pool_contribution > Zero::zero(), Error::::AlreadyMigrated); + + let delegation = + T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); + // delegation should not exist. + ensure!(delegation.is_none(), Error::::AlreadyMigrated); + + T::StakeAdapter::migrate_delegation( + Pool::from(Pallet::::generate_bonded_account(member.pool_id)), + Member::from(member_account), + pool_contribution, + ) + } + /// Create the reward account of a pool with the given id. pub fn generate_reward_account(id: PoolId) -> T::AccountId { // NOTE: in order to have a distinction in the test account id type (u128), we put From efd33249445f04596e5cc16da7867bbf91dfcff2 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 18 Jun 2024 18:17:57 +0200 Subject: [PATCH 02/57] fmt --- polkadot/runtime/westend/src/lib.rs | 15 +++++++++------ substrate/frame/nomination-pools/src/lib.rs | 10 +++++----- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 398ccf35d8d5..c9950056a793 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1007,8 +1007,7 @@ impl InstanceFilter for ProxyType { matches!( c, RuntimeCall::Staking(..) | - RuntimeCall::Session(..) | - RuntimeCall::Utility(..) | + RuntimeCall::Session(..) | RuntimeCall::Utility(..) | RuntimeCall::FastUnstake(..) | RuntimeCall::VoterList(..) | RuntimeCall::NominationPools(..) @@ -2726,11 +2725,15 @@ mod remote_tests { success = 0; // iterate over all pool members pallet_nomination_pools::PoolMembers::::iter_keys().for_each(|k| { - if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration(k.clone()) { + if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( + k.clone(), + ) { needs_migration = needs_migration + 1; - pallet_nomination_pools::Pallet::::migrate_member_to_delegate_stake(k.clone()) - .map(|_| success = success + 1) - .map_err(|e| log::error!("Failed to migrate member {}: {:?}", k, e)); + pallet_nomination_pools::Pallet::::migrate_member_to_delegate_stake( + k.clone(), + ) + .map(|_| success = success + 1) + .map_err(|e| log::error!("Failed to migrate member {}: {:?}", k, e)); } }); diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index d155797fb6c1..b9a68c5e2a5d 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3082,11 +3082,11 @@ impl Pallet { // ensure pool is migrated. ensure!( - T::StakeAdapter::pool_strategy(Pool::from(Self::generate_bonded_account( - member.pool_id - ))) == adapter::StakeStrategyType::Delegate, - Error::::NotMigrated - ); + T::StakeAdapter::pool_strategy(Pool::from(Self::generate_bonded_account( + member.pool_id + ))) == adapter::StakeStrategyType::Delegate, + Error::::NotMigrated + ); let pool_contribution = member.total_balance(); ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); From 3f8d2fc4bb5401b0349286030a2cf245c6298f43 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 18 Jun 2024 21:04:29 +0200 Subject: [PATCH 03/57] undo pool changes --- polkadot/runtime/westend/src/lib.rs | 26 ++++++--- substrate/frame/nomination-pools/src/lib.rs | 58 ++++++++++----------- 2 files changed, 46 insertions(+), 38 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index c9950056a793..9cd782c7c847 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1007,7 +1007,8 @@ impl InstanceFilter for ProxyType { matches!( c, RuntimeCall::Staking(..) | - RuntimeCall::Session(..) | RuntimeCall::Utility(..) | + RuntimeCall::Session(..) | + RuntimeCall::Utility(..) | RuntimeCall::FastUnstake(..) | RuntimeCall::VoterList(..) | RuntimeCall::NominationPools(..) @@ -2698,6 +2699,11 @@ mod remote_tests { .await .unwrap(); ext.execute_with(|| { + // create an account with some balance + let alice = AccountId::from([1u8; 32]); + use frame_support::traits::Currency; + Balances::deposit_creating(&alice, 100_000 * UNITS); + let mut needs_migration = 0; let mut success = 0; @@ -2707,14 +2713,18 @@ mod remote_tests { k.clone(), ) { needs_migration = needs_migration + 1; - pallet_nomination_pools::Pallet::::migrate_to_delegate_stake(k) - .map(|_| success = success + 1) - .map_err(|e| log::error!("Failed to migrate pool {}: {:?}", k, e)); + pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( + RuntimeOrigin::signed(alice.clone()).into(), + k, + ) + .map(|_| success = success + 1) + .map_err(|e| log::error!(target: "remote_test", "Failed to migrate pool {}: {:?}", k, e)); } }); // log summary log::info!( + target: "remote_test", "Migration summary: {} pools needed migration, {} pools successfully migrated", needs_migration, success @@ -2729,16 +2739,18 @@ mod remote_tests { k.clone(), ) { needs_migration = needs_migration + 1; - pallet_nomination_pools::Pallet::::migrate_member_to_delegate_stake( - k.clone(), + pallet_nomination_pools::Pallet::::migrate_delegation( + RuntimeOrigin::signed(alice.clone()).into(), + sp_runtime::MultiAddress::Id(k.clone()), ) .map(|_| success = success + 1) - .map_err(|e| log::error!("Failed to migrate member {}: {:?}", k, e)); + .map_err(|e| log::error!(target: "remote_test", "Failed to migrate member {}: {:?}", k, e)); } }); // log summary log::info!( + target: "remote_test", "Migration summary: {} members needed migration, {} members successfully migrated", needs_migration, success diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index b9a68c5e2a5d..2aaea0446366 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2916,7 +2916,32 @@ pub mod pallet { ); let member_account = T::Lookup::lookup(member_account)?; - Self::migrate_member_to_delegate_stake(member_account)?; + let member = + PoolMembers::::get(&member_account).ok_or(Error::::PoolMemberNotFound)?; + + // ensure pool is migrated. + ensure!( + T::StakeAdapter::pool_strategy(Pool::from(Self::generate_bonded_account( + member.pool_id + ))) == adapter::StakeStrategyType::Delegate, + Error::::NotMigrated + ); + + let pool_contribution = member.total_balance(); + ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); + // the member must have some contribution to be migrated. + ensure!(pool_contribution > Zero::zero(), Error::::AlreadyMigrated); + + let delegation = + T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); + // delegation should not exist. + ensure!(delegation.is_none(), Error::::AlreadyMigrated); + + T::StakeAdapter::migrate_delegation( + Pool::from(Pallet::::generate_bonded_account(member.pool_id)), + Member::from(member_account), + pool_contribution, + )?; // if successful, we refund the fee. Ok(Pays::No.into()) @@ -3069,42 +3094,13 @@ impl Pallet { T::PalletId::get().into_sub_account_truncating((AccountType::Bonded, id)) } - pub fn migrate_to_delegate_stake(id: PoolId) -> DispatchResult { + fn migrate_to_delegate_stake(id: PoolId) -> DispatchResult { T::StakeAdapter::migrate_nominator_to_agent( Pool::from(Self::generate_bonded_account(id)), &Self::generate_reward_account(id), ) } - pub fn migrate_member_to_delegate_stake(member_account: T::AccountId) -> DispatchResult { - let member = - PoolMembers::::get(&member_account).ok_or(Error::::PoolMemberNotFound)?; - - // ensure pool is migrated. - ensure!( - T::StakeAdapter::pool_strategy(Pool::from(Self::generate_bonded_account( - member.pool_id - ))) == adapter::StakeStrategyType::Delegate, - Error::::NotMigrated - ); - - let pool_contribution = member.total_balance(); - ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); - // the member must have some contribution to be migrated. - ensure!(pool_contribution > Zero::zero(), Error::::AlreadyMigrated); - - let delegation = - T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); - // delegation should not exist. - ensure!(delegation.is_none(), Error::::AlreadyMigrated); - - T::StakeAdapter::migrate_delegation( - Pool::from(Pallet::::generate_bonded_account(member.pool_id)), - Member::from(member_account), - pool_contribution, - ) - } - /// Create the reward account of a pool with the given id. pub fn generate_reward_account(id: PoolId) -> T::AccountId { // NOTE: in order to have a distinction in the test account id type (u128), we put From 7388573c8d4eb15d15fdd10d3fb2c5f612b29eb3 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 19 Jun 2024 00:39:00 +0200 Subject: [PATCH 04/57] fixes all NotEnoughFunds error --- polkadot/runtime/westend/src/lib.rs | 7 ++++- substrate/frame/delegated-staking/src/lib.rs | 27 ++++++++------------ 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 9cd782c7c847..59be317cdb33 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2744,7 +2744,12 @@ mod remote_tests { sp_runtime::MultiAddress::Id(k.clone()), ) .map(|_| success = success + 1) - .map_err(|e| log::error!(target: "remote_test", "Failed to migrate member {}: {:?}", k, e)); + .map_err(|e| { + // let pool_member = pallet_nomination_pools::PoolMembers::::get(&k); + // let ledger = pallet_staking::Ledger::::get(&k); + log::error!(target: "remote_test", "Failed to migrate member {}: {:?}", k, e); + log::error!(target: "remote_test", "member free balance {:?}", Balances::free_balance(&k)); + }); } }); diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 4b924bce3a57..f1ba296c054f 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -149,6 +149,8 @@ use frame_support::{ Defensive, DefensiveOption, Imbalance, OnUnbalanced, }, }; +use frame_support::traits::tokens::Fortitude::Force; +use frame_support::traits::tokens::Restriction; use sp_runtime::{ traits::{AccountIdConversion, CheckedAdd, CheckedSub, Zero}, ArithmeticError, DispatchResult, Perbill, RuntimeDebug, Saturating, @@ -682,33 +684,24 @@ impl Pallet { source_delegation.update_or_kill(&source_delegator); - // release funds from source - let released = T::Currency::release( + // transfer the released amount to `destination_delegator`. + let _ = T::Currency::transfer_on_hold( &HoldReason::StakingDelegation.into(), &source_delegator, + &destination_delegator, amount, - Precision::BestEffort, + Precision::Exact, + Restriction::Free, + Fortitude::Polite, )?; - defensive_assert!(released == amount, "hold should have been released fully"); - - // transfer the released amount to `destination_delegator`. - let post_balance = T::Currency::transfer( - &source_delegator, - &destination_delegator, - amount, - Preservation::Expendable, - ) - .map_err(|_| Error::::BadState)?; + let post_balance_src = T::Currency::total_balance(&source_delegator); // if balance is zero, clear provider for source (proxy) delegator. - if post_balance == Zero::zero() { + if post_balance_src == Zero::zero() { let _ = frame_system::Pallet::::dec_providers(&source_delegator).defensive(); } - // hold the funds again in the new delegator account. - T::Currency::hold(&HoldReason::StakingDelegation.into(), &destination_delegator, amount)?; - Self::deposit_event(Event::::MigratedDelegation { agent, delegator: destination_delegator, From 6634d25a677405b5e500973a45f44b55c7151297 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 19 Jun 2024 00:42:23 +0200 Subject: [PATCH 05/57] return false if pool does not exist --- substrate/frame/nomination-pools/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 2aaea0446366..2c65ae04fe72 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3862,7 +3862,13 @@ impl Pallet { return false } + // if pool does not exist, return false. + if !BondedPools::::contains_key(pool_id) { + return false + } + let pool_account = Self::generate_bonded_account(pool_id); + // true if pool is still not migrated to `DelegateStake`. T::StakeAdapter::pool_strategy(Pool::from(pool_account)) != adapter::StakeStrategyType::Delegate From a3901a9091bbc6095f5ccdc6317617dde837d734 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 19 Jun 2024 00:50:52 +0200 Subject: [PATCH 06/57] fix transfer on hold amount should still be held in dst acc --- substrate/frame/delegated-staking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index f1ba296c054f..276cf660323d 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -691,7 +691,7 @@ impl Pallet { &destination_delegator, amount, Precision::Exact, - Restriction::Free, + Restriction::OnHold, Fortitude::Polite, )?; From 462be1f35f0ccc15979a4e11ac4ad259c763a442 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 19 Jun 2024 00:51:13 +0200 Subject: [PATCH 07/57] fmt --- polkadot/runtime/westend/src/lib.rs | 12 +++++++----- substrate/frame/delegated-staking/src/lib.rs | 6 +++--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 59be317cdb33..c5675864ec25 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1007,8 +1007,7 @@ impl InstanceFilter for ProxyType { matches!( c, RuntimeCall::Staking(..) | - RuntimeCall::Session(..) | - RuntimeCall::Utility(..) | + RuntimeCall::Session(..) | RuntimeCall::Utility(..) | RuntimeCall::FastUnstake(..) | RuntimeCall::VoterList(..) | RuntimeCall::NominationPools(..) @@ -2718,7 +2717,9 @@ mod remote_tests { k, ) .map(|_| success = success + 1) - .map_err(|e| log::error!(target: "remote_test", "Failed to migrate pool {}: {:?}", k, e)); + .map_err( + |e| log::error!(target: "remote_test", "Failed to migrate pool {}: {:?}", k, e), + ); } }); @@ -2745,8 +2746,9 @@ mod remote_tests { ) .map(|_| success = success + 1) .map_err(|e| { - // let pool_member = pallet_nomination_pools::PoolMembers::::get(&k); - // let ledger = pallet_staking::Ledger::::get(&k); + // let pool_member = + // pallet_nomination_pools::PoolMembers::::get(&k); let ledger = + // pallet_staking::Ledger::::get(&k); log::error!(target: "remote_test", "Failed to migrate member {}: {:?}", k, e); log::error!(target: "remote_test", "member free balance {:?}", Balances::free_balance(&k)); }); diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 276cf660323d..0f5e550e25fc 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -145,12 +145,12 @@ use frame_support::{ }, Balanced, Inspect as FunInspect, Mutate as FunMutate, }, - tokens::{fungible::Credit, Fortitude, Precision, Preservation}, + tokens::{ + fungible::Credit, Fortitude, Fortitude::Force, Precision, Preservation, Restriction, + }, Defensive, DefensiveOption, Imbalance, OnUnbalanced, }, }; -use frame_support::traits::tokens::Fortitude::Force; -use frame_support::traits::tokens::Restriction; use sp_runtime::{ traits::{AccountIdConversion, CheckedAdd, CheckedSub, Zero}, ArithmeticError, DispatchResult, Perbill, RuntimeDebug, Saturating, From c7e9f6c8d85224cf5210c0599de057f1c2f8df05 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 19 Jun 2024 11:15:54 +0200 Subject: [PATCH 08/57] unexpected errors check --- polkadot/runtime/westend/src/lib.rs | 37 +++++++++++++------- substrate/frame/delegated-staking/src/lib.rs | 2 +- substrate/frame/nomination-pools/src/lib.rs | 2 +- 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index c5675864ec25..1f5b0d64c3f2 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2633,6 +2633,7 @@ sp_api::impl_runtime_apis! { #[cfg(all(test, feature = "try-runtime"))] mod remote_tests { use super::*; + use frame_support::assert_noop; use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect}; use remote_externalities::{ Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport, @@ -2671,6 +2672,7 @@ mod remote_tests { #[tokio::test] async fn delegate_stake_migration() { use frame_support::{migrations::SteppedMigration, traits::TryState}; + use sp_runtime::traits::Zero; sp_tracing::try_init_simple(); let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); @@ -2733,34 +2735,43 @@ mod remote_tests { // reset counters needs_migration = 0; - success = 0; + let mut unexpected_fails = 0; // iterate over all pool members pallet_nomination_pools::PoolMembers::::iter_keys().for_each(|k| { if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( k.clone(), ) { needs_migration = needs_migration + 1; - pallet_nomination_pools::Pallet::::migrate_delegation( + let pool_member = pallet_nomination_pools::PoolMembers::::get(&k); + let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); + let member_balance = Balances::free_balance(&k); + + let res = pallet_nomination_pools::Pallet::::migrate_delegation( RuntimeOrigin::signed(alice.clone()).into(), sp_runtime::MultiAddress::Id(k.clone()), - ) - .map(|_| success = success + 1) - .map_err(|e| { - // let pool_member = - // pallet_nomination_pools::PoolMembers::::get(&k); let ledger = - // pallet_staking::Ledger::::get(&k); - log::error!(target: "remote_test", "Failed to migrate member {}: {:?}", k, e); - log::error!(target: "remote_test", "member free balance {:?}", Balances::free_balance(&k)); - }); + ); + + if is_direct_staker { + if res.is_ok() { + log::error!(target: "remote_test", "Member {} is direct staker but migration succeeded.", k); + } + } else if member_balance.is_zero() { + if res.is_ok() { + log::error!(target: "remote_test", "Member {} has balance {:?} but migration succeeded.", k, member_balance); + } + } else if res.is_err() { + log::error!(target: "remote_test", "Failed to migrate member {}: {:?}", k, res.unwrap_err()); + unexpected_fails = unexpected_fails + 1; + } } }); // log summary log::info!( target: "remote_test", - "Migration summary: {} members needed migration, {} members successfully migrated", + "Migration summary: {} members needed migration, {} failed to migrate", needs_migration, - success + unexpected_fails ); }); } diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 0f5e550e25fc..604ec800b5cf 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -372,7 +372,7 @@ pub mod pallet { let agent = ensure_signed(origin)?; // Ensure they have minimum delegation. - ensure!(amount >= T::Currency::minimum_balance(), Error::::NotEnoughFunds); + // ensure!(amount >= T::Currency::minimum_balance(), Error::::NotEnoughFunds); // Ensure delegator is sane. ensure!(!Self::is_agent(&delegator), Error::::NotAllowed); diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 2c65ae04fe72..8a38670da6c5 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2928,7 +2928,7 @@ pub mod pallet { ); let pool_contribution = member.total_balance(); - ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); + // ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); // the member must have some contribution to be migrated. ensure!(pool_contribution > Zero::zero(), Error::::AlreadyMigrated); From 9a68720a8a603ab07710cbbc14770fa9c2d00028 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 19 Jun 2024 13:24:31 +0200 Subject: [PATCH 09/57] all non staking accounts succeed when given minimum balance --- polkadot/runtime/westend/src/lib.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 1f5b0d64c3f2..0f04f1929a75 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2733,6 +2733,8 @@ mod remote_tests { success ); + log::info!(target: "remote_test", "Existential Deposit: {:?}", EXISTENTIAL_DEPOSIT); + // reset counters needs_migration = 0; let mut unexpected_fails = 0; @@ -2755,13 +2757,20 @@ mod remote_tests { if res.is_ok() { log::error!(target: "remote_test", "Member {} is direct staker but migration succeeded.", k); } - } else if member_balance.is_zero() { - if res.is_ok() { - log::error!(target: "remote_test", "Member {} has balance {:?} but migration succeeded.", k, member_balance); - } } else if res.is_err() { - log::error!(target: "remote_test", "Failed to migrate member {}: {:?}", k, res.unwrap_err()); - unexpected_fails = unexpected_fails + 1; + log::info!(target: "remote_test", "Try One: Failed to migrate member {} with balance {:?}. Err: {:?}", k, member_balance, res.unwrap_err()); + // try giving these accounts minimum balance + Balances::deposit_creating(&k, EXISTENTIAL_DEPOSIT); + // retry migration + let res = pallet_nomination_pools::Pallet::::migrate_delegation( + RuntimeOrigin::signed(alice.clone()).into(), + sp_runtime::MultiAddress::Id(k.clone()), + ); + + if res.is_err() { + log::error!(target: "remote_test", "Try Two: Failed to re-migrate member {} with balance {:?} after giving them ed. Err: {:?}", k, Balances::free_balance(&k), res.unwrap_err()); + unexpected_fails = unexpected_fails + 1; + } } } }); From 268d0a5d94660482414bd10cf878505e2f13bb5c Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 19 Jun 2024 23:39:04 +0200 Subject: [PATCH 10/57] assert migrate tests --- polkadot/runtime/westend/src/lib.rs | 83 ++++++------------- .../frame/delegated-staking/src/impls.rs | 2 +- substrate/frame/delegated-staking/src/lib.rs | 4 +- 3 files changed, 28 insertions(+), 61 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 0f04f1929a75..b5bd2fafbbc1 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -92,7 +92,7 @@ use sp_runtime::{ IdentityLookup, Keccak256, OpaqueKeys, SaturatedConversion, Verify, }, transaction_validity::{TransactionPriority, TransactionSource, TransactionValidity}, - ApplyExtrinsicResult, FixedU128, KeyTypeId, Perbill, Percent, Permill, + ApplyExtrinsicResult, FixedU128, KeyTypeId, Percent, Permill, }; use sp_staking::SessionIndex; use sp_std::{ @@ -2671,9 +2671,12 @@ mod remote_tests { #[tokio::test] async fn delegate_stake_migration() { - use frame_support::{migrations::SteppedMigration, traits::TryState}; - use sp_runtime::traits::Zero; + if var("RUN_MIGRATION_TESTS").is_err() { + return; + } + use frame_support::{assert_ok, migrations::SteppedMigration, traits::TryState}; sp_tracing::try_init_simple(); + let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); let mut ext = Builder::::default() @@ -2703,85 +2706,51 @@ mod remote_tests { // create an account with some balance let alice = AccountId::from([1u8; 32]); use frame_support::traits::Currency; - Balances::deposit_creating(&alice, 100_000 * UNITS); - - let mut needs_migration = 0; - let mut success = 0; + let _ = Balances::deposit_creating(&alice, 100_000 * UNITS); // iterate over all pools pallet_nomination_pools::BondedPools::::iter_keys().for_each(|k| { if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration( k.clone(), ) { - needs_migration = needs_migration + 1; - pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( - RuntimeOrigin::signed(alice.clone()).into(), - k, - ) - .map(|_| success = success + 1) - .map_err( - |e| log::error!(target: "remote_test", "Failed to migrate pool {}: {:?}", k, e), + assert_ok!( + pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( + RuntimeOrigin::signed(alice.clone()).into(), + k, + ) ); } }); - // log summary - log::info!( - target: "remote_test", - "Migration summary: {} pools needed migration, {} pools successfully migrated", - needs_migration, - success - ); - - log::info!(target: "remote_test", "Existential Deposit: {:?}", EXISTENTIAL_DEPOSIT); - - // reset counters - needs_migration = 0; - let mut unexpected_fails = 0; // iterate over all pool members pallet_nomination_pools::PoolMembers::::iter_keys().for_each(|k| { if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( k.clone(), ) { - needs_migration = needs_migration + 1; - let pool_member = pallet_nomination_pools::PoolMembers::::get(&k); let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); - let member_balance = Balances::free_balance(&k); - - let res = pallet_nomination_pools::Pallet::::migrate_delegation( + let migration = pallet_nomination_pools::Pallet::::migrate_delegation( RuntimeOrigin::signed(alice.clone()).into(), sp_runtime::MultiAddress::Id(k.clone()), ); - if is_direct_staker { - if res.is_ok() { - log::error!(target: "remote_test", "Member {} is direct staker but migration succeeded.", k); - } - } else if res.is_err() { - log::info!(target: "remote_test", "Try One: Failed to migrate member {} with balance {:?}. Err: {:?}", k, member_balance, res.unwrap_err()); - // try giving these accounts minimum balance - Balances::deposit_creating(&k, EXISTENTIAL_DEPOSIT); - // retry migration - let res = pallet_nomination_pools::Pallet::::migrate_delegation( + if is_direct_staker { + // if the member is a direct staker, the migration should fail until pool + // member unstakes all funds from pallet-staking. + assert_eq!( + migration.unwrap_err(), + pallet_delegated_staking::Error::::AlreadyStaking.into() + ); + } else if migration.is_err() { + // migration can fail if the member has less than the existential deposit. + // we try funding them with the existential deposit and retry migration. + let _ = Balances::deposit_creating(&k, EXISTENTIAL_DEPOSIT); + assert_ok!(pallet_nomination_pools::Pallet::::migrate_delegation( RuntimeOrigin::signed(alice.clone()).into(), sp_runtime::MultiAddress::Id(k.clone()), - ); - - if res.is_err() { - log::error!(target: "remote_test", "Try Two: Failed to re-migrate member {} with balance {:?} after giving them ed. Err: {:?}", k, Balances::free_balance(&k), res.unwrap_err()); - unexpected_fails = unexpected_fails + 1; - } + )); } } }); - - // log summary - log::info!( - target: "remote_test", - "Migration summary: {} members needed migration, {} failed to migrate", - needs_migration, - unexpected_fails - ); }); } } diff --git a/substrate/frame/delegated-staking/src/impls.rs b/substrate/frame/delegated-staking/src/impls.rs index 9f5649672d70..ea63ae582788 100644 --- a/substrate/frame/delegated-staking/src/impls.rs +++ b/substrate/frame/delegated-staking/src/impls.rs @@ -19,7 +19,7 @@ //! Implementations of public traits, namely [`DelegationInterface`] and [`OnStakingUpdate`]. use super::*; -use sp_staking::{Agent, DelegationInterface, DelegationMigrator, Delegator, OnStakingUpdate}; +use sp_staking::{DelegationInterface, DelegationMigrator, OnStakingUpdate}; impl DelegationInterface for Pallet { type Balance = BalanceOf; diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 604ec800b5cf..434bb4433fbc 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -145,9 +145,7 @@ use frame_support::{ }, Balanced, Inspect as FunInspect, Mutate as FunMutate, }, - tokens::{ - fungible::Credit, Fortitude, Fortitude::Force, Precision, Preservation, Restriction, - }, + tokens::{fungible::Credit, Fortitude, Precision, Preservation, Restriction}, Defensive, DefensiveOption, Imbalance, OnUnbalanced, }, }; From 000762f978f1f4f4f910c0a9e509139fbf634937 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 21 Jun 2024 16:47:55 +0200 Subject: [PATCH 11/57] remove clone --- polkadot/runtime/westend/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index b5bd2fafbbc1..7c97efdf9d88 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2710,9 +2710,8 @@ mod remote_tests { // iterate over all pools pallet_nomination_pools::BondedPools::::iter_keys().for_each(|k| { - if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration( - k.clone(), - ) { + if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration(k) + { assert_ok!( pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( RuntimeOrigin::signed(alice.clone()).into(), From a904fc8bd362a064c288f2665620a4915f1dd40f Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 21 Jun 2024 17:05:02 +0200 Subject: [PATCH 12/57] uncomment --- substrate/frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 8a38670da6c5..2c65ae04fe72 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2928,7 +2928,7 @@ pub mod pallet { ); let pool_contribution = member.total_balance(); - // ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); + ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); // the member must have some contribution to be migrated. ensure!(pool_contribution > Zero::zero(), Error::::AlreadyMigrated); From 0c776b8ead775b601fe61585331e23153fd23f10 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 29 Jun 2024 02:23:27 +0200 Subject: [PATCH 13/57] incrementing provider fixes users migrating with zero balance --- substrate/frame/delegated-staking/src/lib.rs | 31 ++++++++++++----- .../frame/delegated-staking/src/tests.rs | 33 +++++++++++++++++++ .../frame/delegated-staking/src/types.rs | 11 ++++--- .../test-delegate-stake/src/lib.rs | 5 +-- 4 files changed, 66 insertions(+), 14 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 434bb4433fbc..6aa0736d251a 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -483,7 +483,8 @@ impl Pallet { let proxy_delegator = Self::generate_proxy_delegator(Agent::from(who.clone())); // Keep proxy delegator alive until all funds are migrated. - frame_system::Pallet::::inc_providers(&proxy_delegator.clone().get()); + // TODO (ank4n) don't need it. + // frame_system::Pallet::::inc_providers(&proxy_delegator.clone().get()); // Get current stake let stake = T::CoreStaking::stake(who)?; @@ -550,8 +551,6 @@ impl Pallet { let delegator = delegator.get(); let mut ledger = AgentLedger::::get(&agent).ok_or(Error::::NotAgent)?; - // try to hold the funds. - T::Currency::hold(&HoldReason::StakingDelegation.into(), &delegator, amount)?; let new_delegation_amount = if let Some(existing_delegation) = Delegation::::get(&delegator) { @@ -561,10 +560,15 @@ impl Pallet { .checked_add(&amount) .ok_or(ArithmeticError::Overflow)? } else { + // if this is the first time delegation, increment provider count. + frame_system::Pallet::::inc_providers(&delegator); amount }; - Delegation::::new(&agent, new_delegation_amount).update_or_kill(&delegator); + // try to hold the funds. + T::Currency::hold(&HoldReason::StakingDelegation.into(), &delegator, amount)?; + + let _ = Delegation::::new(&agent, new_delegation_amount).update(&delegator); ledger.total_delegated = ledger.total_delegated.checked_add(&amount).ok_or(ArithmeticError::Overflow)?; ledger.update(&agent); @@ -636,7 +640,12 @@ impl Pallet { .defensive_ok_or(ArithmeticError::Overflow)?; // remove delegator if nothing delegated anymore - delegation.update_or_kill(&delegator); + let delegation_killed = delegation.update(&delegator); + + // if delegation killed, decrement provider count. + if delegation_killed { + frame_system::Pallet::::dec_providers(&delegator); + } let released = T::Currency::release( &HoldReason::StakingDelegation.into(), @@ -673,14 +682,19 @@ impl Pallet { let agent = source_delegation.agent.clone(); // update delegations - Delegation::::new(&agent, amount).update_or_kill(&destination_delegator); + let _ = Delegation::::new(&agent, amount).update(&destination_delegator); + frame_system::Pallet::::inc_providers(&destination_delegator); source_delegation.amount = source_delegation .amount .checked_sub(&amount) .defensive_ok_or(Error::::BadState)?; - source_delegation.update_or_kill(&source_delegator); + let delegation_killed = source_delegation.update(&source_delegator); + // if delegation killed, decrement provider count. + if delegation_killed { + frame_system::Pallet::::dec_providers(&source_delegator); + } // transfer the released amount to `destination_delegator`. let _ = T::Currency::transfer_on_hold( @@ -740,7 +754,8 @@ impl Pallet { agent_ledger.remove_slash(actual_slash).save(); delegation.amount = delegation.amount.checked_sub(&actual_slash).ok_or(ArithmeticError::Overflow)?; - delegation.update_or_kill(&delegator); + // TODO(ank4n) See what to do + let _ = delegation.update(&delegator); if let Some(reporter) = maybe_reporter { let reward_payout: BalanceOf = T::SlashRewardFraction::get() * actual_slash; diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index 2295f7d0c871..51577b52ed37 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -334,6 +334,39 @@ fn apply_pending_slash() { }); } +#[test] +fn allow_full_amount_to_be_delegated() { + ExtBuilder::default().build_and_execute(|| { + let agent: AccountId = 200; + let reward_acc: AccountId = 201; + let delegator: AccountId = 300; + + // set intention to accept delegation. + fund(&agent, 1000); + assert_ok!(DelegatedStaking::register_agent( + RawOrigin::Signed(agent).into(), + reward_acc + )); + + // delegate to this account + fund(&delegator, 1000); + assert_ok!(DelegatedStaking::delegate_to_agent( + RawOrigin::Signed(delegator).into(), + agent, + 1000 + )); + + // verify + assert!(DelegatedStaking::is_agent(&agent)); + assert_eq!(DelegatedStaking::stakeable_balance(Agent::from(agent)), 1000); + assert_eq!( + Balances::balance_on_hold(&HoldReason::StakingDelegation.into(), &delegator), + 1000 + ); + assert_eq!(DelegatedStaking::held_balance_of(Delegator::from(delegator)), 1000); + }); +} + /// Integration tests with pallet-staking. mod staking_integration { use super::*; diff --git a/substrate/frame/delegated-staking/src/types.rs b/substrate/frame/delegated-staking/src/types.rs index 24b457356544..c01b4ba3d8ba 100644 --- a/substrate/frame/delegated-staking/src/types.rs +++ b/substrate/frame/delegated-staking/src/types.rs @@ -64,15 +64,18 @@ impl Delegation { ) } - /// Save self to storage. If the delegation amount is zero, remove the delegation. - pub(crate) fn update_or_kill(self, key: &T::AccountId) { + /// Save self to storage. + /// + /// If the delegation amount is zero, remove the delegation and returns true. + pub(crate) fn update(self, key: &T::AccountId) -> bool { // Clean up if no delegation left. if self.amount == Zero::zero() { >::remove(key); - return + return true } - >::insert(key, self) + >::insert(key, self); + false } } diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs index 51f6470f90d0..0f5593408abc 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -1113,8 +1113,9 @@ fn pool_migration_e2e() { assert_eq!(Balances::total_balance(&21), pre_migrate_balance_21 + 10); // MIGRATE 22 - let pre_migrate_balance_22 = Balances::total_balance(&22); assert_eq!(Balances::total_balance_on_hold(&22), 0); + // make balance of 22 as 0. + let _ = Balances::make_free_balance_be(&22, 0); // migrate delegation for 22. assert!(Pools::api_member_needs_delegate_migration(22)); @@ -1128,7 +1129,7 @@ fn pool_migration_e2e() { ); // tokens moved to 22's account and held there. - assert_eq!(Balances::total_balance(&22), pre_migrate_balance_22 + 10); + assert_eq!(Balances::total_balance(&22), 10); assert_eq!(Balances::total_balance_on_hold(&22), 10); // withdraw fails since 22 only unbonds at era 8. From 3d2379e62d132ca14784c830c804046e6485411c Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 6 Jul 2024 22:04:52 +0200 Subject: [PATCH 14/57] test for clean up accounts --- substrate/frame/delegated-staking/src/lib.rs | 73 ++++++++----------- .../frame/delegated-staking/src/tests.rs | 68 ++++++++++++++++- 2 files changed, 96 insertions(+), 45 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 842caeda567d..4e00cb72e664 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -369,9 +369,6 @@ pub mod pallet { ) -> DispatchResult { let agent = ensure_signed(origin)?; - // Ensure they have minimum delegation. - // ensure!(amount >= T::Currency::minimum_balance(), Error::::NotEnoughFunds); - // Ensure delegator is sane. ensure!(!Self::is_agent(&delegator), Error::::NotAllowed); ensure!(!Self::is_delegator(&delegator), Error::::NotAllowed); @@ -482,10 +479,6 @@ impl Pallet { // transferred to actual delegator. let proxy_delegator = Self::generate_proxy_delegator(Agent::from(who.clone())); - // Keep proxy delegator alive until all funds are migrated. - // TODO (ank4n) don't need it. - // frame_system::Pallet::::inc_providers(&proxy_delegator.clone().get()); - // Get current stake let stake = T::CoreStaking::stake(who)?; @@ -560,7 +553,8 @@ impl Pallet { .checked_add(&amount) .ok_or(ArithmeticError::Overflow)? } else { - // if this is the first time delegation, increment provider count. + // if this is the first time delegation, increment provider count. This ensures that + // amount including existential deposit can be held. frame_system::Pallet::::inc_providers(&delegator); amount }; @@ -598,7 +592,7 @@ impl Pallet { // if we do not already have enough funds to be claimed, try withdraw some more. // keep track if we killed the staker in the process. - let stash_killed = if agent_ledger.ledger.unclaimed_withdrawals < amount { + let maybe_stash_killed = if agent_ledger.ledger.unclaimed_withdrawals < amount { // withdraw account. let killed = T::CoreStaking::withdraw_unbonded(agent.clone(), num_slashing_spans) .map_err(|_| Error::::WithdrawFailed)?; @@ -612,14 +606,15 @@ impl Pallet { // if we still do not have enough funds to release, abort. ensure!(agent_ledger.ledger.unclaimed_withdrawals >= amount, Error::::NotEnoughFunds); - // Claim withdraw from agent. Kill agent if no delegation left. - // TODO: Ideally if there is a register, there should be an unregister that should - // clean up the agent. Can be improved in future. + // Withdraw funds from agent. + // If agent is killed, we ensure it is killed in `CoreStaking` as well. if agent_ledger.remove_unclaimed_withdraw(amount)?.update_or_kill()? { - match stash_killed { + // TODO: Ideally if there is a register, there should be an unregister that should + // clean up the agent. Can be improved in future. + match maybe_stash_killed { Some(killed) => { // this implies we did a `CoreStaking::withdraw` before release. Ensure - // we killed the staker as well. + // the staker is killed as well. ensure!(killed, Error::::BadState); }, None => { @@ -639,14 +634,6 @@ impl Pallet { .checked_sub(&amount) .defensive_ok_or(ArithmeticError::Overflow)?; - // remove delegator if nothing delegated anymore - let delegation_killed = delegation.update(&delegator); - - // if delegation killed, decrement provider count. - if delegation_killed { - frame_system::Pallet::::dec_providers(&delegator); - } - let released = T::Currency::release( &HoldReason::StakingDelegation.into(), &delegator, @@ -656,6 +643,12 @@ impl Pallet { defensive_assert!(released == amount, "hold should have been released fully"); + // update delegation. + if delegation.update(&delegator) { + // remove provider for delegator if no delegation left. + let _ = frame_system::Pallet::::dec_providers(&delegator).defensive(); + } + Self::deposit_event(Event::::Released { agent, delegator, amount }); Ok(()) @@ -683,6 +676,8 @@ impl Pallet { let agent = source_delegation.agent.clone(); // update delegations let _ = Delegation::::new(&agent, amount).update(&destination_delegator); + // Provide for the delegator account. This ensures that amount including existential deposit + // can be held. frame_system::Pallet::::inc_providers(&destination_delegator); source_delegation.amount = source_delegation @@ -690,12 +685,6 @@ impl Pallet { .checked_sub(&amount) .defensive_ok_or(Error::::BadState)?; - let delegation_killed = source_delegation.update(&source_delegator); - // if delegation killed, decrement provider count. - if delegation_killed { - frame_system::Pallet::::dec_providers(&source_delegator); - } - // transfer the released amount to `destination_delegator`. let _ = T::Currency::transfer_on_hold( &HoldReason::StakingDelegation.into(), @@ -707,10 +696,9 @@ impl Pallet { Fortitude::Polite, )?; - let post_balance_src = T::Currency::total_balance(&source_delegator); - - // if balance is zero, clear provider for source (proxy) delegator. - if post_balance_src == Zero::zero() { + // update source delegation. + if source_delegation.update(&source_delegator) { + // remove provider for delegator if no delegation left. let _ = frame_system::Pallet::::dec_providers(&source_delegator).defensive(); } @@ -804,18 +792,21 @@ impl Pallet { ledgers: BTreeMap>, ) -> Result<(), sp_runtime::TryRuntimeError> { for (agent, ledger) in ledgers { - ensure!( - matches!( - T::CoreStaking::status(&agent).expect("agent should be bonded"), - sp_staking::StakerStatus::Nominator(_) | sp_staking::StakerStatus::Idle - ), - "agent should be bonded and not validator" - ); + let staked_value = ledger.stakeable_balance(); + + if !staked_value.is_zero() { + ensure!( + matches!( + T::CoreStaking::status(&agent).expect("agent should be bonded"), + sp_staking::StakerStatus::Nominator(_) | sp_staking::StakerStatus::Idle + ), + "agent should be bonded and not validator" + ); + } ensure!( ledger.stakeable_balance() >= - T::CoreStaking::total_stake(&agent) - .expect("agent should exist as a nominator"), + T::CoreStaking::total_stake(&agent).unwrap_or_default(), "Cannot stake more than balance" ); } diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index 4f396b58953f..04642ba30569 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -343,10 +343,7 @@ fn allow_full_amount_to_be_delegated() { // set intention to accept delegation. fund(&agent, 1000); - assert_ok!(DelegatedStaking::register_agent( - RawOrigin::Signed(agent).into(), - reward_acc - )); + assert_ok!(DelegatedStaking::register_agent(RawOrigin::Signed(agent).into(), reward_acc)); // delegate to this account fund(&delegator, 1000); @@ -743,6 +740,69 @@ mod staking_integration { ); }); } + + #[test] + fn accounts_are_cleaned_up() { + ExtBuilder::default().build_and_execute(|| { + let agent: AccountId = 200; + let reward_acc: AccountId = 201; + let delegator: AccountId = 300; + + // set intention to accept delegation. + fund(&agent, 1000); + + // Agent is provided since it has balance > ED. + assert_eq!(System::providers(&agent), 1); + assert_ok!(DelegatedStaking::register_agent( + RawOrigin::Signed(agent).into(), + reward_acc + )); + // becoming an agent adds another provider. + assert_eq!(System::providers(&agent), 2); + + // delegate to this account + fund(&delegator, 1000); + // account has one provider since its funded. + assert_eq!(System::providers(&delegator), 1); + assert_ok!(DelegatedStaking::delegate_to_agent( + RawOrigin::Signed(delegator).into(), + agent, + 500 + )); + // delegator has an extra provider now. + assert_eq!(System::providers(&delegator), 2); + // all 1000 tokens including ED can be held. + assert_ok!(DelegatedStaking::delegate_to_agent( + RawOrigin::Signed(delegator).into(), + agent, + 500 + )); + // free balance dropping below ED will reduce a provider, but it still has one provider + // left. + assert_eq!(System::providers(&delegator), 1); + + // withdraw all delegation + assert_ok!(Staking::unbond(RawOrigin::Signed(agent).into(), 1000)); + start_era(4); + assert_ok!(Staking::withdraw_unbonded(RawOrigin::Signed(agent).into(), 0)); + assert_ok!(DelegatedStaking::release_delegation( + RawOrigin::Signed(agent).into(), + delegator, + 1000, + 0 + )); + + // agent and delegator provider is decremented. + assert_eq!(System::providers(&delegator), 1); + assert_eq!(System::providers(&agent), 1); + + // if we transfer all funds, providers are removed. + assert_ok!(Balances::transfer_all(RawOrigin::Signed(delegator).into(), 1337, false)); + assert_ok!(Balances::transfer_all(RawOrigin::Signed(agent).into(), 1337, false)); + assert_eq!(System::providers(&delegator), 0); + assert_eq!(System::providers(&agent), 0); + }); + } } mod pool_integration { From ec9035cf981c3bde4183d516dba0a5abc83552d3 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 6 Jul 2024 22:20:29 +0200 Subject: [PATCH 15/57] providers are incremented correctly when migration agent/delegators --- substrate/frame/delegated-staking/src/lib.rs | 1 - substrate/frame/delegated-staking/src/tests.rs | 10 +++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 4e00cb72e664..bbdba61ebe54 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -500,7 +500,6 @@ impl Pallet { T::CoreStaking::update_payee(who, reward_account)?; // delegate all transferred funds back to agent. Self::do_delegate(proxy_delegator, Agent::from(who.clone()), amount_to_transfer)?; - // if the transferred/delegated amount was greater than the stake, mark the extra as // unclaimed withdrawal. let unclaimed_withdraws = amount_to_transfer diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index 04642ba30569..8d696fc70161 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -681,6 +681,9 @@ mod staking_integration { DelegatedStaking::generate_proxy_delegator(Agent::from(200)).get(); assert_ok!(DelegatedStaking::migrate_to_agent(RawOrigin::Signed(200).into(), 201)); + // after migration, funds are moved to proxy delegator, still a provider exists. + assert_eq!(System::providers(&200), 1); + assert_eq!(Balances::free_balance(200), 0); // verify all went well let mut expected_proxy_delegated_amount = agent_amount; @@ -702,14 +705,15 @@ mod staking_integration { let delegator_share = agent_amount / 4; for delegator in 300..304 { assert_eq!(Balances::free_balance(delegator), 0); - // fund them with ED - fund(&delegator, ExistentialDeposit::get()); - // migrate 1/4th amount into each delegator + assert_eq!(System::providers(&delegator), 0); + + // No pre-balance needed to migrate delegator. assert_ok!(DelegatedStaking::migrate_delegation( RawOrigin::Signed(200).into(), delegator, delegator_share )); + assert_eq!(System::providers(&delegator), 1); assert_eq!( Balances::balance_on_hold(&HoldReason::StakingDelegation.into(), &delegator), delegator_share From a9d45955aed137e049d72c68a3d940ed5c5ac633 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 6 Jul 2024 22:39:45 +0200 Subject: [PATCH 16/57] migrated agents/delegators are also cleaned up correctly --- .../frame/delegated-staking/src/tests.rs | 75 ++++++++++++++----- 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index 8d696fc70161..1eff7446f549 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -655,35 +655,42 @@ mod staking_integration { #[test] fn migration_works() { ExtBuilder::default().build_and_execute(|| { + // initial era + start_era(1); + // add a nominator let staked_amount = 4000; let agent_amount = 5000; - fund(&200, agent_amount); + let agent = 200; + fund(&agent, agent_amount); assert_ok!(Staking::bond( - RuntimeOrigin::signed(200), + RuntimeOrigin::signed(agent), staked_amount, RewardDestination::Account(201) )); - assert_ok!(Staking::nominate(RuntimeOrigin::signed(200), vec![GENESIS_VALIDATOR],)); - let init_stake = Staking::stake(&200).unwrap(); + assert_ok!(Staking::nominate(RuntimeOrigin::signed(agent), vec![GENESIS_VALIDATOR],)); + let init_stake = Staking::stake(&agent).unwrap(); // scenario: 200 is a pool account, and the stake comes from its 4 delegators (300..304) // in equal parts. lets try to migrate this nominator into delegate based stake. // all balance currently is in 200 - assert_eq!(Balances::free_balance(200), agent_amount); + assert_eq!(Balances::free_balance(agent), agent_amount); // to migrate, nominator needs to set an account as a proxy delegator where staked funds // will be moved and delegated back to this old nominator account. This should be funded // with at least ED. let proxy_delegator = - DelegatedStaking::generate_proxy_delegator(Agent::from(200)).get(); + DelegatedStaking::generate_proxy_delegator(Agent::from(agent)).get(); - assert_ok!(DelegatedStaking::migrate_to_agent(RawOrigin::Signed(200).into(), 201)); + assert_ok!(DelegatedStaking::migrate_to_agent(RawOrigin::Signed(agent).into(), 201)); // after migration, funds are moved to proxy delegator, still a provider exists. - assert_eq!(System::providers(&200), 1); - assert_eq!(Balances::free_balance(200), 0); + assert_eq!(System::providers(&agent), 1); + assert_eq!(Balances::free_balance(agent), 0); + // proxy delegator has one provider as well with no free balance. + assert_eq!(System::providers(&proxy_delegator), 1); + assert_eq!(Balances::free_balance(proxy_delegator), 0); // verify all went well let mut expected_proxy_delegated_amount = agent_amount; @@ -692,12 +699,12 @@ mod staking_integration { expected_proxy_delegated_amount ); // stake amount is transferred from delegate to proxy delegator account. - assert_eq!(Balances::free_balance(200), 0); - assert_eq!(Staking::stake(&200).unwrap(), init_stake); - assert_eq!(get_agent_ledger(&200).ledger.effective_balance(), agent_amount); - assert_eq!(get_agent_ledger(&200).available_to_bond(), 0); + assert_eq!(Balances::free_balance(agent), 0); + assert_eq!(Staking::stake(&agent).unwrap(), init_stake); + assert_eq!(get_agent_ledger(&agent).ledger.effective_balance(), agent_amount); + assert_eq!(get_agent_ledger(&agent).available_to_bond(), 0); assert_eq!( - get_agent_ledger(&200).ledger.unclaimed_withdrawals, + get_agent_ledger(&agent).ledger.unclaimed_withdrawals, agent_amount - staked_amount ); @@ -709,7 +716,7 @@ mod staking_integration { // No pre-balance needed to migrate delegator. assert_ok!(DelegatedStaking::migrate_delegation( - RawOrigin::Signed(200).into(), + RawOrigin::Signed(agent).into(), delegator, delegator_share )); @@ -728,20 +735,48 @@ mod staking_integration { ); // delegate stake is unchanged. - assert_eq!(Staking::stake(&200).unwrap(), init_stake); - assert_eq!(get_agent_ledger(&200).ledger.effective_balance(), agent_amount); - assert_eq!(get_agent_ledger(&200).available_to_bond(), 0); + assert_eq!(Staking::stake(&agent).unwrap(), init_stake); + assert_eq!(get_agent_ledger(&agent).ledger.effective_balance(), agent_amount); + assert_eq!(get_agent_ledger(&agent).available_to_bond(), 0); assert_eq!( - get_agent_ledger(&200).ledger.unclaimed_withdrawals, + get_agent_ledger(&agent).ledger.unclaimed_withdrawals, agent_amount - staked_amount ); } // cannot use migrate delegator anymore assert_noop!( - DelegatedStaking::migrate_delegation(RawOrigin::Signed(200).into(), 305, 1), + DelegatedStaking::migrate_delegation(RawOrigin::Signed(agent).into(), 305, 1), Error::::NotEnoughFunds ); + + // no provider left on proxy delegator since all funds are migrated + assert_eq!(System::providers(&proxy_delegator), 0); + + // withdraw all delegations from delegators + assert_ok!(Staking::chill(RuntimeOrigin::signed(agent))); + assert_ok!(Staking::unbond(RawOrigin::Signed(agent).into(), staked_amount)); + start_era(4); + assert_ok!(Staking::withdraw_unbonded(RawOrigin::Signed(agent).into(), 0)); + for delegator in 300..304 { + assert_ok!(DelegatedStaking::release_delegation( + RawOrigin::Signed(agent).into(), + delegator, + delegator_share, + 0 + )); + // delegator is cleaned up from storage. + assert!(!Delegators::::contains_key(delegator)); + // has free balance now + assert_eq!(Balances::free_balance(delegator), delegator_share); + // and only one provider as delegator_share > ED + assert_eq!(System::providers(&delegator), 1); + } + + // agent is cleaned up from storage + assert!(!Agents::::contains_key(agent)); + // and no provider left. + assert_eq!(System::providers(&agent), 0); }); } From 8a27ae071d9674513070b21f71ca405185f114d2 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 7 Jul 2024 01:50:44 +0200 Subject: [PATCH 17/57] improve remote tests --- polkadot/runtime/westend/src/lib.rs | 42 +++++++++++++++---- substrate/frame/delegated-staking/src/lib.rs | 16 ++++--- substrate/frame/nomination-pools/src/lib.rs | 18 ++++++-- .../src/traits/tokens/fungible/hold.rs | 1 - 4 files changed, 59 insertions(+), 18 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 67b53b892f7d..2942afe44587 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2673,6 +2673,7 @@ mod remote_tests { #[tokio::test] async fn delegate_stake_migration() { + // Intended to be run only manually. if var("RUN_MIGRATION_TESTS").is_err() { return; } @@ -2723,35 +2724,58 @@ mod remote_tests { } }); + // member migration stats + let mut success = 0; + let mut members_below_ed = 0; + let mut direct_stakers = 0; + let mut unexpected_errors = 0; + // iterate over all pool members - pallet_nomination_pools::PoolMembers::::iter_keys().for_each(|k| { + pallet_nomination_pools::PoolMembers::::iter().for_each(|(k, member)| { if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( k.clone(), ) { + // reasons migrations can fail: let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); + let member_balance_below_ed = member.total_balance() < ExistentialDeposit::get(); + let migration = pallet_nomination_pools::Pallet::::migrate_delegation( RuntimeOrigin::signed(alice.clone()).into(), sp_runtime::MultiAddress::Id(k.clone()), ); - if is_direct_staker { + if member_balance_below_ed { + // if the member has balance below ED, the migration should fail. + members_below_ed += 1; + assert_eq!( + migration.unwrap_err(), + pallet_nomination_pools::Error::::MinimumBondNotMet.into() + ); + } else if is_direct_staker { // if the member is a direct staker, the migration should fail until pool // member unstakes all funds from pallet-staking. + direct_stakers += 1; assert_eq!( migration.unwrap_err(), pallet_delegated_staking::Error::::AlreadyStaking.into() ); } else if migration.is_err() { - // migration can fail if the member has less than the existential deposit. - // we try funding them with the existential deposit and retry migration. - let _ = Balances::deposit_creating(&k, EXISTENTIAL_DEPOSIT); - assert_ok!(pallet_nomination_pools::Pallet::::migrate_delegation( - RuntimeOrigin::signed(alice.clone()).into(), - sp_runtime::MultiAddress::Id(k.clone()), - )); + unexpected_errors += 1; + log::error!(target: "remote_test", "Unexpected error {:?} while migrating {:?}", migration.unwrap_err(), k); + } else { + success += 1; } } }); + + log::info!( + target: "remote_test", + "Migration stats: success: {}, members_below_ed: {}, direct_stakers: {}, unexpected_errors: {}", + success, + members_below_ed, + direct_stakers, + unexpected_errors + ); }); } } diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index bbdba61ebe54..a109f9e166f7 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -666,14 +666,14 @@ impl Pallet { let mut source_delegation = Delegators::::get(&source_delegator).defensive_ok_or(Error::::BadState)?; - // some checks that must have already been checked before. + // ensure source has enough funds to migrate. ensure!(source_delegation.amount >= amount, Error::::NotEnoughFunds); debug_assert!( !Self::is_delegator(&destination_delegator) && !Self::is_agent(&destination_delegator) ); let agent = source_delegation.agent.clone(); - // update delegations + // create a new delegation for destination delegator. let _ = Delegation::::new(&agent, amount).update(&destination_delegator); // Provide for the delegator account. This ensures that amount including existential deposit // can be held. @@ -684,7 +684,10 @@ impl Pallet { .checked_sub(&amount) .defensive_ok_or(Error::::BadState)?; - // transfer the released amount to `destination_delegator`. + // ensure amount is greater than ED otherwise transfer would fail. + ensure!(amount >= T::Currency::minimum_balance(), Error::::NotEnoughFunds); + + // transfer the held amount in `source_delegator` to `destination_delegator`. let _ = T::Currency::transfer_on_hold( &HoldReason::StakingDelegation.into(), &source_delegator, @@ -741,8 +744,11 @@ impl Pallet { agent_ledger.remove_slash(actual_slash).save(); delegation.amount = delegation.amount.checked_sub(&actual_slash).ok_or(ArithmeticError::Overflow)?; - // TODO(ank4n) See what to do - let _ = delegation.update(&delegator); + + if delegation.update(&delegator) { + // remove provider for delegator if no delegation left. + let _ = frame_system::Pallet::::dec_providers(&delegator).defensive(); + } if let Some(reporter) = maybe_reporter { let reward_payout: BalanceOf = T::SlashRewardFraction::get() * actual_slash; diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 2c65ae04fe72..cf1d6f35ab35 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2928,9 +2928,12 @@ pub mod pallet { ); let pool_contribution = member.total_balance(); - ensure!(pool_contribution >= MinJoinBond::::get(), Error::::MinimumBondNotMet); - // the member must have some contribution to be migrated. - ensure!(pool_contribution > Zero::zero(), Error::::AlreadyMigrated); + // ensure the pool contribution is greater than the existential deposit otherwise we + // cannot transfer funds to member account. + ensure!( + pool_contribution >= T::Currency::minimum_balance(), + Error::::MinimumBondNotMet + ); let delegation = T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); @@ -3902,6 +3905,15 @@ impl Pallet { }) .unwrap_or_default() } + + /// Contribution of the member in the pool. + /// + /// Includes balance that is unbonding currently. + pub fn api_member_total_balance(who: T::AccountId) -> BalanceOf { + PoolMembers::::get(who.clone()) + .map(|m| m.total_balance()) + .unwrap_or_default() + } } impl sp_staking::OnStakingUpdate> for Pallet { diff --git a/substrate/frame/support/src/traits/tokens/fungible/hold.rs b/substrate/frame/support/src/traits/tokens/fungible/hold.rs index 28ece25c91d4..0611eb0b0ab0 100644 --- a/substrate/frame/support/src/traits/tokens/fungible/hold.rs +++ b/substrate/frame/support/src/traits/tokens/fungible/hold.rs @@ -361,7 +361,6 @@ pub trait Mutate: ensure!(amount <= liquid, TokenError::Frozen); ensure!(amount <= have, TokenError::FundsUnavailable); } - // We want to make sure we can deposit the amount in advance. If we can't then something is // very wrong. ensure!(Self::can_deposit(dest, amount, Extant) == Success, TokenError::CannotCreate); From 7fd484aef447a1f83d9c11e361a1372431129f71 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 7 Jul 2024 02:21:10 +0200 Subject: [PATCH 18/57] add runtime api for total member balance --- polkadot/runtime/westend/src/lib.rs | 4 ++++ substrate/bin/node/runtime/src/lib.rs | 4 ++++ substrate/frame/nomination-pools/runtime-api/src/lib.rs | 3 +++ 3 files changed, 11 insertions(+) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 2942afe44587..3bd5e4570a65 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2334,6 +2334,10 @@ sp_api::impl_runtime_apis! { fn member_needs_delegate_migration(member: AccountId) -> bool { NominationPools::api_member_needs_delegate_migration(member) } + + fn member_total_balance(member: AccountId) -> Balance { + NominationPools::api_member_total_balance(member) + } } impl pallet_staking_runtime_api::StakingApi for Runtime { diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index fc87fea57ba2..6a0a3351129e 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -2772,6 +2772,10 @@ impl_runtime_apis! { fn member_needs_delegate_migration(member: AccountId) -> bool { NominationPools::api_member_needs_delegate_migration(member) } + + fn member_total_balance(member: AccountId) -> Balance { + NominationPools::api_member_total_balance(member) + } } impl pallet_staking_runtime_api::StakingApi for Runtime { diff --git a/substrate/frame/nomination-pools/runtime-api/src/lib.rs b/substrate/frame/nomination-pools/runtime-api/src/lib.rs index 67627e0acb13..7e181dd0f859 100644 --- a/substrate/frame/nomination-pools/runtime-api/src/lib.rs +++ b/substrate/frame/nomination-pools/runtime-api/src/lib.rs @@ -63,5 +63,8 @@ sp_api::decl_runtime_apis! { /// [`migrate_delegation`](pallet_nomination_pools::Call::migrate_delegation) /// to migrate the funds of the pool member. fn member_needs_delegate_migration(member: AccountId) -> bool; + + /// Returns the total contribution of a pool member including any balance that is unbonding. + fn member_total_balance(who: AccountId) -> Balance; } } From 9925a9ff5d99c8dec1a465133bda179772dbe058 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 7 Jul 2024 04:20:16 +0200 Subject: [PATCH 19/57] ensure member can't bond extra if they are in unmigrated state --- .../frame/delegated-staking/src/impls.rs | 2 +- substrate/frame/nomination-pools/src/lib.rs | 15 ++- .../test-delegate-stake/src/lib.rs | 98 ++++++++++++++++++- substrate/primitives/staking/src/lib.rs | 5 +- 4 files changed, 112 insertions(+), 8 deletions(-) diff --git a/substrate/frame/delegated-staking/src/impls.rs b/substrate/frame/delegated-staking/src/impls.rs index ea63ae582788..a3bb5eaed5b8 100644 --- a/substrate/frame/delegated-staking/src/impls.rs +++ b/substrate/frame/delegated-staking/src/impls.rs @@ -36,7 +36,7 @@ impl DelegationInterface for Pallet { Delegation::::get(&delegator.get()).map(|d| d.amount) } - /// Delegate funds to an `Agent`. + /// Register a new `Agent` and delegate funds to it. fn delegate( who: Delegator, agent: Agent, diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index cf1d6f35ab35..aeb2e7ac2995 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2001,6 +2001,8 @@ pub mod pallet { pool_id: PoolId, ) -> DispatchResult { let who = ensure_signed(origin)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); ensure!(amount >= MinJoinBond::::get(), Error::::MinimumBondNotMet); // If a member already exists that means they already belong to a pool @@ -3333,6 +3335,11 @@ impl Pallet { let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&member_account)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(bonded_pool.id), Error::::NotMigrated); + // ensure member is not in an un-migrated state. + ensure!(!Self::api_member_needs_delegate_migration(member_account.clone()), Error::::NotMigrated); + // payout related stuff: we must claim the payouts, and updated recorded payout data // before updating the bonded pool points, similar to that of `join` transaction. reward_pool.update_records( @@ -3890,10 +3897,10 @@ impl Pallet { PoolMembers::::get(who.clone()) .map(|pool_member| { - if Self::api_pool_needs_delegate_migration(pool_member.pool_id) { - // the pool needs to be migrated before members can be migrated. - return false - } + // if Self::api_pool_needs_delegate_migration(pool_member.pool_id) { + // // the pool needs to be migrated before members can be migrated. + // return false + // } let member_balance = pool_member.total_balance(); let delegated_balance = diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs index 0f5593408abc..7ffc8328fdc1 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -999,7 +999,6 @@ fn pool_migration_e2e() { LegacyAdapter::set(false); // cannot migrate the member delegation unless pool is migrated first. - assert!(!Pools::api_member_needs_delegate_migration(20)); assert_noop!( Pools::migrate_delegation(RuntimeOrigin::signed(10), 20), PoolsError::::NotMigrated @@ -1184,3 +1183,100 @@ fn pool_migration_e2e() { ); }) } + +#[test] +fn disable_pool_operations_on_non_migrated() { + new_test_ext().execute_with(|| { + LegacyAdapter::set(true); + assert_eq!(Balances::minimum_balance(), 5); + assert_eq!(Staking::current_era(), None); + + // create the pool with TransferStake strategy. + assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10)); + assert_eq!(LastPoolId::::get(), 1); + + // have the pool nominate. + assert_ok!(Pools::nominate(RuntimeOrigin::signed(10), 1, vec![1, 2, 3])); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 50 }] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Created { depositor: 10, pool_id: 1 }, + PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 50, joined: true }, + ] + ); + + let pre_20 = Balances::free_balance(20); + assert_ok!(Pools::join(RuntimeOrigin::signed(20), 10, 1)); + + // verify members balance is moved to pool. + assert_eq!(Balances::free_balance(20), pre_20 - 10); + + assert_eq!( + staking_events_since_last_call(), + vec![ + StakingEvent::Bonded { stash: POOL1_BONDED, amount: 10 }, + ] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: 10, joined: true }, + ] + ); + + // we reset the adapter to `DelegateStake`. + LegacyAdapter::set(false); + + // pool is pending migration. + assert!(Pools::api_pool_needs_delegate_migration(1)); + // cannot join a pool that is pending migration. + assert_noop!(Pools::join(RuntimeOrigin::signed(21), 10, 1), PoolsError::::NotMigrated); + + assert_ok!(Pools::migrate_pool_to_delegate_stake(RuntimeOrigin::signed(10), 1)); + assert_eq!( + delegated_staking_events_since_last_call(), + vec![ + DelegatedStakingEvent::Delegated { agent: POOL1_BONDED, delegator: DelegatedStaking::generate_proxy_delegator(Agent::from(POOL1_BONDED)).get(), amount: 50 + 10 }, + ] + ); + + // `bond_extra` for 20 **before** the migration would fail. + assert_noop!(Pools::bond_extra(RuntimeOrigin::signed(20), BondExtra::FreeBalance(5)), PoolsError::::NotMigrated); + + // migrate 20 + assert_ok!(Pools::migrate_delegation(RuntimeOrigin::signed(10), 20)); + // now `bond_extra` for 20 works. + assert_ok!(Pools::bond_extra(RuntimeOrigin::signed(20), BondExtra::FreeBalance(5))); + + assert_eq!( + staking_events_since_last_call(), + vec![ + StakingEvent::Bonded { stash: POOL1_BONDED, amount: 5 }, + ] + ); + + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: 5, joined: false }, + ] + ); + + assert_eq!( + delegated_staking_events_since_last_call(), + vec![ + DelegatedStakingEvent::MigratedDelegation { + agent: POOL1_BONDED, + delegator: 20, + amount: 10 + }, + DelegatedStakingEvent::Delegated { agent: POOL1_BONDED, delegator: 20, amount: 5 }, + ] + ); + }) +} diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index c1cf7f2778ff..9a334cccbe28 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -530,7 +530,8 @@ pub trait DelegationInterface { /// Delegate funds to `Agent`. /// - /// Only used for the initial delegation. Use [`Self::delegate_extra`] to add more delegation. + /// Only used for the first delegation to the agent. Use [`Self::delegate_extra`] if agent has + /// already some delegations. fn delegate( delegator: Delegator, agent: Agent, @@ -540,7 +541,7 @@ pub trait DelegationInterface { /// Add more delegation to the `Agent`. /// - /// If this is the first delegation, use [`Self::delegate`] instead. + /// If this is the first delegation to this `Agent`, use [`Self::delegate`] instead. fn delegate_extra( delegator: Delegator, agent: Agent, From 4a9f35fabfad7fa2f384b07b1af3c69239bdda72 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 7 Jul 2024 04:40:27 +0200 Subject: [PATCH 20/57] gate all calls if non migrated state as defensive measure --- substrate/frame/nomination-pools/src/lib.rs | 66 +++++++++++++++++-- .../test-delegate-stake/src/lib.rs | 52 ++++++++++----- 2 files changed, 94 insertions(+), 24 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index aeb2e7ac2995..7542a05b6a3f 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2123,6 +2123,12 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; let member_account = T::Lookup::lookup(member_account)?; + // ensure member is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(member_account.clone()), + Error::::NotMigrated + ); + let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&member_account)?; @@ -2206,6 +2212,9 @@ pub mod pallet { num_slashing_spans: u32, ) -> DispatchResult { let _ = ensure_signed(origin)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); + let pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; // For now we only allow a pool to withdraw unbonded if its not destroying. If the pool @@ -2252,6 +2261,12 @@ pub mod pallet { ) -> DispatchResultWithPostInfo { let caller = ensure_signed(origin)?; let member_account = T::Lookup::lookup(member_account)?; + // ensure member is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(member_account.clone()), + Error::::NotMigrated + ); + let mut member = PoolMembers::::get(&member_account).ok_or(Error::::PoolMemberNotFound)?; let current_era = T::StakeAdapter::current_era(); @@ -2467,6 +2482,8 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; let bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); ensure!(bonded_pool.can_nominate(&who), Error::::NotNominator); let depositor_points = PoolMembers::::get(&bonded_pool.roles.depositor) @@ -2501,6 +2518,8 @@ pub mod pallet { let who = ensure_signed(origin)?; let mut bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; ensure!(bonded_pool.state != PoolState::Destroying, Error::::CanNotChangeState); + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); if bonded_pool.can_toggle_state(&who) { bonded_pool.set_state(state); @@ -2536,6 +2555,8 @@ pub mod pallet { .can_set_metadata(&who), Error::::DoesNotHavePermission ); + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); Metadata::::mutate(pool_id, |pool_meta| *pool_meta = metadata); @@ -2612,6 +2633,9 @@ pub mod pallet { }, }; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); + match new_root { ConfigOp::Noop => (), ConfigOp::Remove => bonded_pool.roles.root = None, @@ -2658,8 +2682,9 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::chill())] pub fn chill(origin: OriginFor, pool_id: PoolId) -> DispatchResult { let who = ensure_signed(origin)?; - let bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); let depositor_points = PoolMembers::::get(&bonded_pool.roles.depositor) .ok_or(Error::::PoolMemberNotFound)? @@ -2711,9 +2736,14 @@ pub mod pallet { permission: ClaimPermission, ) -> DispatchResult { let who = ensure_signed(origin)?; - ensure!(PoolMembers::::contains_key(&who), Error::::PoolMemberNotFound); + // ensure member is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(who.clone()), + Error::::NotMigrated + ); + ClaimPermissions::::mutate(who, |source| { *source = permission; }); @@ -2747,6 +2777,9 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; let mut bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); + ensure!(bonded_pool.can_manage_commission(&who), Error::::DoesNotHavePermission); let mut reward_pool = RewardPools::::get(pool_id) @@ -2783,6 +2816,9 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; let mut bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); + ensure!(bonded_pool.can_manage_commission(&who), Error::::DoesNotHavePermission); bonded_pool.commission.try_update_max(pool_id, max_commission)?; @@ -2805,6 +2841,8 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; let mut bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); ensure!(bonded_pool.can_manage_commission(&who), Error::::DoesNotHavePermission); bonded_pool.commission.try_update_change_rate(change_rate)?; @@ -2856,6 +2894,8 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; let mut bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); ensure!(bonded_pool.can_manage_commission(&who), Error::::DoesNotHavePermission); bonded_pool.commission.claim_permission = permission.clone(); @@ -3324,6 +3364,12 @@ impl Pallet { member_account: T::AccountId, extra: BondExtra>, ) -> DispatchResult { + // ensure member is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(member_account.clone()), + Error::::NotMigrated + ); + if signer != member_account { ensure!( ClaimPermissions::::get(&member_account).can_bond_extra(), @@ -3335,11 +3381,6 @@ impl Pallet { let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&member_account)?; - // ensure pool is not in an un-migrated state. - ensure!(!Self::api_pool_needs_delegate_migration(bonded_pool.id), Error::::NotMigrated); - // ensure member is not in an un-migrated state. - ensure!(!Self::api_member_needs_delegate_migration(member_account.clone()), Error::::NotMigrated); - // payout related stuff: we must claim the payouts, and updated recorded payout data // before updating the bonded pool points, similar to that of `join` transaction. reward_pool.update_records( @@ -3378,6 +3419,8 @@ impl Pallet { fn do_claim_commission(who: T::AccountId, pool_id: PoolId) -> DispatchResult { let bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); ensure!(bonded_pool.can_claim_commission(&who), Error::::DoesNotHavePermission); let mut reward_pool = RewardPools::::get(pool_id) @@ -3424,6 +3467,12 @@ impl Pallet { signer: T::AccountId, member_account: T::AccountId, ) -> DispatchResult { + // ensure member is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(member_account.clone()), + Error::::NotMigrated + ); + if signer != member_account { ensure!( ClaimPermissions::::get(&member_account).can_claim_payout(), @@ -3446,6 +3495,9 @@ impl Pallet { fn do_adjust_pool_deposit(who: T::AccountId, pool: PoolId) -> DispatchResult { let bonded_pool = BondedPool::::get(pool).ok_or(Error::::PoolNotFound)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool), Error::::NotMigrated); + let reward_acc = &bonded_pool.reward_account(); let pre_frozen_balance = T::Currency::balance_frozen(&FreezeReason::PoolMinBalance.into(), reward_acc); diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs index 7ffc8328fdc1..f4d1a87ee033 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -1218,15 +1218,11 @@ fn disable_pool_operations_on_non_migrated() { assert_eq!( staking_events_since_last_call(), - vec![ - StakingEvent::Bonded { stash: POOL1_BONDED, amount: 10 }, - ] + vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 10 },] ); assert_eq!( pool_events_since_last_call(), - vec![ - PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: 10, joined: true }, - ] + vec![PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: 10, joined: true },] ); // we reset the adapter to `DelegateStake`. @@ -1235,18 +1231,44 @@ fn disable_pool_operations_on_non_migrated() { // pool is pending migration. assert!(Pools::api_pool_needs_delegate_migration(1)); // cannot join a pool that is pending migration. - assert_noop!(Pools::join(RuntimeOrigin::signed(21), 10, 1), PoolsError::::NotMigrated); + assert_noop!( + Pools::join(RuntimeOrigin::signed(21), 10, 1), + PoolsError::::NotMigrated + ); + // cannot unbond from a pool that is pending migration. + assert_noop!( + Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(20), 1, 0), + PoolsError::::NotMigrated + ); assert_ok!(Pools::migrate_pool_to_delegate_stake(RuntimeOrigin::signed(10), 1)); assert_eq!( delegated_staking_events_since_last_call(), - vec![ - DelegatedStakingEvent::Delegated { agent: POOL1_BONDED, delegator: DelegatedStaking::generate_proxy_delegator(Agent::from(POOL1_BONDED)).get(), amount: 50 + 10 }, - ] + vec![DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: DelegatedStaking::generate_proxy_delegator(Agent::from(POOL1_BONDED)) + .get(), + amount: 50 + 10 + },] ); // `bond_extra` for 20 **before** the migration would fail. - assert_noop!(Pools::bond_extra(RuntimeOrigin::signed(20), BondExtra::FreeBalance(5)), PoolsError::::NotMigrated); + assert_noop!( + Pools::bond_extra(RuntimeOrigin::signed(20), BondExtra::FreeBalance(5)), + PoolsError::::NotMigrated + ); + + // cannot claim payout either. + assert_noop!( + Pools::claim_payout(RuntimeOrigin::signed(20)), + PoolsError::::NotMigrated + ); + + // cannot unbond + assert_noop!( + Pools::unbond(RuntimeOrigin::signed(20), 20, 5), + PoolsError::::NotMigrated + ); // migrate 20 assert_ok!(Pools::migrate_delegation(RuntimeOrigin::signed(10), 20)); @@ -1255,16 +1277,12 @@ fn disable_pool_operations_on_non_migrated() { assert_eq!( staking_events_since_last_call(), - vec![ - StakingEvent::Bonded { stash: POOL1_BONDED, amount: 5 }, - ] + vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 5 },] ); assert_eq!( pool_events_since_last_call(), - vec![ - PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: 5, joined: false }, - ] + vec![PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: 5, joined: false },] ); assert_eq!( From bc3ce11322847ea006cc20fb755ba9eb950c9767 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sun, 7 Jul 2024 04:57:24 +0200 Subject: [PATCH 21/57] ensure all calls are gated --- .../test-delegate-stake/src/lib.rs | 84 ++++++++++++++++--- 1 file changed, 73 insertions(+), 11 deletions(-) diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs index f4d1a87ee033..e70650408416 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -25,8 +25,8 @@ use frame_support::{ }; use mock::*; use pallet_nomination_pools::{ - BondExtra, BondedPools, Error as PoolsError, Event as PoolsEvent, LastPoolId, PoolMember, - PoolMembers, PoolState, + BondExtra, BondedPools, CommissionChangeRate, ConfigOp, Error as PoolsError, + Event as PoolsEvent, LastPoolId, PoolMember, PoolMembers, PoolState, }; use pallet_staking::{ CurrentEra, Error as StakingError, Event as StakingEvent, Payee, RewardDestination, @@ -34,7 +34,7 @@ use pallet_staking::{ use pallet_delegated_staking::{Error as DelegatedStakingError, Event as DelegatedStakingEvent}; -use sp_runtime::{bounded_btree_map, traits::Zero}; +use sp_runtime::{bounded_btree_map, traits::Zero, Perbill}; use sp_staking::Agent; #[test] @@ -1230,17 +1230,72 @@ fn disable_pool_operations_on_non_migrated() { // pool is pending migration. assert!(Pools::api_pool_needs_delegate_migration(1)); - // cannot join a pool that is pending migration. + + // ensure pool mutation is not allowed until pool is migrated. assert_noop!( Pools::join(RuntimeOrigin::signed(21), 10, 1), PoolsError::::NotMigrated ); - // cannot unbond from a pool that is pending migration. assert_noop!( - Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(20), 1, 0), + Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(10), 1, 0), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::nominate(RuntimeOrigin::signed(10), 1, vec![1, 2, 3]), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::set_state(RuntimeOrigin::signed(10), 1, PoolState::Blocked), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::set_metadata(RuntimeOrigin::signed(10), 1, vec![1, 1]), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::update_roles( + RuntimeOrigin::signed(10), + 1, + ConfigOp::Set(5), + ConfigOp::Set(6), + ConfigOp::Set(7) + ), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::chill(RuntimeOrigin::signed(10), 1), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::set_commission(RuntimeOrigin::signed(10), 1, None), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::set_commission_max(RuntimeOrigin::signed(10), 1, Zero::zero()), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::set_commission_change_rate( + RuntimeOrigin::signed(10), + 1, + CommissionChangeRate { max_increase: Perbill::from_percent(1), min_delay: 2_u64 } + ), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::claim_commission(RuntimeOrigin::signed(10), 1), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::adjust_pool_deposit(RuntimeOrigin::signed(10), 1), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::set_commission_claim_permission(RuntimeOrigin::signed(10), 1, None), PoolsError::::NotMigrated ); + // migrate the pool. assert_ok!(Pools::migrate_pool_to_delegate_stake(RuntimeOrigin::signed(10), 1)); assert_eq!( delegated_staking_events_since_last_call(), @@ -1252,23 +1307,30 @@ fn disable_pool_operations_on_non_migrated() { },] ); - // `bond_extra` for 20 **before** the migration would fail. + // member is pending migration. + assert!(Pools::api_member_needs_delegate_migration(20)); + + // ensure member mutation is not allowed until member's delegation is migrated. assert_noop!( Pools::bond_extra(RuntimeOrigin::signed(20), BondExtra::FreeBalance(5)), PoolsError::::NotMigrated ); - - // cannot claim payout either. + assert_noop!( + Pools::bond_extra_other(RuntimeOrigin::signed(10), 20, BondExtra::Rewards), + PoolsError::::NotMigrated + ); assert_noop!( Pools::claim_payout(RuntimeOrigin::signed(20)), PoolsError::::NotMigrated ); - - // cannot unbond assert_noop!( Pools::unbond(RuntimeOrigin::signed(20), 20, 5), PoolsError::::NotMigrated ); + assert_noop!( + Pools::withdraw_unbonded(RuntimeOrigin::signed(20), 20, 0), + PoolsError::::NotMigrated + ); // migrate 20 assert_ok!(Pools::migrate_delegation(RuntimeOrigin::signed(10), 20)); From 5b916443a6eedf3784cd52bab8b9ff25b881221a Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Jul 2024 04:37:19 +0530 Subject: [PATCH 22/57] add runtime api for pool balance --- polkadot/runtime/westend/src/lib.rs | 4 ++++ substrate/bin/node/runtime/src/lib.rs | 4 ++++ substrate/frame/nomination-pools/runtime-api/src/lib.rs | 3 +++ substrate/frame/nomination-pools/src/lib.rs | 6 ++++++ 4 files changed, 17 insertions(+) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 3bd5e4570a65..a8d61757ad33 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2338,6 +2338,10 @@ sp_api::impl_runtime_apis! { fn member_total_balance(member: AccountId) -> Balance { NominationPools::api_member_total_balance(member) } + + fn pool_balance(pool_id: pallet_nomination_pools::PoolId) -> Balance { + NominationPools::api_pool_balance(pool_id) + } } impl pallet_staking_runtime_api::StakingApi for Runtime { diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 6a0a3351129e..0400d46ecd02 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -2776,6 +2776,10 @@ impl_runtime_apis! { fn member_total_balance(member: AccountId) -> Balance { NominationPools::api_member_total_balance(member) } + + fn pool_balance(pool_id: pallet_nomination_pools::PoolId) -> Balance { + NominationPools::api_pool_balance(pool_id) + } } impl pallet_staking_runtime_api::StakingApi for Runtime { diff --git a/substrate/frame/nomination-pools/runtime-api/src/lib.rs b/substrate/frame/nomination-pools/runtime-api/src/lib.rs index 7e181dd0f859..d81ad1dd4954 100644 --- a/substrate/frame/nomination-pools/runtime-api/src/lib.rs +++ b/substrate/frame/nomination-pools/runtime-api/src/lib.rs @@ -66,5 +66,8 @@ sp_api::decl_runtime_apis! { /// Returns the total contribution of a pool member including any balance that is unbonding. fn member_total_balance(who: AccountId) -> Balance; + + /// Total balance contributed to the pool. + fn pool_balance(pool_id: PoolId) -> Balance; } } diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 7542a05b6a3f..3c16c3d0c14b 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3973,6 +3973,12 @@ impl Pallet { .map(|m| m.total_balance()) .unwrap_or_default() } + + /// Total balance contributed to the pool. + pub fn api_pool_balance(pool_id: PoolId) -> BalanceOf { + T::StakeAdapter::total_balance(Pool::from(Self::generate_bonded_account(pool_id))) + .unwrap_or_default() + } } impl sp_staking::OnStakingUpdate> for Pallet { From 6ceb1ba5a54b15eab3afed4c45e288b3fbe5ed8a Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Jul 2024 04:38:52 +0530 Subject: [PATCH 23/57] remove unused imports --- polkadot/runtime/westend/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index a8d61757ad33..80c82aa095fb 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2643,7 +2643,6 @@ sp_api::impl_runtime_apis! { #[cfg(all(test, feature = "try-runtime"))] mod remote_tests { use super::*; - use frame_support::assert_noop; use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect}; use remote_externalities::{ Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport, @@ -2685,7 +2684,6 @@ mod remote_tests { if var("RUN_MIGRATION_TESTS").is_err() { return; } - use frame_support::{assert_ok, migrations::SteppedMigration, traits::TryState}; sp_tracing::try_init_simple(); let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); From de480834bbcca3558036b72ba3da5fa82b97388c Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Jul 2024 06:00:03 +0530 Subject: [PATCH 24/57] fix import --- polkadot/runtime/westend/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 80c82aa095fb..c07be7e8cf4c 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2684,6 +2684,7 @@ mod remote_tests { if var("RUN_MIGRATION_TESTS").is_err() { return; } + use frame_support::assert_ok; sp_tracing::try_init_simple(); let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); From ab2317b9f4f628bf8b4923d6027f24a800dde54c Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Jul 2024 08:53:36 +0530 Subject: [PATCH 25/57] fix test --- .../test-delegate-stake/src/lib.rs | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs index e70650408416..53633fda1bea 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -32,7 +32,7 @@ use pallet_staking::{ CurrentEra, Error as StakingError, Event as StakingEvent, Payee, RewardDestination, }; -use pallet_delegated_staking::{Error as DelegatedStakingError, Event as DelegatedStakingEvent}; +use pallet_delegated_staking::Event as DelegatedStakingEvent; use sp_runtime::{bounded_btree_map, traits::Zero, Perbill}; use sp_staking::Agent; @@ -1030,13 +1030,17 @@ fn pool_migration_e2e() { // move to era 5 when 20 can withdraw unbonded funds. CurrentEra::::set(Some(5)); - // Unbond works even without claiming delegation. Lets unbond 22. - assert_ok!(Pools::unbond(RuntimeOrigin::signed(22), 22, 5)); + + // Cannot unbond without claiming delegation. Lets unbond 22. + assert_noop!( + Pools::unbond(RuntimeOrigin::signed(22), 22, 5), + PoolsError::::NotMigrated + ); // withdraw fails for 20 before claiming delegation assert_noop!( Pools::withdraw_unbonded(RuntimeOrigin::signed(20), 20, 10), - DelegatedStakingError::::NotDelegator + PoolsError::::NotMigrated ); let pre_claim_balance_20 = Balances::total_balance(&20); @@ -1059,17 +1063,11 @@ fn pool_migration_e2e() { assert_eq!( staking_events_since_last_call(), - vec![ - StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 5 }, - StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 5 } - ] + vec![StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 5 }] ); assert_eq!( pool_events_since_last_call(), - vec![ - PoolsEvent::Unbonded { member: 22, pool_id: 1, balance: 5, points: 5, era: 8 }, - PoolsEvent::Withdrawn { member: 20, pool_id: 1, balance: 5, points: 5 }, - ] + vec![PoolsEvent::Withdrawn { member: 20, pool_id: 1, balance: 5, points: 5 },] ); assert_eq!( delegated_staking_events_since_last_call(), @@ -1131,14 +1129,17 @@ fn pool_migration_e2e() { assert_eq!(Balances::total_balance(&22), 10); assert_eq!(Balances::total_balance_on_hold(&22), 10); - // withdraw fails since 22 only unbonds at era 8. + // unbond 22 should work now + assert_ok!(Pools::unbond(RuntimeOrigin::signed(22), 22, 5)); + + // withdraw fails since 22 only unbonds after era 9. assert_noop!( Pools::withdraw_unbonded(RuntimeOrigin::signed(22), 22, 5), PoolsError::::CannotWithdrawAny ); // go to era when 22 can unbond - CurrentEra::::set(Some(10)); + CurrentEra::::set(Some(9)); // withdraw works now assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(22), 22, 10)); @@ -1151,6 +1152,7 @@ fn pool_migration_e2e() { staking_events_since_last_call(), vec![ StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 10 }, + StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 5 }, StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 5 } ] ); @@ -1161,6 +1163,7 @@ fn pool_migration_e2e() { PoolsEvent::Withdrawn { member: 21, pool_id: 1, balance: 10, points: 10 }, // 21 was fully unbonding and removed from pool. PoolsEvent::MemberRemoved { member: 21, pool_id: 1 }, + PoolsEvent::Unbonded { member: 22, pool_id: 1, balance: 5, points: 5, era: 9 }, PoolsEvent::Withdrawn { member: 22, pool_id: 1, balance: 5, points: 5 }, ] ); From d5634c4edff15b7bb901d17bec29449daa52cae4 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 12 Jul 2024 07:56:34 +0530 Subject: [PATCH 26/57] add prdoc --- prdoc/pr_4822.prdoc | 23 +++++++++++++++++++ .../src/traits/tokens/fungible/hold.rs | 1 + 2 files changed, 24 insertions(+) create mode 100644 prdoc/pr_4822.prdoc diff --git a/prdoc/pr_4822.prdoc b/prdoc/pr_4822.prdoc new file mode 100644 index 000000000000..91f989d83c6e --- /dev/null +++ b/prdoc/pr_4822.prdoc @@ -0,0 +1,23 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Ensure as many as possible pool members can migrate to `DelegateStake` + +doc: + - audience: Runtime Dev + description: | + 1. Allows pool members to use their total balance while joining pool with `DelegateStake`. + 2. Gates call mutating pool or member in unmigrated state. + 3. Runtime apis for reading pool and member balance. + +crates: + - name: westend-runtime + bump: patch + - name: kitchensink-runtime + bump: patch + - name: pallet-delegated-staking + bump: patch + - name: pallet-nomination-pools + bump: patch + - name: sp-staking + bump: patch diff --git a/substrate/frame/support/src/traits/tokens/fungible/hold.rs b/substrate/frame/support/src/traits/tokens/fungible/hold.rs index 0611eb0b0ab0..28ece25c91d4 100644 --- a/substrate/frame/support/src/traits/tokens/fungible/hold.rs +++ b/substrate/frame/support/src/traits/tokens/fungible/hold.rs @@ -361,6 +361,7 @@ pub trait Mutate: ensure!(amount <= liquid, TokenError::Frozen); ensure!(amount <= have, TokenError::FundsUnavailable); } + // We want to make sure we can deposit the amount in advance. If we can't then something is // very wrong. ensure!(Self::can_deposit(dest, amount, Extant) == Success, TokenError::CannotCreate); From 0da98070c40f66c6647cd66a34c48f99e1689f93 Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 13 Jul 2024 15:30:02 +0530 Subject: [PATCH 27/57] fix prdoc bump --- prdoc/pr_4822.prdoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prdoc/pr_4822.prdoc b/prdoc/pr_4822.prdoc index 91f989d83c6e..d18875e4c43f 100644 --- a/prdoc/pr_4822.prdoc +++ b/prdoc/pr_4822.prdoc @@ -12,12 +12,12 @@ doc: crates: - name: westend-runtime - bump: patch + bump: minor - name: kitchensink-runtime bump: patch - name: pallet-delegated-staking bump: patch - name: pallet-nomination-pools - bump: patch + bump: minor - name: sp-staking bump: patch From 1514c2b097e59240ebaa56fb436e3d5602e219aa Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 13 Jul 2024 16:48:26 +0530 Subject: [PATCH 28/57] add missing crate to prdoc --- prdoc/pr_4822.prdoc | 2 + .../frame/delegated-staking/src/migration.rs | 1163 +++++++++++++++++ 2 files changed, 1165 insertions(+) create mode 100644 substrate/frame/delegated-staking/src/migration.rs diff --git a/prdoc/pr_4822.prdoc b/prdoc/pr_4822.prdoc index d18875e4c43f..44f3e41d8d5a 100644 --- a/prdoc/pr_4822.prdoc +++ b/prdoc/pr_4822.prdoc @@ -21,3 +21,5 @@ crates: bump: minor - name: sp-staking bump: patch + - name: pallet-nomination-pools-runtime-api + bump: minor diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs new file mode 100644 index 000000000000..a9222ea53d75 --- /dev/null +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -0,0 +1,1163 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use super::*; +use crate::log; +use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade}; +use sp_std::{collections::btree_map::BTreeMap, vec::Vec}; + +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + +/// Exports for versioned migration `type`s for this pallet. +pub mod versioned { + use super::*; + + /// v8: Adds commission claim permissions to `BondedPools`. + pub type V7ToV8 = frame_support::migrations::VersionedMigration< + 7, + 8, + v8::VersionUncheckedMigrateV7ToV8, + crate::pallet::Pallet, + ::DbWeight, + >; + + /// Migration V6 to V7 wrapped in a [`frame_support::migrations::VersionedMigration`], ensuring + /// the migration is only performed when on-chain version is 6. + pub type V6ToV7 = frame_support::migrations::VersionedMigration< + 6, + 7, + v7::VersionUncheckedMigrateV6ToV7, + crate::pallet::Pallet, + ::DbWeight, + >; + + /// Wrapper over `MigrateToV6` with convenience version checks. + pub type V5toV6 = frame_support::migrations::VersionedMigration< + 5, + 6, + v6::MigrateToV6, + crate::pallet::Pallet, + ::DbWeight, + >; +} + +pub mod unversioned { + use super::*; + + /// Checks and updates `TotalValueLocked` if out of sync. + pub struct TotalValueLockedSync(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for TotalValueLockedSync { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + fn on_runtime_upgrade() -> Weight { + let migrated = BondedPools::::count(); + + // recalculate the `TotalValueLocked` to compare with the current on-chain TVL which may + // be out of sync. + let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); + let onchain_tvl = TotalValueLocked::::get(); + + let writes = if tvl != onchain_tvl { + TotalValueLocked::::set(tvl); + + log!( + info, + "on-chain TVL was out of sync, update. Old: {:?}, new: {:?}", + onchain_tvl, + tvl + ); + + // writes: onchain version + set total value locked. + 2 + } else { + log!(info, "on-chain TVL was OK: {:?}", tvl); + + // writes: onchain version write. + 1 + }; + + // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain + // version + // + // writes: current version + (maybe) TVL + T::DbWeight::get() + .reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), writes) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + Ok(()) + } + } + + /// Migrate existing pools from [`adapter::StakeStrategyType::Transfer`] to + /// [`adapter::StakeStrategyType::Delegate`]. + /// + /// Note: This only migrates the pools, the members are not migrated. They can use the + /// permission-less [`Pallet::migrate_delegation()`] to migrate their funds. + /// + /// This migration does not break any existing pool storage item, does not need to happen in any + /// sequence and hence can be applied unversioned on a production runtime. + /// + /// Takes `MaxPools` as type parameter to limit the number of pools that should be migrated in a + /// single block. It should be set such that migration weight does not exceed the block weight + /// limit. If all pools can be safely migrated, it is good to keep this number a little higher + /// than the actual number of pools to handle any extra pools created while the migration is + /// proposed, and before it is executed. + /// + /// If there are pools that fail to migrate or did not fit in the bounds, the remaining pools + /// can be migrated via the permission-less extrinsic [`Call::migrate_pool_to_delegate_stake`]. + pub struct DelegationStakeMigration(sp_std::marker::PhantomData<(T, MaxPools)>); + + impl> OnRuntimeUpgrade for DelegationStakeMigration { + fn on_runtime_upgrade() -> Weight { + let mut count: u32 = 0; + + BondedPools::::iter_keys().take(MaxPools::get() as usize).for_each(|id| { + let pool_acc = Pallet::::generate_bonded_account(id); + + // only migrate if the pool is in Transfer Strategy. + if T::StakeAdapter::pool_strategy(Pool::from(pool_acc)) == + adapter::StakeStrategyType::Transfer + { + let _ = Pallet::::migrate_to_delegate_stake(id).map_err(|err| { + log!( + warn, + "failed to migrate pool {:?} to delegate stake strategy with err: {:?}", + id, + err + ) + }); + count.saturating_inc(); + } + }); + + log!(info, "migrated {:?} pools to delegate stake strategy", count); + + // reads: (bonded pool key + current pool strategy) * MaxPools (worst case) + T::DbWeight::get() + .reads_writes(2, 0) + .saturating_mul(MaxPools::get() as u64) + // migration weight: `pool_migrate` weight * count + .saturating_add(T::WeightInfo::pool_migrate().saturating_mul(count.into())) + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + // ensure stake adapter is correct. + ensure!( + T::StakeAdapter::strategy_type() == adapter::StakeStrategyType::Delegate, + "Current strategy is not `Delegate" + ); + + if BondedPools::::count() > MaxPools::get() { + // we log a warning if the number of pools exceeds the bound. + log!( + warn, + "Number of pools {} exceeds the maximum bound {}. This would leave some pools unmigrated.", BondedPools::::count(), MaxPools::get() + ); + } + + let mut pool_balances: Vec> = Vec::new(); + BondedPools::::iter_keys().take(MaxPools::get() as usize).for_each(|id| { + let pool_account = Pallet::::generate_bonded_account(id); + + // we ensure migration is idempotent. + let pool_balance = T::StakeAdapter::total_balance(Pool::from(pool_account.clone())) + // we check actual account balance if pool has not migrated yet. + .unwrap_or(T::Currency::total_balance(&pool_account)); + + pool_balances.push(pool_balance); + }); + + Ok(pool_balances.encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { + let expected_pool_balances: Vec> = Decode::decode(&mut &data[..]).unwrap(); + + for (index, id) in + BondedPools::::iter_keys().take(MaxPools::get() as usize).enumerate() + { + let pool_account = Pallet::::generate_bonded_account(id); + if T::StakeAdapter::pool_strategy(Pool::from(pool_account.clone())) == + adapter::StakeStrategyType::Transfer + { + log!(error, "Pool {} failed to migrate", id,); + return Err(TryRuntimeError::Other("Pool failed to migrate")); + } + + let actual_balance = + T::StakeAdapter::total_balance(Pool::from(pool_account.clone())) + .expect("after migration, this should return a value"); + let expected_balance = expected_pool_balances.get(index).unwrap(); + + if actual_balance != *expected_balance { + log!( + error, + "Pool {} balance mismatch. Expected: {:?}, Actual: {:?}", + id, + expected_balance, + actual_balance + ); + return Err(TryRuntimeError::Other("Pool balance mismatch")); + } + + // account balance should be zero. + let pool_account_balance = T::Currency::total_balance(&pool_account); + if pool_account_balance != Zero::zero() { + log!( + error, + "Pool account balance was expected to be zero. Pool: {}, Balance: {:?}", + id, + pool_account_balance + ); + return Err(TryRuntimeError::Other("Pool account balance not migrated")); + } + } + + Ok(()) + } + } +} + +pub mod v8 { + use super::{v7::V7BondedPoolInner, *}; + + impl V7BondedPoolInner { + fn migrate_to_v8(self) -> BondedPoolInner { + BondedPoolInner { + commission: Commission { + current: self.commission.current, + max: self.commission.max, + change_rate: self.commission.change_rate, + throttle_from: self.commission.throttle_from, + // `claim_permission` is a new field. + claim_permission: None, + }, + member_counter: self.member_counter, + points: self.points, + roles: self.roles, + state: self.state, + } + } + } + + pub struct VersionUncheckedMigrateV7ToV8(sp_std::marker::PhantomData); + impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV7ToV8 { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + fn on_runtime_upgrade() -> Weight { + let mut translated = 0u64; + BondedPools::::translate::, _>(|_key, old_value| { + translated.saturating_inc(); + Some(old_value.migrate_to_v8()) + }); + T::DbWeight::get().reads_writes(translated, translated + 1) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + // Check new `claim_permission` field is present. + ensure!( + BondedPools::::iter() + .all(|(_, inner)| inner.commission.claim_permission.is_none()), + "`claim_permission` value has not been set correctly." + ); + Ok(()) + } + } +} + +/// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. +/// +/// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated +/// arbitrarily. Otherwise this migration could fail due to too high weight. +pub(crate) mod v7 { + use super::*; + + #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, DebugNoBound, PartialEq, Clone)] + #[codec(mel_bound(T: Config))] + #[scale_info(skip_type_params(T))] + pub struct V7Commission { + pub current: Option<(Perbill, T::AccountId)>, + pub max: Option, + pub change_rate: Option>>, + pub throttle_from: Option>, + } + + #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, DebugNoBound, PartialEq, Clone)] + #[codec(mel_bound(T: Config))] + #[scale_info(skip_type_params(T))] + pub struct V7BondedPoolInner { + pub commission: V7Commission, + pub member_counter: u32, + pub points: BalanceOf, + pub roles: PoolRoles, + pub state: PoolState, + } + + #[allow(dead_code)] + #[derive(RuntimeDebugNoBound)] + #[cfg_attr(feature = "std", derive(Clone, PartialEq))] + pub struct V7BondedPool { + /// The identifier of the pool. + id: PoolId, + /// The inner fields. + inner: V7BondedPoolInner, + } + + impl V7BondedPool { + #[allow(dead_code)] + fn bonded_account(&self) -> T::AccountId { + Pallet::::generate_bonded_account(self.id) + } + } + + // NOTE: We cannot put a V7 prefix here since that would change the storage key. + #[frame_support::storage_alias] + pub type BondedPools = + CountedStorageMap, Twox64Concat, PoolId, V7BondedPoolInner>; + + pub struct VersionUncheckedMigrateV6ToV7(sp_std::marker::PhantomData); + impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV6ToV7 { + fn on_runtime_upgrade() -> Weight { + let migrated = BondedPools::::count(); + // The TVL should be the sum of all the funds that are actively staked and in the + // unbonding process of the account of each pool. + let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); + + TotalValueLocked::::set(tvl); + + log!(info, "Upgraded {} pools with a TVL of {:?}", migrated, tvl); + + // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain + // version + // + // writes: current version + TVL + T::DbWeight::get().reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), 2) + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { + // check that the `TotalValueLocked` written is actually the sum of `total_stake` of the + // `BondedPools`` + let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); + ensure!( + TotalValueLocked::::get() == tvl, + "TVL written is not equal to `Staking::total_stake` of all `BondedPools`." + ); + + // calculate the sum of `total_balance` of all `PoolMember` as the upper bound for the + // `TotalValueLocked`. + let total_balance_members: BalanceOf = PoolMembers::::iter() + .map(|(_, member)| member.total_balance()) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + ensure!( + TotalValueLocked::::get() <= total_balance_members, + "TVL is greater than the balance of all PoolMembers." + ); + + ensure!( + Pallet::::on_chain_storage_version() >= 7, + "nomination-pools::migration::v7: wrong storage version" + ); + + Ok(()) + } + } +} + +mod v6 { + use super::*; + + /// This migration would restrict reward account of pools to go below ED by doing a named + /// freeze on all the existing pools. + pub struct MigrateToV6(sp_std::marker::PhantomData); + + impl MigrateToV6 { + fn freeze_ed(pool_id: PoolId) -> Result<(), ()> { + let reward_acc = Pallet::::generate_reward_account(pool_id); + Pallet::::freeze_pool_deposit(&reward_acc).map_err(|e| { + log!(error, "Failed to freeze ED for pool {} with error: {:?}", pool_id, e); + () + }) + } + } + impl UncheckedOnRuntimeUpgrade for MigrateToV6 { + fn on_runtime_upgrade() -> Weight { + let mut success = 0u64; + let mut fail = 0u64; + + BondedPools::::iter_keys().for_each(|p| { + if Self::freeze_ed(p).is_ok() { + success.saturating_inc(); + } else { + fail.saturating_inc(); + } + }); + + if fail > 0 { + log!(error, "Failed to freeze ED for {} pools", fail); + } else { + log!(info, "Freezing ED succeeded for {} pools", success); + } + + let total = success.saturating_add(fail); + // freeze_ed = r:2 w:2 + // reads: (freeze_ed + bonded pool key) * total + // writes: freeze_ed * total + T::DbWeight::get().reads_writes(3u64.saturating_mul(total), 2u64.saturating_mul(total)) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { + // there should be no ED imbalances anymore.. + Pallet::::check_ed_imbalance() + } + } +} +pub mod v5 { + use super::*; + + #[derive(Decode)] + pub struct OldRewardPool { + last_recorded_reward_counter: T::RewardCounter, + last_recorded_total_payouts: BalanceOf, + total_rewards_claimed: BalanceOf, + } + + impl OldRewardPool { + fn migrate_to_v5(self) -> RewardPool { + RewardPool { + last_recorded_reward_counter: self.last_recorded_reward_counter, + last_recorded_total_payouts: self.last_recorded_total_payouts, + total_rewards_claimed: self.total_rewards_claimed, + total_commission_pending: Zero::zero(), + total_commission_claimed: Zero::zero(), + } + } + } + + /// This migration adds `total_commission_pending` and `total_commission_claimed` field to every + /// `RewardPool`, if any. + pub struct MigrateToV5(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV5 { + fn on_runtime_upgrade() -> Weight { + let in_code = Pallet::::in_code_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + log!( + info, + "Running migration with in-code storage version {:?} / onchain {:?}", + in_code, + onchain + ); + + if in_code == 5 && onchain == 4 { + let mut translated = 0u64; + RewardPools::::translate::, _>(|_id, old_value| { + translated.saturating_inc(); + Some(old_value.migrate_to_v5()) + }); + + in_code.put::>(); + log!(info, "Upgraded {} pools, storage to version {:?}", translated, in_code); + + // reads: translated + onchain version. + // writes: translated + current.put. + T::DbWeight::get().reads_writes(translated + 1, translated + 1) + } else { + log!(info, "Migration did not execute. This probably should be removed"); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + let rpool_keys = RewardPools::::iter_keys().count(); + let rpool_values = RewardPools::::iter_values().count(); + if rpool_keys != rpool_values { + log!(info, "🔥 There are {} undecodable RewardPools in storage. This migration will try to correct them. keys: {}, values: {}", rpool_keys.saturating_sub(rpool_values), rpool_keys, rpool_values); + } + + ensure!( + PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), + "There are undecodable PoolMembers in storage. This migration will not fix that." + ); + ensure!( + BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), + "There are undecodable BondedPools in storage. This migration will not fix that." + ); + ensure!( + SubPoolsStorage::::iter_keys().count() == + SubPoolsStorage::::iter_values().count(), + "There are undecodable SubPools in storage. This migration will not fix that." + ); + ensure!( + Metadata::::iter_keys().count() == Metadata::::iter_values().count(), + "There are undecodable Metadata in storage. This migration will not fix that." + ); + + Ok((rpool_values as u64).encode()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { + let old_rpool_values: u64 = Decode::decode(&mut &data[..]).unwrap(); + let rpool_keys = RewardPools::::iter_keys().count() as u64; + let rpool_values = RewardPools::::iter_values().count() as u64; + ensure!( + rpool_keys == rpool_values, + "There are STILL undecodable RewardPools - migration failed" + ); + + if old_rpool_values != rpool_values { + log!( + info, + "🎉 Fixed {} undecodable RewardPools.", + rpool_values.saturating_sub(old_rpool_values) + ); + } + + // ensure all RewardPools items now contain `total_commission_pending` and + // `total_commission_claimed` field. + ensure!( + RewardPools::::iter().all(|(_, reward_pool)| reward_pool + .total_commission_pending >= + Zero::zero() && reward_pool + .total_commission_claimed >= + Zero::zero()), + "a commission value has been incorrectly set" + ); + ensure!( + Pallet::::on_chain_storage_version() >= 5, + "nomination-pools::migration::v5: wrong storage version" + ); + + // These should not have been touched - just in case. + ensure!( + PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), + "There are undecodable PoolMembers in storage." + ); + ensure!( + BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), + "There are undecodable BondedPools in storage." + ); + ensure!( + SubPoolsStorage::::iter_keys().count() == + SubPoolsStorage::::iter_values().count(), + "There are undecodable SubPools in storage." + ); + ensure!( + Metadata::::iter_keys().count() == Metadata::::iter_values().count(), + "There are undecodable Metadata in storage." + ); + + Ok(()) + } + } +} + +pub mod v4 { + use super::*; + + #[derive(Decode)] + pub struct OldBondedPoolInner { + pub points: BalanceOf, + pub state: PoolState, + pub member_counter: u32, + pub roles: PoolRoles, + } + + impl OldBondedPoolInner { + fn migrate_to_v4(self) -> BondedPoolInner { + BondedPoolInner { + commission: Commission::default(), + member_counter: self.member_counter, + points: self.points, + state: self.state, + roles: self.roles, + } + } + } + + /// Migrates from `v3` directly to `v5` to avoid the broken `v4` migration. + #[allow(deprecated)] + pub type MigrateV3ToV5 = (v4::MigrateToV4, v5::MigrateToV5); + + /// # Warning + /// + /// To avoid mangled storage please use `MigrateV3ToV5` instead. + /// See: github.com/paritytech/substrate/pull/13715 + /// + /// This migration adds a `commission` field to every `BondedPoolInner`, if + /// any. + #[deprecated( + note = "To avoid mangled storage please use `MigrateV3ToV5` instead. See: github.com/paritytech/substrate/pull/13715" + )] + pub struct MigrateToV4(sp_std::marker::PhantomData<(T, U)>); + #[allow(deprecated)] + impl> OnRuntimeUpgrade for MigrateToV4 { + fn on_runtime_upgrade() -> Weight { + let current = Pallet::::in_code_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + log!( + info, + "Running migration with in-code storage version {:?} / onchain {:?}", + current, + onchain + ); + + if onchain == 3 { + log!(warn, "Please run MigrateToV5 immediately after this migration. See github.com/paritytech/substrate/pull/13715"); + let initial_global_max_commission = U::get(); + GlobalMaxCommission::::set(Some(initial_global_max_commission)); + log!( + info, + "Set initial global max commission to {:?}.", + initial_global_max_commission + ); + + let mut translated = 0u64; + BondedPools::::translate::, _>(|_key, old_value| { + translated.saturating_inc(); + Some(old_value.migrate_to_v4()) + }); + + StorageVersion::new(4).put::>(); + log!(info, "Upgraded {} pools, storage to version {:?}", translated, current); + + // reads: translated + onchain version. + // writes: translated + current.put + initial global commission. + T::DbWeight::get().reads_writes(translated + 1, translated + 2) + } else { + log!(info, "Migration did not execute. This probably should be removed"); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + // ensure all BondedPools items now contain an `inner.commission: Commission` field. + ensure!( + BondedPools::::iter().all(|(_, inner)| + // Check current + (inner.commission.current.is_none() || + inner.commission.current.is_some()) && + // Check max + (inner.commission.max.is_none() || inner.commission.max.is_some()) && + // Check change_rate + (inner.commission.change_rate.is_none() || + inner.commission.change_rate.is_some()) && + // Check throttle_from + (inner.commission.throttle_from.is_none() || + inner.commission.throttle_from.is_some())), + "a commission value has not been set correctly" + ); + ensure!( + GlobalMaxCommission::::get() == Some(U::get()), + "global maximum commission error" + ); + ensure!( + Pallet::::on_chain_storage_version() >= 4, + "nomination-pools::migration::v4: wrong storage version" + ); + Ok(()) + } + } +} + +pub mod v3 { + use super::*; + + /// This migration removes stale bonded-pool metadata, if any. + pub struct MigrateToV3(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV3 { + fn on_runtime_upgrade() -> Weight { + let current = Pallet::::in_code_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + if onchain == 2 { + log!( + info, + "Running migration with in-code storage version {:?} / onchain {:?}", + current, + onchain + ); + + let mut metadata_iterated = 0u64; + let mut metadata_removed = 0u64; + Metadata::::iter_keys() + .filter(|id| { + metadata_iterated += 1; + !BondedPools::::contains_key(&id) + }) + .collect::>() + .into_iter() + .for_each(|id| { + metadata_removed += 1; + Metadata::::remove(&id); + }); + StorageVersion::new(3).put::>(); + // metadata iterated + bonded pools read + a storage version read + let total_reads = metadata_iterated * 2 + 1; + // metadata removed + a storage version write + let total_writes = metadata_removed + 1; + T::DbWeight::get().reads_writes(total_reads, total_writes) + } else { + log!(info, "MigrateToV3 should be removed"); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + Ok(Vec::new()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + ensure!( + Metadata::::iter_keys().all(|id| BondedPools::::contains_key(&id)), + "not all of the stale metadata has been removed" + ); + ensure!( + Pallet::::on_chain_storage_version() >= 3, + "nomination-pools::migration::v3: wrong storage version" + ); + Ok(()) + } + } +} + +pub mod v2 { + use super::*; + use sp_runtime::Perbill; + + #[test] + fn migration_assumption_is_correct() { + // this migrations cleans all the reward accounts to contain exactly ed, and all members + // having no claimable rewards. In this state, all fields of the `RewardPool` and + // `member.last_recorded_reward_counter` are all zero. + use crate::mock::*; + ExtBuilder::default().build_and_execute(|| { + let join = |x| { + Currency::set_balance(&x, Balances::minimum_balance() + 10); + frame_support::assert_ok!(Pools::join(RuntimeOrigin::signed(x), 10, 1)); + }; + + assert_eq!(BondedPool::::get(1).unwrap().points, 10); + assert_eq!( + RewardPools::::get(1).unwrap(), + RewardPool { ..Default::default() } + ); + assert_eq!( + PoolMembers::::get(10).unwrap().last_recorded_reward_counter, + Zero::zero() + ); + + join(20); + assert_eq!(BondedPool::::get(1).unwrap().points, 20); + assert_eq!( + RewardPools::::get(1).unwrap(), + RewardPool { ..Default::default() } + ); + assert_eq!( + PoolMembers::::get(10).unwrap().last_recorded_reward_counter, + Zero::zero() + ); + assert_eq!( + PoolMembers::::get(20).unwrap().last_recorded_reward_counter, + Zero::zero() + ); + + join(30); + assert_eq!(BondedPool::::get(1).unwrap().points, 30); + assert_eq!( + RewardPools::::get(1).unwrap(), + RewardPool { ..Default::default() } + ); + assert_eq!( + PoolMembers::::get(10).unwrap().last_recorded_reward_counter, + Zero::zero() + ); + assert_eq!( + PoolMembers::::get(20).unwrap().last_recorded_reward_counter, + Zero::zero() + ); + assert_eq!( + PoolMembers::::get(30).unwrap().last_recorded_reward_counter, + Zero::zero() + ); + }); + } + + #[derive(Decode)] + pub struct OldRewardPool { + pub balance: B, + pub total_earnings: B, + pub points: U256, + } + + #[derive(Decode)] + pub struct OldPoolMember { + pub pool_id: PoolId, + pub points: BalanceOf, + pub reward_pool_total_earnings: BalanceOf, + pub unbonding_eras: BoundedBTreeMap, T::MaxUnbonding>, + } + + /// Migrate the pool reward scheme to the new version, as per + /// . + pub struct MigrateToV2(sp_std::marker::PhantomData); + impl MigrateToV2 { + fn run(current: StorageVersion) -> Weight { + let mut reward_pools_translated = 0u64; + let mut members_translated = 0u64; + // just for logging. + let mut total_value_locked = BalanceOf::::zero(); + let mut total_points_locked = BalanceOf::::zero(); + + // store each member of the pool, with their active points. In the process, migrate + // their data as well. + let mut temp_members = BTreeMap::)>>::new(); + PoolMembers::::translate::, _>(|key, old_member| { + let id = old_member.pool_id; + temp_members.entry(id).or_default().push((key, old_member.points)); + + total_points_locked += old_member.points; + members_translated += 1; + Some(PoolMember:: { + last_recorded_reward_counter: Zero::zero(), + pool_id: old_member.pool_id, + points: old_member.points, + unbonding_eras: old_member.unbonding_eras, + }) + }); + + // translate all reward pools. In the process, do the last payout as well. + RewardPools::::translate::>, _>( + |id, _old_reward_pool| { + // each pool should have at least one member. + let members = match temp_members.get(&id) { + Some(x) => x, + None => { + log!(error, "pool {} has no member! deleting it..", id); + return None + }, + }; + let bonded_pool = match BondedPools::::get(id) { + Some(x) => x, + None => { + log!(error, "pool {} has no bonded pool! deleting it..", id); + return None + }, + }; + + let accumulated_reward = RewardPool::::current_balance(id); + let reward_account = Pallet::::generate_reward_account(id); + let mut sum_paid_out = BalanceOf::::zero(); + + members + .into_iter() + .filter_map(|(who, points)| { + let bonded_pool = match BondedPool::::get(id) { + Some(x) => x, + None => { + log!(error, "pool {} for member {:?} does not exist!", id, who); + return None + }, + }; + + total_value_locked += bonded_pool.points_to_balance(*points); + let portion = Perbill::from_rational(*points, bonded_pool.points); + let last_claim = portion * accumulated_reward; + + log!( + debug, + "{:?} has {:?} ({:?}) of pool {} with total reward of {:?}", + who, + portion, + last_claim, + id, + accumulated_reward + ); + + if last_claim.is_zero() { + None + } else { + Some((who, last_claim)) + } + }) + .for_each(|(who, last_claim)| { + let outcome = T::Currency::transfer( + &reward_account, + &who, + last_claim, + Preservation::Preserve, + ); + + if let Err(reason) = outcome { + log!(warn, "last reward claim failed due to {:?}", reason,); + } else { + sum_paid_out = sum_paid_out.saturating_add(last_claim); + } + + Pallet::::deposit_event(Event::::PaidOut { + member: who.clone(), + pool_id: id, + payout: last_claim, + }); + }); + + // this can only be because of rounding down, or because the person we + // wanted to pay their reward to could not accept it (dust). + let leftover = accumulated_reward.saturating_sub(sum_paid_out); + if !leftover.is_zero() { + // pay it all to depositor. + let o = T::Currency::transfer( + &reward_account, + &bonded_pool.roles.depositor, + leftover, + Preservation::Preserve, + ); + log!(warn, "paying {:?} leftover to the depositor: {:?}", leftover, o); + } + + // finally, migrate the reward pool. + reward_pools_translated += 1; + + Some(RewardPool { + last_recorded_reward_counter: Zero::zero(), + last_recorded_total_payouts: Zero::zero(), + total_rewards_claimed: Zero::zero(), + total_commission_claimed: Zero::zero(), + total_commission_pending: Zero::zero(), + }) + }, + ); + + log!( + info, + "Upgraded {} members, {} reward pools, TVL {:?} TPL {:?}, storage to version {:?}", + members_translated, + reward_pools_translated, + total_value_locked, + total_points_locked, + current + ); + current.put::>(); + + T::DbWeight::get().reads_writes(members_translated + 1, reward_pools_translated + 1) + } + } + + impl OnRuntimeUpgrade for MigrateToV2 { + fn on_runtime_upgrade() -> Weight { + let current = Pallet::::in_code_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + log!( + info, + "Running migration with in-code storage version {:?} / onchain {:?}", + current, + onchain + ); + + if current == 2 && onchain == 1 { + Self::run(current) + } else { + log!(info, "MigrateToV2 did not executed. This probably should be removed"); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + // all reward accounts must have more than ED. + RewardPools::::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> { + ensure!( + >::balance(&Pallet::::generate_reward_account(id)) >= + T::Currency::minimum_balance(), + "Reward accounts must have greater balance than ED." + ); + Ok(()) + })?; + + Ok(Vec::new()) + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + // new version must be set. + ensure!( + Pallet::::on_chain_storage_version() == 2, + "The onchain version must be updated after the migration." + ); + + // no reward or bonded pool has been skipped. + ensure!( + RewardPools::::iter().count() as u32 == RewardPools::::count(), + "The count of reward pools must remain the same after the migration." + ); + ensure!( + BondedPools::::iter().count() as u32 == BondedPools::::count(), + "The count of reward pools must remain the same after the migration." + ); + + // all reward pools must have exactly ED in them. This means no reward can be claimed, + // and that setting reward counters all over the board to zero will work henceforth. + RewardPools::::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> { + ensure!( + RewardPool::::current_balance(id) == Zero::zero(), + "Reward pool balance must be zero.", + ); + Ok(()) + })?; + + log!(info, "post upgrade hook for MigrateToV2 executed."); + Ok(()) + } + } +} + +pub mod v1 { + use super::*; + + #[derive(Decode)] + pub struct OldPoolRoles { + pub depositor: AccountId, + pub root: AccountId, + pub nominator: AccountId, + pub bouncer: AccountId, + } + + impl OldPoolRoles { + fn migrate_to_v1(self) -> PoolRoles { + PoolRoles { + depositor: self.depositor, + root: Some(self.root), + nominator: Some(self.nominator), + bouncer: Some(self.bouncer), + } + } + } + + #[derive(Decode)] + pub struct OldBondedPoolInner { + pub points: BalanceOf, + pub state: PoolState, + pub member_counter: u32, + pub roles: OldPoolRoles, + } + + impl OldBondedPoolInner { + fn migrate_to_v1(self) -> BondedPoolInner { + // Note: `commission` field not introduced to `BondedPoolInner` until + // migration 4. + BondedPoolInner { + points: self.points, + commission: Commission::default(), + member_counter: self.member_counter, + state: self.state, + roles: self.roles.migrate_to_v1(), + } + } + } + + /// Trivial migration which makes the roles of each pool optional. + /// + /// Note: The depositor is not optional since they can never change. + pub struct MigrateToV1(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV1 { + fn on_runtime_upgrade() -> Weight { + let current = Pallet::::in_code_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + log!( + info, + "Running migration with in-code storage version {:?} / onchain {:?}", + current, + onchain + ); + + if current == 1 && onchain == 0 { + // this is safe to execute on any runtime that has a bounded number of pools. + let mut translated = 0u64; + BondedPools::::translate::, _>(|_key, old_value| { + translated.saturating_inc(); + Some(old_value.migrate_to_v1()) + }); + + current.put::>(); + + log!(info, "Upgraded {} pools, storage to version {:?}", translated, current); + + T::DbWeight::get().reads_writes(translated + 1, translated + 1) + } else { + log!(info, "Migration did not executed. This probably should be removed"); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { + // new version must be set. + ensure!( + Pallet::::on_chain_storage_version() == 1, + "The onchain version must be updated after the migration." + ); + Pallet::::try_state(frame_system::Pallet::::block_number())?; + Ok(()) + } + } +} + +mod helpers { + use super::*; + + pub(crate) fn calculate_tvl_by_total_stake() -> BalanceOf { + BondedPools::::iter_keys() + .map(|id| { + T::StakeAdapter::total_stake(Pool::from(Pallet::::generate_bonded_account(id))) + }) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default() + } +} From 3dee9c401e78e81d49103b0c89bacb07af0d488d Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 13 Jul 2024 16:50:32 +0530 Subject: [PATCH 29/57] remove unintended file --- .../frame/delegated-staking/src/migration.rs | 1163 ----------------- 1 file changed, 1163 deletions(-) delete mode 100644 substrate/frame/delegated-staking/src/migration.rs diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs deleted file mode 100644 index a9222ea53d75..000000000000 --- a/substrate/frame/delegated-staking/src/migration.rs +++ /dev/null @@ -1,1163 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -use super::*; -use crate::log; -use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade}; -use sp_std::{collections::btree_map::BTreeMap, vec::Vec}; - -#[cfg(feature = "try-runtime")] -use sp_runtime::TryRuntimeError; - -/// Exports for versioned migration `type`s for this pallet. -pub mod versioned { - use super::*; - - /// v8: Adds commission claim permissions to `BondedPools`. - pub type V7ToV8 = frame_support::migrations::VersionedMigration< - 7, - 8, - v8::VersionUncheckedMigrateV7ToV8, - crate::pallet::Pallet, - ::DbWeight, - >; - - /// Migration V6 to V7 wrapped in a [`frame_support::migrations::VersionedMigration`], ensuring - /// the migration is only performed when on-chain version is 6. - pub type V6ToV7 = frame_support::migrations::VersionedMigration< - 6, - 7, - v7::VersionUncheckedMigrateV6ToV7, - crate::pallet::Pallet, - ::DbWeight, - >; - - /// Wrapper over `MigrateToV6` with convenience version checks. - pub type V5toV6 = frame_support::migrations::VersionedMigration< - 5, - 6, - v6::MigrateToV6, - crate::pallet::Pallet, - ::DbWeight, - >; -} - -pub mod unversioned { - use super::*; - - /// Checks and updates `TotalValueLocked` if out of sync. - pub struct TotalValueLockedSync(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for TotalValueLockedSync { - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) - } - - fn on_runtime_upgrade() -> Weight { - let migrated = BondedPools::::count(); - - // recalculate the `TotalValueLocked` to compare with the current on-chain TVL which may - // be out of sync. - let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); - let onchain_tvl = TotalValueLocked::::get(); - - let writes = if tvl != onchain_tvl { - TotalValueLocked::::set(tvl); - - log!( - info, - "on-chain TVL was out of sync, update. Old: {:?}, new: {:?}", - onchain_tvl, - tvl - ); - - // writes: onchain version + set total value locked. - 2 - } else { - log!(info, "on-chain TVL was OK: {:?}", tvl); - - // writes: onchain version write. - 1 - }; - - // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain - // version - // - // writes: current version + (maybe) TVL - T::DbWeight::get() - .reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), writes) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { - Ok(()) - } - } - - /// Migrate existing pools from [`adapter::StakeStrategyType::Transfer`] to - /// [`adapter::StakeStrategyType::Delegate`]. - /// - /// Note: This only migrates the pools, the members are not migrated. They can use the - /// permission-less [`Pallet::migrate_delegation()`] to migrate their funds. - /// - /// This migration does not break any existing pool storage item, does not need to happen in any - /// sequence and hence can be applied unversioned on a production runtime. - /// - /// Takes `MaxPools` as type parameter to limit the number of pools that should be migrated in a - /// single block. It should be set such that migration weight does not exceed the block weight - /// limit. If all pools can be safely migrated, it is good to keep this number a little higher - /// than the actual number of pools to handle any extra pools created while the migration is - /// proposed, and before it is executed. - /// - /// If there are pools that fail to migrate or did not fit in the bounds, the remaining pools - /// can be migrated via the permission-less extrinsic [`Call::migrate_pool_to_delegate_stake`]. - pub struct DelegationStakeMigration(sp_std::marker::PhantomData<(T, MaxPools)>); - - impl> OnRuntimeUpgrade for DelegationStakeMigration { - fn on_runtime_upgrade() -> Weight { - let mut count: u32 = 0; - - BondedPools::::iter_keys().take(MaxPools::get() as usize).for_each(|id| { - let pool_acc = Pallet::::generate_bonded_account(id); - - // only migrate if the pool is in Transfer Strategy. - if T::StakeAdapter::pool_strategy(Pool::from(pool_acc)) == - adapter::StakeStrategyType::Transfer - { - let _ = Pallet::::migrate_to_delegate_stake(id).map_err(|err| { - log!( - warn, - "failed to migrate pool {:?} to delegate stake strategy with err: {:?}", - id, - err - ) - }); - count.saturating_inc(); - } - }); - - log!(info, "migrated {:?} pools to delegate stake strategy", count); - - // reads: (bonded pool key + current pool strategy) * MaxPools (worst case) - T::DbWeight::get() - .reads_writes(2, 0) - .saturating_mul(MaxPools::get() as u64) - // migration weight: `pool_migrate` weight * count - .saturating_add(T::WeightInfo::pool_migrate().saturating_mul(count.into())) - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - // ensure stake adapter is correct. - ensure!( - T::StakeAdapter::strategy_type() == adapter::StakeStrategyType::Delegate, - "Current strategy is not `Delegate" - ); - - if BondedPools::::count() > MaxPools::get() { - // we log a warning if the number of pools exceeds the bound. - log!( - warn, - "Number of pools {} exceeds the maximum bound {}. This would leave some pools unmigrated.", BondedPools::::count(), MaxPools::get() - ); - } - - let mut pool_balances: Vec> = Vec::new(); - BondedPools::::iter_keys().take(MaxPools::get() as usize).for_each(|id| { - let pool_account = Pallet::::generate_bonded_account(id); - - // we ensure migration is idempotent. - let pool_balance = T::StakeAdapter::total_balance(Pool::from(pool_account.clone())) - // we check actual account balance if pool has not migrated yet. - .unwrap_or(T::Currency::total_balance(&pool_account)); - - pool_balances.push(pool_balance); - }); - - Ok(pool_balances.encode()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { - let expected_pool_balances: Vec> = Decode::decode(&mut &data[..]).unwrap(); - - for (index, id) in - BondedPools::::iter_keys().take(MaxPools::get() as usize).enumerate() - { - let pool_account = Pallet::::generate_bonded_account(id); - if T::StakeAdapter::pool_strategy(Pool::from(pool_account.clone())) == - adapter::StakeStrategyType::Transfer - { - log!(error, "Pool {} failed to migrate", id,); - return Err(TryRuntimeError::Other("Pool failed to migrate")); - } - - let actual_balance = - T::StakeAdapter::total_balance(Pool::from(pool_account.clone())) - .expect("after migration, this should return a value"); - let expected_balance = expected_pool_balances.get(index).unwrap(); - - if actual_balance != *expected_balance { - log!( - error, - "Pool {} balance mismatch. Expected: {:?}, Actual: {:?}", - id, - expected_balance, - actual_balance - ); - return Err(TryRuntimeError::Other("Pool balance mismatch")); - } - - // account balance should be zero. - let pool_account_balance = T::Currency::total_balance(&pool_account); - if pool_account_balance != Zero::zero() { - log!( - error, - "Pool account balance was expected to be zero. Pool: {}, Balance: {:?}", - id, - pool_account_balance - ); - return Err(TryRuntimeError::Other("Pool account balance not migrated")); - } - } - - Ok(()) - } - } -} - -pub mod v8 { - use super::{v7::V7BondedPoolInner, *}; - - impl V7BondedPoolInner { - fn migrate_to_v8(self) -> BondedPoolInner { - BondedPoolInner { - commission: Commission { - current: self.commission.current, - max: self.commission.max, - change_rate: self.commission.change_rate, - throttle_from: self.commission.throttle_from, - // `claim_permission` is a new field. - claim_permission: None, - }, - member_counter: self.member_counter, - points: self.points, - roles: self.roles, - state: self.state, - } - } - } - - pub struct VersionUncheckedMigrateV7ToV8(sp_std::marker::PhantomData); - impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV7ToV8 { - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) - } - - fn on_runtime_upgrade() -> Weight { - let mut translated = 0u64; - BondedPools::::translate::, _>(|_key, old_value| { - translated.saturating_inc(); - Some(old_value.migrate_to_v8()) - }); - T::DbWeight::get().reads_writes(translated, translated + 1) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { - // Check new `claim_permission` field is present. - ensure!( - BondedPools::::iter() - .all(|(_, inner)| inner.commission.claim_permission.is_none()), - "`claim_permission` value has not been set correctly." - ); - Ok(()) - } - } -} - -/// This migration accumulates and initializes the [`TotalValueLocked`] for all pools. -/// -/// WARNING: This migration works under the assumption that the [`BondedPools`] cannot be inflated -/// arbitrarily. Otherwise this migration could fail due to too high weight. -pub(crate) mod v7 { - use super::*; - - #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, DebugNoBound, PartialEq, Clone)] - #[codec(mel_bound(T: Config))] - #[scale_info(skip_type_params(T))] - pub struct V7Commission { - pub current: Option<(Perbill, T::AccountId)>, - pub max: Option, - pub change_rate: Option>>, - pub throttle_from: Option>, - } - - #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, DebugNoBound, PartialEq, Clone)] - #[codec(mel_bound(T: Config))] - #[scale_info(skip_type_params(T))] - pub struct V7BondedPoolInner { - pub commission: V7Commission, - pub member_counter: u32, - pub points: BalanceOf, - pub roles: PoolRoles, - pub state: PoolState, - } - - #[allow(dead_code)] - #[derive(RuntimeDebugNoBound)] - #[cfg_attr(feature = "std", derive(Clone, PartialEq))] - pub struct V7BondedPool { - /// The identifier of the pool. - id: PoolId, - /// The inner fields. - inner: V7BondedPoolInner, - } - - impl V7BondedPool { - #[allow(dead_code)] - fn bonded_account(&self) -> T::AccountId { - Pallet::::generate_bonded_account(self.id) - } - } - - // NOTE: We cannot put a V7 prefix here since that would change the storage key. - #[frame_support::storage_alias] - pub type BondedPools = - CountedStorageMap, Twox64Concat, PoolId, V7BondedPoolInner>; - - pub struct VersionUncheckedMigrateV6ToV7(sp_std::marker::PhantomData); - impl UncheckedOnRuntimeUpgrade for VersionUncheckedMigrateV6ToV7 { - fn on_runtime_upgrade() -> Weight { - let migrated = BondedPools::::count(); - // The TVL should be the sum of all the funds that are actively staked and in the - // unbonding process of the account of each pool. - let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); - - TotalValueLocked::::set(tvl); - - log!(info, "Upgraded {} pools with a TVL of {:?}", migrated, tvl); - - // reads: migrated * (BondedPools + Staking::total_stake) + count + onchain - // version - // - // writes: current version + TVL - T::DbWeight::get().reads_writes(migrated.saturating_mul(2).saturating_add(2).into(), 2) - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { - // check that the `TotalValueLocked` written is actually the sum of `total_stake` of the - // `BondedPools`` - let tvl: BalanceOf = helpers::calculate_tvl_by_total_stake::(); - ensure!( - TotalValueLocked::::get() == tvl, - "TVL written is not equal to `Staking::total_stake` of all `BondedPools`." - ); - - // calculate the sum of `total_balance` of all `PoolMember` as the upper bound for the - // `TotalValueLocked`. - let total_balance_members: BalanceOf = PoolMembers::::iter() - .map(|(_, member)| member.total_balance()) - .reduce(|acc, total_balance| acc + total_balance) - .unwrap_or_default(); - - ensure!( - TotalValueLocked::::get() <= total_balance_members, - "TVL is greater than the balance of all PoolMembers." - ); - - ensure!( - Pallet::::on_chain_storage_version() >= 7, - "nomination-pools::migration::v7: wrong storage version" - ); - - Ok(()) - } - } -} - -mod v6 { - use super::*; - - /// This migration would restrict reward account of pools to go below ED by doing a named - /// freeze on all the existing pools. - pub struct MigrateToV6(sp_std::marker::PhantomData); - - impl MigrateToV6 { - fn freeze_ed(pool_id: PoolId) -> Result<(), ()> { - let reward_acc = Pallet::::generate_reward_account(pool_id); - Pallet::::freeze_pool_deposit(&reward_acc).map_err(|e| { - log!(error, "Failed to freeze ED for pool {} with error: {:?}", pool_id, e); - () - }) - } - } - impl UncheckedOnRuntimeUpgrade for MigrateToV6 { - fn on_runtime_upgrade() -> Weight { - let mut success = 0u64; - let mut fail = 0u64; - - BondedPools::::iter_keys().for_each(|p| { - if Self::freeze_ed(p).is_ok() { - success.saturating_inc(); - } else { - fail.saturating_inc(); - } - }); - - if fail > 0 { - log!(error, "Failed to freeze ED for {} pools", fail); - } else { - log!(info, "Freezing ED succeeded for {} pools", success); - } - - let total = success.saturating_add(fail); - // freeze_ed = r:2 w:2 - // reads: (freeze_ed + bonded pool key) * total - // writes: freeze_ed * total - T::DbWeight::get().reads_writes(3u64.saturating_mul(total), 2u64.saturating_mul(total)) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { - // there should be no ED imbalances anymore.. - Pallet::::check_ed_imbalance() - } - } -} -pub mod v5 { - use super::*; - - #[derive(Decode)] - pub struct OldRewardPool { - last_recorded_reward_counter: T::RewardCounter, - last_recorded_total_payouts: BalanceOf, - total_rewards_claimed: BalanceOf, - } - - impl OldRewardPool { - fn migrate_to_v5(self) -> RewardPool { - RewardPool { - last_recorded_reward_counter: self.last_recorded_reward_counter, - last_recorded_total_payouts: self.last_recorded_total_payouts, - total_rewards_claimed: self.total_rewards_claimed, - total_commission_pending: Zero::zero(), - total_commission_claimed: Zero::zero(), - } - } - } - - /// This migration adds `total_commission_pending` and `total_commission_claimed` field to every - /// `RewardPool`, if any. - pub struct MigrateToV5(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for MigrateToV5 { - fn on_runtime_upgrade() -> Weight { - let in_code = Pallet::::in_code_storage_version(); - let onchain = Pallet::::on_chain_storage_version(); - - log!( - info, - "Running migration with in-code storage version {:?} / onchain {:?}", - in_code, - onchain - ); - - if in_code == 5 && onchain == 4 { - let mut translated = 0u64; - RewardPools::::translate::, _>(|_id, old_value| { - translated.saturating_inc(); - Some(old_value.migrate_to_v5()) - }); - - in_code.put::>(); - log!(info, "Upgraded {} pools, storage to version {:?}", translated, in_code); - - // reads: translated + onchain version. - // writes: translated + current.put. - T::DbWeight::get().reads_writes(translated + 1, translated + 1) - } else { - log!(info, "Migration did not execute. This probably should be removed"); - T::DbWeight::get().reads(1) - } - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - let rpool_keys = RewardPools::::iter_keys().count(); - let rpool_values = RewardPools::::iter_values().count(); - if rpool_keys != rpool_values { - log!(info, "🔥 There are {} undecodable RewardPools in storage. This migration will try to correct them. keys: {}, values: {}", rpool_keys.saturating_sub(rpool_values), rpool_keys, rpool_values); - } - - ensure!( - PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), - "There are undecodable PoolMembers in storage. This migration will not fix that." - ); - ensure!( - BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), - "There are undecodable BondedPools in storage. This migration will not fix that." - ); - ensure!( - SubPoolsStorage::::iter_keys().count() == - SubPoolsStorage::::iter_values().count(), - "There are undecodable SubPools in storage. This migration will not fix that." - ); - ensure!( - Metadata::::iter_keys().count() == Metadata::::iter_values().count(), - "There are undecodable Metadata in storage. This migration will not fix that." - ); - - Ok((rpool_values as u64).encode()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { - let old_rpool_values: u64 = Decode::decode(&mut &data[..]).unwrap(); - let rpool_keys = RewardPools::::iter_keys().count() as u64; - let rpool_values = RewardPools::::iter_values().count() as u64; - ensure!( - rpool_keys == rpool_values, - "There are STILL undecodable RewardPools - migration failed" - ); - - if old_rpool_values != rpool_values { - log!( - info, - "🎉 Fixed {} undecodable RewardPools.", - rpool_values.saturating_sub(old_rpool_values) - ); - } - - // ensure all RewardPools items now contain `total_commission_pending` and - // `total_commission_claimed` field. - ensure!( - RewardPools::::iter().all(|(_, reward_pool)| reward_pool - .total_commission_pending >= - Zero::zero() && reward_pool - .total_commission_claimed >= - Zero::zero()), - "a commission value has been incorrectly set" - ); - ensure!( - Pallet::::on_chain_storage_version() >= 5, - "nomination-pools::migration::v5: wrong storage version" - ); - - // These should not have been touched - just in case. - ensure!( - PoolMembers::::iter_keys().count() == PoolMembers::::iter_values().count(), - "There are undecodable PoolMembers in storage." - ); - ensure!( - BondedPools::::iter_keys().count() == BondedPools::::iter_values().count(), - "There are undecodable BondedPools in storage." - ); - ensure!( - SubPoolsStorage::::iter_keys().count() == - SubPoolsStorage::::iter_values().count(), - "There are undecodable SubPools in storage." - ); - ensure!( - Metadata::::iter_keys().count() == Metadata::::iter_values().count(), - "There are undecodable Metadata in storage." - ); - - Ok(()) - } - } -} - -pub mod v4 { - use super::*; - - #[derive(Decode)] - pub struct OldBondedPoolInner { - pub points: BalanceOf, - pub state: PoolState, - pub member_counter: u32, - pub roles: PoolRoles, - } - - impl OldBondedPoolInner { - fn migrate_to_v4(self) -> BondedPoolInner { - BondedPoolInner { - commission: Commission::default(), - member_counter: self.member_counter, - points: self.points, - state: self.state, - roles: self.roles, - } - } - } - - /// Migrates from `v3` directly to `v5` to avoid the broken `v4` migration. - #[allow(deprecated)] - pub type MigrateV3ToV5 = (v4::MigrateToV4, v5::MigrateToV5); - - /// # Warning - /// - /// To avoid mangled storage please use `MigrateV3ToV5` instead. - /// See: github.com/paritytech/substrate/pull/13715 - /// - /// This migration adds a `commission` field to every `BondedPoolInner`, if - /// any. - #[deprecated( - note = "To avoid mangled storage please use `MigrateV3ToV5` instead. See: github.com/paritytech/substrate/pull/13715" - )] - pub struct MigrateToV4(sp_std::marker::PhantomData<(T, U)>); - #[allow(deprecated)] - impl> OnRuntimeUpgrade for MigrateToV4 { - fn on_runtime_upgrade() -> Weight { - let current = Pallet::::in_code_storage_version(); - let onchain = Pallet::::on_chain_storage_version(); - - log!( - info, - "Running migration with in-code storage version {:?} / onchain {:?}", - current, - onchain - ); - - if onchain == 3 { - log!(warn, "Please run MigrateToV5 immediately after this migration. See github.com/paritytech/substrate/pull/13715"); - let initial_global_max_commission = U::get(); - GlobalMaxCommission::::set(Some(initial_global_max_commission)); - log!( - info, - "Set initial global max commission to {:?}.", - initial_global_max_commission - ); - - let mut translated = 0u64; - BondedPools::::translate::, _>(|_key, old_value| { - translated.saturating_inc(); - Some(old_value.migrate_to_v4()) - }); - - StorageVersion::new(4).put::>(); - log!(info, "Upgraded {} pools, storage to version {:?}", translated, current); - - // reads: translated + onchain version. - // writes: translated + current.put + initial global commission. - T::DbWeight::get().reads_writes(translated + 1, translated + 2) - } else { - log!(info, "Migration did not execute. This probably should be removed"); - T::DbWeight::get().reads(1) - } - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { - // ensure all BondedPools items now contain an `inner.commission: Commission` field. - ensure!( - BondedPools::::iter().all(|(_, inner)| - // Check current - (inner.commission.current.is_none() || - inner.commission.current.is_some()) && - // Check max - (inner.commission.max.is_none() || inner.commission.max.is_some()) && - // Check change_rate - (inner.commission.change_rate.is_none() || - inner.commission.change_rate.is_some()) && - // Check throttle_from - (inner.commission.throttle_from.is_none() || - inner.commission.throttle_from.is_some())), - "a commission value has not been set correctly" - ); - ensure!( - GlobalMaxCommission::::get() == Some(U::get()), - "global maximum commission error" - ); - ensure!( - Pallet::::on_chain_storage_version() >= 4, - "nomination-pools::migration::v4: wrong storage version" - ); - Ok(()) - } - } -} - -pub mod v3 { - use super::*; - - /// This migration removes stale bonded-pool metadata, if any. - pub struct MigrateToV3(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for MigrateToV3 { - fn on_runtime_upgrade() -> Weight { - let current = Pallet::::in_code_storage_version(); - let onchain = Pallet::::on_chain_storage_version(); - - if onchain == 2 { - log!( - info, - "Running migration with in-code storage version {:?} / onchain {:?}", - current, - onchain - ); - - let mut metadata_iterated = 0u64; - let mut metadata_removed = 0u64; - Metadata::::iter_keys() - .filter(|id| { - metadata_iterated += 1; - !BondedPools::::contains_key(&id) - }) - .collect::>() - .into_iter() - .for_each(|id| { - metadata_removed += 1; - Metadata::::remove(&id); - }); - StorageVersion::new(3).put::>(); - // metadata iterated + bonded pools read + a storage version read - let total_reads = metadata_iterated * 2 + 1; - // metadata removed + a storage version write - let total_writes = metadata_removed + 1; - T::DbWeight::get().reads_writes(total_reads, total_writes) - } else { - log!(info, "MigrateToV3 should be removed"); - T::DbWeight::get().reads(1) - } - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - Ok(Vec::new()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { - ensure!( - Metadata::::iter_keys().all(|id| BondedPools::::contains_key(&id)), - "not all of the stale metadata has been removed" - ); - ensure!( - Pallet::::on_chain_storage_version() >= 3, - "nomination-pools::migration::v3: wrong storage version" - ); - Ok(()) - } - } -} - -pub mod v2 { - use super::*; - use sp_runtime::Perbill; - - #[test] - fn migration_assumption_is_correct() { - // this migrations cleans all the reward accounts to contain exactly ed, and all members - // having no claimable rewards. In this state, all fields of the `RewardPool` and - // `member.last_recorded_reward_counter` are all zero. - use crate::mock::*; - ExtBuilder::default().build_and_execute(|| { - let join = |x| { - Currency::set_balance(&x, Balances::minimum_balance() + 10); - frame_support::assert_ok!(Pools::join(RuntimeOrigin::signed(x), 10, 1)); - }; - - assert_eq!(BondedPool::::get(1).unwrap().points, 10); - assert_eq!( - RewardPools::::get(1).unwrap(), - RewardPool { ..Default::default() } - ); - assert_eq!( - PoolMembers::::get(10).unwrap().last_recorded_reward_counter, - Zero::zero() - ); - - join(20); - assert_eq!(BondedPool::::get(1).unwrap().points, 20); - assert_eq!( - RewardPools::::get(1).unwrap(), - RewardPool { ..Default::default() } - ); - assert_eq!( - PoolMembers::::get(10).unwrap().last_recorded_reward_counter, - Zero::zero() - ); - assert_eq!( - PoolMembers::::get(20).unwrap().last_recorded_reward_counter, - Zero::zero() - ); - - join(30); - assert_eq!(BondedPool::::get(1).unwrap().points, 30); - assert_eq!( - RewardPools::::get(1).unwrap(), - RewardPool { ..Default::default() } - ); - assert_eq!( - PoolMembers::::get(10).unwrap().last_recorded_reward_counter, - Zero::zero() - ); - assert_eq!( - PoolMembers::::get(20).unwrap().last_recorded_reward_counter, - Zero::zero() - ); - assert_eq!( - PoolMembers::::get(30).unwrap().last_recorded_reward_counter, - Zero::zero() - ); - }); - } - - #[derive(Decode)] - pub struct OldRewardPool { - pub balance: B, - pub total_earnings: B, - pub points: U256, - } - - #[derive(Decode)] - pub struct OldPoolMember { - pub pool_id: PoolId, - pub points: BalanceOf, - pub reward_pool_total_earnings: BalanceOf, - pub unbonding_eras: BoundedBTreeMap, T::MaxUnbonding>, - } - - /// Migrate the pool reward scheme to the new version, as per - /// . - pub struct MigrateToV2(sp_std::marker::PhantomData); - impl MigrateToV2 { - fn run(current: StorageVersion) -> Weight { - let mut reward_pools_translated = 0u64; - let mut members_translated = 0u64; - // just for logging. - let mut total_value_locked = BalanceOf::::zero(); - let mut total_points_locked = BalanceOf::::zero(); - - // store each member of the pool, with their active points. In the process, migrate - // their data as well. - let mut temp_members = BTreeMap::)>>::new(); - PoolMembers::::translate::, _>(|key, old_member| { - let id = old_member.pool_id; - temp_members.entry(id).or_default().push((key, old_member.points)); - - total_points_locked += old_member.points; - members_translated += 1; - Some(PoolMember:: { - last_recorded_reward_counter: Zero::zero(), - pool_id: old_member.pool_id, - points: old_member.points, - unbonding_eras: old_member.unbonding_eras, - }) - }); - - // translate all reward pools. In the process, do the last payout as well. - RewardPools::::translate::>, _>( - |id, _old_reward_pool| { - // each pool should have at least one member. - let members = match temp_members.get(&id) { - Some(x) => x, - None => { - log!(error, "pool {} has no member! deleting it..", id); - return None - }, - }; - let bonded_pool = match BondedPools::::get(id) { - Some(x) => x, - None => { - log!(error, "pool {} has no bonded pool! deleting it..", id); - return None - }, - }; - - let accumulated_reward = RewardPool::::current_balance(id); - let reward_account = Pallet::::generate_reward_account(id); - let mut sum_paid_out = BalanceOf::::zero(); - - members - .into_iter() - .filter_map(|(who, points)| { - let bonded_pool = match BondedPool::::get(id) { - Some(x) => x, - None => { - log!(error, "pool {} for member {:?} does not exist!", id, who); - return None - }, - }; - - total_value_locked += bonded_pool.points_to_balance(*points); - let portion = Perbill::from_rational(*points, bonded_pool.points); - let last_claim = portion * accumulated_reward; - - log!( - debug, - "{:?} has {:?} ({:?}) of pool {} with total reward of {:?}", - who, - portion, - last_claim, - id, - accumulated_reward - ); - - if last_claim.is_zero() { - None - } else { - Some((who, last_claim)) - } - }) - .for_each(|(who, last_claim)| { - let outcome = T::Currency::transfer( - &reward_account, - &who, - last_claim, - Preservation::Preserve, - ); - - if let Err(reason) = outcome { - log!(warn, "last reward claim failed due to {:?}", reason,); - } else { - sum_paid_out = sum_paid_out.saturating_add(last_claim); - } - - Pallet::::deposit_event(Event::::PaidOut { - member: who.clone(), - pool_id: id, - payout: last_claim, - }); - }); - - // this can only be because of rounding down, or because the person we - // wanted to pay their reward to could not accept it (dust). - let leftover = accumulated_reward.saturating_sub(sum_paid_out); - if !leftover.is_zero() { - // pay it all to depositor. - let o = T::Currency::transfer( - &reward_account, - &bonded_pool.roles.depositor, - leftover, - Preservation::Preserve, - ); - log!(warn, "paying {:?} leftover to the depositor: {:?}", leftover, o); - } - - // finally, migrate the reward pool. - reward_pools_translated += 1; - - Some(RewardPool { - last_recorded_reward_counter: Zero::zero(), - last_recorded_total_payouts: Zero::zero(), - total_rewards_claimed: Zero::zero(), - total_commission_claimed: Zero::zero(), - total_commission_pending: Zero::zero(), - }) - }, - ); - - log!( - info, - "Upgraded {} members, {} reward pools, TVL {:?} TPL {:?}, storage to version {:?}", - members_translated, - reward_pools_translated, - total_value_locked, - total_points_locked, - current - ); - current.put::>(); - - T::DbWeight::get().reads_writes(members_translated + 1, reward_pools_translated + 1) - } - } - - impl OnRuntimeUpgrade for MigrateToV2 { - fn on_runtime_upgrade() -> Weight { - let current = Pallet::::in_code_storage_version(); - let onchain = Pallet::::on_chain_storage_version(); - - log!( - info, - "Running migration with in-code storage version {:?} / onchain {:?}", - current, - onchain - ); - - if current == 2 && onchain == 1 { - Self::run(current) - } else { - log!(info, "MigrateToV2 did not executed. This probably should be removed"); - T::DbWeight::get().reads(1) - } - } - - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - // all reward accounts must have more than ED. - RewardPools::::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> { - ensure!( - >::balance(&Pallet::::generate_reward_account(id)) >= - T::Currency::minimum_balance(), - "Reward accounts must have greater balance than ED." - ); - Ok(()) - })?; - - Ok(Vec::new()) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { - // new version must be set. - ensure!( - Pallet::::on_chain_storage_version() == 2, - "The onchain version must be updated after the migration." - ); - - // no reward or bonded pool has been skipped. - ensure!( - RewardPools::::iter().count() as u32 == RewardPools::::count(), - "The count of reward pools must remain the same after the migration." - ); - ensure!( - BondedPools::::iter().count() as u32 == BondedPools::::count(), - "The count of reward pools must remain the same after the migration." - ); - - // all reward pools must have exactly ED in them. This means no reward can be claimed, - // and that setting reward counters all over the board to zero will work henceforth. - RewardPools::::iter().try_for_each(|(id, _)| -> Result<(), TryRuntimeError> { - ensure!( - RewardPool::::current_balance(id) == Zero::zero(), - "Reward pool balance must be zero.", - ); - Ok(()) - })?; - - log!(info, "post upgrade hook for MigrateToV2 executed."); - Ok(()) - } - } -} - -pub mod v1 { - use super::*; - - #[derive(Decode)] - pub struct OldPoolRoles { - pub depositor: AccountId, - pub root: AccountId, - pub nominator: AccountId, - pub bouncer: AccountId, - } - - impl OldPoolRoles { - fn migrate_to_v1(self) -> PoolRoles { - PoolRoles { - depositor: self.depositor, - root: Some(self.root), - nominator: Some(self.nominator), - bouncer: Some(self.bouncer), - } - } - } - - #[derive(Decode)] - pub struct OldBondedPoolInner { - pub points: BalanceOf, - pub state: PoolState, - pub member_counter: u32, - pub roles: OldPoolRoles, - } - - impl OldBondedPoolInner { - fn migrate_to_v1(self) -> BondedPoolInner { - // Note: `commission` field not introduced to `BondedPoolInner` until - // migration 4. - BondedPoolInner { - points: self.points, - commission: Commission::default(), - member_counter: self.member_counter, - state: self.state, - roles: self.roles.migrate_to_v1(), - } - } - } - - /// Trivial migration which makes the roles of each pool optional. - /// - /// Note: The depositor is not optional since they can never change. - pub struct MigrateToV1(sp_std::marker::PhantomData); - impl OnRuntimeUpgrade for MigrateToV1 { - fn on_runtime_upgrade() -> Weight { - let current = Pallet::::in_code_storage_version(); - let onchain = Pallet::::on_chain_storage_version(); - - log!( - info, - "Running migration with in-code storage version {:?} / onchain {:?}", - current, - onchain - ); - - if current == 1 && onchain == 0 { - // this is safe to execute on any runtime that has a bounded number of pools. - let mut translated = 0u64; - BondedPools::::translate::, _>(|_key, old_value| { - translated.saturating_inc(); - Some(old_value.migrate_to_v1()) - }); - - current.put::>(); - - log!(info, "Upgraded {} pools, storage to version {:?}", translated, current); - - T::DbWeight::get().reads_writes(translated + 1, translated + 1) - } else { - log!(info, "Migration did not executed. This probably should be removed"); - T::DbWeight::get().reads(1) - } - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_: Vec) -> Result<(), TryRuntimeError> { - // new version must be set. - ensure!( - Pallet::::on_chain_storage_version() == 1, - "The onchain version must be updated after the migration." - ); - Pallet::::try_state(frame_system::Pallet::::block_number())?; - Ok(()) - } - } -} - -mod helpers { - use super::*; - - pub(crate) fn calculate_tvl_by_total_stake() -> BalanceOf { - BondedPools::::iter_keys() - .map(|id| { - T::StakeAdapter::total_stake(Pool::from(Pallet::::generate_bonded_account(id))) - }) - .reduce(|acc, total_balance| acc + total_balance) - .unwrap_or_default() - } -} From a52f8193b1c2cfe9c87bc62a68f091ca96456eac Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Jul 2024 08:16:30 +0530 Subject: [PATCH 30/57] fix derivation of pool account --- substrate/frame/delegated-staking/Cargo.toml | 1 + substrate/frame/delegated-staking/src/lib.rs | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/substrate/frame/delegated-staking/Cargo.toml b/substrate/frame/delegated-staking/Cargo.toml index 0c1bcf0df0c5..3d83ed9a3813 100644 --- a/substrate/frame/delegated-staking/Cargo.toml +++ b/substrate/frame/delegated-staking/Cargo.toml @@ -19,6 +19,7 @@ scale-info = { features = ["derive"], workspace = true } sp-std = { workspace = true } sp-runtime = { workspace = true } sp-staking = { workspace = true } +sp-io = { workspace = true } [dev-dependencies] sp-core = { workspace = true, default-features = true } diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index a109f9e166f7..0f9a8a69f258 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -150,11 +150,12 @@ use frame_support::{ }, }; use sp_runtime::{ - traits::{AccountIdConversion, CheckedAdd, CheckedSub, Zero}, + traits::{AccountIdConversion, CheckedAdd, CheckedSub, Zero, TrailingZeroInput}, ArithmeticError, DispatchResult, Perbill, RuntimeDebug, Saturating, }; use sp_staking::{Agent, Delegator, EraIndex, StakingInterface, StakingUnchecked}; use sp_std::{convert::TryInto, prelude::*}; +use sp_io::hashing::blake2_256; pub type BalanceOf = <::Currency as FunInspect<::AccountId>>::Balance; @@ -437,7 +438,9 @@ impl Pallet { /// Derive a (keyless) pot account from the given agent account and account type. fn sub_account(account_type: AccountType, acc: T::AccountId) -> T::AccountId { - T::PalletId::get().into_sub_account_truncating((account_type, acc.clone())) + let entropy = (T::PalletId::get(), acc, account_type).using_encoded(blake2_256); + Decode::decode(&mut TrailingZeroInput::new(entropy.as_ref())) + .expect("infinite length input; no invalid inputs for type; qed") } /// Held balance of a delegator. From 41c01e648d47e6e0deef0128193d2531c9e8914e Mon Sep 17 00:00:00 2001 From: Ankan Date: Thu, 11 Jul 2024 09:02:01 +0530 Subject: [PATCH 31/57] fix import --- substrate/frame/delegated-staking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 0f9a8a69f258..45f92c6a0c03 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -150,7 +150,7 @@ use frame_support::{ }, }; use sp_runtime::{ - traits::{AccountIdConversion, CheckedAdd, CheckedSub, Zero, TrailingZeroInput}, + traits::{CheckedAdd, CheckedSub, Zero, TrailingZeroInput}, ArithmeticError, DispatchResult, Perbill, RuntimeDebug, Saturating, }; use sp_staking::{Agent, Delegator, EraIndex, StakingInterface, StakingUnchecked}; From 85498be8b01a7aebb60daa22cf0d92e52a4937b0 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 19 Jul 2024 02:44:43 +0530 Subject: [PATCH 32/57] migration code --- Cargo.lock | 1 + polkadot/runtime/westend/src/lib.rs | 11 +- substrate/frame/delegated-staking/Cargo.toml | 2 + substrate/frame/delegated-staking/src/lib.rs | 17 ++- .../frame/delegated-staking/src/migration.rs | 107 ++++++++++++++++++ 5 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 substrate/frame/delegated-staking/src/migration.rs diff --git a/Cargo.lock b/Cargo.lock index 4a7d47eb0f78..0c918c7a7cfd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10416,6 +10416,7 @@ dependencies = [ "frame-election-provider-support", "frame-support", "frame-system", + "log", "pallet-balances", "pallet-nomination-pools", "pallet-staking", diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 26f00060a57e..1591fed8742e 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1662,6 +1662,7 @@ parameter_types! { // a concern, it is recommended to set to (existing pools + 10) to also account for any new // pools getting created before the migration is actually executed. pub const MaxPoolsToMigrate: u32 = 250; + pub const MaxAgentsToMigrate: u32 = 300; } /// All migrations that will run on the next runtime upgrade. @@ -1697,11 +1698,15 @@ pub mod migrations { pub type Unreleased = ( // Migrate NominationPools to `DelegateStake` adapter. This is unversioned upgrade and // should not be applied yet in Kusama/Polkadot. - pallet_nomination_pools::migration::unversioned::DelegationStakeMigration< + // pallet_nomination_pools::migration::unversioned::DelegationStakeMigration< + // Runtime, + // MaxPoolsToMigrate, + // >, + // pallet_staking::migrations::v15::MigrateV14ToV15, + pallet_delegated_staking::migration::unversioned::ProxyDelegatorMigration< Runtime, - MaxPoolsToMigrate, + MaxAgentsToMigrate, >, - pallet_staking::migrations::v15::MigrateV14ToV15, ); } diff --git a/substrate/frame/delegated-staking/Cargo.toml b/substrate/frame/delegated-staking/Cargo.toml index 3d83ed9a3813..d1272c05c849 100644 --- a/substrate/frame/delegated-staking/Cargo.toml +++ b/substrate/frame/delegated-staking/Cargo.toml @@ -20,6 +20,7 @@ sp-std = { workspace = true } sp-runtime = { workspace = true } sp-staking = { workspace = true } sp-io = { workspace = true } +log = { workspace = true } [dev-dependencies] sp-core = { workspace = true, default-features = true } @@ -40,6 +41,7 @@ std = [ "frame-election-provider-support/std", "frame-support/std", "frame-system/std", + "log/std", "pallet-balances/std", "pallet-nomination-pools/std", "pallet-staking/std", diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 45f92c6a0c03..7152acb7319f 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -126,6 +126,7 @@ #![deny(rustdoc::broken_intra_doc_links)] mod impls; +pub mod migration; #[cfg(test)] mod mock; #[cfg(test)] @@ -149,14 +150,26 @@ use frame_support::{ Defensive, DefensiveOption, Imbalance, OnUnbalanced, }, }; +use sp_io::hashing::blake2_256; use sp_runtime::{ - traits::{CheckedAdd, CheckedSub, Zero, TrailingZeroInput}, + traits::{CheckedAdd, CheckedSub, TrailingZeroInput, Zero}, ArithmeticError, DispatchResult, Perbill, RuntimeDebug, Saturating, }; use sp_staking::{Agent, Delegator, EraIndex, StakingInterface, StakingUnchecked}; use sp_std::{convert::TryInto, prelude::*}; -use sp_io::hashing::blake2_256; +/// The log target of this pallet. +pub const LOG_TARGET: &str = "runtime::delegated-staking"; +// syntactic sugar for logging. +#[macro_export] +macro_rules! log { + ($level:tt, $patter:expr $(, $values:expr)* $(,)?) => { + log::$level!( + target: $crate::LOG_TARGET, + concat!("[{:?}] 🏊‍♂️ ", $patter), >::block_number() $(, $values)* + ) + }; +} pub type BalanceOf = <::Currency as FunInspect<::AccountId>>::Balance; diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs new file mode 100644 index 000000000000..9358628bb51c --- /dev/null +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -0,0 +1,107 @@ +// This file is part of Substrate. + +// Copyright (C) Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use super::*; +use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade}; +use sp_std::vec::Vec; + +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; + +pub mod unversioned { + use super::*; + use frame_support::traits::tokens::Fortitude::Force; + use sp_runtime::traits::AccountIdConversion; + + /// Migrates delegation from older derivation of [`AccountType::ProxyDelegator`] accounts + /// to the new one for all agents. + pub struct ProxyDelegatorMigration(sp_std::marker::PhantomData<(T, MaxAgents)>); + + impl> OnRuntimeUpgrade for ProxyDelegatorMigration { + fn on_runtime_upgrade() -> Weight { + let mut weight = Weight::zero(); + let old_proxy_delegator = |agent: T::AccountId| { + T::PalletId::get() + .into_sub_account_truncating((AccountType::ProxyDelegator, agent.clone())) + }; + + Agents::::iter_keys().take(MaxAgents::get() as usize).for_each(|agent| { + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0)); + + let old_proxy = old_proxy_delegator(agent.clone()); + Delegation::::get(&old_proxy).map(|delegation| { + weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0)); + + let new_proxy = + Pallet::::generate_proxy_delegator(Agent::from(agent.clone())); + + // accrue read writes for `do_migrate_delegation` + weight.saturating_accrue(T::DbWeight::get().reads_writes(8, 8)); + let _ = Pallet::::do_migrate_delegation( + Delegator::from(old_proxy.clone()), + Delegator::from(new_proxy.clone()), + delegation.amount, + ) + .map_err(|e| { + log!( + error, + "Failed to migrate old proxy delegator {:?} to new proxy {:?} for agent {:?} with error: {:?}", + old_proxy, + new_proxy, + agent, + e, + ); + }); + }); + }); + + log!(info, "Finished migrating old proxy delegator accounts to new ones",); + + weight + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { + let mut unmigrated_count = 0; + let old_proxy_delegator = |agent: T::AccountId| { + T::PalletId::get() + .into_sub_account_truncating((AccountType::ProxyDelegator, agent.clone())) + }; + + Agents::::iter_keys().take(MaxAgents::get() as usize).for_each(|agent| { + let old_proxy: T::AccountId = old_proxy_delegator(agent.clone()); + let held_balance = Pallet::::held_balance_of(Delegator::from(old_proxy.clone())); + let delegation = Delegation::::get(&old_proxy); + if delegation.is_some() || !held_balance.is_zero() { + log!( + error, + "Old proxy delegator {:?} for agent {:?} is not migrated.", + old_proxy, + agent, + ); + unmigrated_count = unmigrated_count + 1; + } + }); + + if unmigrated_count > 0 { + Err(TryRuntimeError::Other("Some old proxy delegator accounts are not migrated.")) + } else { + Ok(()) + } + } + } +} From 172b14d634208435e916e202ab027cf5c047f1cc Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 19 Jul 2024 02:54:28 +0530 Subject: [PATCH 33/57] remove released migrations --- polkadot/runtime/westend/src/lib.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 1591fed8742e..fa1081b1781b 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1657,12 +1657,8 @@ pub type SignedExtra = ( ); parameter_types! { - // This is the max pools that will be migrated in the runtime upgrade. Westend has more pools - // than this, but we want to emulate some non migrated pools. In prod runtimes, if weight is not - // a concern, it is recommended to set to (existing pools + 10) to also account for any new - // pools getting created before the migration is actually executed. - pub const MaxPoolsToMigrate: u32 = 250; - pub const MaxAgentsToMigrate: u32 = 300; + /// Bounding number of agent pot accounts to be migrated in a single block. + pub const MaxAgentsToMigrate: u32 = 200; } /// All migrations that will run on the next runtime upgrade. @@ -1696,13 +1692,7 @@ pub mod migrations { /// Unreleased migrations. Add new ones here: pub type Unreleased = ( - // Migrate NominationPools to `DelegateStake` adapter. This is unversioned upgrade and - // should not be applied yet in Kusama/Polkadot. - // pallet_nomination_pools::migration::unversioned::DelegationStakeMigration< - // Runtime, - // MaxPoolsToMigrate, - // >, - // pallet_staking::migrations::v15::MigrateV14ToV15, + // This is only needed for Westend. pallet_delegated_staking::migration::unversioned::ProxyDelegatorMigration< Runtime, MaxAgentsToMigrate, From 28fb07097db661a2c1a8697c1b26d8303244cebc Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 19 Jul 2024 03:00:03 +0530 Subject: [PATCH 34/57] fix clippy --- substrate/frame/delegated-staking/src/migration.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index 9358628bb51c..c317903c08db 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -16,15 +16,13 @@ // limitations under the License. use super::*; -use frame_support::traits::{OnRuntimeUpgrade, UncheckedOnRuntimeUpgrade}; -use sp_std::vec::Vec; +use frame_support::traits::OnRuntimeUpgrade; #[cfg(feature = "try-runtime")] use sp_runtime::TryRuntimeError; pub mod unversioned { use super::*; - use frame_support::traits::tokens::Fortitude::Force; use sp_runtime::traits::AccountIdConversion; /// Migrates delegation from older derivation of [`AccountType::ProxyDelegator`] accounts @@ -41,9 +39,10 @@ pub mod unversioned { Agents::::iter_keys().take(MaxAgents::get() as usize).for_each(|agent| { weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0)); - let old_proxy = old_proxy_delegator(agent.clone()); - Delegation::::get(&old_proxy).map(|delegation| { + + // if delegation does not exist, it does not need to be migrated. + if let Some(delegation) = Delegation::::get(&old_proxy) { weight.saturating_accrue(T::DbWeight::get().reads_writes(1, 0)); let new_proxy = @@ -66,7 +65,7 @@ pub mod unversioned { e, ); }); - }); + }; }); log!(info, "Finished migrating old proxy delegator accounts to new ones",); From 89c196472bfc2afac57f98238178ba70fc117e30 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 19 Jul 2024 03:30:32 +0530 Subject: [PATCH 35/57] fix agent count --- polkadot/runtime/westend/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index fa1081b1781b..93bca0d21fcd 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1658,7 +1658,7 @@ pub type SignedExtra = ( parameter_types! { /// Bounding number of agent pot accounts to be migrated in a single block. - pub const MaxAgentsToMigrate: u32 = 200; + pub const MaxAgentsToMigrate: u32 = 300; } /// All migrations that will run on the next runtime upgrade. From 66b54583db191b6627e75da5c3f1c6b2b03ffd99 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 6 Aug 2024 10:04:43 +0200 Subject: [PATCH 36/57] remove unnecessary conversion --- substrate/frame/delegated-staking/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index c317903c08db..0bedae0839ed 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -52,7 +52,7 @@ pub mod unversioned { weight.saturating_accrue(T::DbWeight::get().reads_writes(8, 8)); let _ = Pallet::::do_migrate_delegation( Delegator::from(old_proxy.clone()), - Delegator::from(new_proxy.clone()), + new_proxy.clone(), delegation.amount, ) .map_err(|e| { From 9ac94ee43f64bdfdb5fb7067ea268538fdcb398f Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 7 Aug 2024 13:19:55 +0200 Subject: [PATCH 37/57] fix import --- substrate/frame/delegated-staking/src/migration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index 0bedae0839ed..d73b53ec72f7 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -27,7 +27,7 @@ pub mod unversioned { /// Migrates delegation from older derivation of [`AccountType::ProxyDelegator`] accounts /// to the new one for all agents. - pub struct ProxyDelegatorMigration(sp_std::marker::PhantomData<(T, MaxAgents)>); + pub struct ProxyDelegatorMigration(PhantomData<(T, MaxAgents)>); impl> OnRuntimeUpgrade for ProxyDelegatorMigration { fn on_runtime_upgrade() -> Weight { From 0bef5a90def33341edd9fa289cf53bd2b3fba28a Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 7 Aug 2024 13:37:23 +0200 Subject: [PATCH 38/57] minor fixes --- substrate/frame/delegated-staking/src/migration.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index d73b53ec72f7..c29b9d8267ff 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -68,13 +68,12 @@ pub mod unversioned { }; }); - log!(info, "Finished migrating old proxy delegator accounts to new ones",); - + log!(info, "Finished migrating old proxy delegator accounts to new ones"); weight } #[cfg(feature = "try-runtime")] - fn post_upgrade(data: Vec) -> Result<(), TryRuntimeError> { + fn post_upgrade(_data: Vec) -> Result<(), TryRuntimeError> { let mut unmigrated_count = 0; let old_proxy_delegator = |agent: T::AccountId| { T::PalletId::get() @@ -92,7 +91,7 @@ pub mod unversioned { old_proxy, agent, ); - unmigrated_count = unmigrated_count + 1; + unmigrated_count += 1; } }); From 0c57ab747d0ef86a0f212087be6b3d80301ab1aa Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 7 Aug 2024 13:57:43 +0200 Subject: [PATCH 39/57] missing import --- substrate/frame/delegated-staking/src/migration.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index c29b9d8267ff..110be2c006c4 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -24,6 +24,8 @@ use sp_runtime::TryRuntimeError; pub mod unversioned { use super::*; use sp_runtime::traits::AccountIdConversion; + #[cfg(feature = "try-runtime")] + use alloc::vec::Vec; /// Migrates delegation from older derivation of [`AccountType::ProxyDelegator`] accounts /// to the new one for all agents. From 0115e7cffbe5178f00a5ae06b28082c2beebdef0 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 7 Aug 2024 18:13:35 +0200 Subject: [PATCH 40/57] rustdoc fix --- substrate/frame/delegated-staking/src/migration.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index 110be2c006c4..5be2cafd6c82 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -27,8 +27,7 @@ pub mod unversioned { #[cfg(feature = "try-runtime")] use alloc::vec::Vec; - /// Migrates delegation from older derivation of [`AccountType::ProxyDelegator`] accounts - /// to the new one for all agents. + /// Migrates `ProxyDelegator` accounts with better entropy than the previous. pub struct ProxyDelegatorMigration(PhantomData<(T, MaxAgents)>); impl> OnRuntimeUpgrade for ProxyDelegatorMigration { From 2e486db3bbcc9061fa2c94f2ba6d68490ef959ce Mon Sep 17 00:00:00 2001 From: Ankan Date: Sat, 10 Aug 2024 12:45:03 +0200 Subject: [PATCH 41/57] remove condition for minimum balance when migrating delegation --- substrate/frame/delegated-staking/src/lib.rs | 3 --- substrate/frame/delegated-staking/src/migration.rs | 2 +- substrate/frame/nomination-pools/src/lib.rs | 8 ++++---- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 642f2ae19606..e0a10e522b77 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -707,9 +707,6 @@ impl Pallet { .checked_sub(&amount) .defensive_ok_or(Error::::BadState)?; - // ensure amount is greater than ED otherwise transfer would fail. - ensure!(amount >= T::Currency::minimum_balance(), Error::::NotEnoughFunds); - // transfer the held amount in `source_delegator` to `destination_delegator`. let _ = T::Currency::transfer_on_hold( &HoldReason::StakingDelegation.into(), diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index 5be2cafd6c82..62797e747aeb 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -23,9 +23,9 @@ use sp_runtime::TryRuntimeError; pub mod unversioned { use super::*; - use sp_runtime::traits::AccountIdConversion; #[cfg(feature = "try-runtime")] use alloc::vec::Vec; + use sp_runtime::traits::AccountIdConversion; /// Migrates `ProxyDelegator` accounts with better entropy than the previous. pub struct ProxyDelegatorMigration(PhantomData<(T, MaxAgents)>); diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 302bcf07bd6e..aefd790b4fed 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3956,10 +3956,10 @@ impl Pallet { PoolMembers::::get(who.clone()) .map(|pool_member| { - // if Self::api_pool_needs_delegate_migration(pool_member.pool_id) { - // // the pool needs to be migrated before members can be migrated. - // return false - // } + if Self::api_pool_needs_delegate_migration(pool_member.pool_id) { + // the pool needs to be migrated before members can be migrated. + return false + } let member_balance = pool_member.total_balance(); let delegated_balance = From acbb67480670130c1e8a5f5f877cbb66cffebcc8 Mon Sep 17 00:00:00 2001 From: Ankan Date: Mon, 12 Aug 2024 11:58:49 +0200 Subject: [PATCH 42/57] refactor based on pr comments --- substrate/frame/delegated-staking/src/lib.rs | 29 ++++++++++---------- substrate/frame/nomination-pools/src/lib.rs | 3 +- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 95d225d8b171..6f2b30df793e 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -551,24 +551,25 @@ impl Pallet { let mut ledger = AgentLedger::::get(&agent).ok_or(Error::::NotAgent)?; - let new_delegation_amount = - if let Some(existing_delegation) = Delegation::::get(&delegator) { - ensure!(existing_delegation.agent == agent, Error::::InvalidDelegation); - existing_delegation - .amount - .checked_add(&amount) - .ok_or(ArithmeticError::Overflow)? - } else { - // if this is the first time delegation, increment provider count. This ensures that - // amount including existential deposit can be held. - frame_system::Pallet::::inc_providers(&delegator); - amount - }; + if let Some(mut existing_delegation) = Delegation::::get(&delegator) { + ensure!(existing_delegation.agent == agent, Error::::InvalidDelegation); + // update amount + existing_delegation.amount = existing_delegation + .amount + .checked_add(&amount) + .ok_or(ArithmeticError::Overflow)?; + existing_delegation + } else { + // if this is the first time delegation, increment provider count. This ensures that + // amount including existential deposit can be held. + frame_system::Pallet::::inc_providers(&delegator); + Delegation::::new(&agent, amount) + } + .update(&delegator); // try to hold the funds. T::Currency::hold(&HoldReason::StakingDelegation.into(), &delegator, amount)?; - let _ = Delegation::::new(&agent, new_delegation_amount).update(&delegator); ledger.total_delegated = ledger.total_delegated.checked_add(&amount).ok_or(ArithmeticError::Overflow)?; ledger.update(&agent); diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 302bcf07bd6e..ce22a3bb93e6 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3974,7 +3974,8 @@ impl Pallet { /// Contribution of the member in the pool. /// - /// Includes balance that is unbonding currently. + /// Includes balance that is unbonded from staking but not claimed yet from the pool, therefore + /// this balance can be higher than the staked funds. pub fn api_member_total_balance(who: T::AccountId) -> BalanceOf { PoolMembers::::get(who.clone()) .map(|m| m.total_balance()) From 65cecb57370a2e323690138b2059c2601e8d03ec Mon Sep 17 00:00:00 2001 From: Ankan Date: Mon, 12 Aug 2024 12:12:10 +0200 Subject: [PATCH 43/57] allow migration lower than ED --- substrate/frame/delegated-staking/src/lib.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 6f2b30df793e..87a0913aa581 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -692,9 +692,6 @@ impl Pallet { .checked_sub(&amount) .defensive_ok_or(Error::::BadState)?; - // ensure amount is greater than ED otherwise transfer would fail. - ensure!(amount >= T::Currency::minimum_balance(), Error::::NotEnoughFunds); - // transfer the held amount in `source_delegator` to `destination_delegator`. let _ = T::Currency::transfer_on_hold( &HoldReason::StakingDelegation.into(), From 94b2e8b7e1eb9be19c4f17399ed0a21e6e23db2d Mon Sep 17 00:00:00 2001 From: Ankan Date: Mon, 12 Aug 2024 13:57:24 +0200 Subject: [PATCH 44/57] add prdoc --- prdoc/pr_4999.prdoc | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 prdoc/pr_4999.prdoc diff --git a/prdoc/pr_4999.prdoc b/prdoc/pr_4999.prdoc new file mode 100644 index 000000000000..d396fcdbe8b3 --- /dev/null +++ b/prdoc/pr_4999.prdoc @@ -0,0 +1,18 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Fixes entropy for derivation of proxy delegator account. + +doc: + - audience: Runtime Dev + description: | + This fixes how ProxyDelegator accounts are derived but may cause issues in Westend since it would use the old + derivative accounts. Does not affect Polkadot/Kusama as this pallet is not deployed to them yet. + +crates: + - name: westend-runtime + bump: patch + - name: pallet-delegated-staking + bump: patch + - name: pallet-nomination-pools + bump: patch \ No newline at end of file From 00ab010f247d2d1f3aa788d38ac5d1604e898710 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 13 Aug 2024 15:06:47 +0200 Subject: [PATCH 45/57] move remote tests to another file --- polkadot/runtime/westend/src/lib.rs | 147 -------------------- polkadot/runtime/westend/src/tests.rs | 147 ++++++++++++++++++++ substrate/frame/nomination-pools/src/lib.rs | 9 +- 3 files changed, 148 insertions(+), 155 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 00fe918393d5..72ca63cf50f1 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2777,153 +2777,6 @@ sp_api::impl_runtime_apis! { } } -#[cfg(all(test, feature = "try-runtime"))] -mod remote_tests { - use super::*; - use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect}; - use remote_externalities::{ - Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport, - }; - use std::env::var; - - #[tokio::test] - async fn run_migrations() { - if var("RUN_MIGRATION_TESTS").is_err() { - return; - } - - sp_tracing::try_init_simple(); - let transport: Transport = - var("WS").unwrap_or("wss://westend-rpc.polkadot.io:443".to_string()).into(); - let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); - let mut ext = Builder::::default() - .mode(if let Some(state_snapshot) = maybe_state_snapshot { - Mode::OfflineOrElseOnline( - OfflineConfig { state_snapshot: state_snapshot.clone() }, - OnlineConfig { - transport, - state_snapshot: Some(state_snapshot), - ..Default::default() - }, - ) - } else { - Mode::Online(OnlineConfig { transport, ..Default::default() }) - }) - .build() - .await - .unwrap(); - ext.execute_with(|| Runtime::on_runtime_upgrade(UpgradeCheckSelect::PreAndPost)); - } - - #[tokio::test] - async fn delegate_stake_migration() { - // Intended to be run only manually. - if var("RUN_MIGRATION_TESTS").is_err() { - return; - } - use frame_support::assert_ok; - sp_tracing::try_init_simple(); - - let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); - let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); - let mut ext = Builder::::default() - .mode(if let Some(state_snapshot) = maybe_state_snapshot { - Mode::OfflineOrElseOnline( - OfflineConfig { state_snapshot: state_snapshot.clone() }, - OnlineConfig { - transport, - state_snapshot: Some(state_snapshot), - pallets: vec![ - "staking".into(), - "system".into(), - "balances".into(), - "nomination-pools".into(), - "delegated-staking".into(), - ], - ..Default::default() - }, - ) - } else { - Mode::Online(OnlineConfig { transport, ..Default::default() }) - }) - .build() - .await - .unwrap(); - ext.execute_with(|| { - // create an account with some balance - let alice = AccountId::from([1u8; 32]); - use frame_support::traits::Currency; - let _ = Balances::deposit_creating(&alice, 100_000 * UNITS); - - // iterate over all pools - pallet_nomination_pools::BondedPools::::iter_keys().for_each(|k| { - if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration(k) - { - assert_ok!( - pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( - RuntimeOrigin::signed(alice.clone()).into(), - k, - ) - ); - } - }); - - // member migration stats - let mut success = 0; - let mut members_below_ed = 0; - let mut direct_stakers = 0; - let mut unexpected_errors = 0; - - // iterate over all pool members - pallet_nomination_pools::PoolMembers::::iter().for_each(|(k, member)| { - if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( - k.clone(), - ) { - // reasons migrations can fail: - let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); - let member_balance_below_ed = member.total_balance() < ExistentialDeposit::get(); - - let migration = pallet_nomination_pools::Pallet::::migrate_delegation( - RuntimeOrigin::signed(alice.clone()).into(), - sp_runtime::MultiAddress::Id(k.clone()), - ); - - if member_balance_below_ed { - // if the member has balance below ED, the migration should fail. - members_below_ed += 1; - assert_eq!( - migration.unwrap_err(), - pallet_nomination_pools::Error::::MinimumBondNotMet.into() - ); - } else if is_direct_staker { - // if the member is a direct staker, the migration should fail until pool - // member unstakes all funds from pallet-staking. - direct_stakers += 1; - assert_eq!( - migration.unwrap_err(), - pallet_delegated_staking::Error::::AlreadyStaking.into() - ); - } else if migration.is_err() { - unexpected_errors += 1; - log::error!(target: "remote_test", "Unexpected error {:?} while migrating {:?}", migration.unwrap_err(), k); - } else { - success += 1; - } - } - }); - - log::info!( - target: "remote_test", - "Migration stats: success: {}, members_below_ed: {}, direct_stakers: {}, unexpected_errors: {}", - success, - members_below_ed, - direct_stakers, - unexpected_errors - ); - }); - } -} - mod clean_state_migration { use super::Runtime; #[cfg(feature = "try-runtime")] diff --git a/polkadot/runtime/westend/src/tests.rs b/polkadot/runtime/westend/src/tests.rs index 4d5e2e946bce..a910a975a459 100644 --- a/polkadot/runtime/westend/src/tests.rs +++ b/polkadot/runtime/westend/src/tests.rs @@ -99,3 +99,150 @@ fn check_treasury_pallet_id() { westend_runtime_constants::TREASURY_PALLET_ID ); } + +#[cfg(all(test, feature = "try-runtime"))] +mod remote_tests { + use super::*; + use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect}; + use remote_externalities::{ + Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport, + }; + use std::env::var; + + #[tokio::test] + async fn run_migrations() { + if var("RUN_MIGRATION_TESTS").is_err() { + return; + } + + sp_tracing::try_init_simple(); + let transport: Transport = + var("WS").unwrap_or("wss://westend-rpc.polkadot.io:443".to_string()).into(); + let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); + let mut ext = Builder::::default() + .mode(if let Some(state_snapshot) = maybe_state_snapshot { + Mode::OfflineOrElseOnline( + OfflineConfig { state_snapshot: state_snapshot.clone() }, + OnlineConfig { + transport, + state_snapshot: Some(state_snapshot), + ..Default::default() + }, + ) + } else { + Mode::Online(OnlineConfig { transport, ..Default::default() }) + }) + .build() + .await + .unwrap(); + ext.execute_with(|| Runtime::on_runtime_upgrade(UpgradeCheckSelect::PreAndPost)); + } + + #[tokio::test] + async fn delegate_stake_migration() { + // Intended to be run only manually. + if var("RUN_MIGRATION_TESTS").is_err() { + return; + } + use frame_support::assert_ok; + sp_tracing::try_init_simple(); + + let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); + let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); + let mut ext = Builder::::default() + .mode(if let Some(state_snapshot) = maybe_state_snapshot { + Mode::OfflineOrElseOnline( + OfflineConfig { state_snapshot: state_snapshot.clone() }, + OnlineConfig { + transport, + state_snapshot: Some(state_snapshot), + pallets: vec![ + "staking".into(), + "system".into(), + "balances".into(), + "nomination-pools".into(), + "delegated-staking".into(), + ], + ..Default::default() + }, + ) + } else { + Mode::Online(OnlineConfig { transport, ..Default::default() }) + }) + .build() + .await + .unwrap(); + ext.execute_with(|| { + // create an account with some balance + let alice = AccountId::from([1u8; 32]); + use frame_support::traits::Currency; + let _ = Balances::deposit_creating(&alice, 100_000 * UNITS); + + // iterate over all pools + pallet_nomination_pools::BondedPools::::iter_keys().for_each(|k| { + if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration(k) + { + assert_ok!( + pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( + RuntimeOrigin::signed(alice.clone()).into(), + k, + ) + ); + } + }); + + // member migration stats + let mut success = 0; + let mut members_below_ed = 0; + let mut direct_stakers = 0; + let mut unexpected_errors = 0; + + // iterate over all pool members + pallet_nomination_pools::PoolMembers::::iter().for_each(|(k, member)| { + if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( + k.clone(), + ) { + // reasons migrations can fail: + let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); + let member_balance_below_ed = member.total_balance() < ExistentialDeposit::get(); + + let migration = pallet_nomination_pools::Pallet::::migrate_delegation( + RuntimeOrigin::signed(alice.clone()).into(), + sp_runtime::MultiAddress::Id(k.clone()), + ); + + if member_balance_below_ed { + // if the member has balance below ED, the migration should fail. + members_below_ed += 1; + assert_eq!( + migration.unwrap_err(), + pallet_nomination_pools::Error::::MinimumBondNotMet.into() + ); + } else if is_direct_staker { + // if the member is a direct staker, the migration should fail until pool + // member unstakes all funds from pallet-staking. + direct_stakers += 1; + assert_eq!( + migration.unwrap_err(), + pallet_delegated_staking::Error::::AlreadyStaking.into() + ); + } else if migration.is_err() { + unexpected_errors += 1; + log::error!(target: "remote_test", "Unexpected error {:?} while migrating {:?}", migration.unwrap_err(), k); + } else { + success += 1; + } + } + }); + + log::info!( + target: "remote_test", + "Migration stats: success: {}, members_below_ed: {}, direct_stakers: {}, unexpected_errors: {}", + success, + members_below_ed, + direct_stakers, + unexpected_errors + ); + }); + } +} diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 5dc89697cfba..40f504e394cb 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2976,19 +2976,12 @@ pub mod pallet { Error::::NotMigrated ); - let pool_contribution = member.total_balance(); - // ensure the pool contribution is greater than the existential deposit otherwise we - // cannot transfer funds to member account. - ensure!( - pool_contribution >= T::Currency::minimum_balance(), - Error::::MinimumBondNotMet - ); - let delegation = T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); // delegation should not exist. ensure!(delegation.is_none(), Error::::AlreadyMigrated); + let pool_contribution = member.total_balance(); T::StakeAdapter::migrate_delegation( Pool::from(Pallet::::generate_bonded_account(member.pool_id)), Member::from(member_account), From 7326b108c1130dd2dfeca25cc8241d82cca07229 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 13 Aug 2024 16:28:45 +0200 Subject: [PATCH 46/57] fix below ed test --- polkadot/runtime/westend/src/tests.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/polkadot/runtime/westend/src/tests.rs b/polkadot/runtime/westend/src/tests.rs index a910a975a459..fa9ea8a1bbf5 100644 --- a/polkadot/runtime/westend/src/tests.rs +++ b/polkadot/runtime/westend/src/tests.rs @@ -144,7 +144,7 @@ mod remote_tests { if var("RUN_MIGRATION_TESTS").is_err() { return; } - use frame_support::assert_ok; + use frame_support::{assert_ok, traits::fungible::Inspect}; sp_tracing::try_init_simple(); let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); @@ -193,32 +193,23 @@ mod remote_tests { // member migration stats let mut success = 0; - let mut members_below_ed = 0; let mut direct_stakers = 0; let mut unexpected_errors = 0; // iterate over all pool members - pallet_nomination_pools::PoolMembers::::iter().for_each(|(k, member)| { + pallet_nomination_pools::PoolMembers::::iter_keys().for_each(|k| { if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( k.clone(), ) { // reasons migrations can fail: let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); - let member_balance_below_ed = member.total_balance() < ExistentialDeposit::get(); let migration = pallet_nomination_pools::Pallet::::migrate_delegation( RuntimeOrigin::signed(alice.clone()).into(), sp_runtime::MultiAddress::Id(k.clone()), ); - if member_balance_below_ed { - // if the member has balance below ED, the migration should fail. - members_below_ed += 1; - assert_eq!( - migration.unwrap_err(), - pallet_nomination_pools::Error::::MinimumBondNotMet.into() - ); - } else if is_direct_staker { + if is_direct_staker { // if the member is a direct staker, the migration should fail until pool // member unstakes all funds from pallet-staking. direct_stakers += 1; @@ -237,9 +228,8 @@ mod remote_tests { log::info!( target: "remote_test", - "Migration stats: success: {}, members_below_ed: {}, direct_stakers: {}, unexpected_errors: {}", + "Migration stats: success: {}, direct_stakers: {}, unexpected_errors: {}", success, - members_below_ed, direct_stakers, unexpected_errors ); From 8cdde304e8b8cca2b26933e2a03fb1e2040ae741 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 13 Aug 2024 15:06:47 +0200 Subject: [PATCH 47/57] move remote tests to another file --- polkadot/runtime/westend/src/lib.rs | 147 -------------------- polkadot/runtime/westend/src/tests.rs | 147 ++++++++++++++++++++ substrate/frame/nomination-pools/src/lib.rs | 9 +- 3 files changed, 148 insertions(+), 155 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index c89bcac394a3..e000410a6b45 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -2782,153 +2782,6 @@ sp_api::impl_runtime_apis! { } } -#[cfg(all(test, feature = "try-runtime"))] -mod remote_tests { - use super::*; - use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect}; - use remote_externalities::{ - Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport, - }; - use std::env::var; - - #[tokio::test] - async fn run_migrations() { - if var("RUN_MIGRATION_TESTS").is_err() { - return; - } - - sp_tracing::try_init_simple(); - let transport: Transport = - var("WS").unwrap_or("wss://westend-rpc.polkadot.io:443".to_string()).into(); - let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); - let mut ext = Builder::::default() - .mode(if let Some(state_snapshot) = maybe_state_snapshot { - Mode::OfflineOrElseOnline( - OfflineConfig { state_snapshot: state_snapshot.clone() }, - OnlineConfig { - transport, - state_snapshot: Some(state_snapshot), - ..Default::default() - }, - ) - } else { - Mode::Online(OnlineConfig { transport, ..Default::default() }) - }) - .build() - .await - .unwrap(); - ext.execute_with(|| Runtime::on_runtime_upgrade(UpgradeCheckSelect::PreAndPost)); - } - - #[tokio::test] - async fn delegate_stake_migration() { - // Intended to be run only manually. - if var("RUN_MIGRATION_TESTS").is_err() { - return; - } - use frame_support::assert_ok; - sp_tracing::try_init_simple(); - - let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); - let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); - let mut ext = Builder::::default() - .mode(if let Some(state_snapshot) = maybe_state_snapshot { - Mode::OfflineOrElseOnline( - OfflineConfig { state_snapshot: state_snapshot.clone() }, - OnlineConfig { - transport, - state_snapshot: Some(state_snapshot), - pallets: vec![ - "staking".into(), - "system".into(), - "balances".into(), - "nomination-pools".into(), - "delegated-staking".into(), - ], - ..Default::default() - }, - ) - } else { - Mode::Online(OnlineConfig { transport, ..Default::default() }) - }) - .build() - .await - .unwrap(); - ext.execute_with(|| { - // create an account with some balance - let alice = AccountId::from([1u8; 32]); - use frame_support::traits::Currency; - let _ = Balances::deposit_creating(&alice, 100_000 * UNITS); - - // iterate over all pools - pallet_nomination_pools::BondedPools::::iter_keys().for_each(|k| { - if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration(k) - { - assert_ok!( - pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( - RuntimeOrigin::signed(alice.clone()).into(), - k, - ) - ); - } - }); - - // member migration stats - let mut success = 0; - let mut members_below_ed = 0; - let mut direct_stakers = 0; - let mut unexpected_errors = 0; - - // iterate over all pool members - pallet_nomination_pools::PoolMembers::::iter().for_each(|(k, member)| { - if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( - k.clone(), - ) { - // reasons migrations can fail: - let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); - let member_balance_below_ed = member.total_balance() < ExistentialDeposit::get(); - - let migration = pallet_nomination_pools::Pallet::::migrate_delegation( - RuntimeOrigin::signed(alice.clone()).into(), - sp_runtime::MultiAddress::Id(k.clone()), - ); - - if member_balance_below_ed { - // if the member has balance below ED, the migration should fail. - members_below_ed += 1; - assert_eq!( - migration.unwrap_err(), - pallet_nomination_pools::Error::::MinimumBondNotMet.into() - ); - } else if is_direct_staker { - // if the member is a direct staker, the migration should fail until pool - // member unstakes all funds from pallet-staking. - direct_stakers += 1; - assert_eq!( - migration.unwrap_err(), - pallet_delegated_staking::Error::::AlreadyStaking.into() - ); - } else if migration.is_err() { - unexpected_errors += 1; - log::error!(target: "remote_test", "Unexpected error {:?} while migrating {:?}", migration.unwrap_err(), k); - } else { - success += 1; - } - } - }); - - log::info!( - target: "remote_test", - "Migration stats: success: {}, members_below_ed: {}, direct_stakers: {}, unexpected_errors: {}", - success, - members_below_ed, - direct_stakers, - unexpected_errors - ); - }); - } -} - mod clean_state_migration { use super::Runtime; #[cfg(feature = "try-runtime")] diff --git a/polkadot/runtime/westend/src/tests.rs b/polkadot/runtime/westend/src/tests.rs index 4d5e2e946bce..a910a975a459 100644 --- a/polkadot/runtime/westend/src/tests.rs +++ b/polkadot/runtime/westend/src/tests.rs @@ -99,3 +99,150 @@ fn check_treasury_pallet_id() { westend_runtime_constants::TREASURY_PALLET_ID ); } + +#[cfg(all(test, feature = "try-runtime"))] +mod remote_tests { + use super::*; + use frame_try_runtime::{runtime_decl_for_try_runtime::TryRuntime, UpgradeCheckSelect}; + use remote_externalities::{ + Builder, Mode, OfflineConfig, OnlineConfig, SnapshotConfig, Transport, + }; + use std::env::var; + + #[tokio::test] + async fn run_migrations() { + if var("RUN_MIGRATION_TESTS").is_err() { + return; + } + + sp_tracing::try_init_simple(); + let transport: Transport = + var("WS").unwrap_or("wss://westend-rpc.polkadot.io:443".to_string()).into(); + let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); + let mut ext = Builder::::default() + .mode(if let Some(state_snapshot) = maybe_state_snapshot { + Mode::OfflineOrElseOnline( + OfflineConfig { state_snapshot: state_snapshot.clone() }, + OnlineConfig { + transport, + state_snapshot: Some(state_snapshot), + ..Default::default() + }, + ) + } else { + Mode::Online(OnlineConfig { transport, ..Default::default() }) + }) + .build() + .await + .unwrap(); + ext.execute_with(|| Runtime::on_runtime_upgrade(UpgradeCheckSelect::PreAndPost)); + } + + #[tokio::test] + async fn delegate_stake_migration() { + // Intended to be run only manually. + if var("RUN_MIGRATION_TESTS").is_err() { + return; + } + use frame_support::assert_ok; + sp_tracing::try_init_simple(); + + let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); + let maybe_state_snapshot: Option = var("SNAP").map(|s| s.into()).ok(); + let mut ext = Builder::::default() + .mode(if let Some(state_snapshot) = maybe_state_snapshot { + Mode::OfflineOrElseOnline( + OfflineConfig { state_snapshot: state_snapshot.clone() }, + OnlineConfig { + transport, + state_snapshot: Some(state_snapshot), + pallets: vec![ + "staking".into(), + "system".into(), + "balances".into(), + "nomination-pools".into(), + "delegated-staking".into(), + ], + ..Default::default() + }, + ) + } else { + Mode::Online(OnlineConfig { transport, ..Default::default() }) + }) + .build() + .await + .unwrap(); + ext.execute_with(|| { + // create an account with some balance + let alice = AccountId::from([1u8; 32]); + use frame_support::traits::Currency; + let _ = Balances::deposit_creating(&alice, 100_000 * UNITS); + + // iterate over all pools + pallet_nomination_pools::BondedPools::::iter_keys().for_each(|k| { + if pallet_nomination_pools::Pallet::::api_pool_needs_delegate_migration(k) + { + assert_ok!( + pallet_nomination_pools::Pallet::::migrate_pool_to_delegate_stake( + RuntimeOrigin::signed(alice.clone()).into(), + k, + ) + ); + } + }); + + // member migration stats + let mut success = 0; + let mut members_below_ed = 0; + let mut direct_stakers = 0; + let mut unexpected_errors = 0; + + // iterate over all pool members + pallet_nomination_pools::PoolMembers::::iter().for_each(|(k, member)| { + if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( + k.clone(), + ) { + // reasons migrations can fail: + let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); + let member_balance_below_ed = member.total_balance() < ExistentialDeposit::get(); + + let migration = pallet_nomination_pools::Pallet::::migrate_delegation( + RuntimeOrigin::signed(alice.clone()).into(), + sp_runtime::MultiAddress::Id(k.clone()), + ); + + if member_balance_below_ed { + // if the member has balance below ED, the migration should fail. + members_below_ed += 1; + assert_eq!( + migration.unwrap_err(), + pallet_nomination_pools::Error::::MinimumBondNotMet.into() + ); + } else if is_direct_staker { + // if the member is a direct staker, the migration should fail until pool + // member unstakes all funds from pallet-staking. + direct_stakers += 1; + assert_eq!( + migration.unwrap_err(), + pallet_delegated_staking::Error::::AlreadyStaking.into() + ); + } else if migration.is_err() { + unexpected_errors += 1; + log::error!(target: "remote_test", "Unexpected error {:?} while migrating {:?}", migration.unwrap_err(), k); + } else { + success += 1; + } + } + }); + + log::info!( + target: "remote_test", + "Migration stats: success: {}, members_below_ed: {}, direct_stakers: {}, unexpected_errors: {}", + success, + members_below_ed, + direct_stakers, + unexpected_errors + ); + }); + } +} diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index ce22a3bb93e6..3e763074ca16 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2976,19 +2976,12 @@ pub mod pallet { Error::::NotMigrated ); - let pool_contribution = member.total_balance(); - // ensure the pool contribution is greater than the existential deposit otherwise we - // cannot transfer funds to member account. - ensure!( - pool_contribution >= T::Currency::minimum_balance(), - Error::::MinimumBondNotMet - ); - let delegation = T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); // delegation should not exist. ensure!(delegation.is_none(), Error::::AlreadyMigrated); + let pool_contribution = member.total_balance(); T::StakeAdapter::migrate_delegation( Pool::from(Pallet::::generate_bonded_account(member.pool_id)), Member::from(member_account), From 99698d6021f6997555125b4f0c6ce40a7b86145f Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 13 Aug 2024 16:28:45 +0200 Subject: [PATCH 48/57] fix below ed test --- polkadot/runtime/westend/src/tests.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/polkadot/runtime/westend/src/tests.rs b/polkadot/runtime/westend/src/tests.rs index a910a975a459..fa9ea8a1bbf5 100644 --- a/polkadot/runtime/westend/src/tests.rs +++ b/polkadot/runtime/westend/src/tests.rs @@ -144,7 +144,7 @@ mod remote_tests { if var("RUN_MIGRATION_TESTS").is_err() { return; } - use frame_support::assert_ok; + use frame_support::{assert_ok, traits::fungible::Inspect}; sp_tracing::try_init_simple(); let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); @@ -193,32 +193,23 @@ mod remote_tests { // member migration stats let mut success = 0; - let mut members_below_ed = 0; let mut direct_stakers = 0; let mut unexpected_errors = 0; // iterate over all pool members - pallet_nomination_pools::PoolMembers::::iter().for_each(|(k, member)| { + pallet_nomination_pools::PoolMembers::::iter_keys().for_each(|k| { if pallet_nomination_pools::Pallet::::api_member_needs_delegate_migration( k.clone(), ) { // reasons migrations can fail: let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); - let member_balance_below_ed = member.total_balance() < ExistentialDeposit::get(); let migration = pallet_nomination_pools::Pallet::::migrate_delegation( RuntimeOrigin::signed(alice.clone()).into(), sp_runtime::MultiAddress::Id(k.clone()), ); - if member_balance_below_ed { - // if the member has balance below ED, the migration should fail. - members_below_ed += 1; - assert_eq!( - migration.unwrap_err(), - pallet_nomination_pools::Error::::MinimumBondNotMet.into() - ); - } else if is_direct_staker { + if is_direct_staker { // if the member is a direct staker, the migration should fail until pool // member unstakes all funds from pallet-staking. direct_stakers += 1; @@ -237,9 +228,8 @@ mod remote_tests { log::info!( target: "remote_test", - "Migration stats: success: {}, members_below_ed: {}, direct_stakers: {}, unexpected_errors: {}", + "Migration stats: success: {}, direct_stakers: {}, unexpected_errors: {}", success, - members_below_ed, direct_stakers, unexpected_errors ); From 5e465f298390f7d13e3fa1816eeb7f900736aba1 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 13 Aug 2024 16:37:50 +0200 Subject: [PATCH 49/57] revert comment --- substrate/frame/nomination-pools/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 3e763074ca16..40f504e394cb 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -3949,10 +3949,10 @@ impl Pallet { PoolMembers::::get(who.clone()) .map(|pool_member| { - // if Self::api_pool_needs_delegate_migration(pool_member.pool_id) { - // // the pool needs to be migrated before members can be migrated. - // return false - // } + if Self::api_pool_needs_delegate_migration(pool_member.pool_id) { + // the pool needs to be migrated before members can be migrated. + return false + } let member_balance = pool_member.total_balance(); let delegated_balance = From c38f83cf167dad0a7a200333220c96e69e5bd0f3 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 13 Aug 2024 16:54:42 +0200 Subject: [PATCH 50/57] ensure there is atleast ED to transfer --- substrate/frame/nomination-pools/src/lib.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 40f504e394cb..5dc89697cfba 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2976,12 +2976,19 @@ pub mod pallet { Error::::NotMigrated ); + let pool_contribution = member.total_balance(); + // ensure the pool contribution is greater than the existential deposit otherwise we + // cannot transfer funds to member account. + ensure!( + pool_contribution >= T::Currency::minimum_balance(), + Error::::MinimumBondNotMet + ); + let delegation = T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone())); // delegation should not exist. ensure!(delegation.is_none(), Error::::AlreadyMigrated); - let pool_contribution = member.total_balance(); T::StakeAdapter::migrate_delegation( Pool::from(Pallet::::generate_bonded_account(member.pool_id)), Member::from(member_account), From b3a326396110d38ec03a0c5b9f885400be2607e2 Mon Sep 17 00:00:00 2001 From: Ankan Date: Tue, 13 Aug 2024 17:19:25 +0200 Subject: [PATCH 51/57] fix imports --- polkadot/runtime/westend/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/polkadot/runtime/westend/src/tests.rs b/polkadot/runtime/westend/src/tests.rs index fa9ea8a1bbf5..dc8103ab52c4 100644 --- a/polkadot/runtime/westend/src/tests.rs +++ b/polkadot/runtime/westend/src/tests.rs @@ -144,7 +144,7 @@ mod remote_tests { if var("RUN_MIGRATION_TESTS").is_err() { return; } - use frame_support::{assert_ok, traits::fungible::Inspect}; + use frame_support::assert_ok; sp_tracing::try_init_simple(); let transport: Transport = var("WS").unwrap_or("ws://127.0.0.1:9900".to_string()).into(); From dfcdc4267d7765f700ad38ea90d655277678ffe9 Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Tue, 13 Aug 2024 21:39:44 +0200 Subject: [PATCH 52/57] Update substrate/frame/delegated-staking/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Gonçalo Pestana --- substrate/frame/delegated-staking/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 87a0913aa581..1c7c25d42907 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -651,7 +651,8 @@ impl Pallet { defensive_assert!(released == amount, "hold should have been released fully"); // update delegation. - if delegation.update(&delegator) { + let removed = delegation.update(&delegator); + if removed { // remove provider for delegator if no delegation left. let _ = frame_system::Pallet::::dec_providers(&delegator).defensive(); } From bce9803fb9784fd689425962bff270433b7fce16 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 14 Aug 2024 01:38:49 +0200 Subject: [PATCH 53/57] refactor inc dec of providers --- substrate/frame/delegated-staking/src/lib.rs | 38 ++++--------------- .../frame/delegated-staking/src/tests.rs | 14 ++++++- .../frame/delegated-staking/src/types.rs | 35 +++++++++++++---- 3 files changed, 48 insertions(+), 39 deletions(-) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 0cfff4f5b467..08ce747ec0c0 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -319,9 +319,7 @@ pub mod pallet { Error::::NotAllowed ); - // remove provider reference - let _ = frame_system::Pallet::::dec_providers(&who)?; - >::remove(who); + AgentLedger::::remove(&who); Ok(()) } @@ -490,13 +488,8 @@ impl Pallet { /// Registers a new agent in the system. fn do_register_agent(who: &T::AccountId, reward_account: &T::AccountId) { + // TODO: Consider taking a deposit for being an agent. AgentLedger::::new(reward_account).update(who); - - // Agent does not hold balance of its own but this pallet will provide for this to exist. - // This is expected to be a keyless account and not created by any user directly so safe. - // TODO: Someday if we allow anyone to be an agent, we should take a deposit for - // being a delegator. - frame_system::Pallet::::inc_providers(who); } /// Migrate existing staker account `who` to an `Agent` account. @@ -574,16 +567,13 @@ impl Pallet { if let Some(mut existing_delegation) = Delegation::::get(&delegator) { ensure!(existing_delegation.agent == agent, Error::::InvalidDelegation); - // update amount + // update amount and return the updated delegation. existing_delegation.amount = existing_delegation .amount .checked_add(&amount) .ok_or(ArithmeticError::Overflow)?; existing_delegation } else { - // if this is the first time delegation, increment provider count. This ensures that - // amount including existential deposit can be held. - frame_system::Pallet::::inc_providers(&delegator); Delegation::::new(&agent, amount) } .update(&delegator); @@ -646,11 +636,7 @@ impl Pallet { defensive_assert!(released == amount, "hold should have been released fully"); // update delegation. - let removed = delegation.update(&delegator); - if removed { - // remove provider for delegator if no delegation left. - let _ = frame_system::Pallet::::dec_providers(&delegator).defensive(); - } + delegation.update(&delegator); Self::deposit_event(Event::::Released { agent, delegator, amount }); @@ -678,10 +664,7 @@ impl Pallet { let agent = source_delegation.agent.clone(); // create a new delegation for destination delegator. - let _ = Delegation::::new(&agent, amount).update(&destination_delegator); - // Provide for the delegator account. This ensures that amount including existential deposit - // can be held. - frame_system::Pallet::::inc_providers(&destination_delegator); + Delegation::::new(&agent, amount).update(&destination_delegator); source_delegation.amount = source_delegation .amount @@ -700,10 +683,7 @@ impl Pallet { )?; // update source delegation. - if source_delegation.update(&source_delegator) { - // remove provider for delegator if no delegation left. - let _ = frame_system::Pallet::::dec_providers(&source_delegator).defensive(); - } + source_delegation.update(&source_delegator); Self::deposit_event(Event::::MigratedDelegation { agent, @@ -745,11 +725,7 @@ impl Pallet { agent_ledger.remove_slash(actual_slash).save(); delegation.amount = delegation.amount.checked_sub(&actual_slash).ok_or(ArithmeticError::Overflow)?; - - if delegation.update(&delegator) { - // remove provider for delegator if no delegation left. - let _ = frame_system::Pallet::::dec_providers(&delegator).defensive(); - } + delegation.update(&delegator); if let Some(reporter) = maybe_reporter { let reward_payout: BalanceOf = T::SlashRewardFraction::get() * actual_slash; diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index 9d5382ec622a..2c965e18b1b3 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -773,7 +773,9 @@ mod staking_integration { assert_eq!(System::providers(&delegator), 1); } - // agent is cleaned up from storage + // Agent can be removed now. + assert_ok!(DelegatedStaking::remove_agent(RawOrigin::Signed(agent).into())); + // agent is correctly removed. assert!(!Agents::::contains_key(agent)); // and no provider left. assert_eq!(System::providers(&agent), 0); @@ -824,6 +826,13 @@ mod staking_integration { assert_ok!(Staking::unbond(RawOrigin::Signed(agent).into(), 1000)); start_era(4); assert_ok!(Staking::withdraw_unbonded(RawOrigin::Signed(agent).into(), 0)); + + // Since delegations are still left, agents cannot be removed yet from storage. + assert_noop!( + DelegatedStaking::remove_agent(RawOrigin::Signed(agent).into()), + Error::::NotAllowed + ); + assert_ok!(DelegatedStaking::release_delegation( RawOrigin::Signed(agent).into(), delegator, @@ -831,6 +840,9 @@ mod staking_integration { 0 )); + // now agents can be removed. + assert_ok!(DelegatedStaking::remove_agent(RawOrigin::Signed(agent).into())); + // agent and delegator provider is decremented. assert_eq!(System::providers(&delegator), 1); assert_eq!(System::providers(&agent), 1); diff --git a/substrate/frame/delegated-staking/src/types.rs b/substrate/frame/delegated-staking/src/types.rs index 6d95db7d146f..d4b18b09fa55 100644 --- a/substrate/frame/delegated-staking/src/types.rs +++ b/substrate/frame/delegated-staking/src/types.rs @@ -66,16 +66,23 @@ impl Delegation { /// Save self to storage. /// - /// If the delegation amount is zero, remove the delegation and returns true. - pub(crate) fn update(self, key: &T::AccountId) -> bool { - // Clean up if no delegation left. - if self.amount == Zero::zero() { - >::remove(key); - return true + /// If the delegation amount is zero, remove the delegation. Also adds and removes provider + /// reference as needed. + pub(crate) fn update(self, key: &T::AccountId) { + if >::contains_key(key) { + // Clean up if no delegation left. + if self.amount == Zero::zero() { + >::remove(key); + // Remove provider if no delegation left. + let _ = frame_system::Pallet::::dec_providers(&key).defensive(); + return + } + } else { + // this is a new delegation. Provide for this account. + frame_system::Pallet::::inc_providers(&key); } >::insert(key, self); - false } } @@ -121,10 +128,24 @@ impl AgentLedger { } /// Save self to storage with the given key. + /// + /// Increments provider count if this is a new agent. pub(crate) fn update(self, key: &T::AccountId) { + if !>::contains_key(key) { + // This is a new agent. Provide for this account. + frame_system::Pallet::::inc_providers(&key); + } >::insert(key, self) } + /// Remove self from storage. + pub(crate) fn remove(key: &T::AccountId) { + debug_assert!(>::contains_key(key), "Agent should exist in storage"); + >::remove(key); + // Remove provider reference. + let _ = frame_system::Pallet::::dec_providers(key).defensive(); + } + /// Effective total balance of the `Agent`. /// /// This takes into account any slashes reported to `Agent` but unapplied. From ae9a370340d10646b568b780dffaae14e4b8c77e Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 14 Aug 2024 01:53:34 +0200 Subject: [PATCH 54/57] gate extrinsics on top level function --- substrate/frame/nomination-pools/src/lib.rs | 50 ++++++++++++++------- 1 file changed, 33 insertions(+), 17 deletions(-) diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index c9e323154940..44e3463dc9f2 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2074,6 +2074,13 @@ pub mod pallet { )] pub fn bond_extra(origin: OriginFor, extra: BondExtra>) -> DispatchResult { let who = ensure_signed(origin)?; + + // ensure who is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(who.clone()), + Error::::NotMigrated + ); + Self::do_bond_extra(who.clone(), who, extra) } @@ -2089,6 +2096,12 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::claim_payout())] pub fn claim_payout(origin: OriginFor) -> DispatchResult { let signer = ensure_signed(origin)?; + // ensure signer is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(signer.clone()), + Error::::NotMigrated + ); + Self::do_claim_payout(signer.clone(), signer) } @@ -2747,7 +2760,14 @@ pub mod pallet { extra: BondExtra>, ) -> DispatchResult { let who = ensure_signed(origin)?; - Self::do_bond_extra(who, T::Lookup::lookup(member)?, extra) + let member_account = T::Lookup::lookup(member)?; + // ensure member is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(member_account.clone()), + Error::::NotMigrated + ); + + Self::do_bond_extra(who, member_account, extra) } /// Allows a pool member to set a claim permission to allow or disallow permissionless @@ -2787,6 +2807,12 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::claim_payout())] pub fn claim_payout_other(origin: OriginFor, other: T::AccountId) -> DispatchResult { let signer = ensure_signed(origin)?; + // ensure member is not in an un-migrated state. + ensure!( + !Self::api_member_needs_delegate_migration(other.clone()), + Error::::NotMigrated + ); + Self::do_claim_payout(signer, other) } @@ -2892,6 +2918,9 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::claim_commission())] pub fn claim_commission(origin: OriginFor, pool_id: PoolId) -> DispatchResult { let who = ensure_signed(origin)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); + Self::do_claim_commission(who, pool_id) } @@ -2906,6 +2935,9 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::adjust_pool_deposit())] pub fn adjust_pool_deposit(origin: OriginFor, pool_id: PoolId) -> DispatchResult { let who = ensure_signed(origin)?; + // ensure pool is not in an un-migrated state. + ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); + Self::do_adjust_pool_deposit(who, pool_id) } @@ -3387,12 +3419,6 @@ impl Pallet { member_account: T::AccountId, extra: BondExtra>, ) -> DispatchResult { - // ensure member is not in an un-migrated state. - ensure!( - !Self::api_member_needs_delegate_migration(member_account.clone()), - Error::::NotMigrated - ); - if signer != member_account { ensure!( ClaimPermissions::::get(&member_account).can_bond_extra(), @@ -3442,8 +3468,6 @@ impl Pallet { fn do_claim_commission(who: T::AccountId, pool_id: PoolId) -> DispatchResult { let bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; - // ensure pool is not in an un-migrated state. - ensure!(!Self::api_pool_needs_delegate_migration(pool_id), Error::::NotMigrated); ensure!(bonded_pool.can_claim_commission(&who), Error::::DoesNotHavePermission); let mut reward_pool = RewardPools::::get(pool_id) @@ -3490,12 +3514,6 @@ impl Pallet { signer: T::AccountId, member_account: T::AccountId, ) -> DispatchResult { - // ensure member is not in an un-migrated state. - ensure!( - !Self::api_member_needs_delegate_migration(member_account.clone()), - Error::::NotMigrated - ); - if signer != member_account { ensure!( ClaimPermissions::::get(&member_account).can_claim_payout(), @@ -3518,8 +3536,6 @@ impl Pallet { fn do_adjust_pool_deposit(who: T::AccountId, pool: PoolId) -> DispatchResult { let bonded_pool = BondedPool::::get(pool).ok_or(Error::::PoolNotFound)?; - // ensure pool is not in an un-migrated state. - ensure!(!Self::api_pool_needs_delegate_migration(pool), Error::::NotMigrated); let reward_acc = &bonded_pool.reward_account(); let pre_frozen_balance = From 251ba4cb39a9ee346dcce8b15fb21875b1797cbd Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 14 Aug 2024 01:56:03 +0200 Subject: [PATCH 55/57] fix clippy --- substrate/frame/delegated-staking/src/types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/delegated-staking/src/types.rs b/substrate/frame/delegated-staking/src/types.rs index d4b18b09fa55..a78aa3f55906 100644 --- a/substrate/frame/delegated-staking/src/types.rs +++ b/substrate/frame/delegated-staking/src/types.rs @@ -74,12 +74,12 @@ impl Delegation { if self.amount == Zero::zero() { >::remove(key); // Remove provider if no delegation left. - let _ = frame_system::Pallet::::dec_providers(&key).defensive(); + let _ = frame_system::Pallet::::dec_providers(key).defensive(); return } } else { // this is a new delegation. Provide for this account. - frame_system::Pallet::::inc_providers(&key); + frame_system::Pallet::::inc_providers(key); } >::insert(key, self); @@ -133,7 +133,7 @@ impl AgentLedger { pub(crate) fn update(self, key: &T::AccountId) { if !>::contains_key(key) { // This is a new agent. Provide for this account. - frame_system::Pallet::::inc_providers(&key); + frame_system::Pallet::::inc_providers(key); } >::insert(key, self) } From 4ba0c97487feacc69993c157cc20c0d9134f84ca Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 14 Aug 2024 02:06:22 +0200 Subject: [PATCH 56/57] remove empty line --- substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs index 7fee2a0bdb23..a7cee5175136 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -1394,7 +1394,6 @@ fn pool_no_dangling_delegation() { delegated_staking_events_since_last_call(), vec![DelegatedStakingEvent::Delegated { agent: POOL1_BONDED, - delegator: alice, amount: 40 },] From a886fa9fd7aa6db72e8dbce7b344890a91162675 Mon Sep 17 00:00:00 2001 From: Ankan Date: Wed, 14 Aug 2024 23:29:32 +0200 Subject: [PATCH 57/57] fix rust doc --- substrate/frame/delegated-staking/src/migration.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/substrate/frame/delegated-staking/src/migration.rs b/substrate/frame/delegated-staking/src/migration.rs index 62797e747aeb..8bc7312c4eab 100644 --- a/substrate/frame/delegated-staking/src/migration.rs +++ b/substrate/frame/delegated-staking/src/migration.rs @@ -27,7 +27,8 @@ pub mod unversioned { use alloc::vec::Vec; use sp_runtime::traits::AccountIdConversion; - /// Migrates `ProxyDelegator` accounts with better entropy than the previous. + /// Migrates `ProxyDelegator` accounts with better entropy than the old logic which didn't take + /// into account all the bytes of the agent account ID. pub struct ProxyDelegatorMigration(PhantomData<(T, MaxAgents)>); impl> OnRuntimeUpgrade for ProxyDelegatorMigration {