Use traits to fix and document API between a module and construct runtime.

[gui1117]

Context

Often people don't really see what is implemented on modules and how output of our macros are used by construct_runtime. Not knowing what information is actually used by the construct_runtime macro makes it difficult to understand what is actually the outer API of a module.

For instance it is not straightforward looking at our macros what are actual information gather into the metadata.

Proposition

One solution would be to use traits to make this module interface documented and more understandable.

This will also improve construct_runtime itself as using those traits implemented by our macros will avoid some type issue (like Config<T> versus Config with the weird error saying that there is a wrong number of generic parameter for a certain type)

As an example we can write the trait

trait ModuleGenesisConfig {
    type GenesisConfig: BuildStorage +...;
}

and construct_runtime will just use <Module as ModuleGenesisConfig>::GenesisConfig to access the GenesisConfig type

Draft

we can create a new module in srml/support/src/module_interface.rs

use srml_metadata::ModuleErrorMetadata; // TODO TODO: Choose either put all of them in metadata or here

/// Necessary type defined by the system module trait.
pub trait SystemTrait {
	type BlockNumber;
	// TODO TODO: what is strictly required by construct_runtime ?
	// type Origin;
	// type Hash;
	// type AccountId;
	// type Event;
}

/// Constant metadata allows to put some runtime constant into metadata.
/// Changing those constant must requires a runtime upgrade.
///
/// This is usually implemented by `decl_module`.
pub trait ModuleConstantMetadata {
	fn module_constants_metadata() -> &'static [crate::dispatch::ModuleConstantMetadata];
}

/// Call metadata allows to put calls signature and documentation into metadata.
///
/// This is usually implemented by `decl_module`.
pub trait ModuleCallMetadata {
	fn call_functions() -> &'static [crate::dispatch::FunctionMetadata];
}

/// Event metadata allows to put event type into metadata.
/// This is useful so UI can process event information.
///
/// This is usually implemented by `decl_event`.
pub trait ModuleEventMetadata {
	fn metadata() -> &'static [crate::event::EventMetadata];
}

/// `ModuleEvent` defines the event type of the module, this type is used to construct outer
/// runtime event which will be an enumeration of all modules events.
///
/// This is usually implemented by `decl_event`.
pub trait ModuleEvent {
	type Event: Clone + PartialEq + Eq + codec::Encode + codec::Decode + core::fmt::Debug;
}

pub trait ModuleGenesisConfig {
	type Config: sr_primitives::BuildModuleGenesisStorage + sr_primitives::Serialize + sr_primitives::Deserialize;
	// TODO TODO: also build storage for a module should be related to module instead of config only,
	//            this way we could remove inherited_instance creation and generic parameter of
	//            BuildModuleGenesisStorage trait as if we implement for modules then generics are
	//            correctly used.
}

/// Module interface is the mandatory trait to be implemented by module to be used in
/// `construct_runtime`.
///
/// This trait is automatically implemented for any types implementing its bound. Bounds
/// implementation are advised to be done through support macro such as:
/// * `decl_module`
/// * `decl_storage`
/// * `decl_event`
pub trait ModuleInterface<T: SystemTrait>:
	sr_primitives::traits::OnInitialize<<T as SystemTrait>::BlockNumber>
	+ sr_primitives::traits::OnFinalize<<T as SystemTrait>::BlockNumber>
	+ sr_primitives::traits::OffchainWorker<<T as SystemTrait>::BlockNumber>
	+ crate::dispatch::Callable<T>
	+ crate::unsigned::ValidateUnsigned<Call = <Self as crate::dispatch::Callable<T>>::Call>
	+ ModuleEvent
	+ ModuleConstantMetadata
	+ ModuleCallMetadata
	+ crate::dispatch::ModuleErrorMetadata
	+ ModuleEventMetadata
	+ ModuleGenesisConfig
{}

impl<Module, ModuleTrait> ModuleInterface<ModuleTrait> for Module
	where
		ModuleTrait: SystemTrait,
		Module: sr_primitives::traits::OnInitialize<<ModuleTrait as SystemTrait>::BlockNumber>
			+ sr_primitives::traits::OnFinalize<<ModuleTrait as SystemTrait>::BlockNumber>
			+ sr_primitives::traits::OffchainWorker<<ModuleTrait as SystemTrait>::BlockNumber>
			+ crate::dispatch::Callable<ModuleTrait>
			+ crate::unsigned::ValidateUnsigned<Call = <Self as crate::dispatch::Callable<ModuleTrait>>::Call>
			+ ModuleEvent
			+ ModuleConstantMetadata
			+ ModuleCallMetadata
			+ ModuleErrorMetadata
			+ ModuleEventMetadata
			+ ModuleGenesisConfig
{}

// TODO TODO: For inherent and origin this can be done as well.

Note:

in the draft you can see that GenesisConfig is now mandatory, I think it is fine to have those types mandatory, one can just make an empty implementation here.
But if we prefer to still have this struct optionnally implemented then we can just make some OptionalModuleInterface traits that is also fine

[shawntabrizi]

Looks like a really awesome direction @thiolliere

in the draft you can see that GenesisConfig is now mandatory, I think it is fine to have those types mandatory, one can just make an empty implementation here.

I agree with this point

[gui1117]

about inherents we can do:

• inherent call can only be created by its module (doesn't break current codebase, but we might want to discuss it though.)
• inherent call can only be checked by its module:
  babe needs to check timestamp::Call::Set inherent but this can be done by making an additional associated type like timestamp::Trait::AdditionalTimestampSetCheck which would be implemented by Babe,
  such associated type would make construct runtime easier as no need for with optional parenthesis.

The final trait woud look like

pub trait ModuleInherentCall: ModuleCall {
	fn create_inherent(_: &InherentData) -> Option<Self::Call> {
		None
	}
	fn check_inherent(call: &Self::Call, data: &InherentData) -> result::Result<(), Self::Error> {
		Err(RuntimeString::from("Default implementation doesn't allow any inherent").into())
	}
}

[bkchr]

* inherent call can only be checked by its module:
  babe needs to check timestamp::Call::Set inherent but this can be done by making an additional associated type like timestamp::Trait::AdditionalTimestampSetCheck which would be implemented by Babe,
  such associated type would make construct runtime easier as no need for with optional parenthesis.

I already have some ideas to remove the requirement on giving the name of the other module. However, I'm strongly against introducing any associated types to make this possible. This always requires that you as a module author are aware that someone else maybe also wants to use your inherent.

[gui1117]

This always requires that you as a module author are aware that someone else maybe also wants to use your inherent.

true, but I prefer having this someone else defined by associated type than by construct_runtime macro but maybe there is other solution.

Also a side note currently Babe is checking a precise module implementation, that doesn't work with instantiable modules.

[gui1117]

I think the idea of using trait should still be useful for some places: like for #8743
But otherwise I'm not sure we need that much more trait between pallet definition and construct_runtime.