From feacf2f3f9f39c1dbf0ecbee983c4488f948fd61 Mon Sep 17 00:00:00 2001 From: Ankan <10196091+Ank4n@users.noreply.github.com> Date: Wed, 14 Aug 2024 21:48:13 +0200 Subject: [PATCH] [Pools] Fix issues with member migration to `DelegateStake` (#4822) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Context Pool members using the old `TransferStake` strategy were able to transfer all their funds to the pool. With `DelegateStake` changes, we want to ensure similar behaviour by allowing members to delegate all their stake to the pool. ## Changes - Ensure all the balance including ED of an account can be delegated (and used in the pool) by adding a provider for delegators. - Gates calls that mutates the pool or pool member if they are in unmigrated state. Closes https://github.com/paritytech-secops/srlabs_findings/issues/409. - Adds remote test to migrate all pools and members to `DelegateStake` which can be used with `Kusama` and `Polkadot` runtime state. closes https://github.com/paritytech/polkadot-sdk/issues/4629. - Add new runtime apis to read pool and member balance. ## Addressing possible migration errors Pool members migrating can run into two types of errors: - Already Staking: If the pool member is already staking, we cannot migrate them to `DelegateStake` since this may mean they are able to use the same staked funds in the pool. Users would need to withdraw all their funds from staking, in order to migrate their pool funds. - Pool contribution below ED: For these cases transfer from pool account to member account would fail. The affected users can top up their accounts and redo migration. Another error that was earlier possible was when member's free balance is below ED. This PR adds a provider to delegator allowing all user balance including ED can be contributed towards the pool. This helps `1095` accounts in Polkadot and `41` accounts in Kusama to migrate now which would have earlier failed. ## Results from RemoteExternalities Tests. ### Kusama `Migration stats: success: 3017, direct_stakers: 361, unexpected_errors: 0` ### Polkadot `Migration stats: success: 42859, direct_stakers: 643, unexpected_errors: 0` ## TODO - [x] Add runtime api for member total balance. - [x] New [issue](https://github.com/paritytech/polkadot-sdk/issues/5009) to reap pool members with contribution below ED. - [x] Add provider for delegators so whole balance including ED can be held while contributing to pools. - [x] Gate all pool extrinsics if pool/member is in non-migrated state. --------- Co-authored-by: Gonçalo Pestana --- polkadot/runtime/westend/src/lib.rs | 49 +--- polkadot/runtime/westend/src/tests.rs | 137 +++++++++++ prdoc/pr_4822.prdoc | 25 ++ substrate/bin/node/runtime/src/lib.rs | 8 + .../frame/delegated-staking/src/impls.rs | 2 +- substrate/frame/delegated-staking/src/lib.rs | 112 ++++----- .../frame/delegated-staking/src/tests.rs | 186 +++++++++++++-- .../frame/delegated-staking/src/types.rs | 38 ++- .../nomination-pools/runtime-api/src/lib.rs | 6 + substrate/frame/nomination-pools/src/lib.rs | 112 ++++++++- .../test-delegate-stake/src/lib.rs | 221 ++++++++++++++++-- 11 files changed, 733 insertions(+), 163 deletions(-) create mode 100644 prdoc/pr_4822.prdoc diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index aa446b03368d..a9fbbb4f33da 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -99,7 +99,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; #[cfg(any(feature = "std", test))] @@ -2476,6 +2476,14 @@ 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) + } + + fn pool_balance(pool_id: pallet_nomination_pools::PoolId) -> Balance { + NominationPools::api_pool_balance(pool_id) + } } impl pallet_staking_runtime_api::StakingApi for Runtime { @@ -2776,45 +2784,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)); - } -} - 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..dc8103ab52c4 100644 --- a/polkadot/runtime/westend/src/tests.rs +++ b/polkadot/runtime/westend/src/tests.rs @@ -99,3 +99,140 @@ 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 direct_stakers = 0; + let mut unexpected_errors = 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(), + ) { + // reasons migrations can fail: + let is_direct_staker = pallet_staking::Bonded::::contains_key(&k); + + let migration = pallet_nomination_pools::Pallet::::migrate_delegation( + RuntimeOrigin::signed(alice.clone()).into(), + sp_runtime::MultiAddress::Id(k.clone()), + ); + + 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: {}, direct_stakers: {}, unexpected_errors: {}", + success, + direct_stakers, + unexpected_errors + ); + }); + } +} diff --git a/prdoc/pr_4822.prdoc b/prdoc/pr_4822.prdoc new file mode 100644 index 000000000000..44f3e41d8d5a --- /dev/null +++ b/prdoc/pr_4822.prdoc @@ -0,0 +1,25 @@ +# 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: minor + - name: kitchensink-runtime + bump: patch + - name: pallet-delegated-staking + bump: patch + - name: pallet-nomination-pools + bump: minor + - name: sp-staking + bump: patch + - name: pallet-nomination-pools-runtime-api + bump: minor diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index ff3f7e26baf2..ea9a66862d64 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -2784,6 +2784,14 @@ 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) + } + + 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/delegated-staking/src/impls.rs b/substrate/frame/delegated-staking/src/impls.rs index 8c05b0bcfc22..4e6812dee249 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 1882989cfa7d..08ce747ec0c0 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -148,7 +148,7 @@ use frame_support::{ }, Balanced, Inspect as FunInspect, Mutate as FunMutate, }, - tokens::{fungible::Credit, Fortitude, Precision, Preservation}, + tokens::{fungible::Credit, Fortitude, Precision, Preservation, Restriction}, Defensive, DefensiveOption, Imbalance, OnUnbalanced, }, }; @@ -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(()) } @@ -392,9 +390,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); @@ -493,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. @@ -510,9 +500,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. - frame_system::Pallet::::inc_providers(&proxy_delegator.clone().get()); - // Get current stake let stake = T::CoreStaking::stake(who)?; @@ -534,7 +521,6 @@ impl Pallet { T::CoreStaking::set_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 @@ -578,21 +564,23 @@ impl Pallet { let delegator = delegator.get(); let mut ledger = AgentLedger::::get(&agent).ok_or(Error::::NotAgent)?; + + if let Some(mut existing_delegation) = Delegation::::get(&delegator) { + ensure!(existing_delegation.agent == agent, Error::::InvalidDelegation); + // update amount and return the updated delegation. + existing_delegation.amount = existing_delegation + .amount + .checked_add(&amount) + .ok_or(ArithmeticError::Overflow)?; + existing_delegation + } else { + Delegation::::new(&agent, amount) + } + .update(&delegator); + // 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) { - ensure!(existing_delegation.agent == agent, Error::::InvalidDelegation); - existing_delegation - .amount - .checked_add(&amount) - .ok_or(ArithmeticError::Overflow)? - } else { - amount - }; - - Delegation::::new(&agent, new_delegation_amount).update_or_kill(&delegator); ledger.total_delegated = ledger.total_delegated.checked_add(&amount).ok_or(ArithmeticError::Overflow)?; ledger.update(&agent); @@ -638,9 +626,6 @@ impl Pallet { .checked_sub(&amount) .defensive_ok_or(ArithmeticError::Overflow)?; - // remove delegator if nothing delegated anymore - delegation.update_or_kill(&delegator); - let released = T::Currency::release( &HoldReason::StakingDelegation.into(), &delegator, @@ -650,6 +635,9 @@ impl Pallet { defensive_assert!(released == amount, "hold should have been released fully"); + // update delegation. + delegation.update(&delegator); + Self::deposit_event(Event::::Released { agent, delegator, amount }); Ok(()) @@ -668,49 +656,34 @@ 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 - Delegation::::new(&agent, amount).update_or_kill(&destination_delegator); + // create a new delegation for destination delegator. + Delegation::::new(&agent, amount).update(&destination_delegator); source_delegation.amount = source_delegation .amount .checked_sub(&amount) .defensive_ok_or(Error::::BadState)?; - source_delegation.update_or_kill(&source_delegator); - - // release funds from source - let released = T::Currency::release( + // transfer the held amount in `source_delegator` to `destination_delegator`. + let _ = T::Currency::transfer_on_hold( &HoldReason::StakingDelegation.into(), - &source_delegator, - amount, - Precision::BestEffort, - )?; - - 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)?; - - // if balance is zero, clear provider for source (proxy) delegator. - if post_balance == Zero::zero() { - let _ = frame_system::Pallet::::dec_providers(&source_delegator).defensive(); - } + Precision::Exact, + Restriction::OnHold, + Fortitude::Polite, + )?; - // hold the funds again in the new delegator account. - T::Currency::hold(&HoldReason::StakingDelegation.into(), &destination_delegator, amount)?; + // update source delegation. + source_delegation.update(&source_delegator); Self::deposit_event(Event::::MigratedDelegation { agent, @@ -752,7 +725,7 @@ 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); + delegation.update(&delegator); if let Some(reporter) = maybe_reporter { let reward_payout: BalanceOf = T::SlashRewardFraction::get() * actual_slash; @@ -801,18 +774,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 bc8bc2dccc3c..2c965e18b1b3 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -334,6 +334,36 @@ 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::*; @@ -625,32 +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(&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; @@ -659,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 ); @@ -672,14 +712,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(), + RawOrigin::Signed(agent).into(), delegator, delegator_share )); + assert_eq!(System::providers(&delegator), 1); assert_eq!( Balances::balance_on_hold(&HoldReason::StakingDelegation.into(), &delegator), delegator_share @@ -694,20 +735,123 @@ 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 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); + }); + } + + #[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)); + + // 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, + 1000, + 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); + + // 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); }); } } diff --git a/substrate/frame/delegated-staking/src/types.rs b/substrate/frame/delegated-staking/src/types.rs index aff282774c3c..a78aa3f55906 100644 --- a/substrate/frame/delegated-staking/src/types.rs +++ b/substrate/frame/delegated-staking/src/types.rs @@ -64,15 +64,25 @@ 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) { - // Clean up if no delegation left. - if self.amount == Zero::zero() { - >::remove(key); - return + /// Save self to storage. + /// + /// 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) + >::insert(key, self); } } @@ -118,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. diff --git a/substrate/frame/nomination-pools/runtime-api/src/lib.rs b/substrate/frame/nomination-pools/runtime-api/src/lib.rs index 67627e0acb13..d81ad1dd4954 100644 --- a/substrate/frame/nomination-pools/runtime-api/src/lib.rs +++ b/substrate/frame/nomination-pools/runtime-api/src/lib.rs @@ -63,5 +63,11 @@ 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; + + /// 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 9108060ccb2a..44e3463dc9f2 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -2010,6 +2010,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 @@ -2072,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) } @@ -2087,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) } @@ -2130,6 +2145,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)?; @@ -2213,6 +2234,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 @@ -2259,6 +2283,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(); @@ -2493,6 +2523,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) @@ -2527,6 +2559,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); @@ -2562,6 +2596,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); @@ -2638,6 +2674,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, @@ -2684,8 +2723,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)? @@ -2720,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 @@ -2737,9 +2784,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; }); @@ -2755,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) } @@ -2773,6 +2831,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) @@ -2809,6 +2870,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)?; @@ -2831,6 +2895,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)?; @@ -2852,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) } @@ -2866,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) } @@ -2882,6 +2954,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(); @@ -2956,9 +3030,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())); @@ -3459,6 +3536,7 @@ impl Pallet { fn do_adjust_pool_deposit(who: T::AccountId, pool: PoolId) -> DispatchResult { let bonded_pool = BondedPool::::get(pool).ok_or(Error::::PoolNotFound)?; + let reward_acc = &bonded_pool.reward_account(); let pre_frozen_balance = T::Currency::balance_frozen(&FreezeReason::PoolMinBalance.into(), reward_acc); @@ -3880,7 +3958,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 @@ -3914,6 +3998,22 @@ impl Pallet { }) .unwrap_or_default() } + + /// Contribution of the member in the pool. + /// + /// 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()) + .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 { 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 a50a6b73f5fc..7fee2a0bdb23 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -25,16 +25,16 @@ 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, }; -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}; +use sp_runtime::{bounded_btree_map, traits::Zero, Perbill}; use sp_staking::Agent; #[test] @@ -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 @@ -1031,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); @@ -1060,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(), @@ -1113,8 +1110,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,17 +1126,20 @@ 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. + // 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, released_balance: 0 }, + PoolsEvent::Unbonded { member: 22, pool_id: 1, balance: 5, points: 5, era: 9 }, PoolsEvent::Withdrawn { member: 22, pool_id: 1, balance: 5, points: 5 }, ] ); @@ -1184,6 +1187,183 @@ 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)); + + // ensure pool mutation is not allowed until pool is migrated. + assert_noop!( + Pools::join(RuntimeOrigin::signed(21), 10, 1), + PoolsError::::NotMigrated + ); + assert_noop!( + 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(), + vec![DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: DelegatedStaking::generate_proxy_delegator(Agent::from(POOL1_BONDED)) + .get(), + amount: 50 + 10 + },] + ); + + // 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 + ); + assert_noop!( + Pools::bond_extra_other(RuntimeOrigin::signed(10), 20, BondExtra::Rewards), + PoolsError::::NotMigrated + ); + assert_noop!( + Pools::claim_payout(RuntimeOrigin::signed(20)), + PoolsError::::NotMigrated + ); + 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)); + // 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 }, + ] + ); + }) +} + #[test] fn pool_no_dangling_delegation() { new_test_ext().execute_with(|| { @@ -1214,6 +1394,7 @@ fn pool_no_dangling_delegation() { delegated_staking_events_since_last_call(), vec![DelegatedStakingEvent::Delegated { agent: POOL1_BONDED, + delegator: alice, amount: 40 },]