Skip to content
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

Use frame umbrella crate in pallet-babe & pallet-staking-reward-curve #6412

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

snowmead
Copy link
Contributor

@snowmead snowmead commented Nov 8, 2024

Description

Modifying pallet-babe and pallet-staking-reward-curve to use umbrella crate whilst adding a few types and preludes I deemed necessary for other pallets to potentially use as well.

These modifications are inspired by #5995

Review Notes

Some notable changes to umbrella crate preludes:

  • Exporting arithmetic prelude since this is used a lot throughout various pallets.

  • Exporting a few crypto primitives in the hashing prelude.

    Maybe we should rename hashing to cryptography, something more general to encompass hashing and cryptographic methods such as VRFs.

  • Updated pallet-staking-reward-curve proc-macro to use umbrella crate since it was being used in pallet-babe. The change is reverse compatible as to not force all other pallets using this to update to the frame umbrella crate.

  • Removed std feature enabled dev-dependency pallets from pallet-babe

Polkadot address: 15Ug5Xtwf8wpD5PCKZcetXMh8NadVm3hHWBn8NQan3cqD

@snowmead snowmead changed the title Use frame umbrella crate in pallet-babe Use frame umbrella crate in pallet-babe & staking/reward-curve Nov 8, 2024
@snowmead snowmead changed the title Use frame umbrella crate in pallet-babe & staking/reward-curve Use frame umbrella crate in pallet-babe & pallet-staking-reward-curve Nov 8, 2024
@snowmead snowmead marked this pull request as ready for review November 8, 2024 21:37
@snowmead snowmead requested a review from a team as a code owner November 8, 2024 21:37
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Some small changes needed, but looks good so far.

@re-gius can you also review this please?

@re-gius re-gius added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Nov 13, 2024
substrate/frame/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/babe/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/staking/reward-curve/src/lib.rs Outdated Show resolved Hide resolved
@re-gius re-gius added the R0-silent Changes should not be mentioned in any release notes label Nov 27, 2024
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.

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.

Good!

#[doc(no_inline)]
pub use sp_runtime::{DispatchErrorWithPostInfo, DispatchResultWithInfo, TokenError};
pub use sp_runtime::{
curve::PiecewiseLinear, BoundToRuntimeAppPublic, ConsensusEngineId,
Copy link
Contributor

Choose a reason for hiding this comment

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

To be a bit picky on the decision of what goes into the prelude and what doesn't, I suggest we ack that types that are used in pallet-babe only should not be in prelude. So I would remove ConsensusEngineId and DigestItem.

Rule of thumb: Whatever is used in at least 2+ pallets can be in prelude. Whatever not should use deps.

cc @re-gius

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM after resolving existing open comemnts.

@re-gius re-gius added T2-pallets This PR/Issue is related to a particular pallet. and removed T1-FRAME This PR/Issue is related to core FRAME, the framework. labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes 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