Make bags-list generic over node value and instantiable (paritytech#10997)

* make instantiable

* update

* cargo fmt

* Clean up

* bags-list: Make it generic over node value

* Respond to some feedback

* Apply suggestions from code review

Co-authored-by: Kian Paimani <[email protected]>

* Add back default impl for weight update worst case

* Update to Score in more places'

* Use VoteWeight, not u64 to reduce test diff

* FMT

* FullCodec implies Codec

* formatting

* Fixup bags list remote test

Co-authored-by: doordashcon <[email protected]>
Co-authored-by: Doordashcon <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>

Author: 3 people

bin/node/runtime/src/lib.rs

@@ -690,9 +690,10 @@ parameter_types! {
 
 impl pallet_bags_list::Config for Runtime {
 	type Event = Event;
-	type VoteWeightProvider = Staking;
+	type ScoreProvider = Staking;
 	type WeightInfo = pallet_bags_list::weights::SubstrateWeight<Runtime>;
 	type BagThresholds = BagThresholds;
+	type Score = sp_npos_elections::VoteWeight;
 }
 
 parameter_types! {

frame/bags-list/remote-tests/src/lib.rs

@@ -17,6 +17,7 @@
 
 //! Utilities for remote-testing pallet-bags-list.
 
+use frame_election_provider_support::ScoreProvider;
 use sp_std::prelude::*;
 
 /// A common log target to use.
@@ -55,8 +56,12 @@ pub fn display_and_check_bags<Runtime: RuntimeT>(currency_unit: u64, currency_na
 	let mut rebaggable = 0;
 	let mut active_bags = 0;
 	for vote_weight_thresh in <Runtime as pallet_bags_list::Config>::BagThresholds::get() {
+		let vote_weight_thresh_u64: u64 = (*vote_weight_thresh)
+			.try_into()
+			.map_err(|_| "runtime must configure score to at most u64 to use this test")
+			.unwrap();
 		// threshold in terms of UNITS (e.g. KSM, DOT etc)
-		let vote_weight_thresh_as_unit = *vote_weight_thresh as f64 / currency_unit as f64;
+		let vote_weight_thresh_as_unit = vote_weight_thresh_u64 as f64 / currency_unit as f64;
 		let pretty_thresh = format!("Threshold: {}. {}", vote_weight_thresh_as_unit, currency_name);
 
 		let bag = match pallet_bags_list::Pallet::<Runtime>::list_bags_get(*vote_weight_thresh) {
@@ -70,9 +75,13 @@ pub fn display_and_check_bags<Runtime: RuntimeT>(currency_unit: u64, currency_na
 		active_bags += 1;
 
 		for id in bag.std_iter().map(|node| node.std_id().clone()) {
-			let vote_weight = pallet_staking::Pallet::<Runtime>::weight_of(&id);
+			let vote_weight = <Runtime as pallet_bags_list::Config>::ScoreProvider::score(&id);
+			let vote_weight_thresh_u64: u64 = (*vote_weight_thresh)
+				.try_into()
+				.map_err(|_| "runtime must configure score to at most u64 to use this test")
+				.unwrap();
 			let vote_weight_as_balance: pallet_staking::BalanceOf<Runtime> =
-				vote_weight.try_into().map_err(|_| "can't convert").unwrap();
+				vote_weight_thresh_u64.try_into().map_err(|_| "can't convert").unwrap();
 
 			if vote_weight_as_balance < min_nominator_bond {
 				log::trace!(
@@ -87,13 +96,17 @@ pub fn display_and_check_bags<Runtime: RuntimeT>(currency_unit: u64, currency_na
 				pallet_bags_list::Node::<Runtime>::get(&id).expect("node in bag must exist.");
 			if node.is_misplaced(vote_weight) {
 				rebaggable += 1;
+				let notional_bag = pallet_bags_list::notional_bag_for::<Runtime, _>(vote_weight);
+				let notional_bag_as_u64: u64 = notional_bag
+					.try_into()
+					.map_err(|_| "runtime must configure score to at most u64 to use this test")
+					.unwrap();
 				log::trace!(
 					target: LOG_TARGET,
 					"Account {:?} can be rebagged from {:?} to {:?}",
 					id,
 					vote_weight_thresh_as_unit,
-					pallet_bags_list::notional_bag_for::<Runtime>(vote_weight) as f64 /
-						currency_unit as f64
+					notional_bag_as_u64 as f64 / currency_unit as f64
 				);
 			}
 		}

frame/bags-list/src/benchmarks.rs

@@ -20,9 +20,10 @@
 use super::*;
 use crate::list::List;
 use frame_benchmarking::{account, whitelist_account, whitelisted_caller};
-use frame_election_provider_support::VoteWeightProvider;
+use frame_election_provider_support::ScoreProvider;
 use frame_support::{assert_ok, traits::Get};
 use frame_system::RawOrigin as SystemOrigin;
+use sp_runtime::traits::One;
 
 frame_benchmarking::benchmarks! {
 	rebag_non_terminal {
@@ -36,29 +37,29 @@ frame_benchmarking::benchmarks! {
 
 		// clear any pre-existing storage.
 		// NOTE: safe to call outside block production
-		List::<T>::unsafe_clear();
+		List::<T, _>::unsafe_clear();
 
 		// define our origin and destination thresholds.
 		let origin_bag_thresh = T::BagThresholds::get()[0];
 		let dest_bag_thresh = T::BagThresholds::get()[1];
 
 		// seed items in the origin bag.
 		let origin_head: T::AccountId = account("origin_head", 0, 0);
-		assert_ok!(List::<T>::insert(origin_head.clone(), origin_bag_thresh));
+		assert_ok!(List::<T, _>::insert(origin_head.clone(), origin_bag_thresh));
 
 		let origin_middle: T::AccountId = account("origin_middle", 0, 0); // the node we rebag (_R_)
-		assert_ok!(List::<T>::insert(origin_middle.clone(), origin_bag_thresh));
+		assert_ok!(List::<T, _>::insert(origin_middle.clone(), origin_bag_thresh));
 
 		let origin_tail: T::AccountId  = account("origin_tail", 0, 0);
-		assert_ok!(List::<T>::insert(origin_tail.clone(), origin_bag_thresh));
+		assert_ok!(List::<T, _>::insert(origin_tail.clone(), origin_bag_thresh));
 
 		// seed items in the destination bag.
 		let dest_head: T::AccountId  = account("dest_head", 0, 0);
-		assert_ok!(List::<T>::insert(dest_head.clone(), dest_bag_thresh));
+		assert_ok!(List::<T, _>::insert(dest_head.clone(), dest_bag_thresh));
 
 		// the bags are in the expected state after initial setup.
 		assert_eq!(
-			List::<T>::get_bags(),
+			List::<T, _>::get_bags(),
 			vec![
 				(origin_bag_thresh, vec![origin_head.clone(), origin_middle.clone(), origin_tail.clone()]),
 				(dest_bag_thresh, vec![dest_head.clone()])
@@ -67,12 +68,12 @@ frame_benchmarking::benchmarks! {
 
 		let caller = whitelisted_caller();
 		// update the weight of `origin_middle` to guarantee it will be rebagged into the destination.
-		T::VoteWeightProvider::set_vote_weight_of(&origin_middle, dest_bag_thresh);
+		T::ScoreProvider::set_score_of(&origin_middle, dest_bag_thresh);
 	}: rebag(SystemOrigin::Signed(caller), origin_middle.clone())
 	verify {
 		// check the bags have updated as expected.
 		assert_eq!(
-			List::<T>::get_bags(),
+			List::<T, _>::get_bags(),
 			vec![
 				(
 					origin_bag_thresh,
@@ -104,18 +105,18 @@ frame_benchmarking::benchmarks! {
 
 		// seed items in the origin bag.
 		let origin_head: T::AccountId = account("origin_head", 0, 0);
-		assert_ok!(List::<T>::insert(origin_head.clone(), origin_bag_thresh));
+		assert_ok!(List::<T, _>::insert(origin_head.clone(), origin_bag_thresh));
 
 		let origin_tail: T::AccountId  = account("origin_tail", 0, 0); // the node we rebag (_R_)
-		assert_ok!(List::<T>::insert(origin_tail.clone(), origin_bag_thresh));
+		assert_ok!(List::<T, _>::insert(origin_tail.clone(), origin_bag_thresh));
 
 		// seed items in the destination bag.
 		let dest_head: T::AccountId  = account("dest_head", 0, 0);
-		assert_ok!(List::<T>::insert(dest_head.clone(), dest_bag_thresh));
+		assert_ok!(List::<T, _>::insert(dest_head.clone(), dest_bag_thresh));
 
 		// the bags are in the expected state after initial setup.
 		assert_eq!(
-			List::<T>::get_bags(),
+			List::<T, _>::get_bags(),
 			vec![
 				(origin_bag_thresh, vec![origin_head.clone(), origin_tail.clone()]),
 				(dest_bag_thresh, vec![dest_head.clone()])
@@ -124,12 +125,12 @@ frame_benchmarking::benchmarks! {
 
 		let caller = whitelisted_caller();
 		// update the weight of `origin_tail` to guarantee it will be rebagged into the destination.
-		T::VoteWeightProvider::set_vote_weight_of(&origin_tail, dest_bag_thresh);
+		T::ScoreProvider::set_score_of(&origin_tail, dest_bag_thresh);
 	}: rebag(SystemOrigin::Signed(caller), origin_tail.clone())
 	verify {
 		// check the bags have updated as expected.
 		assert_eq!(
-			List::<T>::get_bags(),
+			List::<T, _>::get_bags(),
 			vec![
 				(origin_bag_thresh, vec![origin_head.clone()]),
 				(dest_bag_thresh, vec![dest_head.clone(), origin_tail.clone()])
@@ -147,30 +148,30 @@ frame_benchmarking::benchmarks! {
 
 		// insert the nodes in order
 		let lighter: T::AccountId = account("lighter", 0, 0);
-		assert_ok!(List::<T>::insert(lighter.clone(), bag_thresh));
+		assert_ok!(List::<T, _>::insert(lighter.clone(), bag_thresh));
 
 		let heavier_prev: T::AccountId = account("heavier_prev", 0, 0);
-		assert_ok!(List::<T>::insert(heavier_prev.clone(), bag_thresh));
+		assert_ok!(List::<T, _>::insert(heavier_prev.clone(), bag_thresh));
 
 		let heavier: T::AccountId = account("heavier", 0, 0);
-		assert_ok!(List::<T>::insert(heavier.clone(), bag_thresh));
+		assert_ok!(List::<T, _>::insert(heavier.clone(), bag_thresh));
 
 		let heavier_next: T::AccountId = account("heavier_next", 0, 0);
-		assert_ok!(List::<T>::insert(heavier_next.clone(), bag_thresh));
+		assert_ok!(List::<T, _>::insert(heavier_next.clone(), bag_thresh));
 
-		T::VoteWeightProvider::set_vote_weight_of(&lighter, bag_thresh - 1);
-		T::VoteWeightProvider::set_vote_weight_of(&heavier, bag_thresh);
+		T::ScoreProvider::set_score_of(&lighter, bag_thresh - One::one());
+		T::ScoreProvider::set_score_of(&heavier, bag_thresh);
 
 		assert_eq!(
-			List::<T>::iter().map(|n| n.id().clone()).collect::<Vec<_>>(),
+			List::<T, _>::iter().map(|n| n.id().clone()).collect::<Vec<_>>(),
 			vec![lighter.clone(), heavier_prev.clone(), heavier.clone(), heavier_next.clone()]
 		);
 
 		whitelist_account!(heavier);
 	}: _(SystemOrigin::Signed(heavier.clone()), lighter.clone())
 	verify {
 		assert_eq!(
-			List::<T>::iter().map(|n| n.id().clone()).collect::<Vec<_>>(),
+			List::<T, _>::iter().map(|n| n.id().clone()).collect::<Vec<_>>(),
 			vec![heavier, lighter, heavier_prev, heavier_next]
 		)
 	}