Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Rename BagsList to VoterList and Add Score #5463

Merged
merged 11 commits into from May 19, 2022
Merged

Rename BagsList to VoterList and Add Score #5463

merged 11 commits into from May 19, 2022

Conversation

shawntabrizi
Copy link
Contributor

@shawntabrizi shawntabrizi commented May 5, 2022 •
edited

Simple rename of BagsList to VoterList, and adding Score to the bags list pallet.

Part of: https://github.com/paritytech/substrate/pull/11013/files

Companion to: paritytech/substrate#11357

Tested with try-runtime on Polkadot:

2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Initializer    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Dmp    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Ump    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Hrmp    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for ParaSessionInfo    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for ParasDisputes    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Registrar    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Slots    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Auctions    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ✅ no migration for Crowdloan    
2022-05-06 16:05:29.242  INFO main runtime::frame-support: ⚠️ XcmPallet declares internal migrations (which *might* execute). On-chain `StorageVersion(0)` vs current storage version `StorageVersion(0)`    
2022-05-06 16:05:29.598  INFO main try-runtime::cli: proof: 000002c236c73798f7dfb580cd0a1853c0126e7f59e29af4bd45ba7927a58ced01ecafa6772a5b6e50364fdb567f8ba93eb69a636aa533f92b1762842dcbd9618401c45295d4174b1132e66e2351d60e10cf3522bb9669d02c8c3bfa28e89ac3324f07e44efa8f010000000002c236c73798f7dfb580cd0a1853c0126e7f59e29af4bd45ba7927a58ced0b00e244616c010b00e244616c010051017401000075010000760100007701000078010000790100007a0100007b0100007c0100007d0100007e0100007f010000800100008101000082010000830100008401000085010000860100008701000088010000890100008a0100008b0100008c0100008d0100008e0100008f010000900100009101000092010000930100009401000095010000960100009701000098010000990100009a0100009b0100009c0100009d0100009e0100009f010000a0010000a1010000a2010000a3010000a4010000a5010000a6010000a7010000a8010000a9010000aa010000ab010000ac010000ad010000ae010000af010000b0010000b1010000b2010000b3010000b4010000b5010000b6010000b7010000b8010000b9010000ba010000bb010000bc010000bd010000be010000bf010000c0010000c1010000c2010000c3010000c4010000c5010000c6010000c70100000000966d74f8027e07b43717b6876d97544fe0d71f...00bf010000c0010000c1010000c2010000c3010000fff7d044faccb1f0d6548503159882306f86575cc01371534deba9dc3d8c7d57012446d15d60c6db2a5da5fcf3af4183005db44151e35d96038ba15711b8ad0351017900f9d76fe38be8efd3ba8b48ac1793437b861295d816aae8084e9f7d3c7594b5c1203dc30a0000fff7d044faccb1f0d6548503159882306f86575cc01371534deba9dc3d8c7d570b81b1ef7b4e0a0b81b1ef7b4e0a005101e6000000e7000000e8000000e9000000ea000000eb000000ec000000ed000000ee000000ef000000f0000000f1000000f2000000f3000000f4000000f5000000f6000000f7000000f8000000f9000000fa000000fb000000fc000000fd000000fe000000ff000000000100000101000002010000030100000401000005010000060100000701000008010000090100000a0100000b0100000c0100000d0100000e0100000f010000100100001101000012010000130100001401000015010000160100001701000018010000190100001a0100001b0100001c0100001d0100001e0100001f010000200100002101000022010000230100002401000025010000260100002701000028010000290100002a0100002b0100002c0100002d0100002e0100002f01000030010000310100003201000033010000340100003501000036010000370100003801000039010000 / 170049 nodes    
2022-05-06 16:05:29.599  INFO main try-runtime::cli: proof size: 24.49 MB (25080.4638671875 KB) (25682395 bytes)    
2022-05-06 16:05:29.599  INFO main try-runtime::cli: compact proof size: 20.98 MB (21484.76171875 KB) (22000396 bytes)    
2022-05-06 16:05:29.599  INFO main try-runtime::cli: zstd-compressed compact proof 8.94 MB (9158.56640625 KB) (9378372 bytes)    
2022-05-06 16:05:29.850  INFO main try-runtime::cli: TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = 18446744073709551615, total weight = 2000000000000 (9223372.036854776)

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label May 5, 2022
@paritytech-ci paritytech-ci requested review from a team May 5, 2022 14:17
@shawntabrizi shawntabrizi changed the title Rename BagsList to VotersList Rename BagsList to VoterList May 5, 2022
@shawntabrizi shawntabrizi added this to In progress in Runtime via automation May 5, 2022
@DrW3RK
Copy link

