feat: new pop add-to command + pop new pallet add pallet to runtime #492
Conversation
…te pop new pallet to include the created pallet in runtime if needed; rust_writer module added to allow these features
…'s been run in a runtime workspace
…'s been run in a runtime workspace
…ith unpreserve function affecting declarative macros
…ynamically computed pallet path
…so not integration at all
…eturns an unexpected fmt
…eturns an unexpected fmt
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new CLI commands to add features to existing Polkadot-SDK projects. Key changes include:
- Implementation of the new “pop add-to” command with subcommands for adding a pallet to a runtime and adding a config-type to a pallet.
- Refactoring of path resolution and manifest handling across pop-common and pop-cli using updated rustilities functions.
- Extensive additions and updates to tests covering the new commands and workflow.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/pop-common/src/manifest.rs | Removed obsolete workspace lookup and added new helper functions. |
| crates/pop-common/src/lib.rs & helpers.rs | Replaced deprecated path functions with rustilities equivalents. |
| crates/pop-common/src/errors.rs | Introduced new error variants to better capture various error cases. |
| crates/pop-cli/* | Added new command implementations and tests for “pop add-to” features. |
Comments suppressed due to low confidence (2)
crates/pop-cli/src/commands/new/pallet.rs:221
- [nitpick] The code assumes that the crate name always contains the prefix 'pallet-'. Consider adding a check or a more robust fallback for crate names that do not follow this convention.
let pallet_ident = Ident::new(&capitalize_str(&pallet_crate_name.split("pallet-").last().ok_or(anyhow::anyhow! {
crates/pop-cli/src/commands/add/pallet_config_type.rs:69
- Consider changing "correspond" to "corresponds" for correct subject–verb agreement.
cmd.error(ErrorKind::InvalidValue, "Make sure that the used path correspond to a pallet crate.")
… pop add-to runtime pallet
There was a problem hiding this comment.
Thanks @tsenovilla for your great work. I just gave a high level review, my suggestion is we should reduce the number of external crates added to pop-cli because it increase the cost of maintaining, especially crates are not battle-tested.
On the other hand, could you add to the README the overall flow of the functionalities? It would be very helpful for reviewers to understand how the code logic works exactly 💪
| }, | ||
| preserver::Preserver, | ||
| }; | ||
| use rustilities::manifest::{ManifestDependencyConfig, ManifestDependencyOrigin}; |
There was a problem hiding this comment.
Visited this crate and seems like you made it, awesome! But as the crate is new, I don't think using it in pop-cli makes sense which will increase maintenance cost if there is something wrong with the downstream dependencies. There is a file pop-common/manifest.rs which might include the needed logic already, have you checked it? If not, feel free to implement new methods there.
There was a problem hiding this comment.
Good point, Tin! I totally understand your concerns about maintenance on our side. However, I suggested to him that it should be in a separate crate because it was introducing a lot of code into pop-cli that wasn’t directly related to pallet development. Tomas did a great job with that library, and I believe it could be useful across different ecosystem projects and other Rust projects as well. So it makes more sense for it to live in its own crate and be made more widely available.
There was a problem hiding this comment.
Hey @chungquantin!
Those new crates (rust_writer, fs_rollback and rustilities) were born as a natural consequence of this specific PR. All that functionality (in a primitive way sometimes, as I redefined many parts of it in those crates) were originally part of this PR, being part of the manifest file you mentioned and many others. As @AlexD10S said, we agreed off-chain to move all that functionality out of this repo, cause it's "general Rust functionality", meaning that it can be used here and in other projects as well. I partially agree with you when you said that it might increase maintenance cost, but having all that code inside pop-cli is kinda unnecessary, it'd just flood the repo with code that doesn't need to be defined here, so that also increase maintenance cost.
I invite you to check this commit, which is the last one that contains all that functionality inside pop cli, so you'd get a clearer picture of how it'd look like.
Finally, IMHO, it makes more sense to keep the actual approach of having that functionality outside pop-cli, but at the end of the day this is your product, so if your team reaches consensus in the other direction we can just revert the commit I mentioned above, just let me know what you think :)
There was a problem hiding this comment.
Hey @tsenovilla, thanks for your detailed explanation! I discussed with Alex and I agree this approach makes sense to me know. Will review it more to see if there are any parts to optimize.
There was a problem hiding this comment.
As there has been a lot of dependencies added, I believe we should be more careful with new dependencies added to the codebase.
| glob = "0.3.1" | ||
| log = "0.4.20" | ||
| mockito = "1.4.0" | ||
| pathdiff = "0.2.1" |
There was a problem hiding this comment.
I don't think this library is needed. We can simply implement it in the codebase if needed. As can be seen here https://docs.rs/pathdiff/0.2.3/src/pathdiff/lib.rs.html#43-86 the implementation is not that complicated so would be better to have it in the repo.
There was a problem hiding this comment.
Let me know if the team agrees on this and I'd implement the code in your codebase then :)
Co-authored-by: Tin Chung <[email protected]>
There was a problem hiding this comment.
I did think about a repo structure for tests when implementing other commands. Should we have a tests.rs file in each commands/* and bring tests there? So tests will be located specifically to the command folder. What do you think @AlexD10S? It also makes the CI of this PR simpler.
There was a problem hiding this comment.
This folder is for the integration tests, before we only had one for parachains and one for contracts but now I see might be growing so yes, we should improve the organization. Do you mean have it under specific command folder?
There was a problem hiding this comment.
This is a nice point, but not straightforward probably. I also thought about that possibility, but there's the painpoint that assert_cmd has some limitations out of integration tests: https://docs.rs/assert_cmd/latest/assert_cmd/cargo/index.html
So that'd need some rework for all the tests, not just the ones introduced by this PR
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub enum RuntimeUsedMacro { |
There was a problem hiding this comment.
Could we add documentation to all public enums and enum fields? Apologies if the PR is still WIP, just a friendly reminder 😄.
There was a problem hiding this comment.
Sure sorry, I though it was clear enough, but this is definitely something to do
| - name: Run integration tests | ||
| run: cargo test --no-default-features --features parachain --test parachain | ||
|
|
||
| file-writer-integration-tests: |
There was a problem hiding this comment.
Curious why do we need an additional CI job here? Wouldn't it be covered by other integration tests CI?
There was a problem hiding this comment.
It's currently not covered in parachain-integration-tests because we have cargo test --no-default-features --features parachain --test parachain. But definitely easier to just add it there (https://github.com/r0gue-io/pop-cli/blob/main/.github/workflows/ci.yml#L201) than having a whole new job:
run: |
cargo test --no-default-features --features parachain --test parachain
cargo test --no-default-features --features parachain --test new_pallet_added_to_existing_runtime
cargo test --no-default-features --features parachain --test pop_add_to_pallet_config_type
cargo test --no-default-features --features parachain --test pop_add_to_runtime_pallet
| @@ -0,0 +1,322 @@ | |||
| // SPDX-License-Identifier: GPL-3.0 | |||
|
|
|||
| mod common_types; | |||
There was a problem hiding this comment.
mod should be located after use (just a convention in this repo 😅)
| let pallet_mock_path = pallet_path.join("src").join("mock.rs"); | ||
|
|
||
| let spinner = cliclack::spinner(); | ||
| spinner.start("Updating pallet's config trait..."); |
There was a problem hiding this comment.
| spinner.start("Updating pallet's config trait..."); | |
| spinner.start("Updating `trait Config` of the pallet..."); |
There was a problem hiding this comment.
Not sure if this is better.
| Some(_) => self.pallet_impl_path.clone(), | ||
| _ => Some(manifest::get_pallet_impl_path( | ||
| &runtime_path, | ||
| &pallet_crate_name.splitn(2, '-').nth(1).unwrap_or("pallet").to_owned(), |
There was a problem hiding this comment.
Could we refactor this logic to a separate method called get_pallet_crate_name? Plus a test for it would make it easier to imagine what is the expected input and output.
|
|
||
| let roll_pallet_lib_path = rollback | ||
| .get_noted_file(&pallet_lib_path) | ||
| .expect("The file has been noted above; qed;"); |
There was a problem hiding this comment.
Can we return a better error?
There was a problem hiding this comment.
This isn't an error. get_noted_file returns Option<&Path> (https://docs.rs/fs_rollback/latest/fs_rollback/struct.Rollback.html#method.get_noted_file). That expect means that at this point of the flow it's always Some cause you've used the note_file method above
| roll_pallet_lib_path | ||
| }; | ||
|
|
||
| // Define preservers for the most common used struct names for default config. |
| } | ||
|
|
||
| pub fn get_needed_composite_enums(&self) -> Vec<Item> { | ||
| match self { |
There was a problem hiding this comment.
| match self { | |
| match self { | |
| CommonTypes::RuntimeHoldReason => vec![parse_quote! { | |
| /// A reason for the pallet placing a hold on funds. | |
| #[pallet::composite_enum] | |
| pub enum HoldReason { | |
| /// Some hold reason | |
| #[codec(index = 0)] | |
| SomeHoldReason, | |
| } | |
| }], | |
| _ => vec![] |
There was a problem hiding this comment.
Nice job! There's a lot of great work here, really cool stuff to handle such a complex feature.
I’ve left a few comments in the code, but we’ll definitely need a more thorough review before merging to main. My main concern right now is around polishing the core functionality and improving the DevEx.
As mentioned in one comment, I’d recommend simplifying the CLI commands to something like:
pop add palletpop add config(although, personally, I don’t find this second one super useful)
A few functionality issues I ran into:
-
Running
pop add-to runtime palletin a freshly generated template fails if I selectBalances(because it already exists), and selectingContractsalso fails, I believe due to version conflicts.
Can we detect maybe if Balances already exist and not display it? or too complex? -
Running
pop new palletfollowed bypop add-to pallet config-type(selectingFungibles) throws this error:
Error: Cannot mutate using Mutator: ItemToImpl { trait_name: Some("Config"), implementor_name: "Test", impl_item: ImplItem::Type { attrs: [], vis: Visibility::Inherited, defaultness: None, type_token: Type, ident: Ident(Fungibles), generics: Generics { lt_token: None, params: [], gt_token: None, where_clause: None }, eq_token: Eq, ty: Type::Path { qself: None, path: Path { leading_colon: None, segments: [PathSegment { ident: Ident(Balances), arguments: PathArguments::None }] } }, semi_token: Semi } }
|
|
||
| pub fn get_version(&self) -> String { | ||
| match self { | ||
| CommonPallets::Balances => "39.0.0".to_string(), |
There was a problem hiding this comment.
We need to find a way to avoid hardcoding this version number, as it can easily lead to errors and add unnecessary maintenance overhead.
Maybe we can fetch the latest version dynamically from crates.io and also prompt the user or let him specify it in the CLI the version they want to use. This would also make it compatible with older versions, giving more flexibility.
| self.pallets | ||
| }; | ||
|
|
||
| let mut rollback = Rollback::default(); |
There was a problem hiding this comment.
Is the rollback needed here?
In another part of the code we handle something similar only with: https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-parachains/src/build/mod.rs#L160
let temp_file = tempfile::NamedTempFile::new_in(std::env::temp_dir())?;.
temp_file.persist(plain_chain_spec).map_err(|e| {
Error::AnyhowError(anyhow!(
"Failed to replace the chain spec file with the temporary file: {}",
e.to_string()
))
})?;There was a problem hiding this comment.
It's needed due to the command modifies several files: at least the runtime lib and the runtime manifest, but also the file where the new pallet is implemented if it's not at runtime lib. So this rollback takes care of everything and doesn't modify any of the files if it cannot modify all of them. IMO it's a really nice to have, so the command modifies the file system atomically
| #[clap(alias = "C")] | ||
| Clean(clean::CleanArgs), | ||
| /// Add a new feature to your existing polkadot-sdk project | ||
| #[clap(name = "add-to", alias = "a")] |
There was a problem hiding this comment.
I like pop add more too, is more simple as the rest of the commands. Does makes more sense to have:
pop add palletpop add config
Or you had something specific in mind to include in the runtime and the config to justify the subcommands?
There was a problem hiding this comment.
No need change now, just a heads-up for future work: changes in pop new pallet isn't within the scope of this PR (which focuses on pop add). It would be better to keep that change in a separate PR to keep things clean and focused.
| @@ -0,0 +1,35 @@ | |||
| // SPDX-License-Identifier: GPL-3.0 | |||
There was a problem hiding this comment.
To keep things simpler and avoid introducing extra new code (even if it's just sample files for testing), would it be possible to just instantiate a new pallet in a temporary file instead?
For reference, we follow a similar approach for parachain testing here: https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-parachains/src/new_parachain.rs#L102
| @@ -0,0 +1,258 @@ | |||
| // SPDX-License-Identifier: GPL-3.0 | |||
There was a problem hiding this comment.
Same as above, to avoid introducing extra new code (even if it's just sample files for testing), you can use something like this: https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-parachains/src/new_parachain.rs#L102
| @@ -0,0 +1,118 @@ | |||
| // SPDX-License-Identifier: GPL-3.0 | |||
|
|
|||
| use super::*; | |||
There was a problem hiding this comment.
For consistency with the rest of the codebase, we keep unit tests in the same file as the implementation. You can move this code to rust_writer_helpers.
| - name: Run integration tests | ||
| run: cargo test --no-default-features --features parachain --test parachain | ||
|
|
||
| file-writer-integration-tests: |
There was a problem hiding this comment.
It's currently not covered in parachain-integration-tests because we have cargo test --no-default-features --features parachain --test parachain. But definitely easier to just add it there (https://github.com/r0gue-io/pop-cli/blob/main/.github/workflows/ci.yml#L201) than having a whole new job:
run: |
cargo test --no-default-features --features parachain --test parachain
cargo test --no-default-features --features parachain --test new_pallet_added_to_existing_runtime
cargo test --no-default-features --features parachain --test pop_add_to_pallet_config_type
cargo test --no-default-features --features parachain --test pop_add_to_runtime_pallet
| CommonPallets::Contracts => parse_quote! { | ||
| ///TEMP_DOC | ||
| #[derive_impl(pallet_contracts::config_preludes::TestDefaultConfig)] | ||
| impl pallet_contracts::Config for Runtime{ |
There was a problem hiding this comment.
Is this not wrong config? e.g our template: https://github.com/r0gue-io/contracts-parachain/blob/main/runtime/src/configs/contracts.rs#L77
|
As discussed offline, this PR should be splitted in smaller ones. I've just created the first one: So I'm closing this one as it's not longer needed. I keep the branch for the moment as we might be interested in bringing the rest of the functionality in further PRs |
3 participants
Hey there!
This PR adds a new command to the POP CLI:
pop add-to, which enables modifying existing Polkadot-SDK workspaces by adding predefined structures to their code.Overview
The command follows a structured format:
pop add-to pallet config-type→ Adds a new config type to a pallet.pop add-to runtime pallet→ Adds a common pallet to a runtime.Each command ensures proper integration and includes a rollback mechanism to prevent partial modifications if an error occurs.
Available Modes
pop add-to pallet config-typeUsed to modify a pallet by adding a new config type. The changes are applied to:
✅ The pallet itself.
✅ The pallet’s mock runtime.
✅ The pallet’s config preludes (if they exist).
✅ The runtime’s implementation block (if needed).
📌 Usage:
📌 Supported Config Types:
RuntimeEventRuntimeOriginRuntimeHoldReasonRuntimeFreezeReasonFungiblesNew types can be added easily by modifying:
crates/pop-cli/src/commands/add/pallet_config_type/common_types.rspop add-to runtime palletUsed to add a pallet to an existing runtime, supporting both
construct_runtimeand#[runtime]macros. This command:✅ Adds the pallet to the runtime manifest.
✅ Creates a new file
configs/{pallet_name}.rsto manage its configuration.✅ Allows specifying an alternative path for the implementation block if needed.
📌 Usage:
📌 Supported Pallets:
BalancesContractsNew pallets can be added easily by modifying:
crates/pop-cli/src/commands/runtime_pallet/common_pallets.rsNote that
pop add-to runtimeandpop add-to palletare both valid commands, andpop add-to runtime pallet/pop add-to pallet config-typeare just subcommands. Maybe other subcommands can be added in the future or maybe not, but anyway I think it's worth it to use this format as it clearly specifies what is being added (config-type/pallet) and where (pallet/runtime).Additionally, the PR modifies the pop new pallet command, so now if it's run inside a workspace containing a runtime, the new pallet will be automatically added to that runtime.
I hope you may find this contribution useful, and happy to discuss any improvement in the UX that you can think of :)