Skip to content
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

Merged

Conversation

tsenovilla
Copy link
Contributor

@tsenovilla tsenovilla commented Jul 23, 2024

Description

This PR gives a facelift to the command

pop new pallet

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:

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

  2. 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 the Balances pallet and all the infrastructure needed to manage it properly.
  1. 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.

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

  3. The user may choose whether using a genesis state for the pallet.

  4. 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 the EnsureOrigin trait to this custom origin and some template functions that may be used to convert the OriginFor 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.

  • I created a modular folder, trying to keep as much stuff as possible out of lib.rs/tests.ts. I think it's nice to start building pallets in this way instead of having huge files. Specially for lib.rs, all the stuff which isn't under a
#[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

pub enum TemplatePalletConfigCommonTypes

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.

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 68.94737% with 177 lines in your changes missing coverage. Please review.

Project coverage is 70.25%. Comparing base (e829471) to head (c8afe97).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/pop-parachains/src/new_pallet.rs 73.57% 35 Missing and 44 partials ⚠️
crates/pop-cli/src/commands/new/pallet.rs 21.87% 75 Missing ⚠️
crates/pop-parachains/src/generator/pallet.rs 21.42% 0 Missing and 11 partials ⚠️
crates/pop-common/src/manifest.rs 93.49% 8 Missing ⚠️
...op-parachains/src/new_pallet/new_pallet_options.rs 0.00% 3 Missing ⚠️
crates/pop-cli/src/common/helpers.rs 0.00% 1 Missing ⚠️
@@            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     
Files with missing lines Coverage Δ
crates/pop-common/src/helpers.rs 90.27% <100.00%> (+8.69%) ⬆️
crates/pop-common/src/lib.rs 63.63% <ø> (ø)
crates/pop-parachains/src/errors.rs 0.00% <ø> (ø)
crates/pop-cli/src/common/helpers.rs 0.00% <0.00%> (ø)
...op-parachains/src/new_pallet/new_pallet_options.rs 0.00% <0.00%> (ø)
crates/pop-common/src/manifest.rs 93.22% <93.49%> (+2.02%) ⬆️
crates/pop-parachains/src/generator/pallet.rs 29.16% <21.42%> (-7.20%) ⬇️
crates/pop-cli/src/commands/new/pallet.rs 16.27% <21.87%> (+16.27%) ⬆️
crates/pop-parachains/src/new_pallet.rs 73.33% <73.57%> (-3.85%) ⬇️

@tsenovilla tsenovilla marked this pull request as ready for review August 15, 2024 11:21
Copy link
Collaborator

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

crates/pop-cli/src/commands/new/pallet.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/new/pallet.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/new/pallet.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/new/pallet.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/new/pallet.rs Show resolved Hide resolved
crates/pop-common/src/manifest.rs Show resolved Hide resolved
crates/pop-parachains/Cargo.toml Show resolved Hide resolved
crates/pop-parachains/src/new_pallet.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/new_pallet.rs Show resolved Hide resolved
@brunopgalvao
Copy link
Collaborator

  1. The --help prompt order should be consistent with the order of how the commands (OPTIONS, NAME COMMAND) are typed. So this:
Usage: pop new pallet [OPTIONS] [NAME] [COMMAND]

Commands:
  advanced  Using the advanced mode will unlock all the POP CLI potential. You'll be able to fully customize your pallet template!. Don't use this mode unless you exactly know what you want for your pallet
  help      Print this message or the help of the given subcommand(s)

Arguments:
  [NAME]  Name of the pallet [default: pallet-template]

Options:
  -a, --authors <AUTHORS>          Name of authors [default: Anonymous]
  -d, --description <DESCRIPTION>  Pallet description [default: "Frame Pallet"]
  -p, --path <PATH>                Path to the pallet, [default: current directory]
  -h, --help                       Print help

should be revised to this:

Usage: pop new pallet [OPTIONS] [NAME] [COMMAND]

Options:
-a, --authors <AUTHORS>          Name of authors [default: Anonymous]
-d, --description <DESCRIPTION>  Pallet description [default: "Frame Pallet"]
-p, --path <PATH>                Path to the pallet, [default: current directory]
-h, --help                       Print help

Arguments:
[NAME]  Name of the pallet [default: pallet-template]

