Merge branch 'murisi/cli-public-keys' (#2209)

* origin/murisi/cli-public-keys:
  Added changelog entry.
  Fixed sign_raw in the case that the public keys map is non-consecutive.
  Now compress transactions before sending them to hardware wallet.
  Made build_unjail_validator respect force flag.
  Made the key not found error more informative by embedding alias inside. Corrected extend_from_pre_genesis_validator to also extend public keys.
  Removed the verification key flag since it has been made redundant by the signing keys argument.
  The flags for TxInitValidator and ConsensusKeyChange are now public keys.
  Make the CLI take public keys instead of secret keys so that hardware wallet signing is not precluded.

.changelog/unreleased/improvements/2209-cli-public-keys.md

@@ -0,0 +1,2 @@
+- Increase hardware wallet support in the CLI
+  ([\#2209](https://github.com/anoma/namada/pull/2209))

apps/src/lib/cli.rs

@@ -2985,7 +2985,7 @@ pub mod args {
         arg_opt("gas-spending-key");
     pub const FEE_AMOUNT_OPT: ArgOpt<token::DenominatedAmount> =
         arg_opt("gas-price");
-    pub const FEE_PAYER_OPT: ArgOpt<WalletKeypair> = arg_opt("gas-payer");
+    pub const FEE_PAYER_OPT: ArgOpt<WalletPublicKey> = arg_opt("gas-payer");
     pub const FORCE: ArgFlag = flag("force");
     pub const GAS_LIMIT: ArgDefault<GasLimit> =
         arg_default("gas-limit", DefaultFn(|| GasLimit::from(25_000)));
@@ -3070,9 +3070,8 @@ pub mod args {
         arg("self-bond-amount");
     pub const SENDER: Arg<String> = arg("sender");
     pub const SIGNER: ArgOpt<WalletAddress> = arg_opt("signer");
-    pub const SIGNING_KEY_OPT: ArgOpt<WalletKeypair> = SIGNING_KEY.opt();
-    pub const SIGNING_KEY: Arg<WalletKeypair> = arg("signing-key");
-    pub const SIGNING_KEYS: ArgMulti<WalletKeypair> = arg_multi("signing-keys");
+    pub const SIGNING_KEYS: ArgMulti<WalletPublicKey> =
+        arg_multi("signing-keys");
     pub const SIGNATURES: ArgMulti<PathBuf> = arg_multi("signatures");
     pub const SOURCE: Arg<WalletAddress> = arg("source");
     pub const SOURCE_OPT: ArgOpt<WalletAddress> = SOURCE.opt();
@@ -3101,17 +3100,15 @@ pub mod args {
         arg_opt("account-key");
     pub const VALIDATOR_ACCOUNT_KEYS: ArgMulti<WalletPublicKey> =
         arg_multi("account-keys");
-    pub const VALIDATOR_CONSENSUS_KEY: ArgOpt<WalletKeypair> =
+    pub const VALIDATOR_CONSENSUS_KEY: ArgOpt<WalletPublicKey> =
         arg_opt("consensus-key");
     pub const VALIDATOR_CODE_PATH: ArgOpt<PathBuf> =
         arg_opt("validator-code-path");
-    pub const VALIDATOR_ETH_COLD_KEY: ArgOpt<WalletKeypair> =
+    pub const VALIDATOR_ETH_COLD_KEY: ArgOpt<WalletPublicKey> =
         arg_opt("eth-cold-key");
-    pub const VALIDATOR_ETH_HOT_KEY: ArgOpt<WalletKeypair> =
+    pub const VALIDATOR_ETH_HOT_KEY: ArgOpt<WalletPublicKey> =
         arg_opt("eth-hot-key");
     pub const VALUE: ArgOpt<String> = arg_opt("value");
-    pub const VERIFICATION_KEY: ArgOpt<WalletPublicKey> =
-        arg_opt("verification-key");
     pub const VIEWING_KEY: Arg<WalletViewingKey> = arg("key");
     pub const WALLET_ALIAS_FORCE: ArgFlag = flag("wallet-alias-force");
     pub const WASM_CHECKSUMS_PATH: Arg<PathBuf> = arg("wasm-checksums-path");
@@ -4077,13 +4074,9 @@ pub mod args {
                 tx,
                 scheme: self.scheme,
                 address: chain_ctx.get(&self.address),
-                consensus_key: self
-                    .consensus_key
-                    .map(|x| chain_ctx.get_cached(&x)),
-                eth_cold_key: self
-                    .eth_cold_key
-                    .map(|x| chain_ctx.get_cached(&x)),
-                eth_hot_key: self.eth_hot_key.map(|x| chain_ctx.get_cached(&x)),
+                consensus_key: self.consensus_key.map(|x| chain_ctx.get(&x)),
+                eth_cold_key: self.eth_cold_key.map(|x| chain_ctx.get(&x)),
+                eth_hot_key: self.eth_hot_key.map(|x| chain_ctx.get(&x)),
                 protocol_key: self.protocol_key.map(|x| chain_ctx.get(&x)),
                 commission_rate: self.commission_rate,
                 max_commission_rate_change: self.max_commission_rate_change,
@@ -4202,13 +4195,9 @@ pub mod args {
                     .map(|x| chain_ctx.get(x))
                     .collect(),
                 threshold: self.threshold,
-                consensus_key: self
-                    .consensus_key
-                    .map(|x| chain_ctx.get_cached(&x)),
-                eth_cold_key: self
-                    .eth_cold_key
-                    .map(|x| chain_ctx.get_cached(&x)),
-                eth_hot_key: self.eth_hot_key.map(|x| chain_ctx.get_cached(&x)),
+                consensus_key: self.consensus_key.map(|x| chain_ctx.get(&x)),
+                eth_cold_key: self.eth_cold_key.map(|x| chain_ctx.get(&x)),
+                eth_hot_key: self.eth_hot_key.map(|x| chain_ctx.get(&x)),
                 protocol_key: self.protocol_key.map(|x| chain_ctx.get(&x)),
                 commission_rate: self.commission_rate,
                 max_commission_rate_change: self.max_commission_rate_change,
@@ -5336,9 +5325,7 @@ pub mod args {
             ConsensusKeyChange::<SdkTypes> {
                 tx,
                 validator: chain_ctx.get(&self.validator),
-                consensus_key: self
-                    .consensus_key
-                    .map(|x| chain_ctx.get_cached(&x)),
+                consensus_key: self.consensus_key.map(|x| chain_ctx.get(&x)),
                 tx_code_path: self.tx_code_path.to_path_buf(),
             }
         }
@@ -5865,26 +5852,21 @@ pub mod args {
                 signing_keys: self
                     .signing_keys
                     .iter()
-                    .map(|key| ctx.get_cached(key))
+                    .map(|key| ctx.get(key))
                     .collect(),
                 signatures: self
                     .signatures
                     .iter()
                     .map(|path| std::fs::read(path).unwrap())
                     .collect(),
-                verification_key: self
-                    .verification_key
-                    .map(|public_key| ctx.get(&public_key)),
                 disposable_signing_key: self.disposable_signing_key,
                 tx_reveal_code_path: self.tx_reveal_code_path,
                 password: self.password,
                 expiration: self.expiration,
                 chain_id: self
                     .chain_id
                     .or_else(|| Some(ctx.config.ledger.chain_id.clone())),
-                wrapper_fee_payer: self
-                    .wrapper_fee_payer
-                    .map(|x| ctx.get_cached(&x)),
+                wrapper_fee_payer: self.wrapper_fee_payer.map(|x| ctx.get(&x)),
                 use_device: self.use_device,
             }
         }
@@ -5968,10 +5950,7 @@ pub mod args {
                          public key, public key hash or alias from your \
                          wallet.",
                     )
-                    .conflicts_with_all([
-                        SIGNATURES.name,
-                        VERIFICATION_KEY.name,
-                    ]),
+                    .conflicts_with_all([SIGNATURES.name]),
             )
             .arg(
                 SIGNATURES
@@ -5981,25 +5960,12 @@ pub mod args {
                          to be attached to a transaction. Requires to provide \
                          a gas payer.",
                     )
-                    .conflicts_with_all([
-                        SIGNING_KEYS.name,
-                        VERIFICATION_KEY.name,
-                    ])
+                    .conflicts_with_all([SIGNING_KEYS.name])
                     .requires(FEE_PAYER_OPT.name),
             )
             .arg(OUTPUT_FOLDER_PATH.def().help(
                 "The output folder path where the artifact will be stored.",
             ))
-            .arg(
-                VERIFICATION_KEY
-                    .def()
-                    .help(
-                        "Sign the transaction with the key for the given \
-                         public key, public key hash or alias from your \
-                         wallet.",
-                    )
-                    .conflicts_with_all([SIGNING_KEYS.name, SIGNATURES.name]),
-            )
             .arg(CHAIN_ID_OPT.def().help("The chain ID."))
             .arg(
                 FEE_PAYER_OPT
@@ -6036,7 +6002,6 @@ pub mod args {
             let disposable_signing_key = DISPOSABLE_SIGNING_KEY.parse(matches);
             let signing_keys = SIGNING_KEYS.parse(matches);
             let signatures = SIGNATURES.parse(matches);
-            let verification_key = VERIFICATION_KEY.parse(matches);
             let tx_reveal_code_path = PathBuf::from(TX_REVEAL_PK);
             let chain_id = CHAIN_ID_OPT.parse(matches);
             let password = None;
@@ -6060,7 +6025,6 @@ pub mod args {
                 disposable_signing_key,
                 signing_keys,
                 signatures,
-                verification_key,
                 tx_reveal_code_path,
                 password,
                 chain_id,

apps/src/lib/cli/wallet.rs

@@ -135,7 +135,7 @@ fn address_key_find(
                 Ok(spending_key) => {
                     display_line!(io, "Spending key: {}", spending_key)
                 }
-                Err(FindKeyError::KeyNotFound) => {}
+                Err(FindKeyError::KeyNotFound(_)) => {}
                 Err(err) => edisplay_line!(io, "{}", err),
             }
         }

apps/src/lib/client/tx.rs

@@ -134,9 +134,12 @@ pub async fn sign(
                             response_pubkey, pubkey,
                         )));
                     }
+                    // Remove unnecessary detail for Ledger signing
+                    let mut compressed_tx = tx.clone();
+                    compressed_tx.wallet_filter();
                     // Get the Ledger to sign using our obtained derivation path
                     let response = app
-                        .sign(&path, &tx.serialize_to_vec())
+                        .sign(&path, &compressed_tx.serialize_to_vec())
                         .await
                         .map_err(|err| error::Error::Other(err.to_string()))?;
                     // Sign the raw header if that is requested
@@ -383,8 +386,8 @@ pub async fn submit_change_consensus_key(
     let mut wallet = namada.wallet_mut().await;
     let consensus_key = consensus_key
         .map(|key| match key {
-            common::SecretKey::Ed25519(_) => key,
-            common::SecretKey::Secp256k1(_) => {
+            common::PublicKey::Ed25519(_) => key,
+            common::PublicKey::Secp256k1(_) => {
                 edisplay_line!(
                     namada.io(),
                     "Consensus key can only be ed25519"
@@ -406,14 +409,15 @@ pub async fn submit_change_consensus_key(
                 )
                 .expect("Key generation should not fail.")
                 .1
+                .ref_to()
         });
     // To avoid wallet deadlocks in following operations
     drop(wallet);
 
     // Check that the new consensus key is unique
     let consensus_keys = rpc::query_consensus_keys(namada.client()).await;
 
-    let new_ck = consensus_key.ref_to();
+    let new_ck = consensus_key;
     if consensus_keys.contains(&new_ck) {
         edisplay_line!(namada.io(), "Consensus key can only be ed25519");
         safe_exit(1)
@@ -601,8 +605,8 @@ pub async fn submit_become_validator(
     let mut wallet = namada.wallet_mut().await;
     let consensus_key = consensus_key
         .map(|key| match key {
-            common::SecretKey::Ed25519(_) => key,
-            common::SecretKey::Secp256k1(_) => {
+            common::PublicKey::Ed25519(_) => key,
+            common::PublicKey::Secp256k1(_) => {
                 edisplay_line!(
                     namada.io(),
                     "Consensus key can only be ed25519"
@@ -625,12 +629,13 @@ pub async fn submit_become_validator(
                 )
                 .expect("Key generation should not fail.")
                 .1
+                .ref_to()
         });
 
     let eth_cold_pk = eth_cold_key
         .map(|key| match key {
-            common::SecretKey::Secp256k1(_) => key.ref_to(),
-            common::SecretKey::Ed25519(_) => {
+            common::PublicKey::Secp256k1(_) => key,
+            common::PublicKey::Ed25519(_) => {
                 edisplay_line!(
                     namada.io(),
                     "Eth cold key can only be secp256k1"
@@ -658,8 +663,8 @@ pub async fn submit_become_validator(
 
     let eth_hot_pk = eth_hot_key
         .map(|key| match key {
-            common::SecretKey::Secp256k1(_) => key.ref_to(),
-            common::SecretKey::Ed25519(_) => {
+            common::PublicKey::Secp256k1(_) => key,
+            common::PublicKey::Ed25519(_) => {
                 edisplay_line!(
                     namada.io(),
                     "Eth hot key can only be secp256k1"
@@ -727,7 +732,7 @@ pub async fn submit_become_validator(
     let mut tx = Tx::new(chain_id, tx_args.expiration);
     let data = BecomeValidator {
         address: address.clone(),
-        consensus_key: consensus_key.ref_to(),
+        consensus_key: consensus_key.clone(),
         eth_cold_key: key::secp256k1::PublicKey::try_from_pk(&eth_cold_pk)
             .unwrap(),
         eth_hot_key: key::secp256k1::PublicKey::try_from_pk(&eth_hot_pk)
@@ -753,7 +758,7 @@ pub async fn submit_become_validator(
         });
     let mut all_pks: Vec<_> =
         account.public_keys_map.pk_to_idx.into_keys().collect();
-    all_pks.push(consensus_key.to_public());
+    all_pks.push(consensus_key.clone());
     all_pks.push(eth_cold_pk);
     all_pks.push(eth_hot_pk);
     all_pks.push(data.protocol_key.clone());
@@ -789,21 +794,21 @@ pub async fn submit_become_validator(
 
         if !tx_args.dry_run {
             // add validator address and keys to the wallet
-            namada
-                .wallet_mut()
-                .await
-                .add_validator_data(address.clone(), validator_keys);
-            namada
-                .wallet_mut()
-                .await
+            let mut wallet = namada.wallet_mut().await;
+            wallet.add_validator_data(address.clone(), validator_keys);
+            wallet
                 .save()
                 .unwrap_or_else(|err| edisplay_line!(namada.io(), "{}", err));
 
             let tendermint_home = config.ledger.cometbft_dir();
             tendermint_node::write_validator_key(
                 &tendermint_home,
-                &consensus_key,
+                &wallet
+                    .find_key_by_pk(&consensus_key, None)
+                    .expect("unable to find consensus key pair in the wallet"),
             );
+            // To avoid wallet deadlocks in following operations
+            drop(wallet);
             tendermint_node::write_validator_state(tendermint_home);
 
             // Write Namada config stuff or figure out how to do the above
@@ -1039,8 +1044,14 @@ where
         )
         .await?;
 
+        let mut wallet = namada.wallet_mut().await;
         let signed_offline_proposal = proposal.sign(
-            args.tx.signing_keys,
+            args.tx
+                .signing_keys
+                .iter()
+                .map(|pk| wallet.find_key_by_pk(pk, None))
+                .collect::<Result<_, _>>()
+                .expect("secret keys corresponding to public keys not found"),
             &signing_data.account_public_keys_map.unwrap(),
         );
         let output_file_path = signed_offline_proposal
@@ -1186,8 +1197,14 @@ where
             delegations,
         );
 
+        let mut wallet = namada.wallet_mut().await;
         let offline_signed_vote = offline_vote.sign(
-            args.tx.signing_keys,
+            args.tx
+                .signing_keys
+                .iter()
+                .map(|pk| wallet.find_key_by_pk(pk, None))
+                .collect::<Result<_, _>>()
+                .expect("secret keys corresponding to public keys not found"),
             &signing_data.account_public_keys_map.unwrap(),
         );
         let output_file_path = offline_signed_vote

apps/src/lib/config/genesis/transactions.rs

@@ -345,7 +345,7 @@ pub fn sign_delegation_bond_tx(
     };
     let source_key = match found_key {
         Ok(key) => key,
-        Err(FindKeyError::KeyNotFound) => {
+        Err(FindKeyError::KeyNotFound(_)) => {
             // If it's not in the wallet, it must be an established account
             // so we need to look-up its public key first
             let pk = established_accounts

apps/src/lib/wallet/mod.rs

@@ -223,12 +223,13 @@ where
 {
     maybe_pk
         .map(|pk| {
+            let pkh = PublicKeyHash::from(&pk);
             wallet
                 // TODO: optionally encrypt validator keys
-                .find_key_by_pkh(&PublicKeyHash::from(&pk), None)
+                .find_key_by_pkh(&pkh, None)
                 .ok()
                 .or_else(|| wallet.get_validator_data().map(extract_key))
-                .ok_or(FindKeyError::KeyNotFound)
+                .ok_or_else(|| FindKeyError::KeyNotFound(pkh.to_string()))
         })
         .transpose()
 }

core/src/proto/types.rs

@@ -507,7 +507,7 @@ impl Signature {
             // vector instead of a map
             assert!(
                 secret_keys.keys().cloned().eq(0..(secret_keys.len() as u8)),
-                "secret keys must be enumerateed when signer address is absent"
+                "secret keys must be enumerated when signer address is absent"
             );
             Signer::PubKeys(secret_keys.values().map(RefTo::ref_to).collect())
         };
@@ -1573,9 +1573,15 @@ impl Tx {
         let hashes = vec![self.raw_header_hash()];
         self.protocol_filter();
 
+        let secret_keys = if signer.is_some() {
+            account_public_keys_map.index_secret_keys(keypairs)
+        } else {
+            (0..).zip(keypairs.into_iter()).collect()
+        };
+
         self.add_section(Section::Signature(Signature::new(
             hashes,
-            account_public_keys_map.index_secret_keys(keypairs),
+            secret_keys,
             signer,
         )));
         self