Skip to content

Migrate pallet-grandpa to use the FRAME umbrella crate #8264

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Wolfenheimm
Copy link
Contributor

@Wolfenheimm Wolfenheimm commented Apr 16, 2025

Description

This PR migrates pallet-grandpa to use the polkadot-sdk-frame (FRAME umbrella crate), as tracked in #6504. All direct imports from frame_support, sp_runtime, and similar crates have been replaced with imports from the umbrella crate, using the appropriate prelude or domain-specific modules. Where necessary, missing items have been re-exported from the umbrella crate to ensure all functionality is available exclusively via frame.

Resolves pallet-grandpa in issue #6504

Integration

  • Downstream projects should now import all FRAME types, traits, and macros for pallet-grandpa via the frame crate and its preludes.
  • No direct imports from frame_support, sp_runtime, or other underlying crates should remain in the pallet.
  • Example usage:
    - use frame_support::{assert_ok, traits::OnFinalize};
    + use frame::{assert_ok, traits::OnFinalize};

Review Notes

  • All imports in pallet-grandpa now use frame (umbrella crate) and its preludes.
  • Any missing types, traits, or macros required by pallet-grandpa have been added to the appropriate prelude or re-exported in frame.
  • All tests and benchmarks have been updated to use only the frame crate for imports.
  • No logic changes are introduced—only import and re-export adjustments for the migration.

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required).
  • I have made corresponding changes to the documentation (if applicable).
  • I have added tests that prove my fix is effective or that my feature works (if applicable).

Thank you for your review!

@Wolfenheimm Wolfenheimm requested a review from a team as a code owner April 16, 2025 17:16
@Wolfenheimm Wolfenheimm marked this pull request as draft April 16, 2025 17:17
@Wolfenheimm
Copy link
Contributor Author

Wolfenheimm commented Apr 16, 2025

@re-gius Good day! I've decided to write this PR for pallet-grandpa, I'm available for any required changes.

One issue I noticed from my PR is that I cannot run tests without adding the runtime-benchmarks feature, if this is ok then it's as intended.

Looking forward to the review!

@Wolfenheimm Wolfenheimm marked this pull request as ready for review April 16, 2025 18:19
@bkchr bkchr requested a review from re-gius April 22, 2025 09:28
@re-gius re-gius added the T2-pallets This PR/Issue is related to a particular pallet. label Apr 22, 2025
Copy link
Contributor

@re-gius re-gius left a comment

Choose a reason for hiding this comment

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

Please:

  • merge the changes from master branch
  • run taplo format --config .config/taplo.toml to fix any toml formatting issues

@github-actions github-actions bot requested a review from bkchr April 23, 2025 07:37
Copy link
Contributor

Review required! Latest push from author must always be reviewed

@Wolfenheimm
Copy link
Contributor Author

Wolfenheimm commented May 3, 2025

@re-gius, apologies for the previous issues and thank you for pointing them out - I'm seeing quite a few errors here in the workflow - is there anything else I should focus on?

i.e. "You can just apply the patch (git apply PATCH_NAME) that was printed to make this CI check succeed." in check-umbrella. Not quite sure how to approach that one.

Copy link
Contributor

@re-gius re-gius left a comment

Choose a reason for hiding this comment

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

Hope that my comments are enough to solve most of the issues

@@ -122,6 +122,12 @@ pub use sp_runtime::{
self, print, traits::Printable, ConsensusEngineId, MAX_MODULE_ERROR_ENCODED_SIZE,
};

pub use self::migrations::VersionedMigration;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove all the changes from this file, you shouldn't touch it. It's better to change only the frame umbrella crate.

"pallet-timestamp/std",
"scale-info/std",
"sp-application-crypto/std",
"sp-consensus-grandpa/std",
"sp-core/std",
"sp-io/std",
"sp-runtime/std",
"sp-session/std",
"sp-staking/std",
Copy link
Contributor

Choose a reason for hiding this comment

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

Add "sp-weights/std" here

VariantCount, VariantCountOf,
Contains, Currency, Defensive, DefensiveSaturating, EitherOf,
EstimateNextSessionRotation, Everything, InsideBoth, InstanceFilter, IsSubType,
KeyOwnerProofSystem, MapSuccess, NoOpPoll, OnFinalize, OnRuntimeUpgrade,
Copy link
Contributor

Choose a reason for hiding this comment

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

OnFinalize here creates ambiguous imports in pallet staking-async, so it's better to avoid it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants