Don't require module name in inherents (#6576)

* Start

* Cleanup `construct_runtime!`

* Add tests

* Fix after merge

* Update the docs

substrate/frame/support/Cargo.toml

@@ -34,6 +34,7 @@ smallvec = "1.4.0"
 [dev-dependencies]
 pretty_assertions = "0.6.1"
 frame-system = { version = "2.0.0-rc4", path = "../system" }
+parity-util-mem = { version = "0.6.1", features = ["primitive-types"] }
 
 [features]
 default = ["std"]

substrate/frame/support/procedural/src/construct_runtime/mod.rs

@@ -87,7 +87,12 @@ fn construct_runtime_parsed(definition: RuntimeDefinition) -> Result<TokenStream
 	let dispatch = decl_outer_dispatch(&name, modules.iter(), &scrate);
 	let metadata = decl_runtime_metadata(&name, modules.iter(), &scrate, &unchecked_extrinsic);
 	let outer_config = decl_outer_config(&name, modules.iter(), &scrate);
-	let inherent = decl_outer_inherent(&block, &unchecked_extrinsic, modules.iter(), &scrate);
+	let inherent = decl_outer_inherent(
+		&block,
+		&unchecked_extrinsic,
+		modules.iter(),
+		&scrate,
+	);
 	let validate_unsigned = decl_validate_unsigned(&name, modules.iter(), &scrate);
 	let integrity_test = decl_integrity_test(&scrate);
 
