Minimize Macro Magic

[shawntabrizi]

I wanted to formally share some feedback I have seen regarding new developers building on Substrate, and some of the ways we can improve the experience by reducing the "magic" done by our Substrate macros.

I think it obvious that the Substrate macros are both a blessing and a curse. It helps users write clean and concise code, and reduces the amount of boilerplate code needed to get things working. However, it is also the place where users get the most complicated errors to solve, where it is hardest to gain visibility about what is happening in the background, and where it is hard to get help to resolve issues without talking to our team directly (versus talking to the Rust community or Rust references for example).

I suggest that we update some of the "features" in our various Substrate macros to increase consistancy and clarity on certain aspects, while having a minimal impact on the amount of code written.

Here are a list of "features" which I think should be removed, plus a small argument as to why I think this:

• decl_module: Omitting origin as the first parameter makes a function require root origin.

  • This feature allows a user to omit origin as the first parameter of a dispatchable function. When this happens, the macro automatically adds origin back as the first parameter, and also adds ensure_root(origin)? as the first line of the function. To begin, this decreases clarity of the "rules" when writing dispatchable functions. It is very easy to say "all dispatchable functions require origin as the first parameter to determine where the call is coming from, and the first check made in that function is to verify the origin is as expected." However, with this "feature", users who look at the Substrate code base will notice that we do not include origin with all of our functions, and it is not clear at all in the program that the function is "privileged" in some way. Furthermore, in terms of auditing high privileged calls, our current syntax makes it impossible to do things like search for ensure_root to get a list of all the function which require ROOT origin. By removing this feature, we introduce at most ~25 characters to each privileged function, but increase the clarity of these functions and our rules immensely.

• decl_module: Omitting when functions return Result and automatically adding Ok(()) at the end.

  • This feature allows a user to specify a dispatchable function to not return a Result and to not have Ok(()) returned at the end. When the macro detects this, both of these items are added back to the function automatically. Again, this causes confusion to new users who are told that all dispatchable functions should return a Result, and then they look at our code and see that this is not the case. It will increase the number of characters by ~15 in functions which use the feature, but again drive clarity and simplicity where the alternative really is not needed.

• construct_runtime: the use of "no types" or default when defining your modules.

  • The construct runtime macro allows a user to omit the specific types exposed by the module if they match the "default" set, which is: Module, Call, Storage, Event, Config. The construct_runtime macro is only ever defined one time for a blockchain, so trying to optimize the number of characters written here seems silly. Users are confused when they look at a runtime definition, and see that system uses default + another type, that balances uses nothing, and that their custom runtime uses Module, Call, Storage, Event. Being explicit about the types that each runtime exposes can be good to inform users building a runtime and picking from various modules created by third parties, and to users who want to better understand what is going on. Having greater visibility into the types here I think can only be a positive for new users trying to understand how things work. Again, since this is only ever defined once, I do not see the reason to choose optimization over clarity here.

• decl_module: containing functions which are not dispatchable/callable

  • Currently the decl_module macro has a set of "reserved function names" like on_initialize, on_finalize, deposit_event, and soon offchain_worker. These functions live next to all the dispatchable functions in your module, yet behave completely differently from them. It seem that the decl_module macro should be split into two parts, one which handles only your dispatchable functions, and the other which handles these special reserved functions. This is not actually something which drives a lot of confusion to users (at least not that I have seen), but I think something worth doing for additional clarity, and reducing the amount of work a single macro does.

I would love feedback to understand if these opinions do not resonate with others.

[gui1117]

For 1, 2 and 3 I generally agree that it adds difficulty for using and understanding the macro and all in all doesn't help that much more than removing a few lines maybe.
Also as a note I think it is possible to make all those stuff as deprecated.

For the point 4 there is different solutions:

• No help by the macro: the user has to implement the trait OnInitialize himself for instance and then give it to construct_runtime, (this is what we do for ValidateUnsigned for the moment).
• Make a new macro decl_traits_impl or something that does the same as decl_module for those reserved functions. (but then all modules needs to call this macro, so instead of decl_module now all modules will have to declare decl_dispatch and decl_traits).

[bkchr]

construct_runtime: the use of "no types" or default when defining your modules.

In short term, I want to rework this. In my "vision" the user has just to add System: system and that's it. The system automatically finds which features a module exposes.

decl_module: containing functions which are not dispatchable/callable

As long as Rust does not support specialization, we can not drop OnInitialize etc.

[shawntabrizi]

@bkchr does that mean that tricks like removing the "Call" type from a module, as I did with the sudo-contract project would not be possible?

[bkchr]

@bkchr does that mean that tricks like removing the "Call" type from a module, as I did with the sudo-contract project would not be possible?

We could probably support opt-out of features.

[gavofyork]

I'm ok with 1 and 3. assuming 4 will just force users to add an extra impl OnInitialise for Module { ... } then i don't like it - too verbose. 2 is a shorthand that i am quite attached to. maybe we could have it as an explicit module/macro-level opt-in, eg

decl_module! {
	#[elide_returns]
	pub struct Module<T: Trait> for enum Call where origin: T::Origin {
		...
	}
}

[shawntabrizi]

We should address bullet 3 before v2.0

[bkchr]

Not sure that this should be a blocker for 2.0. Who will work on this?

[shawntabrizi]

To remove feature 3 should be trivial?

The other 2 Gav says he would like to keep.

[bkchr]

Ahh I didn't read 3 properly 😅 I thought it is already about what I have in my mind, because that would take more time.

[burdges]

A compromise on 2 might be const<E> OK: Result<(),E> = Ok(()); except rust lacks polymorphic constants, but maybe you could define an OK constant for each module?
Parentheses are far more annoying than two capital letters.