Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vision: Improve Developer Experience with better Config Traits #288

Open
shawntabrizi opened this issue Jan 18, 2022 · 13 comments
Open

Vision: Improve Developer Experience with better Config Traits #288

shawntabrizi opened this issue Jan 18, 2022 · 13 comments
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@shawntabrizi
Copy link
Member

shawntabrizi commented Jan 18, 2022

There are common types defined within the config of different pallets, such as:

type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;

type Call: IsType<<Self as frame_system::Config>::Call>
	+ Dispatchable<Origin = Self::Origin, PostInfo = PostDispatchInfo>
	+ GetDispatchInfo
	+ FullCodec
	+ From<frame_system::Call<Self>>;

type Origin: From<RawOrigin<Self::AccountId, I>>;

etc...

All of these types have some complicated traits which are expected from the construct_runtime! macro, and often the user struggles to define all those types when doing runtime development.

We could simply wrap all of these traits in a simple to understand trait exposed in frame_support like:

trait FrameCall: IsType<<Self as frame_system::Config>::Call>
	+ Dispatchable<Origin = Self::Origin, PostInfo = PostDispatchInfo>
	+ GetDispatchInfo
	+ FullCodec
	+ From<frame_system::Call<Self>>
{}

Then simply define:

type Call: frame_support::FrameCall

or something like that.

@dharjeezy
Copy link
Contributor

Hello @shawntabrizi is this particular issue available for pick up?
I am willing to pick it up

@bkchr
Copy link
Member

bkchr commented Jan 18, 2022

All of these types have some complicated traits which are expected from the construct_runtime! macro

That is not correct. You add the traits bounds that you are require for a certain type in your pallet. The runtime is only required to provide types that implement the given traits.

@shawntabrizi
Copy link
Member Author

@bkchr I think you are saying what I am saying?

For example, we know construct_runtime! implements a bunch of traits, like From/Into for inner and outer events, inner and outer origin, inner and outer Call type.

We also know that something like the Call type must satisfy a bunch of traits which are expected in other parts of the codebase, like Dispatchable or GetDispatchInfo. The whole thing I am saying is we can take all these assumptions about common types in the FRAME runtime and place them under a single simple trait.

Or what am I misunderstanding?

@bkchr
Copy link
Member

bkchr commented Jan 26, 2022

That is right what you are saying. I just wanted to highlight that some pallets may only require that a Call implements Encode. Then you also just need to bound for Encode.

@kianenigma
Copy link
Contributor

Related to paritytech/substrate#13454

@kianenigma kianenigma changed the title Improve Developer Experience with better Config Traits Vision: Improve Developer Experience with better Config Traits Mar 10, 2023
@sam0x17 sam0x17 added Z2-medium and removed Z1-easy labels Apr 28, 2023
@sam0x17 sam0x17 self-assigned this Apr 28, 2023
@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I4-refactor Code needs refactoring. C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T1-FRAME This PR/Issue is related to core FRAME, the framework. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed I7-refactor labels Aug 25, 2023
helin6 pushed a commit to boolnetwork/polkadot-sdk that referenced this issue Feb 5, 2024
@kianenigma
Copy link
Contributor

I would say this is done with derive_impl's delivery.

@github-project-automation github-project-automation bot moved this from To Do to Done in Runtime / FRAME Feb 27, 2024
@shawntabrizi
Copy link
Member Author

shawntabrizi commented Feb 27, 2024

@kianenigma can you link context / solution?

I saw paritytech/substrate#13454, but does not look like the same thing is being solved.

this is still a common issue I found in the academy. Would love traits for storage items, callable parameters, basic unsigned integers, etc...

@kianenigma
Copy link
Contributor

True, I misjudged this based on the title.

@kianenigma kianenigma reopened this Apr 1, 2024
@github-project-automation github-project-automation bot moved this from Done to Backlog in Runtime / FRAME Apr 1, 2024
@kianenigma
Copy link
Contributor

If and when done, the author should update https://paritytech.github.io/polkadot-sdk/master/polkadot_sdk_docs/reference_docs/frame_runtime_types/index.html as it goes into a lot of depth about how these Runtime* types and their trait bounds work.

Admittedly. if knowing the above article in depth, you would not feel confused by the trait bounds, but I get that having a simple setup is necessary, especially for something like type RuntimeEvent which is used very very frequently.

@kianenigma
Copy link
Contributor

kianenigma commented Apr 1, 2024

A solution should look like this :

// in frame/src/lib.rs

mod trait_bounds {
    pub use frame_support::traits::{Parameter, Members};
    // TODO: and fix for instancing
    pub trait TRuntimeEvent<PalletEvent, T> = From<PalletEvent<T>> + IsType<<T as frame_system::Config>::RuntimeEvent>
    pub trait TRuntimeCall<PalletCall, T> = ..;
    // This one is a bit more tricky, as `PalletOrigin` might not exist.
    pub trait TRuntimeOrigin<PalletOrigin, T> = ..;

@bkchr
Copy link
Member

bkchr commented Apr 3, 2024

Isn't this going to be solved by this: #3743?

Then we can get rid off all these extra RuntimeCall, RuntimeEvent etc in every pallet and just keep it in frame_system. We should even be able to let the macro generate this for us. Thus, users will not need to write these extra bounds at all.

@shawntabrizi
Copy link
Member Author

@bkchr agree that will def solve a lot of the stuff inside the config.

But I think there is much more overall that can be improved.

From my original issue, I want to change things like:

trait FrameCall: IsType<<Self as frame_system::Config>::Call>
	+ Dispatchable<Origin = Self::Origin, PostInfo = PostDispatchInfo>
	+ GetDispatchInfo
	+ FullCodec
	+ From<frame_system::Call<Self>>
{}

To be a single trait which already has all the requirements we need, like:

type Call: frame_support::FrameCall

Again, coming from someone who does not know so deeply about all these different traits, we can def improve on things.

Another example, in the same idea, is to create more easy to use derive macros.

For example, instead of:

#[derive(Encode, Decode, MaxEncodedLen, TypeInfo)]
#[scale_info(skip_type_params(T))]
pub struct SomeStructForStorage<T: Config> {}

We should be able to do:

#[derive(SubstrateStorage(T))]
pub struct SomeStructForStorage<T: Config> {}

I think this captures what I originally intended with the issue, and as long as we use valid rust syntax and rules (not a ton of macro magic), I think this will be a better experience for everyone.

@kianenigma
Copy link
Contributor

@shawntabrizi the storage thing you wished for will be solved by #4079

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Backlog
Development

No branches or pull requests

7 participants