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

Authority Discovery to FRAME v2 #8620

Merged
5 commits merged into from
Apr 18, 2021
Merged

Conversation

ferrell-code
Copy link
Contributor

migrated the pallet to new macro :). I believe there is a minor issue with the macro in genesis config. The assimilate_storage function needs the Config trait for genesis builder, but when the genesis config doesn't have the Config trait it won't work. Delete my manual impl of assimilate_storage and try to run the tests and you will see what I mean

@bkchr bkchr requested review from gui1117 and shawntabrizi April 14, 2021 19:32
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.

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

name of the pallet in both kusama, polkadot and westend are: AuthorityDiscovery thus no need for migration.

Looks good.

frame/authority-discovery/src/lib.rs Outdated Show resolved Hide resolved
frame/authority-discovery/src/lib.rs Outdated Show resolved Hide resolved
frame/authority-discovery/src/lib.rs Outdated Show resolved Hide resolved
frame/authority-discovery/src/lib.rs Outdated Show resolved Hide resolved
@gui1117 gui1117 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit E1-runtimemigration labels Apr 15, 2021
@shawntabrizi shawntabrizi changed the title Pallet to new macro Authority Discovery to Pallet v2 Apr 15, 2021
@shawntabrizi shawntabrizi changed the title Authority Discovery to Pallet v2 Authority Discovery to FRAME v2 Apr 15, 2021
@shawntabrizi
Copy link
Member

@ferrell-code got a compiler error here:

error: use of deprecated associated function `<impl pallet::GenesisConfig>::assimilate_storage`: use [`GenesisBuild::assimilate_storage`] instead
   --> frame/authority-discovery/src/lib.rs:316:4
    |
316 |         .assimilate_storage::<Test>(&mut t)
    |          ^^^^^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

Are you able to resolve it?

@gui1117
Copy link
Contributor

gui1117 commented Apr 15, 2021

@ferrell-code got a compiler error here:

error: use of deprecated associated function `<impl pallet::GenesisConfig>::assimilate_storage`: use [`GenesisBuild::assimilate_storage`] instead
   --> frame/authority-discovery/src/lib.rs:316:4
    |
316 |         .assimilate_storage::<Test>(&mut t)
    |          ^^^^^^^^^^^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`

Are you able to resolve it?

Sorry I introduced it by deprecated the assimilate_storage.

I think we can remove or deprecate the implementation though, but we can do that in later PR it is fine to me.

EDIT: in general now that we use construct_runtime in tests we should no longer use these method but rather build the GenesisConfig of the runtime directly (instead of each pallet genesis config)

@ferrell-code
Copy link
Contributor Author

Cool I get it, assimilate_storage method is the old way of doing it. I can update the tests to remove the implementation (or deprecate it if necessary), idk if you'd rather it be in this PR or open another one

@gui1117
Copy link
Contributor

gui1117 commented Apr 16, 2021

Cool I get it, assimilate_storage method is the old way of doing it. I can update the tests to remove the implementation (or deprecate it if necessary), idk if you'd rather it be in this PR or open another one

this pattern of reexposing manually the old method instead of using GenesisBuild has been used in other crates as well. So we will need to clean it in the future anyway.

You can do for this pallet in this PR if you want, other in a follow-up, or we'll do once we deprecate all of those.

If you want to merge as-is could you merge master to make CI happy ?

@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Apr 18, 2021

Trying merge.

@ghost ghost merged commit 6a76627 into paritytech:master Apr 18, 2021
@ferrell-code ferrell-code deleted the pallet-to-new-macro branch April 18, 2021 19:34
@gui1117
Copy link
Contributor

gui1117 commented Apr 19, 2021

thank you @ferrell-code

This pull request was closed.
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. 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants