-
Notifications
You must be signed in to change notification settings - Fork 2.6k
[FRAME Core] Adds ability to split a pallet across multiple files #13950
Conversation
do we really need this? |
do we really need this?
When I did my substrate seminar last fall this was the main thing people brought up in the Q&A as a desired feature, with all sorts of reasons ranging from breaking up very large pallets into smaller more digestible files to allowing for code sharing of identical sections that could be shared between multiple pallets.
The outer macro pattern our pallets rely on is extremely powerful, but the price we pay is everything has to be in one module in one file. This removes that restriction without sacrificing any of the power or capabilities the outer macro pattern brings us in FRAME.
There is a reason Rust normally lets you break things up into multiple modules in multiple files. Now we have a way to still do that while nevertheless using the outer macro pattern.
|
You are not talking with the right set of developers for feedbacks. Have you asked senior substrate developers? like the core devs in Parity and parachains, instead of students? We have a lot of complex pallets in Substrate, staking for example is one, have you asked the core dev for pallet-staking to see if this is helpful? At least for all the pallets I maintain, I don't need this. The normal ways of abstracting the logics is enough. Maybe you are right but you need to show an example of how this is beneficial and cannot be done otherwise. Chances are there are easier way to break pallets into smaller files and code reusing without macro magic. Again, this is another thing I don't like how some approach a problem. You need to show there is an indeed a problem, propose a solution, highlight how it fixes the problem, and then code it. How is moving events to its own file enable code reuse? How are you going to reuse those events? Maybe there is a set of storages could be reused by different pallets? But then how is your solution make such use case possible? You should write some pseudocode to see if it works or not. Maybe you already have those? Please show. |
As you would imagine, the current mechanism stays and you can continue to use that if you don't find this useful.
You are absolutely right here. That's why I mentioned that this is a POC and the PR is a draft. Once the approach makes sense, I will start to build examples where this would make sense. The intent of the splitting here was to show that it is possible now to create sections that encompass a few pieces instead of a single monolithic and not that the events should be in its own file (or for any other section for that matter). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good proof of concept. I'd love to see:
- an application of this concept to a large pallet that needs to be split up badly, so we can provide a more motivating example
- a legitimate scenario where this can enable code sharing, if such a scenario actually exists (think: are there places where several pallets have the same boilerplate, and could that boilerplate become a pallet section that lives in a single location and is used in a number of pallets?)
#[import_section(events)] | ||
#[import_section(errors)] | ||
#[import_section(calls)] | ||
#[import_section(storages)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that I'm seeing several imported at once, would be cool to have an #[import_sections(section1, section2, section3)]
macro that expands to the above for scenarios like this. Doesn't have to be addressed in the initial PR but making a note of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and a comment please about what this is doing.
This was discussed a good bit in the parity forum a while ago where @bkchr, Shawn, and @aaronbassett were in favor of it. People coming out of the academy consistently ask for this as well, which I think is an even better indicator. Rather than look at this from a perspective of "why should we add this?" though, consider this: in Rust, it is normal to be able to split up a module into multiple smaller modules. This is just how Rust works, and it enables better readability and a number of positive things commonly associated with modularity that I'm sure we could all agree on such as not having to scroll through a 1000-line file. Pallets are modules, however since FRAME parses pallets using the outer macro pattern, we are stuck with this artificial, non-rusty limitation that the pallet module cannot be split into multiple external files like normal Rust modules can, and explaining this fact to newcomers is a laborious task. The burden of proof should then be on "why shouldn't we remove this non-Rusty artificial limitation if we have the means to do so?" |
Well, you are not wrong, but again, there are ways to break pallets into multiple files and that worked well so far (at least for myself). But yeah maybe those are bad workaround that deserve a better solution. A real life example will help me to change my mind. My main point is, why wasting so much time getting code actually compile when you can just write something and say image if this code compiles? This is just not the correct way to develop such solution. I wrote a lot of resuable code for pallets, most of them live in ORML. You might be surprised how much can be done without macro magic. |
Also yes I can simply not use this features if I don't want to use. I guess I am just getting a bit grumpy when I see people wasting time building something isn't important than fixing critical issues I raised years ago. I still have 46 open issues and I don't think any of them have been take care of https://github.com/paritytech/substrate/issues/created_by/xlc |
The desire to break pallets into smaller parts stems not from PBA or students, but rather from Parity engineers, and dates at least to 2021, with the introduction of the new I agree with the rest of your comments @xlc and I see that @sam0x17 and @gupnik are also onboard, we need to now find use-cases for this. I broadly think this can help us with 3 classes of issues:
If you want to try one, Also, you can just have a new
I think this is perfectly fine for most use cases. Moreover, it forces us to link the two pallets together via a The downside is having too much boilerplate, and some limitations like the fact that someday, we simply will run out of pallets to add to the outer One example that you can look into is
All in all, I think this is a very promising avenue. I hear the call for more validation, and it is fair, but I think enforcing it at an early stage of an experimental work like this will hinder innovation. Sometimes you don't know what you are missing unless if some tool is there to fix it for you. |
It sounds like we don't really want to break pallet into files. This is not the goal. For (1), I don't yet see a big pallet that is build in such way will be benefit from this. We can already move most of the logics to other files. I definitely prefer to have all the definitions in a single file. For (2) and (3), they sound like the same thing to me and split pallet across multiple files is not the right solution. Let's use common programming design patterns when approaching such a common issue. Let's use OO here. A class (pallet) is made of fields (storages) and methods (calls). The calls can return errors and emit events. What happen if we have a god class that's too big to maintain? We break it down into multiple smaller class. We don't break it down into multiple files and write fields in one file and method in another file and somehow allow the fields to be reuseable??? This simply makes zero sense. So the right solution is being able to define a component that's made of storages, calls and supporting types (events, errors and related types) and then build a pallet from those components. How about inheritance. Allow a pallet to inherent everything from another pallet. Then we can build a storage only pallet and that's exactly what you want here. It could even support multiple inheritance (and make your life harder). We can use composition. Allow a pallet to embed child pallet. The child pallet's storage will be placed under parent pallet's storege prefix. The calls events errors can be aggregated and flattened. |
If you do not take students' opinions into account then where do you think the next generation of senior substrate developers will come from? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
#[export_section] | ||
mod calls { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment that it is not possible to add that attribute on a file level like #![export_section]
.
I think that applies to all attribute macros.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this comment would be a good idea.
And yes, custom inner attributes are a nightly-only feature atm
// Return an error if the value has not been set. | ||
None => return Err(Error::<T>::NoneValue.into()), | ||
Some(old) => { | ||
// Increment the value read from storage; will error in the event of overflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can dumb these example down by a lot. Just accepting an should_error
bool is probably enough.
Keeping it condensed is IMHO important to not loose devs that want to understand it.
|
||
#[export_section] | ||
mod errors { | ||
// Errors inform users that something went wrong. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Errors inform users that something went wrong. | |
/// Errors inform users that something went wrong. |
#[import_section(events)] | ||
#[import_section(errors)] | ||
#[import_section(calls)] | ||
#[import_section(storages)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and a comment please about what this is doing.
mod storages; | ||
mod events; | ||
mod errors; | ||
mod calls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are modules, but i am not sure if anyone actually wants to use them as normal Rust modules?
Maybe we can find a more FRAME syntax which just wraps these and the import_sections
below. Although i would definitely keep this syntax working with the manual mods and manual import_sections
.
For example:
#[pallet_sections(
mod storages;
mod events;
mod errors;
mod calls;
)]
(not quite happy with the syntax, but you see what i mean)
I agree with this sentiment, but we are already stuck with how FRAME was designed around the outer macro pattern encompasing one giant pallet module. We could completely re-design FRAME to be more "proper" from an OO perspective for example, but with FRAME the way it is, where a pallet is a giant module, we can't stop people from expecting to be able to use normal Rust mechanics like |
Maybe it is just me but I definitely will be surprised if import form other modules actually work. I just don't agree with you on this. But anyway, lets skip this point. Let's get back to code reuse. You have to give me an example on how this can achieve code reuse. Also the goal is to achieve code reuse, not split files so you NEED to explore alternative solutions and syntax. |
NO. You may find Rust is hard to learn/use and have some idea on how to improve Rust syntax but I would not trust your suggestions on how to improve Rust. Your suggestion maybe great but it MUST be reviewed by multiple Rust experts first and go through a lot of discussions before any implementation. Same thing apply here. I did not see ANY open discussion about this and that makes me sad. Yeah you may have a lot of internal discussion but they are non existent to me if I can't access them. Can't you just write an issue first before coding? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good just want to see the proper path resolution + kian's stuff and we should be good
bot fmt |
@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3042451 was started for your command Comment |
@gupnik Command |
bot fmt |
@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3043015 was started for your command Comment |
@gupnik Command |
@kianenigma @sam0x17 Could you take a look now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me!
had one missing newline other than that should be good to merge
Co-authored-by: Sam Johnson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small suggestions to make the scope of the example even narrower, everything else looks good.
bot merge |
…ritytech#13950) * Initial setup * Updates macro_magic version and refactors accordingly * Removes unwrap from macro * Splits into multiple sections * Uses call_site to fix macro hygiene issue * Initial setup * Removes unnecessary changes * Moves template palet back * Updates cargo.lock * Moves BagsList inside mod * Comments access to internal functions for now * Updates tests * Uncomments code * Fixes test * Moves bags-list to separate crate * Initial setup * Removes bags-list changes * Fix structure * Minor update * Addresses review comment * Adds a couple of UI tests. More to be added * Adds err files * Adds test for no pallet * Adds doc * Updates versions * Adds benchmarking * Updates doc link * ".git/.scripts/commands/fmt/fmt.sh" * Minor update * Adds missing changes * ".git/.scripts/commands/fmt/fmt.sh" * Update frame/support/procedural/src/lib.rs Co-authored-by: Sam Johnson <[email protected]> * Addresses review comments * Addresses review comments * ".git/.scripts/commands/fmt/fmt.sh" * Update frame/support/procedural/src/lib.rs Co-authored-by: Sam Johnson <[email protected]> * Update frame/support/procedural/src/lib.rs Co-authored-by: Sam Johnson <[email protected]> * Update frame/support/procedural/src/lib.rs Co-authored-by: Sam Johnson <[email protected]> * Adds UI test for disambiguation * ".git/.scripts/commands/fmt/fmt.sh" * Makes clippy happy * ".git/.scripts/commands/fmt/fmt.sh" * Fixes frame support test * Fixes frame support test * Split items other than storage * Updates versions * Fixes some review comments * Addresses review comments * ".git/.scripts/commands/fmt/fmt.sh" * Updates docs * Adds experimental disclaimer * ".git/.scripts/commands/fmt/fmt.sh" * Update frame/support/test/tests/split_ui/no_section_found.rs Co-authored-by: Sam Johnson <[email protected]> * Addresses review comments * Fixes test --------- Co-authored-by: command-bot <> Co-authored-by: command-bot <[email protected]> Co-authored-by: Sam Johnson <[email protected]>
This PR adds the ability to split components of a pallet.
A section can be exported as follows:
and then, imported in the main pallet section as follows:
This PR also adds
pallet-example-split
under frame/examples to demonstrate this functionality.