-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Used CountedStorageMap in pallet-staking #10233
Changes from 7 commits
662fffe
48a4825
da2d7ec
31f7bd7
a8c2f16
01a605d
2c192d8
c2f7577
b332858
d771e4a
e449fe5
65d7d02
b9d0a88
9b21bef
d79d380
16d6a44
190ec8e
41f8dae
3a0500f
fac8882
bf1bc0a
36115cb
de685ff
d07414f
256ac95
fa56998
a437c17
b7e8047
da0f59f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,8 +72,8 @@ pub mod v7 { | |
| use super::*; | ||
|
|
||
| pub fn pre_migrate<T: Config>() -> Result<(), &'static str> { | ||
| assert!(CounterForValidators::<T>::get().is_zero(), "CounterForValidators already set."); | ||
| assert!(CounterForNominators::<T>::get().is_zero(), "CounterForNominators already set."); | ||
| assert!(Validators::<T>::count().is_zero(), "Counter for Validators already set."); | ||
| assert!(Nominators::<T>::count().is_zero(), "Counter for Nominators already set."); | ||
| assert!(StorageVersion::<T>::get() == Releases::V6_0_0); | ||
| Ok(()) | ||
| } | ||
|
|
@@ -83,9 +83,6 @@ pub mod v7 { | |
| let validator_count = Validators::<T>::iter().count() as u32; | ||
| let nominator_count = Nominators::<T>::iter().count() as u32; | ||
|
|
||
| CounterForValidators::<T>::put(validator_count); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is technically making this migration code WRONG. Either remove it completely, or make sure it is correct. we don't enforce this now, but a correct way would be to make sure this migration code stays correct over time. That is, it should have its own storage items and not depend on the pallet types. In this case, you need to create (mock) storage types via Or, we need some special If all of this is too much, with despair and sadness, I am also fine with just removing the migration code altogether. But the long term correct thing to do is as I said above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you help with the correct implementation details on how to write the migration code, I am still new to the Substrate repo.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can look into other use cases of and now you have
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is staking pallet using the pallet macro in v7 ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this question for me or @kianenigma
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it was already migrated. Then the very pedantic course of action would be to accept the prefix as the argument of the migration function, but I will be fine with skipping it since the migration code is already at risk to becoming outdated (since it depends on |
||
| CounterForNominators::<T>::put(nominator_count); | ||
|
|
||
| StorageVersion::<T>::put(Releases::V7_0_0); | ||
| log!(info, "Completed staking migration to Releases::V7_0_0"); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -665,8 +665,8 @@ impl<T: Config> Pallet<T> { | |
| maybe_max_len: Option<usize>, | ||
| ) -> Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)> { | ||
| let max_allowed_len = { | ||
| let nominator_count = CounterForNominators::<T>::get() as usize; | ||
| let validator_count = CounterForValidators::<T>::get() as usize; | ||
| let nominator_count = Nominators::<T>::count() as usize; | ||
| let validator_count = Validators::<T>::count() as usize; | ||
| let all_voter_count = validator_count.saturating_add(nominator_count); | ||
| maybe_max_len.unwrap_or(all_voter_count).min(all_voter_count) | ||
| }; | ||
|
|
@@ -765,18 +765,15 @@ impl<T: Config> Pallet<T> { | |
| } | ||
|
|
||
| /// This function will add a nominator to the `Nominators` storage map, | ||
| /// [`SortedListProvider`] and keep track of the `CounterForNominators`. | ||
| /// and [`SortedListProvider`]. | ||
| /// | ||
| /// If the nominator already exists, their nominations will be updated. | ||
| /// | ||
| /// NOTE: you must ALWAYS use this function to add nominator or update their targets. Any access | ||
| /// to `Nominators`, its counter, or `VoterList` outside of this function is almost certainly | ||
| /// to `Nominators` or `VoterList` outside of this function is almost certainly | ||
| /// wrong. | ||
| pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations<T::AccountId>) { | ||
| if !Nominators::<T>::contains_key(who) { | ||
| // maybe update the counter. | ||
| CounterForNominators::<T>::mutate(|x| x.saturating_inc()); | ||
|
|
||
| // maybe update sorted list. Error checking is defensive-only - this should never fail. | ||
| if T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who)).is_err() { | ||
| log!(warn, "attempt to insert duplicate nominator ({:#?})", who); | ||
|
|
@@ -790,53 +787,46 @@ impl<T: Config> Pallet<T> { | |
| } | ||
|
|
||
| /// This function will remove a nominator from the `Nominators` storage map, | ||
| /// [`SortedListProvider`] and keep track of the `CounterForNominators`. | ||
| /// and [`SortedListProvider`]. | ||
| /// | ||
| /// Returns true if `who` was removed from `Nominators`, otherwise false. | ||
| /// | ||
| /// NOTE: you must ALWAYS use this function to remove a nominator from the system. Any access to | ||
| /// `Nominators`, its counter, or `VoterList` outside of this function is almost certainly | ||
| /// `Nominators` or `VoterList` outside of this function is almost certainly | ||
| /// wrong. | ||
| pub fn do_remove_nominator(who: &T::AccountId) -> bool { | ||
| if Nominators::<T>::contains_key(who) { | ||
| Nominators::<T>::remove(who); | ||
| CounterForNominators::<T>::mutate(|x| x.saturating_dec()); | ||
| T::SortedListProvider::on_remove(who); | ||
| debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(())); | ||
| debug_assert_eq!(CounterForNominators::<T>::get(), T::SortedListProvider::count()); | ||
| debug_assert_eq!(Nominators::<T>::count(), T::SortedListProvider::count()); | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| /// This function will add a validator to the `Validators` storage map, and keep track of the | ||
| /// `CounterForValidators`. | ||
| /// This function will add a validator to the `Validators` storage map. | ||
| /// | ||
| /// If the validator already exists, their preferences will be updated. | ||
| /// | ||
| /// NOTE: you must ALWAYS use this function to add a validator to the system. Any access to | ||
| /// `Validators`, its counter, or `VoterList` outside of this function is almost certainly | ||
| /// `Validators` or `VoterList` outside of this function is almost certainly | ||
| /// wrong. | ||
| pub fn do_add_validator(who: &T::AccountId, prefs: ValidatorPrefs) { | ||
| if !Validators::<T>::contains_key(who) { | ||
| CounterForValidators::<T>::mutate(|x| x.saturating_inc()) | ||
| } | ||
| Validators::<T>::insert(who, prefs); | ||
| } | ||
|
|
||
| /// This function will remove a validator from the `Validators` storage map, | ||
| /// and keep track of the `CounterForValidators`. | ||
| /// This function will remove a validator from the `Validators` storage map. | ||
| /// | ||
| /// Returns true if `who` was removed from `Validators`, otherwise false. | ||
| /// | ||
| /// NOTE: you must ALWAYS use this function to remove a validator from the system. Any access to | ||
| /// `Validators`, its counter, or `VoterList` outside of this function is almost certainly | ||
| /// `Validators` or `VoterList` outside of this function is almost certainly | ||
| /// wrong. | ||
| pub fn do_remove_validator(who: &T::AccountId) -> bool { | ||
| if Validators::<T>::contains_key(who) { | ||
| Validators::<T>::remove(who); | ||
| CounterForValidators::<T>::mutate(|x| x.saturating_dec()); | ||
| true | ||
| } else { | ||
| false | ||
|
|
@@ -865,10 +855,10 @@ impl<T: Config> ElectionDataProvider<T::AccountId, BlockNumberFor<T>> for Pallet | |
| fn voters( | ||
| maybe_max_len: Option<usize>, | ||
| ) -> data_provider::Result<Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)>> { | ||
| debug_assert!(<Nominators<T>>::iter().count() as u32 == CounterForNominators::<T>::get()); | ||
| debug_assert!(<Validators<T>>::iter().count() as u32 == CounterForValidators::<T>::get()); | ||
| debug_assert!(<Nominators<T>>::iter().count() as u32 == Nominators::<T>::count()); | ||
|
||
| debug_assert!(<Validators<T>>::iter().count() as u32 == Validators::<T>::count()); | ||
| debug_assert_eq!( | ||
| CounterForNominators::<T>::get(), | ||
| Nominators::<T>::count(), | ||
| T::SortedListProvider::count(), | ||
| "voter_count must be accurate", | ||
| ); | ||
|
|
@@ -881,7 +871,7 @@ impl<T: Config> ElectionDataProvider<T::AccountId, BlockNumberFor<T>> for Pallet | |
| } | ||
|
|
||
| fn targets(maybe_max_len: Option<usize>) -> data_provider::Result<Vec<T::AccountId>> { | ||
| let target_count = CounterForValidators::<T>::get(); | ||
| let target_count = Validators::<T>::count(); | ||
|
|
||
| // We can't handle this case yet -- return an error. | ||
| if maybe_max_len.map_or(false, |max_len| target_count > max_len as u32) { | ||
|
|
@@ -969,8 +959,6 @@ impl<T: Config> ElectionDataProvider<T::AccountId, BlockNumberFor<T>> for Pallet | |
| <Ledger<T>>::remove_all(None); | ||
| <Validators<T>>::remove_all(None); | ||
| <Nominators<T>>::remove_all(None); | ||
| <CounterForNominators<T>>::kill(); | ||
| <CounterForValidators<T>>::kill(); | ||
| let _ = T::SortedListProvider::clear(None); | ||
| } | ||
|
|
||
|
|
@@ -1282,7 +1270,7 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseNominatorsMap<T> { | |
| Box::new(Nominators::<T>::iter().map(|(n, _)| n)) | ||
| } | ||
| fn count() -> u32 { | ||
| CounterForNominators::<T>::get() | ||
| Nominators::<T>::count() | ||
| } | ||
| fn contains(id: &T::AccountId) -> bool { | ||
| Nominators::<T>::contains_key(id) | ||
|
|
@@ -1308,12 +1296,9 @@ impl<T: Config> SortedListProvider<T::AccountId> for UseNominatorsMap<T> { | |
| Ok(()) | ||
| } | ||
| fn clear(maybe_count: Option<u32>) -> u32 { | ||
| let count_before = Nominators::<T>::count(); | ||
| Nominators::<T>::remove_all(maybe_count); | ||
| if let Some(count) = maybe_count { | ||
| CounterForNominators::<T>::mutate(|noms| *noms - count); | ||
| count | ||
| } else { | ||
| CounterForNominators::<T>::take() | ||
| } | ||
| let count_after = Nominators::<T>::count(); | ||
| count_before.saturating_sub(count_after) | ||
shawntabrizi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.