Move some inherent logic outside of construct_runtime

[thiolliere]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Request

related #232

Currently the code generated by construct_runtime for inherent is quite consequential.

https://github.com/paritytech/substrate/blob/51b2f0ed6af8dd4facb18f1a489e192fd0673f7b/frame/support/procedural/src/construct_runtime/expand/inherent.rs#L59-L221

The logic should be defined in another place like executive or support.

Instead construct_runtime should implement is_inherent and check_inherent as an aggregation of pallets implementations (and keep current implementation of create_extrinsic)

other functions check_extrinsics (of a block) and ensure_inherent_are_first should be implemented in a normal module, not generated by the macro.

Are you willing to help with this request?

Yes! I plan to implement it

[ParthDesai]

I would like to volunteer to implement this. Is it still available? (Asking this because of z6-mentor label.)

[ggwpez]

I think this is a rather difficult issue.
Do you want to mentor this @kianenigma / @thiolliere ?

[thiolliere]

I started implementing but then tried to think about the FRAME inherent API as a whole:

Currently inherent API in FRAME and Primitives as one issue:

• only one error can be reported for an inherent identifier, but frame allow to report many non fatal error for a same inherent, if many non fatal error are reported for one inherent, only first is kept.

As a result I questioned myself:

• is non fatal error useful?
  • if yes then we may want to be able to report multiple non fatal error for the same inherent
  • if no then we could get rid of it for simplicity

Some other questions:

• is an optional inherent useful? (e.g. a pallet produce an inherent in some block only), if not should all inherent should be required by default?
• we currently ensure inherent are first but we don't check for the order. maybe it could be checked also

But this issue is more narrow than FRAME inherent API.
My thought on it was to have a trait implemented by construct_runtime which is just the aggregation of logic defined by the pallets (So behaviour is expected and simple as possible)

/// A trait implemented by a substrate runtime to provide and check inherents calls.
pub trait RuntimeProvideInherent {
	/// The call type of the runtime.
	type Call;

	/// Create all inherents out of the given `InherentData`.
	fn create_inherents(data: &InherentData) -> Vec<Self::Call>;

	/// Return whether the call is the call of an inherent.
	///
	/// NOTE: Signed extrinsics are not inherents, but a signed extrinsic with the given call
	/// variant can be dispatched. Only unsigned extrinsics with an inherent call are considered
	/// inherent extrinsics by the runtime.
	fn is_inherent(call: &Self::Call) -> bool;

	/// Check whether the given inherent is valid.
	fn check_inherent(call: &Self::Call, data: &InherentData) -> CheckInherentsResult;

	// Here I couldn't really think of a function that aggregaed the requirements like this
	// ```
	// fn inherent_existence_requirements(data: &InherentData) -> RequirementsToBeChecked??;
	// ```
	// So instead I think we can do a method like this:
	/// Check existence requirements of inherents.
	fn check_inherent_existence_requirements(
		data: &InherentData,
		inherents: &[&Self::Call],
	) -> CheckInherentsResult;
}

Then all method actually used to implement the runtime APIs should be moved to executive.

So I started a branch https://github.com/thiolliere/substrate/tree/gui-inherent-refactor
you can see the diff here https://github.com/paritytech/substrate/compare/master...thiolliere:gui-inherent-refactor?expand=1

EDIT: about mentoring you can ping me on the PR when opened, I am not very active but I can give feedback

[ParthDesai]

I started implementing but then tried to think about the FRAME inherent API as a whole:

Currently inherent API in FRAME and Primitives as one issue:

* only one error can be reported for an inherent identifier, but frame allow to report many non fatal error for a same inherent, if many non fatal error are reported for one inherent, only first is kept.

As a result I questioned myself:

* is non fatal error useful?
  
  * if yes then we may want to be able to report multiple non fatal error for the same inherent
  * if no then we could get rid of it for simplicity

Some other questions:

* is an optional inherent useful? (e.g. a pallet produce an inherent in some block only), if not should all inherent should be required by default?

* we currently ensure inherent are first but we don't check for the order. maybe it could be checked also

But this issue is more narrow than FRAME inherent API. My thought on it was to have a trait implemented by construct_runtime which is just the aggregation of logic defined by the pallets (So behaviour is expected and simple as possible)

/// A trait implemented by a substrate runtime to provide and check inherents calls.
pub trait RuntimeProvideInherent {
	/// The call type of the runtime.
	type Call;

	/// Create all inherents out of the given `InherentData`.
	fn create_inherents(data: &InherentData) -> Vec<Self::Call>;

	/// Return whether the call is the call of an inherent.
	///
	/// NOTE: Signed extrinsics are not inherents, but a signed extrinsic with the given call
	/// variant can be dispatched. Only unsigned extrinsics with an inherent call are considered
	/// inherent extrinsics by the runtime.
	fn is_inherent(call: &Self::Call) -> bool;

	/// Check whether the given inherent is valid.
	fn check_inherent(call: &Self::Call, data: &InherentData) -> CheckInherentsResult;

	// Here I couldn't really think of a function that aggregaed the requirements like this
	// ```
	// fn inherent_existence_requirements(data: &InherentData) -> RequirementsToBeChecked??;
	// ```
	// So instead I think we can do a method like this:
	/// Check existence requirements of inherents.
	fn check_inherent_existence_requirements(
		data: &InherentData,
		inherents: &[&Self::Call],
	) -> CheckInherentsResult;
}

Then all method actually used to implement the runtime APIs should be moved to executive.

So I started a branch https://github.com/thiolliere/substrate/tree/gui-inherent-refactor you can see the diff here https://github.com/paritytech/substrate/compare/master...thiolliere:gui-inherent-refactor?expand=1

EDIT: about mentoring you can ping me on the PR when opened, I am not very active but I can give feedback

I will start working on it this week. Will open a draft PR as soon as I have something and we can go from there.