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

Adds ability to provide defaults for types provided by construct_runtime #14682

Merged
merged 46 commits into from
Aug 25, 2023

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented Jul 30, 2023

Step in paritytech/polkadot-sdk#171

As described in #14453 (comment), this PR attempts to provide defaults for types provided by construct_runtime.

This is achieved by the combination of two macros:

  1. #[pallet::no_default_bounds] that adds the ability to remove trait bounds while creating DefaultConfig. This allows us to set types like RuntimeCall as ().
trait Config {
    #[pallet::no_default_bounds]
    type RuntimeCall: ....; //bounds
}

becomes

trait DefaultConfig {
    type RuntimeCall;
}
  1. #[inject_runtime_type] that can be added to types generated by construct_runtime while registering the default impl. When this is later combined with the user-provided impl, it's replaced with the respective types.
impl DefaultConfig for TestDefaultConfig {
    #[inject_runtime_type]
    type RuntimeCall = ();
}

becomes

impl frame_system::Config for Runtime {
    type RuntimeCall = RuntimeCall;
}

Todo:

  • Docs
  • UI Tests

@gupnik gupnik changed the title Adds ability to use defaults for verbatim types Adds ability to provide defaults for types provided by construct_runtime Jul 30, 2023
@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 Jul 30, 2023
Copy link
Contributor

@sam0x17 sam0x17 left a comment

Choose a reason for hiding this comment

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

this is looking good to me just needs docs in a few places and some integration tests etc

frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
#[derive(derive_syn_parse::Parse, PartialEq, Eq)]
pub enum PalletAttrType {
#[peek(keyword::verbatim, name = "verbatim")]
Verbatim(keyword::verbatim),
Copy link
Contributor

Choose a reason for hiding this comment

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

nice making this extendible 💯

frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor

Seems like the right direction to have a fully elided impl.

Can you elaborate a bit about why you chose the word verbatim?

@sam0x17
Copy link
Contributor

sam0x17 commented Aug 2, 2023

Can you elaborate a bit about why you chose the word verbatim?

that's my fault. Its a term syn uses internally a lot to refer to tokenstreams that are left as-is without any further parsing.

in general it just means "as-is"

@gupnik gupnik marked this pull request as ready for review August 4, 2023 06:27
@gupnik gupnik requested review from a team August 4, 2023 06:27
@gupnik gupnik added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Aug 4, 2023
frame/balances/src/lib.rs Outdated Show resolved Hide resolved
/// A bool for each sub-trait item indicates whether the item has
/// `#[pallet::no_default_bounds]` attached to it. If true, the item will not have any bounds
/// in the generated default sub-trait.
pub items: Vec<(syn::TraitItem, bool)>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use an enum instead of a boolean in here?

Copy link
Contributor Author

@gupnik gupnik Aug 21, 2023

Choose a reason for hiding this comment

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

I am not sure if it adds a lot of value here. What are the enum variants that you would suggest?

frame/system/src/lib.rs Outdated Show resolved Hide resolved
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.

Already looks much better!

frame/support/procedural/src/derive_impl.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/derive_impl.rs Outdated Show resolved Hide resolved
frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
frame/balances/src/lib.rs Outdated Show resolved Hide resolved
@gupnik gupnik requested a review from bkchr August 24, 2023 13:16
@gupnik
Copy link
Contributor Author

gupnik commented Aug 25, 2023

bot merge

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. 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.

8 participants