migrate pallet-elections-phragmen to attribute macros #8044

kianenigma · 2021-02-04T10:30:54Z

part of #7882.

Previous usage of decl_storage set the pallet storage prefix to PhragmenElection, while in both Polkadot and kusama it is set to ElectionsPhragmen

https://github.com/paritytech/polkadot/blob/4866f8e72029e719d7ce74e341bb1e94ebd724f7/runtime/polkadot/src/lib.rs#L1002

https://github.com/paritytech/polkadot/blob/4866f8e72029e719d7ce74e341bb1e94ebd724f7/runtime/kusama/src/lib.rs#L997

Therefore, this will need

Either a migration
Renaming of the pallet in polkadot repo

⚠️ Breaking Change ⚠️

From https://crates.parity.io/frame_support/attr.pallet.html#checking-upgrade-guidelines

storages now use PalletInfo for module_prefix instead of the one given to decl_storage: Thus any use of this pallet in construct_runtime! should be careful to update name in order not to break storage or to upgrade storage (moreover for instantiable pallet). If pallet is published, make sure to warn about this breaking change.

So users of the ElectionsPhragmen pallet must be careful about the name they used in construct_runtime!. Hence the runtime-migration label, which might not be needed depending on the configuration of the ElectionsPhragmen pallet.

polkadot companion: paritytech/polkadot#2765

gui1117

Needs a companion and some nitpicks, otherwise looks good.

frame/elections-phragmen/src/lib.rs

kianenigma · 2021-02-13T11:48:58Z

@thiolliere did you see my note about breaking change though? this will sadly need a migration, or we must rename the runtimes in polkadot/kusama.

Honestly, I wish we could migrate the prefix to ElectionsPhragmen, because the pallet is called elections-phragmen, and the current prefix is just confusing.

gui1117 · 2021-02-13T12:45:19Z

Yes that's what I meant by need a companion, but indeed we can also write the migration code in this pallet. or we decide to rename the pallet instead

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

kianenigma · 2021-03-18T09:38:50Z

I thought about this a bit further and:

My personal preference is to write a migration that moves everything from PhragmenElection to ElectionsPhragmen prefix.
But that will break polkadot js and potentially other tools.
So I can add a companion that will rename the modules in construct_runtime! in our chains, and we should put this in the release notes as well so that other chains do the same.
Or I wait for the hack that @thiolliere said he might come up with using which a pallet can override where in storage it will be kept. (We'll need it for Refactor Account Storage into Accounts Pallet #8254)

gui1117 · 2021-03-18T10:31:52Z

I would prefer third party tools to fetch the storage from the metadata.
I expected polkadotjs to do that already, did they hardcoded the location of the storage for pallet-elections-phragmen ?

If third party tools fetch the storage using the metadata. then they shouldn't break.

kianenigma · 2021-03-18T10:50:42Z

I recall that one of the reasons for preferring to keep the storage of #8254 under system was to remain backward compatible. I am using the same argument here, but honestly I am not sure how much breakage would happen.

bkchr · 2021-03-18T11:34:40Z

AFAIK polkadot js already uses the prefix from the metadata and I think we should assume this model for the rest. Overriding the storage prefix is somewhat weird. This is again just a Polkadot specific feature, while we are here in Substrate ;)

gui1117 · 2021-03-18T12:18:51Z

I recall that one of the reasons for preferring to keep the storage of #8254 under system was to remain backward compatible. I am using the same argument here, but honestly I am not sure how much breakage would happen.

at some point, if third party tools are using metadata to fetch system accounts, then I'm not sure keeping it at the same location will help them. Because the Account storage will no longer be in the metadata in system pallet.

kianenigma · 2021-03-19T06:20:15Z

The conclusion is therefore to change the prefix, and let tools figure it out via the metadata.

shawntabrizi · 2021-04-06T10:37:57Z

Previous usage of decl_storage set the pallet storage prefix to PhragmenElection, while in both Polkadot and kusama it is set to ElectionsPhragmen

Can't we just change the name in construct_runtime?

kianenigma · 2021-04-06T12:47:46Z

Previous usage of decl_storage set the pallet storage prefix to PhragmenElection, while in both Polkadot and kusama it is set to ElectionsPhragmen

