-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: improve pallet template generation #261
feat: improve pallet template generation #261
Conversation
… of cases, allowing pick several options for the same object
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #261 +/- ##
==========================================
- Coverage 70.28% 70.25% -0.04%
==========================================
Files 52 53 +1
Lines 8657 9084 +427
Branches 8657 9084 +427
==========================================
+ Hits 6085 6382 +297
- Misses 1636 1720 +84
- Partials 936 982 +46
|
… that's already in the workspace doesn't include it again
…hen a pallet is generated from a workspace
…hen a pallet is generated from a workspace
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.
Excellent work!
I've left a few comments, mostly suggesting some code refactoring to maintain consistency with the rest of the commands. I also tagged Bruno in a few spots to discuss the best approach. The functionality looks fantastic, this is a big step towards making pop-cli
the tool for pallet development.
should be revised to this:
a) From an end-user perspective (one who has no knowledge of clap or CLI's in general), it is confusing the difference between b) When running advanced mode for the first time, it was not clear to me that the syntax should be So the new revised prompt would look:
|
Regarding your first point, I'm not sure if this change is possible or we are depending in
a) Why should |
Good point on the usage of name in the wider context of other Pop CLI commands. That being said, let's keep name as it is. For the advanced mode, it is not clear which options are already included in the interactive prompt and which are not. That being said, let's include all options,
The above prompt would be created conditionally. For example, if the user did not include storage items then the above prompt should not prompt for Of course, those users who would like to specify all the options via the command line manually, that is also available. The only alteration to the
|
Thank you so much for your great feedback @brunopgalvao @AlexD10S . I've pushed a new version taking all this stuff into account :) Let me know if you come up with another needed improvement |
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.
Excellent job.
I left a small comment, but the only thing left to do is merge main
, there are some conflicts to resolve. This will also fix the test that's failing in the CI.
Description
This PR gives a facelift to the command
This command was creating a static pallet template, now it's interactive, giving the user the possibility to shape its pallet's template accordingly to its special needs. Here's how the new flow works:
The user may choose whether to include a default configuration for its pallet's config trait. If that's selected, the user will be able to decide whether the config trait's types are included in the default config. All the infrastructure needed to use this config will be created, including the
config_preludes
module where the types selected to be included in the default may be set up and the derived configuration ready to be edited in the mock runtime.Then, the user may choose among some types that may appear frequently in pallet's. The available options in this first approach are:
RuntimeEvent
: Nothing special to say here, it's almost everywhere.RuntimeOrigin
: This type is useful when using custom origins. If this is added to the pallet, the user may also add an internal custom origin later on. If the user doesn't create an internal custom origin, it can still be useful to define some external custom origins.Currency
: This type is useful if the pallet's going to interact with the native currency of the blockchain. It creates a loosely coupling with theBalances
pallet and all the infrastructure needed to manage it properly.The user may add some other custom types to its config trait. Here we only ask for their names, as adding all the trait bounds would make the CLI an awful tool. So why should the CLI have this option? Basically because the user is able to annotate which of these types are going to be included in the default config and a lot of boilerplate lines are written following these decisions, when it comes to play with this default config. If the user isn't using the default config, this step isn't that useful IMO, but it's up to the user to use it.
The user now works with the storage. This step consists simply in picking which type of storage's needed and give it a name. The structure for this type of storage will be autogenerated with the user's choices.
The user may choose whether using a genesis state for the pallet.
If
RuntimeOrigin
has been added, the user may choose to create a custom internal origin. If that's selected, the user will introduce all the variants' names for that enum. This also creates a template for implementing theEnsureOrigin
trait to this custom origin and some template functions that may be used to convert theOriginFor
origin received from the outer world into this custom origin.Some other design choices
I chose to don't include events/errors in this process. The only thing we may ask from the user would be the variants' names of those enums, the user may directly write that in the code. I think this is exactly the kind of things that should be avoided to make this tool attractive.
I didn't include any template dispatch. This is the custom logic and then including something like
fn do_something
isn't worthy IMO.
#[pallet::something]
attribute can be moved out.
I opted for using the new frame umbrella crate in all the templates.
Related to avoid linking to substrate.io #260, links to substrate.io has been replaced by links to polkadot-sdk-docs
Potential future work
As I described in Improve template pallet generation #253, the main point here is to find a balance between the time spent creating the pallet and the work we take away from developers, in order to make the use of this CLI attractive. This is just a first approach, based purely in what I found interesting to add, so probably it's far from the ideal.
Keep adding common config types and their respective stuff to the pallet generation. With the work done in this PR, it's as simple as adding a new enum variant to
and edit the templates accordingly :)
Maybe add support for adding external custom origins to the config trait (?) I'm not sure if this would be worth it at all.
Maybe add support for dispatchables (?). As this is the custom logic it's hard to define it from the CLI, but maybe adding some "structure" to it could be interesting.
Move the pallet stuff out of
pop-parachains
. I think that it's independent enough to live in its own crate, and the code would be clearer.Notes
Many of the modified files only include formatting by
cargo fmt
.