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

allow to write pre and post runtime upgrade in pallet macro #8194

Merged
merged 3 commits into from
Feb 25, 2021

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Feb 24, 2021

I removed the default implementation of pre_upgrade and post_upgrade on OnRuntimeUpgrade.
Otherwise if node-runtime forget to activate the my-pallet/try-runtime then the pre_upgrade and post_upgrade of the pallet would simply be ignored without warning. Instead now it fails to compile saying pre_upgrade and post_upgrade needs to be implemented.

But the error is:

error[E0046]: not all trait items implemented, missing: `pre_upgrade`, `post_upgrade`
   --> frame/system/src/lib.rs:262:12
    |
262 |     #[pallet::hooks]
    |               ^^^^^ missing `pre_upgrade`, `post_upgrade` in implementation
    |
    = help: implement the missing item: `fn pre_upgrade() -> Result<(), &'static str> { todo!() }`
    = help: implement the missing item: `fn post_upgrade() -> Result<(), &'static str> { todo!() }`

So it is not super explicit (but still better than silently ignore it).

NOTE: the PR doesn't add the feature to decl_module, we can do in another one if we want.

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Feb 24, 2021
@@ -154,6 +154,7 @@ std = [
"pallet-society/std",
"pallet-recovery/std",
"pallet-vesting/std",
"frame-try-runtime/std",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also added std here in order to be able to compile the crate with try-runtime and std.
(cargo check -p node-runtime --features try-runtime)

Otherwise it fails.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with that is that Cargo currently doesn't support optional dependencies and features.

Not sure how this works 🤷

Copy link
Contributor Author

@gui1117 gui1117 Feb 25, 2021

Choose a reason for hiding this comment

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

AFAICT this will pull frame-try-runtime crate everytime std is activated, which is not what we want, but is not bothering too much either.

EDIT: and we do same for frame-benchmarking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think given the same situation with benchmark, let's move on and let it be, but a refactor issue for it will be appreciated. Maybe we can do it nicer someday.

Copy link
Contributor Author

@gui1117 gui1117 Feb 25, 2021

Choose a reason for hiding this comment

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

yes I think there is plan to support it with something like "frame-try-runtime?/std"

Ok then I'll merge and open an issue: #8202

@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. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 24, 2021
@gui1117 gui1117 marked this pull request as ready for review February 24, 2021 11:52
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Besides the one comment it looks okay.

@@ -154,6 +154,7 @@ std = [
"pallet-society/std",
"pallet-recovery/std",
"pallet-vesting/std",
"frame-try-runtime/std",
Copy link
Member

Choose a reason for hiding this comment

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

The problem with that is that Cargo currently doesn't support optional dependencies and features.

Not sure how this works 🤷

@gui1117 gui1117 merged commit 1febf99 into master Feb 25, 2021
@gui1117 gui1117 deleted the gui-pre-dispatch-hook branch February 25, 2021 10:43
jam10o-new pushed a commit to jam10o-new/substrate that referenced this pull request Feb 28, 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. 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.

4 participants