Can't we just change the name in construct_runtime?

We could, but as noted above my preference is to fix this and move the storage as well.

I am fine with removing the migration from this PR and adding it as a separate PR, but that should make no difference and the code is not that complicated.

Also, we should provide the migration code anyhow, even if we are not going to use it ourselves. This pallet is the only one that I am experimentally trying to maintain with proper versions and all migrations are kept in the code as reusable snippets.

gui1117

looks good to me. with:

removal of added origin
change in pre_migration test function.
needs to merge master: ModuleId is changed into PalletId

gui1117 · 2021-04-14T10:58:40Z

frame/elections-phragmen/src/migrations/v4.rs

+	log::info!("pre-migration elections-phragmen test with new = {}", new);
+
+	// ensure some stuff exist in the old prefix.
+	assert!(sp_io::storage::next_key(OLD_PREFIX).is_some());


this only ensures that there exist some key which are after OLD_PREFIX in the lexical order.
If OLD_PREFIX starts with 0 then any key which starts with 1 will make this succeed.

- // ensure some stuff exist in the old prefix. - assert!(sp_io::storage::next_key(OLD_PREFIX).is_some()); + // the next key must exist, and start with the hash of `OLD_PREFIX`. + let next_key = sp_io::storage::next_key(OLD_PREFIX).unwrap(); + assert!(next_key.starts_with(&sp_io::hashing::twox_128(OLD_PREFIX))); +

I think this makes good sense.

frame/elections-phragmen/src/lib.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

…aritytech/substrate into kiz-elections-phragmen-attribute-macro

kianenigma · 2021-04-15T07:44:55Z

as recommended by @shawntabrizi, I removed the migrations from polkadot repo. The companion will instead rename the name of the pallet. Please check it letter by letter! :D

And once both PRs are approved I will merge this.

Note that I will still provide the migration scripts since other chains might use to actually migrate. I home my changelog is also enough to prevent people from breaking their chain if they accidentally apply this.

gui1117 · 2021-04-15T08:05:41Z

frame/elections-phragmen/src/migrations/v4.rs