Commands:
advanced  Using the advanced mode will unlock all the POP CLI potential. You'll be able to fully customize your pallet template!. Don't use this mode unless you exactly know what you want for your pallet
help      Print this message or the help of the given subcommand(s)

a) From an end-user perspective (one who has no knowledge of clap or CLI's in general), it is confusing the difference between [OPTIONS], [NAME], and [COMMAND]. From a UX standpoint, we need to simplify this. The name of the pallet should be an option. The name should be part of the interactive prompt so that I can call advanced mode like so: pop new pallet advanced and the first prompt will be the name.

b) When running advanced mode for the first time, it was not clear to me that the syntax should be pop new pallet advanced instead I was typing pop new pallet --advanced. I understand that advanced is a command and not an option but I could see others (who are not familiar with clap or CLI syntax) making the same mistake. I would solve this by adding an "example" section similar to what polkadot-sdk does for its subcommands.

So the new revised prompt would look:

Usage: pop new pallet [OPTIONS]
       pop new pallet [OPTIONS] [COMMAND]

Commands:
  advanced         Advanced mode enables detailed customization of pallet development.
  help                  Print this message or the help of the given subcommand(s)

Options:
  -n, --name <NAME>  Pallet name [default: "pallet-template"]
  -a, --authors <AUTHORS>          Name of authors [default: Anonymous]
  -d, --description <DESCRIPTION>  Pallet description [default: "Frame Pallet"]
  -p, --path <PATH>                Path to the pallet, [default: current directory]
  -h, --help                       Print help
  
Examples:
  pop new pallet
    Interactive mode via prompts.
  pop new pallet -n my-pallet
    Non-interactive mode. Specify options manually.
  pop new pallet advanced 
    Interactive advanced mode via prompts.
  pop new pallet advanced  -c runtime-origin,currency
    Non-interactive advance mode. Specify options manually. `pop new pallet advanced --help` for a list of options available.
  1. When I went through the pop new pallet advanced interactive mode, it did not prompt me for a default config nor a genesis config, was this intentional? Because I solely went through the interactive mode, I did not even know that this was an option that was available in the non-interactive mode. If I had known that then I may have chosen to go through the non-interactive mode.

@AlexD10S
Copy link
Collaborator

  1. The --help prompt order should be consistent with the order of how the commands (OPTIONS, NAME COMMAND) are typed. So this:
Usage: pop new pallet [OPTIONS] [NAME] [COMMAND]

Commands:
  advanced  Using the advanced mode will unlock all the POP CLI potential. You'll be able to fully customize your pallet template!. Don't use this mode unless you exactly know what you want for your pallet
  help      Print this message or the help of the given subcommand(s)

Arguments:
  [NAME]  Name of the pallet [default: pallet-template]

Options:
  -a, --authors <AUTHORS>          Name of authors [default: Anonymous]
  -d, --description <DESCRIPTION>  Pallet description [default: "Frame Pallet"]
  -p, --path <PATH>                Path to the pallet, [default: current directory]
  -h, --help                       Print help

should be revised to this:

Usage: pop new pallet [OPTIONS] [NAME] [COMMAND]

Options:
-a, --authors <AUTHORS>          Name of authors [default: Anonymous]
-d, --description <DESCRIPTION>  Pallet description [default: "Frame Pallet"]
-p, --path <PATH>                Path to the pallet, [default: current directory]
-h, --help                       Print help

Arguments:
[NAME]  Name of the pallet [default: pallet-template]

Commands:
advanced  Using the advanced mode will unlock all the POP CLI potential. You'll be able to fully customize your pallet template!. Don't use this mode unless you exactly know what you want for your pallet
help      Print this message or the help of the given subcommand(s)

a) From an end-user perspective (one who has no knowledge of clap or CLI's in general), it is confusing the difference between [OPTIONS], [NAME], and [COMMAND]. From a UX standpoint, we need to simplify this. The name of the pallet should be an option. The name should be part of the interactive prompt so that I can call advanced mode like so: pop new pallet advanced and the first prompt will be the name.

b) When running advanced mode for the first time, it was not clear to me that the syntax should be pop new pallet advanced instead I was typing pop new pallet --advanced. I understand that advanced is a command and not an option but I could see others (who are not familiar with clap or CLI syntax) making the same mistake. I would solve this by adding an "example" section similar to what polkadot-sdk does for its subcommands.

