substrate icon indicating copy to clipboard operation
substrate copied to clipboard

Improve Developer Experience + Understanding Code of Pallet and Runtime Events

Open shawntabrizi opened this issue 4 years ago • 5 comments

#[pallet::config]
pub trait Config: frame_system::Config {
   /// Because this pallet emits events, it depends on the runtime's definition of an event.
   type Event: From<Event<Self>> + IsType<<Self as frame_system::Config>::Event>;
   ...
}

We know there is an "outer Event" being generated in the runtime, and there is also a local "pallet Event" being defined in each pallet. There is also a "system Event"...

The fact that all of these named Event is not very great, and can make it hard to understand what the line does.

I propose we can be more specific and rename things to:

#[pallet::config]
pub trait Config: frame_system::Config {
   /// Because this pallet emits events, it depends on the runtime's definition of an event.
   type RuntimeEvent: From<PalletEvent<Self>> + IsType<<Self as frame_system::Config>::SystemEvent>;
   ...
}

Where we use RuntimeEvent, PalletEvent, and SystemEvent to make it more clear what is going on.

Not really fixed on these specific names, but I think that this direction is better, and the extra characters here are worth it.

shawntabrizi avatar Jan 18 '22 02:01 shawntabrizi

Picking this up. @shawntabrizi

dharjeezy avatar Jan 18 '22 04:01 dharjeezy

@dharjeezy I havent had a chance to talk to the others in Substrate development about this change, so let's not jump on it so quickly.

If you need some good projects, I recommend looking at some of the open issues in https://github.com/paritytech/bench-bot/issues or even https://github.com/paritytech/substrate-tip-bot

shawntabrizi avatar Jan 18 '22 05:01 shawntabrizi

Alright @shawntabrizi Thanks for letting me understand, I am looking at contributing to rust projects basically. I will appreciate if you let me know when this issue is available for pick up after confirming from the substrate dev team. Thanks once again.

dharjeezy avatar Jan 18 '22 09:01 dharjeezy

So, in line with the spirit discussed in #11736, I think instead of calling it OuterEvent, we should instead call it RuntimeEvent.

SystemEvent and PalletEvent looks fine to me.

KiChjang avatar Jul 30 '22 19:07 KiChjang

@KiChjang 100%, updated everything to represent this

shawntabrizi avatar Jul 30 '22 21:07 shawntabrizi

Sorry for silly question but in frame_system pallet

   2         /// The aggregated event type of the runtime.                                                                                 
   1         type RuntimeEvent: Parameter                                                                                                  
299              + Member                                   
   1             + From<Event<Self>>                                                                                                       
   2             + Debug                                                                                                                   
   3             + IsType<<Self as frame_system::Config>::RuntimeEvent>; 

IsType<> Bound is redundant ?

vivekvpandya avatar Mar 05 '23 06:03 vivekvpandya

Sorry for silly question but in frame_system pallet

   2         /// The aggregated event type of the runtime.                                                                                 
   1         type RuntimeEvent: Parameter                                                                                                  
299              + Member                                   
   1             + From<Event<Self>>                                                                                                       
   2             + Debug                                                                                                                   
   3             + IsType<<Self as frame_system::Config>::RuntimeEvent>; 

IsType<> Bound is redundant ?

As far as I can tell it makes sure that all the RuntimeEvent(s) always inherit from the ‘frame_system’ RuntimeEvent. Which allows access to whatever methods that has.

ruseinov avatar Mar 05 '23 09:03 ruseinov

That I understand but it seems redundant for system pallet itself. It makes sense for all other pallets

vivekvpandya avatar Mar 05 '23 10:03 vivekvpandya

You can always just delete it and see if everything still works.

xlc avatar Mar 05 '23 22:03 xlc

You can always just delete it and see if everything still works.

I have tried it but it gives compilation error from parse of macro expansion. It is because the type name is Runtime event, perhaps it will work if I rename it to Systemevent. I will try PR soon.

vivekvpandya avatar Mar 06 '23 02:03 vivekvpandya

Created WIP PR https://github.com/paritytech/substrate/pull/13541 To demonstrate things works fine I have renamed all references of system::Config::RuntimeEvent to SystemEvent in pallet assets. Similar work is required for all other pallet. If we really want this change in substrate and other companion project I can take my PR forward. :pray: @shawntabrizi

vivekvpandya avatar Mar 06 '23 06:03 vivekvpandya

type RuntimeEvent: From<PalletEvent<Self>> + IsType<<Self as frame_system::Config>::SystemEvent>;

This must have been a mistake because frame_system::Config::Event (RuntimeEvent) is also the event type as declared in the runtime.

The issue here was also already done by renaming Event to RuntimeEvent. Sorry for the "wasted" work @vivekvpandya

bkchr avatar Mar 06 '23 09:03 bkchr