Consider creating a convention to expose the most common Config types for discoverability

[KiChjang]

Take for example frame_system::Config. While constructing the runtime, I would have to implement the Config trait for the runtime, something similar to the following:

impl frame_system::Config for Runtime {
    type BaseCallFilter = frame_support::traits::AllowAll;
    // ...
}

It is not readily apparent that frame_support::traits contains a type that is assignable to BaseCallFilter. In addition, the runtime creator will have to go through the source file of frame_support::traits in order to know that there is also the DenyAll enum that could be assigned to BaseCallFilter.

It would be so great for pallets to generate an enum that contains the most commonly used configuration types, e.g. generating something akin to the following for BaseCallFilter:

enum BaseCallFilterOptions {
    AllowAll,
    DenyAll,
}

This would greatly help in discoverability, since this works very well with IDE code completions. Moreover, should we decide to take this approach, we could establish a convention where [ConfigTypeName]Options is always an enum containing the most commonly used configuration types, thereby facilitating ease of configuration.

Further work can also be done automate the creation of such an enum while creating the pallet. What I imagine pallet authors would be able to do in the future would be something like the following:

#[pallet::config]
pub trait frame_system::Config {
    #[pallet::common_config_options(AllowAll, DenyAll)]
    type BaseCallFilter: Filter<Self::Call>;
    // ...
}

This would then automatically generate a BaseCallFilterOptions enum for the pallet, and also implement the Filter trait on BaseCallFilterOptions, which essentially forwards the appropriate method calls to its variants. The pallet author would then be responsible for implementing Filter on the AllowAll and DenyAll types.

cc @shawntabrizi @thiolliere

[KiChjang]

An alternative implementation would be to create a base_call_filter_options module instead of an enum, and then declare the AllowAll/DenyAll types inside of it. Pallet authors would then implement the Filter trait on base_call_filter_options::{AllowAll, DenyAll} types.

[gui1117]

It is not readily apparent that frame_support::traits contains a type that is assignable to BaseCallFilter. In addition, the runtime creator will have to go through the source file of frame_support::traits in order to know that there is also the DenyAll enum that could be assigned to BaseCallFilter.

I don't think user have to go through the source file of frame_support::traits to see that DenyAll and AllowAll exist.
frame_system::Config::BaseCallFilter requires a type which implements Filter<Call>.
In the rust doc it is possible to see the implementation of the trait inside the dependencies and the crate itself.
Here https://crates.parity.io/frame_support/traits/trait.Filter.html#implementors
This way user should be able to discover already available implementation of the trait. And otherwise they must provide their own implementation for more configurability.

I feel like discovering implementation of traits is not so difficult using the rust doc implementors section.

[KiChjang]

Hmm... I think you and I have a different mental model of how an average FRAME developer looks like in our minds. I definitely do agree that it's discoverable, but only in the case where the FRAME dev knows to look for the rustdocs.

In contrast, I am imagining FRAME devs coming from backgrounds that are not necessarily Rust-based (or even non-software development altogether), and the only thing that can characterize them is their passion for building things on blockchain. With this in mind, I would say that they would not necessarily know where to look for information/documentation. I'd even go further to suggest that they may also not know where to ask for help, and you can see how little road bumps like these pile up and eventually cause them to abandon developing in FRAME altogether (I've seen a recent case where this happened in the subport repo, so you can imagine the number of unreported cases similar to this).

Perhaps the solution to the issue that I'm describing is simply having easily accessible docs and presenting information more openly and clearer, and I would agree with that, but I also believe that there is a benefit in designing a system where common conventions exist, which would facilitate the development experience of FRAME devs.

[gui1117]

