diff --git a/frame/bags-list/fuzzer/src/main.rs b/frame/bags-list/fuzzer/src/main.rs index 387c266d32256..d0586f0372ac4 100644 --- a/frame/bags-list/fuzzer/src/main.rs +++ b/frame/bags-list/fuzzer/src/main.rs @@ -64,19 +64,19 @@ fn main() { Action::Insert => { if BagsList::on_insert(id, vote_weight).is_err() { // this was a duplicate id, which is ok. We can just update it. - BagsList::on_update(&id, vote_weight); + BagsList::on_update(&id, vote_weight).unwrap(); } assert!(BagsList::contains(&id)); }, Action::Update => { let already_contains = BagsList::contains(&id); - BagsList::on_update(&id, vote_weight); + BagsList::on_update(&id, vote_weight).unwrap(); if already_contains { assert!(BagsList::contains(&id)); } }, Action::Remove => { - BagsList::on_remove(&id); + BagsList::on_remove(&id).unwrap(); assert!(!BagsList::contains(&id)); }, } diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 53ccd5e4c19f1..77fe609ad8123 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -194,12 +194,14 @@ pub mod pallet { #[pallet::error] #[cfg_attr(test, derive(PartialEq))] pub enum Error { - /// Attempted to place node in front of a node in another bag. - NotInSameBag, - /// Id not found in list. - IdNotFound, - /// An Id does not have a greater score than another Id. - NotHeavier, + /// A error in the list interface implementation. + List(ListError), + } + + impl From for Error { + fn from(t: ListError) -> Self { + Error::::List(t) + } } #[pallet::call] @@ -231,7 +233,9 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::put_in_front_of())] pub fn put_in_front_of(origin: OriginFor, lighter: T::AccountId) -> DispatchResult { let heavier = ensure_signed(origin)?; - List::::put_in_front_of(&lighter, &heavier).map_err(Into::into) + List::::put_in_front_of(&lighter, &heavier) + .map_err::, _>(Into::into) + .map_err::(Into::into) } } @@ -250,16 +254,18 @@ pub mod pallet { impl, I: 'static> Pallet { /// Move an account from one bag to another, depositing an event on success. /// - /// If the account changed bags, returns `Some((from, to))`. - pub fn do_rebag(account: &T::AccountId, new_weight: T::Score) -> Option<(T::Score, T::Score)> { - // if no voter at that node, don't do anything. - // the caller just wasted the fee to call this. - let maybe_movement = list::Node::::get(account) - .and_then(|node| List::update_position_for(node, new_weight)); + /// If the account changed bags, returns `Ok(Some((from, to)))`. + pub fn do_rebag( + account: &T::AccountId, + new_score: T::Score, + ) -> Result, ListError> { + // If no voter at that node, don't do anything. the caller just wasted the fee to call this. + let node = list::Node::::get(&account).ok_or(ListError::NodeNotFound)?; + let maybe_movement = List::update_position_for(node, new_score); if let Some((from, to)) = maybe_movement { Self::deposit_event(Event::::Rebagged { who: account.clone(), from, to }); }; - maybe_movement + Ok(maybe_movement) } /// Equivalent to `ListBags::get`, but public. Useful for tests in outside of this crate. @@ -296,11 +302,11 @@ impl, I: 'static> SortedListProvider for Pallet List::::insert(id, score) } - fn on_update(id: &T::AccountId, new_score: T::Score) { - Pallet::::do_rebag(id, new_score); + fn on_update(id: &T::AccountId, new_score: T::Score) -> Result<(), ListError> { + Pallet::::do_rebag(id, new_score).map(|_| ()) } - fn on_remove(id: &T::AccountId) { + fn on_remove(id: &T::AccountId) -> Result<(), ListError> { List::::remove(id) } diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 4de70569253da..f93bfca1081e9 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -27,7 +27,10 @@ use crate::Config; use codec::{Decode, Encode, MaxEncodedLen}; use frame_election_provider_support::ScoreProvider; -use frame_support::{traits::Get, DefaultNoBound}; +use frame_support::{ + traits::{Defensive, Get}, + DefaultNoBound, PalletError, +}; use scale_info::TypeInfo; use sp_runtime::traits::{Bounded, Zero}; use sp_std::{ @@ -38,10 +41,14 @@ use sp_std::{ vec::Vec, }; -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Encode, Decode, MaxEncodedLen, TypeInfo, PalletError)] pub enum ListError { /// A duplicate id has been detected. Duplicate, + /// An Id does not have a greater score than another Id. + NotHeavier, + /// Attempted to place node in front of a node in another bag. + NotInSameBag, /// Given node id was not found. NodeNotFound, } @@ -321,9 +328,13 @@ impl, I: 'static> List { Ok(()) } - /// Remove an id from the list. - pub(crate) fn remove(id: &T::AccountId) { - Self::remove_many(sp_std::iter::once(id)); + /// Remove an id from the list, returning an error if `id` does not exists. + pub(crate) fn remove(id: &T::AccountId) -> Result<(), ListError> { + if !Self::contains(id) { + return Err(ListError::NodeNotFound) + } + let _ = Self::remove_many(sp_std::iter::once(id)); + Ok(()) } /// Remove many ids from the list. @@ -416,31 +427,31 @@ impl, I: 'static> List { pub(crate) fn put_in_front_of( lighter_id: &T::AccountId, heavier_id: &T::AccountId, - ) -> Result<(), crate::pallet::Error> { - use crate::pallet; + ) -> Result<(), ListError> { use frame_support::ensure; - let lighter_node = Node::::get(lighter_id).ok_or(pallet::Error::IdNotFound)?; - let heavier_node = Node::::get(heavier_id).ok_or(pallet::Error::IdNotFound)?; + let lighter_node = Node::::get(&lighter_id).ok_or(ListError::NodeNotFound)?; + let heavier_node = Node::::get(&heavier_id).ok_or(ListError::NodeNotFound)?; - ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag); + ensure!(lighter_node.bag_upper == heavier_node.bag_upper, ListError::NotInSameBag); // this is the most expensive check, so we do it last. ensure!( - T::ScoreProvider::score(heavier_id) > T::ScoreProvider::score(lighter_id), - pallet::Error::NotHeavier + T::ScoreProvider::score(&heavier_id) > T::ScoreProvider::score(&lighter_id), + ListError::NotHeavier ); // remove the heavier node from this list. Note that this removes the node from storage and // decrements the node counter. - Self::remove(heavier_id); + // defensive: both nodes have been checked to exist. + let _ = Self::remove(&heavier_id).defensive(); // re-fetch `lighter_node` from storage since it may have been updated when `heavier_node` // was removed. let lighter_node = Node::::get(lighter_id).ok_or_else(|| { debug_assert!(false, "id that should exist cannot be found"); crate::log!(warn, "id that should exist cannot be found"); - pallet::Error::IdNotFound + ListError::NodeNotFound })?; // insert `heavier_node` directly in front of `lighter_node`. This will update both nodes diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index c8e233f1e62cc..ff7dd2871c237 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -21,7 +21,7 @@ use crate::{ ListBags, ListNodes, }; use frame_election_provider_support::{SortedListProvider, VoteWeight}; -use frame_support::{assert_ok, assert_storage_noop}; +use frame_support::{assert_noop, assert_ok, assert_storage_noop}; #[test] fn basic_setup_works() { @@ -98,7 +98,7 @@ fn remove_last_node_in_bags_cleans_bag() { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // bump 1 to a bigger bag - List::::remove(&1); + List::::remove(&1).unwrap(); assert_ok!(List::::insert(1, 10_000)); // then the bag with bound 10 is wiped from storage. @@ -265,10 +265,10 @@ mod list { ExtBuilder::default().build_and_execute(|| { // removing a non-existent id is a noop assert!(!ListNodes::::contains_key(42)); - assert_storage_noop!(List::::remove(&42)); + assert_noop!(List::::remove(&42), ListError::NodeNotFound); // when removing a node from a bag with multiple nodes: - List::::remove(&2); + List::::remove(&2).unwrap(); // then assert_eq!(get_list_as_ids(), vec![3, 4, 1]); @@ -276,7 +276,7 @@ mod list { ensure_left(2, 3); // when removing a node from a bag with only one node: - List::::remove(&1); + List::::remove(&1).unwrap(); // then assert_eq!(get_list_as_ids(), vec![3, 4]); @@ -286,11 +286,11 @@ mod list { assert!(!ListBags::::contains_key(10)); // remove remaining ids to make sure storage cleans up as expected - List::::remove(&3); + List::::remove(&3).unwrap(); ensure_left(3, 1); assert_eq!(get_list_as_ids(), vec![4]); - List::::remove(&4); + List::::remove(&4).unwrap(); ensure_left(4, 0); assert_eq!(get_list_as_ids(), Vec::::new()); @@ -573,7 +573,7 @@ mod bags { }); // when we make a pre-existing bag empty - List::::remove(&1); + List::::remove(&1).unwrap(); // then assert_eq!(Bag::::get(10), None) diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index 941623229dc27..c63ab26d59a67 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -34,7 +34,7 @@ mod pallet { vec![(10, vec![1]), (20, vec![42]), (1_000, vec![2, 3, 4])] ); - // when increasing vote weight to the level of non-existent bag + // when increasing score to the level of non-existent bag StakingMock::set_score_of(&42, 2_000); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); @@ -44,7 +44,7 @@ mod pallet { vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (2_000, vec![42])] ); - // when decreasing weight within the range of the current bag + // when decreasing score within the range of the current bag StakingMock::set_score_of(&42, 1_001); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); @@ -54,7 +54,7 @@ mod pallet { vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (2_000, vec![42])] ); - // when reducing weight to the level of a non-existent bag + // when reducing score to the level of a non-existent bag StakingMock::set_score_of(&42, 30); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); @@ -64,7 +64,7 @@ mod pallet { vec![(10, vec![1]), (30, vec![42]), (1_000, vec![2, 3, 4])] ); - // when increasing weight to the level of a pre-existing bag + // when increasing score to the level of a pre-existing bag StakingMock::set_score_of(&42, 500); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); @@ -380,7 +380,7 @@ mod pallet { // then assert_noop!( BagsList::put_in_front_of(Origin::signed(3), 2), - crate::pallet::Error::::NotHeavier + crate::pallet::Error::::List(ListError::NotHeavier) ); }); } @@ -394,7 +394,7 @@ mod pallet { // then assert_noop!( BagsList::put_in_front_of(Origin::signed(3), 4), - crate::pallet::Error::::NotHeavier + crate::pallet::Error::::List(ListError::NotHeavier) ); }); } @@ -411,7 +411,7 @@ mod pallet { // then assert_noop!( BagsList::put_in_front_of(Origin::signed(5), 4), - crate::pallet::Error::::IdNotFound + crate::pallet::Error::::List(ListError::NodeNotFound) ); }); @@ -425,7 +425,7 @@ mod pallet { // then assert_noop!( BagsList::put_in_front_of(Origin::signed(4), 5), - crate::pallet::Error::::IdNotFound + crate::pallet::Error::::List(ListError::NodeNotFound) ); }); } @@ -439,7 +439,7 @@ mod pallet { // then assert_noop!( BagsList::put_in_front_of(Origin::signed(4), 1), - crate::pallet::Error::::NotInSameBag + crate::pallet::Error::::List(ListError::NotInSameBag) ); }); } @@ -491,12 +491,12 @@ mod sorted_list_provider { assert_eq!(BagsList::count(), 5); // when removing - BagsList::on_remove(&201); + BagsList::on_remove(&201).unwrap(); // then the count goes down assert_eq!(BagsList::count(), 4); // when updating - BagsList::on_update(&201, VoteWeight::MAX); + assert_noop!(BagsList::on_update(&201, VoteWeight::MAX), ListError::NodeNotFound); // then the count stays the same assert_eq!(BagsList::count(), 4); }); @@ -554,8 +554,8 @@ mod sorted_list_provider { ); assert_eq!(BagsList::count(), 5); - // when increasing weight to the level of non-existent bag - BagsList::on_update(&42, 2_000); + // when increasing score to the level of non-existent bag + BagsList::on_update(&42, 2_000).unwrap(); // then the bag is created with the id in it, assert_eq!( @@ -565,8 +565,8 @@ mod sorted_list_provider { // and the id position is updated in the list. assert_eq!(BagsList::iter().collect::>(), vec![42, 2, 3, 4, 1]); - // when decreasing weight within the range of the current bag - BagsList::on_update(&42, 1_001); + // when decreasing score within the range of the current bag + BagsList::on_update(&42, 1_001).unwrap(); // then the id does not change bags, assert_eq!( @@ -576,8 +576,8 @@ mod sorted_list_provider { // or change position in the list. assert_eq!(BagsList::iter().collect::>(), vec![42, 2, 3, 4, 1]); - // when increasing weight to the level of a non-existent bag with the max threshold - BagsList::on_update(&42, VoteWeight::MAX); + // when increasing score to the level of a non-existent bag with the max threshold + BagsList::on_update(&42, VoteWeight::MAX).unwrap(); // the the new bag is created with the id in it, assert_eq!( @@ -587,8 +587,8 @@ mod sorted_list_provider { // and the id position is updated in the list. assert_eq!(BagsList::iter().collect::>(), vec![42, 2, 3, 4, 1]); - // when decreasing the weight to a pre-existing bag - BagsList::on_update(&42, 1_000); + // when decreasing the score to a pre-existing bag + BagsList::on_update(&42, 1_000).unwrap(); // then id is moved to the correct bag (as the last member), assert_eq!( @@ -615,10 +615,10 @@ mod sorted_list_provider { ExtBuilder::default().build_and_execute(|| { // it is a noop removing a non-existent id assert!(!ListNodes::::contains_key(42)); - assert_storage_noop!(BagsList::on_remove(&42)); + assert_noop!(BagsList::on_remove(&42), ListError::NodeNotFound); // when removing a node from a bag with multiple nodes - BagsList::on_remove(&2); + BagsList::on_remove(&2).unwrap(); // then assert_eq!(get_list_as_ids(), vec![3, 4, 1]); @@ -626,7 +626,7 @@ mod sorted_list_provider { ensure_left(2, 3); // when removing a node from a bag with only one node - BagsList::on_remove(&1); + BagsList::on_remove(&1).unwrap(); // then assert_eq!(get_list_as_ids(), vec![3, 4]); @@ -634,10 +634,10 @@ mod sorted_list_provider { ensure_left(1, 2); // when removing all remaining ids - BagsList::on_remove(&4); + BagsList::on_remove(&4).unwrap(); assert_eq!(get_list_as_ids(), vec![3]); ensure_left(4, 1); - BagsList::on_remove(&3); + BagsList::on_remove(&3).unwrap(); // then the storage is completely cleaned up assert_eq!(get_list_as_ids(), Vec::::new()); diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 69bd3025fa768..af366d5b8f919 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -171,7 +171,7 @@ pub mod traits; #[cfg(feature = "std")] use codec::{Decode, Encode}; use frame_support::{weights::Weight, BoundedVec, RuntimeDebug}; -use sp_runtime::traits::Bounded; +use sp_runtime::traits::{Bounded, Saturating, Zero}; use sp_std::{fmt::Debug, prelude::*}; /// Re-export the solution generation macro. @@ -439,7 +439,7 @@ pub trait SortedListProvider { type Error: sp_std::fmt::Debug; /// The type used by the list to compare nodes for ordering. - type Score: Bounded; + type Score: Bounded + Saturating + Zero; /// An iterator over the list, which can have `take` called on it. fn iter() -> Box>; @@ -456,13 +456,21 @@ pub trait SortedListProvider { fn contains(id: &AccountId) -> bool; /// Hook for inserting a new id. + /// + /// Implementation should return an error if duplicate item is being inserted. fn on_insert(id: AccountId, score: Self::Score) -> Result<(), Self::Error>; /// Hook for updating a single id. - fn on_update(id: &AccountId, score: Self::Score); + /// + /// The `new` score is given. + /// + /// Returns `Ok(())` iff it successfully updates an item, an `Err(_)` otherwise. + fn on_update(id: &AccountId, score: Self::Score) -> Result<(), Self::Error>; /// Hook for removing am id from the list. - fn on_remove(id: &AccountId); + /// + /// Returns `Ok(())` iff it successfully removes an item, an `Err(_)` otherwise. + fn on_remove(id: &AccountId) -> Result<(), Self::Error>; /// Regenerate this list from scratch. Returns the count of items inserted. /// diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 0ac61e21fb323..34d7a176670f2 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -803,7 +803,7 @@ impl Pallet { pub fn do_remove_nominator(who: &T::AccountId) -> bool { let outcome = if Nominators::::contains_key(who) { Nominators::::remove(who); - T::VoterList::on_remove(who); + let _ = T::VoterList::on_remove(who).defensive(); true } else { false @@ -850,7 +850,7 @@ impl Pallet { pub fn do_remove_validator(who: &T::AccountId) -> bool { let outcome = if Validators::::contains_key(who) { Validators::::remove(who); - T::VoterList::on_remove(who); + let _ = T::VoterList::on_remove(who).defensive(); true } else { false @@ -1343,11 +1343,13 @@ impl SortedListProvider for UseNominatorsAndValidatorsM // nothing to do on insert. Ok(()) } - fn on_update(_: &T::AccountId, _weight: Self::Score) { + fn on_update(_: &T::AccountId, _weight: VoteWeight) -> Result<(), Self::Error> { // nothing to do on update. + Ok(()) } - fn on_remove(_: &T::AccountId) { + fn on_remove(_: &T::AccountId) -> Result<(), Self::Error> { // nothing to do on remove. + Ok(()) } fn unsafe_regenerate( _: impl IntoIterator, diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 117bc6e7c15bb..e53464195de23 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -22,8 +22,8 @@ use frame_support::{ dispatch::Codec, pallet_prelude::*, traits::{ - Currency, CurrencyToVote, DefensiveSaturating, EnsureOrigin, EstimateNextNewSession, Get, - LockIdentifier, LockableCurrency, OnUnbalanced, UnixTime, + Currency, CurrencyToVote, Defensive, DefensiveSaturating, EnsureOrigin, + EstimateNextNewSession, Get, LockIdentifier, LockableCurrency, OnUnbalanced, UnixTime, }, weights::Weight, }; @@ -858,7 +858,8 @@ pub mod pallet { Self::update_ledger(&controller, &ledger); // update this staker in the sorted list, if they exist in it. if T::VoterList::contains(&stash) { - T::VoterList::on_update(&stash, Self::weight_of(&ledger.stash)); + let _ = + T::VoterList::on_update(&stash, Self::weight_of(&ledger.stash)).defensive(); debug_assert_eq!(T::VoterList::sanity_check(), Ok(())); } @@ -941,7 +942,8 @@ pub mod pallet { // update this staker in the sorted list, if they exist in it. if T::VoterList::contains(&ledger.stash) { - T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash)); + let _ = T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash)) + .defensive(); } Self::deposit_event(Event::::Unbonded(ledger.stash, value)); @@ -1426,7 +1428,8 @@ pub mod pallet { // NOTE: ledger must be updated prior to calling `Self::weight_of`. Self::update_ledger(&controller, &ledger); if T::VoterList::contains(&ledger.stash) { - T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash)); + let _ = T::VoterList::on_update(&ledger.stash, Self::weight_of(&ledger.stash)) + .defensive(); } let removed_chunks = 1u32 // for the case where the last iterated chunk is not removed