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

[FRAME] Simplify pallet config definition: remove RuntimeEvent associated type #7229

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

Conversation

dastansam
Copy link
Contributor

@dastansam dastansam commented Jan 17, 2025

part of #3743

Motivation

This PR removes the need for defining RuntimeEvent in the Config trait of a pallet. It uses associated type bound feature under the hood to make sure that Event of the pallet is convertible to the aggregated runtime event type frame_system::RuntimeEvent.

This is an initial PR for RuntimeEvent type and will be followed with other types, e.g RuntimeCall. As a demo, example pallets' config definition is updated to use this feature.

With this change, we can do this:

#[pallet::config]
pub trait Config: frame_system::Config {
}

instead of this:

#[pallet::config]
pub trait Config: frame_system::Config {
        /// Overarching event type.
       type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;
}

The latter will emit deprecation warnings and is redundant.

polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

@dastansam dastansam marked this pull request as ready for review January 17, 2025 15:57
@dastansam dastansam requested a review from a team as a code owner January 17, 2025 15:57
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.

Ty for doing this!

substrate/frame/examples/split/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 24, 2025 12:27
@dastansam dastansam requested review from gui1117 and bkchr January 24, 2025 12:36
@dastansam dastansam changed the title [FRAME] Simplify pallet config definition: RuntimeEvent as an associated type bound [FRAME] Simplify pallet config definition: remove RuntimeEvent associated type Jan 24, 2025
@dastansam
Copy link
Contributor Author

@bkchr bunch of CI is failing because they treat the RuntimeEvent deprecation warnings as error. Do you think it's worth fixing all those deprecation warnings in this PR? I thought it would be a lot of changes and also would feel more comfortable doing it once this change is carefully reviewed.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team January 25, 2025 09:05
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, after the suggestion, and also a few comments

substrate/frame/support/procedural/src/pallet/parse/mod.rs Outdated Show resolved Hide resolved
///
/// ```rs
/// pub trait Config: frame_system::Config {
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

As a note personally I would always bound the supertrait: frame_system::Config<RuntimeEvent: From<Event>>, always, except if there is the flag is_frame_system.
If user wants to write it, he can do, then it will still compile fine.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7cfad8342293e593a0c8041cd4b5a958

But this is also ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for this tip, really helpful)

@dastansam
Copy link
Contributor Author

dastansam commented Jan 27, 2025

@bkchr I replaced with proc macro warning but now I have to put the #[allow(deprecated)] above the mod pallet:

#[frame_support::pallet]
#[allow(deprecated)]
pub mod pallet {

but this way it would take a big span and we could ignore other deprecation warnings as well. Putting #[allow(deprecated)] above pallet config implementations is not ignoring the warning. what would you suggest? am I missing sth?

@bkchr
Copy link
Member

bkchr commented Jan 28, 2025

Putting #[allow(deprecated)] above pallet config implementations is not ignoring the warning. what would you suggest? am I missing sth?

You need to detect the allow(deprecated) in the macro and then forward this to the usage of this item.

@dastansam dastansam requested review from cheme and a team as code owners February 15, 2025 12:16
@paritytech-review-bot paritytech-review-bot bot requested a review from a team February 15, 2025 12:16
substrate/frame/system/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/lib.rs Show resolved Hide resolved
substrate/frame/support/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/src/lib.rs Outdated Show resolved Hide resolved
@dastansam dastansam force-pushed the feat/runtime-event-associated-type-bound branch from 85171bc to cf1d716 Compare February 16, 2025 07:23
Copy link
Contributor Author

@dastansam dastansam left a comment

Choose a reason for hiding this comment

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

macro is now always auto inserting system supertrait with a RuntimeEvent bound if it detects the valid #[pallet::event] and simply ignoring the deprecated RuntimeEvent. it now looks bit different and much smaller than the initial PR, so I guess reviewing from scratch would be easier)
@gui1117

@dastansam dastansam requested review from bkchr and gui1117 February 16, 2025 07:35
@gui1117 gui1117 added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Feb 17, 2025
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.

Some minor comments, then it looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants