refactor: review fixes and improvements

crates/pop-cli/src/commands/call/chain.rs

@@ -44,7 +44,13 @@ pub struct CallChainCommand {
 	#[arg(short, long)]
 	suri: Option<String>,
 	/// Use a browser extension wallet to sign the extrinsic.
-	#[arg(name = "use-wallet", short('w'), long, default_value = "false", conflicts_with = "suri")]
+	#[arg(
+		name = "use-wallet",
+		short = 'w',
+		long,
+		default_value = "false",
+		conflicts_with = "suri"
+	)]
 	use_wallet: bool,
 	/// SCALE encoded bytes representing the call data of the extrinsic.
 	#[arg(name = "call", short, long, conflicts_with_all = ["pallet", "function", "args"])]

crates/pop-cli/src/commands/call/contract.rs

@@ -60,7 +60,13 @@ pub struct CallContractCommand {
 	#[arg(short, long, default_value = DEFAULT_URI)]
 	suri: String,
 	/// Use a browser extension wallet to sign the extrinsic.
-	#[arg(name = "use-wallet", long, short('w'), default_value = "false", conflicts_with = "suri")]
+	#[arg(
+		name = "use-wallet",
+		long,
+		short = 'w',
+		default_value = "false",
+		conflicts_with = "suri"
+	)]
 	use_wallet: bool,
 	/// Submit an extrinsic for on-chain execution.
 	#[arg(short = 'x', long)]
