Update Pallet Bags List to use ListError (#11342)

* extract list error changes from kiz-revamp-sorted-list-providers-2-approval-stake

* some fixes

* weight -> score

* Update tests.rs

* Update tests.rs

* more fixes

* remove score updated event

* Update frame/bags-list/src/lib.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

substrate/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));
 				},
 			}

substrate/frame/bags-list/src/lib.rs

@@ -194,12 +194,14 @@ pub mod pallet {
 	#[pallet::error]
 	#[cfg_attr(test, derive(PartialEq))]
 	pub enum Error<T, I = ()> {
-		/// 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<T, I> From<ListError> for Error<T, I> {
+		fn from(t: ListError) -> Self {
+			Error::<T, I>::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<T>, lighter: T::AccountId) -> DispatchResult {
 			let heavier = ensure_signed(origin)?;
-			List::<T, I>::put_in_front_of(&lighter, &heavier).map_err(Into::into)
+			List::<T, I>::put_in_front_of(&lighter, &heavier)
+				.map_err::<Error<T, I>, _>(Into::into)
+				.map_err::<DispatchError, _>(Into::into)
 		}
 	}
 
@@ -250,16 +254,18 @@ pub mod pallet {
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
 	/// 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::<T, I>::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<Option<(T::Score, T::Score)>, ListError> {
+		// If no voter at that node, don't do anything. the caller just wasted the fee to call this.
+		let node = list::Node::<T, I>::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::<T, I>::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<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
 		List::<T, I>::insert(id, score)
 	}
 
-	fn on_update(id: &T::AccountId, new_score: T::Score) {
-		Pallet::<T, I>::do_rebag(id, new_score);
+	fn on_update(id: &T::AccountId, new_score: T::Score) -> Result<(), ListError> {
+		Pallet::<T, I>::do_rebag(id, new_score).map(|_| ())
 	}
 
-	fn on_remove(id: &T::AccountId) {
+	fn on_remove(id: &T::AccountId) -> Result<(), ListError> {
 		List::<T, I>::remove(id)
 	}
 

substrate/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<T: Config<I>, I: 'static> List<T, I> {
 		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<T: Config<I>, I: 'static> List<T, I> {
 	pub(crate) fn put_in_front_of(
 		lighter_id: &T::AccountId,
 		heavier_id: &T::AccountId,
-	) -> Result<(), crate::pallet::Error<T, I>> {
-		use crate::pallet;
+	) -> Result<(), ListError> {
 		use frame_support::ensure;
 
-		let lighter_node = Node::<T, I>::get(lighter_id).ok_or(pallet::Error::IdNotFound)?;
-		let heavier_node = Node::<T, I>::get(heavier_id).ok_or(pallet::Error::IdNotFound)?;
+		let lighter_node = Node::<T, I>::get(&lighter_id).ok_or(ListError::NodeNotFound)?;
+		let heavier_node = Node::<T, I>::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::<T, I>::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

substrate/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::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]);
 
 		// bump 1 to a bigger bag
-		List::<Runtime>::remove(&1);
+		List::<Runtime>::remove(&1).unwrap();
 		assert_ok!(List::<Runtime>::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::<Runtime>::contains_key(42));
-			assert_storage_noop!(List::<Runtime>::remove(&42));
+			assert_noop!(List::<Runtime>::remove(&42), ListError::NodeNotFound);
 
 			// when removing a node from a bag with multiple nodes:
-			List::<Runtime>::remove(&2);
+			List::<Runtime>::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::<Runtime>::remove(&1);
+			List::<Runtime>::remove(&1).unwrap();
 
 			// then
 			assert_eq!(get_list_as_ids(), vec![3, 4]);
@@ -286,11 +286,11 @@ mod list {
 			assert!(!ListBags::<Runtime>::contains_key(10));
 
 			// remove remaining ids to make sure storage cleans up as expected
-			List::<Runtime>::remove(&3);
+			List::<Runtime>::remove(&3).unwrap();
 			ensure_left(3, 1);
 			assert_eq!(get_list_as_ids(), vec![4]);
 
-			List::<Runtime>::remove(&4);
+			List::<Runtime>::remove(&4).unwrap();
 			ensure_left(4, 0);
 			assert_eq!(get_list_as_ids(), Vec::<AccountId>::new());
 
@@ -573,7 +573,7 @@ mod bags {
 				});
 
 			// when we make a pre-existing bag empty
-			List::<Runtime>::remove(&1);
+			List::<Runtime>::remove(&1).unwrap();
 
 			// then
 			assert_eq!(Bag::<Runtime>::get(10), None)

substrate/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::<Runtime>::NotHeavier
+				crate::pallet::Error::<Runtime>::List(ListError::NotHeavier)
 			);
 		});
 	}
@@ -394,7 +394,7 @@ mod pallet {
 			// then
 			assert_noop!(
 				BagsList::put_in_front_of(Origin::signed(3), 4),
-				crate::pallet::Error::<Runtime>::NotHeavier
+				crate::pallet::Error::<Runtime>::List(ListError::NotHeavier)
 			);
 		});
 	}
@@ -411,7 +411,7 @@ mod pallet {
 			// then
 			assert_noop!(
 				BagsList::put_in_front_of(Origin::signed(5), 4),
-				crate::pallet::Error::<Runtime>::IdNotFound
+				crate::pallet::Error::<Runtime>::List(ListError::NodeNotFound)
 			);
 		});
 
@@ -425,7 +425,7 @@ mod pallet {
 			// then
 			assert_noop!(
 				BagsList::put_in_front_of(Origin::signed(4), 5),
-				crate::pallet::Error::<Runtime>::IdNotFound
+				crate::pallet::Error::<Runtime>::List(ListError::NodeNotFound)
 			);
 		});
 	}
@@ -439,7 +439,7 @@ mod pallet {
 			// then
 			assert_noop!(
 				BagsList::put_in_front_of(Origin::signed(4), 1),
-				crate::pallet::Error::<Runtime>::NotInSameBag
+				crate::pallet::Error::<Runtime>::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<_>>(), 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<_>>(), 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<_>>(), 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::<Runtime>::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::<AccountId>::new());

substrate/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<AccountId> {
 	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<dyn Iterator<Item = AccountId>>;
@@ -456,13 +456,21 @@ pub trait SortedListProvider<AccountId> {
 	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.
 	///

substrate/frame/staking/src/pallet/impls.rs

@@ -803,7 +803,7 @@ impl<T: Config> Pallet<T> {
 	pub fn do_remove_nominator(who: &T::AccountId) -> bool {
 		let outcome = if Nominators::<T>::contains_key(who) {
 			Nominators::<T>::remove(who);
-			T::VoterList::on_remove(who);
+			let _ = T::VoterList::on_remove(who).defensive();
 			true
 		} else {
 			false
@@ -850,7 +850,7 @@ impl<T: Config> Pallet<T> {
 	pub fn do_remove_validator(who: &T::AccountId) -> bool {
 		let outcome = if Validators::<T>::contains_key(who) {
 			Validators::<T>::remove(who);
-			T::VoterList::on_remove(who);
+			let _ = T::VoterList::on_remove(who).defensive();
 			true
 		} else {
 			false
@@ -1343,11 +1343,13 @@ impl<T: Config> SortedListProvider<T::AccountId> 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<Item = T::AccountId>,

substrate/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::<T>::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