From 5c48b57af767109920d0a87359cc5d021a9b1c2d Mon Sep 17 00:00:00 2001 From: doordashcon Date: Wed, 2 Mar 2022 00:41:01 +0100 Subject: [PATCH 01/14] make instantiable --- frame/bags-list/src/benchmarks.rs | 36 +++++----- frame/bags-list/src/lib.rs | 54 +++++++------- frame/bags-list/src/list/mod.rs | 114 +++++++++++++++--------------- frame/bags-list/src/mock.rs | 2 +- 4 files changed, 103 insertions(+), 103 deletions(-) diff --git a/frame/bags-list/src/benchmarks.rs b/frame/bags-list/src/benchmarks.rs index cc575d7d1efff..396f0071821a9 100644 --- a/frame/bags-list/src/benchmarks.rs +++ b/frame/bags-list/src/benchmarks.rs @@ -36,7 +36,7 @@ frame_benchmarking::benchmarks! { // clear any pre-existing storage. // NOTE: safe to call outside block production - List::::unsafe_clear(); + List::::unsafe_clear(); // define our origin and destination thresholds. let origin_bag_thresh = T::BagThresholds::get()[0]; @@ -44,21 +44,21 @@ frame_benchmarking::benchmarks! { // seed items in the origin bag. let origin_head: T::AccountId = account("origin_head", 0, 0); - assert_ok!(List::::insert(origin_head.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(origin_middle.clone(), origin_bag_thresh)); + assert_ok!(List::::insert(origin_middle.clone(), origin_bag_thresh)); let origin_tail: T::AccountId = account("origin_tail", 0, 0); - assert_ok!(List::::insert(origin_tail.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(dest_head.clone(), dest_bag_thresh)); + assert_ok!(List::::insert(dest_head.clone(), dest_bag_thresh)); // the bags are in the expected state after initial setup. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ (origin_bag_thresh, vec![origin_head.clone(), origin_middle.clone(), origin_tail.clone()]), (dest_bag_thresh, vec![dest_head.clone()]) @@ -72,7 +72,7 @@ frame_benchmarking::benchmarks! { verify { // check the bags have updated as expected. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ ( origin_bag_thresh, @@ -104,18 +104,18 @@ frame_benchmarking::benchmarks! { // seed items in the origin bag. let origin_head: T::AccountId = account("origin_head", 0, 0); - assert_ok!(List::::insert(origin_head.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(origin_tail.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(dest_head.clone(), dest_bag_thresh)); + assert_ok!(List::::insert(dest_head.clone(), dest_bag_thresh)); // the bags are in the expected state after initial setup. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ (origin_bag_thresh, vec![origin_head.clone(), origin_tail.clone()]), (dest_bag_thresh, vec![dest_head.clone()]) @@ -129,7 +129,7 @@ frame_benchmarking::benchmarks! { verify { // check the bags have updated as expected. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ (origin_bag_thresh, vec![origin_head.clone()]), (dest_bag_thresh, vec![dest_head.clone(), origin_tail.clone()]) @@ -147,22 +147,22 @@ frame_benchmarking::benchmarks! { // insert the nodes in order let lighter: T::AccountId = account("lighter", 0, 0); - assert_ok!(List::::insert(lighter.clone(), bag_thresh)); + assert_ok!(List::::insert(lighter.clone(), bag_thresh)); let heavier_prev: T::AccountId = account("heavier_prev", 0, 0); - assert_ok!(List::::insert(heavier_prev.clone(), bag_thresh)); + assert_ok!(List::::insert(heavier_prev.clone(), bag_thresh)); let heavier: T::AccountId = account("heavier", 0, 0); - assert_ok!(List::::insert(heavier.clone(), bag_thresh)); + assert_ok!(List::::insert(heavier.clone(), bag_thresh)); let heavier_next: T::AccountId = account("heavier_next", 0, 0); - assert_ok!(List::::insert(heavier_next.clone(), bag_thresh)); + assert_ok!(List::::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); assert_eq!( - List::::iter().map(|n| n.id().clone()).collect::>(), + List::::iter().map(|n| n.id().clone()).collect::>(), vec![lighter.clone(), heavier_prev.clone(), heavier.clone(), heavier_next.clone()] ); @@ -170,7 +170,7 @@ frame_benchmarking::benchmarks! { }: _(SystemOrigin::Signed(heavier.clone()), lighter.clone()) verify { assert_eq!( - List::::iter().map(|n| n.id().clone()).collect::>(), + List::::iter().map(|n| n.id().clone()).collect::>(), vec![heavier, lighter, heavier_prev, heavier_next] ) } diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 89c54db87023f..d0384367f5ded 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -92,12 +92,12 @@ pub mod pallet { #[pallet::pallet] #[pallet::generate_store(pub(crate) trait Store)] - pub struct Pallet(_); + pub struct Pallet(_); #[pallet::config] - pub trait Config: frame_system::Config { + pub trait Config: frame_system::Config { /// The overarching event type. - type Event: From> + IsType<::Event>; + type Event: From> + IsType<::Event>; /// Weight information for extrinsics in this pallet. type WeightInfo: weights::WeightInfo; @@ -156,25 +156,25 @@ pub mod pallet { /// /// Nodes store links forward and back within their respective bags. #[pallet::storage] - pub(crate) type ListNodes = - CountedStorageMap<_, Twox64Concat, T::AccountId, list::Node>; + pub(crate) type ListNodes, I: 'static = ()> = + CountedStorageMap<_, Twox64Concat, T::AccountId, list::Node>; /// A bag stored in storage. /// /// Stores a `Bag` struct, which stores head and tail pointers to itself. #[pallet::storage] - pub(crate) type ListBags = StorageMap<_, Twox64Concat, VoteWeight, list::Bag>; + pub(crate) type ListBags, I: 'static = ()> = StorageMap<_, Twox64Concat, VoteWeight, list::Bag>; #[pallet::event] #[pallet::generate_deposit(pub(crate) fn deposit_event)] - pub enum Event { + pub enum Event, I: 'static = ()> { /// Moved an account from one bag to another. Rebagged { who: T::AccountId, from: VoteWeight, to: VoteWeight }, } #[pallet::error] #[cfg_attr(test, derive(PartialEq))] - pub enum Error { + pub enum Error { /// Attempted to place node in front of a node in another bag. NotInSameBag, /// Id not found in list. @@ -184,7 +184,7 @@ pub mod pallet { } #[pallet::call] - impl Pallet { + impl, I: 'static> Pallet { /// Declare that some `dislocated` account has, through rewards or penalties, sufficiently /// changed its weight that it should properly fall into a different bag than its current /// one. @@ -197,7 +197,7 @@ pub mod pallet { pub fn rebag(origin: OriginFor, dislocated: T::AccountId) -> DispatchResult { ensure_signed(origin)?; let current_weight = T::VoteWeightProvider::vote_weight(&dislocated); - let _ = Pallet::::do_rebag(&dislocated, current_weight); + let _ = Pallet::::do_rebag(&dislocated, current_weight); Ok(()) } @@ -212,12 +212,12 @@ 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) } } #[pallet::hooks] - impl Hooks> for Pallet { + impl, I: 'static> Hooks> for Pallet { fn integrity_test() { // ensure they are strictly increasing, this also implies that duplicates are detected. assert!( @@ -228,7 +228,7 @@ pub mod pallet { } } -impl 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))`. @@ -238,46 +238,46 @@ impl Pallet { ) -> Option<(VoteWeight, VoteWeight)> { // 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) + let maybe_movement = list::Node::::get(&account) .and_then(|node| List::update_position_for(node, new_weight)); if let Some((from, to)) = maybe_movement { - Self::deposit_event(Event::::Rebagged { who: account.clone(), from, to }); + Self::deposit_event(Event::::Rebagged { who: account.clone(), from, to }); }; maybe_movement } /// Equivalent to `ListBags::get`, but public. Useful for tests in outside of this crate. #[cfg(feature = "std")] - pub fn list_bags_get(weight: VoteWeight) -> Option> { + pub fn list_bags_get(weight: VoteWeight) -> Option> { ListBags::get(weight) } } -impl SortedListProvider for Pallet { +impl, I: 'static> SortedListProvider for Pallet { type Error = Error; fn iter() -> Box> { - Box::new(List::::iter().map(|n| n.id().clone())) + Box::new(List::::iter().map(|n| n.id().clone())) } fn count() -> u32 { - ListNodes::::count() + ListNodes::::count() } fn contains(id: &T::AccountId) -> bool { - List::::contains(id) + List::::contains(id) } fn on_insert(id: T::AccountId, weight: VoteWeight) -> Result<(), Error> { - List::::insert(id, weight) + List::::insert(id, weight) } fn on_update(id: &T::AccountId, new_weight: VoteWeight) { - Pallet::::do_rebag(id, new_weight); + Pallet::::do_rebag(id, new_weight); } fn on_remove(id: &T::AccountId) { - List::::remove(id) + List::::remove(id) } fn unsafe_regenerate( @@ -287,12 +287,12 @@ impl SortedListProvider for Pallet { // NOTE: This call is unsafe for the same reason as SortedListProvider::unsafe_regenerate. // I.e. because it can lead to many storage accesses. // So it is ok to call it as caller must ensure the conditions. - List::::unsafe_regenerate(all, weight_of) + List::::unsafe_regenerate(all, weight_of) } #[cfg(feature = "std")] fn sanity_check() -> Result<(), &'static str> { - List::::sanity_check() + List::::sanity_check() } #[cfg(not(feature = "std"))] @@ -304,14 +304,14 @@ impl SortedListProvider for Pallet { // NOTE: This call is unsafe for the same reason as SortedListProvider::unsafe_clear. // I.e. because it can lead to many storage accesses. // So it is ok to call it as caller must ensure the conditions. - List::::unsafe_clear() + List::::unsafe_clear() } #[cfg(feature = "runtime-benchmarks")] fn weight_update_worst_case(who: &T::AccountId, is_increase: bool) -> VoteWeight { use frame_support::traits::Get as _; let thresholds = T::BagThresholds::get(); - let node = list::Node::::get(who).unwrap(); + let node = list::Node::::get(who).unwrap(); let current_bag_idx = thresholds .iter() .chain(sp_std::iter::once(&VoteWeight::MAX)) diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 8cd89b8fff1cc..a6da25ae6be16 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -53,7 +53,7 @@ mod tests; /// /// Note that even if the thresholds list does not have `VoteWeight::MAX` as its final member, this /// function behaves as if it does. -pub fn notional_bag_for(weight: VoteWeight) -> VoteWeight { +pub fn notional_bag_for, I: 'static>(weight: VoteWeight) -> VoteWeight { let thresholds = T::BagThresholds::get(); let idx = thresholds.partition_point(|&threshold| weight > threshold); thresholds.get(idx).copied().unwrap_or(VoteWeight::MAX) @@ -73,9 +73,9 @@ pub fn notional_bag_for(weight: VoteWeight) -> VoteWeight { // weight decreases as successive bags are reached. This means that it is valid to truncate // iteration at any desired point; only those ids in the lowest bag can be excluded. This // satisfies both the desire for fairness and the requirement for efficiency. -pub struct List(PhantomData); +pub struct List, I: 'static = ()>(PhantomData); -impl List { +impl, I: 'static> List { /// Remove all data associated with the list from storage. /// /// ## WARNING @@ -83,8 +83,8 @@ impl List { /// this function should generally not be used in production as it could lead to a very large /// number of storage accesses. pub(crate) fn unsafe_clear() { - crate::ListBags::::remove_all(None); - crate::ListNodes::::remove_all(); + crate::ListBags::::remove_all(None); + crate::ListNodes::::remove_all(); } /// Regenerate all of the data from the given ids. @@ -135,11 +135,11 @@ impl List { // we can't check all preconditions, but we can check one debug_assert!( - crate::ListBags::::iter().all(|(threshold, _)| old_thresholds.contains(&threshold)), + crate::ListBags::::iter().all(|(threshold, _)| old_thresholds.contains(&threshold)), "not all `bag_upper` currently in storage are members of `old_thresholds`", ); debug_assert!( - crate::ListNodes::::iter().all(|(_, node)| old_thresholds.contains(&node.bag_upper)), + crate::ListNodes::::iter().all(|(_, node)| old_thresholds.contains(&node.bag_upper)), "not all `node.bag_upper` currently in storage are members of `old_thresholds`", ); @@ -166,7 +166,7 @@ impl List { continue } - if let Some(bag) = Bag::::get(affected_bag) { + if let Some(bag) = Bag::::get(affected_bag) { affected_accounts.extend(bag.iter().map(|node| node.id)); } } @@ -178,7 +178,7 @@ impl List { continue } - if let Some(bag) = Bag::::get(removed_bag) { + if let Some(bag) = Bag::::get(removed_bag) { affected_accounts.extend(bag.iter().map(|node| node.id)); } } @@ -199,10 +199,10 @@ impl List { // lookups. for removed_bag in removed_bags { debug_assert!( - !crate::ListNodes::::iter().any(|(_, node)| node.bag_upper == removed_bag), + !crate::ListNodes::::iter().any(|(_, node)| node.bag_upper == removed_bag), "no id should be present in a removed bag", ); - crate::ListBags::::remove(removed_bag); + crate::ListBags::::remove(removed_bag); } debug_assert_eq!(Self::sanity_check(), Ok(())); @@ -212,14 +212,14 @@ impl List { /// Returns `true` if the list contains `id`, otherwise returns `false`. pub(crate) fn contains(id: &T::AccountId) -> bool { - crate::ListNodes::::contains_key(id) + crate::ListNodes::::contains_key(id) } /// Iterate over all nodes in all bags in the list. /// /// Full iteration can be expensive; it's recommended to limit the number of items with /// `.take(n)`. - pub(crate) fn iter() -> impl Iterator> { + pub(crate) fn iter() -> impl Iterator> { // We need a touch of special handling here: because we permit `T::BagThresholds` to // omit the final bound, we need to ensure that we explicitly include that threshold in the // list. @@ -266,8 +266,8 @@ impl List { return Err(Error::Duplicate) } - let bag_weight = notional_bag_for::(weight); - let mut bag = Bag::::get_or_make(bag_weight); + let bag_weight = notional_bag_for::(weight); + let mut bag = Bag::::get_or_make(bag_weight); // unchecked insertion is okay; we just got the correct `notional_bag_for`. bag.insert_unchecked(id.clone()); @@ -280,7 +280,7 @@ impl List { id, weight, bag_weight, - crate::ListNodes::::count(), + crate::ListNodes::::count(), ); Ok(()) @@ -301,7 +301,7 @@ impl List { let mut count = 0; for id in ids.into_iter() { - let node = match Node::::get(id) { + let node = match Node::::get(id) { Some(node) => node, None => continue, }; @@ -314,7 +314,7 @@ impl List { // this node is a head or tail, so the bag needs to be updated let bag = bags .entry(node.bag_upper) - .or_insert_with(|| Bag::::get_or_make(node.bag_upper)); + .or_insert_with(|| Bag::::get_or_make(node.bag_upper)); // node.bag_upper must be correct, therefore this bag will contain this node. bag.remove_node_unchecked(&node); } @@ -341,7 +341,7 @@ impl List { /// [`self.insert`]. However, given large quantities of nodes to move, it may be more efficient /// to call [`self.remove_many`] followed by [`self.insert_many`]. pub(crate) fn update_position_for( - node: Node, + node: Node, new_weight: VoteWeight, ) -> Option<(VoteWeight, VoteWeight)> { node.is_misplaced(new_weight).then(move || { @@ -351,7 +351,7 @@ impl List { // this node is not a head or a tail, so we can just cut it out of the list. update // and put the prev and next of this node, we do `node.put` inside `insert_note`. node.excise(); - } else if let Some(mut bag) = Bag::::get(node.bag_upper) { + } else if let Some(mut bag) = Bag::::get(node.bag_upper) { // this is a head or tail, so the bag must be updated. bag.remove_node_unchecked(&node); bag.put(); @@ -365,8 +365,8 @@ impl List { } // put the node into the appropriate new bag. - let new_bag_upper = notional_bag_for::(new_weight); - let mut bag = Bag::::get_or_make(new_bag_upper); + let new_bag_upper = notional_bag_for::(new_weight); + let mut bag = Bag::::get_or_make(new_bag_upper); // prev, next, and bag_upper of the node are updated inside `insert_node`, also // `node.put` is in there. bag.insert_node_unchecked(node); @@ -381,12 +381,12 @@ impl List { pub(crate) fn put_in_front_of( lighter_id: &T::AccountId, heavier_id: &T::AccountId, - ) -> Result<(), crate::pallet::Error> { + ) -> Result<(), crate::pallet::Error> { use crate::pallet; 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(pallet::Error::IdNotFound)?; + let heavier_node = Node::::get(&heavier_id).ok_or(pallet::Error::IdNotFound)?; ensure!(lighter_node.bag_upper == heavier_node.bag_upper, pallet::Error::NotInSameBag); @@ -403,7 +403,7 @@ impl List { // 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(|| { + 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 @@ -422,7 +422,7 @@ impl List { /// - this is a naive function in that it does not check if `node` belongs to the same bag as /// `at`. It is expected that the call site will check preconditions. /// - this will panic if `at.bag_upper` is not a bag that already exists in storage. - fn insert_at_unchecked(mut at: Node, mut node: Node) { + fn insert_at_unchecked(mut at: Node, mut node: Node) { // connect `node` to its new `prev`. node.prev = at.prev.clone(); if let Some(mut prev) = at.prev() { @@ -439,7 +439,7 @@ impl List { // since `node` is always in front of `at` we know that 1) there is always at least 2 // nodes in the bag, and 2) only `node` could be the head and only `at` could be the // tail. - let mut bag = Bag::::get(at.bag_upper) + let mut bag = Bag::::get(at.bag_upper) .expect("given nodes must always have a valid bag. qed."); if node.prev == None { @@ -473,8 +473,8 @@ impl List { ); let iter_count = Self::iter().count() as u32; - let stored_count = crate::ListNodes::::count(); - let nodes_count = crate::ListNodes::::iter().count() as u32; + let stored_count = crate::ListNodes::::count(); + let nodes_count = crate::ListNodes::::iter().count() as u32; ensure!(iter_count == stored_count, "iter_count != stored_count"); ensure!(stored_count == nodes_count, "stored_count != nodes_count"); @@ -489,7 +489,7 @@ impl List { // otherwise, insert it here. thresholds.chain(iter::once(VoteWeight::MAX)).collect() }; - thresholds.into_iter().filter_map(|t| Bag::::get(t)) + thresholds.into_iter().filter_map(|t| Bag::::get(t)) }; let _ = active_bags.clone().map(|b| b.sanity_check()).collect::>()?; @@ -502,7 +502,7 @@ impl List { // check that all nodes are sane. We check the `ListNodes` storage item directly in case we // have some "stale" nodes that are not in a bag. - for (_id, node) in crate::ListNodes::::iter() { + for (_id, node) in crate::ListNodes::::iter() { node.sanity_check()? } @@ -531,7 +531,7 @@ impl List { }; iter.filter_map(|t| { - Bag::::get(t).map(|bag| (t, bag.iter().map(|n| n.id().clone()).collect::>())) + Bag::::get(t).map(|bag| (t, bag.iter().map(|n| n.id().clone()).collect::>())) }) .collect::>() } @@ -548,7 +548,7 @@ impl List { #[codec(mel_bound())] #[scale_info(skip_type_params(T))] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))] -pub struct Bag { +pub struct Bag, I: 'static = ()> { head: Option, tail: Option, @@ -556,7 +556,7 @@ pub struct Bag { bag_upper: VoteWeight, } -impl Bag { +impl, I: 'static> Bag { #[cfg(test)] pub(crate) fn new( head: Option, @@ -567,8 +567,8 @@ impl Bag { } /// Get a bag by its upper vote weight. - pub(crate) fn get(bag_upper: VoteWeight) -> Option> { - crate::ListBags::::try_get(bag_upper).ok().map(|mut bag| { + pub(crate) fn get(bag_upper: VoteWeight) -> Option> { + crate::ListBags::::try_get(bag_upper).ok().map(|mut bag| { bag.bag_upper = bag_upper; bag }) @@ -576,7 +576,7 @@ impl Bag { /// Get a bag by its upper vote weight or make it, appropriately initialized. Does not check if /// if `bag_upper` is a valid threshold. - fn get_or_make(bag_upper: VoteWeight) -> Bag { + fn get_or_make(bag_upper: VoteWeight) -> Bag { Self::get(bag_upper).unwrap_or(Bag { bag_upper, ..Default::default() }) } @@ -588,24 +588,24 @@ impl Bag { /// Put the bag back into storage. fn put(self) { if self.is_empty() { - crate::ListBags::::remove(self.bag_upper); + crate::ListBags::::remove(self.bag_upper); } else { - crate::ListBags::::insert(self.bag_upper, self); + crate::ListBags::::insert(self.bag_upper, self); } } /// Get the head node in this bag. - fn head(&self) -> Option> { + fn head(&self) -> Option> { self.head.as_ref().and_then(|id| Node::get(id)) } /// Get the tail node in this bag. - fn tail(&self) -> Option> { + fn tail(&self) -> Option> { self.tail.as_ref().and_then(|id| Node::get(id)) } /// Iterate over the nodes in this bag. - pub(crate) fn iter(&self) -> impl Iterator> { + pub(crate) fn iter(&self) -> impl Iterator> { sp_std::iter::successors(self.head(), |prev| prev.next()) } @@ -620,7 +620,7 @@ impl Bag { // insert_node will overwrite `prev`, `next` and `bag_upper` to the proper values. As long // as this bag is the correct one, we're good. All calls to this must come after getting the // correct [`notional_bag_for`]. - self.insert_node_unchecked(Node:: { id, prev: None, next: None, bag_upper: 0 }); + self.insert_node_unchecked(Node:: { id, prev: None, next: None, bag_upper: 0 }); } /// Insert a node into this bag. @@ -630,7 +630,7 @@ impl Bag { /// /// Storage note: this modifies storage, but only for the node. You still need to call /// `self.put()` after use. - fn insert_node_unchecked(&mut self, mut node: Node) { + fn insert_node_unchecked(&mut self, mut node: Node) { if let Some(tail) = &self.tail { if *tail == node.id { // this should never happen, but this check prevents one path to a worst case @@ -674,7 +674,7 @@ impl Bag { /// /// Storage note: this modifies storage, but only for adjacent nodes. You still need to call /// `self.put()` and `ListNodes::remove(id)` to update storage for the bag and `node`. - fn remove_node_unchecked(&mut self, node: &Node) { + fn remove_node_unchecked(&mut self, node: &Node) { // reassign neighboring nodes. node.excise(); @@ -735,7 +735,7 @@ impl Bag { /// Iterate over the nodes in this bag (public for tests). #[cfg(feature = "std")] #[allow(dead_code)] - pub fn std_iter(&self) -> impl Iterator> { + pub fn std_iter(&self) -> impl Iterator> { sp_std::iter::successors(self.head(), |prev| prev.next()) } @@ -751,22 +751,22 @@ impl Bag { #[codec(mel_bound())] #[scale_info(skip_type_params(T))] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))] -pub struct Node { +pub struct Node, I: 'static = ()> { id: T::AccountId, prev: Option, next: Option, bag_upper: VoteWeight, } -impl Node { +impl, I: 'static> Node { /// Get a node by id. - pub fn get(id: &T::AccountId) -> Option> { - crate::ListNodes::::try_get(id).ok() + pub fn get(id: &T::AccountId) -> Option> { + crate::ListNodes::::try_get(id).ok() } /// Put the node back into storage. fn put(self) { - crate::ListNodes::::insert(self.id.clone(), self); + crate::ListNodes::::insert(self.id.clone(), self); } /// Update neighboring nodes to point to reach other. @@ -790,22 +790,22 @@ impl Node { /// /// It is naive because it does not check if the node has first been removed from its bag. fn remove_from_storage_unchecked(&self) { - crate::ListNodes::::remove(&self.id) + crate::ListNodes::::remove(&self.id) } /// Get the previous node in the bag. - fn prev(&self) -> Option> { + fn prev(&self) -> Option> { self.prev.as_ref().and_then(|id| Node::get(id)) } /// Get the next node in the bag. - fn next(&self) -> Option> { + fn next(&self) -> Option> { self.next.as_ref().and_then(|id| Node::get(id)) } /// `true` when this voter is in the wrong bag. pub fn is_misplaced(&self, current_weight: VoteWeight) -> bool { - notional_bag_for::(current_weight) != self.bag_upper + notional_bag_for::(current_weight) != self.bag_upper } /// `true` when this voter is a bag head or tail. @@ -834,7 +834,7 @@ impl Node { #[cfg(feature = "std")] fn sanity_check(&self) -> Result<(), &'static str> { - let expected_bag = Bag::::get(self.bag_upper).ok_or("bag not found for node")?; + let expected_bag = Bag::::get(self.bag_upper).ok_or("bag not found for node")?; let id = self.id(); diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs index aa3f549e12dec..14b161f1bc25c 100644 --- a/frame/bags-list/src/mock.rs +++ b/frame/bags-list/src/mock.rs @@ -91,7 +91,7 @@ frame_support::construct_runtime!( UncheckedExtrinsic = UncheckedExtrinsic, { System: frame_system::{Pallet, Call, Storage, Event, Config}, - BagsList: bags_list::{Pallet, Call, Storage, Event}, + BagsList: bags_list::{Pallet, Call, Storage, Event}, } ); From 90d09f243d32557929f1d939f359bf01fd9223db Mon Sep 17 00:00:00 2001 From: doordashcon Date: Fri, 4 Mar 2022 20:05:37 +0100 Subject: [PATCH 02/14] update --- frame/bags-list/remote-tests/src/lib.rs | 2 +- frame/bags-list/src/benchmarks.rs | 36 ++++++------- frame/bags-list/src/list/mod.rs | 13 +++-- frame/bags-list/src/list/tests.rs | 71 +++++++++++++------------ frame/bags-list/src/mock.rs | 2 +- frame/bags-list/src/tests.rs | 11 ++-- 6 files changed, 70 insertions(+), 65 deletions(-) diff --git a/frame/bags-list/remote-tests/src/lib.rs b/frame/bags-list/remote-tests/src/lib.rs index 9a88ff24f2f3b..32686a63858ce 100644 --- a/frame/bags-list/remote-tests/src/lib.rs +++ b/frame/bags-list/remote-tests/src/lib.rs @@ -92,7 +92,7 @@ pub fn display_and_check_bags(currency_unit: u64, currency_na "Account {:?} can be rebagged from {:?} to {:?}", id, vote_weight_thresh_as_unit, - pallet_bags_list::notional_bag_for::(vote_weight) as f64 / + pallet_bags_list::notional_bag_for::(vote_weight) as f64 / currency_unit as f64 ); } diff --git a/frame/bags-list/src/benchmarks.rs b/frame/bags-list/src/benchmarks.rs index 396f0071821a9..2d89a0027eca2 100644 --- a/frame/bags-list/src/benchmarks.rs +++ b/frame/bags-list/src/benchmarks.rs @@ -36,7 +36,7 @@ frame_benchmarking::benchmarks! { // clear any pre-existing storage. // NOTE: safe to call outside block production - List::::unsafe_clear(); + List::::unsafe_clear(); // define our origin and destination thresholds. let origin_bag_thresh = T::BagThresholds::get()[0]; @@ -44,21 +44,21 @@ frame_benchmarking::benchmarks! { // seed items in the origin bag. let origin_head: T::AccountId = account("origin_head", 0, 0); - assert_ok!(List::::insert(origin_head.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(origin_middle.clone(), origin_bag_thresh)); + assert_ok!(List::::insert(origin_middle.clone(), origin_bag_thresh)); let origin_tail: T::AccountId = account("origin_tail", 0, 0); - assert_ok!(List::::insert(origin_tail.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(dest_head.clone(), dest_bag_thresh)); + assert_ok!(List::::insert(dest_head.clone(), dest_bag_thresh)); // the bags are in the expected state after initial setup. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ (origin_bag_thresh, vec![origin_head.clone(), origin_middle.clone(), origin_tail.clone()]), (dest_bag_thresh, vec![dest_head.clone()]) @@ -72,7 +72,7 @@ frame_benchmarking::benchmarks! { verify { // check the bags have updated as expected. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ ( origin_bag_thresh, @@ -104,18 +104,18 @@ frame_benchmarking::benchmarks! { // seed items in the origin bag. let origin_head: T::AccountId = account("origin_head", 0, 0); - assert_ok!(List::::insert(origin_head.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(origin_tail.clone(), origin_bag_thresh)); + assert_ok!(List::::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::::insert(dest_head.clone(), dest_bag_thresh)); + assert_ok!(List::::insert(dest_head.clone(), dest_bag_thresh)); // the bags are in the expected state after initial setup. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ (origin_bag_thresh, vec![origin_head.clone(), origin_tail.clone()]), (dest_bag_thresh, vec![dest_head.clone()]) @@ -129,7 +129,7 @@ frame_benchmarking::benchmarks! { verify { // check the bags have updated as expected. assert_eq!( - List::::get_bags(), + List::::get_bags(), vec![ (origin_bag_thresh, vec![origin_head.clone()]), (dest_bag_thresh, vec![dest_head.clone(), origin_tail.clone()]) @@ -147,22 +147,22 @@ frame_benchmarking::benchmarks! { // insert the nodes in order let lighter: T::AccountId = account("lighter", 0, 0); - assert_ok!(List::::insert(lighter.clone(), bag_thresh)); + assert_ok!(List::::insert(lighter.clone(), bag_thresh)); let heavier_prev: T::AccountId = account("heavier_prev", 0, 0); - assert_ok!(List::::insert(heavier_prev.clone(), bag_thresh)); + assert_ok!(List::::insert(heavier_prev.clone(), bag_thresh)); let heavier: T::AccountId = account("heavier", 0, 0); - assert_ok!(List::::insert(heavier.clone(), bag_thresh)); + assert_ok!(List::::insert(heavier.clone(), bag_thresh)); let heavier_next: T::AccountId = account("heavier_next", 0, 0); - assert_ok!(List::::insert(heavier_next.clone(), bag_thresh)); + assert_ok!(List::::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); assert_eq!( - List::::iter().map(|n| n.id().clone()).collect::>(), + List::::iter().map(|n| n.id().clone()).collect::>(), vec![lighter.clone(), heavier_prev.clone(), heavier.clone(), heavier_next.clone()] ); @@ -170,7 +170,7 @@ frame_benchmarking::benchmarks! { }: _(SystemOrigin::Signed(heavier.clone()), lighter.clone()) verify { assert_eq!( - List::::iter().map(|n| n.id().clone()).collect::>(), + List::::iter().map(|n| n.id().clone()).collect::>(), vec![heavier, lighter, heavier_prev, heavier_next] ) } diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index a6da25ae6be16..0c2bdd21e0c30 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -73,7 +73,7 @@ pub fn notional_bag_for, I: 'static>(weight: VoteWeight) -> VoteWei // weight decreases as successive bags are reached. This means that it is valid to truncate // iteration at any desired point; only those ids in the lowest bag can be excluded. This // satisfies both the desire for fairness and the requirement for efficiency. -pub struct List, I: 'static = ()>(PhantomData); +pub struct List, I: 'static = ()>(PhantomData<(T, I)>); impl, I: 'static> List { /// Remove all data associated with the list from storage. @@ -546,9 +546,10 @@ impl, I: 'static> List { /// appearing within the ids set. #[derive(DefaultNoBound, Encode, Decode, MaxEncodedLen, TypeInfo)] #[codec(mel_bound())] -#[scale_info(skip_type_params(T))] +#[scale_info(skip_type_params(T, I))] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))] pub struct Bag, I: 'static = ()> { + phantom: PhantomData, head: Option, tail: Option, @@ -559,11 +560,12 @@ pub struct Bag, I: 'static = ()> { impl, I: 'static> Bag { #[cfg(test)] pub(crate) fn new( + phantom: PhantomData, head: Option, tail: Option, bag_upper: VoteWeight, ) -> Self { - Self { head, tail, bag_upper } + Self { phantom, head, tail, bag_upper } } /// Get a bag by its upper vote weight. @@ -620,7 +622,7 @@ impl, I: 'static> Bag { // insert_node will overwrite `prev`, `next` and `bag_upper` to the proper values. As long // as this bag is the correct one, we're good. All calls to this must come after getting the // correct [`notional_bag_for`]. - self.insert_node_unchecked(Node:: { id, prev: None, next: None, bag_upper: 0 }); + self.insert_node_unchecked(Node:: { phantom: Default::default(), id, prev: None, next: None, bag_upper: 0 }); } /// Insert a node into this bag. @@ -749,9 +751,10 @@ impl, I: 'static> Bag { /// A Node is the fundamental element comprising the doubly-linked list described by `Bag`. #[derive(Encode, Decode, MaxEncodedLen, TypeInfo)] #[codec(mel_bound())] -#[scale_info(skip_type_params(T))] +#[scale_info(skip_type_params(T, I))] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))] pub struct Node, I: 'static = ()> { + phantom: PhantomData, id: T::AccountId, prev: Option, next: Option, diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index aaa215b0af1ca..08ea07c4902f8 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -27,7 +27,7 @@ use frame_support::{assert_ok, assert_storage_noop}; fn basic_setup_works() { ExtBuilder::default().build_and_execute(|| { // syntactic sugar to create a raw node - let node = |id, prev, next, bag_upper| Node:: { id, prev, next, bag_upper }; + let node = |phantom, id, prev, next, bag_upper| Node:: { phantom, id, prev, next, bag_upper }; assert_eq!(ListNodes::::count(), 4); assert_eq!(ListNodes::::iter().count(), 4); @@ -38,17 +38,17 @@ fn basic_setup_works() { // the state of the bags is as expected assert_eq!( ListBags::::get(10).unwrap(), - Bag:: { head: Some(1), tail: Some(1), bag_upper: 0 } + Bag:: { phantom: PhantomData, head: Some(1), tail: Some(1), bag_upper: 0 } ); assert_eq!( ListBags::::get(1_000).unwrap(), - Bag:: { head: Some(2), tail: Some(4), bag_upper: 0 } + Bag:: { phantom: PhantomData, head: Some(2), tail: Some(4), bag_upper: 0 } ); - assert_eq!(ListNodes::::get(2).unwrap(), node(2, None, Some(3), 1_000)); - assert_eq!(ListNodes::::get(3).unwrap(), node(3, Some(2), Some(4), 1_000)); - assert_eq!(ListNodes::::get(4).unwrap(), node(4, Some(3), None, 1_000)); - assert_eq!(ListNodes::::get(1).unwrap(), node(1, None, None, 10)); + assert_eq!(ListNodes::::get(2).unwrap(), node(PhantomData, 2, None, Some(3), 1_000)); + assert_eq!(ListNodes::::get(3).unwrap(), node(PhantomData, 3, Some(2), Some(4), 1_000)); + assert_eq!(ListNodes::::get(4).unwrap(), node(PhantomData, 4, Some(3), None, 1_000)); + assert_eq!(ListNodes::::get(1).unwrap(), node(PhantomData, 1, None, None, 10)); // non-existent id does not have a storage footprint assert_eq!(ListNodes::::get(42), None); @@ -65,14 +65,14 @@ fn basic_setup_works() { #[test] fn notional_bag_for_works() { // under a threshold gives the next threshold. - assert_eq!(notional_bag_for::(0), 10); - assert_eq!(notional_bag_for::(9), 10); + assert_eq!(notional_bag_for::(0), 10); + assert_eq!(notional_bag_for::(9), 10); // at a threshold gives that threshold. - assert_eq!(notional_bag_for::(10), 10); + assert_eq!(notional_bag_for::(10), 10); // above the threshold, gives the next threshold. - assert_eq!(notional_bag_for::(11), 20); + assert_eq!(notional_bag_for::(11), 20); let max_explicit_threshold = *::BagThresholds::get().last().unwrap(); assert_eq!(max_explicit_threshold, 10_000); @@ -81,8 +81,8 @@ fn notional_bag_for_works() { assert!(VoteWeight::MAX > max_explicit_threshold); // then anything above it will belong to the VoteWeight::MAX bag. - assert_eq!(notional_bag_for::(max_explicit_threshold), max_explicit_threshold); - assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); + assert_eq!(notional_bag_for::(max_explicit_threshold), max_explicit_threshold); + assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); } #[test] @@ -388,8 +388,8 @@ mod list { #[should_panic = "given nodes must always have a valid bag. qed."] fn put_in_front_of_panics_if_bag_not_found() { ExtBuilder::default().skip_genesis_ids().build_and_execute_no_post_check(|| { - let node_10_no_bag = Node:: { id: 10, prev: None, next: None, bag_upper: 15 }; - let node_11_no_bag = Node:: { id: 11, prev: None, next: None, bag_upper: 15 }; + let node_10_no_bag = Node:: { phantom: PhantomData, id: 10, prev: None, next: None, bag_upper: 15 }; + let node_11_no_bag = Node:: { phantom: PhantomData, id: 11, prev: None, next: None, bag_upper: 15 }; // given ListNodes::::insert(10, node_10_no_bag); @@ -415,7 +415,7 @@ mod list { // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. let node_42 = - Node:: { id: 42, prev: Some(1), next: Some(2), bag_upper: 1_000 }; + Node:: { phantom: PhantomData, id: 42, prev: Some(1), next: Some(2), bag_upper: 1_000 }; assert!(!crate::ListNodes::::contains_key(42)); let node_1 = crate::ListNodes::::get(&1).unwrap(); @@ -438,7 +438,7 @@ mod list { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. - let node_42 = Node:: { id: 42, prev: Some(4), next: None, bag_upper: 1_000 }; + let node_42 = Node:: { phantom: PhantomData, id: 42, prev: Some(4), next: None, bag_upper: 1_000 }; assert!(!crate::ListNodes::::contains_key(42)); let node_2 = crate::ListNodes::::get(&2).unwrap(); @@ -461,7 +461,7 @@ mod list { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. - let node_42 = Node:: { id: 42, prev: None, next: Some(2), bag_upper: 1_000 }; + let node_42 = Node:: { phantom: PhantomData, id: 42, prev: None, next: Some(2), bag_upper: 1_000 }; assert!(!crate::ListNodes::::contains_key(42)); let node_3 = crate::ListNodes::::get(&3).unwrap(); @@ -485,7 +485,7 @@ mod list { // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. let node_42 = - Node:: { id: 42, prev: Some(42), next: Some(42), bag_upper: 1_000 }; + Node:: { phantom: PhantomData, id: 42, prev: Some(42), next: Some(42), bag_upper: 1_000 }; assert!(!crate::ListNodes::::contains_key(42)); let node_4 = crate::ListNodes::::get(&4).unwrap(); @@ -512,7 +512,7 @@ mod bags { let bag = Bag::::get(bag_upper).unwrap(); let bag_ids = bag.iter().map(|n| *n.id()).collect::>(); - assert_eq!(bag, Bag:: { head, tail, bag_upper }); + assert_eq!(bag, Bag:: { phantom: PhantomData, head, tail, bag_upper }); assert_eq!(bag_ids, ids); }; @@ -543,7 +543,7 @@ mod bags { #[test] fn insert_node_sets_proper_bag() { ExtBuilder::default().build_and_execute_no_post_check(|| { - let node = |id, bag_upper| Node:: { id, prev: None, next: None, bag_upper }; + let node = |id, bag_upper| Node:: { phantom: PhantomData, id, prev: None, next: None, bag_upper }; assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); @@ -552,7 +552,7 @@ mod bags { assert_eq!( ListNodes::::get(&42).unwrap(), - Node { bag_upper: 10, prev: Some(1), next: None, id: 42 } + Node { phantom: PhantomData, bag_upper: 10, prev: Some(1), next: None, id: 42 } ); }); } @@ -560,7 +560,7 @@ mod bags { #[test] fn insert_node_happy_paths_works() { ExtBuilder::default().build_and_execute_no_post_check(|| { - let node = |id, bag_upper| Node:: { id, prev: None, next: None, bag_upper }; + let node = |id, bag_upper| Node:: { phantom: PhantomData, id, prev: None, next: None, bag_upper }; // when inserting into a bag with 1 node let mut bag_10 = Bag::::get(10).unwrap(); @@ -582,14 +582,14 @@ mod bags { // when inserting a node pointing to the accounts not in the bag let node_61 = - Node:: { id: 61, prev: Some(21), next: Some(101), bag_upper: 20 }; + Node:: { phantom: PhantomData, id: 61, prev: Some(21), next: Some(101), bag_upper: 20 }; bag_20.insert_node_unchecked(node_61); // then ids are in order assert_eq!(bag_as_ids(&bag_20), vec![62, 61]); // and when the node is re-fetched all the info is correct assert_eq!( Node::::get(&61).unwrap(), - Node:: { id: 61, prev: Some(62), next: None, bag_upper: 20 } + Node:: { phantom: PhantomData, id: 61, prev: Some(62), next: None, bag_upper: 20 } ); // state of all bags is as expected @@ -604,12 +604,12 @@ mod bags { // Document improper ways `insert_node` may be getting used. #[test] fn insert_node_bad_paths_documented() { - let node = |id, prev, next, bag_upper| Node:: { id, prev, next, bag_upper }; + let node = | phantom, id, prev, next, bag_upper| Node:: { phantom, id, prev, next, bag_upper }; ExtBuilder::default().build_and_execute_no_post_check(|| { // when inserting a node with both prev & next pointing at an account in an incorrect // bag. let mut bag_1000 = Bag::::get(1_000).unwrap(); - bag_1000.insert_node_unchecked(node(42, Some(1), Some(1), 500)); + bag_1000.insert_node_unchecked(node(PhantomData, 42, Some(1), Some(1), 500)); // then the proper prev and next is set. assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 4, 42]); @@ -617,7 +617,7 @@ mod bags { // and when the node is re-fetched all the info is correct assert_eq!( Node::::get(&42).unwrap(), - node(42, Some(4), None, bag_1000.bag_upper) + node(PhantomData, 42, Some(4), None, bag_1000.bag_upper) ); }); @@ -627,7 +627,7 @@ mod bags { assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 4]); // when inserting a node with duplicate id 3 - bag_1000.insert_node_unchecked(node(3, None, None, bag_1000.bag_upper)); + bag_1000.insert_node_unchecked(node(PhantomData, 3, None, None, bag_1000.bag_upper)); // then all the nodes after the duplicate are lost (because it is set as the tail) assert_eq!(bag_as_ids(&bag_1000), vec![2, 3]); @@ -637,7 +637,7 @@ mod bags { // and the last accessible node has an **incorrect** prev pointer. assert_eq!( Node::::get(&3).unwrap(), - node(3, Some(4), None, bag_1000.bag_upper) + node(PhantomData, 3, Some(4), None, bag_1000.bag_upper) ); }); @@ -645,7 +645,7 @@ mod bags { // when inserting a duplicate id of the head let mut bag_1000 = Bag::::get(1_000).unwrap(); assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 4]); - bag_1000.insert_node_unchecked(node(2, None, None, 0)); + bag_1000.insert_node_unchecked(node(PhantomData, 2, None, None, 0)); // then all nodes after the head are lost assert_eq!(bag_as_ids(&bag_1000), vec![2]); @@ -653,11 +653,11 @@ mod bags { // and the re-fetched node has bad pointers assert_eq!( Node::::get(&2).unwrap(), - node(2, Some(4), None, bag_1000.bag_upper) + node(PhantomData, 2, Some(4), None, bag_1000.bag_upper) ); // ^^^ despite being the bags head, it has a prev - assert_eq!(bag_1000, Bag { head: Some(2), tail: Some(2), bag_upper: 1_000 }) + assert_eq!(bag_1000, Bag { phantom: PhantomData, head: Some(2), tail: Some(2), bag_upper: 1_000 }) }); } @@ -669,7 +669,7 @@ mod bags { )] fn insert_node_duplicate_tail_panics_with_debug_assert() { ExtBuilder::default().build_and_execute(|| { - let node = |id, prev, next, bag_upper| Node:: { id, prev, next, bag_upper }; + let node = | phantom, id, prev, next, bag_upper| Node:: { phantom, id, prev, next, bag_upper }; // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])],); @@ -678,7 +678,7 @@ mod bags { // when inserting a duplicate id that is already the tail assert_eq!(bag_1000.tail, Some(4)); assert_eq!(bag_1000.iter().count(), 3); - bag_1000.insert_node_unchecked(node(4, None, None, bag_1000.bag_upper)); // panics in debug + bag_1000.insert_node_unchecked(node(PhantomData, 4, None, None, bag_1000.bag_upper)); // panics in debug assert_eq!(bag_1000.iter().count(), 3); // in release we expect it to silently ignore the request. }); } @@ -797,6 +797,7 @@ mod bags { fn remove_node_bad_paths_documented() { ExtBuilder::default().build_and_execute_no_post_check(|| { let bad_upper_node_2 = Node:: { + phantom: PhantomData, id: 2, prev: None, next: Some(3), diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs index 14b161f1bc25c..aa3f549e12dec 100644 --- a/frame/bags-list/src/mock.rs +++ b/frame/bags-list/src/mock.rs @@ -91,7 +91,7 @@ frame_support::construct_runtime!( UncheckedExtrinsic = UncheckedExtrinsic, { System: frame_system::{Pallet, Call, Storage, Event, Config}, - BagsList: bags_list::{Pallet, Call, Storage, Event}, + BagsList: bags_list::{Pallet, Call, Storage, Event}, } ); diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index 43397b3c120f5..2cc2f39cbcad9 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -21,6 +21,7 @@ use super::*; use frame_election_provider_support::SortedListProvider; use list::Bag; use mock::{test_utils::*, *}; +use sp_std::marker::PhantomData; mod pallet { use super::*; @@ -90,7 +91,7 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 4]), (1_000, vec![2, 3])]); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(2), Some(3), 1_000)); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(2), Some(3), 1_000)); // when StakingMock::set_vote_weight_of(&3, 10); @@ -99,8 +100,8 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 4, 3]), (1_000, vec![2])]); - assert_eq!(Bag::::get(10).unwrap(), Bag::new(Some(1), Some(3), 10)); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(2), Some(2), 1_000)); + assert_eq!(Bag::::get(10).unwrap(), Bag::new(PhantomData, Some(1), Some(3), 10)); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(2), Some(2), 1_000)); assert_eq!(get_list_as_ids(), vec![2u32, 1, 4, 3]); // when @@ -124,7 +125,7 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 2]), (1_000, vec![3, 4])]); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(3), Some(4), 1_000)); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(3), Some(4), 1_000)); // when StakingMock::set_vote_weight_of(&3, 10); @@ -132,7 +133,7 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 2, 3]), (1_000, vec![4])]); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(4), Some(4), 1_000)); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(4), Some(4), 1_000)); // when StakingMock::set_vote_weight_of(&4, 10); From 7d6cb8dc7f5054c3c1ce801be9ab8f274101dcc9 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Fri, 4 Mar 2022 20:06:47 +0100 Subject: [PATCH 03/14] cargo fmt --- frame/bags-list/src/lib.rs | 3 +- frame/bags-list/src/list/mod.rs | 17 +++- frame/bags-list/src/list/tests.rs | 127 +++++++++++++++++++++++++----- frame/bags-list/src/tests.rs | 25 ++++-- 4 files changed, 142 insertions(+), 30 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index d0384367f5ded..477494402231e 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -163,7 +163,8 @@ pub mod pallet { /// /// Stores a `Bag` struct, which stores head and tail pointers to itself. #[pallet::storage] - pub(crate) type ListBags, I: 'static = ()> = StorageMap<_, Twox64Concat, VoteWeight, list::Bag>; + pub(crate) type ListBags, I: 'static = ()> = + StorageMap<_, Twox64Concat, VoteWeight, list::Bag>; #[pallet::event] #[pallet::generate_deposit(pub(crate) fn deposit_event)] diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 0c2bdd21e0c30..ad086da6d0a7b 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -135,11 +135,13 @@ impl, I: 'static> List { // we can't check all preconditions, but we can check one debug_assert!( - crate::ListBags::::iter().all(|(threshold, _)| old_thresholds.contains(&threshold)), + crate::ListBags::::iter() + .all(|(threshold, _)| old_thresholds.contains(&threshold)), "not all `bag_upper` currently in storage are members of `old_thresholds`", ); debug_assert!( - crate::ListNodes::::iter().all(|(_, node)| old_thresholds.contains(&node.bag_upper)), + crate::ListNodes::::iter() + .all(|(_, node)| old_thresholds.contains(&node.bag_upper)), "not all `node.bag_upper` currently in storage are members of `old_thresholds`", ); @@ -531,7 +533,8 @@ impl, I: 'static> List { }; iter.filter_map(|t| { - Bag::::get(t).map(|bag| (t, bag.iter().map(|n| n.id().clone()).collect::>())) + Bag::::get(t) + .map(|bag| (t, bag.iter().map(|n| n.id().clone()).collect::>())) }) .collect::>() } @@ -622,7 +625,13 @@ impl, I: 'static> Bag { // insert_node will overwrite `prev`, `next` and `bag_upper` to the proper values. As long // as this bag is the correct one, we're good. All calls to this must come after getting the // correct [`notional_bag_for`]. - self.insert_node_unchecked(Node:: { phantom: Default::default(), id, prev: None, next: None, bag_upper: 0 }); + self.insert_node_unchecked(Node:: { + phantom: Default::default(), + id, + prev: None, + next: None, + bag_upper: 0, + }); } /// Insert a node into this bag. diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index 08ea07c4902f8..0aa8662bdd683 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -27,7 +27,13 @@ use frame_support::{assert_ok, assert_storage_noop}; fn basic_setup_works() { ExtBuilder::default().build_and_execute(|| { // syntactic sugar to create a raw node - let node = |phantom, id, prev, next, bag_upper| Node:: { phantom, id, prev, next, bag_upper }; + let node = |phantom, id, prev, next, bag_upper| Node:: { + phantom, + id, + prev, + next, + bag_upper, + }; assert_eq!(ListNodes::::count(), 4); assert_eq!(ListNodes::::iter().count(), 4); @@ -45,9 +51,18 @@ fn basic_setup_works() { Bag:: { phantom: PhantomData, head: Some(2), tail: Some(4), bag_upper: 0 } ); - assert_eq!(ListNodes::::get(2).unwrap(), node(PhantomData, 2, None, Some(3), 1_000)); - assert_eq!(ListNodes::::get(3).unwrap(), node(PhantomData, 3, Some(2), Some(4), 1_000)); - assert_eq!(ListNodes::::get(4).unwrap(), node(PhantomData, 4, Some(3), None, 1_000)); + assert_eq!( + ListNodes::::get(2).unwrap(), + node(PhantomData, 2, None, Some(3), 1_000) + ); + assert_eq!( + ListNodes::::get(3).unwrap(), + node(PhantomData, 3, Some(2), Some(4), 1_000) + ); + assert_eq!( + ListNodes::::get(4).unwrap(), + node(PhantomData, 4, Some(3), None, 1_000) + ); assert_eq!(ListNodes::::get(1).unwrap(), node(PhantomData, 1, None, None, 10)); // non-existent id does not have a storage footprint @@ -388,8 +403,20 @@ mod list { #[should_panic = "given nodes must always have a valid bag. qed."] fn put_in_front_of_panics_if_bag_not_found() { ExtBuilder::default().skip_genesis_ids().build_and_execute_no_post_check(|| { - let node_10_no_bag = Node:: { phantom: PhantomData, id: 10, prev: None, next: None, bag_upper: 15 }; - let node_11_no_bag = Node:: { phantom: PhantomData, id: 11, prev: None, next: None, bag_upper: 15 }; + let node_10_no_bag = Node:: { + phantom: PhantomData, + id: 10, + prev: None, + next: None, + bag_upper: 15, + }; + let node_11_no_bag = Node:: { + phantom: PhantomData, + id: 11, + prev: None, + next: None, + bag_upper: 15, + }; // given ListNodes::::insert(10, node_10_no_bag); @@ -414,8 +441,13 @@ mod list { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. - let node_42 = - Node:: { phantom: PhantomData, id: 42, prev: Some(1), next: Some(2), bag_upper: 1_000 }; + let node_42 = Node:: { + phantom: PhantomData, + id: 42, + prev: Some(1), + next: Some(2), + bag_upper: 1_000, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_1 = crate::ListNodes::::get(&1).unwrap(); @@ -438,7 +470,13 @@ mod list { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. - let node_42 = Node:: { phantom: PhantomData, id: 42, prev: Some(4), next: None, bag_upper: 1_000 }; + let node_42 = Node:: { + phantom: PhantomData, + id: 42, + prev: Some(4), + next: None, + bag_upper: 1_000, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_2 = crate::ListNodes::::get(&2).unwrap(); @@ -461,7 +499,13 @@ mod list { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. - let node_42 = Node:: { phantom: PhantomData, id: 42, prev: None, next: Some(2), bag_upper: 1_000 }; + let node_42 = Node:: { + phantom: PhantomData, + id: 42, + prev: None, + next: Some(2), + bag_upper: 1_000, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_3 = crate::ListNodes::::get(&3).unwrap(); @@ -484,8 +528,13 @@ mod list { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. - let node_42 = - Node:: { phantom: PhantomData, id: 42, prev: Some(42), next: Some(42), bag_upper: 1_000 }; + let node_42 = Node:: { + phantom: PhantomData, + id: 42, + prev: Some(42), + next: Some(42), + bag_upper: 1_000, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_4 = crate::ListNodes::::get(&4).unwrap(); @@ -543,7 +592,13 @@ mod bags { #[test] fn insert_node_sets_proper_bag() { ExtBuilder::default().build_and_execute_no_post_check(|| { - let node = |id, bag_upper| Node:: { phantom: PhantomData, id, prev: None, next: None, bag_upper }; + let node = |id, bag_upper| Node:: { + phantom: PhantomData, + id, + prev: None, + next: None, + bag_upper, + }; assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); @@ -560,7 +615,13 @@ mod bags { #[test] fn insert_node_happy_paths_works() { ExtBuilder::default().build_and_execute_no_post_check(|| { - let node = |id, bag_upper| Node:: { phantom: PhantomData, id, prev: None, next: None, bag_upper }; + let node = |id, bag_upper| Node:: { + phantom: PhantomData, + id, + prev: None, + next: None, + bag_upper, + }; // when inserting into a bag with 1 node let mut bag_10 = Bag::::get(10).unwrap(); @@ -581,15 +642,26 @@ mod bags { assert_eq!(bag_as_ids(&bag_20), vec![62]); // when inserting a node pointing to the accounts not in the bag - let node_61 = - Node:: { phantom: PhantomData, id: 61, prev: Some(21), next: Some(101), bag_upper: 20 }; + let node_61 = Node:: { + phantom: PhantomData, + id: 61, + prev: Some(21), + next: Some(101), + bag_upper: 20, + }; bag_20.insert_node_unchecked(node_61); // then ids are in order assert_eq!(bag_as_ids(&bag_20), vec![62, 61]); // and when the node is re-fetched all the info is correct assert_eq!( Node::::get(&61).unwrap(), - Node:: { phantom: PhantomData, id: 61, prev: Some(62), next: None, bag_upper: 20 } + Node:: { + phantom: PhantomData, + id: 61, + prev: Some(62), + next: None, + bag_upper: 20 + } ); // state of all bags is as expected @@ -604,7 +676,13 @@ mod bags { // Document improper ways `insert_node` may be getting used. #[test] fn insert_node_bad_paths_documented() { - let node = | phantom, id, prev, next, bag_upper| Node:: { phantom, id, prev, next, bag_upper }; + let node = |phantom, id, prev, next, bag_upper| Node:: { + phantom, + id, + prev, + next, + bag_upper, + }; ExtBuilder::default().build_and_execute_no_post_check(|| { // when inserting a node with both prev & next pointing at an account in an incorrect // bag. @@ -657,7 +735,10 @@ mod bags { ); // ^^^ despite being the bags head, it has a prev - assert_eq!(bag_1000, Bag { phantom: PhantomData, head: Some(2), tail: Some(2), bag_upper: 1_000 }) + assert_eq!( + bag_1000, + Bag { phantom: PhantomData, head: Some(2), tail: Some(2), bag_upper: 1_000 } + ) }); } @@ -669,7 +750,13 @@ mod bags { )] fn insert_node_duplicate_tail_panics_with_debug_assert() { ExtBuilder::default().build_and_execute(|| { - let node = | phantom, id, prev, next, bag_upper| Node:: { phantom, id, prev, next, bag_upper }; + let node = |phantom, id, prev, next, bag_upper| Node:: { + phantom, + id, + prev, + next, + bag_upper, + }; // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])],); diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index 2cc2f39cbcad9..9e0d7de567c28 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -91,7 +91,10 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 4]), (1_000, vec![2, 3])]); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(2), Some(3), 1_000)); + assert_eq!( + Bag::::get(1_000).unwrap(), + Bag::new(PhantomData, Some(2), Some(3), 1_000) + ); // when StakingMock::set_vote_weight_of(&3, 10); @@ -100,8 +103,14 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 4, 3]), (1_000, vec![2])]); - assert_eq!(Bag::::get(10).unwrap(), Bag::new(PhantomData, Some(1), Some(3), 10)); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(2), Some(2), 1_000)); + assert_eq!( + Bag::::get(10).unwrap(), + Bag::new(PhantomData, Some(1), Some(3), 10) + ); + assert_eq!( + Bag::::get(1_000).unwrap(), + Bag::new(PhantomData, Some(2), Some(2), 1_000) + ); assert_eq!(get_list_as_ids(), vec![2u32, 1, 4, 3]); // when @@ -125,7 +134,10 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 2]), (1_000, vec![3, 4])]); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(3), Some(4), 1_000)); + assert_eq!( + Bag::::get(1_000).unwrap(), + Bag::new(PhantomData, Some(3), Some(4), 1_000) + ); // when StakingMock::set_vote_weight_of(&3, 10); @@ -133,7 +145,10 @@ mod pallet { // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 2, 3]), (1_000, vec![4])]); - assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(PhantomData, Some(4), Some(4), 1_000)); + assert_eq!( + Bag::::get(1_000).unwrap(), + Bag::new(PhantomData, Some(4), Some(4), 1_000) + ); // when StakingMock::set_vote_weight_of(&4, 10); From 0b9a13ecaf6391f7b3abb8e9ae96353e4b80385c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 8 Mar 2022 13:40:21 +0000 Subject: [PATCH 04/14] Clean up --- frame/bags-list/src/list/mod.rs | 10 ++--- frame/bags-list/src/list/tests.rs | 68 +++++++++++++++---------------- frame/bags-list/src/tests.rs | 11 +++-- 3 files changed, 44 insertions(+), 45 deletions(-) diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index ad086da6d0a7b..e2c74a85417d1 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -552,23 +552,23 @@ impl, I: 'static> List { #[scale_info(skip_type_params(T, I))] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))] pub struct Bag, I: 'static = ()> { - phantom: PhantomData, head: Option, tail: Option, #[codec(skip)] bag_upper: VoteWeight, + #[codec(skip)] + phantom: PhantomData, } impl, I: 'static> Bag { #[cfg(test)] pub(crate) fn new( - phantom: PhantomData, head: Option, tail: Option, bag_upper: VoteWeight, ) -> Self { - Self { phantom, head, tail, bag_upper } + Self { head, tail, bag_upper, phantom: PhantomData, } } /// Get a bag by its upper vote weight. @@ -626,11 +626,11 @@ impl, I: 'static> Bag { // as this bag is the correct one, we're good. All calls to this must come after getting the // correct [`notional_bag_for`]. self.insert_node_unchecked(Node:: { - phantom: Default::default(), id, prev: None, next: None, bag_upper: 0, + phantom: PhantomData, }); } @@ -763,11 +763,11 @@ impl, I: 'static> Bag { #[scale_info(skip_type_params(T, I))] #[cfg_attr(feature = "std", derive(frame_support::DebugNoBound, Clone, PartialEq))] pub struct Node, I: 'static = ()> { - phantom: PhantomData, id: T::AccountId, prev: Option, next: Option, bag_upper: VoteWeight, + phantom: PhantomData, } impl, I: 'static> Node { diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index 0aa8662bdd683..a9e13cbc8efcf 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -27,12 +27,12 @@ use frame_support::{assert_ok, assert_storage_noop}; fn basic_setup_works() { ExtBuilder::default().build_and_execute(|| { // syntactic sugar to create a raw node - let node = |phantom, id, prev, next, bag_upper| Node:: { - phantom, + let node = |id, prev, next, bag_upper| Node:: { id, prev, next, bag_upper, + phantom: PhantomData, }; assert_eq!(ListNodes::::count(), 4); @@ -44,26 +44,26 @@ fn basic_setup_works() { // the state of the bags is as expected assert_eq!( ListBags::::get(10).unwrap(), - Bag:: { phantom: PhantomData, head: Some(1), tail: Some(1), bag_upper: 0 } + Bag:: { head: Some(1), tail: Some(1), bag_upper: 0, phantom: PhantomData } ); assert_eq!( ListBags::::get(1_000).unwrap(), - Bag:: { phantom: PhantomData, head: Some(2), tail: Some(4), bag_upper: 0 } + Bag:: { head: Some(2), tail: Some(4), bag_upper: 0, phantom: PhantomData } ); assert_eq!( ListNodes::::get(2).unwrap(), - node(PhantomData, 2, None, Some(3), 1_000) + node(2, None, Some(3), 1_000) ); assert_eq!( ListNodes::::get(3).unwrap(), - node(PhantomData, 3, Some(2), Some(4), 1_000) + node(3, Some(2), Some(4), 1_000) ); assert_eq!( ListNodes::::get(4).unwrap(), - node(PhantomData, 4, Some(3), None, 1_000) + node(4, Some(3), None, 1_000) ); - assert_eq!(ListNodes::::get(1).unwrap(), node(PhantomData, 1, None, None, 10)); + assert_eq!(ListNodes::::get(1).unwrap(), node(1, None, None, 10)); // non-existent id does not have a storage footprint assert_eq!(ListNodes::::get(42), None); @@ -404,18 +404,18 @@ mod list { fn put_in_front_of_panics_if_bag_not_found() { ExtBuilder::default().skip_genesis_ids().build_and_execute_no_post_check(|| { let node_10_no_bag = Node:: { - phantom: PhantomData, id: 10, prev: None, next: None, bag_upper: 15, + phantom: PhantomData, }; let node_11_no_bag = Node:: { - phantom: PhantomData, id: 11, prev: None, next: None, bag_upper: 15, + phantom: PhantomData, }; // given @@ -442,11 +442,11 @@ mod list { // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. let node_42 = Node:: { - phantom: PhantomData, id: 42, prev: Some(1), next: Some(2), bag_upper: 1_000, + phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -471,11 +471,11 @@ mod list { // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. let node_42 = Node:: { - phantom: PhantomData, id: 42, prev: Some(4), next: None, bag_upper: 1_000, + phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -500,11 +500,11 @@ mod list { // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. let node_42 = Node:: { - phantom: PhantomData, id: 42, prev: None, next: Some(2), bag_upper: 1_000, + phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -529,11 +529,11 @@ mod list { // implicitly also test that `node`'s `prev`/`next` are correctly re-assigned. let node_42 = Node:: { - phantom: PhantomData, id: 42, prev: Some(42), next: Some(42), bag_upper: 1_000, + phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -561,7 +561,7 @@ mod bags { let bag = Bag::::get(bag_upper).unwrap(); let bag_ids = bag.iter().map(|n| *n.id()).collect::>(); - assert_eq!(bag, Bag:: { phantom: PhantomData, head, tail, bag_upper }); + assert_eq!(bag, Bag:: { head, tail, bag_upper, phantom: PhantomData }); assert_eq!(bag_ids, ids); }; @@ -593,11 +593,11 @@ mod bags { fn insert_node_sets_proper_bag() { ExtBuilder::default().build_and_execute_no_post_check(|| { let node = |id, bag_upper| Node:: { - phantom: PhantomData, id, prev: None, next: None, bag_upper, + phantom: PhantomData, }; assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); @@ -607,7 +607,7 @@ mod bags { assert_eq!( ListNodes::::get(&42).unwrap(), - Node { phantom: PhantomData, bag_upper: 10, prev: Some(1), next: None, id: 42 } + Node { bag_upper: 10, prev: Some(1), next: None, id: 42, phantom: PhantomData } ); }); } @@ -616,11 +616,11 @@ mod bags { fn insert_node_happy_paths_works() { ExtBuilder::default().build_and_execute_no_post_check(|| { let node = |id, bag_upper| Node:: { - phantom: PhantomData, id, prev: None, next: None, bag_upper, + phantom: PhantomData, }; // when inserting into a bag with 1 node @@ -643,11 +643,11 @@ mod bags { // when inserting a node pointing to the accounts not in the bag let node_61 = Node:: { - phantom: PhantomData, id: 61, prev: Some(21), next: Some(101), bag_upper: 20, + phantom: PhantomData, }; bag_20.insert_node_unchecked(node_61); // then ids are in order @@ -656,11 +656,11 @@ mod bags { assert_eq!( Node::::get(&61).unwrap(), Node:: { - phantom: PhantomData, id: 61, prev: Some(62), next: None, - bag_upper: 20 + bag_upper: 20, + phantom: PhantomData, } ); @@ -676,18 +676,18 @@ mod bags { // Document improper ways `insert_node` may be getting used. #[test] fn insert_node_bad_paths_documented() { - let node = |phantom, id, prev, next, bag_upper| Node:: { - phantom, + let node = |id, prev, next, bag_upper| Node:: { id, prev, next, bag_upper, + phantom: PhantomData, }; ExtBuilder::default().build_and_execute_no_post_check(|| { // when inserting a node with both prev & next pointing at an account in an incorrect // bag. let mut bag_1000 = Bag::::get(1_000).unwrap(); - bag_1000.insert_node_unchecked(node(PhantomData, 42, Some(1), Some(1), 500)); + bag_1000.insert_node_unchecked(node(42, Some(1), Some(1), 500)); // then the proper prev and next is set. assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 4, 42]); @@ -695,7 +695,7 @@ mod bags { // and when the node is re-fetched all the info is correct assert_eq!( Node::::get(&42).unwrap(), - node(PhantomData, 42, Some(4), None, bag_1000.bag_upper) + node(42, Some(4), None, bag_1000.bag_upper) ); }); @@ -705,7 +705,7 @@ mod bags { assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 4]); // when inserting a node with duplicate id 3 - bag_1000.insert_node_unchecked(node(PhantomData, 3, None, None, bag_1000.bag_upper)); + bag_1000.insert_node_unchecked(node(3, None, None, bag_1000.bag_upper)); // then all the nodes after the duplicate are lost (because it is set as the tail) assert_eq!(bag_as_ids(&bag_1000), vec![2, 3]); @@ -715,7 +715,7 @@ mod bags { // and the last accessible node has an **incorrect** prev pointer. assert_eq!( Node::::get(&3).unwrap(), - node(PhantomData, 3, Some(4), None, bag_1000.bag_upper) + node(3, Some(4), None, bag_1000.bag_upper) ); }); @@ -723,7 +723,7 @@ mod bags { // when inserting a duplicate id of the head let mut bag_1000 = Bag::::get(1_000).unwrap(); assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 4]); - bag_1000.insert_node_unchecked(node(PhantomData, 2, None, None, 0)); + bag_1000.insert_node_unchecked(node(2, None, None, 0)); // then all nodes after the head are lost assert_eq!(bag_as_ids(&bag_1000), vec![2]); @@ -731,13 +731,13 @@ mod bags { // and the re-fetched node has bad pointers assert_eq!( Node::::get(&2).unwrap(), - node(PhantomData, 2, Some(4), None, bag_1000.bag_upper) + node(2, Some(4), None, bag_1000.bag_upper) ); // ^^^ despite being the bags head, it has a prev assert_eq!( bag_1000, - Bag { phantom: PhantomData, head: Some(2), tail: Some(2), bag_upper: 1_000 } + Bag { head: Some(2), tail: Some(2), bag_upper: 1_000, phantom: PhantomData } ) }); } @@ -750,12 +750,12 @@ mod bags { )] fn insert_node_duplicate_tail_panics_with_debug_assert() { ExtBuilder::default().build_and_execute(|| { - let node = |phantom, id, prev, next, bag_upper| Node:: { - phantom, + let node = |id, prev, next, bag_upper| Node:: { id, prev, next, bag_upper, + phantom: PhantomData, }; // given @@ -765,7 +765,7 @@ mod bags { // when inserting a duplicate id that is already the tail assert_eq!(bag_1000.tail, Some(4)); assert_eq!(bag_1000.iter().count(), 3); - bag_1000.insert_node_unchecked(node(PhantomData, 4, None, None, bag_1000.bag_upper)); // panics in debug + bag_1000.insert_node_unchecked(node(4, None, None, bag_1000.bag_upper)); // panics in debug assert_eq!(bag_1000.iter().count(), 3); // in release we expect it to silently ignore the request. }); } @@ -884,11 +884,11 @@ mod bags { fn remove_node_bad_paths_documented() { ExtBuilder::default().build_and_execute_no_post_check(|| { let bad_upper_node_2 = Node:: { - phantom: PhantomData, id: 2, prev: None, next: Some(3), bag_upper: 10, // should be 1_000 + phantom: PhantomData, }; let mut bag_1000 = Bag::::get(1_000).unwrap(); diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index 9e0d7de567c28..848b4ac22b374 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -21,7 +21,6 @@ use super::*; use frame_election_provider_support::SortedListProvider; use list::Bag; use mock::{test_utils::*, *}; -use sp_std::marker::PhantomData; mod pallet { use super::*; @@ -93,7 +92,7 @@ mod pallet { assert_eq!(List::::get_bags(), vec![(10, vec![1, 4]), (1_000, vec![2, 3])]); assert_eq!( Bag::::get(1_000).unwrap(), - Bag::new(PhantomData, Some(2), Some(3), 1_000) + Bag::new(Some(2), Some(3), 1_000) ); // when @@ -105,11 +104,11 @@ mod pallet { assert_eq!( Bag::::get(10).unwrap(), - Bag::new(PhantomData, Some(1), Some(3), 10) + Bag::new(Some(1), Some(3), 10) ); assert_eq!( Bag::::get(1_000).unwrap(), - Bag::new(PhantomData, Some(2), Some(2), 1_000) + Bag::new(Some(2), Some(2), 1_000) ); assert_eq!(get_list_as_ids(), vec![2u32, 1, 4, 3]); @@ -136,7 +135,7 @@ mod pallet { assert_eq!(List::::get_bags(), vec![(10, vec![1, 2]), (1_000, vec![3, 4])]); assert_eq!( Bag::::get(1_000).unwrap(), - Bag::new(PhantomData, Some(3), Some(4), 1_000) + Bag::new(Some(3), Some(4), 1_000) ); // when @@ -147,7 +146,7 @@ mod pallet { assert_eq!(List::::get_bags(), vec![(10, vec![1, 2, 3]), (1_000, vec![4])]); assert_eq!( Bag::::get(1_000).unwrap(), - Bag::new(PhantomData, Some(4), Some(4), 1_000) + Bag::new(Some(4), Some(4), 1_000) ); // when From 7a0f55c3192bde57ba088355229de8b20457478c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 8 Mar 2022 17:54:35 +0000 Subject: [PATCH 05/14] bags-list: Make it generic over node value --- bin/node/runtime/src/lib.rs | 3 +- frame/bags-list/src/benchmarks.rs | 11 +-- frame/bags-list/src/lib.rs | 66 ++++++++++------ frame/bags-list/src/list/mod.rs | 90 ++++++++++++---------- frame/bags-list/src/list/tests.rs | 32 +++----- frame/bags-list/src/mock.rs | 29 +++---- frame/bags-list/src/tests.rs | 78 ++++++++----------- frame/election-provider-support/src/lib.rs | 20 ++--- frame/staking/src/mock.rs | 3 +- frame/staking/src/pallet/impls.rs | 22 ++++-- frame/staking/src/pallet/mod.rs | 2 +- 11 files changed, 183 insertions(+), 173 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index e2903c0b314da..bad147a9d0944 100644 --- a/bin/node/runtime/src/lib.rs +++ b/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 ValueProvider = Staking; type WeightInfo = pallet_bags_list::weights::SubstrateWeight; type BagThresholds = BagThresholds; + type Value = sp_npos_elections::VoteWeight; } parameter_types! { diff --git a/frame/bags-list/src/benchmarks.rs b/frame/bags-list/src/benchmarks.rs index 2d89a0027eca2..7bd174ff21b49 100644 --- a/frame/bags-list/src/benchmarks.rs +++ b/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::ValueProvider; use frame_support::{assert_ok, traits::Get}; use frame_system::RawOrigin as SystemOrigin; +use sp_runtime::traits::One; frame_benchmarking::benchmarks! { rebag_non_terminal { @@ -67,7 +68,7 @@ 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::ValueProvider::set_value_of(&origin_middle, dest_bag_thresh); }: rebag(SystemOrigin::Signed(caller), origin_middle.clone()) verify { // check the bags have updated as expected. @@ -124,7 +125,7 @@ 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::ValueProvider::set_value_of(&origin_tail, dest_bag_thresh); }: rebag(SystemOrigin::Signed(caller), origin_tail.clone()) verify { // check the bags have updated as expected. @@ -158,8 +159,8 @@ frame_benchmarking::benchmarks! { let heavier_next: T::AccountId = account("heavier_next", 0, 0); assert_ok!(List::::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::ValueProvider::set_value_of(&lighter, bag_thresh - One::one()); + T::ValueProvider::set_value_of(&heavier, bag_thresh); assert_eq!( List::::iter().map(|n| n.id().clone()).collect::>(), diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 477494402231e..485ac3fb2f8a3 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -17,13 +17,14 @@ //! # Bags-List Pallet //! -//! A semi-sorted list, where items hold an `AccountId` based on some `VoteWeight`. The `AccountId` -//! (`id` for short) might be synonym to a `voter` or `nominator` in some context, and `VoteWeight` -//! signifies the chance of each id being included in the final [`SortedListProvider::iter`]. +//! A semi-sorted list, where items hold an `AccountId` based on some `Value`. The +//! `AccountId` (`id` for short) might be synonym to a `voter` or `nominator` in some context, and +//! `Value` signifies the chance of each id being included in the final +//! [`SortedListProvider::iter`]. //! //! It implements [`frame_election_provider_support::SortedListProvider`] to provide a semi-sorted //! list of accounts to another pallet. It needs some other pallet to give it some information about -//! the weights of accounts via [`frame_election_provider_support::VoteWeightProvider`]. +//! the weights of accounts via [`frame_election_provider_support::ValueProvider`]. //! //! This pallet is not configurable at genesis. Whoever uses it should call appropriate functions of //! the `SortedListProvider` (e.g. `on_insert`, or `unsafe_regenerate`) at their genesis. @@ -52,8 +53,10 @@ #![cfg_attr(not(feature = "std"), no_std)] -use frame_election_provider_support::{SortedListProvider, VoteWeight, VoteWeightProvider}; +use codec::{Codec, FullCodec}; +use frame_election_provider_support::{SortedListProvider, ValueProvider}; use frame_system::ensure_signed; +use sp_runtime::traits::{AtLeast32BitUnsigned, Bounded}; use sp_std::prelude::*; #[cfg(any(feature = "runtime-benchmarks", test))] @@ -103,7 +106,7 @@ pub mod pallet { type WeightInfo: weights::WeightInfo; /// Something that provides the weights of ids. - type VoteWeightProvider: VoteWeightProvider; + type ValueProvider: ValueProvider; /// The list of thresholds separating the various bags. /// @@ -120,10 +123,10 @@ pub mod pallet { /// This constant must be sorted in strictly increasing order. Duplicate items are not /// permitted. /// - /// There is an implied upper limit of `VoteWeight::MAX`; that value does not need to be + /// There is an implied upper limit of `Value::MAX`; that value does not need to be /// specified within the bag. For any two threshold lists, if one ends with - /// `VoteWeight::MAX`, the other one does not, and they are otherwise equal, the two lists - /// will behave identically. + /// `Value::MAX`, the other one does not, and they are otherwise equal, the two + /// lists will behave identically. /// /// # Calculation /// @@ -149,7 +152,23 @@ pub mod pallet { /// In the event that this list ever changes, a copy of the old bags list must be retained. /// With that `List::migrate` can be called, which will perform the appropriate migration. #[pallet::constant] - type BagThresholds: Get<&'static [VoteWeight]>; + type BagThresholds: Get<&'static [Self::Value]>; + + /// The type of value used to dictate a node position relative to other nodes. + type Value: Clone + + Default + + PartialEq + + Eq + + Ord + + PartialOrd + + sp_std::fmt::Debug + + Copy + + AtLeast32BitUnsigned + + Bounded + + TypeInfo + + Codec + + FullCodec + + MaxEncodedLen; } /// A single node, within some bag. @@ -164,13 +183,13 @@ pub mod pallet { /// Stores a `Bag` struct, which stores head and tail pointers to itself. #[pallet::storage] pub(crate) type ListBags, I: 'static = ()> = - StorageMap<_, Twox64Concat, VoteWeight, list::Bag>; + StorageMap<_, Twox64Concat, T::Value, list::Bag>; #[pallet::event] #[pallet::generate_deposit(pub(crate) fn deposit_event)] pub enum Event, I: 'static = ()> { /// Moved an account from one bag to another. - Rebagged { who: T::AccountId, from: VoteWeight, to: VoteWeight }, + Rebagged { who: T::AccountId, from: T::Value, to: T::Value }, } #[pallet::error] @@ -197,7 +216,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::rebag_non_terminal().max(T::WeightInfo::rebag_terminal()))] pub fn rebag(origin: OriginFor, dislocated: T::AccountId) -> DispatchResult { ensure_signed(origin)?; - let current_weight = T::VoteWeightProvider::vote_weight(&dislocated); + let current_weight = T::ValueProvider::value(&dislocated); let _ = Pallet::::do_rebag(&dislocated, current_weight); Ok(()) } @@ -209,7 +228,7 @@ pub mod pallet { /// /// Only works if /// - both nodes are within the same bag, - /// - and `origin` has a greater `VoteWeight` than `lighter`. + /// - and `origin` has a greater `Value` than `lighter`. #[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)?; @@ -233,10 +252,7 @@ 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: VoteWeight, - ) -> Option<(VoteWeight, VoteWeight)> { + pub fn do_rebag(account: &T::AccountId, new_weight: T::Value) -> Option<(T::Value, T::Value)> { // 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) @@ -249,7 +265,7 @@ impl, I: 'static> Pallet { /// Equivalent to `ListBags::get`, but public. Useful for tests in outside of this crate. #[cfg(feature = "std")] - pub fn list_bags_get(weight: VoteWeight) -> Option> { + pub fn list_bags_get(weight: T::Value) -> Option> { ListBags::get(weight) } } @@ -257,6 +273,8 @@ impl, I: 'static> Pallet { impl, I: 'static> SortedListProvider for Pallet { type Error = Error; + type Value = T::Value; + fn iter() -> Box> { Box::new(List::::iter().map(|n| n.id().clone())) } @@ -269,11 +287,11 @@ impl, I: 'static> SortedListProvider for Pallet List::::contains(id) } - fn on_insert(id: T::AccountId, weight: VoteWeight) -> Result<(), Error> { + fn on_insert(id: T::AccountId, weight: T::Value) -> Result<(), Error> { List::::insert(id, weight) } - fn on_update(id: &T::AccountId, new_weight: VoteWeight) { + fn on_update(id: &T::AccountId, new_weight: T::Value) { Pallet::::do_rebag(id, new_weight); } @@ -283,7 +301,7 @@ impl, I: 'static> SortedListProvider for Pallet fn unsafe_regenerate( all: impl IntoIterator, - weight_of: Box VoteWeight>, + weight_of: Box T::Value>, ) -> u32 { // NOTE: This call is unsafe for the same reason as SortedListProvider::unsafe_regenerate. // I.e. because it can lead to many storage accesses. @@ -309,13 +327,13 @@ impl, I: 'static> SortedListProvider for Pallet } #[cfg(feature = "runtime-benchmarks")] - fn weight_update_worst_case(who: &T::AccountId, is_increase: bool) -> VoteWeight { + fn weight_update_worst_case(who: &T::AccountId, is_increase: bool) -> Self::Value { use frame_support::traits::Get as _; let thresholds = T::BagThresholds::get(); let node = list::Node::::get(who).unwrap(); let current_bag_idx = thresholds .iter() - .chain(sp_std::iter::once(&VoteWeight::MAX)) + .chain(sp_std::iter::once(&T::Value::max_value())) .position(|w| w == &node.bag_upper()) .unwrap(); diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index e2c74a85417d1..1e284fe6a82b6 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -26,9 +26,10 @@ use crate::Config; use codec::{Decode, Encode, MaxEncodedLen}; -use frame_election_provider_support::{VoteWeight, VoteWeightProvider}; +use frame_election_provider_support::ValueProvider; use frame_support::{traits::Get, DefaultNoBound}; use scale_info::TypeInfo; +use sp_runtime::traits::{Bounded, Zero}; use sp_std::{ boxed::Box, collections::{btree_map::BTreeMap, btree_set::BTreeSet}, @@ -46,26 +47,26 @@ pub enum Error { #[cfg(test)] mod tests; -/// Given a certain vote weight, to which bag does it belong to? +/// Given a certain value, to which bag does it belong to? /// /// Bags are identified by their upper threshold; the value returned by this function is guaranteed /// to be a member of `T::BagThresholds`. /// -/// Note that even if the thresholds list does not have `VoteWeight::MAX` as its final member, this -/// function behaves as if it does. -pub fn notional_bag_for, I: 'static>(weight: VoteWeight) -> VoteWeight { +/// Note that even if the thresholds list does not have `T::Value::max_value()` as its final member, +/// this function behaves as if it does. +pub fn notional_bag_for, I: 'static>(weight: T::Value) -> T::Value { let thresholds = T::BagThresholds::get(); let idx = thresholds.partition_point(|&threshold| weight > threshold); - thresholds.get(idx).copied().unwrap_or(VoteWeight::MAX) + thresholds.get(idx).copied().unwrap_or(T::Value::max_value()) } /// The **ONLY** entry point of this module. All operations to the bags-list should happen through /// this interface. It is forbidden to access other module members directly. // -// Data structure providing efficient mostly-accurate selection of the top N id by `VoteWeight`. +// Data structure providing efficient mostly-accurate selection of the top N id by `Value`. // // It's implemented as a set of linked lists. Each linked list comprises a bag of ids of -// arbitrary and unbounded length, all having a vote weight within a particular constant range. +// arbitrary and unbounded length, all having a value within a particular constant range. // This structure means that ids can be added and removed in `O(1)` time. // // Iteration is accomplished by chaining the iteration of each bag, from greatest to least. While @@ -98,7 +99,7 @@ impl, I: 'static> List { /// Returns the number of ids migrated. pub fn unsafe_regenerate( all: impl IntoIterator, - weight_of: Box VoteWeight>, + weight_of: Box T::Value>, ) -> u32 { // NOTE: This call is unsafe for the same reason as SortedListProvider::unsafe_regenerate. // I.e. because it can lead to many storage accesses. @@ -127,7 +128,7 @@ impl, I: 'static> List { /// - ids whose bags change at all are implicitly rebagged into the appropriate bag in the new /// threshold set. #[allow(dead_code)] - pub fn migrate(old_thresholds: &[VoteWeight]) -> u32 { + pub fn migrate(old_thresholds: &[T::Value]) -> u32 { let new_thresholds = T::BagThresholds::get(); if new_thresholds == old_thresholds { return 0 @@ -160,7 +161,7 @@ impl, I: 'static> List { let affected_bag = { // this recreates `notional_bag_for` logic, but with the old thresholds. let idx = old_thresholds.partition_point(|&threshold| inserted_bag > threshold); - old_thresholds.get(idx).copied().unwrap_or(VoteWeight::MAX) + old_thresholds.get(idx).copied().unwrap_or(T::Value::max_value()) }; if !affected_old_bags.insert(affected_bag) { // If the previous threshold list was [10, 20], and we insert [3, 5], then there's @@ -187,7 +188,7 @@ impl, I: 'static> List { // migrate the voters whose bag has changed let num_affected = affected_accounts.len() as u32; - let weight_of = T::VoteWeightProvider::vote_weight; + let weight_of = T::ValueProvider::value; let _removed = Self::remove_many(&affected_accounts); debug_assert_eq!(_removed, num_affected); let _inserted = Self::insert_many(affected_accounts.into_iter(), weight_of); @@ -230,12 +231,14 @@ impl, I: 'static> List { // easier; they can just configure `type BagThresholds = ()`. let thresholds = T::BagThresholds::get(); let iter = thresholds.iter().copied(); - let iter: Box> = if thresholds.last() == Some(&VoteWeight::MAX) { + let iter: Box> = if thresholds.last() == + Some(&T::Value::max_value()) + { // in the event that they included it, we can just pass the iterator through unchanged. Box::new(iter.rev()) } else { // otherwise, insert it here. - Box::new(iter.chain(iter::once(VoteWeight::MAX)).rev()) + Box::new(iter.chain(iter::once(T::Value::max_value())).rev()) }; iter.filter_map(Bag::get).flat_map(|bag| bag.iter()) @@ -247,7 +250,7 @@ impl, I: 'static> List { /// Returns the final count of number of ids inserted. fn insert_many( ids: impl IntoIterator, - weight_of: impl Fn(&T::AccountId) -> VoteWeight, + weight_of: impl Fn(&T::AccountId) -> T::Value, ) -> u32 { let mut count = 0; ids.into_iter().for_each(|v| { @@ -263,7 +266,7 @@ impl, I: 'static> List { /// Insert a new id into the appropriate bag in the list. /// /// Returns an error if the list already contains `id`. - pub(crate) fn insert(id: T::AccountId, weight: VoteWeight) -> Result<(), Error> { + pub(crate) fn insert(id: T::AccountId, weight: T::Value) -> Result<(), Error> { if Self::contains(&id) { return Err(Error::Duplicate) } @@ -278,7 +281,8 @@ impl, I: 'static> List { crate::log!( debug, - "inserted {:?} with weight {} into bag {:?}, new count is {}", + "inserted {:?} with weight {:? + } into bag {:?}, new count is {}", id, weight, bag_weight, @@ -344,8 +348,8 @@ impl, I: 'static> List { /// to call [`self.remove_many`] followed by [`self.insert_many`]. pub(crate) fn update_position_for( node: Node, - new_weight: VoteWeight, - ) -> Option<(VoteWeight, VoteWeight)> { + new_weight: T::Value, + ) -> Option<(T::Value, T::Value)> { node.is_misplaced(new_weight).then(move || { let old_bag_upper = node.bag_upper; @@ -394,8 +398,7 @@ impl, I: 'static> List { // this is the most expensive check, so we do it last. ensure!( - T::VoteWeightProvider::vote_weight(&heavier_id) > - T::VoteWeightProvider::vote_weight(&lighter_id), + T::ValueProvider::value(&heavier_id) > T::ValueProvider::value(&lighter_id), pallet::Error::NotHeavier ); @@ -484,13 +487,14 @@ impl, I: 'static> List { let active_bags = { let thresholds = T::BagThresholds::get().iter().copied(); - let thresholds: Vec = if thresholds.clone().last() == Some(VoteWeight::MAX) { - // in the event that they included it, we don't need to make any changes - thresholds.collect() - } else { - // otherwise, insert it here. - thresholds.chain(iter::once(VoteWeight::MAX)).collect() - }; + let thresholds: Vec = + if thresholds.clone().last() == Some(T::Value::max_value()) { + // in the event that they included it, we don't need to make any changes + thresholds.collect() + } else { + // otherwise, insert it here. + thresholds.chain(iter::once(T::Value::max_value())).collect() + }; thresholds.into_iter().filter_map(|t| Bag::::get(t)) }; @@ -519,17 +523,19 @@ impl, I: 'static> List { /// Returns the nodes of all non-empty bags. For testing and benchmarks. #[cfg(any(feature = "std", feature = "runtime-benchmarks"))] #[allow(dead_code)] - pub(crate) fn get_bags() -> Vec<(VoteWeight, Vec)> { + pub(crate) fn get_bags() -> Vec<(T::Value, Vec)> { use frame_support::traits::Get as _; let thresholds = T::BagThresholds::get(); let iter = thresholds.iter().copied(); - let iter: Box> = if thresholds.last() == Some(&VoteWeight::MAX) { + let iter: Box> = if thresholds.last() == + Some(&T::Value::max_value()) + { // in the event that they included it, we can just pass the iterator through unchanged. Box::new(iter) } else { // otherwise, insert it here. - Box::new(iter.chain(sp_std::iter::once(VoteWeight::MAX))) + Box::new(iter.chain(sp_std::iter::once(T::Value::max_value()))) }; iter.filter_map(|t| { @@ -556,7 +562,7 @@ pub struct Bag, I: 'static = ()> { tail: Option, #[codec(skip)] - bag_upper: VoteWeight, + bag_upper: T::Value, #[codec(skip)] phantom: PhantomData, } @@ -566,22 +572,22 @@ impl, I: 'static> Bag { pub(crate) fn new( head: Option, tail: Option, - bag_upper: VoteWeight, + bag_upper: T::Value, ) -> Self { - Self { head, tail, bag_upper, phantom: PhantomData, } + Self { head, tail, bag_upper, phantom: PhantomData } } - /// Get a bag by its upper vote weight. - pub(crate) fn get(bag_upper: VoteWeight) -> Option> { + /// Get a bag by its upper value. + pub(crate) fn get(bag_upper: T::Value) -> Option> { crate::ListBags::::try_get(bag_upper).ok().map(|mut bag| { bag.bag_upper = bag_upper; bag }) } - /// Get a bag by its upper vote weight or make it, appropriately initialized. Does not check if + /// Get a bag by its upper value or make it, appropriately initialized. Does not check if /// if `bag_upper` is a valid threshold. - fn get_or_make(bag_upper: VoteWeight) -> Bag { + fn get_or_make(bag_upper: T::Value) -> Bag { Self::get(bag_upper).unwrap_or(Bag { bag_upper, ..Default::default() }) } @@ -629,7 +635,7 @@ impl, I: 'static> Bag { id, prev: None, next: None, - bag_upper: 0, + bag_upper: Zero::zero(), phantom: PhantomData, }); } @@ -766,7 +772,7 @@ pub struct Node, I: 'static = ()> { id: T::AccountId, prev: Option, next: Option, - bag_upper: VoteWeight, + bag_upper: T::Value, phantom: PhantomData, } @@ -816,7 +822,7 @@ impl, I: 'static> Node { } /// `true` when this voter is in the wrong bag. - pub fn is_misplaced(&self, current_weight: VoteWeight) -> bool { + pub fn is_misplaced(&self, current_weight: T::Value) -> bool { notional_bag_for::(current_weight) != self.bag_upper } @@ -840,7 +846,7 @@ impl, I: 'static> Node { /// The bag this nodes belongs to (public for benchmarks). #[cfg(feature = "runtime-benchmarks")] #[allow(dead_code)] - pub fn bag_upper(&self) -> VoteWeight { + pub fn bag_upper(&self) -> T::Value { self.bag_upper } diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index a9e13cbc8efcf..72ac20a51034b 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -51,18 +51,9 @@ fn basic_setup_works() { Bag:: { head: Some(2), tail: Some(4), bag_upper: 0, phantom: PhantomData } ); - assert_eq!( - ListNodes::::get(2).unwrap(), - node(2, None, Some(3), 1_000) - ); - assert_eq!( - ListNodes::::get(3).unwrap(), - node(3, Some(2), Some(4), 1_000) - ); - assert_eq!( - ListNodes::::get(4).unwrap(), - node(4, Some(3), None, 1_000) - ); + assert_eq!(ListNodes::::get(2).unwrap(), node(2, None, Some(3), 1_000)); + assert_eq!(ListNodes::::get(3).unwrap(), node(3, Some(2), Some(4), 1_000)); + assert_eq!(ListNodes::::get(4).unwrap(), node(4, Some(3), None, 1_000)); assert_eq!(ListNodes::::get(1).unwrap(), node(1, None, None, 10)); // non-existent id does not have a storage footprint @@ -92,12 +83,12 @@ fn notional_bag_for_works() { let max_explicit_threshold = *::BagThresholds::get().last().unwrap(); assert_eq!(max_explicit_threshold, 10_000); - // if the max explicit threshold is less than VoteWeight::MAX, - assert!(VoteWeight::MAX > max_explicit_threshold); + // if the max explicit threshold is less than T::Value::max_value(), + assert!(u64::MAX > max_explicit_threshold); - // then anything above it will belong to the VoteWeight::MAX bag. + // then anything above it will belong to the T::Value::max_value() bag. assert_eq!(notional_bag_for::(max_explicit_threshold), max_explicit_threshold); - assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); + assert_eq!(notional_bag_for::(max_explicit_threshold + 1), u64::MAX); } #[test] @@ -141,8 +132,7 @@ fn migrate_works() { assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]); // when the new thresholds adds `15` and removes `2_000` - const NEW_THRESHOLDS: &'static [VoteWeight] = - &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; + const NEW_THRESHOLDS: &'static [u64] = &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; BagThresholds::set(NEW_THRESHOLDS); // and we call List::::migrate(old_thresholds); @@ -421,8 +411,8 @@ mod list { // given ListNodes::::insert(10, node_10_no_bag); ListNodes::::insert(11, node_11_no_bag); - StakingMock::set_vote_weight_of(&10, 14); - StakingMock::set_vote_weight_of(&11, 15); + StakingMock::set_value_of(&10, 14); + StakingMock::set_value_of(&11, 15); assert!(!ListBags::::contains_key(15)); assert_eq!(List::::get_bags(), vec![]); @@ -574,7 +564,7 @@ mod bags { // and all other bag thresholds don't get bags. ::BagThresholds::get() .iter() - .chain(iter::once(&VoteWeight::MAX)) + .chain(iter::once(&u64::MAX)) .filter(|bag_upper| !vec![10, 1_000].contains(bag_upper)) .for_each(|bag_upper| { assert_storage_noop!(assert_eq!(Bag::::get(*bag_upper), None)); diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs index aa3f549e12dec..5c14236b53461 100644 --- a/frame/bags-list/src/mock.rs +++ b/frame/bags-list/src/mock.rs @@ -19,7 +19,6 @@ use super::*; use crate::{self as bags_list}; -use frame_election_provider_support::VoteWeight; use frame_support::parameter_types; use std::collections::HashMap; @@ -27,19 +26,21 @@ pub type AccountId = u32; pub type Balance = u32; parameter_types! { - // Set the vote weight for any id who's weight has _not_ been set with `set_vote_weight_of`. - pub static NextVoteWeight: VoteWeight = 0; - pub static NextVoteWeightMap: HashMap = Default::default(); + // Set the vote weight for any id who's weight has _not_ been set with `set_value_of`. + pub static NextVoteWeight: u64 = 0; + pub static NextVoteWeightMap: HashMap = Default::default(); } pub struct StakingMock; -impl frame_election_provider_support::VoteWeightProvider for StakingMock { - fn vote_weight(id: &AccountId) -> VoteWeight { +impl frame_election_provider_support::ValueProvider for StakingMock { + type Value = u64; + + fn value(id: &AccountId) -> Self::Value { *NextVoteWeightMap::get().get(id).unwrap_or(&NextVoteWeight::get()) } #[cfg(any(feature = "runtime-benchmarks", test))] - fn set_vote_weight_of(id: &AccountId, weight: VoteWeight) { + fn set_value_of(id: &AccountId, weight: Self::Value) { NEXT_VOTE_WEIGHT_MAP.with(|m| m.borrow_mut().insert(id.clone(), weight)); } } @@ -72,14 +73,15 @@ impl frame_system::Config for Runtime { } parameter_types! { - pub static BagThresholds: &'static [VoteWeight] = &[10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; + pub static BagThresholds: &'static [u64] = &[10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; } impl bags_list::Config for Runtime { type Event = Event; type WeightInfo = (); type BagThresholds = BagThresholds; - type VoteWeightProvider = StakingMock; + type ValueProvider = StakingMock; + type Value = u64; } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; @@ -96,12 +98,11 @@ frame_support::construct_runtime!( ); /// Default AccountIds and their weights. -pub(crate) const GENESIS_IDS: [(AccountId, VoteWeight); 4] = - [(1, 10), (2, 1_000), (3, 1_000), (4, 1_000)]; +pub(crate) const GENESIS_IDS: [(AccountId, u64); 4] = [(1, 10), (2, 1_000), (3, 1_000), (4, 1_000)]; #[derive(Default)] pub struct ExtBuilder { - ids: Vec<(AccountId, VoteWeight)>, + ids: Vec<(AccountId, u64)>, skip_genesis_ids: bool, } @@ -115,7 +116,7 @@ impl ExtBuilder { /// Add some AccountIds to insert into `List`. #[cfg(test)] - pub(crate) fn add_ids(mut self, ids: Vec<(AccountId, VoteWeight)>) -> Self { + pub(crate) fn add_ids(mut self, ids: Vec<(AccountId, u64)>) -> Self { self.ids = ids; self } @@ -134,7 +135,7 @@ impl ExtBuilder { ext.execute_with(|| { for (id, weight) in ids_with_weight { frame_support::assert_ok!(List::::insert(*id, *weight)); - StakingMock::set_vote_weight_of(id, *weight); + StakingMock::set_value_of(id, *weight); } }); diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index 848b4ac22b374..eb295f89f1505 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -35,7 +35,7 @@ mod pallet { ); // when increasing vote weight to the level of non-existent bag - StakingMock::set_vote_weight_of(&42, 2_000); + StakingMock::set_value_of(&42, 2_000); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); // then a new bag is created and the id moves into it @@ -45,7 +45,7 @@ mod pallet { ); // when decreasing weight within the range of the current bag - StakingMock::set_vote_weight_of(&42, 1_001); + StakingMock::set_value_of(&42, 1_001); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); // then the id does not move @@ -55,7 +55,7 @@ mod pallet { ); // when reducing weight to the level of a non-existent bag - StakingMock::set_vote_weight_of(&42, 30); + StakingMock::set_value_of(&42, 30); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); // then a new bag is created and the id moves into it @@ -65,7 +65,7 @@ mod pallet { ); // when increasing weight to the level of a pre-existing bag - StakingMock::set_vote_weight_of(&42, 500); + StakingMock::set_value_of(&42, 500); assert_ok!(BagsList::rebag(Origin::signed(0), 42)); // then the id moves into that bag @@ -85,35 +85,26 @@ mod pallet { assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); // when - StakingMock::set_vote_weight_of(&4, 10); + StakingMock::set_value_of(&4, 10); assert_ok!(BagsList::rebag(Origin::signed(0), 4)); // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 4]), (1_000, vec![2, 3])]); - assert_eq!( - Bag::::get(1_000).unwrap(), - Bag::new(Some(2), Some(3), 1_000) - ); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(2), Some(3), 1_000)); // when - StakingMock::set_vote_weight_of(&3, 10); + StakingMock::set_value_of(&3, 10); assert_ok!(BagsList::rebag(Origin::signed(0), 3)); // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 4, 3]), (1_000, vec![2])]); - assert_eq!( - Bag::::get(10).unwrap(), - Bag::new(Some(1), Some(3), 10) - ); - assert_eq!( - Bag::::get(1_000).unwrap(), - Bag::new(Some(2), Some(2), 1_000) - ); + assert_eq!(Bag::::get(10).unwrap(), Bag::new(Some(1), Some(3), 10)); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(2), Some(2), 1_000)); assert_eq!(get_list_as_ids(), vec![2u32, 1, 4, 3]); // when - StakingMock::set_vote_weight_of(&2, 10); + StakingMock::set_value_of(&2, 10); assert_ok!(BagsList::rebag(Origin::signed(0), 2)); // then @@ -128,29 +119,23 @@ mod pallet { fn rebag_head_works() { ExtBuilder::default().build_and_execute(|| { // when - StakingMock::set_vote_weight_of(&2, 10); + StakingMock::set_value_of(&2, 10); assert_ok!(BagsList::rebag(Origin::signed(0), 2)); // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 2]), (1_000, vec![3, 4])]); - assert_eq!( - Bag::::get(1_000).unwrap(), - Bag::new(Some(3), Some(4), 1_000) - ); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(3), Some(4), 1_000)); // when - StakingMock::set_vote_weight_of(&3, 10); + StakingMock::set_value_of(&3, 10); assert_ok!(BagsList::rebag(Origin::signed(0), 3)); // then assert_eq!(List::::get_bags(), vec![(10, vec![1, 2, 3]), (1_000, vec![4])]); - assert_eq!( - Bag::::get(1_000).unwrap(), - Bag::new(Some(4), Some(4), 1_000) - ); + assert_eq!(Bag::::get(1_000).unwrap(), Bag::new(Some(4), Some(4), 1_000)); // when - StakingMock::set_vote_weight_of(&4, 10); + StakingMock::set_value_of(&4, 10); assert_ok!(BagsList::rebag(Origin::signed(0), 4)); // then @@ -181,7 +166,7 @@ mod pallet { #[test] #[should_panic = "thresholds must strictly increase, and have no duplicates"] fn duplicate_in_bags_threshold_panics() { - const DUPE_THRESH: &[VoteWeight; 4] = &[10, 20, 30, 30]; + const DUPE_THRESH: &[u64; 4] = &[10, 20, 30, 30]; BagThresholds::set(DUPE_THRESH); BagsList::integrity_test(); } @@ -189,7 +174,7 @@ mod pallet { #[test] #[should_panic = "thresholds must strictly increase, and have no duplicates"] fn decreasing_in_bags_threshold_panics() { - const DECREASING_THRESH: &[VoteWeight; 4] = &[10, 30, 20, 40]; + const DECREASING_THRESH: &[u64; 4] = &[10, 30, 20, 40]; BagThresholds::set(DECREASING_THRESH); BagsList::integrity_test(); } @@ -200,15 +185,12 @@ mod pallet { ExtBuilder::default().build_and_execute(|| { // everyone in the same bag. - assert_eq!(List::::get_bags(), vec![(VoteWeight::MAX, vec![1, 2, 3, 4])]); + assert_eq!(List::::get_bags(), vec![(u64::MAX, vec![1, 2, 3, 4])]); // any insertion goes there as well. assert_ok!(List::::insert(5, 999)); assert_ok!(List::::insert(6, 0)); - assert_eq!( - List::::get_bags(), - vec![(VoteWeight::MAX, vec![1, 2, 3, 4, 5, 6])] - ); + assert_eq!(List::::get_bags(), vec![(u64::MAX, vec![1, 2, 3, 4, 5, 6])]); // any rebag is noop. assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 1).is_ok())); @@ -256,7 +238,7 @@ mod pallet { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); - StakingMock::set_vote_weight_of(&3, 999); + StakingMock::set_value_of(&3, 999); // when assert_ok!(BagsList::put_in_front_of(Origin::signed(4), 3)); @@ -277,7 +259,7 @@ mod pallet { vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5, 6])] ); - StakingMock::set_vote_weight_of(&5, 999); + StakingMock::set_value_of(&5, 999); // when assert_ok!(BagsList::put_in_front_of(Origin::signed(3), 5)); @@ -296,7 +278,7 @@ mod pallet { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); - StakingMock::set_vote_weight_of(&2, 999); + StakingMock::set_value_of(&2, 999); // when assert_ok!(BagsList::put_in_front_of(Origin::signed(3), 2)); @@ -312,7 +294,7 @@ mod pallet { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); - StakingMock::set_vote_weight_of(&3, 999); + StakingMock::set_value_of(&3, 999); // when assert_ok!(BagsList::put_in_front_of(Origin::signed(4), 3)); @@ -328,7 +310,7 @@ mod pallet { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); - StakingMock::set_vote_weight_of(&2, 999); + StakingMock::set_value_of(&2, 999); // when assert_ok!(BagsList::put_in_front_of(Origin::signed(5), 2)); @@ -344,7 +326,7 @@ mod pallet { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); - StakingMock::set_vote_weight_of(&4, 999); + StakingMock::set_value_of(&4, 999); // when BagsList::put_in_front_of(Origin::signed(2), 4).unwrap(); @@ -374,7 +356,7 @@ mod pallet { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); - StakingMock::set_vote_weight_of(&4, 999); + StakingMock::set_value_of(&4, 999); // when BagsList::put_in_front_of(Origin::signed(2), 4).unwrap(); @@ -390,7 +372,7 @@ mod pallet { // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); - StakingMock::set_vote_weight_of(&3, 999); + StakingMock::set_value_of(&3, 999); // then assert_noop!( @@ -490,7 +472,7 @@ mod sorted_list_provider { assert_eq!(BagsList::count(), 4); // when updating - BagsList::on_update(&201, VoteWeight::MAX); + BagsList::on_update(&201, u64::MAX); // then the count stays the same assert_eq!(BagsList::count(), 4); }); @@ -571,12 +553,12 @@ mod sorted_list_provider { 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); + BagsList::on_update(&42, u64::MAX); // the the new bag is created with the id in it, assert_eq!( List::::get_bags(), - vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (VoteWeight::MAX, vec![42])] + vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (u64::MAX, vec![42])] ); // and the id position is updated in the list. assert_eq!(BagsList::iter().collect::>(), vec![42, 2, 3, 4, 1]); diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index ca22e3093855c..b29606f7651f2 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -343,6 +343,8 @@ pub trait SortedListProvider { /// The list's error type. type Error: sp_std::fmt::Debug; + type Value; + /// An iterator over the list, which can have `take` called on it. fn iter() -> Box>; @@ -353,10 +355,10 @@ pub trait SortedListProvider { fn contains(id: &AccountId) -> bool; /// Hook for inserting a new id. - fn on_insert(id: AccountId, weight: VoteWeight) -> Result<(), Self::Error>; + fn on_insert(id: AccountId, weight: Self::Value) -> Result<(), Self::Error>; /// Hook for updating a single id. - fn on_update(id: &AccountId, weight: VoteWeight); + fn on_update(id: &AccountId, weight: Self::Value); /// Hook for removing am id from the list. fn on_remove(id: &AccountId); @@ -371,7 +373,7 @@ pub trait SortedListProvider { /// new list, which can lead to too many storage accesses, exhausting the block weight. fn unsafe_regenerate( all: impl IntoIterator, - weight_of: Box VoteWeight>, + weight_of: Box Self::Value>, ) -> u32; /// Remove all items from the list. @@ -388,21 +390,21 @@ pub trait SortedListProvider { /// If `who` changes by the returned amount they are guaranteed to have a worst case change /// in their list position. #[cfg(feature = "runtime-benchmarks")] - fn weight_update_worst_case(_who: &AccountId, _is_increase: bool) -> VoteWeight { - VoteWeight::MAX - } + fn weight_update_worst_case(_who: &AccountId, _is_increase: bool) -> Self::Value; } /// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and /// [`ElectionDataProvider`], this should typically be implementing by whoever is supposed to *use* /// `SortedListProvider`. -pub trait VoteWeightProvider { +pub trait ValueProvider { + type Value; + /// Get the current `VoteWeight` of `who`. - fn vote_weight(who: &AccountId) -> VoteWeight; + fn value(who: &AccountId) -> Self::Value; /// For tests and benchmarks, set the `VoteWeight`. #[cfg(any(feature = "runtime-benchmarks", test))] - fn set_vote_weight_of(_: &AccountId, _: VoteWeight) {} + fn set_value_of(_: &AccountId, _: Self::Value) {} } /// Something that can compute the result to an NPoS solution. diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 12d3804b4e303..0088e2d6bff5e 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -240,8 +240,9 @@ parameter_types! { impl pallet_bags_list::Config for Test { type Event = Event; type WeightInfo = (); - type VoteWeightProvider = Staking; + type ValueProvider = Staking; type BagThresholds = BagThresholds; + type Value = u64; } impl onchain::Config for Test { diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 5cd0d0107f015..9e5f35524c417 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -19,7 +19,7 @@ use frame_election_provider_support::{ data_provider, ElectionDataProvider, ElectionProvider, SortedListProvider, Supports, - VoteWeight, VoteWeightProvider, VoterOf, + VoteWeight, ValueProvider, VoterOf, }; use frame_support::{ pallet_prelude::*, @@ -1244,13 +1244,15 @@ where } } -impl VoteWeightProvider for Pallet { - fn vote_weight(who: &T::AccountId) -> VoteWeight { +impl ValueProvider for Pallet { + type Value = VoteWeight; + + fn value(who: &T::AccountId) -> Self::Value { Self::weight_of(who) } #[cfg(feature = "runtime-benchmarks")] - fn set_vote_weight_of(who: &T::AccountId, weight: VoteWeight) { + fn set_value_of(who: &T::AccountId, weight: Self::Value) { // this will clearly results in an inconsistent state, but it should not matter for a // benchmark. let active: BalanceOf = weight.try_into().map_err(|_| ()).unwrap(); @@ -1277,8 +1279,10 @@ impl VoteWeightProvider for Pallet { /// does not provided nominators in sorted ordered. If you desire nominators in a sorted order take /// a look at [`pallet-bags-list]. pub struct UseNominatorsMap(sp_std::marker::PhantomData); + impl SortedListProvider for UseNominatorsMap { type Error = (); + type Value = VoteWeight; /// Returns iterator over voter list, which can have `take` called on it. fn iter() -> Box> { @@ -1290,11 +1294,11 @@ impl SortedListProvider for UseNominatorsMap { fn contains(id: &T::AccountId) -> bool { Nominators::::contains_key(id) } - fn on_insert(_: T::AccountId, _weight: VoteWeight) -> Result<(), Self::Error> { + fn on_insert(_: T::AccountId, _weight: Self::Value) -> Result<(), Self::Error> { // nothing to do on insert. Ok(()) } - fn on_update(_: &T::AccountId, _weight: VoteWeight) { + fn on_update(_: &T::AccountId, _weight: Self::Value) { // nothing to do on update. } fn on_remove(_: &T::AccountId) { @@ -1302,7 +1306,7 @@ impl SortedListProvider for UseNominatorsMap { } fn unsafe_regenerate( _: impl IntoIterator, - _: Box VoteWeight>, + _: Box Self::Value>, ) -> u32 { // nothing to do upon regenerate. 0 @@ -1316,4 +1320,8 @@ impl SortedListProvider for UseNominatorsMap { // condition of SortedListProvider::unsafe_clear. Nominators::::remove_all(); } + + fn weight_update_worst_case(_: &T::AccountId, _: bool) -> Self::Value { + u64::MAX + } } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 58f9fd237263b..473e23a697c9e 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -166,7 +166,7 @@ pub mod pallet { /// Something that can provide a sorted list of voters in a somewhat sorted way. The /// original use case for this was designed with `pallet_bags_list::Pallet` in mind. If /// the bags-list is not desired, [`impls::UseNominatorsMap`] is likely the desired option. - type SortedListProvider: SortedListProvider; + type SortedListProvider: SortedListProvider; /// The maximum number of `unlocking` chunks a [`StakingLedger`] can have. Effectively /// determines how many unique eras a staker may be unbonding in. From be54ba932f56df5b45f77f474ad81fa6912badcb Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 13:40:37 +0000 Subject: [PATCH 06/14] Respond to some feedback --- frame/bags-list/src/list/mod.rs | 9 ++++---- frame/bags-list/src/list/tests.rs | 38 +++++++++++++++---------------- frame/staking/src/pallet/impls.rs | 1 + frame/staking/src/pallet/mod.rs | 2 +- 4 files changed, 26 insertions(+), 24 deletions(-) diff --git a/frame/bags-list/src/list/mod.rs b/frame/bags-list/src/list/mod.rs index 1e284fe6a82b6..591dc86a3d9b0 100644 --- a/frame/bags-list/src/list/mod.rs +++ b/frame/bags-list/src/list/mod.rs @@ -564,7 +564,7 @@ pub struct Bag, I: 'static = ()> { #[codec(skip)] bag_upper: T::Value, #[codec(skip)] - phantom: PhantomData, + _phantom: PhantomData, } impl, I: 'static> Bag { @@ -574,7 +574,7 @@ impl, I: 'static> Bag { tail: Option, bag_upper: T::Value, ) -> Self { - Self { head, tail, bag_upper, phantom: PhantomData } + Self { head, tail, bag_upper, _phantom: PhantomData } } /// Get a bag by its upper value. @@ -636,7 +636,7 @@ impl, I: 'static> Bag { prev: None, next: None, bag_upper: Zero::zero(), - phantom: PhantomData, + _phantom: PhantomData, }); } @@ -773,7 +773,8 @@ pub struct Node, I: 'static = ()> { prev: Option, next: Option, bag_upper: T::Value, - phantom: PhantomData, + #[codec(skip)] + _phantom: PhantomData, } impl, I: 'static> Node { diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index 72ac20a51034b..8df3a43a7b4da 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -32,7 +32,7 @@ fn basic_setup_works() { prev, next, bag_upper, - phantom: PhantomData, + _phantom: PhantomData, }; assert_eq!(ListNodes::::count(), 4); @@ -44,11 +44,11 @@ fn basic_setup_works() { // the state of the bags is as expected assert_eq!( ListBags::::get(10).unwrap(), - Bag:: { head: Some(1), tail: Some(1), bag_upper: 0, phantom: PhantomData } + Bag:: { head: Some(1), tail: Some(1), bag_upper: 0, _phantom: PhantomData } ); assert_eq!( ListBags::::get(1_000).unwrap(), - Bag:: { head: Some(2), tail: Some(4), bag_upper: 0, phantom: PhantomData } + Bag:: { head: Some(2), tail: Some(4), bag_upper: 0, _phantom: PhantomData } ); assert_eq!(ListNodes::::get(2).unwrap(), node(2, None, Some(3), 1_000)); @@ -398,14 +398,14 @@ mod list { prev: None, next: None, bag_upper: 15, - phantom: PhantomData, + _phantom: PhantomData, }; let node_11_no_bag = Node:: { id: 11, prev: None, next: None, bag_upper: 15, - phantom: PhantomData, + _phantom: PhantomData, }; // given @@ -436,7 +436,7 @@ mod list { prev: Some(1), next: Some(2), bag_upper: 1_000, - phantom: PhantomData, + _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -465,7 +465,7 @@ mod list { prev: Some(4), next: None, bag_upper: 1_000, - phantom: PhantomData, + _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -494,7 +494,7 @@ mod list { prev: None, next: Some(2), bag_upper: 1_000, - phantom: PhantomData, + _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -523,7 +523,7 @@ mod list { prev: Some(42), next: Some(42), bag_upper: 1_000, - phantom: PhantomData, + _phantom: PhantomData, }; assert!(!crate::ListNodes::::contains_key(42)); @@ -551,7 +551,7 @@ mod bags { let bag = Bag::::get(bag_upper).unwrap(); let bag_ids = bag.iter().map(|n| *n.id()).collect::>(); - assert_eq!(bag, Bag:: { head, tail, bag_upper, phantom: PhantomData }); + assert_eq!(bag, Bag:: { head, tail, bag_upper, _phantom: PhantomData }); assert_eq!(bag_ids, ids); }; @@ -587,7 +587,7 @@ mod bags { prev: None, next: None, bag_upper, - phantom: PhantomData, + _phantom: PhantomData, }; assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); @@ -597,7 +597,7 @@ mod bags { assert_eq!( ListNodes::::get(&42).unwrap(), - Node { bag_upper: 10, prev: Some(1), next: None, id: 42, phantom: PhantomData } + Node { bag_upper: 10, prev: Some(1), next: None, id: 42, _phantom: PhantomData } ); }); } @@ -610,7 +610,7 @@ mod bags { prev: None, next: None, bag_upper, - phantom: PhantomData, + _phantom: PhantomData, }; // when inserting into a bag with 1 node @@ -637,7 +637,7 @@ mod bags { prev: Some(21), next: Some(101), bag_upper: 20, - phantom: PhantomData, + _phantom: PhantomData, }; bag_20.insert_node_unchecked(node_61); // then ids are in order @@ -650,7 +650,7 @@ mod bags { prev: Some(62), next: None, bag_upper: 20, - phantom: PhantomData, + _phantom: PhantomData, } ); @@ -671,7 +671,7 @@ mod bags { prev, next, bag_upper, - phantom: PhantomData, + _phantom: PhantomData, }; ExtBuilder::default().build_and_execute_no_post_check(|| { // when inserting a node with both prev & next pointing at an account in an incorrect @@ -727,7 +727,7 @@ mod bags { assert_eq!( bag_1000, - Bag { head: Some(2), tail: Some(2), bag_upper: 1_000, phantom: PhantomData } + Bag { head: Some(2), tail: Some(2), bag_upper: 1_000, _phantom: PhantomData } ) }); } @@ -745,7 +745,7 @@ mod bags { prev, next, bag_upper, - phantom: PhantomData, + _phantom: PhantomData, }; // given @@ -878,7 +878,7 @@ mod bags { prev: None, next: Some(3), bag_upper: 10, // should be 1_000 - phantom: PhantomData, + _phantom: PhantomData, }; let mut bag_1000 = Bag::::get(1_000).unwrap(); diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 9e5f35524c417..cb17f9fc5c058 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1321,6 +1321,7 @@ impl SortedListProvider for UseNominatorsMap { Nominators::::remove_all(); } + #[cfg(feature = "runtime-benchmarks")] fn weight_update_worst_case(_: &T::AccountId, _: bool) -> Self::Value { u64::MAX } diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 473e23a697c9e..faacc92313592 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -166,7 +166,7 @@ pub mod pallet { /// Something that can provide a sorted list of voters in a somewhat sorted way. The /// original use case for this was designed with `pallet_bags_list::Pallet` in mind. If /// the bags-list is not desired, [`impls::UseNominatorsMap`] is likely the desired option. - type SortedListProvider: SortedListProvider; + type SortedListProvider: SortedListProvider; /// The maximum number of `unlocking` chunks a [`StakingLedger`] can have. Effectively /// determines how many unique eras a staker may be unbonding in. From a30a27f77836cdcd4c1b0a2e8ee4ee0a279c053a Mon Sep 17 00:00:00 2001 From: Zeke Mostov Date: Wed, 9 Mar 2022 13:46:14 +0000 Subject: [PATCH 07/14] Apply suggestions from code review Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> --- frame/staking/src/pallet/impls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index cb17f9fc5c058..b37c464f47d9a 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1323,6 +1323,6 @@ impl SortedListProvider for UseNominatorsMap { #[cfg(feature = "runtime-benchmarks")] fn weight_update_worst_case(_: &T::AccountId, _: bool) -> Self::Value { - u64::MAX + VoteWeight::MAX } } From b6af0ab35648f3f16403dc831ac4a1e98d9d773a Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 14:05:34 +0000 Subject: [PATCH 08/14] Add back default impl for weight update worst case --- frame/bags-list/src/mock.rs | 17 +++++++++-------- frame/election-provider-support/Cargo.toml | 2 +- frame/election-provider-support/src/lib.rs | 7 +++++-- frame/staking/src/pallet/impls.rs | 5 ----- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/frame/bags-list/src/mock.rs b/frame/bags-list/src/mock.rs index 5c14236b53461..5d77814c76451 100644 --- a/frame/bags-list/src/mock.rs +++ b/frame/bags-list/src/mock.rs @@ -21,19 +21,20 @@ use super::*; use crate::{self as bags_list}; use frame_support::parameter_types; use std::collections::HashMap; +use frame_election_provider_support::VoteWeight; pub type AccountId = u32; pub type Balance = u32; parameter_types! { // Set the vote weight for any id who's weight has _not_ been set with `set_value_of`. - pub static NextVoteWeight: u64 = 0; - pub static NextVoteWeightMap: HashMap = Default::default(); + pub static NextVoteWeight: VoteWeight = 0; + pub static NextVoteWeightMap: HashMap = Default::default(); } pub struct StakingMock; impl frame_election_provider_support::ValueProvider for StakingMock { - type Value = u64; + type Value = VoteWeight; fn value(id: &AccountId) -> Self::Value { *NextVoteWeightMap::get().get(id).unwrap_or(&NextVoteWeight::get()) @@ -73,7 +74,7 @@ impl frame_system::Config for Runtime { } parameter_types! { - pub static BagThresholds: &'static [u64] = &[10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; + pub static BagThresholds: &'static [VoteWeight] = &[10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]; } impl bags_list::Config for Runtime { @@ -81,7 +82,7 @@ impl bags_list::Config for Runtime { type WeightInfo = (); type BagThresholds = BagThresholds; type ValueProvider = StakingMock; - type Value = u64; + type Value = VoteWeight; } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; @@ -98,11 +99,11 @@ frame_support::construct_runtime!( ); /// Default AccountIds and their weights. -pub(crate) const GENESIS_IDS: [(AccountId, u64); 4] = [(1, 10), (2, 1_000), (3, 1_000), (4, 1_000)]; +pub(crate) const GENESIS_IDS: [(AccountId, VoteWeight); 4] = [(1, 10), (2, 1_000), (3, 1_000), (4, 1_000)]; #[derive(Default)] pub struct ExtBuilder { - ids: Vec<(AccountId, u64)>, + ids: Vec<(AccountId, VoteWeight)>, skip_genesis_ids: bool, } @@ -116,7 +117,7 @@ impl ExtBuilder { /// Add some AccountIds to insert into `List`. #[cfg(test)] - pub(crate) fn add_ids(mut self, ids: Vec<(AccountId, u64)>) -> Self { + pub(crate) fn add_ids(mut self, ids: Vec<(AccountId, VoteWeight)>) -> Self { self.ids = ids; self } diff --git a/frame/election-provider-support/Cargo.toml b/frame/election-provider-support/Cargo.toml index e66774c6b5e71..b95bd994fa70a 100644 --- a/frame/election-provider-support/Cargo.toml +++ b/frame/election-provider-support/Cargo.toml @@ -18,12 +18,12 @@ scale-info = { version = "2.0.1", default-features = false, features = ["derive" sp-std = { version = "4.0.0", default-features = false, path = "../../primitives/std" } sp-arithmetic = { version = "5.0.0", default-features = false, path = "../../primitives/arithmetic" } sp-npos-elections = { version = "4.0.0-dev", default-features = false, path = "../../primitives/npos-elections" } +sp-runtime = { version = "6.0.0", default-features = false, path = "../../primitives/runtime" } frame-support = { version = "4.0.0-dev", default-features = false, path = "../support" } frame-system = { version = "4.0.0-dev", default-features = false, path = "../system" } [dev-dependencies] sp-npos-elections = { version = "4.0.0-dev", path = "../../primitives/npos-elections" } -sp-runtime = { version = "6.0.0", path = "../../primitives/runtime" } sp-core = { version = "6.0.0", path = "../../primitives/core" } sp-io = { version = "6.0.0", path = "../../primitives/io" } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index b29606f7651f2..afbebf0c8bb32 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -169,6 +169,7 @@ pub mod onchain; use frame_support::{traits::Get, BoundedVec}; use sp_std::{fmt::Debug, prelude::*}; +use sp_runtime::traits::Bounded; /// Re-export some type as they are used in the interface. pub use sp_arithmetic::PerThing; @@ -343,7 +344,7 @@ pub trait SortedListProvider { /// The list's error type. type Error: sp_std::fmt::Debug; - type Value; + type Value: Bounded; /// An iterator over the list, which can have `take` called on it. fn iter() -> Box>; @@ -390,7 +391,9 @@ pub trait SortedListProvider { /// If `who` changes by the returned amount they are guaranteed to have a worst case change /// in their list position. #[cfg(feature = "runtime-benchmarks")] - fn weight_update_worst_case(_who: &AccountId, _is_increase: bool) -> Self::Value; + fn weight_update_worst_case(_who: &AccountId, _is_increase: bool) -> Self::Value { + Self::Value::max_value() + } } /// Something that can provide the `VoteWeight` of an account. Similar to [`ElectionProvider`] and diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index cb17f9fc5c058..2baaef91fd972 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1320,9 +1320,4 @@ impl SortedListProvider for UseNominatorsMap { // condition of SortedListProvider::unsafe_clear. Nominators::::remove_all(); } - - #[cfg(feature = "runtime-benchmarks")] - fn weight_update_worst_case(_: &T::AccountId, _: bool) -> Self::Value { - u64::MAX - } } From 43af39b89faa3c57aa6ef0f481b991b4dbcddd5d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 15:09:24 +0000 Subject: [PATCH 09/14] Update to Score in more places' --- frame/bags-list/src/tests.rs | 15 ++++++++------- frame/staking/src/mock.rs | 5 +++-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index 5030a12fac02a..7c3238fae1d59 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -21,6 +21,7 @@ use super::*; use frame_election_provider_support::SortedListProvider; use list::Bag; use mock::{test_utils::*, *}; +use frame_election_provider_support::VoteWeight; mod pallet { use super::*; @@ -166,7 +167,7 @@ mod pallet { #[test] #[should_panic = "thresholds must strictly increase, and have no duplicates"] fn duplicate_in_bags_threshold_panics() { - const DUPE_THRESH: &[u64; 4] = &[10, 20, 30, 30]; + const DUPE_THRESH: &[VoteWeight; 4] = &[10, 20, 30, 30]; BagThresholds::set(DUPE_THRESH); BagsList::integrity_test(); } @@ -174,7 +175,7 @@ mod pallet { #[test] #[should_panic = "thresholds must strictly increase, and have no duplicates"] fn decreasing_in_bags_threshold_panics() { - const DECREASING_THRESH: &[u64; 4] = &[10, 30, 20, 40]; + const DECREASING_THRESH: &[VoteWeight; 4] = &[10, 30, 20, 40]; BagThresholds::set(DECREASING_THRESH); BagsList::integrity_test(); } @@ -185,12 +186,12 @@ mod pallet { ExtBuilder::default().build_and_execute(|| { // everyone in the same bag. - assert_eq!(List::::get_bags(), vec![(u64::MAX, vec![1, 2, 3, 4])]); + assert_eq!(List::::get_bags(), vec![(VoteWeight::MAX, vec![1, 2, 3, 4])]); // any insertion goes there as well. assert_ok!(List::::insert(5, 999)); assert_ok!(List::::insert(6, 0)); - assert_eq!(List::::get_bags(), vec![(u64::MAX, vec![1, 2, 3, 4, 5, 6])]); + assert_eq!(List::::get_bags(), vec![(VoteWeight::MAX, vec![1, 2, 3, 4, 5, 6])]); // any rebag is noop. assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 1).is_ok())); @@ -472,7 +473,7 @@ mod sorted_list_provider { assert_eq!(BagsList::count(), 4); // when updating - BagsList::on_update(&201, u64::MAX); + BagsList::on_update(&201, VoteWeight::MAX); // then the count stays the same assert_eq!(BagsList::count(), 4); }); @@ -553,12 +554,12 @@ mod sorted_list_provider { 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, u64::MAX); + BagsList::on_update(&42, VoteWeight::MAX); // the the new bag is created with the id in it, assert_eq!( List::::get_bags(), - vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (u64::MAX, vec![42])] + vec![(10, vec![1]), (1_000, vec![2, 3, 4]), (VoteWeight::MAX, vec![42])] ); // and the id position is updated in the list. assert_eq!(BagsList::iter().collect::>(), vec![42, 2, 3, 4, 1]); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 0088e2d6bff5e..91d87e5658f2a 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -36,6 +36,7 @@ use sp_runtime::{ }; use sp_staking::offence::{DisableStrategy, OffenceDetails, OnOffenceHandler}; use std::cell::RefCell; +use frame_election_provider_support::VoteWeight; pub const INIT_TIMESTAMP: u64 = 30_000; pub const BLOCK_TIME: u64 = 1000; @@ -240,9 +241,9 @@ parameter_types! { impl pallet_bags_list::Config for Test { type Event = Event; type WeightInfo = (); - type ValueProvider = Staking; + type ScoreProvider = Staking; type BagThresholds = BagThresholds; - type Value = u64; + type Score = VoteWeight; } impl onchain::Config for Test { From 38500e16f567b1b5609055e3428f34c8ff4e3720 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 15:18:22 +0000 Subject: [PATCH 10/14] Use VoteWeight, not u64 to reduce test diff --- frame/bags-list/src/list/tests.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index ec31a2902d6f3..123b4e7ed098c 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -22,6 +22,7 @@ use crate::{ }; use frame_election_provider_support::SortedListProvider; use frame_support::{assert_ok, assert_storage_noop}; +use frame_election_provider_support::VoteWeight; #[test] fn basic_setup_works() { @@ -84,11 +85,11 @@ fn notional_bag_for_works() { assert_eq!(max_explicit_threshold, 10_000); // if the max explicit threshold is less than T::Value::max_value(), - assert!(u64::MAX > max_explicit_threshold); + assert!(VoteWeight::MAX > max_explicit_threshold); // then anything above it will belong to the T::Value::max_value() bag. assert_eq!(notional_bag_for::(max_explicit_threshold), max_explicit_threshold); - assert_eq!(notional_bag_for::(max_explicit_threshold + 1), u64::MAX); + assert_eq!(notional_bag_for::(max_explicit_threshold + 1), VoteWeight::MAX); } #[test] @@ -132,7 +133,7 @@ fn migrate_works() { assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]); // when the new thresholds adds `15` and removes `2_000` - const NEW_THRESHOLDS: &'static [u64] = &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; + const NEW_THRESHOLDS: &'static [VoteWeight] = &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; BagThresholds::set(NEW_THRESHOLDS); // and we call List::::migrate(old_thresholds); @@ -564,7 +565,7 @@ mod bags { // and all other bag thresholds don't get bags. ::BagThresholds::get() .iter() - .chain(iter::once(&u64::MAX)) + .chain(iter::once(&VoteWeight::MAX)) .filter(|bag_upper| !vec![10, 1_000].contains(bag_upper)) .for_each(|bag_upper| { assert_storage_noop!(assert_eq!(Bag::::get(*bag_upper), None)); From b0365a804cf39127b1dcdfc8cc0a2822f794032d Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 15:19:15 +0000 Subject: [PATCH 11/14] FMT --- frame/bags-list/src/lib.rs | 8 ++++---- frame/bags-list/src/list/tests.rs | 6 +++--- frame/bags-list/src/tests.rs | 8 +++++--- frame/staking/src/mock.rs | 3 +-- frame/staking/src/pallet/impls.rs | 4 ++-- frame/staking/src/pallet/mod.rs | 5 ++++- 6 files changed, 19 insertions(+), 15 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 5ef39be5b53e5..86b134cfd04ae 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -47,14 +47,14 @@ //! it will worsen its position in list iteration; this reduces incentives for some types of spam //! that involve consistently removing and inserting for better position. Further, ordering //! granularity is thus dictated by range between each bag threshold. -//! - if an item's score changes to a value no longer within the range of its current bag the -//! item's position will need to be updated by an external actor with rebag (update), or removal -//! and insertion. +//! - if an item's score changes to a value no longer within the range of its current bag the item's +//! position will need to be updated by an external actor with rebag (update), or removal and +//! insertion. #![cfg_attr(not(feature = "std"), no_std)] use codec::{Codec, FullCodec}; -use frame_election_provider_support::{SortedListProvider, ScoreProvider}; +use frame_election_provider_support::{ScoreProvider, SortedListProvider}; use frame_system::ensure_signed; use sp_runtime::traits::{AtLeast32BitUnsigned, Bounded}; use sp_std::prelude::*; diff --git a/frame/bags-list/src/list/tests.rs b/frame/bags-list/src/list/tests.rs index 123b4e7ed098c..9b7a078b44284 100644 --- a/frame/bags-list/src/list/tests.rs +++ b/frame/bags-list/src/list/tests.rs @@ -20,9 +20,8 @@ use crate::{ mock::{test_utils::*, *}, ListBags, ListNodes, }; -use frame_election_provider_support::SortedListProvider; +use frame_election_provider_support::{SortedListProvider, VoteWeight}; use frame_support::{assert_ok, assert_storage_noop}; -use frame_election_provider_support::VoteWeight; #[test] fn basic_setup_works() { @@ -133,7 +132,8 @@ fn migrate_works() { assert_eq!(old_thresholds, vec![10, 20, 30, 40, 50, 60, 1_000, 2_000, 10_000]); // when the new thresholds adds `15` and removes `2_000` - const NEW_THRESHOLDS: &'static [VoteWeight] = &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; + const NEW_THRESHOLDS: &'static [VoteWeight] = + &[10, 15, 20, 30, 40, 50, 60, 1_000, 10_000]; BagThresholds::set(NEW_THRESHOLDS); // and we call List::::migrate(old_thresholds); diff --git a/frame/bags-list/src/tests.rs b/frame/bags-list/src/tests.rs index 7c3238fae1d59..99396c9cbb3e3 100644 --- a/frame/bags-list/src/tests.rs +++ b/frame/bags-list/src/tests.rs @@ -18,10 +18,9 @@ use frame_support::{assert_noop, assert_ok, assert_storage_noop, traits::IntegrityTest}; use super::*; -use frame_election_provider_support::SortedListProvider; +use frame_election_provider_support::{SortedListProvider, VoteWeight}; use list::Bag; use mock::{test_utils::*, *}; -use frame_election_provider_support::VoteWeight; mod pallet { use super::*; @@ -191,7 +190,10 @@ mod pallet { // any insertion goes there as well. assert_ok!(List::::insert(5, 999)); assert_ok!(List::::insert(6, 0)); - assert_eq!(List::::get_bags(), vec![(VoteWeight::MAX, vec![1, 2, 3, 4, 5, 6])]); + assert_eq!( + List::::get_bags(), + vec![(VoteWeight::MAX, vec![1, 2, 3, 4, 5, 6])] + ); // any rebag is noop. assert_storage_noop!(assert!(BagsList::rebag(Origin::signed(0), 1).is_ok())); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 91d87e5658f2a..843791a46ade2 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -18,7 +18,7 @@ //! Test utilities use crate::{self as pallet_staking, *}; -use frame_election_provider_support::{onchain, SortedListProvider}; +use frame_election_provider_support::{onchain, SortedListProvider, VoteWeight}; use frame_support::{ assert_ok, parameter_types, traits::{ @@ -36,7 +36,6 @@ use sp_runtime::{ }; use sp_staking::offence::{DisableStrategy, OffenceDetails, OnOffenceHandler}; use std::cell::RefCell; -use frame_election_provider_support::VoteWeight; pub const INIT_TIMESTAMP: u64 = 30_000; pub const BLOCK_TIME: u64 = 1000; diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index aacaa2a531a1b..360db6170084e 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -18,8 +18,8 @@ //! Implementations for the Staking FRAME Pallet. use frame_election_provider_support::{ - data_provider, ElectionDataProvider, ElectionProvider, SortedListProvider, Supports, - ScoreProvider, VoteWeight, VoterOf, + data_provider, ElectionDataProvider, ElectionProvider, ScoreProvider, SortedListProvider, + Supports, VoteWeight, VoterOf, }; use frame_support::{ pallet_prelude::*, diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index d443caf4f5ab1..b50ce5ed4502c 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -166,7 +166,10 @@ pub mod pallet { /// Something that can provide a sorted list of voters in a somewhat sorted way. The /// original use case for this was designed with `pallet_bags_list::Pallet` in mind. If /// the bags-list is not desired, [`impls::UseNominatorsMap`] is likely the desired option. - type SortedListProvider: SortedListProvider; + type SortedListProvider: SortedListProvider< + Self::AccountId, + Score = frame_election_provider_support::VoteWeight, + >; /// The maximum number of `unlocking` chunks a [`StakingLedger`] can have. Effectively /// determines how many unique eras a staker may be unbonding in. From 1c4bae258b9293fa2ee726985a32efb2691c1579 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 15:22:51 +0000 Subject: [PATCH 12/14] FullCodec implies Codec --- frame/bags-list/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frame/bags-list/src/lib.rs b/frame/bags-list/src/lib.rs index 86b134cfd04ae..c502245409fdb 100644 --- a/frame/bags-list/src/lib.rs +++ b/frame/bags-list/src/lib.rs @@ -53,7 +53,7 @@ #![cfg_attr(not(feature = "std"), no_std)] -use codec::{Codec, FullCodec}; +use codec::FullCodec; use frame_election_provider_support::{ScoreProvider, SortedListProvider}; use frame_system::ensure_signed; use sp_runtime::traits::{AtLeast32BitUnsigned, Bounded}; @@ -166,7 +166,6 @@ pub mod pallet { + AtLeast32BitUnsigned + Bounded + TypeInfo - + Codec + FullCodec + MaxEncodedLen; } From 62170c3d3a527df459eeeca63abd8790dddcefe5 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 15:27:43 +0000 Subject: [PATCH 13/14] formatting --- frame/staking/src/pallet/impls.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 360db6170084e..a4aadb16ab1b9 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1279,7 +1279,6 @@ impl ScoreProvider for Pallet { /// does not provided nominators in sorted ordered. If you desire nominators in a sorted order take /// a look at [`pallet-bags-list]. pub struct UseNominatorsMap(sp_std::marker::PhantomData); - impl SortedListProvider for UseNominatorsMap { type Error = (); type Score = VoteWeight; From 37f77349325b77ab5c8daea7a69894177b950499 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Wed, 9 Mar 2022 15:53:09 +0000 Subject: [PATCH 14/14] Fixup bags list remote test --- frame/bags-list/remote-tests/src/lib.rs | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/frame/bags-list/remote-tests/src/lib.rs b/frame/bags-list/remote-tests/src/lib.rs index 32686a63858ce..83c322f93134e 100644 --- a/frame/bags-list/remote-tests/src/lib.rs +++ b/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(currency_unit: u64, currency_na let mut rebaggable = 0; let mut active_bags = 0; for vote_weight_thresh in ::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::::list_bags_get(*vote_weight_thresh) { @@ -70,9 +75,13 @@ pub fn display_and_check_bags(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::::weight_of(&id); + let vote_weight = ::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 = - 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(currency_unit: u64, currency_na pallet_bags_list::Node::::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::(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::(vote_weight) as f64 / - currency_unit as f64 + notional_bag_as_u64 as f64 / currency_unit as f64 ); } }