Merge pull request #3681 from anoma/grarco/client-tx-signature

Improve secret key decryption UX

.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))

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::<CliWalletUtils>(decrypt, None) {
+                match spending_key.get::<CliWalletUtils>(
+                    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::<CliWalletUtils>(decrypt, None) {
+                match stored_keypair.get::<CliWalletUtils>(
+                    decrypt,
+                    None,
+                    Some(&alias),
+                ) {
                     Ok(keypair) => {
                         if unsafe_show_secret {
                             display_line!(io,

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::<Vec<common::SecretKey>>();
+
+    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),
         );

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<String> {
+    fn read_password(
+        confirm: bool,
+        target_key: Option<&str>,
+    ) -> Zeroizing<String> {
         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))
     }
 }
 

crates/apps_lib/src/wallet/pre_genesis.rs

@@ -73,21 +73,27 @@ pub fn load(store_dir: &Path) -> Result<ValidatorWallet, ReadError> {
     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::<CliWalletUtils>(true, password.clone())?;
-    let eth_cold_key = store
-        .eth_cold_key
-        .get::<CliWalletUtils>(true, password.clone())?;
+    let consensus_key = store.consensus_key.get::<CliWalletUtils>(
+        true,
+        password.clone(),
+        Some("consensus key"),
+    )?;
+    let eth_cold_key = store.eth_cold_key.get::<CliWalletUtils>(
+        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::<CliWalletUtils>(true, password)?;
+    let tendermint_node_key = store.tendermint_node_key.get::<CliWalletUtils>(
+        true,
+        password,
+        Some("tendermint node key"),
+    )?;
 
     Ok(ValidatorWallet {
         store,

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::<Vec<common::SecretKey>>();
+        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

crates/sdk/src/wallet/keys.rs

@@ -306,12 +306,13 @@ where
         &self,
         decrypt: bool,
         password: Option<Zeroizing<String>>,
+        target_key: Option<&str>,
     ) -> Result<T, DecryptionError> {
         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 {

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<String> {
+    fn read_password(
+        _confirm: bool,
+        _target_key: Option<&str>,
+    ) -> Zeroizing<String> {
         panic!("attempted to prompt for password in non-interactive mode");
     }
 
@@ -1017,11 +1020,29 @@ impl<U: WalletIo> Wallet<U> {
     {
         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)

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

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