@@ -153,19 +158,17 @@ fn decl_outer_inherent<'a>(
 ) -> TokenStream2 {
 	let modules_tokens = module_declarations.filter_map(|module_declaration| {
 		let maybe_config_part = module_declaration.find_part("Inherent");
-		maybe_config_part.map(|config_part| {
-			let arg = config_part
-				.args
-				.as_ref()
-				.and_then(|parens| parens.content.inner.iter().next())
-				.unwrap_or(&module_declaration.name);
+		maybe_config_part.map(|_| {
 			let name = &module_declaration.name;
-			quote!(#name : #arg,)
+			quote!(#name,)
 		})
 	});
 	quote!(
 		#scrate::impl_outer_inherent!(
-			impl Inherents where Block = #block, UncheckedExtrinsic = #unchecked_extrinsic {
+			impl Inherents where
+				Block = #block,
+				UncheckedExtrinsic = #unchecked_extrinsic
+			{
 				#(#modules_tokens)*
 			}
 		);

substrate/frame/support/procedural/src/construct_runtime/parse.rs

@@ -279,18 +279,6 @@ impl ModulePartKeyword {
 		Ident::new(self.name(), self.span())
 	}
 
-	/// Returns `true` if this module part allows to have an argument.
-	///
-	/// For example `Inherent(Timestamp)`.
-	fn allows_arg(&self) -> bool {
-		Self::all_allow_arg().iter().any(|n| *n == self.name())
-	}
-
-	/// Returns the names of all module parts that allow to have an argument.
-	fn all_allow_arg() -> &'static [&'static str] {
-		&["Inherent"]
-	}
-
 	/// Returns `true` if this module part is allowed to have generic arguments.
 	fn allows_generic(&self) -> bool {
 		Self::all_generic_arg().iter().any(|n| *n == self.name())
@@ -321,7 +309,6 @@ impl Spanned for ModulePartKeyword {
 pub struct ModulePart {
 	pub keyword: ModulePartKeyword,
 	pub generics: syn::Generics,
-	pub args: Option<ext::Parens<ext::Punctuated<Ident, Token![,]>>>,
 }
 
 impl Parse for ModulePart {
@@ -339,27 +326,10 @@ impl Parse for ModulePart {
 			);
 			return Err(syn::Error::new(keyword.span(), msg));
 		}
-		let args = if input.peek(token::Paren) {
-			if !keyword.allows_arg() {
-				let syn::group::Parens { token: parens, .. } = syn::group::parse_parens(input)?;
-				let valid_names = ModulePart::format_names(ModulePartKeyword::all_allow_arg());
-				let msg = format!(
-					"`{}` is not allowed to have arguments in parens. \
-					 Only the following modules are allowed to have arguments in parens: {}.",
-					keyword.name(),
-					valid_names,
-				);
-				return Err(syn::Error::new(parens.span, msg));
-			}
-			Some(input.parse()?)
-		} else {
-			None
-		};
 
 		Ok(Self {
 			keyword,
 			generics,
-			args,
 		})
 	}
 }

substrate/frame/support/procedural/src/lib.rs

@@ -277,10 +277,8 @@ pub fn decl_storage(input: TokenStream) -> TokenStream {
 /// - `Event` or `Event<T>` (if the event is generic)
 /// - `Origin` or `Origin<T>` (if the origin is generic)
 /// - `Config` or `Config<T>` (if the config is generic)
-/// - `Inherent ( $(CALL),* )` - If the module provides/can check inherents. The optional parameter
-///                             is for modules that use a `Call` from a different module as
-///                             inherent.
-/// - `ValidateUnsigned`      - If the module validates unsigned extrinsics.
+/// - `Inherent` - If the module provides/can check inherents.
+/// - `ValidateUnsigned` - If the module validates unsigned extrinsics.
 ///
 /// # Note
 ///

substrate/frame/support/src/dispatch.rs

@@ -54,7 +54,7 @@ pub trait Callable<T> {
 
 // dirty hack to work around serde_derive issue
 // https://github.com/rust-lang/rust/issues/51331
-pub type CallableCallFor<A, T> = <A as Callable<T>>::Call;
+pub type CallableCallFor<A, R> = <A as Callable<R>>::Call;
 
 /// A type that can be used as a parameter in a dispatchable function.
 ///
@@ -1848,8 +1848,8 @@ macro_rules! decl_module {
 	}
 }
 
-pub trait IsSubType<T: Callable<R>, R> {
-	fn is_sub_type(&self) -> Option<&CallableCallFor<T, R>>;
+pub trait IsSubType<T> {
+	fn is_sub_type(&self) -> Option<&T>;
 }
 
 /// Implement a meta-dispatch module to dispatch to other dispatchers.
@@ -1948,7 +1948,7 @@ macro_rules! impl_outer_dispatch {
 		}
 
 		$(
-			impl $crate::dispatch::IsSubType<$camelcase, $runtime> for $call_type {
+			impl $crate::dispatch::IsSubType<$crate::dispatch::CallableCallFor<$camelcase, $runtime>> for $call_type {
 				#[allow(unreachable_patterns)]
 				fn is_sub_type(&self) -> Option<&$crate::dispatch::CallableCallFor<$camelcase, $runtime>> {
 					match *self {
@@ -2372,72 +2372,72 @@ mod tests {
 	}
 
 	const EXPECTED_METADATA: &'static [FunctionMetadata] = &[
-				FunctionMetadata {
-					name: DecodeDifferent::Encode("aux_0"),
-					arguments: DecodeDifferent::Encode(&[]),
-					documentation: DecodeDifferent::Encode(&[
-						" Hi, this is a comment."
-					])
+		FunctionMetadata {
+			name: DecodeDifferent::Encode("aux_0"),
+			arguments: DecodeDifferent::Encode(&[]),
+			documentation: DecodeDifferent::Encode(&[
+				" Hi, this is a comment."
+			])
+		},
+		FunctionMetadata {
+			name: DecodeDifferent::Encode("aux_1"),
+			arguments: DecodeDifferent::Encode(&[
+				FunctionArgumentMetadata {
+					name: DecodeDifferent::Encode("_data"),
+					ty: DecodeDifferent::Encode("Compact<u32>")
+				}
+			]),
+			documentation: DecodeDifferent::Encode(&[]),
+		},
+		FunctionMetadata {
+			name: DecodeDifferent::Encode("aux_2"),
+			arguments: DecodeDifferent::Encode(&[
+				FunctionArgumentMetadata {
+					name: DecodeDifferent::Encode("_data"),
+					ty: DecodeDifferent::Encode("i32"),
 				},
-				FunctionMetadata {
-					name: DecodeDifferent::Encode("aux_1"),
-					arguments: DecodeDifferent::Encode(&[
-						FunctionArgumentMetadata {
-							name: DecodeDifferent::Encode("_data"),
-							ty: DecodeDifferent::Encode("Compact<u32>")
-						}
-					]),
-					documentation: DecodeDifferent::Encode(&[]),
+				FunctionArgumentMetadata {
+					name: DecodeDifferent::Encode("_data2"),
+					ty: DecodeDifferent::Encode("String"),
+				}
+			]),
+			documentation: DecodeDifferent::Encode(&[]),
+		},
+		FunctionMetadata {
+			name: DecodeDifferent::Encode("aux_3"),
+			arguments: DecodeDifferent::Encode(&[]),
+			documentation: DecodeDifferent::Encode(&[]),
+		},
+		FunctionMetadata {
+			name: DecodeDifferent::Encode("aux_4"),
+			arguments: DecodeDifferent::Encode(&[
+				FunctionArgumentMetadata {
+					name: DecodeDifferent::Encode("_data"),
+					ty: DecodeDifferent::Encode("i32"),
+				}
+			]),
+			documentation: DecodeDifferent::Encode(&[]),
+		},
+		FunctionMetadata {
+			name: DecodeDifferent::Encode("aux_5"),
+			arguments: DecodeDifferent::Encode(&[
+				FunctionArgumentMetadata {
+					name: DecodeDifferent::Encode("_data"),
+					ty: DecodeDifferent::Encode("i32"),
 				},
-				FunctionMetadata {
-					name: DecodeDifferent::Encode("aux_2"),
-					arguments: DecodeDifferent::Encode(&[
-						FunctionArgumentMetadata {
-							name: DecodeDifferent::Encode("_data"),
-							ty: DecodeDifferent::Encode("i32"),
-						},
-						FunctionArgumentMetadata {
-							name: DecodeDifferent::Encode("_data2"),
-							ty: DecodeDifferent::Encode("String"),
-						}
-					]),
-					documentation: DecodeDifferent::Encode(&[]),
-				},
-				FunctionMetadata {
-					name: DecodeDifferent::Encode("aux_3"),
-					arguments: DecodeDifferent::Encode(&[]),
-					documentation: DecodeDifferent::Encode(&[]),
-				},
-				FunctionMetadata {
-					name: DecodeDifferent::Encode("aux_4"),
-					arguments: DecodeDifferent::Encode(&[
-						FunctionArgumentMetadata {
-							name: DecodeDifferent::Encode("_data"),
-							ty: DecodeDifferent::Encode("i32"),
-						}
-					]),
-					documentation: DecodeDifferent::Encode(&[]),
-				},
-				FunctionMetadata {
-					name: DecodeDifferent::Encode("aux_5"),
-					arguments: DecodeDifferent::Encode(&[
-						FunctionArgumentMetadata {
-							name: DecodeDifferent::Encode("_data"),
-							ty: DecodeDifferent::Encode("i32"),
-						},
-						FunctionArgumentMetadata {
-							name: DecodeDifferent::Encode("_data2"),
-							ty: DecodeDifferent::Encode("Compact<u32>")
-						}
-					]),
-					documentation: DecodeDifferent::Encode(&[]),
-				},
-				FunctionMetadata {
-					name: DecodeDifferent::Encode("operational"),
-					arguments: DecodeDifferent::Encode(&[]),
-					documentation: DecodeDifferent::Encode(&[]),
-				},
-			];
+				FunctionArgumentMetadata {
+					name: DecodeDifferent::Encode("_data2"),
+					ty: DecodeDifferent::Encode("Compact<u32>")
+				}
+			]),
+			documentation: DecodeDifferent::Encode(&[]),
+		},
+		FunctionMetadata {
+			name: DecodeDifferent::Encode("operational"),
+			arguments: DecodeDifferent::Encode(&[]),
+			documentation: DecodeDifferent::Encode(&[]),
+		},
+	];
 
 	pub struct TraitImpl {}
 

substrate/frame/support/src/inherent.rs

@@ -31,19 +31,20 @@ pub use sp_inherents::{InherentData, ProvideInherent, CheckInherentsResult, IsFa
 /// ```nocompile
 /// impl_outer_inherent! {
 ///     impl Inherents where Block = Block, UncheckedExtrinsic = UncheckedExtrinsic {
-///         timestamp: Timestamp,
-///         consensus: Consensus,
-///         /// Aura module using the `Timestamp` call.
-///         aura: Timestamp,
+///         timestamp,
+///         consensus,
+///         aura,
 ///     }
 /// }
 /// ```
 #[macro_export]
 macro_rules! impl_outer_inherent {
 	(
-		impl Inherents where Block = $block:ident, UncheckedExtrinsic = $uncheckedextrinsic:ident
+		impl Inherents where
+			Block = $block:ident,
+			UncheckedExtrinsic = $uncheckedextrinsic:ident
 		{
-			$( $module:ident: $call:ident, )*
+			$( $module:ident, )*
 		}
 	) => {
 		trait InherentDataExt {
@@ -55,15 +56,14 @@ macro_rules! impl_outer_inherent {
 		impl InherentDataExt for $crate::inherent::InherentData {
 			fn create_extrinsics(&self) ->
 				$crate::inherent::Vec<<$block as $crate::inherent::BlockT>::Extrinsic> {
-				use $crate::inherent::ProvideInherent;
-				use $crate::inherent::Extrinsic;
+				use $crate::inherent::{ProvideInherent, Extrinsic};
 
 				let mut inherents = Vec::new();
 
 				$(
 					if let Some(inherent) = $module::create_inherent(self) {
 						inherents.push($uncheckedextrinsic::new(
-							Call::$call(inherent),
+							inherent.into(),
 							None,
 						).expect("Runtime UncheckedExtrinsic is not Opaque, so it has to return `Some`; qed"));
 					}
@@ -74,6 +74,7 @@ macro_rules! impl_outer_inherent {
 
 			fn check_extrinsics(&self, block: &$block) -> $crate::inherent::CheckInherentsResult {
 				use $crate::inherent::{ProvideInherent, IsFatalError};
+				use $crate::dispatch::IsSubType;
 
 				let mut result = $crate::inherent::CheckInherentsResult::new();
 				for xt in block.extrinsics() {
@@ -81,21 +82,18 @@ macro_rules! impl_outer_inherent {
 						break
 					}
 
-					$(
-						match xt.function {
-							Call::$call(ref call) => {
-								if let Err(e) = $module::check_inherent(call, self) {
-									result.put_error(
-										$module::INHERENT_IDENTIFIER, &e
-									).expect("There is only one fatal error; qed");
-									if e.is_fatal_error() {
-										return result
-									}
+					$({
+						if let Some(call) = IsSubType::<_>::is_sub_type(&xt.function) {
+							if let Err(e) = $module::check_inherent(call, self) {
+								result.put_error(
+									$module::INHERENT_IDENTIFIER, &e
+								).expect("There is only one fatal error; qed");
+								if e.is_fatal_error() {
+									return result
 								}
 							}
-							_ => {},
 						}
-					)*
+					})*
 				}
 
 				$(
@@ -106,10 +104,10 @@ macro_rules! impl_outer_inherent {
 									return false
 								}
 
-								match xt.function {
-									Call::$call(_) => true,
-									_ => false,
-								}
+								let call: Option<&<$module as ProvideInherent>::Call> =
+									xt.function.is_sub_type();
+
+								call.is_some()
 							});
 
 							if !found {
@@ -138,3 +136,139 @@ macro_rules! impl_outer_inherent {
 		}
 	};
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use sp_runtime::{traits, testing::{Header, self}};
+	use crate::dispatch::IsSubType;
+
+	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
+	enum Call {
+		Test(CallTest),
+		Test2(CallTest2),
+	}
+
+	impl From<CallTest> for Call {
+		fn from(call: CallTest) -> Self {
+			Self::Test(call)
+		}
+	}
+
+	impl From<CallTest2> for Call {
+		fn from(call: CallTest2) -> Self {
+			Self::Test2(call)
+		}
+	}
+
+	impl IsSubType<CallTest> for Call {
+		fn is_sub_type(&self) -> Option<&CallTest> {
+			match self {
+				Self::Test(test) => Some(test),
+				_ => None,
+			}
+		}
+	}
+
+	impl IsSubType<CallTest2> for Call {
+		fn is_sub_type(&self) -> Option<&CallTest2> {
+			match self {
+				Self::Test2(test) => Some(test),
+				_ => None,
+			}
+		}
+	}
+
+	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
+	enum CallTest {
+		Something,
+		SomethingElse,
+	}
+
+	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
+	enum CallTest2 {
+		Something,
+	}
+
+	struct ModuleTest;
+	impl ProvideInherent for ModuleTest {
+		type Call = CallTest;
+		type Error = sp_inherents::MakeFatalError<()>;
+		const INHERENT_IDENTIFIER: sp_inherents::InherentIdentifier = *b"test1235";
+
+		fn create_inherent(_: &InherentData) -> Option<Self::Call> {
+			Some(CallTest::Something)
+		}
+
+		fn check_inherent(call: &Self::Call, _: &InherentData) -> Result<(), Self::Error> {
+			match call {
+				CallTest::Something => Ok(()),
+				CallTest::SomethingElse => Err(().into()),
+			}
+		}
+	}
+
+	struct ModuleTest2;
+	impl ProvideInherent for ModuleTest2 {
+		type Call = CallTest2;
+		type Error = sp_inherents::MakeFatalError<()>;
+		const INHERENT_IDENTIFIER: sp_inherents::InherentIdentifier = *b"test1234";
+
+		fn create_inherent(_: &InherentData) -> Option<Self::Call> {
+			Some(CallTest2::Something)
+		}
+	}
+
+	type Block = testing::Block<Extrinsic>;
+
+	#[derive(codec::Encode, codec::Decode, Clone, PartialEq, Eq, Debug, serde::Serialize)]
+	struct Extrinsic {
+		function: Call,
+	}
+
+	impl traits::Extrinsic for Extrinsic {
+		type Call = Call;
+		type SignaturePayload = ();
+
+		fn new(function: Call, _: Option<()>) -> Option<Self> {
+			Some(Self { function })
+		}
+	}
+
+	parity_util_mem::malloc_size_of_is_0!(Extrinsic);
+
+	impl_outer_inherent! {
+		impl Inherents where Block = Block, UncheckedExtrinsic = Extrinsic {
+			ModuleTest,
+			ModuleTest2,
+		}
+	}
+
+	#[test]
+	fn create_inherents_works() {
+		let inherents = InherentData::new().create_extrinsics();
+
+		let expected = vec![
+			Extrinsic { function: Call::Test(CallTest::Something) },
+			Extrinsic { function: Call::Test2(CallTest2::Something) },
+		];
+		assert_eq!(expected, inherents);
+	}
+
+	#[test]
+	fn check_inherents_works() {
+		let block = Block::new(
+			Header::new_from_number(1),
+			vec![Extrinsic { function: Call::Test(CallTest::Something) }],
+		);
+
+		assert!(InherentData::new().check_extrinsics(&block).ok());
+
+		let block = Block::new(
+			Header::new_from_number(1),
+			vec![Extrinsic { function: Call::Test(CallTest::SomethingElse) }],
+		);
+
+		assert!(InherentData::new().check_extrinsics(&block).fatal_error());
+	}
+}

substrate/frame/support/test/tests/construct_runtime_ui/params_in_invalid_module.rs

@@ -1,14 +0,0 @@
-use frame_support::construct_runtime;
-
-construct_runtime! {
-	pub enum Runtime where
-		Block = Block,
-		NodeBlock = Block,
-		UncheckedExtrinsic = UncheckedExtrinsic
-	{
-		System: system::{Module},
-		Balance: balances::<Instance1>::{Call(toto), Origin<I>},
-	}
-}
-
-fn main() {}

substrate/frame/support/test/tests/construct_runtime_ui/params_in_invalid_module.stderr

@@ -1,5 +0,0 @@
-error: `Call` is not allowed to have arguments in parens. Only the following modules are allowed to have arguments in parens: `Inherent`.
-  --> $DIR/params_in_invalid_module.rs:10:40
-   |
-10 |         Balance: balances::<Instance1>::{Call(toto), Origin<I>},
-   |                                              ^^^^^^