@@ -394,7 +400,7 @@ impl CallContractCommand {
 		// Perform signing steps with wallet integration, skipping secure signing for query-only
 		// operations.
 		if self.use_wallet {
-			self.execute_with_secure_signing(call_exec, cli).await?;
+			self.execute_with_wallet(call_exec, cli).await?;
 			return self.finalize_execute_call(cli, prompt_to_repeat_call).await;
 		}
 		if self.dry_run {
@@ -476,7 +482,7 @@ impl CallContractCommand {
 	}
 
 	/// Execute the smart contract call using wallet integration.
-	async fn execute_with_secure_signing(
+	async fn execute_with_wallet(
 		&self,
 		call_exec: CallExec<DefaultConfig, DefaultEnvironment, Keypair>,
 		cli: &mut impl Cli,

crates/pop-cli/src/commands/up/contract.rs

@@ -66,7 +66,7 @@ pub struct UpContractCommand {
 	/// - with a password "//Alice///SECRET_PASSWORD"
 	#[clap(short, long, default_value = "//Alice")]
 	suri: String,
-	/// Use your browser wallet to sign a transaction.
+	/// Use a browser extension wallet to sign the extrinsic.
 	#[clap(
 		name = "use-wallet",
 		long,
@@ -194,14 +194,18 @@ impl UpContractCommand {
 				spinner.start("Uploading contract...");
 
 				if self.upload_only {
-					let result = upload_contract_signed(self.url.as_str(), payload).await;
-					if let Err(e) = result {
-						spinner.error(format!("An error occurred uploading your contract: {e}"));
-						terminate_node(process)?;
-						Cli.outro_cancel(FAILED)?;
-						return Ok(());
-					}
-					let upload_result = result.expect("Error check above.");
+					let upload_result = match upload_contract_signed(self.url.as_str(), payload)
+						.await
+					{
+						Err(e) => {
+							spinner
+								.error(format!("An error occurred uploading your contract: {e}"));
+							terminate_node(process)?;
+							Cli.outro_cancel(FAILED)?;
+							return Ok(());
+						},
+						Ok(result) => result,
+					};
 
 					match get_code_hash_from_event(&upload_result, hash) {
 						Ok(r) => {
@@ -213,15 +217,19 @@ impl UpContractCommand {
 						},
 					};
 				} else {
-					let result = instantiate_contract_signed(self.url.as_str(), payload).await;
-					if let Err(e) = result {
-						spinner.error(format!("An error occurred uploading your contract: {e}"));
-						terminate_node(process)?;
-						Cli.outro_cancel(FAILED)?;
-						return Ok(());
-					}
-
-					let contract_info = result.unwrap();
+					let contract_info =
+						match instantiate_contract_signed(self.url.as_str(), payload).await {
+							Err(e) => {
+								spinner.error(format!(
+									"An error occurred uploading your contract: {e}"
+								));
+								terminate_node(process)?;
+								Cli.outro_cancel(FAILED)?;
+								return Ok(());
+							},
+							Ok(result) => result,
+						};
+
 					let hash = contract_info.code_hash.map(|code_hash| format!("{:?}", code_hash));
 					display_contract_info(
 						&spinner,
@@ -344,7 +352,7 @@ impl UpContractCommand {
 
 	// get the call data and contract code hash
 	async fn get_contract_data(&self) -> anyhow::Result<(Vec<u8>, [u8; 32])> {
-		let contract_code = get_contract_code(self.path.as_ref()).await?;
+		let contract_code = get_contract_code(self.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?;
@@ -358,7 +366,7 @@ impl UpContractCommand {
 				// Frontend will do dry run and update call data.
 				Weight::zero()
 			};
-			let call_data = get_instantiate_payload(instantiate_exec, weight_limit).await?;
+			let call_data = get_instantiate_payload(instantiate_exec, weight_limit)?;
 			Ok((call_data, hash))
 		}
 	}
@@ -480,7 +488,6 @@ mod tests {
 	#[tokio::test]
 	async fn get_upload_and_instantiate_call_data_works() -> anyhow::Result<()> {
 		let (contracts_node_process, port, temp_dir) = start_test_environment().await?;
-		let localhost_url = format!("ws://127.0.0.1:{}", port);
 		sleep(Duration::from_secs(5)).await;
 
 		get_upload_call_data_works(port, temp_dir.path().join("testing")).await?;
@@ -525,7 +532,7 @@ mod tests {
 		assert!(!retrieved_call_data.is_empty());
 
 		// Craft encoded call data for an upload code call.
-		let contract_code = get_contract_code(up_contract_opts.path.as_ref()).await?;
+		let contract_code = get_contract_code(up_contract_opts.path.as_ref())?;
 		let storage_deposit_limit: Option<u128> = None;
 		let upload_code = contract_extrinsics::extrinsic_calls::UploadCode::new(
 			contract_code,
@@ -575,8 +582,7 @@ mod tests {
 		// Craft instantiate call data.
 		let weight = Weight::from_parts(200_000_000, 30_000);
 		let expected_call_data =
-			get_instantiate_payload(set_up_deployment(up_contract_opts.into()).await?, weight)
-				.await?;
+			get_instantiate_payload(set_up_deployment(up_contract_opts.into()).await?, weight)?;
 		// Retrieved call data matches the one crafted above.
 		assert_eq!(retrieved_call_data, expected_call_data);
 

crates/pop-cli/src/common/wallet.rs

@@ -22,13 +22,12 @@ pub async fn request_signature(call_data: Vec<u8>, rpc: String) -> anyhow::Resul
 	let transaction_data = TransactionData::new(rpc, call_data);
 	// Starts server with port 9090.
 	let mut wallet = WalletIntegrationManager::new(ui, transaction_data, Some(9090));
-	let url = wallet.server_url.clone();
-	log::step(format!("Wallet signing portal started at http://{url}."))?;
+	let url = format!("http://{}", &wallet.server_url);
+	log::step(format!("Wallet signing portal started at {url}."))?;
 
 	let spinner = spinner();
-	spinner.start(format!("Opening browser to http://{url}"));
-	let res = open::that(format!("http://{url}"));
-	if let Err(e) = res {
+	spinner.start(format!("Opening browser to {url}"));
+	if let Err(e) = open::that(url) {
 		spinner.error(format!("Failed to launch browser. Please open link manually. {e}"));
 	}
 
@@ -48,7 +47,7 @@ pub async fn request_signature(call_data: Vec<u8>, rpc: String) -> anyhow::Resul
 	}
 	spinner.stop("");
 
-	let signed_payload = wallet.state.lock().await.signed_payload.clone();
+	let signed_payload = wallet.state.lock().await.signed_payload.take();
 	Ok(signed_payload)
 }
 

crates/pop-cli/src/wallet_integration.rs

@@ -13,9 +13,9 @@ use tokio::{
 };
 use tower_http::{cors::Any, services::ServeDir};
 
-/// Make frontend sourcing more flexible by allowing a custom route
-/// to be defined.
+/// Make frontend sourcing more flexible by allowing a custom route to be defined.
 pub trait Frontend {
+	/// Serves the content via a [Router].
 	fn serve_content(&self) -> Router;
 }
 
@@ -28,13 +28,15 @@ pub struct TransactionData {
 }
 
 impl TransactionData {
+	/// Create a new transaction payload.
+	/// # Arguments
+	/// * `chain_rpc`: The RPC of the chain.
+	/// * `call_data`: the call data.
+	/// # Returns
+	/// The transaction payload to be sent to frontend for signing.
 	pub fn new(chain_rpc: String, call_data: Vec<u8>) -> Self {
 		Self { chain_rpc, call_data }
 	}
-	#[allow(dead_code)]
-	pub fn call_data(&self) -> Vec<u8> {
-		self.call_data.clone()
-	}
 }
 
 /// Shared state between routes. Serves two purposes:
@@ -80,6 +82,13 @@ impl WalletIntegrationManager {
 	}
 
 	/// Same as `new`, but allows specifying the address to bind to.
+	/// # Arguments
+	/// * `frontend`: A frontend with custom route to serve content.
+	/// * `payload`: Payload to be sent to the frontend for signing.
+	/// * `server_url`: The address to bind to.
+	///
+	/// # Returns
+	/// A `WalletIntegrationManager` instance, with access to the state and task handle for the
 	pub fn new_with_address<F: Frontend>(
 		frontend: F,
 		payload: TransactionData,
@@ -97,7 +106,7 @@ impl WalletIntegrationManager {
 		let payload = Arc::new(payload);
 
 		let cors = tower_http::cors::CorsLayer::new()
-			.allow_origin(server_url.parse::<HeaderValue>().unwrap())
+			.allow_origin(server_url.parse::<HeaderValue>().expect("invalid server url"))
 			.allow_methods(Any) // Allow any HTTP method
 			.allow_headers(Any); // Allow any headers (like 'Content-Type')
 
@@ -235,6 +244,9 @@ pub struct FrontendFromDir {
 }
 #[allow(dead_code)]
 impl FrontendFromDir {
+	/// A new static server.
+	/// # Arguments
+	/// * `content`: A directory path.
 	pub fn new(content: PathBuf) -> Self {
 		Self { content }
 	}
@@ -253,6 +265,9 @@ pub struct FrontendFromString {
 
 #[allow(dead_code)]
 impl FrontendFromString {
+	/// A new static server.
+	/// # Arguments
+	/// * `content`: A hard-coded HTML string
 	pub fn new(content: String) -> Self {
 		Self { content }
 	}

crates/pop-contracts/src/node/mod.rs → crates/pop-contracts/src/node.rs

@@ -23,6 +23,7 @@ use subxt::{dynamic::Value, SubstrateConfig};
 use tokio::time::sleep;
 
 const BIN_NAME: &str = "substrate-contracts-node";
+const STARTUP: Duration = Duration::from_millis(20_000);
 
 /// Checks if the specified node is alive and responsive.
 ///
@@ -132,8 +133,8 @@ pub async fn run_contracts_node(
 
 	let process = command.spawn()?;
 
-	// Wait 20 secs until the node is ready
-	sleep(Duration::from_millis(20000)).await;
+	// Wait until the node is ready
+	sleep(STARTUP).await;
 
 	let data = Value::from_bytes(subxt::utils::to_hex("initialize contracts node"));
 	let payload = subxt::dynamic::tx("System", "remark", [data].to_vec());