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: liquid standard templates #231

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

al3mart
Copy link
Contributor

@al3mart al3mart commented Jun 28, 2024

This PR adapts pop-cli so that it generates templates with Pop as Provider from a liquid template (r0gue-io/base-parachain).

Includes the ability to customize the name of the project based on user's input.
The specific customized fields are:

  • Name of the node crate name.
  • Name of the runtime crate.
  • Name of the resulting binary.

We maintain the customization of the token's symbol and decimals as well as the initial-endowment for dev accounts.

NewParachainCommand.decimals is now of type String instead of u8.

As it stands at this point base-parachain needs work to include the rest of templates that we are maintaining today that are not the "standard" one.

Jinja templates for parachain generation are removed as we don't need them anymore after this change.

Adds a check to verify that the decimals input given by the user is numeric.

Future work, worth to include in this PR:

  • At this stage this PR introduces a breaking change:

The flow for initializing projects out of the templates effectively changes, meaning that trying to generate a project from base-parachain#v1.11.0 would not serve network.toml with the current binary name for the generated parachain, for instance.

An option could be handling the different versions with different ways of generating the tempaltes.
Another option could be marking the templates released prior to releasing liquid tempaltes as deprecated.

How to test:

This build can be tested against the current liquid tempalted pushed to base-parachain#liquid-template like this:
pop new parachain --template standard -r liquid-template -s LIQUID -d 10 -i "2u64 << 60" test-parachain

Note:

The flow guiding the user only presents them with the released versions of the tempalte, the liquid template hasn't been released yet. But can be used as shown above.

test_parachain_instantiate_standard_template is not passing due to a discrepancy in the naming of the project folder created by TempDir and the generated. The generated is created using kebab-case while the temporary one seems to use camelCase . This needs addressing.

let template_generation_args = GenerateArgs {
template_path: standard_template_path,
name: Some(target.file_name().unwrap().to_str().unwrap().to_string()),
destination: Some(target.parent().unwrap().to_path_buf()),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type conversions here could maybe be done in a cleaner way ?

use tempfile::tempdir;

#[test]
fn test_write_to_file() -> Result<(), Box<dyn std::error::Error>> {
Copy link
Contributor Author

@al3mart al3mart Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't do this anymore, hence the removal.

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.

Nice!
About generating the Zombienet file, we can keep the previous work for that no? In the instantiate_standard_template:

// Add network configuration
use askama::Template;
let network = Network { node: "name-we-want".into() };
write_to_file(&target.join("network.toml"), network.render().expect("infallible").as_ref())?;

And keep the generator/parachain.rs file with:

#[derive(Template)]
#[template(path = "base/network.templ", escape = "none")]
pub(crate) struct Network {
	pub(crate) node: String,
}

crates/pop-parachains/src/utils/helpers.rs Show resolved Hide resolved
@@ -80,12 +89,19 @@ mod tests {

fn setup_template_and_instantiate() -> Result<tempfile::TempDir> {
let temp_dir = tempfile::tempdir().expect("Failed to create temp dir");
println!("{:?}", temp_dir);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

crates/pop-parachains/src/new_parachain.rs Show resolved Hide resolved

let tag = Git::clone_and_degit(template.repository_url()?, source, tag_version)?;
// Placeholder customization
token_symbol.push_str(&*config.symbol);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small comment, this:

let mut token_symbol: String = "token-symbol=".to_string();
token_symbol.push_str(&*config.symbol);

can be refactor to:

let token_symbol: String = format!("token-symbol={}", config.symbol);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this makes more sense!

crates/pop-parachains/src/new_parachain.rs Show resolved Hide resolved
git: Some(
template
.repository_url()
.map_or(String::from("https://github.com/r0gue.io/base-parachain"), |r| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template.repository_url() should never produce an error because it is our responsibility to set the repository URL correctly.
Not sure the best approach here, personally I'd throw an error instead of hardcoding the base-parachain url, or if we don't want to throw an error return a the DEFAULT value in the repository_url function(https://github.com/r0gue-io/pop-cli/blob/main/crates/pop-parachains/src/templates.rs#L225, )

crates/pop-parachains/src/new_parachain.rs Show resolved Hide resolved
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.

2 participants