+
+	match maybe_storage_version {
+		Some(storage_version) if storage_version <= PalletVersion::new(3, 0, 0) => {
+			log::info!("new prefix: {}", new_pallet_name.as_ref());


I think we should check if old prefix == new prefix and if so we don't do the migration.

shawntabrizi

Not too sure we want to bump the version here, but okay.

@gnunicorn thoughts on that?

kianenigma · 2021-04-15T13:02:19Z

FYI I've bumped this crates version in the past as well. I don't think it causes any issues for @gnunicorn as long as we don't release anything solo.

kianenigma · 2021-04-23T06:10:49Z

bot merge

ghost · 2021-04-23T06:10:53Z

Waiting for commit status.

ghost · 2021-04-23T06:27:17Z

Merge aborted: Head SHA changed from 5ce2589 to cd908ed

kianenigma · 2021-04-23T07:12:28Z

bot merge

ghost · 2021-04-23T07:12:33Z

Trying merge.

All done

0719f95

kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 4, 2021

kianenigma mentioned this pull request Feb 4, 2021

Migrate substrate pallets to use pallet! #7882

Closed

44 tasks

paritytech-ci added the D2-breaksapi label Feb 4, 2021

kianenigma added the D1-runtime-migration label Feb 4, 2021

Fix benchmarks

1f28b32

kianenigma requested a review from gui1117 February 5, 2021 07:09

gui1117 added B3-apinoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Feb 12, 2021

gui1117 reviewed Feb 12, 2021

View reviewed changes

kianenigma and others added 5 commits February 19, 2021 14:07

Apply suggestions from code review

efc5623

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

Fix metadata.

0ddd835

Master.into()

0d3e45b

Fix build

397932e

Fix build

975f06e

gui1117 mentioned this pull request Feb 24, 2021

Add some migration helper to help migrating pallet changing pallet prefix #8199

Merged

gui1117 mentioned this pull request Mar 18, 2021

Allow to specify pallet prefix for some storages. #8391

Closed

kianenigma added 2 commits March 30, 2021 11:07

Master.into()

984bbf7

Add migrations

86ec8df

kianenigma removed A7-needspolkadotpr labels Apr 6, 2021

gui1117 approved these changes Apr 14, 2021

View reviewed changes

kianenigma and others added 3 commits April 15, 2021 09:14

Update frame/elections-phragmen/src/lib.rs

8bea3be

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

Fix build

58fd46f

Merge branch 'kiz-elections-phragmen-attribute-macro' of github.com:p…

dcbb86f

…aritytech/substrate into kiz-elections-phragmen-attribute-macro

kianenigma added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Apr 15, 2021

Fix test

9700fc8

gui1117 reviewed Apr 15, 2021

View reviewed changes

kianenigma added 2 commits April 15, 2021 14:21

early exit

56d77d4

Fix

971af54

shawntabrizi approved these changes Apr 15, 2021

View reviewed changes

Master.into()

5ce2589

Fix build

cd908ed

ghost merged commit 20b1a0e into master Apr 23, 2021

ghost deleted the kiz-elections-phragmen-attribute-macro branch April 23, 2021 07:12

sorpaas mentioned this pull request Sep 4, 2021

Runtime upgrade migration: Pelican Town kulupu/kulupu#146

Open

6 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate pallet-elections-phragmen to attribute macros #8044

migrate pallet-elections-phragmen to attribute macros #8044

kianenigma commented Feb 4, 2021 •

edited

Loading

gui1117 left a comment

kianenigma commented Feb 13, 2021 •

edited

Loading

gui1117 commented Feb 13, 2021 •

edited

Loading

kianenigma commented Mar 18, 2021

gui1117 commented Mar 18, 2021

kianenigma commented Mar 18, 2021

bkchr commented Mar 18, 2021

gui1117 commented Mar 18, 2021

kianenigma commented Mar 19, 2021

shawntabrizi commented Apr 6, 2021

kianenigma commented Apr 6, 2021 •

edited

Loading

gui1117 left a comment •

edited

Loading

gui1117 Apr 14, 2021

kianenigma Apr 15, 2021

kianenigma commented Apr 15, 2021

gui1117 Apr 15, 2021

kianenigma Apr 15, 2021

shawntabrizi left a comment

kianenigma commented Apr 15, 2021

kianenigma commented Apr 23, 2021

ghost commented Apr 23, 2021

ghost commented Apr 23, 2021

kianenigma commented Apr 23, 2021

ghost commented Apr 23, 2021

migrate pallet-elections-phragmen to attribute macros #8044

migrate pallet-elections-phragmen to attribute macros #8044

Conversation

kianenigma commented Feb 4, 2021 • edited Loading

⚠️ Breaking Change ⚠️

gui1117 left a comment

Choose a reason for hiding this comment

kianenigma commented Feb 13, 2021 • edited Loading

gui1117 commented Feb 13, 2021 • edited Loading

kianenigma commented Mar 18, 2021

gui1117 commented Mar 18, 2021

kianenigma commented Mar 18, 2021

bkchr commented Mar 18, 2021

gui1117 commented Mar 18, 2021

kianenigma commented Mar 19, 2021

shawntabrizi commented Apr 6, 2021

kianenigma commented Apr 6, 2021 • edited Loading

gui1117 left a comment • edited Loading

Choose a reason for hiding this comment

gui1117 Apr 14, 2021

Choose a reason for hiding this comment

kianenigma Apr 15, 2021

Choose a reason for hiding this comment

kianenigma commented Apr 15, 2021

gui1117 Apr 15, 2021

Choose a reason for hiding this comment

kianenigma Apr 15, 2021

Choose a reason for hiding this comment

shawntabrizi left a comment

Choose a reason for hiding this comment

kianenigma commented Apr 15, 2021

kianenigma commented Apr 23, 2021

ghost commented Apr 23, 2021

ghost commented Apr 23, 2021

kianenigma commented Apr 23, 2021

ghost commented Apr 23, 2021

kianenigma commented Feb 4, 2021 •

edited

Loading

kianenigma commented Feb 13, 2021 •

edited

Loading

gui1117 commented Feb 13, 2021 •

edited

Loading

kianenigma commented Apr 6, 2021 •

edited

Loading

gui1117 left a comment •

edited

Loading