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

Migrate pallet-sudo to pallet! #8448

Merged
merged 11 commits into from
Mar 26, 2021
Merged

Migrate pallet-sudo to pallet! #8448

merged 11 commits into from
Mar 26, 2021

Conversation

ascjones
Copy link
Contributor

Part of #7882.

Converts the Sudo pallet to the new pallet attribute macro introduced in #6877.

Following the upgrade guidelines here: https://crates.parity.io/frame_support/attr.pallet.html#upgrade-guidelines.

⚠️ 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 Sudo 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 Sudo pallet.

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 24, 2021
@ascjones ascjones added B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Mar 24, 2021
@ascjones ascjones marked this pull request as ready for review March 25, 2021 10:11
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Mar 25, 2021
@ascjones ascjones added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. E1-runtimemigration labels Mar 25, 2021
@ascjones ascjones requested a review from gui1117 March 25, 2021 10:18
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

polkadot doesn't needs migration as not used in polkadot/kusama and westend has unchanged pallet prefix.

frame/sudo/src/lib.rs Outdated Show resolved Hide resolved
frame/sudo/src/lib.rs Outdated Show resolved Hide resolved
frame/sudo/src/lib.rs Outdated Show resolved Hide resolved
frame/sudo/src/lib.rs Outdated Show resolved Hide resolved
ascjones and others added 5 commits March 25, 2021 11:01
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@ascjones
Copy link
Contributor Author

ascjones commented Mar 26, 2021

CI is failing with this warning as error https://gitlab.parity.io/parity/substrate/-/jobs/867537#L473:

error: associated function is never used: `call_functions`
  --> frame/sudo/src/mock.rs:48:12
   |
48 |     #[pallet::call]
   |       ^^^^

I don't get it because it is clearly used by the construct_runtime! expansion: FnEncode ( logger :: Pallet :: < Test > :: call_functions. Any ideas @thiolliere?

@gui1117
Copy link
Contributor

gui1117 commented Mar 26, 2021

yes but the metadata function (which uses call_functions) generated by construct_runtime is unused I guess, thus it complains about unused items.

@ascjones
Copy link
Contributor Author

Ah yeah, I wonder how that worked with the legacy decl_module! though

@gui1117
Copy link
Contributor

gui1117 commented Mar 26, 2021

maybe rustc is not doing the same checks for both macros kind.
Anyway you can add a dummy test:

#[test] 
fn dummy_use() {
Test::metadata();
}

Or actually we can add in construct_runtime #[allow_unused] for the function metadata.

@ascjones
Copy link
Contributor Author

Ah yeah, I wonder how that worked with the legacy decl_module! though

The original macro already annotated the generated fn with #[allow(dead_code)], I've just done the same for the new one.

@ascjones ascjones merged commit 84360e1 into master Mar 26, 2021
@ascjones ascjones deleted the aj-migrate-sudo branch March 26, 2021 14:54
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
* WIP convert sudo pallet to attribute macros

* Fix up tests and migrate mock

* Fix up genesis build

* Migrate doc comment example

* Update frame/sudo/src/lib.rs

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

* Update frame/sudo/src/lib.rs

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

* Update frame/sudo/src/lib.rs

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

* Update frame/sudo/src/lib.rs

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

* Allow unused metadata call_functions

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing 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 May 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants