Vision: Note on the API design that involves code auto-generation

[pepyakin]

One of the best things about Rust is the balance between explicit and implicit things. See here to get the mental framework here. One example I want to highlight is importing things.

If you worked with languages like C, to print something, you would write #include <stdio.h>, and it would pull printf among other things. This can be very convenient for somebody who already knows C very well. They already know that printf comes from stdio. In other languages, such as Java or Rust, you have to manually specify everything you import with the fully qualified path, one by one. Additionally, those languages support the wildcard import, *, but it is frowned upon.

I consider that Rust made here a very good decision. It might be annoying to list every possible import, but you get a lot more in return. Let's turn the example upside down. Imagine you are reading code and seeing a previously unseen identifier. In Rust, you would need to grep within the file that identifier, and chances are you will find either an import or the declaration¹. Compare that to C: you will have to check every included header. Moreover, headers can include other headers as well. At that point, you will have to resort to a project-wide grep. C makes it easier to write code, and Rust optimizes for reading.

Now, how this translates to the API design? We have some macros that, unfortunately, do not follow the same spirit of API design. The worst offender so far is the runtime API.

Take for example the declaration of apply_extrinsic:

https://github.com/paritytech/substrate/blob/bd2a876e4c0e79b6e2ab6b0a92c038304a64b20f/primitives/block-builder/src/lib.rs#L27-L33

This will generate code that will implement this trait by calling into the Substrate Runtime at the current block (apply_extrinsic). Besides that it will generate a function (apply_extrinsic_with_context) for calling that Runtime API at a specific context, mostly used to call it at a state root other than the last best.

Besides that, I am aware that there are functions that allow calling the Runtime API of a previous version. Here is the example,

https://github.com/paritytech/substrate/blob/bd2a876e4c0e79b6e2ab6b0a92c038304a64b20f/client/block-builder/src/lib.rs#L212-L218

Now, imagine a person reading this code with fresh eyes. If they are not aware of this code generation, and they try to grep fn apply_extrinsic_before_version_6_with_context they will end up with nothing and will end up very confusing.

I propose the following guideline: avoid pulling new identifiers from thin air. Generally, try to avoid changing the identifiers or signatures provided by the user.

In particular, I think the following is the good shape of the API:

api.apply_extrinsic(xt.clone()) // Returns a struct a-la builder pattern.
    .at(block_id) 
    .context(ExecutionContext::BlockConstruction)
    .call();

It does not matter much how those are grouped, you can imagine at and context to be one call, and/or they can be the final calls (i.e. instead of the separate call call). That's not the point. The point is, that the apply_extrinsic has the same name as BlockBuilder::apply_extrinsic.

If you continue along those lines, you can arrive at an even better fitting API:

api.
    .at(block_id)
    .context(ExecutionContext::BlockConstruction)
    .apply_extrinsic(xt.clone());

This shape of the API has the same name and the same signature. Unfortunately, it may be less pragmatic to implement since at coming from multiple implementations will collide, but still may be worth pursuing.

Footnotes

1. Yes, I know there are exceptions, like inherent methods or traits. Or maybe when IDE is used. But bear with me, I still believe the point stands. ↩

[shawntabrizi]

cc @KiChjang @bkchr @sam0x17

[sam0x17]

I agree that this is a good place to use a builder pattern and I really like the syntax. One key requirement when doing something like this is to make sure it is well documented. This can be tricky with builder pattern because you absolutely need concrete examples since the normal way people poke around (seeing what methods exist on a thing) is more difficult with the builder pattern (i.e., it might be tricky to realize that you have to type.at to eventually get to something that will let you apply_extrinsic). So yeah I really like this as long as there will be some glaring documentation examples for the syntax that someone will see if they are poking around with the api object.

[pepyakin]

Just to clarify, I did not mean to fix only this particular macro. The builder pattern is also not as important, as my main point not to conjure identifiers out of thin air. I do not exclude that there could be other approaches, but that is also not the point of this post.

To address your comment, though, I agree that when you split a single API into a set of APIs, which is the case with the builder pattern, then it will be slightly trickier to understand.

I don't think it's the end of the world, though: when you write runtime API unless you are directed to do that explicitly (somebody metaphorically types with your fingers), you have very likely seen some examples of how to deal with the particular API. Therefore, you probably know that there is the context followed by the at call.

Now, how they interact and what they do would not be particularly clear IMO. I think a good and sufficient solution would be to cross-link every part of the picture in the docs to a single place, and then explain there what is happening.

One potential way to do this, is to leave some documentation on the generated trait, that explains how to use it briefly, with an example, mentioning all kinds of different caveats. One caveat that could be mentioned is in the example below:

api.apply_extrinsic(xt.clone())
    .at(block_id) 
    .context(ExecutionContext::BlockConstruction)
    .call();

No work will be done in case .call is not ultimately called.

But TBH, I don't think it's worth discussing. IMO APIs at this level of user-facing-ness should have this kind of documentation by default.

[bkchr]

I thought about this already since quite some time. Aka that this _context and at parameter were not my best idea. I also thought that we should do a builder pattern, but in reverse. So, something like:

api.runtime_api(block_id).context(context).apply_exitrinsic()

Then there is also no problem around the documentation as there would be visible from the a normal trait.

[KiChjang]

I do believe that this is the direction that we were supposed to move towards (i.e. no magical identifiers coming out of nowhere), but the one macro that gives me pause is construct_runtime. It currently spits out quite a lot of identifiers during macro expansion, such as RuntimeCall and RuntimeEvent. We can't use the builder pattern here for this macro, so I'm wondering what should be the solution for macros that generate new types such as those that I just mentioned.

[bkchr]

You would need to change the syntax to make this more obvious. However, I don't have any idea about a good syntax.

[kianenigma]

Just want to double down in this and add that coming out of the academy, it was extremely difficult to explain these magical keywords to students.

---

For construct_runtime, we can do something 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_session::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 Origin = ..AllPallets;

    // 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};

---

Having written the above:

I think the main block in turning a lot of the FRAME primitives to builders patters (which are arguably a lot nicer) is the over-use of types. Everything in FRAME is types, not a value, and types must be known at compile-time. Of course, the benefit is that we are avoiding a lot of dynamic dispatches by using types rather than values for calls and such.

The best example of this is the choice of

AllPallets = (System, Foo, Bar)

Against

let all_pallets: Vec<Box<dyn CommonPalletTrait>> = vec![System, Foo, Bar];

If something like all_pallets (and other runtime level things) was a value, as opposed to a type, we could have used a builder patter instead of a macro.

I have never had the time to explore this deeply to know how easier life would get if we could easily use the builder pattern, but I highly doubt of the micro performance gain here is actually making life any easier for anyone.

This is, in my opinion, that we had to create so many macros in the first place. Perhaps instead of bending our neck to make a macro that mimics real-rust so much, we should consider such lower-level approaches.

[bkchr]

I think the main block in turning a lot of the FRAME primitives to builders patters (which are arguably a lot nicer) is the over-use of types. Everything in FRAME is types, not a value, and types must be known at compile-time. Of course, the benefit is that we are avoiding a lot of dynamic dispatches by using types rather than values for calls and such.

How should this even look like? I mean what you are proposing there is a completely different DSL. One of the main points we are only operating on types is that the state is the storage and we don't use the memory to store any state. I also don't really get what your builder pattern should look like here? What would it do? If you really want to have some builder pattern, we could also make it work on the type level.

[ntn-x2]

Just wanted to give a huge +1 to the proposed syntax for runtime construction that @kianenigma has suggested here. It was and is a big struggle for us as well to always track down all the identifiers that the construct_runtime (and also other macros) spit out!