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

Abstract away the executive #14742

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Abstract away the executive #14742

wants to merge 12 commits into from

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented Aug 10, 2023

This PR adds the ability to auto-generate the Executive inside construct_runtime. It also creates wrappers on Runtime that internally call the Executive's APIs.

In order to enable this, simply provide a Migrations tuple like so:

construct_runtime!(
    type Migrations = (...);
    pub struct Runtime {
         .....
    }
)

Extension:
We should be able to generate a number of Runtime APIs internally.

@gupnik gupnik added A3-in_progress Pull request is in progress. No review needed at this stage. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels Aug 10, 2023
@gupnik gupnik requested review from a team August 10, 2023 04:29
@gupnik gupnik marked this pull request as draft August 10, 2023 05:04
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.

I generally like this direction. Such simplifications, especially when backwards compatible, can help reduce the hassle of setting up tests and simple runtimes a lot.

I would imagine a syntax like this, avoiding furthe "shady Rust":

// Valid, no migration, no need for executive.
construct_runtime_next {
  struct Runtime { ... }
}
// Valid, some custom migration.
construct_runtime_next {
  type Migration = ..
  struct Runtime { ... }
}
// Valid, custom executive
construct_runtime_next {
  type Executive = ..
  struct Runtime { ... }
}

I am also suggesting that we somewhat make this be a new macro, as I can't find an easy way to make it fully backwards compatible. But this can be solved in different ways.

Regardless of my suggestion above, I think the struct Runtime<<Migration> is a bit iffy, but there could be multiple different alternatives.

@gupnik
Copy link
Contributor Author

gupnik commented Aug 14, 2023

@kianenigma I have updated the syntax as per your suggestion.

Now, we just need to provide a tuple type Migrations = (...) to enable this. In its absence, it defaults to the old behaviour, thus ensuring backwards compatibility.

Please let me know your thoughts.

block: &TokenStream,
executive_section: ExecutiveSection,
) -> Result<TokenStream> {
let executive = generate_crate_access_2018("frame-executive")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will play nicely with the new frame crate, it should re-export the frame-executive.

pub type Executive = #executive::Executive<
#runtime,
#block,
#system::ChainContext<#runtime>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kinda cheating, if you want to be pedantic. There's now no good way to configure this. But I am not sure if this is all that useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I will add the support to provide a custom Executive as well, using the syntax you suggested above.

@bkchr
Copy link
Member

bkchr commented Aug 14, 2023

Why don't we start working on a new construct_runtime! syntax, instead of trying to add more to the current macro. paritytech/polkadot-sdk#232 there was already quite some discussion and the ideas of this pr could be added there as well.

@@ -17,7 +17,8 @@ impl frame_system::Config for Runtime {
struct Dummy;

construct_runtime! {
pub struct Runtime<Dummy>
type Migrations = (Dummy);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work if you use (Dummy,)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it parses correctly and throws the correct error.

@liamaharon
Copy link
Member

liamaharon commented Aug 17, 2023

Why don't we start working on a new construct_runtime! syntax, instead of trying to add more to the current macro. paritytech/polkadot-sdk#232 there was already quite some discussion and the ideas of this pr could be added there as well.

I agree, the mod runtime syntax described here paritytech/polkadot-sdk#232 would be a huge step up imo. These small incremental improvements to construct_runtime! are cool, but are they worth working on if there're few blockers to replacing it entirely with something better?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants