Update Pallet Bags List to use ListError

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

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)
 	}
 

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

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,18 +265,18 @@ 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]);
 			assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 4])]);
 			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)