From e8baac7848c1ce075efad4793b6afc92b6f66106 Mon Sep 17 00:00:00 2001 From: Piet <75956460+PieWol@users.noreply.github.com> Date: Sun, 1 Oct 2023 03:36:48 +0200 Subject: [PATCH] Tvl pool staking (#1322) What does this PR do? - Introduced the TotalValueLocked storage for nomination-pools. - introduced a slashing api in mock.rs - additional test for tracking a slashing event towards a pool without sub-pools - migration for the nomination-pools (V6 to V7) with `VersionedMigration` Why are these changes needed? this is the continuation of the work by @kianenigma in this [PR](https://github.com/paritytech/substrate/pull/13319) How were these changes implemented and what do they affect? - It's an extra StorageValue that's modified whenever funds flow in or out of staking for any of the `bonded_account` of `BondedPools` - The `PoolSlashed`event is now emitted even when no `SubPools` are found Closes https://github.com/paritytech/polkadot-sdk/issues/155 KSM: HHEEgVzcqL3kCXgsxSfJMbsTy8dxoTctuXtpY94n4s8F4pS --------- Co-authored-by: Liam Aharon Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com> Co-authored-by: Ankan Co-authored-by: command-bot <> --- polkadot/runtime/westend/src/lib.rs | 1 + substrate/frame/nomination-pools/src/lib.rs | 155 +++++++++--- .../frame/nomination-pools/src/migration.rs | 87 +++++++ substrate/frame/nomination-pools/src/mock.rs | 48 +++- substrate/frame/nomination-pools/src/tests.rs | 225 +++++++++++++++--- substrate/frame/staking/src/lib.rs | 10 +- substrate/frame/staking/src/mock.rs | 1 + substrate/primitives/staking/src/lib.rs | 2 + 8 files changed, 441 insertions(+), 88 deletions(-) diff --git a/polkadot/runtime/westend/src/lib.rs b/polkadot/runtime/westend/src/lib.rs index 34f116eb9f04..6085b6e37455 100644 --- a/polkadot/runtime/westend/src/lib.rs +++ b/polkadot/runtime/westend/src/lib.rs @@ -1508,6 +1508,7 @@ pub mod migrations { paras_registrar::migration::VersionCheckedMigrateToV1, pallet_nomination_pools::migration::versioned_migrations::V5toV6, pallet_referenda::migration::v1::MigrateV0ToV1, + pallet_nomination_pools::migration::versioned_migrations::V6ToV7, ); } diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 2ec9b537d320..909a930e3821 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -538,6 +538,31 @@ impl PoolMember { } } + /// Total balance of the member, both active and unbonding. + /// Doesn't mutate state. + #[cfg(any(feature = "try-runtime", feature = "fuzzing", test, debug_assertions))] + fn total_balance(&self) -> BalanceOf { + let pool = BondedPool::::get(self.pool_id).unwrap(); + let active_balance = pool.points_to_balance(self.active_points()); + + let sub_pools = match SubPoolsStorage::::get(self.pool_id) { + Some(sub_pools) => sub_pools, + None => return active_balance, + }; + + let unbonding_balance = self.unbonding_eras.iter().fold( + BalanceOf::::zero(), + |accumulator, (era, unlocked_points)| { + // if the `SubPools::with_era` has already been merged into the + // `SubPools::no_era` use this pool instead. + let era_pool = sub_pools.with_era.get(era).unwrap_or(&sub_pools.no_era); + accumulator + (era_pool.point_to_balance(*unlocked_points)) + }, + ); + + active_balance + unbonding_balance + } + /// Total points of this member, both active and unbonding. fn total_points(&self) -> BalanceOf { self.active_points().saturating_add(self.unbonding_points()) @@ -1189,11 +1214,11 @@ impl BondedPool { Ok(()) } - /// Bond exactly `amount` from `who`'s funds into this pool. + /// Bond exactly `amount` from `who`'s funds into this pool. Increases the [`TotalValueLocked`] + /// by `amount`. /// - /// If the bond type is `Create`, `Staking::bond` is called, and `who` - /// is allowed to be killed. Otherwise, `Staking::bond_extra` is called and `who` - /// cannot be killed. + /// If the bond is [`BondType::Create`], [`Staking::bond`] is called, and `who` is allowed to be + /// killed. Otherwise, [`Staking::bond_extra`] is called and `who` cannot be killed. /// /// Returns `Ok(points_issues)`, `Err` otherwise. fn try_bond_funds( @@ -1224,6 +1249,9 @@ impl BondedPool { // found, we exit early. BondType::Later => T::Staking::bond_extra(&bonded_account, amount)?, } + TotalValueLocked::::mutate(|tvl| { + tvl.saturating_accrue(amount); + }); Ok(points_issued) } @@ -1239,6 +1267,27 @@ impl BondedPool { }); }; } + + /// Withdraw all the funds that are already unlocked from staking for the + /// [`BondedPool::bonded_account`]. + /// + /// Also reduces the [`TotalValueLocked`] by the difference of the + /// [`T::Staking::total_stake`] of the [`BondedPool::bonded_account`] that might occur by + /// [`T::Staking::withdraw_unbonded`]. + /// + /// Returns the result of [`T::Staking::withdraw_unbonded`] + fn withdraw_from_staking(&self, num_slashing_spans: u32) -> Result { + let bonded_account = self.bonded_account(); + + let prev_total = T::Staking::total_stake(&bonded_account.clone()).unwrap_or_default(); + let outcome = T::Staking::withdraw_unbonded(bonded_account.clone(), num_slashing_spans); + let diff = prev_total + .defensive_saturating_sub(T::Staking::total_stake(&bonded_account).unwrap_or_default()); + TotalValueLocked::::mutate(|tvl| { + tvl.saturating_reduce(diff); + }); + outcome + } } /// A reward pool. @@ -1437,9 +1486,7 @@ impl UnbondPool { } /// Dissolve some points from the unbonding pool, reducing the balance of the pool - /// proportionally. - /// - /// This is the opposite of `issue`. + /// proportionally. This is the opposite of `issue`. /// /// Returns the actual amount of `Balance` that was removed from the pool. fn dissolve(&mut self, points: BalanceOf) -> BalanceOf { @@ -1525,7 +1572,7 @@ pub mod pallet { use sp_runtime::Perbill; /// The current storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(6); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(7); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] @@ -1602,6 +1649,14 @@ pub mod pallet { type MaxUnbonding: Get; } + /// The sum of funds across all pools. + /// + /// This might be lower but never higher than the sum of `total_balance` of all [`PoolMembers`] + /// because calling `pool_withdraw_unbonded` might decrease the total stake of the pool's + /// `bonded_account` without adjusting the pallet-internal `UnbondingPool`'s. + #[pallet::storage] + pub type TotalValueLocked = StorageValue<_, BalanceOf, ValueQuery>; + /// Minimum amount to bond to join a pool. #[pallet::storage] pub type MinJoinBond = StorageValue<_, BalanceOf, ValueQuery>; @@ -1825,9 +1880,9 @@ pub mod pallet { CannotWithdrawAny, /// The amount does not meet the minimum bond to either join or create a pool. /// - /// The depositor can never unbond to a value less than - /// `Pallet::depositor_min_bond`. The caller does not have nominating - /// permissions for the pool. Members can never unbond to a value below `MinJoinBond`. + /// The depositor can never unbond to a value less than `Pallet::depositor_min_bond`. The + /// caller does not have nominating permissions for the pool. Members can never unbond to a + /// value below `MinJoinBond`. MinimumBondNotMet, /// The transaction could not be executed due to overflow risk for the pool. OverflowRisk, @@ -2114,7 +2169,7 @@ pub mod pallet { /// Call `withdraw_unbonded` for the pools account. This call can be made by any account. /// - /// This is useful if their are too many unlocking chunks to call `unbond`, and some + /// This is useful if there are too many unlocking chunks to call `unbond`, and some /// can be cleared by withdrawing. In the case there are too many unlocking chunks, the user /// would probably see an error like `NoMoreChunks` emitted from the staking system when /// they attempt to unbond. @@ -2127,10 +2182,12 @@ pub mod pallet { ) -> DispatchResult { let _ = ensure_signed(origin)?; 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 // is destroying then `withdraw_unbonded` can be used. ensure!(pool.state != PoolState::Destroying, Error::::NotDestroying); - T::Staking::withdraw_unbonded(pool.bonded_account(), num_slashing_spans)?; + pool.withdraw_from_staking(num_slashing_spans)?; + Ok(()) } @@ -2180,9 +2237,8 @@ pub mod pallet { ensure!(!withdrawn_points.is_empty(), Error::::CannotWithdrawAny); // Before calculating the `balance_to_unbond`, we call withdraw unbonded to ensure the - // `transferable_balance` is correct. - let stash_killed = - T::Staking::withdraw_unbonded(bonded_pool.bonded_account(), num_slashing_spans)?; + // `transferrable_balance` is correct. + let stash_killed = bonded_pool.withdraw_from_staking(num_slashing_spans)?; // defensive-only: the depositor puts enough funds into the stash so that it will only // be destroyed when they are leaving. @@ -2846,12 +2902,9 @@ impl Pallet { }, (false, false) => { // Equivalent to (current_points / current_balance) * new_funds - balance( - u256(current_points) - .saturating_mul(u256(new_funds)) - // We check for zero above - .div(u256(current_balance)), - ) + balance(u256(current_points).saturating_mul(u256(new_funds))) + // We check for zero above + .div(current_balance) }, } } @@ -2871,9 +2924,12 @@ impl Pallet { } // Equivalent of (current_balance / current_points) * points - balance(u256(current_balance).saturating_mul(u256(points))) - // We check for zero above - .div(current_points) + balance( + u256(current_balance) + .saturating_mul(u256(points)) + // We check for zero above + .div(u256(current_points)), + ) } /// If the member has some rewards, transfer a payout from the reward pool to the member. @@ -3242,6 +3298,7 @@ impl Pallet { let mut pools_members = BTreeMap::::new(); let mut pools_members_pending_rewards = BTreeMap::>::new(); let mut all_members = 0u32; + let mut total_balance_members = Default::default(); PoolMembers::::iter().try_for_each(|(_, d)| -> Result<(), TryRuntimeError> { let bonded_pool = BondedPools::::get(d.pool_id).unwrap(); ensure!(!d.total_points().is_zero(), "No member should have zero points"); @@ -3257,6 +3314,7 @@ impl Pallet { let pending_rewards = d.pending_rewards(current_rc).unwrap(); *pools_members_pending_rewards.entry(d.pool_id).or_default() += pending_rewards; } // else this pool has been heavily slashed and cannot have any rewards anymore. + total_balance_members += d.total_balance(); Ok(()) })?; @@ -3280,6 +3338,7 @@ impl Pallet { Ok(()) })?; + let mut expected_tvl: BalanceOf = Default::default(); BondedPools::::iter().try_for_each(|(id, inner)| -> Result<(), TryRuntimeError> { let bonded_pool = BondedPool { id, inner }; ensure!( @@ -3300,13 +3359,28 @@ impl Pallet { "depositor must always have MinCreateBond stake in the pool, except for when the \ pool is being destroyed and the depositor is the last member", ); + + expected_tvl += + T::Staking::total_stake(&bonded_pool.bonded_account()).unwrap_or_default(); + Ok(()) })?; + ensure!( MaxPoolMembers::::get().map_or(true, |max| all_members <= max), Error::::MaxPoolMembers ); + ensure!( + TotalValueLocked::::get() == expected_tvl, + "TVL deviates from the actual sum of funds of all Pools." + ); + + ensure!( + TotalValueLocked::::get() <= total_balance_members, + "TVL must be equal to or less than the total balance of all PoolMembers." + ); + if level <= 1 { return Ok(()) } @@ -3424,20 +3498,30 @@ impl Pallet { } impl sp_staking::OnStakingUpdate> for Pallet { + /// Reduces the balances of the [`SubPools`], that belong to the pool involved in the + /// slash, to the amount that is defined in the `slashed_unlocking` field of + /// [`sp_staking::OnStakingUpdate::on_slash`] + /// + /// Emits the `PoolsSlashed` event. fn on_slash( pool_account: &T::AccountId, // Bonded balance is always read directly from staking, therefore we don't need to update // anything here. slashed_bonded: BalanceOf, slashed_unlocking: &BTreeMap>, + total_slashed: BalanceOf, ) { - if let Some(pool_id) = ReversePoolIdLookup::::get(pool_account) { - let mut sub_pools = match SubPoolsStorage::::get(pool_id).defensive() { - Some(sub_pools) => sub_pools, - None => return, - }; - for (era, slashed_balance) in slashed_unlocking.iter() { - if let Some(pool) = sub_pools.with_era.get_mut(era) { + let Some(pool_id) = ReversePoolIdLookup::::get(pool_account) else { return }; + // As the slashed account belongs to a `BondedPool` the `TotalValueLocked` decreases and + // an event is emitted. + TotalValueLocked::::mutate(|tvl| { + tvl.defensive_saturating_reduce(total_slashed); + }); + + if let Some(mut sub_pools) = SubPoolsStorage::::get(pool_id) { + // set the reduced balance for each of the `SubPools` + slashed_unlocking.iter().for_each(|(era, slashed_balance)| { + if let Some(pool) = sub_pools.with_era.get_mut(era).defensive() { pool.balance = *slashed_balance; Self::deposit_event(Event::::UnbondingPoolSlashed { era: *era, @@ -3445,10 +3529,11 @@ impl sp_staking::OnStakingUpdate> for Pall balance: *slashed_balance, }); } - } - - Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); + }); SubPoolsStorage::::insert(pool_id, sub_pools); + } else if !slashed_unlocking.is_empty() { + defensive!("Expected SubPools were not found"); } + Self::deposit_event(Event::::PoolSlashed { pool_id, balance: slashed_bonded }); } } diff --git a/substrate/frame/nomination-pools/src/migration.rs b/substrate/frame/nomination-pools/src/migration.rs index 606123daa9c6..eef2a976f1a2 100644 --- a/substrate/frame/nomination-pools/src/migration.rs +++ b/substrate/frame/nomination-pools/src/migration.rs @@ -27,6 +27,16 @@ use sp_runtime::TryRuntimeError; pub mod versioned_migrations { use super::*; + /// 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, @@ -37,6 +47,83 @@ pub mod versioned_migrations { >; } +/// 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. +mod v7 { + use super::*; + + pub struct VersionUncheckedMigrateV6ToV7(sp_std::marker::PhantomData); + impl VersionUncheckedMigrateV6ToV7 { + fn calculate_tvl_by_total_stake() -> BalanceOf { + BondedPools::::iter() + .map(|(id, inner)| { + T::Staking::total_stake( + &BondedPool { id, inner: inner.clone() }.bonded_account(), + ) + .unwrap_or_default() + }) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default() + } + } + + impl OnRuntimeUpgrade 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 = Self::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 = Self::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::*; diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 3884518a992b..d806ba071bd4 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -20,7 +20,7 @@ use crate::{self as pools}; use frame_support::{assert_ok, parameter_types, traits::fungible::Mutate, PalletId}; use frame_system::RawOrigin; use sp_runtime::{BuildStorage, FixedU128}; -use sp_staking::Stake; +use sp_staking::{OnStakingUpdate, Stake}; pub type BlockNumber = u64; pub type AccountId = u128; @@ -46,7 +46,8 @@ parameter_types! { pub static CurrentEra: EraIndex = 0; pub static BondingDuration: EraIndex = 3; pub storage BondedBalanceMap: BTreeMap = Default::default(); - pub storage UnbondingBalanceMap: BTreeMap = Default::default(); + // map from a user to a vec of eras and amounts being unlocked in each era. + pub storage UnbondingBalanceMap: BTreeMap> = Default::default(); #[derive(Clone, PartialEq)] pub static MaxUnbonding: u32 = 8; pub static StakingMinBond: Balance = 10; @@ -60,6 +61,19 @@ impl StakingMock { x.insert(who, bonded); BondedBalanceMap::set(&x) } + /// Mimics a slash towards a pool specified by `pool_id`. + /// This reduces the bonded balance of a pool by `amount` and calls [`Pools::on_slash`] to + /// enact changes in the nomination-pool pallet. + /// + /// Does not modify any [`SubPools`] of the pool as [`Default::default`] is passed for + /// `slashed_unlocking`. + pub fn slash_by(pool_id: PoolId, amount: Balance) { + let acc = Pools::create_bonded_account(pool_id); + let bonded = BondedBalanceMap::get(); + let pre_total = bonded.get(&acc).unwrap(); + Self::set_bonded_balance(acc, pre_total - amount); + Pools::on_slash(&acc, pre_total - amount, &Default::default(), amount); + } } impl sp_staking::StakingInterface for StakingMock { @@ -105,8 +119,11 @@ impl sp_staking::StakingInterface for StakingMock { let mut x = BondedBalanceMap::get(); *x.get_mut(who).unwrap() = x.get_mut(who).unwrap().saturating_sub(amount); BondedBalanceMap::set(&x); + + let era = Self::current_era(); + let unlocking_at = era + Self::bonding_duration(); let mut y = UnbondingBalanceMap::get(); - *y.entry(*who).or_insert(Self::Balance::zero()) += amount; + y.entry(*who).or_insert(Default::default()).push((unlocking_at, amount)); UnbondingBalanceMap::set(&y); Ok(()) } @@ -116,11 +133,13 @@ impl sp_staking::StakingInterface for StakingMock { } fn withdraw_unbonded(who: Self::AccountId, _: u32) -> Result { - // Simulates removing unlocking chunks and only having the bonded balance locked - let mut x = UnbondingBalanceMap::get(); - x.remove(&who); - UnbondingBalanceMap::set(&x); + let mut unbonding_map = UnbondingBalanceMap::get(); + let staker_map = unbonding_map.get_mut(&who).ok_or("Nothing to unbond")?; + + let current_era = Self::current_era(); + staker_map.retain(|(unlocking_at, _amount)| *unlocking_at > current_era); + UnbondingBalanceMap::set(&unbonding_map); Ok(UnbondingBalanceMap::get().is_empty() && BondedBalanceMap::get().is_empty()) } @@ -144,14 +163,17 @@ impl sp_staking::StakingInterface for StakingMock { } fn stake(who: &Self::AccountId) -> Result, DispatchError> { - match ( - UnbondingBalanceMap::get().get(who).copied(), - BondedBalanceMap::get().get(who).copied(), - ) { + match (UnbondingBalanceMap::get().get(who), BondedBalanceMap::get().get(who).copied()) { (None, None) => Err(DispatchError::Other("balance not found")), - (Some(v), None) => Ok(Stake { total: v, active: 0 }), + (Some(v), None) => Ok(Stake { + total: v.into_iter().fold(0u128, |acc, &x| acc.saturating_add(x.1)), + active: 0, + }), (None, Some(v)) => Ok(Stake { total: v, active: v }), - (Some(a), Some(b)) => Ok(Stake { total: a + b, active: b }), + (Some(a), Some(b)) => Ok(Stake { + total: a.into_iter().fold(0u128, |acc, &x| acc.saturating_add(x.1)) + b, + active: b, + }), } } diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 67183e25689e..2749e89ecff3 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -59,6 +59,9 @@ fn test_setup_works() { assert_eq!(StakingMock::bonding_duration(), 3); assert!(Metadata::::contains_key(1)); + // initial member. + assert_eq!(TotalValueLocked::::get(), 10); + let last_pool = LastPoolId::::get(); assert_eq!( BondedPool::::get(last_pool).unwrap(), @@ -218,10 +221,7 @@ mod bonded_pool { // slash half of the pool's balance. expected result of `fn api_points_to_balance` // to be 1/2 of the pool's balance. - StakingMock::set_bonded_balance( - default_bonded_account(), - Pools::depositor_min_bond() / 2, - ); + StakingMock::slash_by(1, Pools::depositor_min_bond() / 2); assert_eq!(Pallet::::api_points_to_balance(1, 10), 5); // if pool does not exist, points to balance ratio is 0. @@ -238,10 +238,7 @@ mod bonded_pool { // slash half of the pool's balance. expect result of `fn api_balance_to_points` // to be 2 * of the balance to add to the pool. - StakingMock::set_bonded_balance( - default_bonded_account(), - Pools::depositor_min_bond() / 2, - ); + StakingMock::slash_by(1, Pools::depositor_min_bond() / 2); assert_eq!(Pallet::::api_balance_to_points(1, 10), 20); // if pool does not exist, balance to points ratio is 0. @@ -637,12 +634,12 @@ mod join { // Given Currency::set_balance(&11, ExistentialDeposit::get() + 2); assert!(!PoolMembers::::contains_key(11)); + assert_eq!(TotalValueLocked::::get(), 10); // When assert_ok!(Pools::join(RuntimeOrigin::signed(11), 2, 1)); // Then - assert_eq!( pool_events_since_last_call(), vec![ @@ -651,6 +648,7 @@ mod join { Event::Bonded { member: 11, pool_id: 1, bonded: 2, joined: true }, ] ); + assert_eq!(TotalValueLocked::::get(), 12); assert_eq!( PoolMembers::::get(11).unwrap(), @@ -660,7 +658,7 @@ mod join { // Given // The bonded balance is slashed in half - StakingMock::set_bonded_balance(Pools::create_bonded_account(1), 6); + StakingMock::slash_by(1, 6); // And Currency::set_balance(&12, ExistentialDeposit::get() + 12); @@ -672,8 +670,12 @@ mod join { // Then assert_eq!( pool_events_since_last_call(), - vec![Event::Bonded { member: 12, pool_id: 1, bonded: 12, joined: true }] + vec![ + Event::PoolSlashed { pool_id: 1, balance: 6 }, + Event::Bonded { member: 12, pool_id: 1, bonded: 12, joined: true } + ] ); + assert_eq!(TotalValueLocked::::get(), 18); assert_eq!( PoolMembers::::get(12).unwrap(), @@ -2359,11 +2361,15 @@ mod unbond { .min_join_bond(10) .add_members(vec![(20, 20)]) .build_and_execute(|| { + assert_eq!(TotalValueLocked::::get(), 30); // can unbond to above limit assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 5)); assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 15); assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 5); + // tvl remains unchanged. + assert_eq!(TotalValueLocked::::get(), 30); + // cannot go to below 10: assert_noop!( Pools::unbond(RuntimeOrigin::signed(20), 20, 10), @@ -2669,8 +2675,9 @@ mod unbond { .add_members(vec![(40, 40), (550, 550)]) .build_and_execute(|| { let ed = Currency::minimum_balance(); - // Given a slash from 600 -> 100 - StakingMock::set_bonded_balance(default_bonded_account(), 100); + // Given a slash from 600 -> 500 + StakingMock::slash_by(1, 500); + // and unclaimed rewards of 600. Currency::set_balance(&default_reward_account(), ed + 600); @@ -2702,8 +2709,9 @@ mod unbond { Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, Event::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::PoolSlashed { pool_id: 1, balance: 100 }, Event::PaidOut { member: 40, pool_id: 1, payout: 40 }, - Event::Unbonded { member: 40, pool_id: 1, points: 6, balance: 6, era: 3 } + Event::Unbonded { member: 40, pool_id: 1, balance: 6, points: 6, era: 3 } ] ); @@ -2863,6 +2871,7 @@ mod unbond { ); // When the root kicks then its ok + // Account with ID 100 is kicked. assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(900), 100)); assert_eq!( @@ -2883,6 +2892,7 @@ mod unbond { ); // When the bouncer kicks then its ok + // Account with ID 200 is kicked. assert_ok!(Pools::fully_unbond(RuntimeOrigin::signed(902), 200)); assert_eq!( @@ -2921,7 +2931,7 @@ mod unbond { ); assert_eq!( *UnbondingBalanceMap::get().get(&default_bonded_account()).unwrap(), - 100 + 200 + vec![(3, 100), (3, 200)], ); }); } @@ -3020,7 +3030,10 @@ mod unbond { } ); assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); - assert_eq!(*UnbondingBalanceMap::get().get(&default_bonded_account()).unwrap(), 10); + assert_eq!( + *UnbondingBalanceMap::get().get(&default_bonded_account()).unwrap(), + vec![(6, 10)] + ); }); } @@ -3298,7 +3311,7 @@ mod unbond { assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 0); // slash the default pool - StakingMock::set_bonded_balance(Pools::create_bonded_account(1), 5); + StakingMock::slash_by(1, 5); // cannot unbond even 7, because the value of shares is now less. assert_noop!( @@ -3368,21 +3381,58 @@ mod pool_withdraw_unbonded { #[test] fn pool_withdraw_unbonded_works() { - ExtBuilder::default().build_and_execute(|| { - // Given 10 unbonded directly against the pool account - assert_ok!(StakingMock::unbond(&default_bonded_account(), 5)); - // and the pool account only has 10 balance - assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(5)); - assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(10)); - assert_eq!(Currency::free_balance(&default_bonded_account()), 10); + ExtBuilder::default().add_members(vec![(20, 10)]).build_and_execute(|| { + // Given 10 unbond'ed directly against the pool account + + assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 5)); + + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(20)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 20); // When + CurrentEra::set(StakingMock::current_era() + StakingMock::bonding_duration() + 1); assert_ok!(Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(10), 1, 0)); - // Then there unbonding balance is no longer locked - assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(5)); - assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(5)); - assert_eq!(Currency::free_balance(&default_bonded_account()), 10); + // Then their unbonding balance is no longer locked + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(15)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 20); + }); + } + #[test] + fn pool_withdraw_unbonded_creates_tvl_diff() { + ExtBuilder::default().add_members(vec![(20, 10)]).build_and_execute(|| { + // Given 10 unbond'ed directly against the pool account + assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 5)); + + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(20)); + assert_eq!(Balances::free_balance(&default_bonded_account()), 20); + assert_eq!(TotalValueLocked::::get(), 20); + + // When + CurrentEra::set(StakingMock::current_era() + StakingMock::bonding_duration() + 1); + assert_ok!(Pools::pool_withdraw_unbonded(RuntimeOrigin::signed(10), 1, 0)); + assert_eq!(TotalValueLocked::::get(), 15); + + let member_balance = PoolMembers::::iter() + .map(|(_, member)| member.total_balance()) + .reduce(|acc, total_balance| acc + total_balance) + .unwrap_or_default(); + + // Then their unbonding balance is no longer locked + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Ok(15)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(15)); + assert_eq!(Currency::free_balance(&default_bonded_account()), 20); + + // The difference between TVL and member_balance is exactly the difference between + // `total_stake` and the `free_balance`. + // This relation is not guaranteed in the wild as arbitrary transfers towards + // `free_balance` can be made to the pool that are not accounted for. + let non_locked_balance = Balances::free_balance(&default_bonded_account()) - + StakingMock::total_stake(&default_bonded_account()).unwrap(); + assert_eq!(member_balance, TotalValueLocked::::get() + non_locked_balance); }); } } @@ -3412,24 +3462,33 @@ mod withdraw_unbonded { let unbond_pool = sub_pools.with_era.get_mut(&3).unwrap(); // Sanity check assert_eq!(*unbond_pool, UnbondPool { points: 550 + 40, balance: 550 + 40 }); + assert_eq!(TotalValueLocked::::get(), 600); // Simulate a slash to the pool with_era(current_era), decreasing the balance by // half { unbond_pool.balance /= 2; // 295 SubPoolsStorage::::insert(1, sub_pools); + + // Adjust the TVL for this non-api usage (direct sub-pool modification) + TotalValueLocked::::mutate(|x| *x -= 295); + // Update the equivalent of the unbonding chunks for the `StakingMock` let mut x = UnbondingBalanceMap::get(); - *x.get_mut(&default_bonded_account()).unwrap() /= 5; + x.get_mut(&default_bonded_account()) + .unwrap() + .get_mut(current_era as usize) + .unwrap() + .1 /= 2; UnbondingBalanceMap::set(&x); + Currency::set_balance( &default_bonded_account(), Currency::free_balance(&default_bonded_account()) / 2, // 300 ); - StakingMock::set_bonded_balance( - default_bonded_account(), - StakingMock::active_stake(&default_bonded_account()).unwrap() / 2, - ); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 10); + StakingMock::slash_by(1, 5); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 5); }; // Advance the current_era to ensure all `with_era` pools will be merged into @@ -3465,6 +3524,7 @@ mod withdraw_unbonded { era: 3 }, Event::Unbonded { member: 40, pool_id: 1, points: 40, balance: 40, era: 3 }, + Event::PoolSlashed { pool_id: 1, balance: 5 } ] ); assert_eq!( @@ -3552,7 +3612,7 @@ mod withdraw_unbonded { // Given // current bond is 600, we slash it all to 300. - StakingMock::set_bonded_balance(default_bonded_account(), 300); + StakingMock::slash_by(1, 300); Currency::set_balance(&default_bonded_account(), 300); assert_eq!(StakingMock::total_stake(&default_bonded_account()), Ok(300)); @@ -3572,6 +3632,7 @@ mod withdraw_unbonded { Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, Event::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::PoolSlashed { pool_id: 1, balance: 300 }, Event::Unbonded { member: 40, pool_id: 1, balance: 20, points: 20, era: 3 }, Event::Unbonded { member: 550, @@ -4051,6 +4112,7 @@ mod withdraw_unbonded { #[test] fn full_multi_step_withdrawing_non_depositor() { ExtBuilder::default().add_members(vec![(100, 100)]).build_and_execute(|| { + assert_eq!(TotalValueLocked::::get(), 110); // given assert_ok!(Pools::unbond(RuntimeOrigin::signed(100), 100, 75)); assert_eq!( @@ -4058,6 +4120,9 @@ mod withdraw_unbonded { member_unbonding_eras!(3 => 75) ); + // tvl unchanged. + assert_eq!(TotalValueLocked::::get(), 110); + // progress one era and unbond the leftover. CurrentEra::set(1); assert_ok!(Pools::unbond(RuntimeOrigin::signed(100), 100, 25)); @@ -4070,6 +4135,8 @@ mod withdraw_unbonded { Pools::withdraw_unbonded(RuntimeOrigin::signed(100), 100, 0), Error::::CannotWithdrawAny ); + // tvl unchanged. + assert_eq!(TotalValueLocked::::get(), 110); // now the 75 should be free. CurrentEra::set(3); @@ -4089,6 +4156,8 @@ mod withdraw_unbonded { PoolMembers::::get(100).unwrap().unbonding_eras, member_unbonding_eras!(4 => 25) ); + // tvl updated + assert_eq!(TotalValueLocked::::get(), 35); // the 25 should be free now, and the member removed. CurrentEra::set(4); @@ -4398,6 +4467,7 @@ mod create { let next_pool_stash = Pools::create_bonded_account(2); let ed = Currency::minimum_balance(); + assert_eq!(TotalValueLocked::::get(), 10); assert!(!BondedPools::::contains_key(2)); assert!(!RewardPools::::contains_key(2)); assert!(!PoolMembers::::contains_key(11)); @@ -4411,6 +4481,7 @@ mod create { 456, 789 )); + assert_eq!(TotalValueLocked::::get(), 10 + StakingMock::minimum_nominator_bond()); assert_eq!(Currency::free_balance(&11), 0); assert_eq!( @@ -4701,9 +4772,10 @@ mod set_state { // Given unsafe_set_state(1, PoolState::Open); - let mut bonded_pool = BondedPool::::get(1).unwrap(); - bonded_pool.points = 100; - bonded_pool.put(); + // slash the pool to the point that `max_points_to_balance` ratio is + // surpassed. Making this pool destroyable by anyone. + StakingMock::slash_by(1, 10); + // When assert_ok!(Pools::set_state(RuntimeOrigin::signed(11), 1, PoolState::Destroying)); // Then @@ -4729,6 +4801,7 @@ mod set_state { pool_events_since_last_call(), vec![ Event::StateChanged { pool_id: 1, new_state: PoolState::Destroying }, + Event::PoolSlashed { pool_id: 1, balance: 0 }, Event::StateChanged { pool_id: 1, new_state: PoolState::Destroying }, Event::StateChanged { pool_id: 1, new_state: PoolState::Destroying } ] @@ -4927,8 +5000,10 @@ mod bond_extra { assert_eq!(PoolMembers::::get(10).unwrap().points, 10); assert_eq!(PoolMembers::::get(20).unwrap().points, 20); assert_eq!(BondedPools::::get(1).unwrap().points, 30); + assert_eq!(Currency::free_balance(&10), 35); assert_eq!(Currency::free_balance(&20), 20); + assert_eq!(TotalValueLocked::::get(), 30); // when assert_ok!(Pools::bond_extra(RuntimeOrigin::signed(10), BondExtra::Rewards)); @@ -4936,6 +5011,8 @@ mod bond_extra { // then assert_eq!(Currency::free_balance(&10), 35); + assert_eq!(TotalValueLocked::::get(), 31); + // 10's share of the reward is 1/3, since they gave 10/30 of the total shares. assert_eq!(PoolMembers::::get(10).unwrap().points, 10 + 1); assert_eq!(BondedPools::::get(1).unwrap().points, 30 + 1); @@ -4945,6 +5022,8 @@ mod bond_extra { // then assert_eq!(Currency::free_balance(&20), 20); + assert_eq!(TotalValueLocked::::get(), 33); + // 20's share of the rewards is the other 2/3 of the rewards, since they have 20/30 of // the shares assert_eq!(PoolMembers::::get(20).unwrap().points, 20 + 2); @@ -5354,7 +5433,7 @@ mod reward_counter_precision { ); // slash this pool by 99% of that. - StakingMock::set_bonded_balance(default_bonded_account(), DOT + pool_bond / 100); + StakingMock::slash_by(1, pool_bond * 99 / 100); // some whale now joins with the other half ot the total issuance. This will trigger an // overflow. This test is actually a bit too lenient because all the reward counters are @@ -6868,3 +6947,73 @@ mod commission { }) } } +mod slash { + use super::*; + + #[test] + fn slash_no_subpool_is_tracked() { + let bonded = |points, member_counter| BondedPool:: { + id: 1, + inner: BondedPoolInner { + commission: Commission::default(), + member_counter, + points, + roles: DEFAULT_ROLES, + state: PoolState::Open, + }, + }; + ExtBuilder::default().with_check(0).build_and_execute(|| { + // Given + Currency::set_balance(&11, ExistentialDeposit::get() + 2); + assert!(!PoolMembers::::contains_key(11)); + assert_eq!(TotalValueLocked::::get(), 10); + + // When + assert_ok!(Pools::join(RuntimeOrigin::signed(11), 2, 1)); + + // Then + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Created { depositor: 10, pool_id: 1 }, + Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, + Event::Bonded { member: 11, pool_id: 1, bonded: 2, joined: true }, + ] + ); + assert_eq!(TotalValueLocked::::get(), 12); + + assert_eq!( + PoolMembers::::get(11).unwrap(), + PoolMember:: { pool_id: 1, points: 2, ..Default::default() } + ); + assert_eq!(BondedPool::::get(1).unwrap(), bonded(12, 2)); + + // Given + // The bonded balance is slashed in half + StakingMock::slash_by(1, 6); + + // And + Currency::set_balance(&12, ExistentialDeposit::get() + 12); + assert!(!PoolMembers::::contains_key(12)); + + // When + assert_ok!(Pools::join(RuntimeOrigin::signed(12), 12, 1)); + + // Then + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::PoolSlashed { pool_id: 1, balance: 6 }, + Event::Bonded { member: 12, pool_id: 1, bonded: 12, joined: true } + ] + ); + assert_eq!(TotalValueLocked::::get(), 18); + + assert_eq!( + PoolMembers::::get(12).unwrap(), + PoolMember:: { pool_id: 1, points: 24, ..Default::default() } + ); + assert_eq!(BondedPool::::get(1).unwrap(), bonded(12 + 24, 3)); + }); + } +} diff --git a/substrate/frame/staking/src/lib.rs b/substrate/frame/staking/src/lib.rs index e59b2a3324a6..dcf57a464323 100644 --- a/substrate/frame/staking/src/lib.rs +++ b/substrate/frame/staking/src/lib.rs @@ -671,8 +671,14 @@ impl StakingLedger { // clean unlocking chunks that are set to zero. self.unlocking.retain(|c| !c.value.is_zero()); - T::EventListeners::on_slash(&self.stash, self.active, &slashed_unlocking); - pre_slash_total.saturating_sub(self.total) + let final_slashed_amount = pre_slash_total.saturating_sub(self.total); + T::EventListeners::on_slash( + &self.stash, + self.active, + &slashed_unlocking, + final_slashed_amount, + ); + final_slashed_amount } } diff --git a/substrate/frame/staking/src/mock.rs b/substrate/frame/staking/src/mock.rs index cf08f8be1f27..c41144278f2c 100644 --- a/substrate/frame/staking/src/mock.rs +++ b/substrate/frame/staking/src/mock.rs @@ -278,6 +278,7 @@ impl OnStakingUpdate for EventListenerMock { _pool_account: &AccountId, slashed_bonded: Balance, slashed_chunks: &BTreeMap, + _total_slashed: Balance, ) { LedgerSlashPerEra::set((slashed_bonded, slashed_chunks.clone())); } diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 1621af164b37..8b5797d7918f 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -121,10 +121,12 @@ pub trait OnStakingUpdate { /// * `slashed_active` - The new bonded balance of the staker after the slash was applied. /// * `slashed_unlocking` - A map of slashed eras, and the balance of that unlocking chunk after /// the slash is applied. Any era not present in the map is not affected at all. + /// * `slashed_total` - The aggregated balance that was lost due to the slash. fn on_slash( _stash: &AccountId, _slashed_active: Balance, _slashed_unlocking: &BTreeMap, + _slashed_total: Balance, ) { } }