feat(wallet): add default rpc and EVM execution tools#1964
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR encrypts and persists recovery mnemonics, tracks secret presence across client and server, adds per-chain network/asset defaults with env overrides, provides ERC‑20 calldata encoding and JSON‑RPC helpers, refactors wallet execution to sign-and-broadcast EVM transactions, and expands unit/agent/E2E tests to cover the flows. ChangesWallet Encryption, Execution, and Network Configuration
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletAPI
participant WalletOps
participant EvmSigner
participant RpcNode
Client->>WalletAPI: wallet.execute_prepared (confirmed=true)
WalletAPI->>WalletOps: secret_material(chain)
WalletOps-->>WalletAPI: encrypted_mnemonic + derivation_path
WalletAPI->>EvmSigner: derive signer from decrypted mnemonic
EvmSigner-->>WalletAPI: signer
WalletAPI->>RpcNode: eth_estimateGas (tx)
RpcNode-->>WalletAPI: gas estimate
WalletAPI->>EvmSigner: sign transaction
EvmSigner-->>WalletAPI: signed raw tx
WalletAPI->>RpcNode: eth_sendRawTransaction (signed raw tx)
RpcNode-->>WalletAPI: tx_hash
WalletAPI-->>Client: ExecutionResult { transactionHash, explorerUrl, transaction }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/wallet/execution.rs (1)
1-1155: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this module; it exceeds repository size guidance.
This file is now far above the repository’s Rust module size guideline, which will make future wallet changes harder to review and test safely. Please split into focused units (e.g., quote_store, evm_execution, validation, rpc_helpers, tests).
As per coding guidelines: "Keep Rust source files to ≤ ~500 lines; split modules when growing larger".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/wallet/execution.rs` around lines 1 - 1155, The file is too large—split it into smaller modules: extract the quote storage and lifecycle (move QUOTE_STORE, QUOTE_COUNTER, PreparedTransaction/PreparedKind/PreparedStatus, store_quote, take_quote, prepared_quotes_for_test, reset_quote_store_for_tests) into a quote_store module; put EVM-specific logic (execute_evm_quote, evm_tx_context, evm_balance, evm helpers like hex_to_u256, u256_to_hex, hex_to_bytes, estimated_fee_raw, and any EVM-only imports) into an evm_execution module; move validation helpers (validate_amount, validate_address, validate_calldata, format_amount) into a validation module; put rpc helpers (rpc_call wrappers, network_defaults/supported_assets/chain_status/balances that call RPCs) into rpc_helpers or api module; and relocate the #[cfg(test)] tests into their own tests module/file that imports the small modules; update use paths and keep public APIs (function names like prepare_transfer, prepare_swap, prepare_contract_call, execute_prepared) intact so callers don't change.
🧹 Nitpick comments (2)
app/src/features/wallet/setupLocalWalletFromMnemonic.test.ts (1)
69-80: ⚡ Quick winAdd a test for empty encryption output to lock failure behavior.
Now that encryption is part of the flow, add a case where
openhumanEncryptSecretresolves to an empty result and assert the function rejects without callingsetupLocalWallet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/features/wallet/setupLocalWalletFromMnemonic.test.ts` around lines 69 - 80, Add a test case in the same suite for persistLocalWalletFromMnemonic that simulates openhumanEncryptSecret resolving to an empty value (mockEncryptSecret.mockResolvedValue('') or equivalent) and assert that persistLocalWalletFromMnemonic rejects (expect(...).rejects.toThrow...) and does not call setupLocalWallet (mockSetupLocalWallet) nor setEncryptionKey; reset/mock mockEncryptSecret and mockSetupLocalWallet like the existing test and ensure you assert mockEncryptSecret was called but mockSetupLocalWallet and setEncryptionKey were not called when persistLocalWalletFromMnemonic returns due to empty encryption output.app/test/e2e/specs/chat-harness-wallet-flow.spec.ts (1)
87-95: ⚡ Quick winUse shared element helpers instead of direct DOM scripting for checkbox interaction.
This direct
browser.executeclick path is brittle across platforms/runners. Please route this through the shared E2E helper layer (element-helpers.ts) to keep spec behavior consistent in isolation and CI.As per coding guidelines: "E2E test specs must ensure each spec is runnable in isolation and use helpers from
element-helpers.ts... for cross-platform element interaction".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/test/e2e/specs/chat-harness-wallet-flow.spec.ts` around lines 87 - 95, Update clickRecoveryConsentCheckbox to stop using browser.execute DOM scripting and instead call the shared E2E helpers from element-helpers.ts: import the selector and click helpers (e.g., findBySelector / clickElement or clickCheckbox) to locate 'input[type="checkbox"]', click it if not already checked, then read its checked state via a helper (e.g., getProperty/getChecked) and assert true; keep the function name clickRecoveryConsentCheckbox and preserve the Promise<void> signature so specs still await it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/features/wallet/setupLocalWalletFromMnemonic.ts`:
- Around line 21-29: Before calling setEncryptionKey and setupLocalWallet,
validate the result from openhumanEncryptSecret (the encryptedMnemonic variable)
and reject empty or all-whitespace payloads: check encryptedMnemonic (from
openhumanEncryptSecret(normalizedMnemonic).result) and if it's falsy or only
whitespace, throw/return an error or abort setup instead of proceeding to
setEncryptionKey and calling setupLocalWallet (which currently uses
encryptedMnemonic and deriveWalletAccountsFromMnemonic); ensure the failure path
prevents persisting a "configured" wallet without usable secret material.
In `@src/openhuman/wallet/abi.rs`:
- Around line 9-12: The parsing currently converts amount_raw into a u128 (let
amount = amount_raw.trim().parse::<u128>()...), which wrongly caps ERC‑20
amounts; replace the u128 parsing with full uint256 parsing (e.g., use
ethereum_types::U256 or primitive_types::U256 via U256::from_dec_str or an
equivalent big‑uint decimal parser) for the variables amount/amount_raw so
inputs larger than u128::MAX are accepted, propagate the same error handling
semantics (map_err with a clear message) and update the second occurrence
mentioned (around the other parse at line 37) similarly.
In `@src/openhuman/wallet/execution.rs`:
- Around line 591-600: The evm_balance error branch currently returns a zero
placeholder but still sets provider_status to ProviderStatus::Ready; change that
branch so that on Err(error) you log the error as before but set provider_status
to ProviderStatus::Unavailable (or whatever "down" enum variant exists) instead
of ProviderStatus::Ready, leaving the Ok(path) to set ProviderStatus::Ready;
update the match arm for evm_balance to return ("0".to_string(),
ProviderStatus::Unavailable) so provider availability accurately reflects RPC
failures.
- Around line 818-827: The code calls take_quote(¶ms.quote_id) which
destructively removes the quote before attempting execution (execute_evm_quote),
preventing retries on failure; change to a non-destructive read (e.g.,
get_quote/read_quote or peek_quote) to fetch the quote without removing it, then
only call the destructive action (e.g., consume_quote/delete_quote) after
execute_evm_quote returns Ok; alternatively, if API cannot be changed, reinsert
the original quote back into the store on any Err path (including unsupported
WalletChain branches that use chain_str(other)) so the user can retry with the
same confirmation payload.
In `@src/openhuman/wallet/ops.rs`:
- Around line 401-425: Add tracing to the secret_material flow: instrument the
top of the async function secret_material with a debug/trace entry log including
the target chain (but not secrets), log before/after calling
config_rpc::load_config_with_timeout and when acquiring WALLET_STATE_FILE_LOCK,
log branch decisions such as whether load_stored_wallet_state_unlocked returned
None or Some and whether encrypted_mnemonic was missing/empty, and log the final
success with derivation_path (but never log the encrypted_mnemonic or any secret
material); also convert error returns to include trace/debug logs right before
returning to aid diagnosis (use the tracing/log crate at debug/trace level).
- Around line 374-378: The setup path accepts a missing or empty
encrypted_mnemonic and proceeds to write a configured-but-unsignable wallet;
modify the setup logic in src/openhuman/wallet/ops.rs (the setup function that
uses the encrypted_mnemonic field) to validate that params.encrypted_mnemonic is
Some and not empty before persisting state, and return an Err (e.g. new
WalletError::MissingEncryptedMnemonic or an appropriate Result::Err) early if
the value is None or all-whitespace; do this check where the current builder
maps/filters encrypted_mnemonic (the encrypted_mnemonic:
params.encrypted_mnemonic... block) and prevent any subsequent state write when
the check fails.
In `@src/openhuman/wallet/rpc.rs`:
- Around line 21-26: The debug log currently prints the full RPC endpoint (url)
which may contain secrets; change the logging to redact sensitive parts instead
of printing url directly — e.g., create a redacted value (use the Url parser to
remove username, password, query parameters and sensitive path segments or
replace them with "<redacted>") and log that redacted_url in place of url in the
log::debug call (referencing LOG_PREFIX, chain, method, url variables). Ensure
any helper is named clearly (e.g., redact_url) and used where the current
log::debug("{LOG_PREFIX} chain={:?} method={} url={}", chain, method, url) is
emitted.
- Around line 27-42: The request currently builds a reqwest client without
timeout and calls response.json() before checking HTTP status; update the client
creation in this RPC code to use
reqwest::Client::builder().timeout(Duration::from_secs(15)).build() and use that
client to send the request; after .send().await, call response.status() then
response.text().await to get the raw body string, check status.is_success() and
return an HTTP-failure error including the raw body if not successful, and only
then deserialize the body into a serde_json::Value (e.g., via
serde_json::from_str) into the existing body variable; additionally emit
debug/trace logs after the status check (including method, status and body
string) and after successful deserialization to aid troubleshooting (reference
the variables/identifiers response, status, body, method in
src/openhuman/wallet/rpc.rs).
---
Outside diff comments:
In `@src/openhuman/wallet/execution.rs`:
- Around line 1-1155: The file is too large—split it into smaller modules:
extract the quote storage and lifecycle (move QUOTE_STORE, QUOTE_COUNTER,
PreparedTransaction/PreparedKind/PreparedStatus, store_quote, take_quote,
prepared_quotes_for_test, reset_quote_store_for_tests) into a quote_store
module; put EVM-specific logic (execute_evm_quote, evm_tx_context, evm_balance,
evm helpers like hex_to_u256, u256_to_hex, hex_to_bytes, estimated_fee_raw, and
any EVM-only imports) into an evm_execution module; move validation helpers
(validate_amount, validate_address, validate_calldata, format_amount) into a
validation module; put rpc helpers (rpc_call wrappers,
network_defaults/supported_assets/chain_status/balances that call RPCs) into
rpc_helpers or api module; and relocate the #[cfg(test)] tests into their own
tests module/file that imports the small modules; update use paths and keep
public APIs (function names like prepare_transfer, prepare_swap,
prepare_contract_call, execute_prepared) intact so callers don't change.
---
Nitpick comments:
In `@app/src/features/wallet/setupLocalWalletFromMnemonic.test.ts`:
- Around line 69-80: Add a test case in the same suite for
persistLocalWalletFromMnemonic that simulates openhumanEncryptSecret resolving
to an empty value (mockEncryptSecret.mockResolvedValue('') or equivalent) and
assert that persistLocalWalletFromMnemonic rejects
(expect(...).rejects.toThrow...) and does not call setupLocalWallet
(mockSetupLocalWallet) nor setEncryptionKey; reset/mock mockEncryptSecret and
mockSetupLocalWallet like the existing test and ensure you assert
mockEncryptSecret was called but mockSetupLocalWallet and setEncryptionKey were
not called when persistLocalWalletFromMnemonic returns due to empty encryption
output.
In `@app/test/e2e/specs/chat-harness-wallet-flow.spec.ts`:
- Around line 87-95: Update clickRecoveryConsentCheckbox to stop using
browser.execute DOM scripting and instead call the shared E2E helpers from
element-helpers.ts: import the selector and click helpers (e.g., findBySelector
/ clickElement or clickCheckbox) to locate 'input[type="checkbox"]', click it if
not already checked, then read its checked state via a helper (e.g.,
getProperty/getChecked) and assert true; keep the function name
clickRecoveryConsentCheckbox and preserve the Promise<void> signature so specs
still await it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62357f1e-3029-4cd0-a212-1255df213c5c
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.env.exampleCargo.tomlapp/src/components/settings/panels/__tests__/ConnectionsPanel.test.tsxapp/src/components/settings/panels/__tests__/RecoveryPhrasePanel.test.tsxapp/src/features/wallet/setupLocalWalletFromMnemonic.test.tsapp/src/features/wallet/setupLocalWalletFromMnemonic.tsapp/src/services/walletApi.test.tsapp/src/services/walletApi.tsapp/test/Mnemonic.test.tsxapp/test/e2e/specs/chat-harness-wallet-flow.spec.tssrc/openhuman/agent/agents/crypto_agent/agent.tomlsrc/openhuman/agent/agents/crypto_agent/prompt.mdsrc/openhuman/agent/agents/loader.rssrc/openhuman/agent/harness/test_support_test.rssrc/openhuman/test_support/introspect.rssrc/openhuman/test_support/schemas.rssrc/openhuman/wallet/abi.rssrc/openhuman/wallet/defaults.rssrc/openhuman/wallet/execution.rssrc/openhuman/wallet/mod.rssrc/openhuman/wallet/ops.rssrc/openhuman/wallet/rpc.rssrc/openhuman/wallet/schemas.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/openhuman/wallet/ops.rs (1)
188-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't invalidate pre-existing wallet state during load.
load_stored_wallet_state_unlocked()now runs persisted state throughvalidate_setup(), and that validator requiresencrypted_mnemonic. Olderwallet-state.jsonfiles deserialize withencrypted_mnemonic: None, so upgrade will quarantine them and report the wallet as unconfigured instead of preservingconfigured=truewithsecret_stored=false.Suggested direction
- let validation_params = WalletSetupParams { - consent_granted: state.consent_granted, - source: state.source, - mnemonic_word_count: state.mnemonic_word_count, - encrypted_mnemonic: state.encrypted_mnemonic.clone(), - accounts: state.accounts.clone(), - }; - if let Err(validation_error) = validate_setup(&validation_params) { + if let Err(validation_error) = validate_loaded_state(&state) { warn!( "{LOG_PREFIX} stored wallet state at {} failed validation: {validation_error}", path.display() ); quarantine_corrupted_wallet_state(&path, &validation_error); return Ok(None); }Keep
validate_setup()strict for new writes, but letvalidate_loaded_state()accept legacy records with missingencrypted_mnemonicso only signing paths fail until the user re-imports.Also applies to: 273-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/wallet/ops.rs` around lines 188 - 200, The loader load_stored_wallet_state_unlocked currently calls validate_setup which rejects legacy records missing encrypted_mnemonic, causing valid pre-upgrade states to be quarantined; change the load path to use a new looser validator (e.g., validate_loaded_state) that accepts WalletSetupParams with encrypted_mnemonic = None and only marks signing-capability as unavailable, leaving configured=true and secret_stored=false intact, while keeping validate_setup strict for new writes; update calls around where validate_setup is used in load_stored_wallet_state_unlocked (and the similar block at the other site) to call validate_loaded_state and only call quarantine_corrupted_wallet_state when validate_loaded_state returns an outright corruption error (not merely missing encrypted_mnemonic).src/openhuman/wallet/schemas.rs (1)
45-58: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd JSON-RPC e2e coverage for the newly exposed wallet methods.
This wires up
wallet.network_defaultsandwallet.encode_erc20_transfer, but the suppliedtests/json_rpc_e2e.rsonly coverswallet_supported_assets. Please add transport-level assertions for the two new methods before surfacing them here.As per coding guidelines "Extend
tests/json_rpc_e2e.rswhen adding new JSON-RPC methods to prove they work over the RPC transport before surfacing in the UI".Also applies to: 75-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/wallet/schemas.rs` around lines 45 - 58, The test suite is missing transport-level JSON-RPC coverage for the newly added wallet methods; update tests/json_rpc_e2e.rs to add end-to-end RPC assertions for wallet.network_defaults and wallet.encode_erc20_transfer similar to the existing wallet_supported_assets test: craft RPC requests for the methods, assert successful HTTP/RPC responses and expected payload shapes/fields, and wire these tests so they run with the same helpers/setup used by the existing wallet_supported_assets test; this ensures the methods referenced by all_wallet_controller_schemas() are validated over the transport before being exposed.
♻️ Duplicate comments (1)
src/openhuman/wallet/execution.rs (1)
620-624:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnsupported balance providers shouldn't report
Ready.This branch is still returning a synthetic zero balance, so
ProviderStatus::Readymakes BTC/Solana/Tron look genuinely empty instead of unavailable. Please returnMissinghere until those providers actually back the response.Suggested change
- ("0".to_string(), ProviderStatus::Ready) + ("0".to_string(), ProviderStatus::Missing)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/wallet/execution.rs` around lines 620 - 624, The branch that logs the placeholder balance currently returns ("0".to_string(), ProviderStatus::Ready) which incorrectly marks unsupported providers as available; update that return to ("0".to_string(), ProviderStatus::Missing) so BTC/Solana/Tron report Missing rather than Ready—locate the warn!(...) call that uses chain_str(account.chain) in the balances handling code (execution.rs) and change the ProviderStatus enum value returned there from Ready to Missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/wallet/execution.rs`:
- Around line 411-427: After deriving the EVM signer with MnemonicBuilder (and
creating `signer`) and parsing `quote.from_address` to `from`
(Address::from_str), verify that the actual signer address matches the quote
sender before proceeding: call `signer.address()` (or equivalent) and compare it
to `from`/`quote.from_address`; if they differ, return an error/abort (with a
clear message referencing the mismatch) rather than continuing to use the
quote's nonce/estimation or signing the transaction. Ensure this check occurs
immediately after the signer derivation and address parsing and before any
nonce/estimate or signing logic.
- Around line 519-524: The debug logs and API payloads currently include the
full RPC endpoint from rpc_url_for_chain(WalletChain::Evm) (seen in the debug!
call in execute_prepared and the later block at lines ~573-582); change these
sites to log/return a redacted form (e.g., redact_rpc_url or a flag-based stub)
instead of the raw URL, keep the full URL only inside the transport layer/where
the HTTP client is created, and update the code paths that build the API payload
to use the redacted value so credentials in env-overrides are never surfaced in
logs or responses.
- Around line 835-845: The current flow calls get_quote(¶ms.quote_id) then
awaits execute_evm_quote(...) and only after that calls take_quote(...), which
risks TOCTOU double-submits; change the flow to atomically reserve the quote
before awaiting the async broadcast: introduce or use a reservation API (e.g.,
reserve_quote(params.quote_id) or convert take_quote to a two-step API
reserve_quote/take_quote) so that you mark the quote as reserved/consumed before
calling execute_evm_quote(quote). Then, after execute_evm_quote returns success,
finalize removal (take_quote/finalize_quote); on any failure (broadcast error,
timeouts, etc.) release the reservation (e.g., restore_quote or unreserve_quote)
so the quote remains retryable. Ensure reservation/unreserve calls reference
get_quote, reserve_quote/take_quote, execute_evm_quote, and params.quote_id and
that reservation happens synchronously before the await.
- Around line 381-385: The nonce lookup for outbound sends currently calls
rpc_call(WalletChain::Evm, "eth_getTransactionCount", json!([from_address,
"latest"])) (stored in nonce_hex) which ignores this wallet's pending
transactions; update both places where eth_getTransactionCount is invoked for
preparing a new send (the nonce_hex call and the similar call around lines
470-474) to use "pending" instead of "latest" so the pending nonce is returned
and consecutive sends won't reuse the same nonce.
- Around line 1-35: This module has grown past the repo size cap and should be
split into smaller sibling modules: extract the quote/storage and related types
into a new wallet/quotes.rs (move types and functions that manage quote
storage), move read-only helpers (balance/supporting assets/network defaults)
into wallet/read.rs (functions referencing find_asset, default_networks,
rpc_url_for_chain, explorer_tx_url), move EVM signing/broadcast and
prepare/execute logic into wallet/evm.rs (functions using TypedTransaction,
TransactionRequest, encode_erc20_transfer, MnemonicBuilder/Signer,
secret_material, rpc_call), and move test harnesses into wallet/tests.rs; ensure
public APIs used elsewhere (WalletAccount, WalletChain, WalletAssetDefinition,
wallet_status) keep their visibility and update module declarations and use
paths accordingly so imports compiling elsewhere continue to reference the moved
symbols.
In `@tests/json_rpc_e2e.rs`:
- Around line 2688-2692: The test pins the asset catalog size with
assert_eq!(list.len(), 8) which is brittle; instead remove or relax this
exact-length assertion in tests/json_rpc_e2e.rs and rely on the existing
ETH/USDC presence checks (the subsequent assertions that verify specific tokens)
to validate correct behavior — locate the assert_eq!(list.len(), 8, "expected
four native assets plus EVM tokens: {result}") and either delete it or replace
it with a non-exact check (e.g., ensure list.len() >= expected_minimum) so
benign catalog additions won't break the transport test.
- Around line 543-562: The mock's match on `method` currently sets `result` to
Value::Null in the `_` arm which hides unexpected upstream RPC calls; change the
default arm in the `match method { ... _ => ... }` so it returns a JSON-RPC
error response instead of `Value::Null` (e.g., an error object with code -32601
and a message that includes the unexpected `method`), and adjust the final
return from `Json(json!(...))` accordingly so tests fail fast when
`wallet_execute_prepared` invokes an unhandled RPC method.
---
Outside diff comments:
In `@src/openhuman/wallet/ops.rs`:
- Around line 188-200: The loader load_stored_wallet_state_unlocked currently
calls validate_setup which rejects legacy records missing encrypted_mnemonic,
causing valid pre-upgrade states to be quarantined; change the load path to use
a new looser validator (e.g., validate_loaded_state) that accepts
WalletSetupParams with encrypted_mnemonic = None and only marks
signing-capability as unavailable, leaving configured=true and
secret_stored=false intact, while keeping validate_setup strict for new writes;
update calls around where validate_setup is used in
load_stored_wallet_state_unlocked (and the similar block at the other site) to
call validate_loaded_state and only call quarantine_corrupted_wallet_state when
validate_loaded_state returns an outright corruption error (not merely missing
encrypted_mnemonic).
In `@src/openhuman/wallet/schemas.rs`:
- Around line 45-58: The test suite is missing transport-level JSON-RPC coverage
for the newly added wallet methods; update tests/json_rpc_e2e.rs to add
end-to-end RPC assertions for wallet.network_defaults and
wallet.encode_erc20_transfer similar to the existing wallet_supported_assets
test: craft RPC requests for the methods, assert successful HTTP/RPC responses
and expected payload shapes/fields, and wire these tests so they run with the
same helpers/setup used by the existing wallet_supported_assets test; this
ensures the methods referenced by all_wallet_controller_schemas() are validated
over the transport before being exposed.
---
Duplicate comments:
In `@src/openhuman/wallet/execution.rs`:
- Around line 620-624: The branch that logs the placeholder balance currently
returns ("0".to_string(), ProviderStatus::Ready) which incorrectly marks
unsupported providers as available; update that return to ("0".to_string(),
ProviderStatus::Missing) so BTC/Solana/Tron report Missing rather than
Ready—locate the warn!(...) call that uses chain_str(account.chain) in the
balances handling code (execution.rs) and change the ProviderStatus enum value
returned there from Ready to Missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfd43794-6cc4-445e-9e99-abb927ba7921
📒 Files selected for processing (9)
app/src/features/wallet/setupLocalWalletFromMnemonic.test.tsapp/src/features/wallet/setupLocalWalletFromMnemonic.tsapp/test/e2e/specs/chat-harness-wallet-flow.spec.tssrc/openhuman/wallet/abi.rssrc/openhuman/wallet/execution.rssrc/openhuman/wallet/ops.rssrc/openhuman/wallet/rpc.rssrc/openhuman/wallet/schemas.rstests/json_rpc_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/openhuman/wallet/abi.rs
- app/src/features/wallet/setupLocalWalletFromMnemonic.ts
- src/openhuman/wallet/rpc.rs
- app/test/e2e/specs/chat-harness-wallet-flow.spec.ts
Comment on lines
+1
to
35
| //! Wallet execution surface — read tools (balances / supported assets / | ||
| //! network defaults / chain status) and write tools (prepare-then-execute) | ||
| //! for native sends, token transfers, swaps, and contract calls. | ||
| //! | ||
| //! Design rules (see issue #1396): | ||
| //! - Quote / simulate first, then explicit confirm-and-execute. No one-shot | ||
| //! hidden execution. | ||
| //! - Signing material stays local. `execute_prepared` returns a | ||
| //! `ReadyToSign` structured payload that the desktop keystore consumes — | ||
| //! this module never touches mnemonics or private keys. | ||
| //! - Wallet must be configured (see [`crate::openhuman::wallet::status`]) | ||
| //! before any read or write tool is callable. | ||
| //! - Every decision point emits a grep-friendly `[wallet]` debug log. | ||
| //! | ||
| //! On-chain RPC providers are not yet configured (#1395 ships the keystore; | ||
| //! provider config lives behind `OPENHUMAN_WALLET_RPC_*` env vars). Until a | ||
| //! provider is wired, balances surface `provider_status: "unconfigured"` | ||
| //! with zero values rather than fabricating numbers. | ||
| //! Execution is intentionally narrower than the metadata surface: | ||
| //! - Every write must be prepared first, then explicitly confirmed. | ||
| //! - Secret material stays encrypted at rest in core-owned storage. | ||
| //! - Mainnet EVM signing + broadcast are implemented here today. | ||
| //! - BTC / Solana / Tron remain read-only / quote-only until their | ||
| //! providers and signing flows are actually wired. | ||
|
|
||
| use std::str::FromStr; | ||
| use std::sync::atomic::{AtomicU64, Ordering}; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
|
||
| use ethers_core::types::transaction::eip2718::TypedTransaction; | ||
| use ethers_core::types::{Address, Bytes, NameOrAddress, TransactionRequest, U256}; | ||
| use ethers_signers::{coins_bip39::English, MnemonicBuilder, Signer}; | ||
| use log::{debug, warn}; | ||
| use once_cell::sync::Lazy; | ||
| use parking_lot::Mutex; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::json; | ||
|
|
||
| use crate::openhuman::config::rpc as config_rpc; | ||
| use crate::rpc::RpcOutcome; | ||
|
|
||
| use super::ops::{status as wallet_status, WalletAccount, WalletChain}; | ||
| use super::abi::encode_erc20_transfer; | ||
| use super::defaults::{ | ||
| explorer_tx_url, find_asset, network_defaults as default_networks, rpc_url_for_chain, | ||
| WalletAssetDefinition, WalletNetworkDefaults, | ||
| }; | ||
| use super::ops::{secret_material, status as wallet_status, WalletAccount, WalletChain}; | ||
| use super::rpc::rpc_call; | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Split wallet/execution.rs before it grows further.
This file now bundles quote storage, read tools, EVM signing/broadcast, and a sizable test harness into a single 1.2k+ LOC module. Please break it into smaller src/openhuman/wallet/ siblings before merge; it is already far past the repo's size cap.
As per coding guidelines "Rust modules should generally not exceed ~500 lines; split growing modules into smaller units".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/wallet/execution.rs` around lines 1 - 35, This module has grown
past the repo size cap and should be split into smaller sibling modules: extract
the quote/storage and related types into a new wallet/quotes.rs (move types and
functions that manage quote storage), move read-only helpers (balance/supporting
assets/network defaults) into wallet/read.rs (functions referencing find_asset,
default_networks, rpc_url_for_chain, explorer_tx_url), move EVM
signing/broadcast and prepare/execute logic into wallet/evm.rs (functions using
TypedTransaction, TransactionRequest, encode_erc20_transfer,
MnemonicBuilder/Signer, secret_material, rpc_call), and move test harnesses into
wallet/tests.rs; ensure public APIs used elsewhere (WalletAccount, WalletChain,
WalletAssetDefinition, wallet_status) keep their visibility and update module
declarations and use paths accordingly so imports compiling elsewhere continue
to reference the moved symbols.
Comment on lines
+381
to
+385
| let nonce_hex: String = rpc_call( | ||
| WalletChain::Evm, | ||
| "eth_getTransactionCount", | ||
| json!([from_address, "latest"]), | ||
| ) |
Contributor
There was a problem hiding this comment.
Use the pending nonce for outbound transactions.
Both nonce lookups use "latest", which ignores this wallet's pending txs. Two back-to-back sends can reuse the mined nonce and replace the first pending tx or get rejected as a duplicate. These requests should use "pending" when preparing a new send.
Suggested change
- json!([from_address, "latest"]),
+ json!([from_address, "pending"]),
...
- json!([quote.from_address, "latest"]),
+ json!([quote.from_address, "pending"]),Also applies to: 470-474
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/wallet/execution.rs` around lines 381 - 385, The nonce lookup
for outbound sends currently calls rpc_call(WalletChain::Evm,
"eth_getTransactionCount", json!([from_address, "latest"])) (stored in
nonce_hex) which ignores this wallet's pending transactions; update both places
where eth_getTransactionCount is invoked for preparing a new send (the nonce_hex
call and the similar call around lines 470-474) to use "pending" instead of
"latest" so the pending nonce is returned and consecutive sends won't reuse the
same nonce.
Comment on lines
+411
to
+427
| let signer = MnemonicBuilder::<English>::default() | ||
| .phrase(mnemonic.as_str()) | ||
| .derivation_path(&secret.derivation_path) | ||
| .map_err(|e| { | ||
| format!( | ||
| "invalid EVM derivation path '{}': {e}", | ||
| secret.derivation_path | ||
| ) | ||
| })? | ||
| .build() | ||
| .map_err(|e| format!("failed to derive EVM signer from wallet secret: {e}"))?; | ||
| let from = Address::from_str("e.from_address).map_err(|e| { | ||
| format!( | ||
| "invalid stored EVM sender address '{}': {e}", | ||
| quote.from_address | ||
| ) | ||
| })?; |
Contributor
There was a problem hiding this comment.
Abort if the derived signer address doesn't match the quote.
The sender on a signed EVM transaction comes from the signature, not request.from. If the wallet is reconfigured between prepare and execute, this path can sign with the current mnemonic while still using the old quote's from_address for nonce/estimation, which can fail unpredictably or spend from the wrong account. Compare signer.address() to quote.from_address before continuing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/wallet/execution.rs` around lines 411 - 427, After deriving the
EVM signer with MnemonicBuilder (and creating `signer`) and parsing
`quote.from_address` to `from` (Address::from_str), verify that the actual
signer address matches the quote sender before proceeding: call
`signer.address()` (or equivalent) and compare it to
`from`/`quote.from_address`; if they differ, return an error/abort (with a clear
message referencing the mismatch) rather than continuing to use the quote's
nonce/estimation or signing the transaction. Ensure this check occurs
immediately after the signer derivation and address parsing and before any
nonce/estimate or signing logic.
Comment on lines
+519
to
+524
| debug!( | ||
| "{LOG_PREFIX} execute_prepared quote_id={} chain=evm tx_hash={} rpc={}", | ||
| quote.quote_id, | ||
| tx_hash, | ||
| rpc_url_for_chain(WalletChain::Evm) | ||
| ); |
Contributor
There was a problem hiding this comment.
Redact RPC endpoints before surfacing them.
These paths expose rpc_url_for_chain(...) verbatim in both the API payload and debug logs. Env overrides commonly embed provider credentials in the URL, so this leaks secrets to agents and log sinks. Return a redacted form (or just a configured flag) and keep the full endpoint inside the transport layer.
As per coding guidelines "Never log secrets or full PII—redact."
Also applies to: 573-582
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/wallet/execution.rs` around lines 519 - 524, The debug logs and
API payloads currently include the full RPC endpoint from
rpc_url_for_chain(WalletChain::Evm) (seen in the debug! call in execute_prepared
and the later block at lines ~573-582); change these sites to log/return a
redacted form (e.g., redact_rpc_url or a flag-based stub) instead of the raw
URL, keep the full URL only inside the transport layer/where the HTTP client is
created, and update the code paths that build the API payload to use the
redacted value so credentials in env-overrides are never surfaced in logs or
responses.
Comment on lines
+835
to
+845
| let quote = get_quote(¶ms.quote_id)?; | ||
| let result = match quote.chain { | ||
| WalletChain::Evm => execute_evm_quote(quote).await?, | ||
| other => { | ||
| return Err(format!( | ||
| "on-chain execution is not implemented yet for chain '{}'; prepare-only mode remains active", | ||
| chain_str(other) | ||
| )); | ||
| } | ||
| }; | ||
| let _ = take_quote(¶ms.quote_id)?; |
Contributor
There was a problem hiding this comment.
Reserve the quote before awaiting broadcast.
The current get_quote() → async execute → take_quote() flow leaves a window where two callers can execute the same quote concurrently. Because the external side effect is eth_sendRawTransaction, that TOCTOU can double-submit the same intent, and take_quote() can also turn a successful broadcast into an error if the quote expires before the final removal. Make quote consumption/reservation atomic, then restore retryability only on failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/wallet/execution.rs` around lines 835 - 845, The current flow
calls get_quote(¶ms.quote_id) then awaits execute_evm_quote(...) and only
after that calls take_quote(...), which risks TOCTOU double-submits; change the
flow to atomically reserve the quote before awaiting the async broadcast:
introduce or use a reservation API (e.g., reserve_quote(params.quote_id) or
convert take_quote to a two-step API reserve_quote/take_quote) so that you mark
the quote as reserved/consumed before calling execute_evm_quote(quote). Then,
after execute_evm_quote returns success, finalize removal
(take_quote/finalize_quote); on any failure (broadcast error, timeouts, etc.)
release the reservation (e.g., restore_quote or unreserve_quote) so the quote
remains retryable. Ensure reservation/unreserve calls reference get_quote,
reserve_quote/take_quote, execute_evm_quote, and params.quote_id and that
reservation happens synchronously before the await.
Comment on lines
+543
to
+562
| let result = match method { | ||
| "eth_chainId" => Value::String("0x1".to_string()), | ||
| "eth_getTransactionCount" => Value::String("0x7".to_string()), | ||
| "eth_gasPrice" => Value::String("0x3b9aca00".to_string()), | ||
| "eth_estimateGas" => Value::String("0x5208".to_string()), | ||
| "eth_sendRawTransaction" => { | ||
| if let Some(raw) = params.first().and_then(Value::as_str) { | ||
| match state.raw_txs.lock() { | ||
| Ok(mut guard) => guard.push(raw.to_string()), | ||
| Err(poisoned) => poisoned.into_inner().push(raw.to_string()), | ||
| } | ||
| } | ||
| Value::String( | ||
| "0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".to_string(), | ||
| ) | ||
| } | ||
| "eth_getBalance" => Value::String("0x0".to_string()), | ||
| _ => Value::Null, | ||
| }; | ||
| Json(json!({"jsonrpc":"2.0","id":1,"result":result})) |
Contributor
There was a problem hiding this comment.
Fail fast on unexpected upstream RPC methods.
The default arm returns result: null, so this mock can silently accept contract drift if wallet_execute_prepared starts calling a new JSON-RPC method. Returning a JSON-RPC error here makes the test break at the boundary instead of masking the mismatch.
Suggested change
- _ => Value::Null,
+ _ => {
+ return Json(json!({
+ "jsonrpc": "2.0",
+ "id": payload.get("id").cloned().unwrap_or(Value::Null),
+ "error": {
+ "code": -32601,
+ "message": format!("unexpected mock wallet RPC method: {method}"),
+ }
+ }));
+ }
};
- Json(json!({"jsonrpc":"2.0","id":1,"result":result}))
+ Json(json!({
+ "jsonrpc": "2.0",
+ "id": payload.get("id").cloned().unwrap_or(Value::Null),
+ "result": result
+ }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/json_rpc_e2e.rs` around lines 543 - 562, The mock's match on `method`
currently sets `result` to Value::Null in the `_` arm which hides unexpected
upstream RPC calls; change the default arm in the `match method { ... _ => ...
}` so it returns a JSON-RPC error response instead of `Value::Null` (e.g., an
error object with code -32601 and a message that includes the unexpected
`method`), and adjust the final return from `Json(json!(...))` accordingly so
tests fail fast when `wallet_execute_prepared` invokes an unhandled RPC method.
Comment on lines
+2688
to
+2692
| assert_eq!( | ||
| list.len(), | ||
| 8, | ||
| "expected four native assets plus EVM tokens: {result}" | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid pinning the full default asset catalog size.
The ETH/USDC checks below already verify the behavior that matters. assert_eq!(list.len(), 8) will fail on benign catalog additions and make this transport test noisier than it needs to be.
Suggested change
- assert_eq!(
- list.len(),
- 8,
- "expected four native assets plus EVM tokens: {result}"
- );
+ assert!(
+ list.len() >= 5,
+ "expected native assets plus at least one default EVM token: {result}"
+ );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/json_rpc_e2e.rs` around lines 2688 - 2692, The test pins the asset
catalog size with assert_eq!(list.len(), 8) which is brittle; instead remove or
relax this exact-length assertion in tests/json_rpc_e2e.rs and rely on the
existing ETH/USDC presence checks (the subsequent assertions that verify
specific tokens) to validate correct behavior — locate the
assert_eq!(list.len(), 8, "expected four native assets plus EVM tokens:
{result}") and either delete it or replace it with a non-exact check (e.g.,
ensure list.len() >= expected_minimum) so benign catalog additions won't break
the transport test.
This was referenced May 23, 2026
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
upstream/maininto this branch and resolve the harness conflict by keeping both upstream smart-mock coverage and the branch crypto-wallet flow testsuint256ERC-20 encoding, and recovery-phrase persistence validationProblem
mainand hit a real merge conflict in the agent harness tests, blocking shipmentSolution
src/openhuman/wallet/so chain URLs, explorer bases, and default assets are centralizedwallet_execute_prepared, backed by encrypted mnemonic storage captured during wallet setupwallet_network_defaultsandwallet_encode_erc20_transferso agents can reason about defaults and inspect token calldata without inventing ABI detailsSubmission Checklist
docs/TEST-COVERAGE-MATRIX.md## Related— N/A: wallet execution/tooling does not map to an existing matrix feature row yetdocs/RELEASE-MANUAL-SMOKE.md) — N/A: does not touch installer/update/release-cut flowsCloses #NNNin the## Relatedsection — N/A: no tracked issue exists for this branchImpact
ethers-core/ethers-signersfor EVM signing and ABI encodingRelated
docs/TEST-COVERAGE-MATRIX.mdAI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
e2e-wallet-mock-harnesse375c8f9Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo test test_support_test --libGGML_NATIVE=OFF cargo test wallet:: --libGGML_NATIVE=OFF cargo test crypto_agent_has_narrow_wallet_market_tools_and_safety_on --libpnpm exec vitest run --config test/vitest.config.ts src/services/walletApi.test.ts src/features/wallet/setupLocalWalletFromMnemonic.test.ts src/components/settings/panels/__tests__/ConnectionsPanel.test.tsx src/components/settings/panels/__tests__/RecoveryPhrasePanel.test.tsx test/Mnemonic.test.tsxcargo fmt --check,cargo check --manifest-path app/src-tauri/Cargo.toml, plus localcargo fmt --allcargo check --manifest-path app/src-tauri/Cargo.tomlValidation Blocked
command:pnpm test:coverage/ full merged coverage gate runerror:not run locally in full; relying on CI for merged diff-cover verificationimpact:CI coverage jobs still need to confirm changed-line coverage on the full PRBehavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Tests