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

@paritytech-processbot paritytech-processbot bot merged commit 868869e into master Aug 25, 2023
5 checks passed
@paritytech-processbot paritytech-processbot bot deleted the gupnik/derive-impl-improvements branch August 25, 2023 07:52
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