feat: new command pop add pallet

[tsenovilla]

The PR

• feat: new pop add-to command + pop new pallet add pallet to runtime #492

became a monster without realizing it, as I was working on it in my spare time for a very long period of time. This PR aims to bring a first piece of its functionality (the most important one IMO) to Pop-Cli, the new command pop add with its first subcommand pallet. While it's still a big PR, the review should be way easier.

Further PRs will come in the future in order to complete the whole functionality of the previous PR if they're needed.

Disclaimer: I tried to take into account all the great feedback you gave me in the previous PR related to this command, but some parts have been reorganized and maybe I missed something, so I apologize in advance.

So..

What's new?!

This PR introduces the command pop add pallet, which serves to expand an existing runtime by adding a new pallet and setting everything up for that pallet. The help message for this command looks like this:

Add a new pallet to an existing runtime

Usage: pop add pallet [OPTIONS] --pallets <PALLETS>...

Options:
  -p, --pallets <PALLETS>...
          The pallets added to the runtime. The input should follow the format <pallet>=<version>, where <pallet> is one of the options
          described below.

          Possible values:
          - balances:  Add pallet-balances to your runtime
          - contracts: Add pallet-contracts to your runtime

  -r, --runtime-path <RUNTIME_PATH>
          pop add pallet should be called from a runtime crate or from a workspace containing a runtime crate. If this command is called
          from somewhere else, this argument allows to specify the path to the runtime crate.

      --pallet-impl-path <PALLET_IMPL_PATH>
          pop add pallet will place the impl blocks for your pallets' Config traits inside a dedicated file under the configs directory.
          Use this argument to place them somewhere else.

As you may see, there's only two available pallets: balances and contracts. The PR objective isn't to bring many pallets in the first place but to create a good scaffolding allowing Pop-Cli to unlock this functionality, so more pallets may be added in subsequent PRs easily (the file crates/pop-cli/src/commands/add/pallet/common_pallets.rs contains the supported pallets, modifying it is straightforward).

Additionally, the command needs versions to be passed as arguments for each pallet. As discussed with @AlexD10S in the context of the previous PR, we've to find a better way to deal with versions as this command may be used in different templates and its a bit difficult to find out which versions need each template. But for this first version, specifying the version in the command should be good enough. I opted out for discarding an interactive UI for this first version of the command, as it feels a bit tedious to select the pallets and then being prompted to specify a version for each one.

The command in action

Let's quickly check how the command works. Imagine that we have a parachain template generated with pop new parachain and that we wanna expand it with the contracts pallet. If we run pop add pallet -p contracts=39.0.0, our parachain undergoes these changes:

	Before	After
Cargo.toml -> Pallet added to workspace manifest
runtime/Cargo.toml -> Pallet added to runtime manifest
runtime/Cargo.toml -> std feature updated
runtime/Cargo.toml -> runtime-benchmarks & try-runtime features updated
runtime/src/lib.rs -> Pallet added to runtime declaration
runtime/src/configs/mod.rs -> added new module for contracts config
runtime/src/configs/contracts.rs	Didn't exist before running pop add pallet

This is just one example but there's much more:

• Support for both #[runtime] and construct_runtime! macros.
• Support for a runtime using configs.rs as the root for the configs module instead of configs/mod.rs.
• Support for sending the pallets impl block to somewhere else (typically, this would be runtime/src/lib.rs).

Other design considerations

• The pallets impl blocks use the default implementation.
• If --pallet-impl-path isn't used, the impl block will always be created under configs/<pallet>.rs. Even if a configs module isn't used by the template yet, it'll be created.
• There's a rollback system so the template is atomically updated.
• If one of the specified pallets is already present in the runtime, the command will fail.

Future work

This is just the first version of the command, and there's so much room for improvement. Other pallets should be added in further PRs, so a Pop-Cli user would be able to add 2,3,4,... pallets to their template with a single command. As discussed with @AlexD10S offline, the optimal would be to provide support for all the existing pallets in the Polkadot SDK. This may be achieved adding all them to crates/pop-cli/src/commands/runtime_pallet/common_pallets.rs, but maybe there's a nice way to query them on the fly.

There's also the versions stuff that I talked about above, I'm sure there's a better way to deal with them.

[AlexD10S]

Thanks for addressing some of the feedback and putting in the effort to split the functionality in a smaller PR. I left some comments in the code.
Some comments from a DevEx perspective, if I run pop add pallet, I get:

error: the following required arguments were not provided:
  --pallets <PALLETS>...

It would be helpful if the UI could show users which pallets are currently supported and allow them to select one.

Also, the <pallet>=<version> format isn't very intuitive. I saw your comment in the PR description mentioning that it would be nice to allow adding 2, 3, 4, etc. pallets in one command, but since it’s unclear when that might happen, wouldn’t it be cleaner to have a flag to specify the version instead?

Finally, we should review the list of supported pallets, it might make sense to add a few more. For example, pallet_revive could be valuable, as it's something users might want to include in their runtimes soon.

[tsenovilla]

Hey @AlexD10S ! Thanks for the feedback!

Thanks for addressing some of the feedback and putting in the effort to split the functionality in a smaller PR. I left some comments in the code. Some comments from a DevEx perspective, if I run pop add pallet, I get:

error: the following required arguments were not provided:
  --pallets <PALLETS>...

It would be helpful if the UI could show users which pallets are currently supported and allow them to select one.

Also, the <pallet>=<version> format isn't very intuitive. I saw your comment in the PR description mentioning that it would be nice to allow adding 2, 3, 4, etc. pallets in one command, but since it’s unclear when that might happen, wouldn’t it be cleaner to have a flag to specify the version instead?

