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

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Oct 28, 2020

This pr ensures that pallet versions are also set at genesis. It does
this by hooking into the runtime GenesisConfig which means that it
will only work when the storage is setup using this genesis config. So,
the version will not be set in pallet local tests. However, I think this
isn't such a problem. The genesis config will call on_genesis on all
pallets. This function comes from the new trait OnGenesis. Currently
the user is not able to provide any custom implementation of this trait.

Besides that it also implements Clone and Copy for the pallet
version struct.

This pr also moves the macro for generating the runtime genesis config
to frame-support as most of the other FRAME related macros.

This pr ensures that pallet versions are also set at genesis. It does
this by hooking into the runtime `GenesisConfig` which means that it
will only work when the storage is setup using this genesis config. So,
the version will not be set in pallet local tests. However, I think this
isn't such a problem. The genesis config will call `on_genesis` on all
pallets. This function comes from the new trait `OnGenesis`. Currently
the user is not able to provide any custom implementation of this trait.

Besides that it also implements `Clone` and `Copy` for the pallet
version struct.

This pr also moves the macro for generating the runtime genesis config
to `frame-support` as most of the other FRAME related macros.
@bkchr bkchr 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 Oct 28, 2020
@bkchr bkchr requested review from apopiak and gui1117 October 28, 2020 17:11
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.

looks good to me

@apopiak
Copy link
Contributor

apopiak commented Oct 29, 2020

[...]
The genesis config will call on_genesis on all
pallets. This function comes from the new trait OnGenesis. Currently
the user is not able to provide any custom implementation of this trait.
[...]

In the sense that they would have to implement the trait manually and cannot just define an on_genesis hook in decl_module, right?

@bkchr
Copy link
Member Author

bkchr commented Oct 29, 2020

[...]
The genesis config will call on_genesis on all
pallets. This function comes from the new trait OnGenesis. Currently
the user is not able to provide any custom implementation of this trait.
[...]

In the sense that they would have to implement the trait manually and cannot just define an on_genesis hook in decl_module, right?

The trait is always auto implemented, but they can write on_genesis in decl_module! at the moment. Maybe if we come up with something for that we need this, we can add support for it.

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
@bkchr bkchr merged commit d5464dd into master Oct 29, 2020
@bkchr bkchr deleted the bkchr-pallet-version-genesis branch October 29, 2020 19:20
@xlc
Copy link
Contributor

xlc commented Oct 29, 2020

We already have add_extra_genesis { build(|_config| { }) } in decl_stroage that can be used to populate genesis. Any difference between extra genesis build and on_genesis? Other than on_genesis is not user defendable at this stage?

@gui1117
Copy link
Contributor

gui1117 commented Oct 29, 2020

We already have add_extra_genesis { build(|_config| { }) } in decl_stroage that can be used to populate genesis. Any difference between extra genesis build and on_genesis? Other than on_genesis is not user defendable at this stage?

The pallet genesis config is optional, this is why it can't be used here

@xlc
Copy link
Contributor

xlc commented Oct 29, 2020

ok. but there is no need to store pallet version if it doesn’t have storage? at least you won’t have the migration issue if there isn’t anything to migrate

@gui1117
Copy link
Contributor

gui1117 commented Oct 29, 2020 via email

@bkchr
Copy link
Member Author

bkchr commented Oct 29, 2020

ok. but there is no need to store pallet version if it doesn’t have storage? at least you won’t have the migration issue if there isn’t anything to migrate

As said in the original issue, its not only about storage migrations. It's also about the version of the pallet.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants