Make construct_runtime macro more flexible

[arrudagates]

Usually the assumptions made by the construct_runtime macro are correct and help keep the runtime boilerplate free, but sometimes it's desirable to override the implementations generated by the macro.

In my case, for example, I want to override the implementation of From<Origin> for Result<RawOrigin<AccountId>, Origin> so I can allow my custom origins to pass through the default ensure_signed assertions in most pallets' calls.

I would like to propose and get feedback/suggestions on how to make the construct_runtime macro more flexible, while still maintaining the minimum amount of boilerplate. The solution I currently have in mind is probably not the best possible, thus I'd like to brainstorm solutions.

I'll preface this with a note that I'm not super knowledgeable in Rust macros, so I'm not sure if this is possible to implement like this, but here's what I had in mind:

construct_runtime!(
    pub enum Runtime where
        Block = Block,
        NodeBlock = opaque::Block,
        UncheckedExtrinsic = UncheckedExtrinsic
    {
        System: frame_system::{Pallet, Call, Config, Storage, Event<T>} = 0,
        [...]
    }

    // Anything implemented manually here won't get generated by the macro automatically and instead will be passed through. 
    impl From<Origin> for Result<System::Origin, Origin> {
        // impl here
    }

Perhaps we could even break down the implementations from construct_runtime into multiple macros (like impl_call, impl_origin, etc...) and these could be used by themselves or if the runtime does not require any special implementations, we could have the overarching construct_runtime calling these within itself if they are declared in the macro.

[kianenigma]

I am sure I've pasted this in another issue as well (found it), but I think the new version of this macro should look like this:

#[frame::contruct_rutnime]
mod runtime {
    // The rest, like `AllPalletsReserved` and such have to be 
    // unfortunately aut-generated, or better, deprecated. 
    #[frame::pallets] 
    type AllPallets = (
        frame_system::pallet, 
        pallet_balances::pallet, 
        pallet_staking::pallet,
        pallet_sessin::pallet,
    );
        
    // don't think there is much to this. 
    #[frame::runtime]
    pub struct Runtime; 
        
    // Auto-generate call from `AllPallets`
    #[frame::call]
    pub enum RuntimeCall = ..AllPallets;
        
    // Alternative to the spread syntax above: Explicitly build an enum with the given 
    // pallet paths. 
    //
    // This is very dangerous, only for experts. Probably no real use case for it either..
    #[frame::event]
    pub enum RuntimeEvent = (frame_system, pallet_staking);
    
    #[frame::origin]
    pub enum RuntimeOrigin = ..AllPallets;

    #[frame::genesis_config]
    pun enum RuntimeGenesis = ..AllPallet;

    // Have not thought it through, but see 
    https://github.com/paritytech/substrate/issues/13463#issuecomment-1464013060
    #[frame::ext_builder]
    pub struct ExtBuilder {...}
}
pub use runtime::{Runtime, RuntimeCall, RuntimeEvent, RuntimeOrigin};

Where if you write pub enum Origin = ..AllPallets you get the default implementation, but you can opt out of it any at any point as well.

[arrudagates]

I like this a lot, it not only solves the flexibility issues I mentioned, but also helps with visibility for the enums generated, which are right now created inside the macro and thus hidden in the source code of the runtime. ```

…

On Sun, Jan 15, 2023 at 9:36 AM Kian Paimani ***@***.***> wrote: I am sure I've pasted this in another issue as well, but I think the new version of this macro should look like this: #[frame::contruct_rutnime] mod runtime { // The rest, like `AllPalletsReserved` and such have to be // unfortunately aut-generated, or better, deprecated. #[frame::pallets] type AllPallets = ( frame_system::pallet, pallet_balances::pallet, pallet_staking::pallet, pallet_sessin::pallet, ); // don't think there is much to this. #[frame::runtime] pub struct Runtime; // Auto-generate call from `AllPallets` #[frame::call] pub enum RuntimeCall = ..AllPallets; // Alternative to the speard syntax above: Explicitely build an enum with the given // pallet paths. // // This is very dangerous, only for experts. Probably no real use case for it either.. #[frame::event] pub enum RuntimeEvent = (frame_system, pallet_staking); #[frame::origin] pub enum Origin = ..AllPallets } pub use runtime::{Runtime, RuntimeCall, RuntimeEvent, RuntimeOrigin}; Where if you write pub enum Origin = ..AllPallets you get the default implementation, but you can opt out of it any at any point as well. — Reply to this email directly, view it on GitHub <#232>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHGXNIUDDRNXPJG2AW45S5TWSPVMLANCNFSM6AAAAAAT2PWTBI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- *Gabriel Facco de Arruda* +55 (51) 99826-0101 ***@***.*** ***@***.***>*

[bkchr]

In my case, for example, I want to override the implementation of From<Origin> for Result<RawOrigin<AccountId>, Origin> so I can allow my custom origins to pass through the default ensure_signed assertions in most pallets' calls.

This doesn't really makes sense, why passing your custom origin when the function only needs a signed origin? And how should the From implementation look like? You don't have any context in there and would convert your origin just "blindly" into the RawOrigin. If you need help with what you are doing, please provide some proper write up (what you want to achieve etc) somewhere (maybe Polkadot forum). Then we can look into it and give some tips.

The general idea of @kianenigma looks good and something we should speak about!

[arrudagates]

In my case, for example, I want to override the implementation of From<Origin> for Result<RawOrigin<AccountId>, Origin> so I can allow my custom origins to pass through the default ensure_signed assertions in most pallets' calls.

This doesn't really makes sense, why passing your custom origin when the function only needs a signed origin? And how should the From implementation look like? You don't have any context in there and would convert your origin just "blindly" into the RawOrigin. If you need help with what you are doing, please provide some proper write up (what you want to achieve etc) somewhere (maybe Polkadot forum). Then we can look into it and give some tips.

The general idea of @kianenigma looks good and something we should speak about!

I don't actually need help with that, I already have it working how I want it but I had to clone and modify the frame-support-procedural crate so I could implement it myself, which is why I came to the conclusion that making the construct_runtime macro more flexible would be of help not only to me but possibly to others that want to have unconventional implementation like mine.

And to answer your question of why I want my custom origin to act as signed, it's because the origin is for a multisig pallet that has calls within it to change parameters and handle operations that modify the multisig itself, those need specific data like the multisig id and the original caller who initiated the multisig, I have that data in the custom origin, but I also want the multisig to act as a normal account for everything else. I know it can be easily solved by adding those to the function arguments and then verifying within the function, but I'm building with UX in mind, if something can be achieved by the runtime without concerning the users and developers, then I'll handle that.

[bkchr]

I see what you want to achieve. Currently this is already achievable without forking frame-support by creating a newtype wrapper for the RuntimeOrigin. On this newtype wrapper you could implement the Into<Result<RawOrigin, Self>> as you need it. Then you would set this WrappedRuntimeOrigin as your runtime origin to all the pallets.

We could also add some attribute #[do_not_implement_into_raw_origin] (shitty naming :P) to the current construct_runtime macro to skip the implementation. This is then a short term fix for your issue. I think even in the future dreamed by @kianenigma you would need such an attribute as per default this should always be implemented.

[Polkadot-Forum]

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/less-magic-in-frame-good-or-bad/2287/8

[gavofyork]

#[frame::contruct_rutnime]
mod runtime {
    // The rest, like `AllPalletsReserved` and such have to be 
    // unfortunately aut-generated, or better, deprecated. 
    #[frame::pallets] 
    type AllPallets = (
        frame_system::pallet, 
        pallet_balances::pallet, 
        pallet_staking::pallet,
        pallet_sessin::pallet,
    );
}

How does it handle instances, indices and instance naming?

[kianenigma]

Not a whole lot has changed, but here is a more up-to-date and well-documented version of the above idea, which also answers the question raised by @gavofyork:

#[frame::construct_runtime]
mod runtime {
	/// Mandatory, just to declare `pub struct Runtime`.
	#[frame::runtime]
	pub struct Runtime;

	/// Mandatory; and the benefit now is that we can elide a LOT of the types for this. The macro
	/// will elide all generics for you as:
	/// 1. `System` is already know.
	/// 2. `Block` is already known from system (assuming
	/// https://github.com/paritytech/substrate/pull/14193)
	/// 3. `Context` is already know from System.
	/// 4. `UnsignedValidator` is known from `AllPallets`
	/// 5. `AllPallets` is known
	/// 6. `OnRuntimeUpgrade` will be left blank to its default to `()`.
	///
	/// All of the above can be replaced with `_`. So the typical syntax is:
	///
	/// `pub struct Executive<_, _, _, _, _, Migration>`
	///
	/// Which can be further reduced to:
	///
	/// If you provide `pub struct Executive<Migrations>`
	///
	/// NOTE: In the future, the execution of `OnRuntimeUpgrade` should be fully delegated to a
	/// single "migration dispatcher" pallet (probably the same one as
	/// https://github.com/paritytech/substrate/pull/14275), so this last argument can be removed.
	#[frame::executive]
	pub struct Executive<_, _, _, _ , _>;

	/// Almost exactly the same syntax as the existing one.
	///
	/// This will not be real rust-like, but it will demonstrate the existence of `type AllPallets`.
	#[frame::pallets]
	pub type AllPallets = (
		System = frame_system::pallet = 0,
		BalancesFoo = pallet_balances::pallet = 1,
		BalancesBar = pallet_balances::pallet = 2,
		Staking = pallet_staking::pallet = 42,
		Session = pallet_session::pallet = 43,
	);

	/// This can either be a tuple of pallet names, eg
	///
	/// #[frame::call] pub enum RuntimeCall = (System, BalancesFoo, Staking)
	///
	/// which omits a number of calls, or be the below syntax, which means "derive it from
	/// AllPallets, in the exact same manner".
	#[frame::call]
	pub enum RuntimeCall = ..AllPallets;

	/// Same semantics as `RuntimeCall`.
	#[frame::event]
	pub enum RuntimeEvent = ..AllPallets;

	/// Same semantics as `RuntimeCall`.
	#[frame::origin]
	pub enum RuntimeOrigin = ..AllPallets;

	/// Same semantics as `RuntimeCall`.
	#[frame::genesis_config]
	pub enum RuntimeGenesis = ..AllPallet;

	/// Same semantics as `RuntimeCall`.
	/// This is slightly different from how we do this now. Now, we implement 
	/// `ValidateUnsigned` for `Runtime`, now we should implement it for a new 
	/// type and pass that one to executive.
	#[frame::validate_unsigned]
	pub type UnsignedValidator = ..AllPallets; 

	/// This will auto-populate this module with a known list of types and re-exports that will be
	/// useful either in the rest of this block (`Block`, `Header` etc), or the ones that are
	/// typically used by the node to build genesis (`AccountId`, `OpaqueBlock`) etc.
	#[frame::runtime_prelude]
	pub mod prelude {}

	/// Further Idea paritytech/substrate#1: This can automatically generate nested builders that create a struct with
	/// fields corresponding to the `GenesisConfig` of each pallet.
	///
	/// See: https://github.com/paritytech/substrate/issues/13463#issuecomment-1464013060
	#[frame::ext_builder]
	pub struct ExtBuilder {}

	/// Further idea paritytech/substrate#2: The parent annotation could be
	/// #[frame::construct_runtime(testing|parachain|solo-chain)] which could enforce more rules,
	/// elide more types.
	///
	/// See: https://github.com/paritytech/substrate/issues/13321
}

/// Finally, other than `#[frame::runtime] mod runtime`, this is your single exposed
pub use runtime::prelude::*;

[bkchr]

	/// NOTE: In the future, the execution of `OnRuntimeUpgrade` should be fully delegated to a
	/// single "migration dispatcher" pallet (probably the same one as
	/// https://github.com/paritytech/substrate/pull/14275), so this last argument can be removed.

Migrations are running before on_initialize and on_initialize is controlled by Executive. We could add some extra Hooks, however I'm not sure if this is better.

[gupnik]

How do we feel about the following to maintain rust-like syntax for AllPallets:

#[frame_support::construct_runtime_v2]
mod runtime {
	#[frame::runtime]
	pub struct Runtime;

	#[frame::pallets]
	pub struct AllPallets {
		System: frame_system,
		Timestamp: pallet_timestamp,
		Aura: pallet_aura,
		Grandpa: pallet_grandpa,
		Balances: pallet_balances,
		TransactionPayment: pallet_transaction_payment,
		Sudo: pallet_sudo,
		TemplateModule: pallet_template,
	}

        // Optional
	#[frame::indices]
	pub enum AllPalletIndices {
		System = 0,
		Timestamp = 1,
		Aura = 2,
		Grandpa = 3,
		Balances = 4,
		TransactionPayment = 5,
		Sudo = 6,
		TemplateModule = 7,
	}
}

[bkchr]

IMO declaring the pallets and indices should be done in one place and not split up.

[ggwpez]

Taking naming and instancing into account, maybe:

#[frame::pallets]
pub enum Pallets {
	System      = frame_system::pallet    { index: 0, ..Default::default() },
	BalancesFoo = pallet_balances::pallet { index: 1, instance: 1 },
	BalancesBar = pallet_balances::pallet { index: 2, instance: 2 }
};

or when wanting to indicate that the index is not really pallet specific:

#[frame::pallets]
pub enum Pallets {
	System      = (0, frame_system::pallet::default()),
	BalancesFoo = (1, pallet_balances::pallet { instance: 1 }),
	BalancesBar = (2, pallet_balances::pallet { instance: 2 })
};

[gupnik]

@ggwpez I really like this one. Will explore this further.

#[frame::pallets]
pub enum Pallets {
	System      = (0, frame_system::pallet::default()),
	BalancesFoo = (1, pallet_balances::pallet { instance: 1 }),
	BalancesBar = (2, pallet_balances::pallet { instance: 2 })
};

[bkchr]

Not sure this syntax is that great 🙈

First we should think about what kind of features we need to support. I can think of index, instance, disable_calls and disable_unsigned? By disable_calls I mean that the runtime will not export the calls of the pallet, so they will be un-callable. By disable_unsigned I mean that the runtime doesn't want to support the unsigned calls exposed by the call (aka not exposing the UnsignedValidator of this pallet.
All the other rest like Pallet, Event etc should not be anymore required to be specified by the user.

[gui1117]

Trying to use a pseudo rust types to declare pallets can be confusing.
In this case what is the enum Pallets , is it really declared at the end or removed by the macro?

Kian proposal where items the declared items are really defined by the macro is less confusing IMO.
though the non-rust syntax might be better inside attributes (#[....])

maybe for declaring a pallet we can go with a more explicit declaration a bit like json like this, but it can be too verbose:

pallet(name: Balance, path: pallet_balances, index: 1)
pallet(name: Balance2, path: pallet_balances, index: 2, instance: 4, disable_call, disable_inherent)

or

pallet { name: Balance, path: pallet_balances, index: 1 }
pallet { name: Balance2, path: pallet_balances, index: 2, instance: 4, flags: [disable_call, disable_inherent] }

[gui1117]

while trying to figure out what flag are interesting for users, I made this summary of the code generated by construct_runtime

• 1 call:

  declare type RuntimeCall like a normal aggregation
  derive encode/decode/typeinfo/clone/partialea/eq
  add the use of index when deriving encode/decode

  little magic: generate for each the pallet a call type (e.g. BalancesCall)

  implement trait GetDispatchInfo like a normal aggregation
  implement trait GetCallMetadata like a normal aggregation
  implement trait UnfilteredDispatchable: like a normal aggregation
  implement trait From for each variant inner type like a normal aggregation
  implement trait IsSubType for each variant inner type like a normal aggregation

  implement function sizes: seems like an internal function
  implement function assert_size_under: implementation is quite expected

  MAGIC: implement trait Dispatchable: call filter_call then call dispatch_bypass_filter

• 2 genesis config:

  little magic: generate for each pallet a config type (e.g. BalancesConfig = pallet_balances::GenesisConfig)
  NOTE: this is probably an old artefact to remove

  declare type RuntimeGenesisConfig like a normal aggregation
  derive Serialize/Deserialize/Default

  implement trait BuildStorage like a normal aggregation:
  NOTE: use declaration order for building order (and not index order)

• 3 event:

  declare type RuntimeEvent like a normal aggregation

  implement From and TryInto for each variant inner type like a normal aggregation

• 4 freeze_reason:

  declare type RuntimeFreezeReason like a normal aggregation
  derive Copy/Clone/Eq/PartialEq/Ord/PartialOrd/Encode/Decode/MaxEncodedLen/TypeInfo

  implement From for each variant inner type like a normal aggregation

• 5 hold_reason: same as freeze_reason

• 6 slash_reason: same as freeze_reason

• 7 inherent:

  MAGIC: declare trait InherentDataExt

  implement function InherentDataExt::create_extrinsics in an expected way

  MAGIC: implement function InherentDataExt::check_extrinsics with quite some specific logic using is_inherent and check_inherent implementations in pallet
  NOTE: should be implemented in an abstraction outside macro

  MAGIC: implement trait EnsureInherentAreFirst with quite some specific logic using is_inherent implementations in pallet
  NOTE: should be implemented in an abstraction outside macro

  TODO: instead we should have check_inherent and is_inherent implemented for RuntimeCall by the macro. And other logic in executive or frame_support

• 8 origin:

  MAGIC: declare type RuntimeOrigin with 2 fields: caller and filter
  MAGIC: implement trait OriginTrait
  NOTE: both should probably be implemented in an abstratction outside the macro

  declare type OriginCaller implemented like a normal aggregation

  implement function none
  implement function root
  implement function signed
  implement trait From<system origin> for OriginCaller
  implement trait From<system origin> for RuntimeOrigin
  implement trait From<OriginCaller> for RuntimeOrigin
  implement trait CallerTrait

  implement trait TryFrom<OriginCaller> for system origin
  implement trait From<RuntimeOrigin> for Result<system origin, RuntimeOrigin>
  implement trait From<Option<AccountId>> for RuntimeOrigin

  implement traits From/TryFrom for other variant than system and both OriginCaller and RuntimeOrigin
  implement some other conversions for other variant than system

• 9 metadata: let's skip

• 10 lock_id: same as freeze_reason

• 11 validate unsigned:

  implement trait ValidateUnsigned like a normal aggregation

• 12 runtime

  declare struct Runtime

  implement traits GetNodeBlockType and GetRuntimeBlockType for Runtime
  NOTE: we could make this a real trait implementation for user, or move them to frame_system::Config instead of current syntax:

  	pub struct Runtime where
  		Block = Block,
  		NodeBlock = node_primitives::Block,

  declare a type PalletInfo and implement the trait PalletInfo on it. Implementation is a normal aggregation

  declare some tests for error encoded size and runtime integrity tests.

Final notes

• inherent logic should be implemented inside executive or support, and only the aggregation should be done by the macro.
• maybe origin implementation can improved by abstracting some parts outside the macro.
• the filter of the call added to dispatch and origin with a filter is bit magical indeed, I don't know how to improve though.

[kianenigma]

First we should think about what kind of features we need to support. I can think of index, instance, disable_calls and disable_unsigned? By disable_calls I mean that the runtime will not export the calls of the pallet, so they will be un-callable. By disable_unsigned I mean that the runtime doesn't want to support the unsigned calls exposed by the call (aka not exposing the UnsignedValidator of this pallet.
All the other rest like Pallet, Event etc should not be anymore required to be specified by the user.

I can't find disable_calls or disable_unsigned. You probably mean exclude_parts and include_parts?

I think all of this should handled outside of where you define the list of pallets. This is one fundamental difference between my suggestion (or its general direction) and what we currently have.

Now, if you have 3 pallets and you want to include the call of two of them only, it will look something like this:

construct_runtime!(
  PalletA: pallet_a::{Call, GenesisConfig} = 0,
  PalletB: pallet_b::{Call, GenesisConfig} = 0
  PalletB: pallet_c::{GenesisConfig} = 0
)

This is mixing 3 concepts: instancing, indexing, and individual parts. I am suggesting moving the latter to separate types/macros. This is:

1. Explicit
2. Useful for things like RuntimeCall and RuntimeEvent and RuntimeGenesisConfig and such to become explicit.

So the above example, in my suggestion, would be:

#[frame::runtime] {
	// details of this block's syntax is itself TBD, but it should only specify index and instance
	#[frame::pallets]
	pub type AllPallets = (
		PalletA = 0, 
		PalletB = 1, 
		PalletC = 2
	);

	#[frame::call]
	pub enum RuntimeCall = (PalletA, PalletB);
	
	#[frame::genesis_config]
	pub enum RuntimeGenesis = ..AllPallet;
}

I skipped UnsignedValidator initially, but I think it can be easy enough to add. Updated my comment here.

@gupnik The action-item for you here to investigate an exhaustive list of features that the current construct_runtime has (@thiolliere's comment above helps, but you should look at the code docs of construct_runtime) and map them to the next syntax suggestions to see which one fulfills all, and best.

For example, I know that you can specify Pallet = pallet::{Storage} and I am not sure what this is supposed to mean.

Also, you should investigate the initial request from @arrudagates and if/how it can be fulfilled with the new proposal. I feel like I hijacked his issue here for something unrelated 😅

(As a side note, the keyword Config in construct_runtime should actually be GenesisConfig as it is currently at the risk of being confused with trait Config, cc @juangirini)

[bkchr]

I can't find disable_calls or disable_unsigned. You probably mean exclude_parts and include_parts?

They don't exist currently. Only the ones you mentioned. I thought about the new syntax.

For example, I know that you can specify Pallet = pallet::{Storage} and I am not sure what this is supposed to mean.

Nothing and we should just forget about this. This is what I tried to express, that we don't need all these config options.

(As a side note, the keyword Config in construct_runtime should actually be GenesisConfig as it is currently at the risk of being confused with trait Config, cc @juangirini)

Not sure we should touch this and not just keep it until we have the new syntax.

[juangirini]

Not sure we should touch this and not just keep it until we have the new syntax.

Agree, I have created an issue anyway to keep it in mind though.

[gupnik]

As discussed during the retreat, we could use the following syntax to provide a reasonable default:

#[frame::construct_runtime]
mod runtime {
	#[frame::runtime]
	pub struct Runtime;

	#[frame::pallets]
        #[derive(RuntimeGenesisConfig, RuntimeCall, RuntimeOrigin)] // Aggregate all pallets by default
	pub type AllPallets = (
		System = frame_system::pallet = 0,
		BalancesFoo = pallet_balances::pallet = 1,
		BalancesBar = pallet_balances::pallet = 2,
		Staking = pallet_staking::pallet = 42,
		Session = pallet_session::pallet = 43,
	);
}

There could be an additional syntax to exclude some pallet parts if needed.

[RomarQ]

I see what you want to achieve. Currently this is already achievable without forking frame-support by creating a newtype wrapper for the RuntimeOrigin. On this newtype wrapper you could implement the Into<Result<RawOrigin, Self>> as you need it. Then you would set this WrappedRuntimeOrigin as your runtime origin to all the pallets.

We could also add some attribute #[do_not_implement_into_raw_origin] (shitty naming :P) to the current construct_runtime macro to skip the implementation. This is then a short term fix for your issue. I think even in the future dreamed by @kianenigma you would need such an attribute as per default this should always be implemented.

@bkchr How do you suggest wrapping RuntimeOrigin?

I have tried and it seems to be a lot of work with all the implementations required. For now, the only easy solution I see, is to also fork frame-support.

[bkchr]

@bkchr How do you suggest wrapping RuntimeOrigin?

Yes.

[shawntabrizi]

As discussed during the retreat, we could use the following syntax to provide a reasonable default:

#[frame::construct_runtime]
mod runtime {
	#[frame::runtime]
	pub struct Runtime;

	#[frame::pallets]
        #[derive(RuntimeGenesisConfig, RuntimeCall, RuntimeOrigin)] // Aggregate all pallets by default
	pub type AllPallets = (
		System = frame_system::pallet = 0,
		BalancesFoo = pallet_balances::pallet = 1,
		BalancesBar = pallet_balances::pallet = 2,
		Staking = pallet_staking::pallet = 42,
		Session = pallet_session::pallet = 43,
	);
}

There could be an additional syntax to exclude some pallet parts if needed.

I wanted exactly this when trying to learn about type RuntimeHoldReason = RuntimeHoldReason; and I could not find RuntimeHoldReason in the codebase.

Deeper digging, I realized it was being generated by construct_runtime and this was not obvious.