As I described in the PR description, I didn't include the UI cause selecting a few pallets from the UI and then specifying versions for each didn't look like the best possible UI. The same applies for the <pallet>=<version> format, I was considering using a flag, but I didn't like how it was going with several pallets, something like: --pallets contracts evm revive xcm --versions x.y.z x.y.z x.y.z x.y.z feels so wrong haha.

However, if we opt out for allowing just a pallet per execution, showing a UI with the pallets and using a version flag would be definitely better. I was thinking that, specially for new users creating a parachain via pop new parachain, adding several pallets with a single command would be interesting (in the same way pop new pallet allows you to add several types/storage items to your pallet), hence this decision design. Let me know if this use case isn't interesting and I'll update the PR following your suggestion then.

Finally, we should review the list of supported pallets, it might make sense to add a few more. For example, pallet_revive could be valuable, as it's something users might want to include in their runtimes soon.

Totally agree, but as I said in the PR description, maybe that's not in the scope of this one at all. This PR should be the scaffolding for the functionality, so you'd be able to add as many pallets as possible in subsequents PR, but given that there's a lot of code to review here, I wouldn't grow the pallets file in this PR. IMO the pallets available in this PR aren't the most important thing

[AlexD10S]

Thanks for addressing most of my comments! I've created a PR: tsenovilla#4, which adds a list of suggested pallets that might be more helpful than the current default list. Feel free to review it and add anything you think is missing.

While working on that, I’ve also done some additional testing and noticed a problem with how the functionality behaves when different versions are involved. It works well in an ideal setup, but issues arise when version mismatches occur.

How to reproduce:

1. Run pop new pallet → This generates a parachain based on version stable2412 (see: https://github.com/r0gue-io/base-parachain).
2. Then run pop add pallet → Select the latest version from crates.io. e.g., pallet-contracts: v40.1.0(https://crates.io/crates/pallet-contracts/versions).
3. Since the template isn't yet updated to the latest release (stable2503), the version mismatch leads to compilation issues when running pop build --release.

I know we've talked about the versioning issue already, and we're still not sure what the best solution is, but it definitely complicates the DevEx. If you take a look when you have some time of the scenario above, I'd really love to hear your thoughts from a DevEx perspective.

One possible idea could be to ask users to specify the version of their chain (e.g., stable2503) and then fetch compatible pallet versions directly from the Plan.toml in the corresponding SDK branch that way we can ensure it compiles after adding the pallet.

Next steps

If you rebase with the latest changes from main, I can go ahead and create another PR to implement this logic under the [experimental] feature flag (https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-cli/Cargo.toml#L66C18-L66C25). That way, we sould merge it even if it's not perfect yet, and let users start experimenting with it.

[codecov]

Codecov Report

Attention: Patch coverage is 77.97357% with 250 lines in your changes missing coverage. Please review.

Project coverage is 79.31%. Comparing base (92b9d02) to head (d0b0620).

Files with missing lines	Patch %	Lines
crates/pop-cli/src/commands/add/pallet.rs	0.00%	194 Missing ⚠️
crates/pop-cli/src/common/writer.rs	90.65%	14 Missing and 23 partials ⚠️
crates/pop-common/src/manifest.rs	91.93%	8 Missing and 7 partials ⚠️
crates/pop-cli/src/commands/mod.rs	0.00%	3 Missing ⚠️
.../pop-cli/src/commands/add/pallet/common_pallets.rs	99.71%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #524      +/-   ##
==========================================
- Coverage   79.34%   79.31%   -0.03%     
==========================================
  Files         106      109       +3     
  Lines       25828    26957    +1129     
  Branches    25828    26957    +1129     
==========================================
+ Hits        20494    21382     +888     
- Misses       3047     3258     +211     
- Partials     2287     2317      +30     

Files with missing lines	Coverage Δ
crates/pop-cli/src/common/mod.rs	100.00% <ø> (ø)
crates/pop-common/src/helpers.rs	92.70% <100.00%> (+0.23%)	⬆️
crates/pop-parachains/src/new_parachain.rs	79.04% <ø> (ø)
.../pop-cli/src/commands/add/pallet/common_pallets.rs	99.71% <99.71%> (ø)
crates/pop-cli/src/commands/mod.rs	69.26% <0.00%> (-0.92%)	⬇️
crates/pop-common/src/manifest.rs	93.05% <91.93%> (-0.50%)	⬇️
crates/pop-cli/src/common/writer.rs	90.65% <90.65%> (ø)
crates/pop-cli/src/commands/add/pallet.rs	0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tsenovilla]

@AlexD10S I've merged your PR so now the list of offered pallets is more interesting as you mentioned. I've also added the experimental feature flag for this functionality.

Regarding the DevEx, as you mentioned the user would need to know the concrete version working for their template, which could be challenging. Your proposal of using a Polkadot-SDK release instead looks nicer tbh, however pulling the versions from the Plan isn't necessary if I'm not wrong, we can just add the new pallet pointing to a branch of the polkadot SDK instead of picking a concrete version, eg:

pallet-contracts = { git = "https://github.com/paritytech/polkadot-sdk", branch = "stable2412", default-features = false }

I've updated the PR accordingly, so now the command ask for a release tag instead of a version, which is definitely a better DevEx (think we can assume that the user knows which sdk version is using, so this should be a working version IMO).

However I've noticed there's a problem after running:

pop new parachain my_parachain --release-tag stable2412
pop add pallet -p contracts --release-tag stable2412

This isn't compiling due to a dependency conflict! After looking at the involved dependencies, I realized cumulus-client-consensus is using the version 0.20.0 in the generated template, while the version used in the stable2412 release of the polkadot sdk is 0.21.0

I'll open an issue as this is something you might want to check out.