From a252c4599f5fe01eff7f2ec780334128d378a7ca Mon Sep 17 00:00:00 2001 From: Frank Bell <60948618+evilrobot-01@users.noreply.github.com> Date: Mon, 8 Jul 2024 21:49:50 +0100 Subject: [PATCH] refactor: improve up ux (#248) * refactor: improve up ux * test: update integration tests after refactoring * fix: update url with default if one specified is inaccessible --- crates/pop-cli/src/commands/build/contract.rs | 7 +- crates/pop-cli/src/commands/up/contract.rs | 184 ++++++++++++++---- crates/pop-cli/src/commands/up/mod.rs | 2 +- crates/pop-contracts/src/build.rs | 18 +- crates/pop-contracts/src/lib.rs | 2 +- crates/pop-contracts/src/up.rs | 2 +- .../pop-contracts/src/utils/contracts_node.rs | 19 +- crates/pop-contracts/tests/contract.rs | 23 ++- 8 files changed, 188 insertions(+), 69 deletions(-) diff --git a/crates/pop-cli/src/commands/build/contract.rs b/crates/pop-cli/src/commands/build/contract.rs index 71313103..a410bd66 100644 --- a/crates/pop-cli/src/commands/build/contract.rs +++ b/crates/pop-cli/src/commands/build/contract.rs @@ -2,7 +2,7 @@ use crate::cli; use clap::Args; -use pop_contracts::build_smart_contract; +use pop_contracts::{build_smart_contract, Verbosity}; use std::path::PathBuf; #[cfg(not(test))] use std::{thread::sleep, time::Duration}; @@ -42,8 +42,9 @@ impl BuildContractCommand { } // Build contract. - let build_result = build_smart_contract(self.path.as_deref(), self.release)?; - cli.success(build_result)?; + let build_result = + build_smart_contract(self.path.as_deref(), self.release, Verbosity::Default)?; + cli.success(build_result.display())?; cli.outro("Build completed successfully!")?; Ok("contract") } diff --git a/crates/pop-cli/src/commands/up/contract.rs b/crates/pop-cli/src/commands/up/contract.rs index 21c32cbd..60a225d2 100644 --- a/crates/pop-cli/src/commands/up/contract.rs +++ b/crates/pop-cli/src/commands/up/contract.rs @@ -1,16 +1,27 @@ // SPDX-License-Identifier: GPL-3.0 -use crate::style::style; +use crate::{ + cli::{traits::Cli as _, Cli}, + style::style, +}; use clap::Args; -use cliclack::{clear_screen, confirm, intro, log, log::success, outro, outro_cancel}; +use cliclack::{confirm, log, log::error, spinner}; +use console::{Emoji, Style}; use pop_contracts::{ build_smart_contract, dry_run_gas_estimate_instantiate, dry_run_upload, instantiate_smart_contract, is_chain_alive, parse_hex_bytes, run_contracts_node, - set_up_deployment, set_up_upload, upload_smart_contract, UpOpts, + set_up_deployment, set_up_upload, upload_smart_contract, UpOpts, Verbosity, }; use sp_core::Bytes; use sp_weights::Weight; use std::path::PathBuf; +use std::process::Child; +use tempfile::NamedTempFile; +use url::Url; + +const COMPLETE: &str = "🚀 Deployment complete"; +const DEFAULT_URL: &str = "ws://localhost:9944/"; +const FAILED: &str = "🚫 Deployment failed."; #[derive(Args, Clone)] pub struct UpContractCommand { @@ -39,8 +50,8 @@ pub struct UpContractCommand { /// instances of the same contract code from the same account. #[clap(long, value_parser = parse_hex_bytes)] salt: Option, - /// Websocket endpoint of a node. - #[clap(name = "url", long, value_parser, default_value = "ws://localhost:9944")] + /// Websocket endpoint of a chain. + #[clap(name = "url", long, value_parser, default_value = DEFAULT_URL)] url: url::Url, /// Secret key URI for the account deploying the contract. /// @@ -55,54 +66,105 @@ pub struct UpContractCommand { /// Uploads the contract only, without instantiation. #[clap(short('u'), long)] upload_only: bool, - /// Before start a local node, do not ask the user for confirmation. + /// Before starting a local node, do not ask the user for confirmation. #[clap(short('y'), long)] skip_confirm: bool, } impl UpContractCommand { /// Executes the command. - pub(crate) async fn execute(self) -> anyhow::Result<()> { - clear_screen()?; + pub(crate) async fn execute(mut self) -> anyhow::Result<()> { + Cli.intro("Deploy a smart contract")?; // Check if build exists in the specified "Contract build folder" let build_path = PathBuf::from( self.path.clone().unwrap_or("/.".into()).to_string_lossy().to_string() + "/target/ink", ); - if !build_path.as_path().exists() { - log::warning("NOTE: contract has not yet been built.")?; - intro(format!("{}: Building a contract", style(" Pop CLI ").black().on_magenta()))?; // Build the contract in release mode - let result = build_smart_contract(self.path.as_deref(), true)?; - log::success(result.to_string())?; + 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) { + Ok(result) => result, + Err(e) => { + Cli.outro_cancel(format!("🚫 An error occurred building your contract: {e}\nUse `pop build` to retry with build output."))?; + return Ok(()); + }, + }; + spinner.stop(format!( + "Your contract artifacts are ready. You can find them in: {}", + result.target_directory.display() + )); } - if !is_chain_alive(self.url.clone()).await? { + // Check if specified chain is accessible + let process = if !is_chain_alive(self.url.clone()).await? { if !self.skip_confirm { + let chain = if self.url.as_str() == DEFAULT_URL { + "No endpoint was specified.".into() + } else { + format!("The specified endpoint of {} is inaccessible.", self.url) + }; + if !confirm(format!( - "The chain \"{}\" is not live. Would you like pop to start a local node in the background for testing?", - self.url.to_string() - )) - .interact()? - { - outro_cancel("You need to specify a live chain to deploy the contract.")?; - return Ok(()); - } + "{chain} Would you like to start a local node in the background for testing?", + )) + .initial_value(true) + .interact()? + { + Cli.outro_cancel( + "🚫 You need to specify an accessible endpoint to deploy the contract.", + )?; + return Ok(()); + } } - let process = run_contracts_node(crate::cache()?).await?; - log::success("Local node started successfully in the background.")?; - log::warning(format!("NOTE: The contracts node is running in the background with process ID {}. Please close it manually when done testing.", process.id()))?; - } - // if build exists then proceed - intro(format!("{}: Deploy a smart contract", style(" Pop CLI ").black().on_magenta()))?; + // Update url to that of the launched node + self.url = Url::parse(DEFAULT_URL).expect("default url is valid"); + + let spinner = spinner(); + spinner.start("Starting local node..."); + let log = tempfile::NamedTempFile::new()?; + let process = run_contracts_node(crate::cache()?, Some(log.as_file())).await?; + let bar = Style::new().magenta().dim().apply_to(Emoji("│", "|")); + spinner.stop(format!( + "Local node started successfully:{}", + style(format!( + " +{bar} {} +{bar} {}", + style(format!( + "portal: https://polkadot.js.org/apps/?rpc={}#/explorer", + self.url + )) + .dim(), + style(format!("logs: tail -f {}", log.path().display())).dim(), + )) + .dim() + )); + Some((process, log)) + } else { + None + }; + // Check for upload only. if self.upload_only { - return self.upload_contract().await; + let result = self.upload_contract().await; + Self::terminate_node(process)?; + match result { + Ok(_) => { + Cli.outro(COMPLETE)?; + }, + Err(_) => { + Cli.outro_cancel(FAILED)?; + }, + } + return Ok(()); } - let instantiate_exec = set_up_deployment(UpOpts { + // Otherwise instantiate. + let instantiate_exec = match set_up_deployment(UpOpts { path: self.path.clone(), constructor: self.constructor.clone(), args: self.args.clone(), @@ -113,28 +175,40 @@ impl UpContractCommand { url: self.url.clone(), suri: self.suri.clone(), }) - .await?; + .await + { + Ok(i) => i, + Err(e) => { + error(format!("An error occurred instantiating the contract: {e}"))?; + Self::terminate_node(process)?; + Cli.outro_cancel(FAILED)?; + return Ok(()); + }, + }; let weight_limit; if self.gas_limit.is_some() && self.proof_size.is_some() { weight_limit = Weight::from_parts(self.gas_limit.unwrap(), self.proof_size.unwrap()); } else { - let spinner = cliclack::spinner(); + let spinner = spinner(); spinner.start("Doing a dry run to estimate the gas..."); weight_limit = match dry_run_gas_estimate_instantiate(&instantiate_exec).await { Ok(w) => { - log::info(format!("Gas limit: {:?}", w))?; + spinner.stop(format!("Gas limit estimate: {:?}", w)); w }, Err(e) => { spinner.error(format!("{e}")); - outro_cancel("Deployment failed.")?; + Self::terminate_node(process)?; + Cli.outro_cancel(FAILED)?; return Ok(()); }, }; } + + // Finally upload and instantiate. if !self.dry_run { - let spinner = cliclack::spinner(); + let spinner = spinner(); spinner.start("Uploading and instantiating the contract..."); let contract_address = instantiate_smart_contract(instantiate_exec, weight_limit).await?; @@ -142,8 +216,10 @@ impl UpContractCommand { "Contract deployed and instantiated: The Contract Address is {:?}", contract_address )); - outro("Deployment complete")?; + Self::terminate_node(process)?; + Cli.outro(COMPLETE)?; } + Ok(()) } @@ -161,23 +237,47 @@ impl UpContractCommand { style(format!("{} {s}", console::Emoji("●", ">"))).dim().to_string() }) .collect(); - success(format!("Dry run successful!\n{}", result.join("\n")))?; + Cli.success(format!("Dry run successful!\n{}", result.join("\n")))?; }, Err(_) => { - outro_cancel("Deployment failed.")?; + Cli.outro_cancel(FAILED)?; return Ok(()); }, }; } else { - let spinner = cliclack::spinner(); - spinner.start("Uploading the contract..."); - let code_hash = upload_smart_contract(&upload_exec).await?; + let spinner = spinner(); + spinner.start("Uploading your contract..."); + let code_hash = match upload_smart_contract(&upload_exec).await { + Ok(r) => r, + Err(e) => { + spinner.error(format!("An error occurred uploading your contract: {e}")); + return Err(e.into()); + }, + }; spinner.stop(format!("Contract uploaded: The code hash is {:?}", code_hash)); - outro("Deployment complete")?; log::warning("NOTE: The contract has not been instantiated.")?; } return Ok(()); } + + /// Handles the optional termination of a local running node. + fn terminate_node(process: Option<(Child, NamedTempFile)>) -> anyhow::Result<()> { + // Prompt to close any launched node + let Some((mut process, log)) = process else { + return Ok(()); + }; + if confirm("Would you like to terminate the local node?") + .initial_value(true) + .interact()? + { + process.kill()? + } else { + log.keep()?; + log::warning(format!("NOTE: The node is running in the background with process ID {}. Please terminate it manually when done.", process.id()))?; + } + + Ok(()) + } } impl From for UpOpts { diff --git a/crates/pop-cli/src/commands/up/mod.rs b/crates/pop-cli/src/commands/up/mod.rs index 4954d0ab..500e2e20 100644 --- a/crates/pop-cli/src/commands/up/mod.rs +++ b/crates/pop-cli/src/commands/up/mod.rs @@ -23,7 +23,7 @@ pub(crate) enum Command { #[clap(alias = "p")] Parachain(parachain::ZombienetCommand), #[cfg(feature = "contract")] - /// Deploy a smart contract to a node. + /// Deploy a smart contract. #[clap(alias = "c")] Contract(contract::UpContractCommand), } diff --git a/crates/pop-contracts/src/build.rs b/crates/pop-contracts/src/build.rs index 1bd7bec3..bfbe8e8b 100644 --- a/crates/pop-contracts/src/build.rs +++ b/crates/pop-contracts/src/build.rs @@ -1,7 +1,8 @@ // SPDX-License-Identifier: GPL-3.0 use crate::{errors::Error, utils::helpers::get_manifest_path}; -use contract_build::{execute, BuildMode, ExecuteArgs}; +pub use contract_build::Verbosity; +use contract_build::{execute, BuildMode, BuildResult, ExecuteArgs}; use std::path::Path; /// Build the smart contract located at the specified `path` in `build_release` mode. @@ -9,21 +10,24 @@ use std::path::Path; /// # Arguments /// * `path` - The optional path to the smart contract manifest, defaulting to the current directory if not specified. /// * `release` - Whether the smart contract should be built without any debugging functionality. -pub fn build_smart_contract(path: Option<&Path>, release: bool) -> anyhow::Result { +/// * `verbosity` - The build output verbosity. +pub fn build_smart_contract( + path: Option<&Path>, + release: bool, + verbosity: Verbosity, +) -> anyhow::Result { let manifest_path = get_manifest_path(path)?; let build_mode = match release { true => BuildMode::Release, false => BuildMode::Debug, }; + // Default values - let args = ExecuteArgs { manifest_path, build_mode, ..Default::default() }; + let args = ExecuteArgs { manifest_path, build_mode, verbosity, ..Default::default() }; // Execute the build and log the output of the build - let result = execute(args)?; - let formatted_result = result.display(); - - Ok(formatted_result) + Ok(execute(args)?) } /// Determines whether the manifest at the supplied path is a supported smart contract project. diff --git a/crates/pop-contracts/src/lib.rs b/crates/pop-contracts/src/lib.rs index 9d73a2c7..3b38309f 100644 --- a/crates/pop-contracts/src/lib.rs +++ b/crates/pop-contracts/src/lib.rs @@ -10,7 +10,7 @@ mod test; mod up; mod utils; -pub use build::{build_smart_contract, is_supported}; +pub use build::{build_smart_contract, is_supported, Verbosity}; pub use call::{ call_smart_contract, dry_run_call, dry_run_gas_estimate_call, set_up_call, CallOpts, }; diff --git a/crates/pop-contracts/src/up.rs b/crates/pop-contracts/src/up.rs index a004bdd3..1478dea2 100644 --- a/crates/pop-contracts/src/up.rs +++ b/crates/pop-contracts/src/up.rs @@ -337,7 +337,7 @@ mod tests { mock_build_process(temp_dir.path().join("testing"))?; // Run contracts-node let cache = temp_dir.path().join("cache"); - let mut process = run_contracts_node(cache).await?; + let mut process = run_contracts_node(cache, None).await?; let upload_exec = set_up_upload(UpOpts { path: Some(temp_dir.path().join("testing")), diff --git a/crates/pop-contracts/src/utils/contracts_node.rs b/crates/pop-contracts/src/utils/contracts_node.rs index 7dc4ee66..3b8d4982 100644 --- a/crates/pop-contracts/src/utils/contracts_node.rs +++ b/crates/pop-contracts/src/utils/contracts_node.rs @@ -4,10 +4,10 @@ use flate2::read::GzDecoder; use pop_common::GitHub; use std::{ env::consts::OS, - fs, + fs::{self, File}, io::{Seek, SeekFrom, Write}, path::PathBuf, - process::{Child, Command}, + process::{Child, Command, Stdio}, time::Duration, }; use tar::Archive; @@ -39,13 +39,14 @@ pub async fn is_chain_alive(url: url::Url) -> Result { } } -/// Runs the latest version of the `substracte-contracts-node` in the background. +/// Runs the latest version of the `substrate-contracts-node` in the background. /// /// # Arguments /// /// * `cache` - The path where the binary will be stored. +/// * `output` - The optional log file for node output. /// -pub async fn run_contracts_node(cache: PathBuf) -> Result { +pub async fn run_contracts_node(cache: PathBuf, output: Option<&File>) -> Result { let cached_file = cache.join(BIN_NAME); if !cached_file.exists() { let archive = archive_name_by_target()?; @@ -67,7 +68,13 @@ pub async fn run_contracts_node(cache: PathBuf) -> Result { fs::copy(&extracted_dir.join(BIN_NAME), &cached_file)?; fs::remove_dir_all(&extracted_dir.parent().unwrap_or(&cache.join("artifacts")))?; } - let process = Command::new(cached_file.display().to_string().as_str()).spawn()?; + let mut command = Command::new(cached_file.display().to_string().as_str()); + if let Some(output) = output { + command.stdout(Stdio::from(output.try_clone()?)); + command.stderr(Stdio::from(output.try_clone()?)); + } + + let process = command.spawn()?; // Wait 5 secs until the node is ready sleep(Duration::from_millis(5000)).await; @@ -160,7 +167,7 @@ mod tests { // Run the contracts node let temp_dir = tempfile::tempdir().expect("Could not create temp dir"); let cache = temp_dir.path().join("cache"); - let mut process = run_contracts_node(cache.clone()).await?; + let mut process = run_contracts_node(cache.clone(), None).await?; // Check if the node is alive assert!(is_chain_alive(local_url).await?); assert!(cache.join("substrate-contracts-node").exists()); diff --git a/crates/pop-contracts/tests/contract.rs b/crates/pop-contracts/tests/contract.rs index 73cf6e5d..a4919240 100644 --- a/crates/pop-contracts/tests/contract.rs +++ b/crates/pop-contracts/tests/contract.rs @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0 use anyhow::{Error, Result}; +use contract_build::{BuildMode, Verbosity}; use pop_contracts::{ build_smart_contract, create_smart_contract, dry_run_gas_estimate_instantiate, set_up_deployment, Contract, UpOpts, @@ -9,7 +10,7 @@ use std::fs; use tempfile::TempDir; use url::Url; -fn setup_test_environment() -> std::result::Result { +fn setup_test_environment() -> std::result::Result { let temp_dir = tempfile::tempdir().expect("Could not create temp dir"); let temp_contract_dir = temp_dir.path().join("test_contract"); fs::create_dir(&temp_contract_dir)?; @@ -41,17 +42,23 @@ fn test_contract_build() -> std::result::Result<(), Error> { // Test building in release mode let temp_contract_dir = setup_test_environment()?; - let formatted_result = - build_smart_contract(Some(&temp_contract_dir.path().join("test_contract")), true)?; - assert!(formatted_result.contains("RELEASE")); + let build_result = build_smart_contract( + Some(&temp_contract_dir.path().join("test_contract")), + true, + Verbosity::Default, + )?; + assert_eq!(build_result.build_mode, BuildMode::Release); verify_build_files(temp_contract_dir)?; let temp_debug_contract_dir = setup_test_environment()?; // Test building in debug mode - let formatted_result_debug_mode = - build_smart_contract(Some(&temp_debug_contract_dir.path().join("test_contract")), false)?; - assert!(formatted_result_debug_mode.contains("DEBUG")); + let build_result = build_smart_contract( + Some(&temp_debug_contract_dir.path().join("test_contract")), + false, + Verbosity::Default, + )?; + assert_eq!(build_result.build_mode, BuildMode::Debug); verify_build_files(temp_debug_contract_dir)?; @@ -61,7 +68,7 @@ fn test_contract_build() -> std::result::Result<(), Error> { const CONTRACTS_NETWORK_URL: &str = "wss://rococo-contracts-rpc.polkadot.io"; fn build_smart_contract_test_environment(temp_dir: &TempDir) -> Result<(), Error> { - build_smart_contract(Some(&temp_dir.path().join("test_contract")), true)?; + build_smart_contract(Some(&temp_dir.path().join("test_contract")), true, Verbosity::Default)?; Ok(()) }