From a91ccfd803cf37d93462e43bf56bc6406b8a4344 Mon Sep 17 00:00:00 2001 From: Kazunobu Ndong <33208377+ndkazu@users.noreply.github.com> Date: Tue, 7 Jan 2025 17:50:06 +0900 Subject: [PATCH] chore: support specify contract path input with or without -p flag (#361) * pop -p arg * pop build & pop up completed * re-starting from scratch * Cleaner version of first proposal * cargo +nightly fmt * pop build completed * pop up contract implementation * pop call contract implemented * Fixed some of the issues pointed in the reviews * cargo +nightly fmt * Review corrections implemented * cargo fmt * Fixing CI errors * License --- crates/pop-cli/src/commands/build/mod.rs | 23 +++++-- crates/pop-cli/src/commands/call/contract.rs | 63 ++++++++++++-------- crates/pop-cli/src/commands/up/contract.rs | 23 +++++-- crates/pop-cli/src/common/builds.rs | 12 ++++ crates/pop-cli/src/common/mod.rs | 1 + crates/pop-cli/tests/contract.rs | 13 +++- 6 files changed, 98 insertions(+), 37 deletions(-) create mode 100644 crates/pop-cli/src/common/builds.rs diff --git a/crates/pop-cli/src/commands/build/mod.rs b/crates/pop-cli/src/commands/build/mod.rs index d9ab9ba4..1f805f8d 100644 --- a/crates/pop-cli/src/commands/build/mod.rs +++ b/crates/pop-cli/src/commands/build/mod.rs @@ -1,6 +1,9 @@ // SPDX-License-Identifier: GPL-3.0 -use crate::cli::{self, Cli}; +use crate::{ + cli::{self, Cli}, + common::builds::get_project_path, +}; use clap::{Args, Subcommand}; #[cfg(feature = "contract")] use contract::BuildContract; @@ -23,9 +26,12 @@ pub(crate) mod spec; pub(crate) struct BuildArgs { #[command(subcommand)] pub command: Option, - /// Directory path for your project [default: current directory] + /// Directory path with flag for your project [default: current directory] #[arg(long)] pub(crate) path: Option, + /// Directory path without flag for your project [default: current directory] + #[arg(value_name = "PATH", index = 1, conflicts_with = "path")] + pub(crate) path_pos: Option, /// The package to be built. #[arg(short = 'p', long)] pub(crate) package: Option, @@ -50,25 +56,29 @@ impl Command { /// Executes the command. pub(crate) fn execute(args: BuildArgs) -> anyhow::Result<&'static str> { // If only contract feature enabled, build as contract + let project_path = get_project_path(args.path.clone(), args.path_pos.clone()); + #[cfg(feature = "contract")] - if pop_contracts::is_supported(args.path.as_deref())? { + if pop_contracts::is_supported(project_path.as_deref().map(|v| v))? { + // All commands originating from root command are valid let release = match args.profile { Some(profile) => profile.into(), None => args.release, }; - BuildContract { path: args.path, release }.execute()?; + BuildContract { path: project_path, release }.execute()?; return Ok("contract"); } // If only parachain feature enabled, build as parachain #[cfg(feature = "parachain")] - if pop_parachains::is_supported(args.path.as_deref())? { + if pop_parachains::is_supported(project_path.as_deref().map(|v| v))? { let profile = match args.profile { Some(profile) => profile, None => args.release.into(), }; + let temp_path = PathBuf::from("./"); BuildParachain { - path: args.path.unwrap_or_else(|| PathBuf::from("./")), + path: project_path.unwrap_or_else(|| temp_path).to_path_buf(), package: args.package, profile, } @@ -140,6 +150,7 @@ mod tests { BuildArgs { command: None, path: Some(project_path.clone()), + path_pos: Some(project_path.clone()), package: package.clone(), release, profile: Some(profile.clone()), diff --git a/crates/pop-cli/src/commands/call/contract.rs b/crates/pop-cli/src/commands/call/contract.rs index ab7a826f..629ca204 100644 --- a/crates/pop-cli/src/commands/call/contract.rs +++ b/crates/pop-cli/src/commands/call/contract.rs @@ -3,6 +3,7 @@ use crate::{ cli::{self, traits::*}, common::{ + builds::get_project_path, contracts::has_contract_been_built, wallet::{prompt_to_use_wallet, request_signature}, }, @@ -17,7 +18,7 @@ use pop_contracts::{ parse_account, set_up_call, CallExec, CallOpts, DefaultEnvironment, Verbosity, }; use sp_weights::Weight; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; const DEFAULT_URL: &str = "ws://localhost:9944/"; const DEFAULT_URI: &str = "//Alice"; @@ -28,6 +29,9 @@ pub struct CallContractCommand { /// Path to the contract build directory or a contract artifact. #[arg(short, long)] path: Option, + /// Directory path without flag for your project [default: current directory] + #[arg(value_name = "PATH", index = 1, conflicts_with = "path")] + pub(crate) path_pos: Option, /// The address of the contract to call. #[arg(short, long, env = "CONTRACT")] contract: Option, @@ -109,9 +113,13 @@ impl CallContractCommand { fn display(&self) -> String { let mut full_message = "pop call contract".to_string(); + if let Some(path) = &self.path { full_message.push_str(&format!(" --path {}", path.display())); } + if let Some(path_pos) = &self.path_pos { + full_message.push_str(&format!(" --path {}", path_pos.display())); + } if let Some(contract) = &self.contract { full_message.push_str(&format!(" --contract {}", contract)); } @@ -148,11 +156,12 @@ impl CallContractCommand { /// If the contract has not been built, build it in release mode. async fn ensure_contract_built(&self, cli: &mut impl Cli) -> Result<()> { + let project_path = get_project_path(self.path.clone(), self.path_pos.clone()); // Build the contract in release mode cli.warning("NOTE: contract has not yet been built.")?; let spinner = spinner(); spinner.start("Building contract in RELEASE mode..."); - let result = match build_smart_contract(self.path.as_deref(), true, Verbosity::Quiet) { + let result = match build_smart_contract(project_path.as_deref(), true, Verbosity::Quiet) { Ok(result) => result, Err(e) => { return Err(anyhow!(format!( @@ -182,7 +191,9 @@ impl CallContractCommand { /// Checks whether building the contract is required fn is_contract_build_required(&self) -> bool { - self.path + let project_path = get_project_path(self.path.clone(), self.path_pos.clone()); + + project_path .as_ref() .map(|p| p.is_dir() && !has_contract_been_built(Some(p))) .unwrap_or_default() @@ -190,6 +201,8 @@ impl CallContractCommand { /// Configure the call based on command line arguments/call UI. async fn configure(&mut self, cli: &mut impl Cli, repeat: bool) -> Result<()> { + let mut project_path = get_project_path(self.path.clone(), self.path_pos.clone()); + // Show intro on first run. if !repeat { cli.intro("Call a contract")?; @@ -201,16 +214,15 @@ impl CallContractCommand { } // Resolve path. - if self.path.is_none() { + if project_path.is_none() { let input_path: String = cli .input("Where is your project or contract artifact located?") .placeholder("./") .default_input("./") .interact()?; - self.path = Some(PathBuf::from(input_path)); + project_path = Some(PathBuf::from(input_path)); } - let contract_path = self - .path + let contract_path = project_path .as_ref() .expect("path is guaranteed to be set as input as prompted when None; qed"); @@ -357,6 +369,8 @@ impl CallContractCommand { cli: &mut impl Cli, prompt_to_repeat_call: bool, ) -> Result<()> { + let project_path = get_project_path(self.path.clone(), self.path_pos.clone()); + let message = match &self.message { Some(message) => message.to_string(), None => { @@ -364,8 +378,9 @@ impl CallContractCommand { }, }; // Disable wallet signing and display warning if the call is read-only. + let path = PathBuf::from("./"); let message_metadata = - get_message(self.path.as_deref().unwrap_or_else(|| Path::new("./")), &message)?; + get_message(project_path.as_deref().unwrap_or_else(|| &path), &message)?; if !message_metadata.mutates && self.use_wallet { cli.warning("NOTE: Signing is not required for this read-only call. The '--use-wallet' flag will be ignored.")?; self.use_wallet = false; @@ -378,7 +393,7 @@ impl CallContractCommand { }, }; let call_exec = match set_up_call(CallOpts { - path: self.path.clone(), + path: project_path, contract, message, args: self.args.clone(), @@ -564,6 +579,7 @@ mod tests { // Contract deployed on Pop Network testnet, test get CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: Some("15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm".to_string()), message: Some("get".to_string()), args: vec![].to_vec(), @@ -600,6 +616,7 @@ mod tests { let mut call_config = CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: Some("15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm".to_string()), message: Some("flip".to_string()), args: vec![].to_vec(), @@ -637,6 +654,7 @@ mod tests { // From .contract file let mut call_config = CallContractCommand { path: Some(current_dir.join("pop-contracts/tests/files/testing.contract")), + path_pos: None, contract: Some("15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm".to_string()), message: Some("flip".to_string()), args: vec![].to_vec(), @@ -723,6 +741,7 @@ mod tests { // Contract deployed on Pop Network testnet, test get let mut call_config = CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: Some("15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm".to_string()), message: Some("get".to_string()), args: vec![].to_vec(), @@ -770,10 +789,6 @@ mod tests { Some(items), 1, // "get" message ) - .expect_input( - "Where is your project or contract artifact located?", - temp_dir.path().join("testing").display().to_string(), - ) .expect_input( "Where is your contract deployed?", "wss://rpc1.paseo.popnetwork.xyz".into(), @@ -789,6 +804,7 @@ mod tests { let mut call_config = CallContractCommand { path: None, + path_pos: Some(temp_dir.path().join("testing")), contract: None, message: None, args: vec![].to_vec(), @@ -818,7 +834,7 @@ mod tests { assert!(!call_config.dry_run); assert_eq!(call_config.display(), format!( "pop call contract --path {} --contract 15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm --message get --url wss://rpc1.paseo.popnetwork.xyz/ --suri //Alice", - temp_dir.path().join("testing").display().to_string(), + temp_dir.path().join("testing").display().to_string() )); cli.verify() @@ -844,10 +860,6 @@ mod tests { ]; // The inputs are processed in reverse order. let mut cli = MockCli::new() - .expect_input( - "Where is your project or contract artifact located?", - temp_dir.path().join("testing").display().to_string(), - ) .expect_input( "Where is your contract deployed?", "wss://rpc1.paseo.popnetwork.xyz".into(), @@ -876,6 +888,7 @@ mod tests { let mut call_config = CallContractCommand { path: None, + path_pos: Some(temp_dir.path().join("testing")), contract: None, message: None, args: vec![].to_vec(), @@ -908,7 +921,7 @@ mod tests { assert!(!call_config.dry_run); assert_eq!(call_config.display(), format!( "pop call contract --path {} --contract 15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm --message specific_flip --args \"true\", \"2\" --value 50 --url wss://rpc1.paseo.popnetwork.xyz/ --use-wallet --execute", - temp_dir.path().join("testing").display().to_string(), + temp_dir.path().join("testing").display().to_string() )); cli.verify() @@ -941,10 +954,6 @@ mod tests { Some(items), 2, // "specific_flip" message ) - .expect_input( - "Where is your project or contract artifact located?", - temp_dir.path().join("testing").display().to_string(), - ) .expect_input( "Where is your contract deployed?", "wss://rpc1.paseo.popnetwork.xyz".into(), @@ -964,6 +973,7 @@ mod tests { let mut call_config = CallContractCommand { path: None, + path_pos: Some(temp_dir.path().join("testing")), contract: None, message: None, args: vec![].to_vec(), @@ -996,7 +1006,7 @@ mod tests { assert!(call_config.dev_mode); assert_eq!(call_config.display(), format!( "pop call contract --path {} --contract 15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm --message specific_flip --args \"true\", \"2\" --value 50 --url wss://rpc1.paseo.popnetwork.xyz/ --suri //Alice --execute", - temp_dir.path().join("testing").display().to_string(), + temp_dir.path().join("testing").display().to_string() )); cli.verify() @@ -1023,6 +1033,7 @@ mod tests { // Test the path is a folder with an invalid build. let mut command = CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: None, message: None, args: vec![].to_vec(), @@ -1073,6 +1084,7 @@ mod tests { assert!(matches!( CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: Some("15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm".to_string()), message: None, args: vec![].to_vec(), @@ -1092,6 +1104,7 @@ mod tests { assert!(matches!( CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: None, message: Some("get".to_string()), args: vec![].to_vec(), @@ -1116,6 +1129,7 @@ mod tests { let temp_dir = new_environment("testing")?; let call_config = CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: Some("15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm".to_string()), message: None, args: vec![].to_vec(), @@ -1147,6 +1161,7 @@ mod tests { let temp_dir = new_environment("testing")?; let call_config = CallContractCommand { path: Some(temp_dir.path().join("testing")), + path_pos: None, contract: Some("15XausWjFLBBFLDXUSBRfSfZk25warm4wZRV4ZxhZbfvjrJm".to_string()), message: None, args: vec![].to_vec(), diff --git a/crates/pop-cli/src/commands/up/contract.rs b/crates/pop-cli/src/commands/up/contract.rs index f46c0776..c11e55e5 100644 --- a/crates/pop-cli/src/commands/up/contract.rs +++ b/crates/pop-cli/src/commands/up/contract.rs @@ -3,6 +3,7 @@ use crate::{ cli::{traits::Cli as _, Cli}, common::{ + builds::get_project_path, contracts::{check_contracts_node_and_prompt, has_contract_been_built, terminate_node}, wallet::request_signature, }, @@ -34,6 +35,9 @@ pub struct UpContractCommand { /// Path to the contract build directory. #[arg(short, long)] path: Option, + /// Directory path without flag for your project [default: current directory] + #[arg(value_name = "PATH", index = 1, conflicts_with = "path")] + pub path_pos: Option, /// The name of the contract constructor to call. #[clap(short, long, default_value = "new")] constructor: String, @@ -92,13 +96,18 @@ impl UpContractCommand { pub(crate) async fn execute(mut self) -> anyhow::Result<()> { Cli.intro("Deploy a smart contract")?; + let project_path = get_project_path(self.path.clone(), self.path_pos.clone()); // Check if build exists in the specified "Contract build directory" - if !has_contract_been_built(self.path.as_deref()) { + if !has_contract_been_built(project_path.as_deref()) { // Build the contract in release mode Cli.warning("NOTE: contract has not yet been built.")?; let spinner = spinner(); spinner.start("Building contract in RELEASE mode..."); - let result = match build_smart_contract(self.path.as_deref(), true, Verbosity::Quiet) { + let result = match build_smart_contract( + project_path.as_deref().map(|v| v), + true, + Verbosity::Quiet, + ) { Ok(result) => result, Err(e) => { Cli.outro_cancel(format!("🚫 An error occurred building your contract: {e}\nUse `pop build` to retry with build output."))?; @@ -360,7 +369,8 @@ impl UpContractCommand { // get the call data and contract code hash async fn get_contract_data(&self) -> anyhow::Result<(Vec, [u8; 32])> { - let contract_code = get_contract_code(self.path.as_ref())?; + let project_path = get_project_path(self.path.clone(), self.path_pos.clone()); + let contract_code = get_contract_code(project_path.as_ref())?; let hash = contract_code.code_hash(); if self.upload_only { let call_data = get_upload_payload(contract_code, self.url.as_str()).await?; @@ -435,6 +445,7 @@ mod tests { fn default_up_contract_command() -> UpContractCommand { UpContractCommand { path: None, + path_pos: None, constructor: "new".to_string(), args: vec![], value: "0".to_string(), @@ -510,7 +521,8 @@ mod tests { let localhost_url = format!("ws://127.0.0.1:{}", port); let up_contract_opts = UpContractCommand { - path: Some(temp_dir), + path: Some(temp_dir.clone()), + path_pos: Some(temp_dir), constructor: "new".to_string(), args: vec![], value: "0".to_string(), @@ -561,7 +573,8 @@ mod tests { let localhost_url = format!("ws://127.0.0.1:{}", port); let up_contract_opts = UpContractCommand { - path: Some(temp_dir), + path: Some(temp_dir.clone()), + path_pos: Some(temp_dir), constructor: "new".to_string(), args: vec!["false".to_string()], value: "0".to_string(), diff --git a/crates/pop-cli/src/common/builds.rs b/crates/pop-cli/src/common/builds.rs new file mode 100644 index 00000000..1192ed0c --- /dev/null +++ b/crates/pop-cli/src/common/builds.rs @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: GPL-3.0 +use std::path::PathBuf; + +/// This method is used to get the proper project path format (with or without cli flag) +pub fn get_project_path(path_flag: Option, path_pos: Option) -> Option { + let project_path = if let Some(ref path) = path_pos { + Some(path) // Use positional path if present + } else { + path_flag.as_ref() // Otherwise, use the named path + }; + project_path.cloned() +} diff --git a/crates/pop-cli/src/common/mod.rs b/crates/pop-cli/src/common/mod.rs index 4a89036e..512d2a67 100644 --- a/crates/pop-cli/src/common/mod.rs +++ b/crates/pop-cli/src/common/mod.rs @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-3.0 +pub mod builds; #[cfg(feature = "contract")] pub mod contracts; pub mod helpers; diff --git a/crates/pop-cli/tests/contract.rs b/crates/pop-cli/tests/contract.rs index 1af21b38..a095702a 100644 --- a/crates/pop-cli/tests/contract.rs +++ b/crates/pop-cli/tests/contract.rs @@ -74,6 +74,7 @@ async fn contract_lifecycle() -> Result<()> { .args(&["build", "--path", "./test_contract", "--release"]) .assert() .success(); + // Verify that the directory target has been created assert!(temp_dir.join("test_contract/target").exists()); // Verify that all the artifacts has been generated @@ -88,11 +89,19 @@ async fn contract_lifecycle() -> Result<()> { sleep(Duration::from_secs(5)).await; // Only upload the contract - // pop up contract --upload-only + // pop up contract --path ./test_contract --upload-only Command::cargo_bin("pop") .unwrap() .current_dir(&temp_dir.join("test_contract")) - .args(&["up", "contract", "--upload-only", "--url", default_endpoint]) + .args(&[ + "up", + "contract", + "--path", + "./test_contract", + "--upload-only", + "--url", + default_endpoint, + ]) .assert() .success(); // Instantiate contract, only dry-run