diff --git a/.changelog/unreleased/improvements/3681-client-tx-signature.md b/.changelog/unreleased/improvements/3681-client-tx-signature.md new file mode 100644 index 0000000000..63dc064d04 --- /dev/null +++ b/.changelog/unreleased/improvements/3681-client-tx-signature.md @@ -0,0 +1,2 @@ +- Improved the user experience of the secret key decryption process. + ([\#3681](https://github.com/anoma/namada/pull/3681)) \ No newline at end of file diff --git a/crates/apps_lib/src/cli/wallet.rs b/crates/apps_lib/src/cli/wallet.rs index ebdce5d35c..14a0eab8cb 100644 --- a/crates/apps_lib/src/cli/wallet.rs +++ b/crates/apps_lib/src/cli/wallet.rs @@ -114,7 +114,11 @@ fn shielded_keys_list( // A subset of viewing keys will have corresponding spending keys. // Print those too if they are available and requested. if let Some(spending_key) = spending_key_opt { - match spending_key.get::(decrypt, None) { + match spending_key.get::( + decrypt, + None, + Some(&alias), + ) { // Here the spending key is unencrypted or successfully // decrypted Ok(spending_key) => { @@ -1201,7 +1205,11 @@ fn transparent_keys_list( // A subset of public keys will have corresponding secret keys. // Print those too if they are available and requested. if let Some((stored_keypair, _pkh)) = stored_keypair { - match stored_keypair.get::(decrypt, None) { + match stored_keypair.get::( + decrypt, + None, + Some(&alias), + ) { Ok(keypair) => { if unsafe_show_secret { display_line!(io, diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index c640aec100..4d2c2897a9 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -1030,30 +1030,18 @@ where .await?; let mut wallet = namada.wallet_mut().await; - let secret_keys = &signing_data - .public_keys - .iter() - .filter_map(|public_key| { - if let Ok(secret_key) = - signing::find_key_by_pk(&mut wallet, &tx_args, public_key) - { - Some(secret_key) - } else { - edisplay_line!( - namada.io(), - "Couldn't find the secret key for {}. Skipping signature \ - generation.", - public_key - ); - None - } - }) - .collect::>(); + + let mut secret_keys = vec![]; + for public_key in &signing_data.public_keys { + let secret_key = + signing::find_key_by_pk(&mut wallet, &tx_args, public_key)?; + secret_keys.push(secret_key); + } if let Some(account_public_keys_map) = signing_data.account_public_keys_map { let signatures = tx.compute_section_signature( - secret_keys, + &secret_keys, &account_public_keys_map, Some(owner), ); diff --git a/crates/apps_lib/src/wallet/mod.rs b/crates/apps_lib/src/wallet/mod.rs index b841240c59..56b66d5879 100644 --- a/crates/apps_lib/src/wallet/mod.rs +++ b/crates/apps_lib/src/wallet/mod.rs @@ -3,6 +3,7 @@ pub mod pre_genesis; mod store; mod transport; +use std::borrow::Cow; use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::{env, fs}; @@ -43,7 +44,10 @@ impl FsWalletStorage for CliWalletUtils { impl WalletIo for CliWalletUtils { type Rng = OsRng; - fn read_password(confirm: bool) -> Zeroizing { + fn read_password( + confirm: bool, + target_key: Option<&str>, + ) -> Zeroizing { let pwd = match env::var("NAMADA_WALLET_PASSWORD_FILE") { Ok(path) => Zeroizing::new( fs::read_to_string(path) @@ -64,8 +68,16 @@ impl WalletIo for CliWalletUtils { ) } Err(_) => { - let prompt = "Enter your decryption password: "; - rpassword::read_password_from_tty(Some(prompt)) + let prompt = match target_key { + Some(target) => Cow::Owned(format!( + "Enter your decryption password for {}: ", + target + )), + None => { + Cow::Borrowed("Enter your decryption password: ") + } + }; + rpassword::read_password_from_tty(Some(&prompt)) .map(Zeroizing::new) .expect("Failed reading password from tty.") } @@ -282,7 +294,7 @@ pub fn read_and_confirm_encryption_password( println!("Warning: The keypair will NOT be encrypted."); None } else { - Some(CliWalletUtils::read_password(true)) + Some(CliWalletUtils::read_password(true, None)) } } diff --git a/crates/apps_lib/src/wallet/pre_genesis.rs b/crates/apps_lib/src/wallet/pre_genesis.rs index d383688831..34041b523c 100644 --- a/crates/apps_lib/src/wallet/pre_genesis.rs +++ b/crates/apps_lib/src/wallet/pre_genesis.rs @@ -73,21 +73,27 @@ pub fn load(store_dir: &Path) -> Result { let store = ValidatorStore::decode(store).map_err(ReadError::Decode)?; let password = if store.consensus_key.is_encrypted() { - Some(CliWalletUtils::read_password(false)) + Some(CliWalletUtils::read_password(false, Some("consensus key"))) } else { None }; - let consensus_key = store - .consensus_key - .get::(true, password.clone())?; - let eth_cold_key = store - .eth_cold_key - .get::(true, password.clone())?; + let consensus_key = store.consensus_key.get::( + true, + password.clone(), + Some("consensus key"), + )?; + let eth_cold_key = store.eth_cold_key.get::( + true, + password.clone(), + Some("eth cold key"), + )?; let eth_hot_key = store.validator_keys.eth_bridge_keypair.clone(); - let tendermint_node_key = store - .tendermint_node_key - .get::(true, password)?; + let tendermint_node_key = store.tendermint_node_key.get::( + true, + password, + Some("tendermint node key"), + )?; Ok(ValidatorWallet { store, diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index 5300cf34c7..9e487522cc 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -226,23 +226,16 @@ where if let Some(account_public_keys_map) = signing_data.account_public_keys_map { let mut wallet = wallet.write().await; - let signing_tx_keypairs = signing_data - .public_keys - .iter() - .filter_map(|public_key| { - if used_pubkeys.contains(public_key) { - None - } else { - match find_key_by_pk(&mut wallet, args, public_key) { - Ok(secret_key) => { - used_pubkeys.insert(public_key.clone()); - Some(secret_key) - } - Err(_) => None, - } - } - }) - .collect::>(); + let mut signing_tx_keypairs = vec![]; + + for public_key in &signing_data.public_keys { + if !used_pubkeys.contains(public_key) { + let secret_key = find_key_by_pk(&mut wallet, args, public_key)?; + used_pubkeys.insert(public_key.clone()); + signing_tx_keypairs.push(secret_key); + } + } + if !signing_tx_keypairs.is_empty() { tx.sign_raw( signing_tx_keypairs, @@ -269,8 +262,8 @@ where } } - // Then try signing the fee header with the software wallet otherwise use - // the fallback + // Then try signing the wrapper header (fee payer) with the software wallet + // otherwise use the fallback let key = { // Lock the wallet just long enough to extract a key from it without // interfering with the sign closure call diff --git a/crates/sdk/src/wallet/keys.rs b/crates/sdk/src/wallet/keys.rs index 17e0dc4077..4da7a1e584 100644 --- a/crates/sdk/src/wallet/keys.rs +++ b/crates/sdk/src/wallet/keys.rs @@ -306,12 +306,13 @@ where &self, decrypt: bool, password: Option>, + target_key: Option<&str>, ) -> Result { match self { StoredKeypair::Encrypted(encrypted_keypair) => { if decrypt { - let password = - password.unwrap_or_else(|| U::read_password(false)); + let password = password + .unwrap_or_else(|| U::read_password(false, target_key)); let key = encrypted_keypair.decrypt(password)?; Ok(key) } else { diff --git a/crates/sdk/src/wallet/mod.rs b/crates/sdk/src/wallet/mod.rs index a54930878e..e3d1c3f5a1 100644 --- a/crates/sdk/src/wallet/mod.rs +++ b/crates/sdk/src/wallet/mod.rs @@ -62,7 +62,10 @@ pub trait WalletIo: Sized + Clone { } /// Read the password for decryption from the file/env/stdin. - fn read_password(_confirm: bool) -> Zeroizing { + fn read_password( + _confirm: bool, + _target_key: Option<&str>, + ) -> Zeroizing { panic!("attempted to prompt for password in non-interactive mode"); } @@ -1017,11 +1020,29 @@ impl Wallet { { match stored_key { StoredKeypair::Encrypted(encrypted) => { - let password = - password.unwrap_or_else(|| U::read_password(false)); - let key = encrypted - .decrypt(password) - .map_err(FindKeyError::KeyDecryptionError)?; + let key = match password { + Some(pwd) => encrypted.decrypt(pwd), + None => { + // Two attempts to get the password right in interactive + // mode + let mut key_result = + Err(keys::DecryptionError::EmptyPassword); + for _ in 0..2 { + let pwd = U::read_password( + false, + Some(&alias.to_string()), + ); + key_result = encrypted.decrypt(pwd); + if key_result.is_ok() { + break; + } + } + + key_result + } + } + .map_err(FindKeyError::KeyDecryptionError)?; + decrypted_key_cache.insert(alias.clone(), key); decrypted_key_cache .get(&alias) diff --git a/crates/tests/src/e2e/wallet_tests.rs b/crates/tests/src/e2e/wallet_tests.rs index 9ac110008b..8435917202 100644 --- a/crates/tests/src/e2e/wallet_tests.rs +++ b/crates/tests/src/e2e/wallet_tests.rs @@ -67,7 +67,7 @@ fn wallet_encrypted_key_cmds() -> Result<()> { ))?; cmd.exp_string(" Public key hash:")?; cmd.exp_string(" Public key:")?; - cmd.exp_string("Enter your decryption password:")?; + cmd.exp_string("Enter your decryption password")?; cmd.send_line(password)?; // 3. key list diff --git a/wasm_for_tests/tx_fail/Cargo.toml b/wasm_for_tests/tx_fail/Cargo.toml index f64c125dee..8610f2301f 100644 --- a/wasm_for_tests/tx_fail/Cargo.toml +++ b/wasm_for_tests/tx_fail/Cargo.toml @@ -8,7 +8,6 @@ version.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html -# FIXME: also check these normal deps for all the wasm for tests [dependencies] namada_tx_prelude.workspace = true rlsf.workspace = true