From 8db5ebe1e81e86b92fb73504ea8e10d4e25dbec5 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Wed, 10 Dec 2025 20:35:24 -0500 Subject: [PATCH 01/15] feat(evm): add keystore signer for Foundry-compatible encrypted keystores Add evm::keystore signer that allows signing transactions using Foundry-compatible encrypted keystore files. The password is prompted interactively at runtime for security. Features: - Support keystore account name from default directory (~/.foundry/keystores) - Support custom keystore directory via keystore_path input - Support absolute paths to keystore files - Interactive password prompt via ProvideInputRequest action - Compatible with keystores created via `cast wallet import` Usage: ```hcl signer "deployer" "evm::keystore" { keystore_account = "my-deployer" keystore_path = "./keystores" // optional } ``` Add ProvideInput response handling in SignerInstance::check_activability to enable signers to receive user-provided input values. --- Cargo.lock | 3 + addons/evm/Cargo.toml | 4 +- addons/evm/src/codec/crypto.rs | 64 ++++ addons/evm/src/constants.rs | 7 + addons/evm/src/signers/keystore.rs | 370 +++++++++++++++++++++ addons/evm/src/signers/mod.rs | 9 +- crates/txtx-addon-kit/src/types/signers.rs | 13 +- 7 files changed, 466 insertions(+), 4 deletions(-) create mode 100644 addons/evm/src/signers/keystore.rs diff --git a/Cargo.lock b/Cargo.lock index 409bb7e20..34d8d78eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -980,6 +980,7 @@ dependencies = [ "async-trait", "coins-bip32 0.12.0", "coins-bip39 0.12.0", + "eth-keystore", "k256", "rand 0.8.5", "thiserror 2.0.17", @@ -15192,6 +15193,8 @@ dependencies = [ "alloy-rpc-types", "alloy-signer-local", "async-recursion", + "dirs 5.0.1", + "eth-keystore", "foundry-block-explorers", "foundry-compilers", "foundry-compilers-artifacts-solc", diff --git a/addons/evm/Cargo.toml b/addons/evm/Cargo.toml index 151d4ce24..3aede30b1 100644 --- a/addons/evm/Cargo.toml +++ b/addons/evm/Cargo.toml @@ -38,7 +38,9 @@ alloy-chains = "0.2.14" alloy-primitives = { version = "1.4.1" } alloy-provider = { version = "1.0.41", default-features = false, features = ["debug-api"] } alloy-rpc-types = { version = "1.0.41", features = ["trace"] } -alloy-signer-local = { version = "1.0.41", features = ["mnemonic"] } +alloy-signer-local = { version = "1.0.41", features = ["mnemonic", "keystore"] } +eth-keystore = "0.5.0" +dirs = "5.0" thiserror = "1.0.62" toml = "0.5" foundry-block-explorers = "0.22.0" diff --git a/addons/evm/src/codec/crypto.rs b/addons/evm/src/codec/crypto.rs index 1adacd69d..32208c531 100644 --- a/addons/evm/src/codec/crypto.rs +++ b/addons/evm/src/codec/crypto.rs @@ -49,6 +49,70 @@ pub fn field_bytes_to_secret_key_signer(field_bytes: &Vec) -> Result, +) -> Result { + use std::path::PathBuf; + + let keystore_dir = match keystore_path { + Some(path) => PathBuf::from(path), + None => { + let home = dirs::home_dir() + .ok_or_else(|| "could not determine home directory".to_string())?; + home.join(".foundry").join("keystores") + } + }; + + // If keystore_account is already an absolute path, use it directly + let account_path = PathBuf::from(keystore_account); + if account_path.is_absolute() { + return Ok(account_path); + } + + // Otherwise, join with the keystore directory + // Try with .json extension first, then without + let with_json = keystore_dir.join(format!("{}.json", keystore_account)); + if with_json.exists() { + return Ok(with_json); + } + + let without_json = keystore_dir.join(keystore_account); + if without_json.exists() { + return Ok(without_json); + } + + // Return the path with .json extension (will fail at decrypt time with better error) + Ok(with_json) +} + +/// Decrypts a keystore file and returns a SecretKeySigner +pub fn keystore_to_secret_key_signer( + keystore_path: &std::path::Path, + password: &str, +) -> Result { + if !keystore_path.exists() { + return Err(format!("keystore file not found: {:?}", keystore_path)); + } + + if keystore_path.is_dir() { + return Err(format!( + "keystore path is a directory, expected a file: {:?}", + keystore_path + )); + } + + let secret_key = eth_keystore::decrypt_key(keystore_path, password) + .map_err(|e| format!("failed to decrypt keystore: {}", e))?; + + let signing_key = SigningKey::from_slice(&secret_key) + .map_err(|e| format!("invalid key in keystore: {}", e))?; + + Ok(SecretKeySigner::from_signing_key(signing_key)) +} + pub fn public_key_to_address(public_key_bytes: &Vec) -> Result { let pubkey = VerifyingKey::from_sec1_bytes(&public_key_bytes) .map_err(|e| format!("invalid public key: {}", e))?; diff --git a/addons/evm/src/constants.rs b/addons/evm/src/constants.rs index c50b5a635..5b559ddcc 100644 --- a/addons/evm/src/constants.rs +++ b/addons/evm/src/constants.rs @@ -25,6 +25,12 @@ pub const PUBLIC_KEYS: &str = "public_keys"; pub const SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES: &str = "secret_key_wallet_unsigned_transaction_bytes"; pub const WEB_WALLET_UNSIGNED_TRANSACTION_BYTES: &str = "web_wallet_unsigned_transaction_bytes"; +pub const KEYSTORE_PASSWORD: &str = "keystore_password"; +pub const KEYSTORE_ACCOUNT: &str = "keystore_account"; +pub const KEYSTORE_PATH: &str = "keystore_path"; + +// Default keystore directory (Foundry default) +pub const DEFAULT_FOUNDRY_KEYSTORES_DIR: &str = ".foundry/keystores"; pub const TRANSACTION_PAYLOAD_BYTES: &str = "transaction_payload_bytes"; pub const SIGNED_MESSAGE_BYTES: &str = "signed_message_bytes"; pub const MESSAGE_BYTES: &str = "message_bytes"; @@ -96,6 +102,7 @@ pub const ACTION_ITEM_PROVIDE_PUBLIC_KEY: &str = "provide_public_key"; pub const ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION: &str = "provide_signed_transaction"; pub const ACTION_ITEM_SEND_TRANSACTION: &str = "send_transaction"; pub const ACTION_OPEN_MODAL: &str = "open_modal"; +pub const ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD: &str = "provide_keystore_password"; // Default contracts pub const DEFAULT_CREATE2_FACTORY_ADDRESS: &str = "0x4e59b44847b379578588920cA78FbF26c0B4956C"; diff --git a/addons/evm/src/signers/keystore.rs b/addons/evm/src/signers/keystore.rs new file mode 100644 index 000000000..38796bf89 --- /dev/null +++ b/addons/evm/src/signers/keystore.rs @@ -0,0 +1,370 @@ +use alloy::network::{EthereumWallet, TransactionBuilder}; +use alloy::primitives::Address; +use alloy_rpc_types::TransactionRequest; +use std::collections::HashMap; +use txtx_addon_kit::channel; +use txtx_addon_kit::constants::SIGNATURE_APPROVED; +use txtx_addon_kit::types::commands::CommandExecutionResult; +use txtx_addon_kit::types::frontend::{ + ActionItemRequestType, ActionItemStatus, ProvideInputRequest, + ProvideSignedTransactionRequest, ReviewInputRequest, +}; +use txtx_addon_kit::types::frontend::{Actions, BlockEvent}; +use txtx_addon_kit::types::signers::{ + return_synchronous_result, CheckSignabilityOk, SignerActionErr, SignerActionsFutureResult, + SignerActivateFutureResult, SignerInstance, SignerSignFutureResult, +}; +use txtx_addon_kit::types::signers::{SignerImplementation, SignerSpecification}; +use txtx_addon_kit::types::stores::ValueStore; +use txtx_addon_kit::types::AuthorizationContext; +use txtx_addon_kit::types::{ + commands::CommandSpecification, + diagnostics::Diagnostic, + types::{Type, Value}, +}; +use txtx_addon_kit::types::{ + signers::SignersState, types::RunbookSupervisionContext, ConstructDid, +}; + +use crate::codec::crypto::{field_bytes_to_secret_key_signer, keystore_to_secret_key_signer, resolve_keystore_path}; +use crate::constants::{ + ACTION_ITEM_CHECK_ADDRESS, ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD, + ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION, CHAIN_ID, FORMATTED_TRANSACTION, + KEYSTORE_ACCOUNT, KEYSTORE_PASSWORD, KEYSTORE_PATH, NAMESPACE, RPC_API_URL, + SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, TX_HASH, +}; +use crate::rpc::EvmWalletRpc; +use crate::typing::EvmValue; +use txtx_addon_kit::types::signers::return_synchronous_actions; + +use crate::constants::PUBLIC_KEYS; + +lazy_static! { + pub static ref EVM_KEYSTORE_SIGNER: SignerSpecification = define_signer! { + EvmKeystoreSigner => { + name: "EVM Keystore Signer", + matcher: "keystore", + documentation: txtx_addon_kit::indoc! {r#" +The `evm::keystore` signer uses a Foundry-compatible encrypted keystore file to sign transactions. +The keystore password will be prompted at runtime for security. + +### Inputs + +| Name | Type | Required | Description | +|--------------------|--------|----------|-----------------------------------------------------------------------------| +| `keystore_account` | string | Yes | The account name (filename without .json) or full path to the keystore file | +| `keystore_path` | string | No | Directory containing keystores. Defaults to `~/.foundry/keystores` | + +### Outputs + +| Name | Type | Description | +|-----------|--------|------------------------------------------| +| `address` | string | The Ethereum address derived from the keystore | + +### Security + +- The keystore password is never stored in manifests +- Password is prompted interactively at runtime +- Compatible with keystores created by `cast wallet import` +"#}, + inputs: [ + keystore_account: { + documentation: "The account name (filename without .json) in the keystores directory, or a full path to the keystore file.", + typing: Type::string(), + optional: false, + tainting: true, + sensitive: false + }, + keystore_path: { + documentation: "The directory containing keystore files. Defaults to ~/.foundry/keystores", + typing: Type::string(), + optional: true, + tainting: true, + sensitive: false + } + ], + outputs: [ + address: { + documentation: "The address derived from the keystore.", + typing: Type::string() + } + ], + example: txtx_addon_kit::indoc! {r#" + // Use a keystore from the default Foundry keystores directory + signer "deployer" "evm::keystore" { + keystore_account = "my-deployer-account" + } + + // Use a keystore from a custom directory + signer "deployer" "evm::keystore" { + keystore_account = "deployer" + keystore_path = "./keystores" + } + + // Use a full path to a keystore file + signer "deployer" "evm::keystore" { + keystore_account = "/path/to/my-keystore.json" + } + "#} + } + }; +} + +pub struct EvmKeystoreSigner; + +impl SignerImplementation for EvmKeystoreSigner { + fn check_instantiability( + _ctx: &SignerSpecification, + _args: Vec, + ) -> Result { + unimplemented!() + } + + #[cfg(not(feature = "wasm"))] + fn check_activability( + construct_did: &ConstructDid, + instance_name: &str, + _spec: &SignerSpecification, + values: &ValueStore, + mut signer_state: ValueStore, + signers: SignersState, + _signers_instances: &HashMap, + supervision_context: &RunbookSupervisionContext, + auth_ctx: &txtx_addon_kit::types::AuthorizationContext, + _is_balance_check_required: bool, + _is_public_key_required: bool, + ) -> SignerActionsFutureResult { + use txtx_addon_kit::constants::DESCRIPTION; + use crate::constants::CHECKED_ADDRESS; + + let mut actions = Actions::none(); + + // If we already have the signer set up, we're done + if signer_state.get_value(PUBLIC_KEYS).is_some() { + return return_synchronous_actions(Ok((signers, signer_state, actions))); + } + + let description = values.get_string(DESCRIPTION).map(|d| d.to_string()); + let markdown = values + .get_markdown(auth_ctx) + .map_err(|d| (signers.clone(), signer_state.clone(), d))?; + + // Get keystore configuration + let keystore_account = values + .get_expected_string(KEYSTORE_ACCOUNT) + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + let keystore_path = values.get_string(KEYSTORE_PATH); + + // Resolve the keystore file path + let resolved_path = resolve_keystore_path(keystore_account, keystore_path) + .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; + + // Store the resolved path for later use + signer_state.insert( + "resolved_keystore_path", + Value::string(resolved_path.to_string_lossy().to_string()), + ); + + // Check if we have the password yet (from a previous response) + let password = signer_state.get_string(KEYSTORE_PASSWORD); + + if let Some(password) = password { + // We have the password, decrypt the keystore + let signer = keystore_to_secret_key_signer(&resolved_path, password) + .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; + + let expected_address: Address = signer.address(); + + // Store the signer field bytes for later signing + signer_state.insert( + "signer_field_bytes", + EvmValue::signer_field_bytes(signer.to_field_bytes().to_vec()), + ); + signer_state.insert("signer_address", Value::string(expected_address.to_string())); + + if supervision_context.review_input_values { + if signer_state.get_expected_string(CHECKED_ADDRESS).is_ok() { + signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); + } else { + // Ask user to review/confirm the address + actions.push_sub_group( + None, + vec![ReviewInputRequest::new("", &Value::string(expected_address.to_string())) + .to_action_type() + .to_request(instance_name, ACTION_ITEM_CHECK_ADDRESS) + .with_construct_did(construct_did) + .with_some_description(description.clone()) + .with_meta_description(&format!("Check {} expected address", instance_name)) + .with_some_markdown(markdown.clone())], + ); + } + } else { + signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); + } + } else { + // No password yet - prompt for it + let keystore_display = resolved_path + .file_name() + .map(|f| f.to_string_lossy().to_string()) + .unwrap_or_else(|| keystore_account.to_string()); + + let request = ProvideInputRequest { + default_value: None, + input_name: KEYSTORE_PASSWORD.to_string(), + typing: Type::string(), + }; + + actions.push_sub_group( + Some(format!("Enter password for keystore '{}'", keystore_display)), + vec![ActionItemRequestType::ProvideInput(request) + .to_request(instance_name, ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD) + .with_construct_did(construct_did) + .with_some_description(description.clone()) + .with_meta_description(&format!("Unlock keystore for {}", instance_name)) + .with_some_markdown(markdown.clone()) + .with_status(ActionItemStatus::Todo)], + ); + } + + let future = async move { Ok((signers, signer_state, actions)) }; + Ok(Box::pin(future)) + } + + fn activate( + _construct_id: &ConstructDid, + _spec: &SignerSpecification, + _values: &ValueStore, + signer_state: ValueStore, + signers: SignersState, + _signers_instances: &HashMap, + _progress_tx: &channel::Sender, + ) -> SignerActivateFutureResult { + let mut result = CommandExecutionResult::new(); + let address = signer_state + .get_expected_value("signer_address") + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + result.outputs.insert("address".into(), address.clone()); + return_synchronous_result(Ok((signers, signer_state, result))) + } + + fn check_signability( + construct_did: &ConstructDid, + title: &str, + description: &Option, + meta_description: &Option, + markdown: &Option, + payload: &Value, + _spec: &SignerSpecification, + values: &ValueStore, + signer_state: ValueStore, + signers: SignersState, + _signers_instances: &HashMap, + supervision_context: &RunbookSupervisionContext, + _auth_ctx: &AuthorizationContext, + ) -> Result { + let actions = if supervision_context.review_input_values { + let construct_did_str = &construct_did.to_string(); + if signer_state.get_scoped_value(construct_did_str, SIGNATURE_APPROVED).is_some() { + return Ok((signers, signer_state, Actions::none())); + } + + let chain_id = values + .get_expected_uint(CHAIN_ID) + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + + let status = ActionItemStatus::Todo; + + let formatted_payload = + signer_state.get_scoped_value(construct_did_str, FORMATTED_TRANSACTION); + + let request = ProvideSignedTransactionRequest::new( + &signer_state.uuid, + payload, + NAMESPACE, + &chain_id.to_string(), + ) + .check_expectation_action_uuid(construct_did) + .only_approval_needed() + .formatted_payload(formatted_payload) + .to_action_type() + .to_request(title, ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION) + .with_construct_did(construct_did) + .with_some_description(description.clone()) + .with_some_meta_description(meta_description.clone()) + .with_some_markdown(markdown.clone()) + .with_status(status); + + Actions::append_item( + request, + Some("Review and sign the transactions from the list below"), + Some("Transaction Signing"), + ) + } else { + Actions::none() + }; + Ok((signers, signer_state, actions)) + } + + fn sign( + caller_uuid: &ConstructDid, + _title: &str, + _payload: &Value, + _spec: &SignerSpecification, + values: &ValueStore, + signer_state: ValueStore, + signers: SignersState, + _signers_instances: &HashMap, + ) -> SignerSignFutureResult { + let caller_uuid = caller_uuid.clone(); + let values = values.clone(); + + let future = async move { + let mut result = CommandExecutionResult::new(); + + let rpc_api_url = values + .get_expected_string(RPC_API_URL) + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + + let signer_field_bytes = signer_state + .get_expected_buffer_bytes("signer_field_bytes") + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + + let payload_bytes = signer_state + .get_expected_scoped_buffer_bytes( + &caller_uuid.to_string(), + SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, + ) + .unwrap(); + + let secret_key_signer = field_bytes_to_secret_key_signer(&signer_field_bytes) + .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; + + let eth_signer = EthereumWallet::from(secret_key_signer); + + let mut tx: TransactionRequest = serde_json::from_slice(&payload_bytes).unwrap(); + if tx.to.is_none() { + tx.set_create(); + } + + let tx_envelope = tx.build(ð_signer).await.map_err(|e| { + ( + signers.clone(), + signer_state.clone(), + diagnosed_error!("failed to build transaction envelope: {e}"), + ) + })?; + + let rpc = EvmWalletRpc::new(&rpc_api_url, eth_signer) + .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; + + let tx_hash = rpc.sign_and_send_tx(tx_envelope).await.map_err(|e| { + (signers.clone(), signer_state.clone(), diagnosed_error!("{}", e.to_string())) + })?; + + result.outputs.insert(TX_HASH.to_string(), EvmValue::tx_hash(tx_hash.to_vec())); + + Ok((signers, signer_state, result)) + }; + Ok(Box::pin(future)) + } +} diff --git a/addons/evm/src/signers/mod.rs b/addons/evm/src/signers/mod.rs index e4b3e11f9..73d1f6441 100644 --- a/addons/evm/src/signers/mod.rs +++ b/addons/evm/src/signers/mod.rs @@ -1,13 +1,18 @@ use txtx_addon_kit::types::signers::SignerSpecification; pub mod common; +mod keystore; mod secret_key; mod web_wallet; +use keystore::EVM_KEYSTORE_SIGNER; use secret_key::EVM_SECRET_KEY_SIGNER; use web_wallet::EVM_WEB_WALLET; lazy_static! { - pub static ref WALLETS: Vec = - vec![EVM_SECRET_KEY_SIGNER.clone(), EVM_WEB_WALLET.clone()]; + pub static ref WALLETS: Vec = vec![ + EVM_SECRET_KEY_SIGNER.clone(), + EVM_KEYSTORE_SIGNER.clone(), + EVM_WEB_WALLET.clone(), + ]; } diff --git a/crates/txtx-addon-kit/src/types/signers.rs b/crates/txtx-addon-kit/src/types/signers.rs index 287800b5f..fbf4d4aff 100644 --- a/crates/txtx-addon-kit/src/types/signers.rs +++ b/crates/txtx-addon-kit/src/types/signers.rs @@ -417,7 +417,18 @@ impl SignerInstance { } } } - + ActionItemResponseType::ProvideInput(response) => { + let Some(requests) = action_item_requests else { continue }; + let Some(request) = requests.iter().find(|r| r.id.eq(&action_item_id)) + else { + continue; + }; + let Some(signer_did) = &request.construct_did else { continue }; + + let mut signer_state = signers.pop_signer_state(signer_did).unwrap(); + signer_state.insert(&response.input_name, response.updated_value.clone()); + signers.push_signer_state(signer_state); + } _ => {} } } From 07eb2b414a2253818e0c04efafd49c730c6d1df9 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Wed, 10 Dec 2025 20:45:38 -0500 Subject: [PATCH 02/15] refactor(evm): extract shared signer logic to common.rs Extract activate_signer, check_signability, and sign_transaction functions to common.rs to eliminate code duplication between secret_key.rs and keystore.rs signers. Remove unused imports. --- addons/evm/src/signers/common.rs | 146 +++++++++++++++++++++- addons/evm/src/signers/keystore.rs | 167 +++++-------------------- addons/evm/src/signers/secret_key.rs | 178 ++++++--------------------- 3 files changed, 206 insertions(+), 285 deletions(-) diff --git a/addons/evm/src/signers/common.rs b/addons/evm/src/signers/common.rs index 6d2ee4910..dab8bbfd5 100644 --- a/addons/evm/src/signers/common.rs +++ b/addons/evm/src/signers/common.rs @@ -1,23 +1,31 @@ +use alloy::network::{EthereumWallet, TransactionBuilder}; use alloy::primitives::{utils::format_units, Address}; +use alloy_rpc_types::TransactionRequest; use txtx_addon_kit::{ + constants::SIGNATURE_APPROVED, indexmap::IndexMap, types::{ + commands::CommandExecutionResult, frontend::{ - ActionItemRequest, ActionItemRequestType, ActionItemStatus, ProvidePublicKeyRequest, - ReviewInputRequest, + ActionItemRequest, ActionItemRequestType, ActionItemStatus, Actions, + ProvidePublicKeyRequest, ProvideSignedTransactionRequest, ReviewInputRequest, }, + signers::{SignerActionErr, SignersState}, stores::ValueStore, - types::Value, + types::{RunbookSupervisionContext, Value}, ConstructDid, }, }; use crate::{ + codec::crypto::field_bytes_to_secret_key_signer, constants::{ ACTION_ITEM_CHECK_ADDRESS, ACTION_ITEM_CHECK_BALANCE, ACTION_ITEM_PROVIDE_PUBLIC_KEY, - DEFAULT_MESSAGE, NAMESPACE, + ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION, CHAIN_ID, DEFAULT_MESSAGE, FORMATTED_TRANSACTION, + NAMESPACE, RPC_API_URL, SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, TX_HASH, }, - rpc::EvmRpc, + rpc::{EvmRpc, EvmWalletRpc}, + typing::EvmValue, }; pub async fn get_additional_actions_for_address( @@ -160,3 +168,131 @@ impl NonceManager { Ok(NonceManager(IndexMap::new())) } } + +/// Shared activate implementation for secret_key and keystore signers +pub fn activate_signer( + signer_state: ValueStore, + signers: SignersState, +) -> Result<(SignersState, ValueStore, CommandExecutionResult), (SignersState, ValueStore, txtx_addon_kit::types::diagnostics::Diagnostic)> { + let mut result = CommandExecutionResult::new(); + let address = signer_state + .get_expected_value("signer_address") + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + result.outputs.insert("address".into(), address.clone()); + Ok((signers, signer_state, result)) +} + +/// Shared check_signability implementation for secret_key and keystore signers +pub fn check_signability( + construct_did: &ConstructDid, + title: &str, + description: &Option, + meta_description: &Option, + markdown: &Option, + payload: &Value, + values: &ValueStore, + signer_state: ValueStore, + signers: SignersState, + supervision_context: &RunbookSupervisionContext, +) -> Result<(SignersState, ValueStore, Actions), SignerActionErr> { + let actions = if supervision_context.review_input_values { + let construct_did_str = construct_did.to_string(); + if signer_state.get_scoped_value(&construct_did_str, SIGNATURE_APPROVED).is_some() { + return Ok((signers, signer_state, Actions::none())); + } + + let chain_id = values + .get_expected_uint(CHAIN_ID) + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + + let formatted_payload = + signer_state.get_scoped_value(&construct_did_str, FORMATTED_TRANSACTION); + + let request = ProvideSignedTransactionRequest::new( + &signer_state.uuid, + payload, + NAMESPACE, + &chain_id.to_string(), + ) + .check_expectation_action_uuid(construct_did) + .only_approval_needed() + .formatted_payload(formatted_payload) + .to_action_type() + .to_request(title, ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION) + .with_construct_did(construct_did) + .with_some_description(description.clone()) + .with_some_meta_description(meta_description.clone()) + .with_some_markdown(markdown.clone()) + .with_status(ActionItemStatus::Todo); + + Actions::append_item( + request, + Some("Review and sign the transactions from the list below"), + Some("Transaction Signing"), + ) + } else { + Actions::none() + }; + Ok((signers, signer_state, actions)) +} + +/// Shared sign implementation for secret_key and keystore signers +pub async fn sign_transaction( + caller_uuid: &ConstructDid, + values: &ValueStore, + signer_state: ValueStore, + signers: SignersState, +) -> Result<(SignersState, ValueStore, CommandExecutionResult), (SignersState, ValueStore, txtx_addon_kit::types::diagnostics::Diagnostic)> { + let mut result = CommandExecutionResult::new(); + + let rpc_api_url = values + .get_expected_string(RPC_API_URL) + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + + let signer_field_bytes = signer_state + .get_expected_buffer_bytes("signer_field_bytes") + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + + let payload_bytes = signer_state + .get_expected_scoped_buffer_bytes( + &caller_uuid.to_string(), + SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, + ) + .map_err(|e| (signers.clone(), signer_state.clone(), e))?; + + let secret_key_signer = field_bytes_to_secret_key_signer(&signer_field_bytes) + .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; + + let eth_signer = EthereumWallet::from(secret_key_signer); + + let mut tx: TransactionRequest = serde_json::from_slice(&payload_bytes).map_err(|e| { + ( + signers.clone(), + signer_state.clone(), + diagnosed_error!("failed to deserialize transaction: {e}"), + ) + })?; + + if tx.to.is_none() { + tx.set_create(); + } + + let tx_envelope = tx.build(ð_signer).await.map_err(|e| { + ( + signers.clone(), + signer_state.clone(), + diagnosed_error!("failed to build transaction envelope: {e}"), + ) + })?; + + let rpc = EvmWalletRpc::new(rpc_api_url, eth_signer) + .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; + + let tx_hash = rpc.sign_and_send_tx(tx_envelope).await.map_err(|e| { + (signers.clone(), signer_state.clone(), diagnosed_error!("{}", e.to_string())) + })?; + + result.outputs.insert(TX_HASH.to_string(), EvmValue::tx_hash(tx_hash.to_vec())); + + Ok((signers, signer_state, result)) +} diff --git a/addons/evm/src/signers/keystore.rs b/addons/evm/src/signers/keystore.rs index 38796bf89..53717bb65 100644 --- a/addons/evm/src/signers/keystore.rs +++ b/addons/evm/src/signers/keystore.rs @@ -1,44 +1,30 @@ -use alloy::network::{EthereumWallet, TransactionBuilder}; use alloy::primitives::Address; -use alloy_rpc_types::TransactionRequest; use std::collections::HashMap; use txtx_addon_kit::channel; -use txtx_addon_kit::constants::SIGNATURE_APPROVED; -use txtx_addon_kit::types::commands::CommandExecutionResult; use txtx_addon_kit::types::frontend::{ - ActionItemRequestType, ActionItemStatus, ProvideInputRequest, - ProvideSignedTransactionRequest, ReviewInputRequest, + ActionItemRequestType, ActionItemStatus, Actions, BlockEvent, ProvideInputRequest, + ReviewInputRequest, }; -use txtx_addon_kit::types::frontend::{Actions, BlockEvent}; use txtx_addon_kit::types::signers::{ return_synchronous_result, CheckSignabilityOk, SignerActionErr, SignerActionsFutureResult, - SignerActivateFutureResult, SignerInstance, SignerSignFutureResult, + SignerActivateFutureResult, SignerImplementation, SignerInstance, SignerSignFutureResult, + SignerSpecification, SignersState, }; -use txtx_addon_kit::types::signers::{SignerImplementation, SignerSpecification}; use txtx_addon_kit::types::stores::ValueStore; -use txtx_addon_kit::types::AuthorizationContext; -use txtx_addon_kit::types::{ - commands::CommandSpecification, - diagnostics::Diagnostic, - types::{Type, Value}, -}; -use txtx_addon_kit::types::{ - signers::SignersState, types::RunbookSupervisionContext, ConstructDid, -}; +use txtx_addon_kit::types::types::{RunbookSupervisionContext, Type, Value}; +use txtx_addon_kit::types::commands::CommandSpecification; +use txtx_addon_kit::types::{diagnostics::Diagnostic, AuthorizationContext, ConstructDid}; -use crate::codec::crypto::{field_bytes_to_secret_key_signer, keystore_to_secret_key_signer, resolve_keystore_path}; +use super::common::{activate_signer, check_signability, sign_transaction}; +use crate::codec::crypto::{keystore_to_secret_key_signer, resolve_keystore_path}; use crate::constants::{ - ACTION_ITEM_CHECK_ADDRESS, ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD, - ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION, CHAIN_ID, FORMATTED_TRANSACTION, - KEYSTORE_ACCOUNT, KEYSTORE_PASSWORD, KEYSTORE_PATH, NAMESPACE, RPC_API_URL, - SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, TX_HASH, + ACTION_ITEM_CHECK_ADDRESS, ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD, CHECKED_ADDRESS, + KEYSTORE_ACCOUNT, KEYSTORE_PASSWORD, KEYSTORE_PATH, PUBLIC_KEYS, }; -use crate::rpc::EvmWalletRpc; use crate::typing::EvmValue; +use txtx_addon_kit::constants::DESCRIPTION; use txtx_addon_kit::types::signers::return_synchronous_actions; -use crate::constants::PUBLIC_KEYS; - lazy_static! { pub static ref EVM_KEYSTORE_SIGNER: SignerSpecification = define_signer! { EvmKeystoreSigner => { @@ -130,16 +116,12 @@ impl SignerImplementation for EvmKeystoreSigner { signers: SignersState, _signers_instances: &HashMap, supervision_context: &RunbookSupervisionContext, - auth_ctx: &txtx_addon_kit::types::AuthorizationContext, + auth_ctx: &AuthorizationContext, _is_balance_check_required: bool, _is_public_key_required: bool, ) -> SignerActionsFutureResult { - use txtx_addon_kit::constants::DESCRIPTION; - use crate::constants::CHECKED_ADDRESS; - let mut actions = Actions::none(); - // If we already have the signer set up, we're done if signer_state.get_value(PUBLIC_KEYS).is_some() { return return_synchronous_actions(Ok((signers, signer_state, actions))); } @@ -149,33 +131,25 @@ impl SignerImplementation for EvmKeystoreSigner { .get_markdown(auth_ctx) .map_err(|d| (signers.clone(), signer_state.clone(), d))?; - // Get keystore configuration let keystore_account = values .get_expected_string(KEYSTORE_ACCOUNT) .map_err(|e| (signers.clone(), signer_state.clone(), e))?; let keystore_path = values.get_string(KEYSTORE_PATH); - // Resolve the keystore file path let resolved_path = resolve_keystore_path(keystore_account, keystore_path) .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; - // Store the resolved path for later use signer_state.insert( "resolved_keystore_path", - Value::string(resolved_path.to_string_lossy().to_string()), + Value::string(resolved_path.to_string_lossy().into_owned()), ); - // Check if we have the password yet (from a previous response) - let password = signer_state.get_string(KEYSTORE_PASSWORD); - - if let Some(password) = password { - // We have the password, decrypt the keystore + if let Some(password) = signer_state.get_string(KEYSTORE_PASSWORD) { let signer = keystore_to_secret_key_signer(&resolved_path, password) .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; let expected_address: Address = signer.address(); - // Store the signer field bytes for later signing signer_state.insert( "signer_field_bytes", EvmValue::signer_field_bytes(signer.to_field_bytes().to_vec()), @@ -186,7 +160,6 @@ impl SignerImplementation for EvmKeystoreSigner { if signer_state.get_expected_string(CHECKED_ADDRESS).is_ok() { signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); } else { - // Ask user to review/confirm the address actions.push_sub_group( None, vec![ReviewInputRequest::new("", &Value::string(expected_address.to_string())) @@ -202,10 +175,9 @@ impl SignerImplementation for EvmKeystoreSigner { signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); } } else { - // No password yet - prompt for it let keystore_display = resolved_path .file_name() - .map(|f| f.to_string_lossy().to_string()) + .map(|f| f.to_string_lossy().into_owned()) .unwrap_or_else(|| keystore_account.to_string()); let request = ProvideInputRequest { @@ -239,11 +211,7 @@ impl SignerImplementation for EvmKeystoreSigner { _signers_instances: &HashMap, _progress_tx: &channel::Sender, ) -> SignerActivateFutureResult { - let mut result = CommandExecutionResult::new(); - let address = signer_state - .get_expected_value("signer_address") - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - result.outputs.insert("address".into(), address.clone()); + let (signers, signer_state, result) = activate_signer(signer_state, signers)?; return_synchronous_result(Ok((signers, signer_state, result))) } @@ -262,47 +230,18 @@ impl SignerImplementation for EvmKeystoreSigner { supervision_context: &RunbookSupervisionContext, _auth_ctx: &AuthorizationContext, ) -> Result { - let actions = if supervision_context.review_input_values { - let construct_did_str = &construct_did.to_string(); - if signer_state.get_scoped_value(construct_did_str, SIGNATURE_APPROVED).is_some() { - return Ok((signers, signer_state, Actions::none())); - } - - let chain_id = values - .get_expected_uint(CHAIN_ID) - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - - let status = ActionItemStatus::Todo; - - let formatted_payload = - signer_state.get_scoped_value(construct_did_str, FORMATTED_TRANSACTION); - - let request = ProvideSignedTransactionRequest::new( - &signer_state.uuid, - payload, - NAMESPACE, - &chain_id.to_string(), - ) - .check_expectation_action_uuid(construct_did) - .only_approval_needed() - .formatted_payload(formatted_payload) - .to_action_type() - .to_request(title, ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION) - .with_construct_did(construct_did) - .with_some_description(description.clone()) - .with_some_meta_description(meta_description.clone()) - .with_some_markdown(markdown.clone()) - .with_status(status); - - Actions::append_item( - request, - Some("Review and sign the transactions from the list below"), - Some("Transaction Signing"), - ) - } else { - Actions::none() - }; - Ok((signers, signer_state, actions)) + check_signability( + construct_did, + title, + description, + meta_description, + markdown, + payload, + values, + signer_state, + signers, + supervision_context, + ) } fn sign( @@ -318,53 +257,7 @@ impl SignerImplementation for EvmKeystoreSigner { let caller_uuid = caller_uuid.clone(); let values = values.clone(); - let future = async move { - let mut result = CommandExecutionResult::new(); - - let rpc_api_url = values - .get_expected_string(RPC_API_URL) - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - - let signer_field_bytes = signer_state - .get_expected_buffer_bytes("signer_field_bytes") - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - - let payload_bytes = signer_state - .get_expected_scoped_buffer_bytes( - &caller_uuid.to_string(), - SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, - ) - .unwrap(); - - let secret_key_signer = field_bytes_to_secret_key_signer(&signer_field_bytes) - .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; - - let eth_signer = EthereumWallet::from(secret_key_signer); - - let mut tx: TransactionRequest = serde_json::from_slice(&payload_bytes).unwrap(); - if tx.to.is_none() { - tx.set_create(); - } - - let tx_envelope = tx.build(ð_signer).await.map_err(|e| { - ( - signers.clone(), - signer_state.clone(), - diagnosed_error!("failed to build transaction envelope: {e}"), - ) - })?; - - let rpc = EvmWalletRpc::new(&rpc_api_url, eth_signer) - .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; - - let tx_hash = rpc.sign_and_send_tx(tx_envelope).await.map_err(|e| { - (signers.clone(), signer_state.clone(), diagnosed_error!("{}", e.to_string())) - })?; - - result.outputs.insert(TX_HASH.to_string(), EvmValue::tx_hash(tx_hash.to_vec())); - - Ok((signers, signer_state, result)) - }; + let future = async move { sign_transaction(&caller_uuid, &values, signer_state, signers).await }; Ok(Box::pin(future)) } } diff --git a/addons/evm/src/signers/secret_key.rs b/addons/evm/src/signers/secret_key.rs index 6313a9203..3f666a886 100644 --- a/addons/evm/src/signers/secret_key.rs +++ b/addons/evm/src/signers/secret_key.rs @@ -1,41 +1,22 @@ -use alloy::network::{EthereumWallet, TransactionBuilder}; use alloy::primitives::Address; -use alloy_rpc_types::TransactionRequest; use std::collections::HashMap; use txtx_addon_kit::channel; -use txtx_addon_kit::constants::SIGNATURE_APPROVED; -use txtx_addon_kit::types::commands::CommandExecutionResult; -use txtx_addon_kit::types::frontend::{ - ActionItemStatus, ProvideSignedTransactionRequest, ReviewInputRequest, -}; -use txtx_addon_kit::types::frontend::{Actions, BlockEvent}; +use txtx_addon_kit::constants::DESCRIPTION; +use txtx_addon_kit::types::frontend::{Actions, BlockEvent, ReviewInputRequest}; use txtx_addon_kit::types::signers::{ - return_synchronous_result, CheckSignabilityOk, SignerActionErr, SignerActionsFutureResult, - SignerActivateFutureResult, SignerInstance, SignerSignFutureResult, + return_synchronous_actions, return_synchronous_result, CheckSignabilityOk, SignerActionErr, + SignerActionsFutureResult, SignerActivateFutureResult, SignerImplementation, SignerInstance, + SignerSignFutureResult, SignerSpecification, SignersState, }; -use txtx_addon_kit::types::signers::{SignerImplementation, SignerSpecification}; use txtx_addon_kit::types::stores::ValueStore; -use txtx_addon_kit::types::AuthorizationContext; -use txtx_addon_kit::types::{ - commands::CommandSpecification, - diagnostics::Diagnostic, - types::{Type, Value}, -}; -use txtx_addon_kit::types::{ - signers::SignersState, types::RunbookSupervisionContext, ConstructDid, -}; +use txtx_addon_kit::types::types::{RunbookSupervisionContext, Type, Value}; +use txtx_addon_kit::types::commands::CommandSpecification; +use txtx_addon_kit::types::{diagnostics::Diagnostic, AuthorizationContext, ConstructDid}; -use crate::codec::crypto::field_bytes_to_secret_key_signer; -use crate::constants::{ - ACTION_ITEM_CHECK_ADDRESS, ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION, CHAIN_ID, - FORMATTED_TRANSACTION, NAMESPACE, RPC_API_URL, SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, - TX_HASH, -}; -use crate::rpc::EvmWalletRpc; +use super::common::{activate_signer, check_signability, sign_transaction}; +use crate::codec::crypto::{mnemonic_to_secret_key_signer, secret_key_to_secret_key_signer}; +use crate::constants::{ACTION_ITEM_CHECK_ADDRESS, CHECKED_ADDRESS, PUBLIC_KEYS}; use crate::typing::EvmValue; -use txtx_addon_kit::types::signers::return_synchronous_actions; - -use crate::constants::PUBLIC_KEYS; lazy_static! { pub static ref EVM_SECRET_KEY_SIGNER: SignerSpecification = define_signer! { @@ -124,22 +105,16 @@ impl SignerImplementation for EvmSecretKeySigner { signers: SignersState, _signers_instances: &HashMap, supervision_context: &RunbookSupervisionContext, - auth_ctx: &txtx_addon_kit::types::AuthorizationContext, + auth_ctx: &AuthorizationContext, _is_balance_check_required: bool, _is_public_key_required: bool, ) -> SignerActionsFutureResult { - use txtx_addon_kit::constants::DESCRIPTION; - - use crate::{ - codec::crypto::{mnemonic_to_secret_key_signer, secret_key_to_secret_key_signer}, - constants::CHECKED_ADDRESS, - }; - let mut actions = Actions::none(); if signer_state.get_value(PUBLIC_KEYS).is_some() { return return_synchronous_actions(Ok((signers, signer_state, actions))); } + let description = values.get_string(DESCRIPTION).map(|d| d.to_string()); let markdown = values .get_markdown(auth_ctx) @@ -162,13 +137,14 @@ impl SignerImplementation for EvmSecretKeySigner { let expected_address: Address = expected_signer.address(); + signer_state.insert( + "signer_field_bytes", + EvmValue::signer_field_bytes(expected_signer.to_field_bytes().to_vec()), + ); + signer_state.insert("signer_address", Value::string(expected_address.to_string())); + if supervision_context.review_input_values { - if let Ok(_) = signer_state.get_expected_string(CHECKED_ADDRESS) { - signer_state.insert( - "signer_field_bytes", - EvmValue::signer_field_bytes(expected_signer.to_field_bytes().to_vec()), - ); - signer_state.insert("signer_address", Value::string(expected_address.to_string())); + if signer_state.get_expected_string(CHECKED_ADDRESS).is_ok() { signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); } else { actions.push_sub_group( @@ -184,12 +160,8 @@ impl SignerImplementation for EvmSecretKeySigner { } } else { signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); - signer_state.insert( - "signer_field_bytes", - EvmValue::signer_field_bytes(expected_signer.to_field_bytes().to_vec()), - ); - signer_state.insert("signer_address", Value::string(expected_address.to_string())); } + let future = async move { Ok((signers, signer_state, actions)) }; Ok(Box::pin(future)) } @@ -203,11 +175,7 @@ impl SignerImplementation for EvmSecretKeySigner { _signers_instances: &HashMap, _progress_tx: &channel::Sender, ) -> SignerActivateFutureResult { - let mut result = CommandExecutionResult::new(); - let address = signer_state - .get_expected_value("signer_address") - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - result.outputs.insert("address".into(), address.clone()); + let (signers, signer_state, result) = activate_signer(signer_state, signers)?; return_synchronous_result(Ok((signers, signer_state, result))) } @@ -226,47 +194,18 @@ impl SignerImplementation for EvmSecretKeySigner { supervision_context: &RunbookSupervisionContext, _auth_ctx: &AuthorizationContext, ) -> Result { - let actions = if supervision_context.review_input_values { - let construct_did_str = &construct_did.to_string(); - if let Some(_) = signer_state.get_scoped_value(&construct_did_str, SIGNATURE_APPROVED) { - return Ok((signers, signer_state, Actions::none())); - } - - let chain_id = values - .get_expected_uint(CHAIN_ID) - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - - let status = ActionItemStatus::Todo; - - let formatted_payload = - signer_state.get_scoped_value(&construct_did_str, FORMATTED_TRANSACTION); - - let request = ProvideSignedTransactionRequest::new( - &signer_state.uuid, - &payload, - NAMESPACE, - &chain_id.to_string(), - ) - .check_expectation_action_uuid(construct_did) - .only_approval_needed() - .formatted_payload(formatted_payload) - .to_action_type() - .to_request(title, ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION) - .with_construct_did(construct_did) - .with_some_description(description.clone()) - .with_some_meta_description(meta_description.clone()) - .with_some_markdown(markdown.clone()) - .with_status(status); - - Actions::append_item( - request, - Some("Review and sign the transactions from the list below"), - Some("Transaction Signing"), - ) - } else { - Actions::none() - }; - Ok((signers, signer_state, actions)) + check_signability( + construct_did, + title, + description, + meta_description, + markdown, + payload, + values, + signer_state, + signers, + supervision_context, + ) } fn sign( @@ -282,54 +221,7 @@ impl SignerImplementation for EvmSecretKeySigner { let caller_uuid = caller_uuid.clone(); let values = values.clone(); - let future = async move { - let mut result = CommandExecutionResult::new(); - - let rpc_api_url = values - .get_expected_string(RPC_API_URL) - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - - let signer_field_bytes = signer_state - .get_expected_buffer_bytes("signer_field_bytes") - .map_err(|e| (signers.clone(), signer_state.clone(), e))?; - - let payload_bytes = signer_state - .get_expected_scoped_buffer_bytes( - &caller_uuid.to_string(), - SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES, - ) - .unwrap(); - - let secret_key_signer = field_bytes_to_secret_key_signer(&signer_field_bytes) - .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; - - let eth_signer = EthereumWallet::from(secret_key_signer); - - let mut tx: TransactionRequest = serde_json::from_slice(&payload_bytes).unwrap(); - if tx.to.is_none() { - // there's no to address on the tx, which is invalid unless it's set as "create" - tx.set_create(); - } - - let tx_envelope = tx.build(ð_signer).await.map_err(|e| { - ( - signers.clone(), - signer_state.clone(), - diagnosed_error!("failed to build transaction envelope: {e}"), - ) - })?; - - let rpc = EvmWalletRpc::new(&rpc_api_url, eth_signer) - .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; - - let tx_hash = rpc.sign_and_send_tx(tx_envelope).await.map_err(|e| { - (signers.clone(), signer_state.clone(), diagnosed_error!("{}", e.to_string())) - })?; - - result.outputs.insert(TX_HASH.to_string(), EvmValue::tx_hash(tx_hash.to_vec())); - - Ok((signers, signer_state, result)) - }; + let future = async move { sign_transaction(&caller_uuid, &values, signer_state, signers).await }; Ok(Box::pin(future)) } } From 5aeb45802cd1f9014e2b9992ac602bd5b922096d Mon Sep 17 00:00:00 2001 From: cds-amal Date: Wed, 10 Dec 2025 20:57:44 -0500 Subject: [PATCH 03/15] test(evm): add unit tests for keystore signer and shared code Add unit tests for: - resolve_keystore_path: path resolution, .json extension, default dirs - keystore_to_secret_key_signer: decrypt, error handling, roundtrip - mnemonic_to_secret_key_signer: valid/invalid mnemonics, derivation paths - NonceManager: serialization, state management - activate_signer: success and missing address cases - check_signability: review modes, approval state, missing chain_id Security test verifies keystore cannot be decrypted without password. Add tempfile and rand as dev-dependencies for test fixtures. --- Cargo.lock | 2 + addons/evm/Cargo.toml | 4 + addons/evm/src/codec/crypto.rs | 288 +++++++++++++++++++++++++++++++ addons/evm/src/signers/common.rs | 276 +++++++++++++++++++++++++++++ 4 files changed, 570 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 34d8d78eb..8b6c08d81 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15203,11 +15203,13 @@ dependencies = [ "lazy_static", "libsecp256k1 0.7.1", "pbkdf2 0.12.2", + "rand 0.8.5", "semver 1.0.26", "serde", "serde_derive", "serde_json", "sha2 0.10.9", + "tempfile", "thiserror 1.0.69", "tiny-hderive", "toml 0.5.11", diff --git a/addons/evm/Cargo.toml b/addons/evm/Cargo.toml index 3aede30b1..d2ffd7198 100644 --- a/addons/evm/Cargo.toml +++ b/addons/evm/Cargo.toml @@ -55,6 +55,10 @@ wasm = [ "txtx-addon-kit/wasm", ] +[dev-dependencies] +tempfile = "3" +rand = "0.8" + [lib] crate-type = ["cdylib", "rlib"] path = "src/lib.rs" diff --git a/addons/evm/src/codec/crypto.rs b/addons/evm/src/codec/crypto.rs index 32208c531..7baac9a8a 100644 --- a/addons/evm/src/codec/crypto.rs +++ b/addons/evm/src/codec/crypto.rs @@ -157,3 +157,291 @@ pub fn public_key_from_signed_message( let public_key_bytes = public_key.serialize().to_vec(); Ok(public_key_bytes) } + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use tempfile::TempDir; + + // ============ resolve_keystore_path tests ============ + + #[test] + fn test_resolve_keystore_path_absolute_path() { + let result = resolve_keystore_path("/absolute/path/to/keystore.json", None); + assert!(result.is_ok()); + assert_eq!( + result.unwrap().to_str().unwrap(), + "/absolute/path/to/keystore.json" + ); + } + + #[test] + fn test_resolve_keystore_path_with_custom_directory() { + let temp_dir = TempDir::new().unwrap(); + let keystore_path = temp_dir.path().join("myaccount.json"); + fs::write(&keystore_path, "{}").unwrap(); + + let result = + resolve_keystore_path("myaccount", Some(temp_dir.path().to_str().unwrap())); + assert!(result.is_ok()); + assert_eq!(result.unwrap(), keystore_path); + } + + #[test] + fn test_resolve_keystore_path_adds_json_extension() { + let temp_dir = TempDir::new().unwrap(); + let keystore_path = temp_dir.path().join("myaccount.json"); + fs::write(&keystore_path, "{}").unwrap(); + + let result = + resolve_keystore_path("myaccount", Some(temp_dir.path().to_str().unwrap())); + assert!(result.is_ok()); + assert!(result.unwrap().to_string_lossy().ends_with(".json")); + } + + #[test] + fn test_resolve_keystore_path_without_json_extension() { + let temp_dir = TempDir::new().unwrap(); + let keystore_path = temp_dir.path().join("myaccount"); + fs::write(&keystore_path, "{}").unwrap(); + + let result = + resolve_keystore_path("myaccount", Some(temp_dir.path().to_str().unwrap())); + assert!(result.is_ok()); + assert_eq!(result.unwrap(), keystore_path); + } + + #[test] + fn test_resolve_keystore_path_nonexistent_returns_with_json() { + let temp_dir = TempDir::new().unwrap(); + + let result = + resolve_keystore_path("nonexistent", Some(temp_dir.path().to_str().unwrap())); + assert!(result.is_ok()); + assert!(result + .unwrap() + .to_string_lossy() + .ends_with("nonexistent.json")); + } + + #[test] + fn test_resolve_keystore_path_default_foundry_dir() { + let result = resolve_keystore_path("myaccount", None); + assert!(result.is_ok()); + let path = result.unwrap(); + assert!(path.to_string_lossy().contains(".foundry")); + assert!(path.to_string_lossy().contains("keystores")); + } + + #[test] + fn test_resolve_keystore_path_prefers_json_extension() { + let temp_dir = TempDir::new().unwrap(); + // Create both files + let with_json = temp_dir.path().join("myaccount.json"); + let without_json = temp_dir.path().join("myaccount"); + fs::write(&with_json, "json").unwrap(); + fs::write(&without_json, "no-json").unwrap(); + + let result = + resolve_keystore_path("myaccount", Some(temp_dir.path().to_str().unwrap())); + assert!(result.is_ok()); + // Should prefer .json extension + assert_eq!(result.unwrap(), with_json); + } + + // ============ keystore_to_secret_key_signer tests ============ + + #[test] + fn test_keystore_to_secret_key_signer_file_not_found() { + let result = keystore_to_secret_key_signer( + std::path::Path::new("/nonexistent/path.json"), + "password", + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("not found")); + } + + #[test] + fn test_keystore_to_secret_key_signer_directory_error() { + let temp_dir = TempDir::new().unwrap(); + + let result = keystore_to_secret_key_signer(temp_dir.path(), "password"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("directory")); + } + + #[test] + fn test_keystore_to_secret_key_signer_invalid_json() { + let temp_dir = TempDir::new().unwrap(); + let keystore_path = temp_dir.path().join("invalid.json"); + fs::write(&keystore_path, "not valid json").unwrap(); + + let result = keystore_to_secret_key_signer(&keystore_path, "password"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("decrypt")); + } + + #[test] + fn test_keystore_to_secret_key_signer_wrong_password() { + let temp_dir = TempDir::new().unwrap(); + + // Create a valid keystore with known password + // Note: eth_keystore::encrypt_key returns UUID, but file is named by the `name` param + let secret_key = [1u8; 32]; + let mut rng = rand::thread_rng(); + let _uuid = eth_keystore::encrypt_key( + temp_dir.path(), + &mut rng, + &secret_key, + "correct_password", + Some("test"), + ) + .unwrap(); + + let keystore_path = temp_dir.path().join("test"); + let result = keystore_to_secret_key_signer(&keystore_path, "wrong_password"); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("decrypt")); + } + + #[test] + fn test_keystore_cannot_decrypt_without_password() { + let temp_dir = TempDir::new().unwrap(); + + let secret_key = [0xABu8; 32]; + let mut rng = rand::thread_rng(); + let _uuid = eth_keystore::encrypt_key( + temp_dir.path(), + &mut rng, + &secret_key, + "strong_password_123", + Some("secured"), + ) + .unwrap(); + + let keystore_path = temp_dir.path().join("secured"); + + // Verify keystore file exists and contains encrypted data + assert!(keystore_path.exists()); + let keystore_contents = fs::read_to_string(&keystore_path).unwrap(); + assert!(keystore_contents.contains("crypto")); + assert!(keystore_contents.contains("ciphertext")); + // The raw secret key should NOT appear in the file + assert!(!keystore_contents.contains("abababab")); + + // Empty password must fail + let result_empty = keystore_to_secret_key_signer(&keystore_path, ""); + assert!(result_empty.is_err(), "Empty password should fail decryption"); + + // Partial password must fail + let result_partial = keystore_to_secret_key_signer(&keystore_path, "strong"); + assert!(result_partial.is_err(), "Partial password should fail decryption"); + + // Correct password succeeds + let result_correct = keystore_to_secret_key_signer(&keystore_path, "strong_password_123"); + assert!(result_correct.is_ok(), "Correct password should succeed"); + } + + #[test] + fn test_keystore_to_secret_key_signer_success() { + let temp_dir = TempDir::new().unwrap(); + + let secret_key = [0x42u8; 32]; + let mut rng = rand::thread_rng(); + let _uuid = eth_keystore::encrypt_key( + temp_dir.path(), + &mut rng, + &secret_key, + "test_password", + Some("testaccount"), + ) + .unwrap(); + + // File is named by the `name` parameter, not the returned UUID + let keystore_path = temp_dir.path().join("testaccount"); + let result = keystore_to_secret_key_signer(&keystore_path, "test_password"); + + assert!(result.is_ok()); + let signer = result.unwrap(); + let _address = signer.address(); + } + + /// Tests the full keystore encryption/decryption roundtrip: + /// 1. Create a signer from a known mnemonic (source of truth) + /// 2. Extract the private key bytes and encrypt them into a keystore file + /// 3. Decrypt the keystore with the correct password + /// 4. Verify the decrypted signer produces the same address as the original + /// + /// This proves that providing the correct password extracts the original key, + /// since identical addresses can only be derived from identical private keys. + #[test] + fn test_keystore_roundtrip_extracts_correct_key_with_password() { + let temp_dir = TempDir::new().unwrap(); + + // Step 1: Create signer from known mnemonic - this is our source of truth + let mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + let original_signer = + mnemonic_to_secret_key_signer(mnemonic, None, None, None).unwrap(); + let expected_address = original_signer.address(); + + // Step 2: Encrypt the private key into a keystore file + let field_bytes = original_signer.to_field_bytes().to_vec(); + let mut rng = rand::thread_rng(); + let _uuid = eth_keystore::encrypt_key( + temp_dir.path(), + &mut rng, + &field_bytes, + "password", + Some("test"), + ) + .unwrap(); + + // Step 3: Decrypt keystore with correct password + let keystore_path = temp_dir.path().join("test"); + let decrypted_signer = + keystore_to_secret_key_signer(&keystore_path, "password").unwrap(); + + // Step 4: Verify decrypted key matches original by comparing addresses + // (address is derived from private key, so matching = same key) + assert_eq!(decrypted_signer.address(), expected_address); + } + + // ============ mnemonic_to_secret_key_signer tests ============ + + #[test] + fn test_mnemonic_to_secret_key_signer_valid() { + let mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + let result = mnemonic_to_secret_key_signer(mnemonic, None, None, None); + assert!(result.is_ok()); + } + + #[test] + fn test_mnemonic_to_secret_key_signer_invalid_mnemonic() { + let result = + mnemonic_to_secret_key_signer("invalid mnemonic words", None, None, None); + assert!(result.is_err()); + } + + #[test] + fn test_mnemonic_to_secret_key_signer_custom_derivation_path() { + let mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + let result1 = mnemonic_to_secret_key_signer(mnemonic, None, None, None); + let result2 = + mnemonic_to_secret_key_signer(mnemonic, Some("m/44'/60'/0'/0/1"), None, None); + + assert!(result1.is_ok()); + assert!(result2.is_ok()); + // Different derivation paths should produce different addresses + assert_ne!(result1.unwrap().address(), result2.unwrap().address()); + } + + #[test] + fn test_mnemonic_to_secret_key_signer_encrypted_not_supported() { + let mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + let result = + mnemonic_to_secret_key_signer(mnemonic, None, Some(true), Some("password")); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("not yet supported")); + } +} diff --git a/addons/evm/src/signers/common.rs b/addons/evm/src/signers/common.rs index dab8bbfd5..0151d9009 100644 --- a/addons/evm/src/signers/common.rs +++ b/addons/evm/src/signers/common.rs @@ -296,3 +296,279 @@ pub async fn sign_transaction( Ok((signers, signer_state, result)) } + +#[cfg(test)] +mod tests { + use super::*; + use txtx_addon_kit::types::types::RunbookSupervisionContext; + use txtx_addon_kit::types::Did; + + fn create_test_construct_did(name: &str) -> ConstructDid { + ConstructDid(Did::from_components(vec![name.as_bytes()])) + } + + fn create_test_value_store(name: &str) -> ValueStore { + ValueStore::new(name, &Did::from_components(vec![name.as_bytes()])) + } + + // ============ NonceManager tests ============ + + #[test] + fn test_nonce_manager_empty_state() { + let signer_state = create_test_value_store("test"); + let result = NonceManager::from_signer_state(&signer_state); + assert!(result.is_ok()); + let manager = result.unwrap(); + assert!(manager.0.is_empty()); + } + + #[test] + fn test_nonce_manager_get_nonce_empty() { + let signer_state = create_test_value_store("test"); + let construct_did = create_test_construct_did("test-construct"); + + let nonce = NonceManager::get_nonce_for_construct(&signer_state, 1, &construct_did); + assert!(nonce.is_none()); + } + + #[test] + fn test_nonce_manager_serialization_roundtrip() { + let mut manager = NonceManager(IndexMap::new()); + let chain_id = 1u64; + let construct_did = create_test_construct_did("test-construct"); + + let mut chain_map = IndexMap::new(); + chain_map.insert(construct_did.clone(), 42u64); + manager.0.insert(chain_id, chain_map); + + let serialized = serde_json::to_string(&manager).unwrap(); + let deserialized: NonceManager = serde_json::from_str(&serialized).unwrap(); + + assert_eq!( + deserialized + .0 + .get(&chain_id) + .unwrap() + .get(&construct_did), + Some(&42u64) + ); + } + + #[test] + fn test_nonce_manager_get_nonce_after_storing() { + let mut signer_state = create_test_value_store("test"); + let chain_id = 1u64; + let construct_did = create_test_construct_did("test-construct"); + + // Manually create and store a nonce manager + let mut manager = NonceManager(IndexMap::new()); + let mut chain_map = IndexMap::new(); + chain_map.insert(construct_did.clone(), 100u64); + manager.0.insert(chain_id, chain_map); + + let serialized = serde_json::to_string(&manager).unwrap(); + signer_state.insert(NonceManager::KEY, Value::string(serialized)); + + // Now retrieve it + let nonce = + NonceManager::get_nonce_for_construct(&signer_state, chain_id, &construct_did); + assert_eq!(nonce, Some(100u64)); + } + + #[test] + fn test_nonce_manager_get_nonce_wrong_chain() { + let mut signer_state = create_test_value_store("test"); + let chain_id = 1u64; + let construct_did = create_test_construct_did("test-construct"); + + let mut manager = NonceManager(IndexMap::new()); + let mut chain_map = IndexMap::new(); + chain_map.insert(construct_did.clone(), 100u64); + manager.0.insert(chain_id, chain_map); + + let serialized = serde_json::to_string(&manager).unwrap(); + signer_state.insert(NonceManager::KEY, Value::string(serialized)); + + // Try to get nonce for a different chain + let nonce = + NonceManager::get_nonce_for_construct(&signer_state, 42u64, &construct_did); + assert!(nonce.is_none()); + } + + #[test] + fn test_nonce_manager_invalid_json() { + let mut signer_state = create_test_value_store("test"); + signer_state.insert(NonceManager::KEY, Value::string("not valid json".to_string())); + + let result = NonceManager::from_signer_state(&signer_state); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("failed to parse")); + } + + // ============ activate_signer tests ============ + + #[test] + fn test_activate_signer_success() { + let mut signer_state = create_test_value_store("test"); + signer_state.insert( + "signer_address", + Value::string("0x1234567890abcdef".to_string()), + ); + let signers = SignersState::new(); + + let result = activate_signer(signer_state, signers); + assert!(result.is_ok()); + + let (_, _, execution_result) = result.unwrap(); + assert!(execution_result.outputs.contains_key("address")); + assert_eq!( + execution_result.outputs.get("address").unwrap().as_string(), + Some("0x1234567890abcdef") + ); + } + + #[test] + fn test_activate_signer_missing_address() { + let signer_state = create_test_value_store("test"); + let signers = SignersState::new(); + + let result = activate_signer(signer_state, signers); + assert!(result.is_err()); + } + + // ============ check_signability tests ============ + + #[test] + fn test_check_signability_no_review_returns_empty_actions() { + let signer_state = create_test_value_store("test"); + let signers = SignersState::new(); + let construct_did = create_test_construct_did("test"); + let mut values = create_test_value_store("values"); + values.insert(CHAIN_ID, Value::integer(1)); + + let supervision_context = RunbookSupervisionContext { + review_input_values: false, + review_input_default_values: false, + is_supervised: false, + }; + + let result = check_signability( + &construct_did, + "Test Transaction", + &None, + &None, + &None, + &Value::null(), + &values, + signer_state, + signers, + &supervision_context, + ); + + assert!(result.is_ok()); + let (_, _, actions) = result.unwrap(); + assert!(actions.store.is_empty()); + } + + #[test] + fn test_check_signability_with_review_returns_signing_action() { + let signer_state = create_test_value_store("test"); + let signers = SignersState::new(); + let construct_did = create_test_construct_did("test"); + let mut values = create_test_value_store("values"); + values.insert(CHAIN_ID, Value::integer(1)); + + let supervision_context = RunbookSupervisionContext { + review_input_values: true, + review_input_default_values: false, + is_supervised: true, + }; + + let result = check_signability( + &construct_did, + "Test Transaction", + &Some("Description".to_string()), + &None, + &None, + &Value::null(), + &values, + signer_state, + signers, + &supervision_context, + ); + + assert!(result.is_ok()); + let (_, _, actions) = result.unwrap(); + assert!(!actions.store.is_empty()); + } + + #[test] + fn test_check_signability_already_approved_returns_empty() { + let mut signer_state = create_test_value_store("test"); + let construct_did = create_test_construct_did("test-construct"); + + // Mark as already approved + signer_state.insert_scoped_value( + &construct_did.to_string(), + SIGNATURE_APPROVED, + Value::bool(true), + ); + + let signers = SignersState::new(); + let mut values = create_test_value_store("values"); + values.insert(CHAIN_ID, Value::integer(1)); + + let supervision_context = RunbookSupervisionContext { + review_input_values: true, + review_input_default_values: false, + is_supervised: true, + }; + + let result = check_signability( + &construct_did, + "Test Transaction", + &None, + &None, + &None, + &Value::null(), + &values, + signer_state, + signers, + &supervision_context, + ); + + assert!(result.is_ok()); + let (_, _, actions) = result.unwrap(); + assert!(actions.store.is_empty()); + } + + #[test] + fn test_check_signability_missing_chain_id() { + let signer_state = create_test_value_store("test"); + let signers = SignersState::new(); + let construct_did = create_test_construct_did("test"); + let values = create_test_value_store("values"); + // Note: not inserting CHAIN_ID + + let supervision_context = RunbookSupervisionContext { + review_input_values: true, + review_input_default_values: false, + is_supervised: true, + }; + + let result = check_signability( + &construct_did, + "Test Transaction", + &None, + &None, + &None, + &Value::null(), + &values, + signer_state, + signers, + &supervision_context, + ); + + assert!(result.is_err()); + } +} From 74368de27024e0ccfc3eeb1dc8baec939c88f52a Mon Sep 17 00:00:00 2001 From: cds-amal Date: Wed, 10 Dec 2025 21:07:43 -0500 Subject: [PATCH 04/15] refactor(evm): simplify nested logic in keystore check_activability Use early return pattern with `let Some(...) else` to flatten the deeply nested if/else structure. Combine nested boolean conditions into a single `needs_review` variable for clarity. --- addons/evm/src/signers/keystore.rs | 67 ++++++++++++++++-------------- 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/addons/evm/src/signers/keystore.rs b/addons/evm/src/signers/keystore.rs index 53717bb65..8a12dcfcf 100644 --- a/addons/evm/src/signers/keystore.rs +++ b/addons/evm/src/signers/keystore.rs @@ -144,37 +144,8 @@ impl SignerImplementation for EvmKeystoreSigner { Value::string(resolved_path.to_string_lossy().into_owned()), ); - if let Some(password) = signer_state.get_string(KEYSTORE_PASSWORD) { - let signer = keystore_to_secret_key_signer(&resolved_path, password) - .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; - - let expected_address: Address = signer.address(); - - signer_state.insert( - "signer_field_bytes", - EvmValue::signer_field_bytes(signer.to_field_bytes().to_vec()), - ); - signer_state.insert("signer_address", Value::string(expected_address.to_string())); - - if supervision_context.review_input_values { - if signer_state.get_expected_string(CHECKED_ADDRESS).is_ok() { - signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); - } else { - actions.push_sub_group( - None, - vec![ReviewInputRequest::new("", &Value::string(expected_address.to_string())) - .to_action_type() - .to_request(instance_name, ACTION_ITEM_CHECK_ADDRESS) - .with_construct_did(construct_did) - .with_some_description(description.clone()) - .with_meta_description(&format!("Check {} expected address", instance_name)) - .with_some_markdown(markdown.clone())], - ); - } - } else { - signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); - } - } else { + // If no password yet, prompt for it and return early + let Some(password) = signer_state.get_string(KEYSTORE_PASSWORD) else { let keystore_display = resolved_path .file_name() .map(|f| f.to_string_lossy().into_owned()) @@ -196,6 +167,40 @@ impl SignerImplementation for EvmKeystoreSigner { .with_some_markdown(markdown.clone()) .with_status(ActionItemStatus::Todo)], ); + + let future = async move { Ok((signers, signer_state, actions)) }; + return Ok(Box::pin(future)); + }; + + // Password available - decrypt keystore + let signer = keystore_to_secret_key_signer(&resolved_path, password) + .map_err(|e| (signers.clone(), signer_state.clone(), diagnosed_error!("{e}")))?; + + let expected_address: Address = signer.address(); + + signer_state.insert( + "signer_field_bytes", + EvmValue::signer_field_bytes(signer.to_field_bytes().to_vec()), + ); + signer_state.insert("signer_address", Value::string(expected_address.to_string())); + + // Handle address verification based on supervision context + let needs_review = supervision_context.review_input_values + && signer_state.get_expected_string(CHECKED_ADDRESS).is_err(); + + if needs_review { + actions.push_sub_group( + None, + vec![ReviewInputRequest::new("", &Value::string(expected_address.to_string())) + .to_action_type() + .to_request(instance_name, ACTION_ITEM_CHECK_ADDRESS) + .with_construct_did(construct_did) + .with_some_description(description.clone()) + .with_meta_description(&format!("Check {} expected address", instance_name)) + .with_some_markdown(markdown.clone())], + ); + } else { + signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); } let future = async move { Ok((signers, signer_state, actions)) }; From 3743d1a6469fa8b8433007c5ff28d783330dbfc8 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Wed, 10 Dec 2025 22:51:39 -0500 Subject: [PATCH 05/15] feat(evm): prompt keystore password interactively before execution Make evm::keystore signer unsupervised-only with interactive password prompting at CLI startup (Foundry-style UX). Password is never stored in the manifest. - Add prompt_for_keystore_passwords() in CLI before unsupervised execution - Block keystore signer in supervised mode with error recommending web_wallet - Remove password from documented inputs (internal use only) - Simplify check_activability by removing action generation code Security analysis: Data at rest: - Password never written to manifest or state files - Keystore file remains encrypted on disk - No password logging (marked sensitive: true) Data in flight: - Password read via dialoguer::Password (hidden input) - Passed through in-memory structures only - RunbookExecutionContext not serialized, preventing disk leakage --- addons/evm/src/constants.rs | 1 + addons/evm/src/signers/keystore.rs | 111 ++++++++++-------------- crates/txtx-cli/src/cli/runbooks/mod.rs | 51 ++++++++++- 3 files changed, 96 insertions(+), 67 deletions(-) diff --git a/addons/evm/src/constants.rs b/addons/evm/src/constants.rs index 5b559ddcc..6a9fbe44f 100644 --- a/addons/evm/src/constants.rs +++ b/addons/evm/src/constants.rs @@ -28,6 +28,7 @@ pub const WEB_WALLET_UNSIGNED_TRANSACTION_BYTES: &str = "web_wallet_unsigned_tra pub const KEYSTORE_PASSWORD: &str = "keystore_password"; pub const KEYSTORE_ACCOUNT: &str = "keystore_account"; pub const KEYSTORE_PATH: &str = "keystore_path"; +pub const PASSWORD: &str = "password"; // Default keystore directory (Foundry default) pub const DEFAULT_FOUNDRY_KEYSTORES_DIR: &str = ".foundry/keystores"; diff --git a/addons/evm/src/signers/keystore.rs b/addons/evm/src/signers/keystore.rs index 8a12dcfcf..15d4cec93 100644 --- a/addons/evm/src/signers/keystore.rs +++ b/addons/evm/src/signers/keystore.rs @@ -1,10 +1,7 @@ use alloy::primitives::Address; use std::collections::HashMap; use txtx_addon_kit::channel; -use txtx_addon_kit::types::frontend::{ - ActionItemRequestType, ActionItemStatus, Actions, BlockEvent, ProvideInputRequest, - ReviewInputRequest, -}; +use txtx_addon_kit::types::frontend::{Actions, BlockEvent}; use txtx_addon_kit::types::signers::{ return_synchronous_result, CheckSignabilityOk, SignerActionErr, SignerActionsFutureResult, SignerActivateFutureResult, SignerImplementation, SignerInstance, SignerSignFutureResult, @@ -17,12 +14,8 @@ use txtx_addon_kit::types::{diagnostics::Diagnostic, AuthorizationContext, Const use super::common::{activate_signer, check_signability, sign_transaction}; use crate::codec::crypto::{keystore_to_secret_key_signer, resolve_keystore_path}; -use crate::constants::{ - ACTION_ITEM_CHECK_ADDRESS, ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD, CHECKED_ADDRESS, - KEYSTORE_ACCOUNT, KEYSTORE_PASSWORD, KEYSTORE_PATH, PUBLIC_KEYS, -}; +use crate::constants::{CHECKED_ADDRESS, KEYSTORE_ACCOUNT, KEYSTORE_PATH, PASSWORD, PUBLIC_KEYS}; use crate::typing::EvmValue; -use txtx_addon_kit::constants::DESCRIPTION; use txtx_addon_kit::types::signers::return_synchronous_actions; lazy_static! { @@ -32,7 +25,8 @@ lazy_static! { matcher: "keystore", documentation: txtx_addon_kit::indoc! {r#" The `evm::keystore` signer uses a Foundry-compatible encrypted keystore file to sign transactions. -The keystore password will be prompted at runtime for security. + +**Note:** This signer is only supported in unsupervised mode (`--unsupervised` flag). For supervised/interactive mode, use `evm::web_wallet` instead. ### Inputs @@ -47,11 +41,15 @@ The keystore password will be prompted at runtime for security. |-----------|--------|------------------------------------------| | `address` | string | The Ethereum address derived from the keystore | +### Password + +The keystore password is prompted interactively at CLI startup (Foundry-style UX). The password is never stored in the manifest or on disk. + ### Security -- The keystore password is never stored in manifests -- Password is prompted interactively at runtime - Compatible with keystores created by `cast wallet import` +- Password is prompted securely (hidden input) before execution begins +- Password is held only in memory during execution "#}, inputs: [ keystore_account: { @@ -67,6 +65,13 @@ The keystore password will be prompted at runtime for security. optional: true, tainting: true, sensitive: false + }, + password: { + documentation: "Internal use only - populated by CLI interactive prompt.", + typing: Type::string(), + optional: true, + tainting: false, + sensitive: true } ], outputs: [ @@ -108,29 +113,36 @@ impl SignerImplementation for EvmKeystoreSigner { #[cfg(not(feature = "wasm"))] fn check_activability( - construct_did: &ConstructDid, - instance_name: &str, + _construct_did: &ConstructDid, + _instance_name: &str, _spec: &SignerSpecification, values: &ValueStore, mut signer_state: ValueStore, signers: SignersState, _signers_instances: &HashMap, supervision_context: &RunbookSupervisionContext, - auth_ctx: &AuthorizationContext, + _auth_ctx: &AuthorizationContext, _is_balance_check_required: bool, _is_public_key_required: bool, ) -> SignerActionsFutureResult { + // Keystore signer only supports unsupervised mode - for supervised mode use web_wallet + if supervision_context.is_supervised { + return Err(( + signers, + signer_state, + diagnosed_error!( + "evm::keystore signer is only supported in unsupervised mode. \ + For supervised mode, use evm::web_wallet instead." + ), + )); + } + let mut actions = Actions::none(); if signer_state.get_value(PUBLIC_KEYS).is_some() { return return_synchronous_actions(Ok((signers, signer_state, actions))); } - let description = values.get_string(DESCRIPTION).map(|d| d.to_string()); - let markdown = values - .get_markdown(auth_ctx) - .map_err(|d| (signers.clone(), signer_state.clone(), d))?; - let keystore_account = values .get_expected_string(KEYSTORE_ACCOUNT) .map_err(|e| (signers.clone(), signer_state.clone(), e))?; @@ -144,33 +156,18 @@ impl SignerImplementation for EvmKeystoreSigner { Value::string(resolved_path.to_string_lossy().into_owned()), ); - // If no password yet, prompt for it and return early - let Some(password) = signer_state.get_string(KEYSTORE_PASSWORD) else { - let keystore_display = resolved_path - .file_name() - .map(|f| f.to_string_lossy().into_owned()) - .unwrap_or_else(|| keystore_account.to_string()); - - let request = ProvideInputRequest { - default_value: None, - input_name: KEYSTORE_PASSWORD.to_string(), - typing: Type::string(), - }; - - actions.push_sub_group( - Some(format!("Enter password for keystore '{}'", keystore_display)), - vec![ActionItemRequestType::ProvideInput(request) - .to_request(instance_name, ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD) - .with_construct_did(construct_did) - .with_some_description(description.clone()) - .with_meta_description(&format!("Unlock keystore for {}", instance_name)) - .with_some_markdown(markdown.clone()) - .with_status(ActionItemStatus::Todo)], - ); - - let future = async move { Ok((signers, signer_state, actions)) }; - return Ok(Box::pin(future)); - }; + // Password is injected by CLI before execution (interactive prompt) + // Password is never stored in the manifest + let password = values.get_string(PASSWORD).ok_or_else(|| { + ( + signers.clone(), + signer_state.clone(), + diagnosed_error!( + "keystore password not provided. This should not happen in unsupervised mode - \ + the password should be prompted interactively before execution." + ), + ) + })?; // Password available - decrypt keystore let signer = keystore_to_secret_key_signer(&resolved_path, password) @@ -184,24 +181,8 @@ impl SignerImplementation for EvmKeystoreSigner { ); signer_state.insert("signer_address", Value::string(expected_address.to_string())); - // Handle address verification based on supervision context - let needs_review = supervision_context.review_input_values - && signer_state.get_expected_string(CHECKED_ADDRESS).is_err(); - - if needs_review { - actions.push_sub_group( - None, - vec![ReviewInputRequest::new("", &Value::string(expected_address.to_string())) - .to_action_type() - .to_request(instance_name, ACTION_ITEM_CHECK_ADDRESS) - .with_construct_did(construct_did) - .with_some_description(description.clone()) - .with_meta_description(&format!("Check {} expected address", instance_name)) - .with_some_markdown(markdown.clone())], - ); - } else { - signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); - } + // In unsupervised mode, we don't need interactive address review + signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); let future = async move { Ok((signers, signer_state, actions)) }; Ok(Box::pin(future)) diff --git a/crates/txtx-cli/src/cli/runbooks/mod.rs b/crates/txtx-cli/src/cli/runbooks/mod.rs index e85dcebc9..4f3ff9d06 100644 --- a/crates/txtx-cli/src/cli/runbooks/mod.rs +++ b/crates/txtx-cli/src/cli/runbooks/mod.rs @@ -2,7 +2,7 @@ use super::{env::TxtxEnv, CheckRunbook, Context, CreateRunbook, ExecuteRunbook, use crate::{get_addon_by_namespace, get_available_addons}; use ascii_table::AsciiTable; use console::Style; -use dialoguer::{theme::ColorfulTheme, Confirm, Input, Select}; +use dialoguer::{theme::ColorfulTheme, Confirm, Input, Password, Select}; use indicatif::{MultiProgress, ProgressBar, ProgressStyle}; use itertools::Itertools; use log::{debug, error, info, trace, warn}; @@ -17,7 +17,7 @@ use std::{ use tokio::sync::RwLock; use txtx_cloud::router::TxtxAuthenticatedCloudServiceRouter; use txtx_core::{ - kit::types::{commands::UnevaluatedInputsMap, stores::ValueStore}, + kit::types::{commands::UnevaluatedInputsMap, stores::{ValueMap, ValueStore}}, mustache, templates::{TXTX_MANIFEST_TEMPLATE, TXTX_README_TEMPLATE}, utils::try_write_outputs_to_file, @@ -684,6 +684,11 @@ pub async fn handle_run_command( setup_logger(runbook.to_instance_context(), &cmd.log_level).unwrap(); let log_filter: LogLevel = cmd.log_level.as_str().into(); + // Prompt for keystore passwords before unsupervised execution + if is_execution_unsupervised { + prompt_for_keystore_passwords(&mut runbook)?; + } + // should not be generating actions if is_execution_unsupervised { let _ = hiro_system_kit::thread_named("Display background tasks logs").spawn(move || { @@ -1274,3 +1279,45 @@ fn handle_log_event( }, } } + +/// Prompts for passwords for all keystore signers in the runbook. +/// This is called before unsupervised execution to gather passwords interactively. +/// Password is never stored in the manifest - always prompted securely. +fn prompt_for_keystore_passwords(runbook: &mut Runbook) -> Result<(), String> { + for flow_context in runbook.flow_contexts.iter_mut() { + for (construct_did, signer_instance) in + flow_context.execution_context.signers_instances.iter() + { + // Check if this is a keystore signer + if signer_instance.specification.matcher != "keystore" { + continue; + } + + // Display name for the password prompt + let display_name = signer_instance + .block + .body + .get_attribute("keystore_account") + .map(|attr| attr.value.to_string().trim_matches('"').to_string()) + .unwrap_or_else(|| signer_instance.name.clone()); + + // Prompt for password (Foundry-style UX) + let password = Password::with_theme(&ColorfulTheme::default()) + .with_prompt(format!("Enter password for keystore '{}'", display_name)) + .interact() + .map_err(|e| format!("Failed to read password: {}", e))?; + + // Store password in the signer's input evaluation results + let eval_result = flow_context + .execution_context + .commands_inputs_evaluation_results + .entry(construct_did.clone()) + .or_insert_with(|| { + CommandInputsEvaluationResult::new(&signer_instance.name, &ValueMap::new()) + }); + + eval_result.inputs.insert("password", Value::string(password)); + } + } + Ok(()) +} From f94d808445d9bdeea9de1f5a2a8705bb9fd59bd2 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Thu, 11 Dec 2025 13:55:17 -0500 Subject: [PATCH 06/15] fix(evm): correct inverted CHECKED_ADDRESS logic in secret_key signer Fix bug where the condition was inverted: when CHECKED_ADDRESS already existed (user confirmed), the code redundantly overwrote it. When it didn't exist (needs review), it showed the review action but would never set CHECKED_ADDRESS afterward. Change is_ok() to is_err() so the review action only appears when the address hasn't been checked yet. --- addons/evm/src/signers/secret_key.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/addons/evm/src/signers/secret_key.rs b/addons/evm/src/signers/secret_key.rs index 3f666a886..02158b50a 100644 --- a/addons/evm/src/signers/secret_key.rs +++ b/addons/evm/src/signers/secret_key.rs @@ -144,9 +144,8 @@ impl SignerImplementation for EvmSecretKeySigner { signer_state.insert("signer_address", Value::string(expected_address.to_string())); if supervision_context.review_input_values { - if signer_state.get_expected_string(CHECKED_ADDRESS).is_ok() { - signer_state.insert(CHECKED_ADDRESS, Value::string(expected_address.to_string())); - } else { + // Only show review action if address hasn't been checked yet + if signer_state.get_expected_string(CHECKED_ADDRESS).is_err() { actions.push_sub_group( None, vec![ReviewInputRequest::new("", &Value::string(expected_address.to_string())) From cc7c51c06f14f88d70176fffd86c27d336907a8a Mon Sep 17 00:00:00 2001 From: cds-amal Date: Thu, 11 Dec 2025 16:51:02 -0500 Subject: [PATCH 07/15] factor: remove unused variables - ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD - DEFAULT_FOUNDRY_KEYSTORES_DIR - KEYSTORE_PASSWORD --- addons/evm/src/constants.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/addons/evm/src/constants.rs b/addons/evm/src/constants.rs index 6a9fbe44f..cdd952963 100644 --- a/addons/evm/src/constants.rs +++ b/addons/evm/src/constants.rs @@ -25,13 +25,10 @@ pub const PUBLIC_KEYS: &str = "public_keys"; pub const SECRET_KEY_WALLET_UNSIGNED_TRANSACTION_BYTES: &str = "secret_key_wallet_unsigned_transaction_bytes"; pub const WEB_WALLET_UNSIGNED_TRANSACTION_BYTES: &str = "web_wallet_unsigned_transaction_bytes"; -pub const KEYSTORE_PASSWORD: &str = "keystore_password"; pub const KEYSTORE_ACCOUNT: &str = "keystore_account"; pub const KEYSTORE_PATH: &str = "keystore_path"; pub const PASSWORD: &str = "password"; -// Default keystore directory (Foundry default) -pub const DEFAULT_FOUNDRY_KEYSTORES_DIR: &str = ".foundry/keystores"; pub const TRANSACTION_PAYLOAD_BYTES: &str = "transaction_payload_bytes"; pub const SIGNED_MESSAGE_BYTES: &str = "signed_message_bytes"; pub const MESSAGE_BYTES: &str = "message_bytes"; @@ -103,7 +100,6 @@ pub const ACTION_ITEM_PROVIDE_PUBLIC_KEY: &str = "provide_public_key"; pub const ACTION_ITEM_PROVIDE_SIGNED_TRANSACTION: &str = "provide_signed_transaction"; pub const ACTION_ITEM_SEND_TRANSACTION: &str = "send_transaction"; pub const ACTION_OPEN_MODAL: &str = "open_modal"; -pub const ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD: &str = "provide_keystore_password"; // Default contracts pub const DEFAULT_CREATE2_FACTORY_ADDRESS: &str = "0x4e59b44847b379578588920cA78FbF26c0B4956C"; From 4137ce855a2a169af493bcc6216f50d7598b288b Mon Sep 17 00:00:00 2001 From: cds-amal Date: Thu, 11 Dec 2025 18:12:07 -0500 Subject: [PATCH 08/15] feat(evm): cache keystore passwords so users don't go insane Extract CachedPasswordResolver to remember passwords across flows. Same keystore appearing in 5 flows? One prompt. Not five. You're welcome. Add PasswordProvider trait for testability because mocking Runbook is a journey no one should take twice...and traits are dope. Include 10 tests proving the cache actually works, featuring: - MockPasswordProvider that tattles on every password request - Scenarios with alice, bob, charlie, and their various keystores - Mathematical proof that 1 < 3 (prompts, that is) --- crates/txtx-cli/src/cli/runbooks/mod.rs | 310 +++++++++++++++++++++++- 1 file changed, 301 insertions(+), 9 deletions(-) diff --git a/crates/txtx-cli/src/cli/runbooks/mod.rs b/crates/txtx-cli/src/cli/runbooks/mod.rs index 4f3ff9d06..81fc92f4e 100644 --- a/crates/txtx-cli/src/cli/runbooks/mod.rs +++ b/crates/txtx-cli/src/cli/runbooks/mod.rs @@ -1280,34 +1280,103 @@ fn handle_log_event( } } +/// Trait for providing passwords for keystore signers. +/// This abstraction enables testing without interactive prompts. +trait PasswordProvider { + fn get_password( + &mut self, + keystore_account: &str, + keystore_path: Option<&str>, + ) -> Result; +} + +/// Default password provider using dialoguer for interactive prompts. +struct DialoguerPasswordProvider; + +impl PasswordProvider for DialoguerPasswordProvider { + fn get_password( + &mut self, + keystore_account: &str, + _keystore_path: Option<&str>, + ) -> Result { + Password::with_theme(&ColorfulTheme::default()) + .with_prompt(format!("Enter password for keystore '{}'", keystore_account)) + .interact() + .map_err(|e| format!("Failed to read password: {}", e)) + } +} + /// Prompts for passwords for all keystore signers in the runbook. /// This is called before unsupervised execution to gather passwords interactively. /// Password is never stored in the manifest - always prompted securely. +/// If the same keystore account appears in multiple flows, only prompts once. fn prompt_for_keystore_passwords(runbook: &mut Runbook) -> Result<(), String> { + prompt_for_keystore_passwords_with_provider(runbook, &mut DialoguerPasswordProvider) +} + +/// Cached password resolver that prompts only once per unique (account, path) pair. +/// Returns the password for each request, using cached values when available. +struct CachedPasswordResolver<'a, P: PasswordProvider> { + provider: &'a mut P, + cache: std::collections::HashMap<(String, Option), String>, +} + +impl<'a, P: PasswordProvider> CachedPasswordResolver<'a, P> { + fn new(provider: &'a mut P) -> Self { + Self { + provider, + cache: std::collections::HashMap::new(), + } + } + + /// Get password for the given account/path, prompting only if not cached. + fn get_password( + &mut self, + keystore_account: &str, + keystore_path: Option<&str>, + ) -> Result { + let key = (keystore_account.to_string(), keystore_path.map(String::from)); + + if let Some(password) = self.cache.get(&key) { + return Ok(password.clone()); + } + + let password = self.provider.get_password(keystore_account, keystore_path)?; + self.cache.insert(key, password.clone()); + Ok(password) + } +} + +/// Internal implementation that accepts a password provider for testability. +fn prompt_for_keystore_passwords_with_provider( + runbook: &mut Runbook, + provider: &mut P, +) -> Result<(), String> { + let mut resolver = CachedPasswordResolver::new(provider); + for flow_context in runbook.flow_contexts.iter_mut() { for (construct_did, signer_instance) in flow_context.execution_context.signers_instances.iter() { - // Check if this is a keystore signer if signer_instance.specification.matcher != "keystore" { continue; } - // Display name for the password prompt - let display_name = signer_instance + let keystore_account = signer_instance .block .body .get_attribute("keystore_account") .map(|attr| attr.value.to_string().trim_matches('"').to_string()) .unwrap_or_else(|| signer_instance.name.clone()); - // Prompt for password (Foundry-style UX) - let password = Password::with_theme(&ColorfulTheme::default()) - .with_prompt(format!("Enter password for keystore '{}'", display_name)) - .interact() - .map_err(|e| format!("Failed to read password: {}", e))?; + let keystore_path = signer_instance + .block + .body + .get_attribute("keystore_path") + .map(|attr| attr.value.to_string().trim_matches('"').to_string()); + + let password = resolver.get_password(&keystore_account, keystore_path.as_deref())?; - // Store password in the signer's input evaluation results let eval_result = flow_context .execution_context .commands_inputs_evaluation_results @@ -1319,5 +1388,228 @@ fn prompt_for_keystore_passwords(runbook: &mut Runbook) -> Result<(), String> { eval_result.inputs.insert("password", Value::string(password)); } } + Ok(()) } + +#[cfg(test)] +mod keystore_password_tests { + use super::*; + use std::collections::HashMap; + + /// Mock password provider for testing the password caching logic + struct MockPasswordProvider { + passwords: HashMap<(String, Option), String>, + requests: Vec<(String, Option)>, + } + + impl MockPasswordProvider { + fn new(passwords: HashMap<(String, Option), String>) -> Self { + Self { passwords, requests: Vec::new() } + } + + fn request_count(&self) -> usize { + self.requests.len() + } + + fn get_requests(&self) -> &[(String, Option)] { + &self.requests + } + } + + impl PasswordProvider for MockPasswordProvider { + fn get_password( + &mut self, + keystore_account: &str, + keystore_path: Option<&str>, + ) -> Result { + let key = (keystore_account.to_string(), keystore_path.map(String::from)); + self.requests.push(key.clone()); + self.passwords + .get(&key) + .cloned() + .ok_or_else(|| format!("No password for '{}'", keystore_account)) + } + } + + #[test] + fn test_mock_provider_returns_configured_password() { + let mut passwords = HashMap::new(); + passwords.insert(("account1".to_string(), None), "password1".to_string()); + passwords.insert( + ("account2".to_string(), Some("/path".to_string())), + "password2".to_string(), + ); + + let mut provider = MockPasswordProvider::new(passwords); + + assert_eq!( + provider.get_password("account1", None).unwrap(), + "password1" + ); + assert_eq!( + provider.get_password("account2", Some("/path")).unwrap(), + "password2" + ); + assert_eq!(provider.request_count(), 2); + } + + #[test] + fn test_mock_provider_tracks_requests() { + let mut passwords = HashMap::new(); + passwords.insert(("acc".to_string(), None), "pass".to_string()); + + let mut provider = MockPasswordProvider::new(passwords); + + let _ = provider.get_password("acc", None); + let _ = provider.get_password("acc", None); + let _ = provider.get_password("acc", None); + + assert_eq!(provider.request_count(), 3); + } + + #[test] + fn test_mock_provider_missing_password_returns_error() { + let provider_passwords = HashMap::new(); + let mut provider = MockPasswordProvider::new(provider_passwords); + + let result = provider.get_password("unknown", None); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("unknown")); + } + + #[test] + fn test_mock_provider_different_paths_are_different_keys() { + let mut passwords = HashMap::new(); + passwords.insert(("acc".to_string(), Some("/a".to_string())), "pass_a".to_string()); + passwords.insert(("acc".to_string(), Some("/b".to_string())), "pass_b".to_string()); + + let mut provider = MockPasswordProvider::new(passwords); + + assert_eq!(provider.get_password("acc", Some("/a")).unwrap(), "pass_a"); + assert_eq!(provider.get_password("acc", Some("/b")).unwrap(), "pass_b"); + + // Same account with no path should fail + assert!(provider.get_password("acc", None).is_err()); + } + + #[test] + fn test_dialoguer_provider_struct_exists() { + // Verify the DialoguerPasswordProvider can be instantiated + let _provider = DialoguerPasswordProvider; + } + + // ==================== CachedPasswordResolver Tests ==================== + // These tests prove that the caching logic only prompts once per unique key + + #[test] + fn test_cached_resolver_same_account_prompts_once() { + // Simulates: same keystore account in 2 different flows + let mut passwords = HashMap::new(); + passwords.insert(("deployer".to_string(), None), "secret123".to_string()); + let mut provider = MockPasswordProvider::new(passwords); + + let mut resolver = CachedPasswordResolver::new(&mut provider); + + // First flow requests password for "deployer" + let pw1 = resolver.get_password("deployer", None).unwrap(); + // Second flow requests password for same "deployer" + let pw2 = resolver.get_password("deployer", None).unwrap(); + + assert_eq!(pw1, "secret123"); + assert_eq!(pw2, "secret123"); + // Provider should only have been called ONCE + assert_eq!(provider.request_count(), 1); + } + + #[test] + fn test_cached_resolver_different_accounts_prompt_separately() { + let mut passwords = HashMap::new(); + passwords.insert(("alice".to_string(), None), "alice_pw".to_string()); + passwords.insert(("bob".to_string(), None), "bob_pw".to_string()); + let mut provider = MockPasswordProvider::new(passwords); + + let mut resolver = CachedPasswordResolver::new(&mut provider); + + let pw_alice = resolver.get_password("alice", None).unwrap(); + let pw_bob = resolver.get_password("bob", None).unwrap(); + + assert_eq!(pw_alice, "alice_pw"); + assert_eq!(pw_bob, "bob_pw"); + // Two different accounts = two prompts + assert_eq!(provider.request_count(), 2); + } + + #[test] + fn test_cached_resolver_same_account_different_paths_prompt_separately() { + // Same account name but different keystore_path = different keystores + let mut passwords = HashMap::new(); + passwords.insert( + ("deployer".to_string(), Some("/keys/prod".to_string())), + "prod_pw".to_string(), + ); + passwords.insert( + ("deployer".to_string(), Some("/keys/dev".to_string())), + "dev_pw".to_string(), + ); + let mut provider = MockPasswordProvider::new(passwords); + + let mut resolver = CachedPasswordResolver::new(&mut provider); + + let pw_prod = resolver.get_password("deployer", Some("/keys/prod")).unwrap(); + let pw_dev = resolver.get_password("deployer", Some("/keys/dev")).unwrap(); + + assert_eq!(pw_prod, "prod_pw"); + assert_eq!(pw_dev, "dev_pw"); + // Different paths = different keystores = two prompts + assert_eq!(provider.request_count(), 2); + } + + #[test] + fn test_cached_resolver_multiple_flows_same_signer_prompts_once() { + // Simulates: 3 flows all using the same keystore signer + let mut passwords = HashMap::new(); + passwords.insert(("shared_signer".to_string(), None), "shared_pw".to_string()); + let mut provider = MockPasswordProvider::new(passwords); + + let mut resolver = CachedPasswordResolver::new(&mut provider); + + // Flow 1 + let pw1 = resolver.get_password("shared_signer", None).unwrap(); + // Flow 2 + let pw2 = resolver.get_password("shared_signer", None).unwrap(); + // Flow 3 + let pw3 = resolver.get_password("shared_signer", None).unwrap(); + + assert_eq!(pw1, "shared_pw"); + assert_eq!(pw2, "shared_pw"); + assert_eq!(pw3, "shared_pw"); + // Only ONE prompt despite 3 flows + assert_eq!(provider.request_count(), 1); + } + + #[test] + fn test_cached_resolver_mixed_signers_correct_prompt_count() { + // Complex scenario: 2 flows, each with 2 signers + // Flow 1: alice, bob + // Flow 2: alice, charlie + // Expected: 3 prompts (alice cached on second use) + let mut passwords = HashMap::new(); + passwords.insert(("alice".to_string(), None), "alice_pw".to_string()); + passwords.insert(("bob".to_string(), None), "bob_pw".to_string()); + passwords.insert(("charlie".to_string(), None), "charlie_pw".to_string()); + let mut provider = MockPasswordProvider::new(passwords); + + let mut resolver = CachedPasswordResolver::new(&mut provider); + + // Flow 1 + resolver.get_password("alice", None).unwrap(); + resolver.get_password("bob", None).unwrap(); + // Flow 2 + resolver.get_password("alice", None).unwrap(); // Should be cached + resolver.get_password("charlie", None).unwrap(); + + // alice=1, bob=1, charlie=1 = 3 total prompts + assert_eq!(provider.request_count(), 3); + } +} From a7244a5183c1abcccf680cb2ee109edc4f1145cd Mon Sep 17 00:00:00 2001 From: cds-amal Date: Thu, 11 Dec 2025 18:23:49 -0500 Subject: [PATCH 09/15] fix(evm): reject absolute keystore paths that forgot their .json Absolute paths without .json extension now get a polite rejection. Sure, eth_keystore::decrypt_key would also reject your grocery list, but "should have .json extension" is friendlier than "failed to decrypt keystore" when you fat-finger a path. This is a CLI run by the user with their own password, so we're not preventing sophisticated attack vectors here, only catching typos before they hurt. --- addons/evm/src/codec/crypto.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/addons/evm/src/codec/crypto.rs b/addons/evm/src/codec/crypto.rs index 7baac9a8a..983cca4f6 100644 --- a/addons/evm/src/codec/crypto.rs +++ b/addons/evm/src/codec/crypto.rs @@ -66,9 +66,19 @@ pub fn resolve_keystore_path( } }; - // If keystore_account is already an absolute path, use it directly + // If keystore_account is already an absolute path, use it directly. + // Security note: This is a CLI tool run by the user who provides both the path + // and password interactively. The file must also be a valid encrypted keystore + // (eth_keystore::decrypt_key will reject anything else). We validate the .json + // extension primarily to catch typos and provide clearer error messages. let account_path = PathBuf::from(keystore_account); if account_path.is_absolute() { + if !account_path.extension().map_or(false, |ext| ext == "json") { + return Err(format!( + "absolute keystore path should have .json extension: {:?}", + account_path + )); + } return Ok(account_path); } @@ -167,7 +177,7 @@ mod tests { // ============ resolve_keystore_path tests ============ #[test] - fn test_resolve_keystore_path_absolute_path() { + fn test_resolve_keystore_path_absolute_path_with_json() { let result = resolve_keystore_path("/absolute/path/to/keystore.json", None); assert!(result.is_ok()); assert_eq!( @@ -176,6 +186,13 @@ mod tests { ); } + #[test] + fn test_resolve_keystore_path_absolute_path_without_json_rejected() { + let result = resolve_keystore_path("/some/path/without/extension", None); + assert!(result.is_err()); + assert!(result.unwrap_err().contains(".json extension")); + } + #[test] fn test_resolve_keystore_path_with_custom_directory() { let temp_dir = TempDir::new().unwrap(); From 9eb98e8170fb507816bd76745a161bced07c0c19 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Thu, 11 Dec 2025 18:59:45 -0500 Subject: [PATCH 10/15] Update addons/evm/src/codec/crypto.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- addons/evm/src/codec/crypto.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/addons/evm/src/codec/crypto.rs b/addons/evm/src/codec/crypto.rs index 983cca4f6..7d73da9ab 100644 --- a/addons/evm/src/codec/crypto.rs +++ b/addons/evm/src/codec/crypto.rs @@ -67,10 +67,8 @@ pub fn resolve_keystore_path( }; // If keystore_account is already an absolute path, use it directly. - // Security note: This is a CLI tool run by the user who provides both the path - // and password interactively. The file must also be a valid encrypted keystore - // (eth_keystore::decrypt_key will reject anything else). We validate the .json - // extension primarily to catch typos and provide clearer error messages. + // Security: The user provides both the path and password interactively; only valid encrypted keystores are accepted. + // UX: The .json extension check is mainly to catch typos and provide clearer error messages. let account_path = PathBuf::from(keystore_account); if account_path.is_absolute() { if !account_path.extension().map_or(false, |ext| ext == "json") { From b4d00fa48dc3cce3ff7e8c0efd1ee6dff2809958 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Thu, 11 Dec 2025 21:16:04 -0500 Subject: [PATCH 11/15] feat(lint): add keystore file validation for evm::keystore signers Add linting rule that validates keystore files exist before runtime. The linter now warns if a keystore file cannot be found at the expected path, and errors if the keystore configuration is invalid (e.g., absolute path without .json extension). The HCL validator previously only extracted input references during parsing. To validate signers, we needed to also extract signer metadata (type, attributes, location). This required threading a new return type (HclValidationRefs) through multiple modules: - validation/types.rs: Add LocatedSignerRef and HclValidationRefs - hcl_validator/visitor.rs: Collect signer declarations with attributes - hcl_validator/block_processors.rs: Extract string attributes from blocks - validation/context.rs, runbook/variables.rs: Update to use new return type - evm/lib.rs: Expose codec module to share resolve_keystore_path function The CLI Linter then uses the extracted signer refs to validate keystore paths, reusing the path resolution logic from the EVM addon. --- addons/evm/src/lib.rs | 2 +- crates/txtx-cli/src/cli/lint/validator.rs | 233 +++++++++++++++++- crates/txtx-core/src/runbook/variables.rs | 4 +- crates/txtx-core/src/validation/context.rs | 8 +- .../hcl_validator/block_processors.rs | 25 +- .../src/validation/hcl_validator/visitor.rs | 52 +++- crates/txtx-core/src/validation/mod.rs | 2 +- crates/txtx-core/src/validation/types.rs | 17 ++ .../src/builders/runbook_builder_enhanced.rs | 4 +- 9 files changed, 323 insertions(+), 24 deletions(-) diff --git a/addons/evm/src/lib.rs b/addons/evm/src/lib.rs index ca9bc9aff..60b26072c 100644 --- a/addons/evm/src/lib.rs +++ b/addons/evm/src/lib.rs @@ -7,7 +7,7 @@ extern crate txtx_addon_kit; #[macro_use] extern crate serde_derive; -mod codec; +pub mod codec; mod commands; #[allow(dead_code)] mod constants; diff --git a/crates/txtx-cli/src/cli/lint/validator.rs b/crates/txtx-cli/src/cli/lint/validator.rs index a59a153be..94658754c 100644 --- a/crates/txtx-cli/src/cli/lint/validator.rs +++ b/crates/txtx-cli/src/cli/lint/validator.rs @@ -5,6 +5,7 @@ use std::path::PathBuf; use txtx_core::validation::{ValidationResult, Diagnostic}; use txtx_core::manifest::WorkspaceManifest; use txtx_addon_kit::helpers::fs::FileLocation; +use txtx_addon_network_evm::codec::crypto::resolve_keystore_path; use crate::cli::common::addon_registry; use super::config::LinterConfig; @@ -133,10 +134,12 @@ impl Linter { file_path, addon_specs, ) { - Ok(input_refs) => { + Ok(refs) => { if let Some(ref manifest) = manifest { - self.validate_with_rules(&input_refs, content, file_path, manifest, environment, &mut result); + self.validate_with_rules(&refs.inputs, content, file_path, manifest, environment, &mut result); } + // Validate signers (e.g., keystore paths) + self.validate_signers(&refs.signers, file_path, &mut result); } Err(e) => { result.errors.push( @@ -275,6 +278,55 @@ impl Linter { let formatter = super::formatter::get_formatter(self.config.format); formatter.format(&result); } + + /// Validate signers, specifically keystore signers for path validity + fn validate_signers( + &self, + signers: &[txtx_core::validation::LocatedSignerRef], + file_path: &str, + result: &mut ValidationResult, + ) { + for signer in signers.iter().filter(|s| s.signer_type == "evm::keystore") { + let Some(keystore_account) = signer.attributes.get("keystore_account") else { + continue; // Missing required attribute - caught by HCL validation + }; + + let keystore_path = signer.attributes.get("keystore_path").map(|s| s.as_str()); + + match resolve_keystore_path(keystore_account, keystore_path) { + Ok(resolved_path) if !resolved_path.exists() => { + let location_hint = keystore_path + .map(|p| format!("looked in directory '{}'", p)) + .unwrap_or_else(|| "looked in ~/.foundry/keystores".to_string()); + + result.warnings.push( + Diagnostic::warning(format!( + "keystore file not found: '{}' ({})", + keystore_account, location_hint + )) + .with_code("keystore-not-found".to_string()) + .with_file(file_path.to_string()) + .with_line(signer.line) + .with_column(signer.column) + .with_suggestion(format!( + "Ensure the keystore file exists. Create one with: cast wallet import {} --interactive", + keystore_account + )) + ); + } + Ok(_) => {} // File exists, all good + Err(e) => { + result.errors.push( + Diagnostic::error(format!("invalid keystore configuration: {}", e)) + .with_code("keystore-invalid".to_string()) + .with_file(file_path.to_string()) + .with_line(signer.line) + .with_column(signer.column) + ); + } + } + } + } } #[cfg(test)] @@ -917,4 +969,181 @@ action "test" { "Warning should have code: {:?}", warning.message); } } + + mod keystore_linting_tests { + use super::*; + use tempfile::TempDir; + use std::fs; + + fn minimal_manifest() -> WorkspaceManifest { + WorkspaceManifest { + name: "test".to_string(), + id: "test-id".to_string(), + runbooks: vec![], + environments: Default::default(), + location: None, + } + } + + // TODO: This local builder is a workaround. The proper fix is to extend + // RunbookBuilder in txtx-test-utils with a `validate_with_cli_linter()` method + // that uses the full CLI Linter instead of just hcl_validator. + struct SignerBuilder { + name: String, + signer_type: String, + attributes: Vec<(String, String)>, + } + + impl SignerBuilder { + fn new(name: &str, signer_type: &str) -> Self { + Self { + name: name.to_string(), + signer_type: signer_type.to_string(), + attributes: Vec::new(), + } + } + + fn keystore(name: &str) -> Self { + Self::new(name, "evm::keystore") + } + + fn web_wallet(name: &str) -> Self { + Self::new(name, "evm::web_wallet") + } + + fn attr(mut self, key: &str, value: &str) -> Self { + self.attributes.push((key.to_string(), value.to_string())); + self + } + + fn keystore_account(self, account: &str) -> Self { + self.attr("keystore_account", account) + } + + fn keystore_path(self, path: &str) -> Self { + self.attr("keystore_path", path) + } + + fn expected_address(self, address: &str) -> Self { + self.attr("expected_address", address) + } + + fn build(&self) -> String { + let attrs = self.attributes + .iter() + .map(|(k, v)| format!(" {} = \"{}\"", k, v)) + .collect::>() + .join("\n"); + + format!( + "signer \"{}\" \"{}\" {{\n{}\n}}", + self.name, self.signer_type, attrs + ) + } + + fn validate(&self) -> ValidationResult { + let linter = Linter::with_defaults(); + linter.validate_content(&self.build(), "test.tx", minimal_manifest(), None) + } + } + + #[test] + fn test_keystore_signer_with_existing_file() { + // Arrange + let temp_dir = TempDir::new().unwrap(); + let keystore_file = temp_dir.path().join("myaccount.json"); + fs::write(&keystore_file, "{}").unwrap(); + + // Act + let result = SignerBuilder::keystore("deployer") + .keystore_account("myaccount") + .keystore_path(&temp_dir.path().display().to_string()) + .validate(); + + // Assert + let keystore_warnings: Vec<_> = result.warnings.iter() + .filter(|w| w.code.as_ref().is_some_and(|c| c.contains("keystore"))) + .collect(); + assert!(keystore_warnings.is_empty(), + "Should not warn about existing keystore file: {:?}", keystore_warnings); + } + + #[test] + fn test_keystore_signer_with_missing_file() { + // Arrange + let temp_dir = TempDir::new().unwrap(); + let nonexistent_path = temp_dir.path().join("nonexistent"); + + // Act + let result = SignerBuilder::keystore("deployer") + .keystore_account("myaccount") + .keystore_path(&nonexistent_path.display().to_string()) + .validate(); + + // Assert + let keystore_warnings: Vec<_> = result.warnings.iter() + .filter(|w| w.code.as_deref() == Some("keystore-not-found")) + .collect(); + assert_eq!(keystore_warnings.len(), 1, + "Should warn about missing keystore file"); + assert!(keystore_warnings[0].message.contains("keystore file not found"), + "Warning should mention 'keystore file not found': {:?}", keystore_warnings[0].message); + } + + #[test] + fn test_keystore_signer_absolute_path_without_json_extension() { + // Arrange & Act + let result = SignerBuilder::keystore("deployer") + .keystore_account("/some/absolute/path/without/extension") + .validate(); + + // Assert + let keystore_errors: Vec<_> = result.errors.iter() + .filter(|e| e.code.as_deref() == Some("keystore-invalid")) + .collect(); + assert_eq!(keystore_errors.len(), 1, + "Should error on absolute path without .json extension"); + assert!(keystore_errors[0].message.contains(".json extension"), + "Error should mention .json extension: {:?}", keystore_errors[0].message); + } + + #[test] + fn test_keystore_signer_includes_suggestion() { + // Arrange + let temp_dir = TempDir::new().unwrap(); + + // Act + let result = SignerBuilder::keystore("deployer") + .keystore_account("myaccount") + .keystore_path(&temp_dir.path().display().to_string()) + .validate(); + + // Assert + let keystore_warnings: Vec<_> = result.warnings.iter() + .filter(|w| w.code.as_deref() == Some("keystore-not-found")) + .collect(); + assert_eq!(keystore_warnings.len(), 1); + assert!(keystore_warnings[0].suggestion.is_some(), + "Warning should include a suggestion"); + let suggestion = keystore_warnings[0].suggestion.as_ref().unwrap(); + assert!(suggestion.contains("cast wallet import"), + "Suggestion should mention 'cast wallet import': {:?}", suggestion); + } + + #[test] + fn test_non_keystore_signer_not_validated() { + // Arrange & Act + let result = SignerBuilder::web_wallet("deployer") + .expected_address("0x1234567890123456789012345678901234567890") + .validate(); + + // Assert + let keystore_issues: Vec<_> = result.warnings.iter() + .chain(result.errors.iter()) + .filter(|d| d.code.as_ref().is_some_and(|c| c.contains("keystore"))) + .collect(); + assert!(keystore_issues.is_empty(), + "Non-keystore signers should not trigger keystore validation"); + } + } } diff --git a/crates/txtx-core/src/runbook/variables.rs b/crates/txtx-core/src/runbook/variables.rs index 1c9809cfa..08e3a84d7 100644 --- a/crates/txtx-core/src/runbook/variables.rs +++ b/crates/txtx-core/src/runbook/variables.rs @@ -104,7 +104,7 @@ impl RunbookVariableIterator { // Run HCL validation to collect input references let mut validation_result = ValidationResult::default(); - let input_refs = validate_with_hcl_and_addons( + let refs = validate_with_hcl_and_addons( &combined_content, &mut validation_result, "runbook", @@ -112,7 +112,7 @@ impl RunbookVariableIterator { )?; // Process collected input references - for input_ref in input_refs { + for input_ref in refs.inputs { let var_name = Self::extract_variable_name(&input_ref.name); // Find which file this reference is in diff --git a/crates/txtx-core/src/validation/context.rs b/crates/txtx-core/src/validation/context.rs index 0979efc4b..921a17a49 100644 --- a/crates/txtx-core/src/validation/context.rs +++ b/crates/txtx-core/src/validation/context.rs @@ -230,17 +230,17 @@ impl ValidationContextExt for ValidationContext { fn validate_hcl(&mut self, result: &mut ValidationResult) -> Result<(), String> { // Delegate to HCL validator if let Some(specs) = self.addon_specs.clone() { - let input_refs = super::hcl_validator::validate_with_hcl_and_addons( + let refs = super::hcl_validator::validate_with_hcl_and_addons( &self.content, result, &self.file_path, specs, )?; - self.input_refs = input_refs; + self.input_refs = refs.inputs; } else { - let input_refs = + let refs = super::hcl_validator::validate_with_hcl(&self.content, result, &self.file_path)?; - self.input_refs = input_refs; + self.input_refs = refs.inputs; } Ok(()) } diff --git a/crates/txtx-core/src/validation/hcl_validator/block_processors.rs b/crates/txtx-core/src/validation/hcl_validator/block_processors.rs index 0c41b64ee..936dbcd96 100644 --- a/crates/txtx-core/src/validation/hcl_validator/block_processors.rs +++ b/crates/txtx-core/src/validation/hcl_validator/block_processors.rs @@ -38,7 +38,7 @@ pub fn process_block( source_mapper: &SourceMapper, ) -> Result, ValidationError> { match block_type { - BlockType::Signer => process_signer(block), + BlockType::Signer => process_signer(block, source_mapper), BlockType::Variable => process_variable(block, source_mapper), BlockType::Output => process_output(block), BlockType::Action => process_action(block, addon_specs, source_mapper), @@ -47,21 +47,42 @@ pub fn process_block( } } -fn process_signer(block: &Block) -> Result, ValidationError> { +fn process_signer(block: &Block, source_mapper: &SourceMapper) -> Result, ValidationError> { let name = block.labels.extract_name() .ok_or(ValidationError::MissingLabel("signer name"))?; let signer_type = block.labels.extract_type() .ok_or(ValidationError::MissingLabel("signer type"))?; + let position = extract_block_position(block, source_mapper); + + // Extract string attributes from the block body + let mut attributes = HashMap::new(); + for attr in block.body.attributes() { + if let Some(value) = extract_string_value(&attr.value) { + attributes.insert(attr.key.to_string(), value); + } + } + Ok(vec![ CollectedItem::Definition(DefinitionItem::Signer { name: name.to_string(), signer_type: signer_type.to_string(), + attributes, + position, }) ]) } +/// Extract a string value from an expression, if it is a simple string literal +fn extract_string_value(expr: &txtx_addon_kit::hcl::expr::Expression) -> Option { + use txtx_addon_kit::hcl::expr::Expression; + match expr { + Expression::String(s) => Some(s.value().to_string()), + _ => None, + } +} + fn process_variable(block: &Block, source_mapper: &SourceMapper) -> Result, ValidationError> { use txtx_addon_kit::hcl::visit::{visit_expr, Visit}; use txtx_addon_kit::hcl::expr::{Expression, TraversalOperator}; diff --git a/crates/txtx-core/src/validation/hcl_validator/visitor.rs b/crates/txtx-core/src/validation/hcl_validator/visitor.rs index 4aeb67e71..6a5f625f0 100644 --- a/crates/txtx-core/src/validation/hcl_validator/visitor.rs +++ b/crates/txtx-core/src/validation/hcl_validator/visitor.rs @@ -28,7 +28,7 @@ use txtx_addon_kit::hcl::{ use crate::runbook::location::{SourceMapper, BlockContext}; use crate::types::ConstructType; -use crate::validation::types::{LocatedInputRef, ValidationResult}; +use crate::validation::types::{HclValidationRefs, LocatedInputRef, LocatedSignerRef, ValidationResult}; use txtx_addon_kit::types::diagnostics::Diagnostic; use crate::kit::types::commands::CommandSpecification; @@ -147,7 +147,12 @@ pub enum CollectedItem { #[derive(Debug)] pub enum DefinitionItem { Variable { name: String, position: Position }, - Signer { name: String, signer_type: String }, + Signer { + name: String, + signer_type: String, + attributes: std::collections::HashMap, + position: Position, + }, Output(String), } @@ -235,6 +240,7 @@ struct Declarations { variables: HashMap, actions: HashMap, flows: HashMap, + signers: HashMap, } struct VariableDeclaration { @@ -252,6 +258,12 @@ struct FlowDeclaration { position: Position, } +struct SignerDeclaration { + signer_type: String, + attributes: HashMap, + position: Position, +} + #[derive(Default)] struct DependencyGraphs { variables: DependencyGraph, @@ -272,8 +284,13 @@ impl ValidationState { self.dependency_graphs.variables.add_node(name.clone(), None); self.declarations.variables.insert(name, VariableDeclaration { position }); } - Signer { name, signer_type } => { - self.definitions.signers.insert(name, signer_type); + Signer { name, signer_type, attributes, position } => { + self.definitions.signers.insert(name.clone(), signer_type.clone()); + self.declarations.signers.insert(name, SignerDeclaration { + signer_type, + attributes, + position, + }); } Output(name) => { self.definitions.outputs.insert(name); @@ -450,7 +467,7 @@ impl<'a> HclValidationVisitor<'a> { } } - pub fn validate(&mut self, body: &Body) -> Vec { + pub fn validate(&mut self, body: &Body) -> HclValidationRefs { // Phase 1: Collection (functional approach) self.collect_definitions(body); @@ -466,7 +483,22 @@ impl<'a> HclValidationVisitor<'a> { // Validate flow inputs after references are collected self.validate_all_flow_inputs(); - std::mem::take(&mut self.state.input_refs) + // Collect signer refs from declarations + let signer_refs: Vec = self.state.declarations.signers + .iter() + .map(|(name, decl)| LocatedSignerRef { + name: name.clone(), + signer_type: decl.signer_type.clone(), + attributes: decl.attributes.clone(), + line: decl.position.line, + column: decl.position.column, + }) + .collect(); + + HclValidationRefs { + inputs: std::mem::take(&mut self.state.input_refs), + signers: signer_refs, + } } fn collect_definitions(&mut self, body: &Body) { @@ -1000,7 +1032,7 @@ impl<'a> BasicHclValidator<'a> { Self { result, file_path, source } } - pub fn validate(&mut self, body: &Body) -> Vec { + pub fn validate(&mut self, body: &Body) -> HclValidationRefs { // Create empty specs inline - no self-reference needed let empty_specs = HashMap::new(); let mut validator = HclValidationVisitor::new( @@ -1023,7 +1055,7 @@ impl<'a> FullHclValidator<'a> { Self { result, file_path, source, addon_specs } } - pub fn validate(&mut self, body: &Body) -> Vec { + pub fn validate(&mut self, body: &Body) -> HclValidationRefs { let mut validator = HclValidationVisitor::new( self.result, self.file_path, @@ -1038,7 +1070,7 @@ pub fn validate_with_hcl( content: &str, result: &mut ValidationResult, file_path: &str, -) -> Result, String> { +) -> Result { let body: Body = content.parse().map_err(|e| format!("Failed to parse: {}", e))?; let mut validator = BasicHclValidator::new(result, file_path, content); Ok(validator.validate(&body)) @@ -1049,7 +1081,7 @@ pub fn validate_with_hcl_and_addons( result: &mut ValidationResult, file_path: &str, addon_specs: HashMap>, -) -> Result, String> { +) -> Result { let body: Body = content.parse().map_err(|e| format!("Failed to parse: {}", e))?; let mut validator = FullHclValidator::new(result, file_path, content, addon_specs); Ok(validator.validate(&body)) diff --git a/crates/txtx-core/src/validation/mod.rs b/crates/txtx-core/src/validation/mod.rs index eeda17690..d3b49f8e5 100644 --- a/crates/txtx-core/src/validation/mod.rs +++ b/crates/txtx-core/src/validation/mod.rs @@ -27,7 +27,7 @@ pub use manifest_validator::{ pub use rule_id::{AddonScope, CoreRuleId, RuleIdentifier}; pub use file_boundary::FileBoundaryMap; pub use types::{ - LocatedInputRef, ValidationResult, ValidationSuggestion, + HclValidationRefs, LocatedInputRef, LocatedSignerRef, ValidationResult, ValidationSuggestion, }; pub use txtx_addon_kit::types::diagnostics::Diagnostic; pub use validator::{validate_runbook, ValidatorConfig}; diff --git a/crates/txtx-core/src/validation/types.rs b/crates/txtx-core/src/validation/types.rs index 2c9a54504..3d21f695f 100644 --- a/crates/txtx-core/src/validation/types.rs +++ b/crates/txtx-core/src/validation/types.rs @@ -14,6 +14,23 @@ pub struct LocatedInputRef { pub column: usize, } +/// A located signer reference with its type and attributes +#[derive(Debug, Clone)] +pub struct LocatedSignerRef { + pub name: String, + pub signer_type: String, + pub attributes: std::collections::HashMap, + pub line: usize, + pub column: usize, +} + +/// Result of HCL validation containing located references +#[derive(Debug, Clone, Default)] +pub struct HclValidationRefs { + pub inputs: Vec, + pub signers: Vec, +} + #[derive(Debug, Default)] pub struct ValidationResult { pub errors: Vec, diff --git a/crates/txtx-test-utils/src/builders/runbook_builder_enhanced.rs b/crates/txtx-test-utils/src/builders/runbook_builder_enhanced.rs index 000533e1a..5ee8dec09 100644 --- a/crates/txtx-test-utils/src/builders/runbook_builder_enhanced.rs +++ b/crates/txtx-test-utils/src/builders/runbook_builder_enhanced.rs @@ -155,14 +155,14 @@ impl RunbookBuilder { &file_path_str, addon_specs, ) { - Ok(input_refs) => { + Ok(refs) => { // If we have manifest context, validate inputs if let (Some(manifest), Some(env_name)) = (&manifest, &environment) { // Convert CLI inputs from builder let cli_inputs: Vec<(String, String)> = vec![]; validate_inputs_against_manifest( - &input_refs, + &refs.inputs, &content, manifest, Some(env_name), From 22a94bdc9d4fcdd3c7e43ab4e4c285599bd6a77a Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 13 Dec 2025 15:03:36 -0500 Subject: [PATCH 12/15] refactor(cli): use local `get_string_literal` to parse HCL keystore attributes --- crates/txtx-cli/src/cli/runbooks/mod.rs | 27 +++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/crates/txtx-cli/src/cli/runbooks/mod.rs b/crates/txtx-cli/src/cli/runbooks/mod.rs index 81fc92f4e..cc6fdd24c 100644 --- a/crates/txtx-cli/src/cli/runbooks/mod.rs +++ b/crates/txtx-cli/src/cli/runbooks/mod.rs @@ -1347,6 +1347,29 @@ impl<'a, P: PasswordProvider> CachedPasswordResolver<'a, P> { } } +/// Extracts a string literal value from an HCL attribute. +/// Returns None if the attribute value is not a simple string literal. +/// +/// This uses `.value()` to get the decoded string content, which properly handles: +/// - Embedded quotes: `"my\"account"` -> `my"account` +/// - Unicode escapes: `"\u0041"` -> `A` +/// - Other escape sequences +/// +/// This is preferred over `attr.value.to_string().trim_matches('"')` which is fragile +/// and fails on strings with embedded quotes or other edge cases. +/// +/// Note: A similar helper `extract_string_value` exists in block_processors.rs but is +/// private to that module. If this pattern is needed more broadly, consider making +/// that function public and exporting it from txtx-core, or promoting to a shared +/// helper in txtx-addon-kit/helpers/hcl.rs. +fn get_string_literal(attr: &txtx_addon_kit::hcl::structure::Attribute) -> Option { + use txtx_addon_kit::hcl::expr::Expression; + match &attr.value { + Expression::String(s) => Some(s.value().to_string()), + _ => None, + } +} + /// Internal implementation that accepts a password provider for testability. fn prompt_for_keystore_passwords_with_provider( runbook: &mut Runbook, @@ -1366,14 +1389,14 @@ fn prompt_for_keystore_passwords_with_provider( .block .body .get_attribute("keystore_account") - .map(|attr| attr.value.to_string().trim_matches('"').to_string()) + .and_then(get_string_literal) .unwrap_or_else(|| signer_instance.name.clone()); let keystore_path = signer_instance .block .body .get_attribute("keystore_path") - .map(|attr| attr.value.to_string().trim_matches('"').to_string()); + .and_then(get_string_literal); let password = resolver.get_password(&keystore_account, keystore_path.as_deref())?; From 38c95dec531671a3edadc6031785b6148a488b17 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 13 Dec 2025 16:20:56 -0500 Subject: [PATCH 13/15] Separate read and write phases in keystore password collection Refactor password collection loop to collect keystore signer info first, then update evaluation results. Makes borrow semantics explicit and uses idiomatic iterator combinators. --- crates/txtx-cli/src/cli/runbooks/mod.rs | 48 ++++++++++++++----------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/crates/txtx-cli/src/cli/runbooks/mod.rs b/crates/txtx-cli/src/cli/runbooks/mod.rs index cc6fdd24c..e21b13ba7 100644 --- a/crates/txtx-cli/src/cli/runbooks/mod.rs +++ b/crates/txtx-cli/src/cli/runbooks/mod.rs @@ -1378,34 +1378,40 @@ fn prompt_for_keystore_passwords_with_provider( let mut resolver = CachedPasswordResolver::new(provider); for flow_context in runbook.flow_contexts.iter_mut() { - for (construct_did, signer_instance) in - flow_context.execution_context.signers_instances.iter() - { - if signer_instance.specification.matcher != "keystore" { - continue; - } - - let keystore_account = signer_instance - .block - .body - .get_attribute("keystore_account") - .and_then(get_string_literal) - .unwrap_or_else(|| signer_instance.name.clone()); - - let keystore_path = signer_instance - .block - .body - .get_attribute("keystore_path") - .and_then(get_string_literal); + // Collect keystore signer info first (immutable borrow of signers_instances) + let keystore_entries: Vec<_> = flow_context + .execution_context + .signers_instances + .iter() + .filter(|(_, signer)| signer.specification.matcher == "keystore") + .map(|(construct_did, signer)| { + let keystore_account = signer + .block + .body + .get_attribute("keystore_account") + .and_then(get_string_literal) + .unwrap_or_else(|| signer.name.clone()); + + let keystore_path = signer + .block + .body + .get_attribute("keystore_path") + .and_then(get_string_literal); + + (construct_did.clone(), signer.name.clone(), keystore_account, keystore_path) + }) + .collect(); + // Now prompt for passwords and update evaluation results (mutable borrow) + for (construct_did, signer_name, keystore_account, keystore_path) in keystore_entries { let password = resolver.get_password(&keystore_account, keystore_path.as_deref())?; let eval_result = flow_context .execution_context .commands_inputs_evaluation_results - .entry(construct_did.clone()) + .entry(construct_did) .or_insert_with(|| { - CommandInputsEvaluationResult::new(&signer_instance.name, &ValueMap::new()) + CommandInputsEvaluationResult::new(&signer_name, &ValueMap::new()) }); eval_result.inputs.insert("password", Value::string(password)); From 545b6e360ffa7269278980d75615692c06b251c5 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 13 Dec 2025 16:35:36 -0500 Subject: [PATCH 14/15] Improve keystore wrong password error message Replace cryptic 'Mac Mismatch' error with user-friendly message that includes the keystore path for easier debugging. --- addons/evm/src/codec/crypto.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/addons/evm/src/codec/crypto.rs b/addons/evm/src/codec/crypto.rs index 7d73da9ab..0a00b87ed 100644 --- a/addons/evm/src/codec/crypto.rs +++ b/addons/evm/src/codec/crypto.rs @@ -112,8 +112,14 @@ pub fn keystore_to_secret_key_signer( )); } - let secret_key = eth_keystore::decrypt_key(keystore_path, password) - .map_err(|e| format!("failed to decrypt keystore: {}", e))?; + let secret_key = eth_keystore::decrypt_key(keystore_path, password).map_err(|e| { + let err_str = e.to_string(); + if err_str.contains("Mac Mismatch") { + format!("incorrect password for keystore '{}'", keystore_path.display()) + } else { + format!("failed to decrypt keystore '{}': {}", keystore_path.display(), err_str) + } + })?; let signing_key = SigningKey::from_slice(&secret_key) .map_err(|e| format!("invalid key in keystore: {}", e))?; @@ -317,7 +323,7 @@ mod tests { let keystore_path = temp_dir.path().join("test"); let result = keystore_to_secret_key_signer(&keystore_path, "wrong_password"); assert!(result.is_err()); - assert!(result.unwrap_err().contains("decrypt")); + assert!(result.unwrap_err().contains("incorrect password")); } #[test] From 96e21cfb3a0f316c61d92e100acf847c9a7db314 Mon Sep 17 00:00:00 2001 From: cds-amal Date: Sat, 13 Dec 2025 16:44:50 -0500 Subject: [PATCH 15/15] Validate keystore password immediately after prompt Attempt to decrypt the keystore right after the user enters the password. If the password is incorrect, exit cleanly with an error message instead of continuing to runbook execution where the failure produces confusing follow-up errors. --- crates/txtx-cli/src/cli/runbooks/mod.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/txtx-cli/src/cli/runbooks/mod.rs b/crates/txtx-cli/src/cli/runbooks/mod.rs index e21b13ba7..2be86c1c5 100644 --- a/crates/txtx-cli/src/cli/runbooks/mod.rs +++ b/crates/txtx-cli/src/cli/runbooks/mod.rs @@ -1,5 +1,6 @@ use super::{env::TxtxEnv, CheckRunbook, Context, CreateRunbook, ExecuteRunbook, ListRunbooks}; use crate::{get_addon_by_namespace, get_available_addons}; +use txtx_addon_network_evm::codec::crypto::{keystore_to_secret_key_signer, resolve_keystore_path}; use ascii_table::AsciiTable; use console::Style; use dialoguer::{theme::ColorfulTheme, Confirm, Input, Password, Select}; @@ -1402,10 +1403,16 @@ fn prompt_for_keystore_passwords_with_provider( }) .collect(); - // Now prompt for passwords and update evaluation results (mutable borrow) + // Now prompt for passwords, validate them, and update evaluation results (mutable borrow) for (construct_did, signer_name, keystore_account, keystore_path) in keystore_entries { let password = resolver.get_password(&keystore_account, keystore_path.as_deref())?; + // Validate password immediately by attempting to decrypt the keystore. + // The returned signer is discarded; its key material is automatically zeroed + // on drop via ZeroizeOnDrop implemented by the underlying SigningKey. + let resolved_path = resolve_keystore_path(&keystore_account, keystore_path.as_deref())?; + let _ = keystore_to_secret_key_signer(&resolved_path, &password)?; + let eval_result = flow_context .execution_context .commands_inputs_evaluation_results