Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[FRAME Core] Adds ability to split a pallet across multiple files #13950

Merged
merged 68 commits into from
Jun 27, 2023

Conversation

gupnik
Copy link
Contributor

@gupnik gupnik commented Apr 19, 2023

This PR adds the ability to split components of a pallet.

A section can be exported as follows:

#[pallet_section]
mod storages {
	#[pallet::storage]
	pub type MyStorageMap<T: Config> = StorageMap<_, _, u32, u64>;
}

and then, imported in the main pallet section as follows:

#[import_section(storages)]
#[frame_support::pallet(dev_mode)]
pub mod pallet {
     //....
}

This PR also adds pallet-example-split under frame/examples to demonstrate this functionality.

@gupnik gupnik added A3-in_progress Pull request is in progress. No review needed at this stage. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes labels Apr 19, 2023
@xlc
Copy link
Contributor

xlc commented Apr 19, 2023

do we really need this?

@sam0x17
Copy link
Contributor

sam0x17 commented Apr 19, 2023 via email

@xlc
Copy link
Contributor

xlc commented Apr 19, 2023

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.

@gupnik
Copy link
Contributor Author

gupnik commented Apr 19, 2023

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.

As you would imagine, the current mechanism stays and you can continue to use that if you don't find this useful.

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?

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).

Copy link
Contributor

@sam0x17 sam0x17 left a 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:

  1. an application of this concept to a large pallet that needs to be split up badly, so we can provide a more motivating example
  2. 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)]
Copy link
Contributor

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

Copy link
Member

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.

frame/support/procedural/src/lib.rs Show resolved Hide resolved
@sam0x17
Copy link
Contributor

sam0x17 commented Apr 19, 2023

Have you asked senior substrate developers?

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?"

@xlc
Copy link
Contributor

xlc commented Apr 19, 2023

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.

@xlc
Copy link
Contributor

xlc commented Apr 19, 2023

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

@kianenigma
Copy link
Contributor

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?

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 pallet macro that forced everything into one file.

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:

  1. Breaking big files into smaller ones. Developers generally like to do this for various reasons and I think that's a totally fair ask. My only worry is that if this breaks RA support even further, I would personally be hesitant to use it.

If you want to try one, pallet-staking is already trying to break itself into smaller parts and could use a new approach.

Also, you can just have a new pallet-example-splitted that has each pallet part in a new file, and an aggregating lib.rs that is only bringing them under one umbrella.

  1. As noted, I think the real killer feature is not splitting, but reusing. Right now, we have a paradigm where if a pallet wants to use a set of features from another code component that needs its own storage/events/calls, it has to be two pallets.

I think this is perfectly fine for most use cases. Moreover, it forces us to link the two pallets together via a trait, which makes you think about the interface between the two components.

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 RuntimeCall enum.

One example that you can look into is pallet-bags-list and how it is being used in pallet-staking. It is basically a pallet that only provides the functionality of a linked-list to some other pallet. I can't say I was particularly annoyed by implementing and using it as a whole new pallet, but I am certainly curious to see how it could have been done otherwise.

  1. Perhaps a subset of the latter, I another feature that I would personally like to see is an easier means of "privatizing" a group of storage items, and putting them behind a firewall that makes sure they are only mutated in a sensible way. A few examples of this that come to mind:

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.

@xlc
Copy link
Contributor

xlc commented Apr 20, 2023

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.

@aaronbassett
Copy link
Contributor

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?

If you do not take students' opinions into account then where do you think the next generation of senior substrate developers will come from?

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

Comment on lines 3 to 4
#[export_section]
mod calls {
Copy link
Member

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.

Copy link
Contributor

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.
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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)]
Copy link
Member

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.

Comment on lines 18 to 21
mod storages;
mod events;
mod errors;
mod calls;
Copy link
Member

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)

@sam0x17
Copy link
Contributor

sam0x17 commented Apr 20, 2023

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.

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 use some::path to bring in other modules containing #[pallet::whatever] stuff. Not being able to do this violates principle of least surprise for people used to how Rust modules are supposed to work, and this PR provides a completely optional path for people who want to organize their code in a way the existing FRAME syntax already strongly suggests you can.

@xlc
Copy link
Contributor

xlc commented Apr 20, 2023

we can't stop people from expecting to be able to use normal Rust mechanics like use some::path to bring in other modules containing #[pallet::whatever] stuff. Not being able to do this violates principle of least surprise for people used to how Rust modules are supposed to work

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.

@xlc
Copy link
Contributor

xlc commented Apr 20, 2023

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?

If you do not take students' opinions into account then where do you think the next generation of senior substrate developers will come from?

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?

Copy link
Contributor

@sam0x17 sam0x17 left a 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

frame/examples/split/Cargo.toml Outdated Show resolved Hide resolved
frame/support/procedural/src/lib.rs Outdated Show resolved Hide resolved
@gupnik
Copy link
Contributor Author

gupnik commented Jun 21, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Jun 21, 2023

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3042451 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-78b7fd27-cdae-4abb-99db-4db4841fd13f to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jun 21, 2023

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3042451 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3042451/artifacts/download.

@gupnik
Copy link
Contributor Author

gupnik commented Jun 21, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Jun 21, 2023

@gupnik https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3043015 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-2d6aaf97-a5e0-4c6f-8d51-510c2c2aceb4 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jun 21, 2023

@gupnik Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3043015 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3043015/artifacts/download.

@gupnik
Copy link
Contributor Author

gupnik commented Jun 21, 2023

@kianenigma @sam0x17 Could you take a look now?

Copy link
Contributor

@sam0x17 sam0x17 left a 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

frame/support/test/tests/split_ui/no_section_found.rs Outdated Show resolved Hide resolved
Co-authored-by: Sam Johnson <sam@durosoft.com>
Copy link
Contributor

@kianenigma kianenigma left a 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.

@gupnik
Copy link
Contributor Author

gupnik commented Jun 27, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit a779e4d into master Jun 27, 2023
@paritytech-processbot paritytech-processbot bot deleted the gupnik/pallet-split branch June 27, 2023 12:12
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
…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 <sam@durosoft.com>

* Addresses review comments

* Addresses review comments

* ".git/.scripts/commands/fmt/fmt.sh"

* Update frame/support/procedural/src/lib.rs

Co-authored-by: Sam Johnson <sam@durosoft.com>

* Update frame/support/procedural/src/lib.rs

Co-authored-by: Sam Johnson <sam@durosoft.com>

* Update frame/support/procedural/src/lib.rs

Co-authored-by: Sam Johnson <sam@durosoft.com>

* 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 <sam@durosoft.com>

* Addresses review comments

* Fixes test

---------

Co-authored-by: command-bot <>
Co-authored-by: command-bot <ci@gitlab.parity.io>
Co-authored-by: Sam Johnson <sam@durosoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants