-
Notifications
You must be signed in to change notification settings - Fork 24
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: add new parachain template and remove outdated one from parity #297
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #297 +/- ##
==========================================
+ Coverage 70.32% 70.66% +0.33%
==========================================
Files 53 53
Lines 9098 9254 +156
Branches 9098 9254 +156
==========================================
+ Hits 6398 6539 +141
- Misses 1718 1721 +3
- Partials 982 994 +12
|
A reminder that removals should be deprecated, if not already |
In this case, we had a message from the start indicating that the Parity templates were deprecated: https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-cli/src/commands/new/parachain.rs#L132. I believe it would be reasonable to remove the deprecated template in the next release. |
Perfect! |
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.
Just an initial quick review.
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.
┌ Pop CLI : Launch a local network
│
▲ ⚠️ The following binaries required to launch the network cannot be found locally:
│ > polkadot-parachain
│
◇ 📦 Would you like to source them automatically now? It may take some time...
│ > polkadot-parachain v1.13.0
│ Yes
│
⚙ ℹ️ Binaries will be cached at /Users/bruno/Library/Caches/pop
│
◇ 📦 Sourcing binaries...
│ ✅ polkadot-parachain
│
◐ 🚀 Launching local network...
thread 'main' panicked at /Users/bruno/.cargo/registry/src/index.crates.io-6f17d22bba15001f/zombienet-orchestrator-0.2.7/src/lib.rs:390:36:
removal index (is 0) should be < len (is 0)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Steps to reproduce:
I generated the Polkadot SDK Parachain Template using pop new parachain
command, then ran pop build
then pop up parachain -f zombienet.toml
@@ -216,7 +216,7 @@ async fn generate_parachain_from_template( | |||
// add next steps | |||
let mut next_steps = vec![ | |||
format!("cd into \"{name_template}\" and enjoy hacking! 🚀"), | |||
"Use `pop build` to build your parachain.".into(), | |||
"Use `pop build --release` to build your parachain.".into(), |
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.
I would keep it pop build
. Adding --release
takes way too long for development, and could degrade the UX for parachain development. --release
should be suggested when prepping the chain for onboarding.
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.
I made this change because it's common in the Zombienet configuration file to reference /target/release/
. See https://github.com/OpenZeppelin/polkadot-runtime-templates/blob/main/generic-template/zombienet-config/devnet.toml#L21 or https://github.com/r0gue-io/pop-cli/blob/main/tests/networks/template.toml#L16.
Without running pop build --release
, the path needs to be manually updated in the Zombienet file for things to work correctly.
message = "EVM", | ||
detailed_message = "Template node for a Frontier (EVM) based parachain.", | ||
serialize = "cpt", | ||
message = "Contracts", |
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.
What is the different between our Pop contract template and the substrate-contract-node?
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.
Parity one is paritytech/polkadot-sdk-parachain-template
and our is contracts
. The cpt
command has been retained for backward compatibility as we changed the naming to paritytech/substrate-contracts-node
but is now marked as deprecated: #297 (comment)
detailed_message = "Template node for a Frontier (EVM) based parachain.", | ||
serialize = "cpt", | ||
message = "Contracts", | ||
detailed_message = "Minimal Substrate node configured for smart contracts via pallet-contracts.", |
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.
Change this:
(Minimal Substrate node configured for smart contracts via pallet-contracts. [deprecated])
To this:
(Minimal Substrate node configured for smart contracts via pallet-contracts. [deprecated]. Suggested to use Pop > Contracts template instead.)
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.
Nitpick, there is an extra space after most of the template prompts e.g.:
(A standard parachain )
(Parachain configured with fungible and non-fungilble asset functionalities. )
(Parachain configured to support WebAssembly smart contracts. )
(Parachain configured with Frontier, enabling compatibility with the Ethereum Virtual Machine (EVM). )
(The Parachain-Ready Template From Polkadot SDK. )
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.
Is this a parachain? The prompt says: "Select the type of parachain" and defaults to ./my-parachain
:
◇ Select a template provider:
│ Parity
│
◇ Select the type of parachain:
│ Contracts
│
⚙ Template License: Unlicense
│
◇ Select a specific release:
│ v0.41.0
│
◆ Where should your project be created?
│ ./my-parachain
If so, we should say:
(Minimal parachain configured for smart contracts via pallet-contracts. [deprecated]. Suggested to use Pop > Contracts template instead.)
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.
I've addressed the first two nitpicks you mentioned, but I'm unsure about the last point. The question with the default value ./my-parachain
is: "Where should your project be created?" To me, it doesn't make sense to add the description there. Could you clarify what you meant?
Steps to reproduceI generated the Parity substrate contracts node (v0.41.0) using |
It looks like you're using the |
I see... If you run |
This PR introduces the new Parity parachain template: Polkadot SDK's Parachain Template
Additionally, the Frontier Parachain template has been removed due to its lack of maintenance (no updates in the past 9 months). For now, we will keep the Contracts Parachain, as its last update was just 4 months ago, but it will remain marked as deprecated.
Closes #238
Note for reviewer:The version bump for Zombienet-SDK is necessary due to the following fix paritytech/zombienet-sdk#253