DrW3RK commented May 6, 2022 •
edited

Naming it as "NominatorList" would be consistent with the terminology already introduced and avoids ambiguity IMO. Would also sound more intuitive to the end user of our Staking system.

intention to nominate: an account that has stated the intention to nominate; also called simply a "nominator".
Ref: https://wiki.polkadot.network/docs/learn-nominator#staking-election-stages

@kianenigma
Copy link
Contributor

Happy to name them NominatorList and ValidatorList for simplicity.

Runtime automation moved this from In progress to Needs Audit May 6, 2022
@kianenigma
Copy link
Contributor

Happy to name them NominatorList and ValidatorList for simplicity.

Happy to name them NominatorList and ValidatorList for simplicity.

I take it back: What you are suggesting to be called NominatorList is in principle a list of all nominators and validators (self-stake), so let's stick to the more generic Voter.

@DrW3RK
Copy link

DrW3RK commented May 6, 2022 •
edited

Makes sense. I could be mistaken, but I vaguely remember reading on plans to separate the votes from Validator self-stake from the bags-list on a PR? If that is the case, NominatorList would still be a valid name I think. Could also argue that with self-stake, the validators are nominating themselves and in that context they are still nominators :)

@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels May 6, 2022
@shawntabrizi shawntabrizi changed the title Rename BagsList to VoterList Rename BagsList to VoterList and Add Score May 6, 2022
@shawntabrizi shawntabrizi mentioned this pull request May 9, 2022
Merged
1 task
@kianenigma kianenigma added this to the v0.9.23 milestone May 12, 2022
@kianenigma
Copy link
Contributor

@paritytech/locks-review need some approves here.

bkchr
bkchr approved these changes May 19, 2022
View reviewed changes
runtime/kusama/src/lib.rs
Comment on lines +1641 to +1642
// For other pre-upgrade checks, we need the storage to already be migrated.
frame_support::storage::migration::move_pallet(b"BagsList", b"VoterList");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why we need this in the pre upgrade check?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just a hack because we have two migrations around the same pallet. All pre-migrations are ran first, then all migrations, then all post migrations.

We need to mimic the old migrations happening to make it work.

Perhaps we can change the logic here to run the pre-post hooks per migration, not all at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was thinking the same thing, but not sure how easily implementable that is given the "tuple" trait stuff.

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for da4d1f0

@shawntabrizi shawntabrizi merged commit 731979d into master May 19, 2022
Runtime automation moved this from Needs Audit to Done May 19, 2022
@shawntabrizi shawntabrizi deleted the shawntabrizi-bags-to-voter-rename branch May 19, 2022 18:33
@jakoblell jakoblell added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Jun 9, 2022
@github-actions github-actions bot mentioned this pull request Jun 22, 2022
Closed
This was referenced Jul 18, 2022
Closed
Open
@ghbot-test ghbot-test bot mentioned this pull request Jul 28, 2022
Open
@ghzlatarev ghzlatarev mentioned this pull request Jul 29, 2022
Merged
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@KiChjang KiChjang KiChjang approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. E0-runtime_migration PR introduces code that might require downstream chains to run a runtime upgrade.
Projects
Runtime
Done
Milestone
v0.9.23
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@shawntabrizi @DrW3RK @kianenigma @KiChjang @bkchr @jakoblell