I feel like providing enum or set of types to be used to implement the Config trait is not scalable. Trait in rust is what allow us to be configurable, sometime the trait will be implemented manually, sometime it is implemented by some other pallet, like Currency trait, which is implemented by pallet_balances::Pallet. (in this case pallet like pallet-treasury can't link to pallet-balances, because it is not one of its dependency).
I can't see how we can constraint with in scope types, for it to be more usable in general.

Though I agree we can improve the doc, and, when it is possible, to directly link to usual implementation. Here BaseCallFilter could link to DenyAll and AllowAll in the doc.

For this particular example about BaseCallFilter, I think in general user want to implement it manually, so anyway they need some basic rust knowledge

[KiChjang]

I can't see how we can constraint with in scope types it to be more usable in general.

Ah, perhaps you misunderstood me. That is not what I'm suggesting to do. I'm suggesting that we keep the traits for the Config types, but additionally create an enum that implements the trait on said enum for the common Config types. Its implementation would simply be to forward function calls to its variants:

enum BaseCallFilterOptions {
    AllowAll(frame_support::traits::AllowAll),
    DenyAll(frame_support::traits::DenyAll),
}

impl<T> Filter<T> for BaseCallFilterOptions {
    // this function will have to change to include a self parameter
    fn filter(&self, arg: &T) -> bool {
        match self {
            Self::AllowAll(a) => a.filter(arg),
            Self::DenyAll(d) => d.filter(arg),
        }
    }
}

FRAME devs will still continue to be able to implement Filter on the types that they want, in order to support extensibility.

In summary, I want to optimize for the common use case and generate BaseCallFilterOptions for the most common configuration options for the specific associated type in Config while leaving the config traits intact.

[gui1117]

yes ok maybe we can improve some associated types indeed

[bkchr]

Not sure that this would not explode very fast into your face. You would need one attribute macro per trait and external traits would not be supported.
I would say that this stuff needs to be solved by better docs, as already highlighted by @thiolliere.

[KiChjang]

You would need one attribute macro per trait and external traits would not be supported.

As explained in this comment, that is not what I'm suggesting to do. FRAME devs will still be able to implement the traits on the types that they want; all I'm suggesting is to create an enum for the most common options.

Perhaps if I named the attribute #[pallet::common_config_options] then it would be more clearer on what the intent is?

[bkchr]

Yeah I understood that, but you want to use a macro to generate this enum or not?

[bkchr]

And to implement the Filter trait for example, the macro will need to be aware of the structure of Filter.

[KiChjang]

Okay, I think I see what you mean now. Firstly, the #[pallet::common_config_options] attribute macro is not required; if the pallet author decides against using it, then everything will work as we have today. Is the complaint here that it's still too verbose to add an additional attribute on top of the Config associated types?

On the part where "external traits would not be supported", I suspect that means the [ConfigTypeName]Options enum would not be aware of any traits that were not trait bounded to the Config associated types, and hence the macro would not be able to generate implementations for it? To me that sounds fine; the [ConfigTypeName]Options enum would simply just wrap an existing type that implements both the traits that were bound to the associated type, and the "external traits" that you mentioned.

But yes, due to the limitations of enums, I considered whether we should instead generate a module like base_call_filter_common_options, and create the necessary types there. In fact, we might not need to generate additional types at all; we can just re-export AllowAll and DenyAll in the generated module for easier discoverability.

You've definitely raised a good point about knowing the structure of the bounded trait on the Config associated types. I would say that we can restrict it to traits that are structurally compatible. But perhaps I should just stick with the idea that I just mentioned -- generate a module and re-export the most common Config types.

[bkchr]

Okay, I think I see what you mean now. Firstly, the #[pallet::common_config_options] attribute macro is not required; if the pallet author decides against using it, then everything will work as we have today. Is the complaint here that it's still too verbose to add an additional attribute on top of the Config associated types?

No that would be fine. However, it would only work for the common FRAME traits.

On the part where "external traits would not be supported", I suspect that means the [ConfigTypeName]Options enum would not be aware of any traits that were not trait bounded to the Config associated types, and hence the macro would not be able to generate implementations for it? To me that sounds fine; the [ConfigTypeName]Options enum would simply just wrap an existing type that implements both the traits that were bound to the associated type, and the "external traits" that you mentioned.

By external trait I mean, any trait that is defined by a downstream project. Maybe Filter2, but that would have a quite different interface.

You've definitely raised a good point about knowing the structure of the bounded trait on the Config associated types. I would say that we can restrict it to traits that are structurally compatible. But perhaps I should just stick with the idea that I just mentioned -- generate a module and re-export the most common Config types.

For all possible config types? Even if they are not used?

[KiChjang]

For all possible config types? Even if they are not used?

Only for those types that were mentioned in #[pallet::common_config_types]. The pallet author will have to pick and choose what they believe to be the most common Config types and add to the attribute accordingly.

[bkchr]

Okay, yeah I have seen your example above again. I'm personally still not fully sold for it :P

I still see this as a mainly a lack of documentation.

[kianenigma]

would be mostly solved by paritytech/substrate#13454

[KiChjang]

I'd close this -- paritytech/substrate#13454 looks like it's a solution to the problem we're facing.