-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[Feature] StakeTracker P1 - VotersList #13079
Changes from all commits
2f0a914
29e1f66
71ad9ab
c4c5248
58bfb70
66d6f5a
0de90d4
ad54a95
3b1d926
16bd6b1
938c4df
74a2a48
d829b91
2ea7d05
1344c24
32f50d2
c3a7bd6
4ce30ed
2d28217
44b318f
a34aaab
8838a07
7d36dc9
3446f0e
ead8b81
387b7b4
9e561e9
363eb6f
2c6275e
fc14202
a0db07f
978076a
504695e
5361a95
78094f2
7925732
561eb2b
1fffe7f
a1a27d7
ad60934
2002537
6108362
f92dfb7
533703a
d6069c3
edbcf89
16671da
aff4599
f15bd1c
85e863d
3b816b8
676a7a9
54a5a6a
7e042ac
3295a29
3f6b5b9
489b71c
aec8879
6bf3fcb
c049653
c987805
e6676f7
e3ae48f
06cd68c
391b0b4
7780763
2d1fe57
435d72b
501243b
21a90ce
8360474
355145e
84f8d8f
17d0fbb
c2d8ad4
3f286e1
0246d7b
41b3c29
d1b783c
e12473f
5736950
7d136f9
9332d75
f143653
605816d
66cd197
a7250dd
0cb3222
d79ac49
9eb705a
6a305f8
20f02e9
9f28619
8bbfd2b
f8dabaa
32509ad
ef6bbc3
69e70f9
9ad762b
03154ec
ecf831e
4fd1c49
78d8d6f
855100a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -476,10 +476,10 @@ where | |
| /// This is generic over `AccountId` and it can represent a validator, a nominator, or any other | ||
| /// entity. | ||
| /// | ||
| /// The scores (see [`Self::Score`]) are ascending, the higher, the better. | ||
| /// | ||
| /// Something that implements this trait will do a best-effort sort over ids, and thus can be | ||
| /// used on the implementing side of [`ElectionDataProvider`]. | ||
| /// | ||
| /// The scores (see [`Self::Score`]) are ascending, the higher, the better. | ||
| pub trait SortedListProvider<AccountId> { | ||
| /// The list's error type. | ||
| type Error: sp_std::fmt::Debug; | ||
|
|
@@ -501,6 +501,13 @@ pub trait SortedListProvider<AccountId> { | |
| /// Return true if the list already contains `id`. | ||
| fn contains(id: &AccountId) -> bool; | ||
|
|
||
| /// Get the score of `id`. | ||
| fn get_score(id: &AccountId) -> Result<Self::Score, Self::Error>; | ||
|
|
||
| /// Check internal state of list. Only meant for debugging. | ||
| #[cfg(feature = "try-runtime")] | ||
| fn try_state() -> Result<(), &'static str>; | ||
|
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 should be feature gated by
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. The thing is that this was ported from the original SortedListProvider and it definitely was not feature-gated there. And I've seen more occurrences of that in the codebase, perhaps in BagsList.
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. Yep, it was missing all around bags-list. So now that's feature-gated it breaks a bunch of things. Going to be fixed as part of this PR.
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. We should probably have fn try_state() in a separate trait and then |
||
|
|
||
| /// Hook for inserting a new id. | ||
| /// | ||
| /// Implementation should return an error if duplicate item is being inserted. | ||
|
|
@@ -513,9 +520,6 @@ pub trait SortedListProvider<AccountId> { | |
| /// Returns `Ok(())` iff it successfully updates an item, an `Err(_)` otherwise. | ||
| fn on_update(id: &AccountId, score: Self::Score) -> Result<(), Self::Error>; | ||
|
|
||
| /// Get the score of `id`. | ||
| fn get_score(id: &AccountId) -> Result<Self::Score, Self::Error>; | ||
|
|
||
| /// Same as `on_update`, but incorporate some increased score. | ||
| fn on_increase(id: &AccountId, additional: Self::Score) -> Result<(), Self::Error> { | ||
| let old_score = Self::get_score(id)?; | ||
|
|
@@ -560,16 +564,13 @@ pub trait SortedListProvider<AccountId> { | |
| /// | ||
| /// This function should never be called in production settings because it can lead to an | ||
| /// unbounded amount of storage accesses. | ||
| #[cfg(any(feature = "runtime-benchmarks", test))] | ||
| fn unsafe_clear(); | ||
|
|
||
| /// Check internal state of the list. Only meant for debugging. | ||
| #[cfg(feature = "try-runtime")] | ||
| fn try_state() -> Result<(), &'static str>; | ||
|
|
||
| /// 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 score_update_worst_case(_who: &AccountId, _is_increase: bool) -> Self::Score; | ||
| #[cfg(any(feature = "runtime-benchmarks", test))] | ||
| fn score_update_worst_case(who: &AccountId, is_increase: bool) -> Self::Score; | ||
| } | ||
|
|
||
| /// Something that can provide the `Score` of an account. Similar to [`ElectionProvider`] and | ||
|
|
@@ -675,3 +676,4 @@ pub type BoundedSupportsOf<E> = BoundedSupports< | |
|
|
||
| sp_core::generate_feature_enabled_macro!(runtime_benchmarks_enabled, feature = "runtime-benchmarks", $); | ||
| sp_core::generate_feature_enabled_macro!(runtime_benchmarks_or_fuzz_enabled, any(feature = "runtime-benchmarks", feature = "fuzzing"), $); | ||
| sp_core::generate_feature_enabled_macro!(runtime_benchmarks_or_test_enabled, any(feature = "runtime-benchmarks", test), $); | ||
kianenigma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,6 +123,13 @@ impl pallet_staking::Config for Runtime { | |
| type OnStakerSlash = Pools; | ||
| type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig; | ||
| type WeightInfo = (); | ||
| type EventListeners = StakeTracker; | ||
| } | ||
|
|
||
| impl pallet_stake_tracker::Config for Runtime { | ||
|
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. We can technically still use I think we should either remove the implementation, or for tests and occasions where it doesn't matter too much, use the old one, essentially not needing to add Although, frankly, I am now a bit confused myself about when to use which, if both are options.
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. Yes, we can use VoterList directly here. StakeTracker is needed, because without it benchmark tests fail, it has been introduced for a reason. |
||
| type Currency = Balances; | ||
| type Staking = Staking; | ||
| type VoterList = VoterList; | ||
| } | ||
|
|
||
| parameter_types! { | ||
|
|
@@ -188,6 +195,7 @@ frame_support::construct_runtime!( | |
| Staking: pallet_staking::{Pallet, Call, Config<T>, Storage, Event<T>}, | ||
| VoterList: pallet_bags_list::<Instance1>::{Pallet, Call, Storage, Event<T>}, | ||
| Pools: pallet_nomination_pools::{Pallet, Call, Storage, Event<T>}, | ||
| StakeTracker: pallet_stake_tracker::{Pallet, Storage}, | ||
| } | ||
| ); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the previous code seems more correct to me. This test is testing the re-generation of
pallet_staking::Config::VoterList. We don't care if it is backed by what..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this we'll need to move
unsafe_regenerateto ReadOnly interface, which I guess is far, since it's an unsafe method.