paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.89k stars 690 forks source link

Provide an interface for user-defined extensions #1679

Open Moliholy opened 1 year ago

Moliholy commented 1 year ago

Is there an existing issue?

Experiencing problems? Have you tried our Stack Exchange first?

Motivation

Right now the interface in frame support to parse macros is private to the crate. See here: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/procedural/src/pallet/mod.rs#L57

If a dev would like to create pallet-level macros to extend their functionality wouldn't be able to use the same robust interface being used for the actual frame_support::pallet procedural macro, hence making the development harder and more error-prone.

Request

Please make the procedural macro's interface available for devs to build upon.

We might probably want to add some structure and guidelines as well in the future, but in the short term I think it'd suffice simply making the interface public.

Solution

I can quickly submit a PR making the interface public, if this request is positively considered.

Are you willing to help with this request?

Yes!

bkchr commented 1 year ago

Can you give an example on what you want to write as extension? IMHO we should not open up these types for external usage. We are already bad at versioning and should not add more public surface.

Moliholy commented 1 year ago

It's actually already implemented. It's a set of macros that mimic pallet-contracts' migration system with only three procedural macros. You can take a look here.

The thing is that it would have been much nicer and cleaner to parse the pallet structure the same way it's done in frame_support::pallet.

sam0x17 commented 1 year ago

I think it is an open question whether we want to pursue this sort of thing, but here are my recommendations if we do and some justifications for why I think it might actually be a good idea:

If we decide to go this direction, I would also see it as an opportunity to standardize how pallet macros are defined, internally (which would have the side effect of significantly DRY-ing up frame_support_procedural) and then we can maybe decide on a safe subset of this functionality that we can expose for people who want to write custom extensions outside of FRAME. That would be a long road but I think it would clean up a lot of things that right now are hard to maintain. So in other words, we write some extremely standardized, sane, clean interface for defining pallet macros / extensions that is flexible enough to accommodate for our existing macros but allows for much DRYer, shorter, code. Then we would migrate all of the existing pallet macros to fit into this interface, and then open up some subset of it for the purpose of custom extensions.

Whether or not we do the above, to enable the custom extensions part we could do something like #[frame::extensions(some::path::SomeIdent)] that compiles as an outer-macro pattern (via macro_magic to access the actual body of the extension) to be placed above #[frame::pallet] and provides a standardized way of introducing additional custom pallet macro syntax / extensions. That path would point to some module that defines the actual extension based on some interface / structure that we decide upon, most likely a module containing impls for Parse and Expand traits as well as generic ways of defining a parsing-related data type and populating and accessing it during the Parse and Expand phases respectively. Access to other extensions' data could also be facilitated using traits if necessary.

So something like:

#[frame::extensions(Migrateable, CustomExtension)]
#[frame::pallet]
pub mod pallet {
    ...
}

If we completely migrate our existing macros to an extension system, this could instead become an argument on #[frame::pallet] like #[frame::pallet(extensions: ..)]. Something like that.

Some things I see this enabling eventually would be:

Anyway just some thoughts cc @kianenigma @ggwpez

sam0x17 commented 1 year ago

TLDR:

bkchr commented 1 year ago

It's actually already implemented. It's a set of macros that mimic pallet-contracts' migration system with only three procedural macros. You can take a look here.

There is work being done on multi block migrations. The pallet-contracts migration system will be replaced by this.

Moliholy commented 1 year ago

There is work being done on multi block migrations.

Yes, I was aware from the beginning. I don't think what I as a dev am particularly doing has something to do with this, specially since I already finished it. The point it, as @sam0x17 mentioned, that regardless of being me in the future or some other dev, it might be interesting to open the possibility to further extend the procedural macros to either replicate the parsing or make it a first class citizen and directly support it. I think he summarized it pretty well in his last comment.

bkchr commented 1 year ago

The point being that it requires quite a lot of work to make the stuff generic and compatible. I'm not sure we need this right now? External FRAME macros should be in the best case just be normal Rust macros that work everywhere and don't require FRAME.

Moliholy commented 1 year ago

We could start by simply making the current interface public, and later on improve it. Or maybe that's bad idea. That's what the issue is for: discussing how desirable this feature could be and the amount of work it could take. If it's a lot, then maybe this could be a long-term project.

I personally don't have a strong opinion about it, and simply thought it'd be worth to make the proposal so that it gets evaluated.

sam0x17 commented 1 year ago

We could start by simply making the current interface public, and later on improve it.

There really is no current interface other than a few loose conventions spread out across a bunch of proc macros, and by virtue of how proc macros work, there really is no easy way of opening this up without building something substantial, other than outer macro pattern tricks.

That said, external contributors can easily extend frame via an additional outer macro pattern that runs before the regular frame one, and there is nothing we actually need to do to enable this -- it is already possible. If we want to, though, we could build a small interface for doing this and then standardize that, which would at least make these extensions less brittle when FRAME changes. So that's the minimal version of what I'm proposing, some #[pallet::extensions(..)] thing that loads some outer macro patterns to run before #[pallet] runs. Would prob be like a 3-4 day feature to implement and test fully, and would at least give external contributors a standardized tool for extending FRAME.

thought: it might be even more useful to have these run after parsing but before expansion so we can just give the extension read/write access to the actual pallet Def maybe 🤔