So the new revised prompt would look:

Usage: pop new pallet [OPTIONS]
       pop new pallet [OPTIONS] [COMMAND]

Commands:
  advanced         Advanced mode enables detailed customization of pallet development.
  help                  Print this message or the help of the given subcommand(s)

Options:
  -n, --name <NAME>  Pallet name [default: "pallet-template"]
  -a, --authors <AUTHORS>          Name of authors [default: Anonymous]
  -d, --description <DESCRIPTION>  Pallet description [default: "Frame Pallet"]
  -p, --path <PATH>                Path to the pallet, [default: current directory]
  -h, --help                       Print help
  
Examples:
  pop new pallet
    Interactive mode via prompts.
  pop new pallet -n my-pallet
    Non-interactive mode. Specify options manually.
  pop new pallet advanced 
    Interactive advanced mode via prompts.
  pop new pallet advanced  -c runtime-origin,currency
    Non-interactive advance mode. Specify options manually. `pop new pallet advanced --help` for a list of options available.
  1. When I went through the pop new pallet advanced interactive mode, it did not prompt me for a default config nor a genesis config, was this intentional? Because I solely went through the interactive mode, I did not even know that this was an option that was available in the non-interactive mode. If I had known that then I may have chosen to go through the non-interactive mode.

Regarding your first point, I'm not sure if this change is possible or we are depending in clap, we'll need to do some research. However, I believe it should be a separate PR. Just keep in mind that we have this pattern in the other commands as well:

Usage: pop new contract [OPTIONS] [NAME]

Arguments:
  [NAME]  The name of the contract

Options:
  -c, --contract-type <CONTRACT_TYPE>  The type of contract [default: examples] [possible values: examples, erc, psp]
  -t, --template <TEMPLATE>            The template to use [possible values: standard, erc20, erc721, erc1155, PSP22, PSP34, dns, cross-contract-calls, multisig]
  -h, --help                           Print help

a) Why should name be an option rather than an argument? From a UX perspective, it seems more intuitive to use pop new pallet pallet_name rather than pop new pallet --name pallet_name.
That said, if you’re suggesting changing name to be an argument instead of an option, it should be handled in a separate PR. In this PR, we're maintaining consistency with the other commands.
About prompt the name I agree, I mentioned it here: #261 (comment) but I disagree with about the first prompt will be the name. We should keep it consistent with the rest of the commands where name is the last prompt .
b) I'm not convinced of what you say that pop new pallet --advanced is more intuitive than pop new pallet advanced. But adding an example of its usage in the --help is a good idea.

@brunopgalvao
Copy link
Collaborator

a) Why should name be an option rather than an argument? From a UX perspective, it seems more intuitive to use pop new pallet pallet_name rather than pop new pallet --name pallet_name.

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, bool and non-bool, conditionally in the prompt.

┌   Pop CLI : Generate a pallet
│
⚙  Additional configuration options
│  
◆  Are you interested in adding one of these types and their usual configuration to your pallet? 
Pick an option by pressing the spacebar. Press enter when you're done!
│  ◻ DefaultConfig (Use a default configuration for your config trait.)
│  ◻ GenesisConfig (Add a genesis config to your pallet.) 
│  ◻ Custom Origin (Add a custom origin to your pallet.) 
└  

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 DefaultConfig. If the user is not interested in any of the options they can simply press ENTER.

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 pop new pallet my-pallet advanced --help prompt would be to add an "example" section:

Examples:
  pop new pallet my-pallet advanced
    Interactive advanced mode. 
  pop new pallet my-pallet advanced --config-common-types runtime-origin,currency --storage storage-value,storage-map
    Non-interactive advanced mode. Specify options manually.

@tsenovilla
Copy link
Contributor Author

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

Copy link
Collaborator

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

crates/pop-cli/src/commands/new/pallet.rs Outdated Show resolved Hide resolved
@AlexD10S AlexD10S changed the title Improve pallet template generation feat: improve pallet template generation Aug 29, 2024
@AlexD10S AlexD10S self-requested a review August 29, 2024 12:52
@AlexD10S AlexD10S merged commit 43b592b into r0gue-io:main Aug 30, 2024
16 of 18 checks passed
@tsenovilla tsenovilla deleted the tsenovilla/improve-pallet-template-generation branch August 31, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants