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: add new parachain template and remove outdated one from parity #297

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

AlexD10S
Copy link
Collaborator

@AlexD10S AlexD10S commented Aug 28, 2024

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

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 19 lines in your changes missing coverage. Please review.

Project coverage is 70.66%. Comparing base (582db18) to head (34ff183).

Files with missing lines Patch % Lines
crates/pop-parachains/src/up/mod.rs 87.39% 2 Missing and 13 partials ⚠️
crates/pop-cli/src/commands/new/parachain.rs 55.55% 4 Missing ⚠️
@@            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     
Files with missing lines Coverage Δ
crates/pop-common/src/templates/mod.rs 64.58% <100.00%> (+7.08%) ⬆️
crates/pop-parachains/src/templates.rs 98.43% <100.00%> (+0.23%) ⬆️
crates/pop-cli/src/commands/new/parachain.rs 46.17% <55.55%> (+0.04%) ⬆️
crates/pop-parachains/src/up/mod.rs 87.52% <87.39%> (+0.11%) ⬆️

@evilrobot-01
Copy link
Contributor

A reminder that removals should be deprecated, if not already

@AlexD10S
Copy link
Collaborator Author

AlexD10S commented Sep 6, 2024

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.

@evilrobot-01
Copy link
Contributor

Perfect!

@AlexD10S AlexD10S marked this pull request as ready for review September 12, 2024 12:25
Copy link
Contributor

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

crates/pop-parachains/src/templates.rs Outdated Show resolved Hide resolved
@evilrobot-01 evilrobot-01 changed the title feat: add new parachain template and remove outdated one from Parity feat: add new parachain template and remove outdated one from parity Sep 18, 2024
crates/pop-parachains/src/templates.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/templates.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/templates.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/templates.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/templates.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/up/mod.rs Show resolved Hide resolved
Copy link
Collaborator

@brunopgalvao brunopgalvao left a 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(),
Copy link
Collaborator

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.

Copy link
Collaborator Author

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",
Copy link
Collaborator

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?

Copy link
Collaborator Author

@AlexD10S AlexD10S Sep 22, 2024

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.",
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator Author

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?

@brunopgalvao
Copy link
Collaborator

   Compiling polkadot-service v11.0.0
   Compiling cumulus-relay-chain-minimal-node v0.11.0
   Compiling cumulus-relay-chain-inprocess-interface v0.11.0
   Compiling cumulus-client-service v0.11.0
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 7m 42s
Error: Missing binary: contracts-node

Steps to reproduce

I generated the Parity substrate contracts node (v0.41.0) using pop new parachain then got this error when running pop build.

@AlexD10S
Copy link
Collaborator Author

┌   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

It looks like you're using the pop installed from the main branch, not from the current branch. The error you're seeing occurs because the parachain template contains [parachain.collator], which was addressed in the latest version of Zombienet. That's why we're updating the Zombienet version in this branch (Read description).

@AlexD10S
Copy link
Collaborator Author

   Compiling polkadot-service v11.0.0
   Compiling cumulus-relay-chain-minimal-node v0.11.0
   Compiling cumulus-relay-chain-inprocess-interface v0.11.0
   Compiling cumulus-client-service v0.11.0
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 7m 42s
Error: Missing binary: contracts-node

Steps to reproduce

I generated the Parity substrate contracts node (v0.41.0) using pop new parachain then got this error when running pop build.

I see... If you run pop up parachain -f zombienet.toml, it works, but the error message during the build looks bad. Could you open an issue in their repository instead? https://github.com/paritytech/substrate-contracts-node. Alternatively, we could decide to remove this template if it's not going to be fixed.

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.

pop new: Use latest paritytech templates
4 participants