Conversation
…ores
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.
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.
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.
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.
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
There was a problem hiding this comment.
Pull request overview
This pull request adds a new evm::keystore signer that enables signing transactions using Foundry-compatible encrypted keystore files. The implementation includes interactive password prompting at CLI startup for enhanced security, ensuring passwords are never persisted to disk or manifest files.
Key Changes:
- New keystore signer supporting account names from default Foundry directory or custom paths
- CLI-level password prompting before unsupervised execution begins
- Refactored common signer code to eliminate duplication between secret_key and keystore implementations
- Support for ProvideInput response handling in SignerInstance for future extensibility
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
crates/txtx-cli/src/cli/runbooks/mod.rs |
Adds password prompting logic for keystore signers before unsupervised execution |
crates/txtx-addon-kit/src/types/signers.rs |
Implements ProvideInput response handling to enable password injection into signers |
addons/evm/src/signers/keystore.rs |
New keystore signer implementation with unsupervised-only execution mode |
addons/evm/src/signers/secret_key.rs |
Refactored to use shared activation, signing, and signability checking logic |
addons/evm/src/signers/common.rs |
Adds shared helper functions and comprehensive test coverage for common signer operations |
addons/evm/src/signers/mod.rs |
Registers the new keystore signer in the EVM addon's wallet list |
addons/evm/src/codec/crypto.rs |
Implements keystore path resolution and decryption with extensive test coverage |
addons/evm/src/constants.rs |
Adds constants for keystore-related configuration and actions |
addons/evm/Cargo.toml |
Adds dependencies for keystore support (eth-keystore, dirs) and test utilities |
Cargo.lock |
Updates lock file with new dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ACTION_ITEM_PROVIDE_KEYSTORE_PASSWORD - DEFAULT_FOUNDRY_KEYSTORES_DIR - KEYSTORE_PASSWORD
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)
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use txtx_addon_kit::types::commands::CommandSpecification; | ||
| use txtx_addon_kit::types::{diagnostics::Diagnostic, AuthorizationContext, ConstructDid}; |
There was a problem hiding this comment.
The imports are reorganized with CommandSpecification on a separate line (line 13) after types imports (line 12). For consistency and better grouping, consider either keeping all txtx_addon_kit::types imports together or adding a comment explaining why CommandSpecification is separated.
| use txtx_addon_kit::types::commands::CommandSpecification; | |
| use txtx_addon_kit::types::{diagnostics::Diagnostic, AuthorizationContext, ConstructDid}; | |
| use txtx_addon_kit::types::{commands::CommandSpecification, diagnostics::Diagnostic, AuthorizationContext, 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}; |
There was a problem hiding this comment.
The imports are organized with CommandSpecification on a separate line (line 12) after types imports (line 11). For consistency and better grouping, consider either keeping all txtx_addon_kit::types imports together or adding a comment explaining why CommandSpecification is separated. This matches a similar pattern in secret_key.rs.
| 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); |
There was a problem hiding this comment.
The unwrap() call on line 260 can panic if get_expected_scoped_buffer_bytes returns an error. This should use proper error handling with ? or map_err to maintain consistency with other error handling in this function and avoid potential panics.
| 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); | |
| fn test_resolve_keystore_path_prefers_json_extension() -> Result<(), Box<dyn std::error::Error>> { | |
| 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")?; | |
| fs::write(&without_json, "no-json")?; | |
| 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); | |
| Ok(()) |
| .map(|attr| attr.value.to_string().trim_matches('"').to_string()) | ||
| .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()); |
There was a problem hiding this comment.
The keystore_account and keystore_path are extracted using trim_matches('"') which assumes the values are quoted. This is fragile and could lead to incorrect behavior if the attribute value format changes or if the values contain quotes in unexpected positions. Consider using a more robust method to extract string values from attributes, such as using proper deserialization or a dedicated getter that handles the Value type correctly.
| .map(|attr| attr.value.to_string().trim_matches('"').to_string()) | |
| .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(|attr| attr.value.as_str().map(|s| s.to_string())) | |
| .unwrap_or_else(|| signer_instance.name.clone()); | |
| let keystore_path = signer_instance | |
| .block | |
| .body | |
| .get_attribute("keystore_path") | |
| .and_then(|attr| attr.value.as_str().map(|s| s.to_string())); |
| }; | ||
| let Some(signer_did) = &request.construct_did else { continue }; | ||
|
|
||
| let mut signer_state = signers.pop_signer_state(signer_did).unwrap(); |
There was a problem hiding this comment.
The unwrap() call on line 428 can panic if pop_signer_state returns None. This should be handled with proper error handling, such as using ok_or_else to return a diagnostic error when the signer state is not found.
| let mut signer_state = signers.pop_signer_state(signer_did).unwrap(); | |
| let Some(mut signer_state) = signers.pop_signer_state(signer_did) else { | |
| // Handle missing signer state gracefully, e.g., by logging, returning, or pushing a diagnostic. | |
| // Here, we simply continue to the next response. | |
| continue; | |
| }; |
| password: { | ||
| documentation: "Internal use only - populated by CLI interactive prompt.", | ||
| typing: Type::string(), | ||
| optional: true, | ||
| tainting: false, | ||
| sensitive: true | ||
| } |
There was a problem hiding this comment.
The password input is documented as "Internal use only" which may be confusing for users who view the signer documentation. Consider clarifying that this parameter is automatically populated by the CLI and should not be manually set by users, or remove it from the documented inputs entirely since it's marked as optional and handled internally.
| password: { | |
| documentation: "Internal use only - populated by CLI interactive prompt.", | |
| typing: Type::string(), | |
| optional: true, | |
| tainting: false, | |
| sensitive: true | |
| } |
| fn prompt_for_keystore_passwords_with_provider<P: PasswordProvider>( | ||
| 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() | ||
| { | ||
| if signer_instance.specification.matcher != "keystore" { | ||
| continue; | ||
| } | ||
|
|
||
| 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()); | ||
|
|
||
| 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())?; | ||
|
|
||
| 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(()) | ||
| } |
There was a problem hiding this comment.
The function prompt_for_keystore_passwords_with_provider lacks test coverage for the actual runbook integration. While the MockPasswordProvider and CachedPasswordResolver are well-tested in isolation, there are no tests that verify the function correctly iterates through flows, extracts keystore_account and keystore_path attributes, and properly injects passwords into the CommandInputsEvaluationResult. Consider adding integration tests that use mock runbook structures to verify the complete flow.
| let keystore_account = values | ||
| .get_expected_string(KEYSTORE_ACCOUNT) | ||
| .map_err(|e| (signers.clone(), signer_state.clone(), e))?; |
There was a problem hiding this comment.
The keystore_account is extracted using trim_matches('"') which assumes the value is quoted. This is fragile and could lead to incorrect behavior if the attribute value format changes or if the value contains quotes in unexpected positions. Consider using a more robust method to extract the string value from the attribute, such as using proper deserialization or a dedicated getter that handles the Value type correctly.
| let keystore_account = values | |
| .get_expected_string(KEYSTORE_ACCOUNT) | |
| .map_err(|e| (signers.clone(), signer_state.clone(), e))?; | |
| let keystore_account = match values.get(KEYSTORE_ACCOUNT) { | |
| Some(Value::String(s)) => s.clone(), | |
| Some(other) => { | |
| return Err(( | |
| signers.clone(), | |
| signer_state.clone(), | |
| diagnosed_error!( | |
| "Expected string for keystore_account, found: {other:?}" | |
| ), | |
| )); | |
| } | |
| None => { | |
| return Err(( | |
| signers.clone(), | |
| signer_state.clone(), | |
| diagnosed_error!( | |
| "Missing required value for keystore_account" | |
| ), | |
| )); | |
| } | |
| }; |
| "keystore password not provided. This should not happen in unsupervised mode - \ | ||
| the password should be prompted interactively before execution." |
There was a problem hiding this comment.
The error message "keystore password not provided. This should not happen in unsupervised mode" is somewhat technical and could be confusing for end users. Consider rephrasing to be more actionable, such as "keystore password was not prompted. Please ensure you're running in unsupervised mode with the --unsupervised flag" or similar user-facing guidance.
| "keystore password not provided. This should not happen in unsupervised mode - \ | |
| the password should be prompted interactively before execution." | |
| "Keystore password was not provided. Please ensure you are running in unsupervised mode with the --unsupervised flag, or provide the password interactively before execution." |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Refactor password collection loop to collect keystore signer info first, then update evaluation results. Makes borrow semantics explicit and uses idiomatic iterator combinators.
Replace cryptic 'Mac Mismatch' error with user-friendly message that includes the keystore path for easier debugging.
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.
Summary
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.
Open questions
Currently the runbook is processed and every keystore account is validated by asking for password. If the password cannot decode the keystore, the txtx process exists cleanly. Questions:
Features
Usage
Implementation Details
Security Analysis
Data at rest
Data in flight