diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 6d6187c9dce6f..64a8eef6396df 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -127,7 +127,7 @@ impl ListScenario { // Find a destination weight that will trigger the worst case scenario let dest_weight_as_vote = - ::SortedListProvider::weight_update_worst_case( + ::SortedListProvider::score_update_worst_case( &pool_origin1, is_increase, ); diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index 6dfc93f1a790b..9f0c7b09bf121 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -122,7 +122,8 @@ impl pallet_bags_list::Config for Runtime { type Event = Event; type WeightInfo = (); type BagThresholds = BagThresholds; - type VoteWeightProvider = Staking; + type ScoreProvider = Staking; + type Score = VoteWeight; } pub struct BalanceToU256; diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 714920ae4693d..04c029be03721 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -422,7 +422,6 @@ impl sp_std::ops::Deref for BondedPool { } } -// TODO: ask a rust guy if this is a bad thing to do. impl sp_std::ops::DerefMut for BondedPool { fn deref_mut(&mut self) -> &mut Self::Target { &mut self.inner @@ -688,7 +687,6 @@ impl BondedPool { let points_issued = self.issue(amount); match ty { - // TODO: Consider making StakingInterface use reference. PoolBond::Create => T::StakingInterface::bond( self.bonded_account(), // We make the stash and controller the same for simplicity @@ -715,8 +713,8 @@ impl BondedPool { n } - // Set the state of `self`, and deposit an event if the state changed. State should never be set directly in - // in order to ensure a state change event is always correctly deposited. + // Set the state of `self`, and deposit an event if the state changed. State should never be set + // directly in in order to ensure a state change event is always correctly deposited. fn set_state(&mut self, state: PoolState) { if self.state != state { self.state = state; @@ -804,52 +802,30 @@ pub struct SubPools { impl SubPools { /// Merge the oldest `with_era` unbond pools into the `no_era` unbond pool. - // TODO: Consider not consuming self. + /// + /// This is often used whilst getting the sub-pool from storage, thus it consumes and returns + /// `Self` for ergonomic purposes. fn maybe_merge_pools(mut self, unbond_era: EraIndex) -> Self { - // TODO: remove - if unbond_era < TotalUnbondingPools::::get().into() { - // For the first `0..TotalUnbondingPools` eras of the chain we don't need to do - // anything. Ex: if `TotalUnbondingPools` is 5 and we are in era 4 we can add a pool - // for this era and have exactly `TotalUnbondingPools` pools. - return self - } - // Ex: if `TotalUnbondingPools` is 5 and current era is 10, we only want to retain pools - // 6..=10. - let newest_era_to_remove = unbond_era.saturating_sub(TotalUnbondingPools::::get()); - - // TODO: eras to keep, one filter, tweak self.no_era whilst filtering. - let eras_to_remove: Vec<_> = self - .with_era - .keys() - .cloned() - .filter(|era| *era <= newest_era_to_remove) - .collect(); - for era in eras_to_remove { - if let Some(p) = self.with_era.remove(&era).defensive() { - self.no_era.points = self.no_era.points.saturating_add(p.points); - self.no_era.balance = self.no_era.balance.saturating_add(p.balance); - } + // 6..=10. Note that in the first few eras where `checked_sub` is `None`, we don't remove + // anything. + if let Some(newest_era_to_remove) = unbond_era.checked_sub(TotalUnbondingPools::::get()) + { + self.with_era.retain(|k, v| { + if *k > newest_era_to_remove { + // keep + true + } else { + // merge into the no-era pool + self.no_era.points = self.no_era.points.saturating_add(v.points); + self.no_era.balance = self.no_era.balance.saturating_add(v.balance); + false + } + }); } self } - - /// Get the unbond pool for `era`. If one does not exist a default entry will be inserted. - /// - /// The caller must ensure that the `SubPools::with_era` has room for 1 more entry. Calling - /// [`SubPools::maybe_merge_pools`] with the current era should the sub pools are in an ok state - /// to call this method. - // TODO: dissolve and move to call site. - fn unchecked_with_era_get_or_make(&mut self, era: EraIndex) -> &mut UnbondPool { - if !self.with_era.contains_key(&era) { - self.with_era - .try_insert(era, UnbondPool::default()) - .expect("caller has checked pre-conditions. qed."); - } - - self.with_era.get_mut(&era).expect("entry inserted on the line above. qed.") - } } /// The maximum amount of eras an unbonding pool can exist prior to being merged with the @@ -1065,6 +1041,8 @@ pub mod pallet { DoesNotHavePermission, /// Metadata exceeds [`T::MaxMetadataLen`] MetadataExceedsMaxLen, + /// Some error occurred that should never happen. This should be reported to the maintainers. + DefensiveError, } #[pallet::call] @@ -1194,18 +1172,29 @@ pub mod pallet { // Unbond in the actual underlying pool T::StakingInterface::unbond(bonded_pool.bonded_account(), balance_to_unbond)?; - // Note that we lazily create the unbonding pools here if they don't already exist - let sub_pools = SubPoolsStorage::::get(delegator.pool_id).unwrap_or_default(); let current_era = T::StakingInterface::current_era(); let unbond_era = T::StakingInterface::bonding_duration().saturating_add(current_era); - - // Merge any older pools into the general, era agnostic unbond pool. Note that we do - // this before inserting to ensure we don't go over the max unbonding pools. - let mut sub_pools = sub_pools.maybe_merge_pools(unbond_era); + // Note that we lazily create the unbonding pools here if they don't already exist + let mut sub_pools = SubPoolsStorage::::get(delegator.pool_id) + .unwrap_or_default() + .maybe_merge_pools(unbond_era); // Update the unbond pool associated with the current era with the unbonded funds. Note // that we lazily create the unbond pool if it does not yet exist. - sub_pools.unchecked_with_era_get_or_make(unbond_era).issue(balance_to_unbond); + if !sub_pools.with_era.contains_key(&unbond_era) { + sub_pools + .with_era + .try_insert(unbond_era, UnbondPool::default()) + // The above call to `maybe_merge_pools` should ensure there is + // always enough space to insert. + .defensive_map_err(|_| Error::::DefensiveError)?; + } + sub_pools + .with_era + .get_mut(&unbond_era) + // The above check ensures the pool exists. + .defensive_ok_or_else(|| Error::::DefensiveError)? + .issue(balance_to_unbond); delegator.unbonding_era = Some(unbond_era); @@ -1466,9 +1455,6 @@ 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); - // TODO: [now] we could check if bonded_pool.ok_to_be_open().is_err(), and if thats - // true always set the state to destroying, regardless of the stat the caller passes. - // The downside is that this seems like a misleading API if bonded_pool.can_toggle_state(&who) { bonded_pool.set_state(state); @@ -1574,8 +1560,8 @@ impl Pallet { /// Create the reward account of a pool with the given id. pub fn create_reward_account(id: PoolId) -> T::AccountId { - // NOTE: in order to have a distinction in the test account id type (u128), we put account_type first so - // it does not get truncated out. + // NOTE: in order to have a distinction in the test account id type (u128), we put + // account_type first so it does not get truncated out. T::PalletId::get().into_sub_account((AccountType::Reward, id)) } @@ -1696,7 +1682,6 @@ impl Pallet { /// If the delegator has some rewards, transfer a payout from the reward pool to the delegator. /// /// # Note - // TODO: revise this. /// This will persist updates for the reward pool to storage. But it will *not* persist updates /// to the `delegator` or `bonded_pool` to storage, that is the responsibility of the caller. fn do_reward_payout( @@ -1719,16 +1704,6 @@ impl Pallet { ExistenceRequirement::AllowDeath, )?; - if reward_pool.total_earnings == BalanceOf::::max_value() && - bonded_pool.state != PoolState::Destroying - { - bonded_pool.state = PoolState::Destroying; - Self::deposit_event(Event::::State { - pool_id: delegator.pool_id, - new_state: PoolState::Destroying, - }); - } - Self::deposit_event(Event::::PaidOut { delegator: delegator_account, pool_id: delegator.pool_id, diff --git a/frame/support/src/storage/bounded_btree_map.rs b/frame/support/src/storage/bounded_btree_map.rs index ed132adac657e..9d0463d658036 100644 --- a/frame/support/src/storage/bounded_btree_map.rs +++ b/frame/support/src/storage/bounded_btree_map.rs @@ -77,6 +77,14 @@ where Self(t, Default::default()) } + /// Exactly the same semantics as `BTreeMap::retain`. + /// + /// The is a safe `&mut self` borrow because `retain` can only ever decrease the length of the + /// inner map. + pub fn retain bool>(&mut self, f: F) { + self.0.retain(f) + } + /// Create a new `BoundedBTreeMap`. /// /// Does not allocate.