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 cc575d7d1efff..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/lib.rs b/frame/bags-list/src/lib.rs index 89c54db87023f..477494402231e 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,26 @@ 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 +185,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 +198,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 +213,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 +229,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 +239,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 +288,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 +305,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..b8b1b01f8a778 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<(T, I)>); -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,13 @@ 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 +168,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 +180,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 +201,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 +214,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 +268,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 +282,7 @@ impl List { id, weight, bag_weight, - crate::ListNodes::::count(), + crate::ListNodes::::count(), ); Ok(()) @@ -301,7 +303,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 +316,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 +343,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 +353,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 +367,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 +383,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 +405,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 +424,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 +441,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 +475,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 +491,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 +504,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 +533,8 @@ 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::>() } @@ -546,29 +549,32 @@ impl 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 { +pub struct Bag, I: 'static = ()> { head: Option, tail: Option, #[codec(skip)] bag_upper: VoteWeight, + + #[codec(skip)] + phantom: PhantomData, } -impl Bag { +impl, I: 'static> Bag { #[cfg(test)] pub(crate) fn new( head: Option, tail: Option, bag_upper: VoteWeight, ) -> Self { - Self { head, tail, bag_upper } + Self { head, tail, bag_upper, phantom: Default::default() } } /// 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 +582,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 +594,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 +626,13 @@ 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, + phantom: Default::default(), + }); } /// Insert a node into this bag. @@ -630,7 +642,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 +686,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 +747,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()) } @@ -749,24 +761,27 @@ impl 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 { +pub struct Node, I: 'static = ()> { id: T::AccountId, prev: Option, next: Option, bag_upper: VoteWeight, + + #[codec(skip)] + phantom: PhantomData, } -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 +805,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 +849,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/list/tests.rs b/frame/bags-list/src/list/tests.rs index aaa215b0af1ca..e8364a822c290 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 = |id, prev, next, bag_upper| Node:: { id, prev, next, bag_upper }; + let node = |id, prev, next, bag_upper| Node:: { + id, + prev, + next, + bag_upper, + phantom: PhantomData, + }; assert_eq!(ListNodes::::count(), 4); assert_eq!(ListNodes::::iter().count(), 4); @@ -38,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 } + 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 } + 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)); @@ -65,14 +71,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 +87,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 +394,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:: { 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:: { + id: 10, + prev: None, + next: None, + bag_upper: 15, + phantom: PhantomData, + }; + let node_11_no_bag = Node:: { + id: 11, + prev: None, + next: None, + bag_upper: 15, + phantom: PhantomData, + }; // given ListNodes::::insert(10, node_10_no_bag); @@ -414,8 +432,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:: { id: 42, prev: Some(1), next: Some(2), bag_upper: 1_000 }; + let node_42 = Node:: { + id: 42, + prev: Some(1), + next: Some(2), + bag_upper: 1_000, + phantom: PhantomData, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_1 = crate::ListNodes::::get(&1).unwrap(); @@ -438,7 +461,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:: { id: 42, prev: Some(4), next: None, bag_upper: 1_000 }; + let node_42 = Node:: { + id: 42, + prev: Some(4), + next: None, + bag_upper: 1_000, + phantom: PhantomData, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_2 = crate::ListNodes::::get(&2).unwrap(); @@ -461,7 +490,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:: { id: 42, prev: None, next: Some(2), bag_upper: 1_000 }; + let node_42 = Node:: { + id: 42, + prev: None, + next: Some(2), + bag_upper: 1_000, + phantom: PhantomData, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_3 = crate::ListNodes::::get(&3).unwrap(); @@ -484,8 +519,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:: { id: 42, prev: Some(42), next: Some(42), bag_upper: 1_000 }; + let node_42 = Node:: { + id: 42, + prev: Some(42), + next: Some(42), + bag_upper: 1_000, + phantom: PhantomData, + }; assert!(!crate::ListNodes::::contains_key(42)); let node_4 = crate::ListNodes::::get(&4).unwrap(); @@ -512,7 +552,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:: { head, tail, bag_upper, phantom: PhantomData }); assert_eq!(bag_ids, ids); }; @@ -543,7 +583,13 @@ 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:: { + id, + prev: None, + next: None, + bag_upper, + phantom: PhantomData, + }; assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])]); @@ -552,7 +598,7 @@ mod bags { assert_eq!( ListNodes::::get(&42).unwrap(), - Node { bag_upper: 10, prev: Some(1), next: None, id: 42 } + Node { id: 42, prev: Some(1), next: None, bag_upper: 10, phantom: PhantomData } ); }); } @@ -560,7 +606,13 @@ 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:: { + id, + prev: None, + next: None, + bag_upper, + phantom: PhantomData, + }; // when inserting into a bag with 1 node let mut bag_10 = Bag::::get(10).unwrap(); @@ -581,15 +633,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:: { id: 61, prev: Some(21), next: Some(101), bag_upper: 20 }; + let node_61 = Node:: { + 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 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:: { + id: 61, + prev: Some(62), + next: None, + bag_upper: 20, + phantom: PhantomData, + } ); // state of all bags is as expected @@ -604,12 +667,18 @@ 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 = |id, prev, next, bag_upper, phantom| Node:: { + id, + prev, + next, + bag_upper, + phantom, + }; 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(42, Some(1), Some(1), 500, PhantomData)); // then the proper prev and next is set. assert_eq!(bag_as_ids(&bag_1000), vec![2, 3, 4, 42]); @@ -617,7 +686,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(42, Some(4), None, bag_1000.bag_upper, PhantomData,) ); }); @@ -627,7 +696,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(3, None, None, bag_1000.bag_upper, PhantomData)); // 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 +706,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(3, Some(4), None, bag_1000.bag_upper, PhantomData) ); }); @@ -645,7 +714,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(2, None, None, 0, PhantomData)); // then all nodes after the head are lost assert_eq!(bag_as_ids(&bag_1000), vec![2]); @@ -653,11 +722,14 @@ 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(2, Some(4), None, bag_1000.bag_upper, PhantomData) ); // ^^^ 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 { head: Some(2), tail: Some(2), bag_upper: 1_000, phantom: PhantomData } + ) }); } @@ -669,7 +741,13 @@ 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 = |id, prev, next, bag_upper, phantom| Node:: { + id, + prev, + next, + bag_upper, + phantom, + }; // given assert_eq!(List::::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4])],); @@ -678,7 +756,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(4, None, None, bag_1000.bag_upper, PhantomData)); // panics in debug assert_eq!(bag_1000.iter().count(), 3); // in release we expect it to silently ignore the request. }); } @@ -801,6 +879,7 @@ mod bags { prev: None, next: Some(3), bag_upper: 10, // should be 1_000 + phantom: PhantomData, }; let mut bag_1000 = Bag::::get(1_000).unwrap();