Repository navigation
feat(wallet): complete multi-chain signing + broadcast (EVM/BTC/Solana/Tron) - #2519
Merged
senamakel merged 5 commits intoMay 23, 2026
Merged
Conversation
…lana/Tron
Extend the wallet domain from EVM-only to full sign+broadcast across four
chain families, plus a multi-network EVM stack (Ethereum + Base + Arbitrum
+ Optimism + Polygon). Adds chain-specific signing primitives, mockable
HTTP transport, deterministic test vectors, and full prepare→execute JSON-RPC
E2E coverage.
## Surface
- **EVM** (chains/evm.rs): single key path, `evmNetwork` parameter routes to
Ethereum / Base / Arbitrum / Optimism / Polygon RPC endpoints with chain_id
cross-check on execute to refuse to sign across network mismatches.
- **Bitcoin** (chains/btc.rs): BIP84 P2WPKH derivation, Esplora REST for
UTXO/balance/broadcast, segwit (BIP143) sighash + ECDSA signing, change
output above dust.
- **Solana** (chains/solana.rs): SLIP-0010 ed25519 derivation, hand-built
legacy message + shortvec wire format (no solana-sdk dep), ed25519
signing, native SOL + SPL token transfer via on-curve check for ATA
derivation (find_program_address).
- **Tron** (chains/tron.rs): BIP44 secp256k1 derivation, TronGrid REST
(createtransaction / triggersmartcontract / broadcasttransaction),
TRC20 ABI parameter encoding, recoverable ECDSA signature, surfaces node
rejection codes.
## Test coverage
- 63 wallet unit tests (was 23). Per-chain: address validation, key
derivation deterministic vectors from the standard test mnemonic, full
sign + broadcast against an axum mock per chain, error paths.
- 8 JSON-RPC E2E tests (was 2): wallet_setup, execution_surface,
evm_base_network_prepare_execute, btc_prepare_execute, solana_prepare_execute,
tron_prepare_execute, tron_trc20_prepare_execute, network_defaults_lists_all_chains.
- Shared test_support module exposes the canonical "abandon × 11 about"
mnemonic + derived addresses so chain-specific tests don't re-derive.
## Dependencies
- bitcoin 0.32 — P2WPKH signing (includes secp256k1).
- ed25519-dalek 2 — Solana signing.
- bs58 0.5 (check feature) — Solana/Tron base58.
- coins-bip39 0.8 — shared mnemonic→seed (already in tree via ethers-signers).
- curve25519-dalek 4 — Solana ATA off-curve check.
- ripemd 0.1 — RIPEMD160 hashes.
Per CLAUDE.md the wallet domain stays under src/openhuman/wallet/ with the
controller-registry pattern preserved; the new per-chain executors live in
chains/{evm,btc,solana,tron}.rs and feed a thin dispatcher in execution.rs.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds per-chain wallet executors (BTC P2WPKH, Solana native/SPL, Tron TRX/TRC20), refactors EVM to be network-aware, centralizes HTTP RPC/REST transport, threads optional evm_network through prepare/execute models, updates defaults and asset catalogs, and expands tests/E2E mocks. ChangesMulti-chain wallet execution
Sequence DiagramsequenceDiagram
participant API
participant Execution as execution.rs
participant Defaults as defaults.rs
participant BTC as chains/btc.rs
participant EVM as chains/evm.rs
participant Solana as chains/solana.rs
participant Tron as chains/tron.rs
API->>Execution: prepare_transfer(chain, evmNetwork)
Execution->>Defaults: evm_asset_catalog(network)
API->>Execution: execute_prepared(quote)
alt chain == Bitcoin
Execution->>BTC: execute_btc_quote()
BTC->>BTC: derive key, select UTXOs, sign, broadcast
else chain == EVM
Execution->>EVM: execute_evm_quote()
EVM->>EVM: derive signer, fetch nonce/gas/chainId, sign, broadcast
else chain == Solana
Execution->>Solana: execute_solana_quote()
Solana->>Solana: derive ed25519 key, build message, sign, sendTransaction
else chain == Tron
Execution->>Tron: execute_tron_quote()
Tron->>Tron: derive secp256k1 key, encode params, sign sha256, broadcast
end
Execution->>API: return ExecutionResult (tx hash / explorer URL)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. Comment |
coderabbitai
Bot
added
the
feature
![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: 10
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/rpc.rs (1)
84-93: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit debug/trace logs on error branches before returning
Err.You already log RPC entry/exit, but non-success HTTP and JSON-RPC error branches currently return without branch-local diagnostics. Add a debug/trace line in each error path with method + redacted URL to align diagnostics across transports.
As per coding guidelines: “Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone.”Also applies to: 115-118, 148-151, 170-173
🤖 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/rpc.rs` around lines 84 - 93, Before each early return in the RPC error branches (the HTTP non-success branch using status.is_success(), the JSON decode error from serde_json::from_str, and the JSON-RPC error branch that inspects body.get("error")), add a debug/trace log that includes the method and a redacted form of the request URL (e.g., call an existing redact helper or add one like redact_url(&url) if none exists) and any safe error snippet (not raw credentials); use log::debug! or tracing::trace! immediately before the Err(...) return so each branch logs method + redacted URL and a short error context; apply the same change to the other similar blocks mentioned (lines ~115-118, ~148-151, ~170-173) referencing the same symbols (status.is_success(), serde_json::from_str, body.get("error")).src/openhuman/wallet/execution.rs (1)
467-535:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReturn EVM balances per network, not just Ethereum mainnet.
This branch hardcodes
EvmNetwork::EthereumMainnetfor both the RPC call and the emittedevm_network. After this PR,supported_assets()andchain_status()enumerate every EVM network, butbalances()still hides Base/Arbitrum/Optimism/Polygon funds.🤖 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 467 - 535, The EVM branch is hardcoded to EvmNetwork::EthereumMainnet; update the WalletChain::Evm match to destructure/extract the actual EVM network from account.chain (match WalletChain::Evm(network) => ...) and pass that network into chain_evm::evm_balance(...) and set the resulting BalanceInfo.evm_network to Some(network) instead of always Some(EvmNetwork::EthereumMainnet); ensure you update any equality check that set evm_network (the later if account.chain == WalletChain::Evm) to use the extracted network value so per-network balances (Base/Arbitrum/Optimism/Polygon/etc.) are returned.
🧹 Nitpick comments (5)
src/openhuman/wallet/chains/tron.rs (1)
55-66: ⚡ Quick winAdd structured logs around TronGrid calls and rejection branches.
This module only emits a success log after broadcast.
getaccount,createtransaction,triggersmartcontract, and the rejection path at Lines 324-332 currently lose the method/address/quote context needed to diagnose node-side failures in production.As per coding guidelines "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."Also applies to: 199-239, 319-344
🤖 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/chains/tron.rs` around lines 55 - 66, The native_balance function lacks structured logs around the Tron RPC call and error branches; add debug/trace logs at entry (including the address and that validate_tron_address passed), before the RPC call (include method "getaccount", target URL and hex address from tron_address_to_hex), after a successful response showing returned balance, and an error log on any early returns or on rest_post_json failures; use the existing functions/native symbols (native_balance, validate_tron_address, rpc_url_for_chain, tron_address_to_hex, rest_post_json) and emit structured logs with context fields like method="getaccount", address, hex_address, url, and balance so failures can be diagnosed.src/openhuman/wallet/chains/solana.rs (1)
78-87: ⚡ Quick winAdd RPC entry/error logs for blockhash and sendTransaction.
The module only logs the success path after broadcast. When
getBalance,getLatestBlockhash, orsendTransactionfails, you lose the method/address/quote context needed to debug provider issues.As per coding guidelines "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."Also applies to: 296-315, 317-383
🤖 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/chains/solana.rs` around lines 78 - 87, The functions calling Solana RPCs (e.g., native_balance and the routines handling getLatestBlockhash and sendTransaction) lack entry/exit and error logs, which makes debugging provider failures hard; update each RPC caller (references: native_balance, the getLatestBlockhash handler and sendTransaction path in this module) to log at debug/trace level when an RPC call is about to be made (include RPC method name like "getBalance"/"getLatestBlockhash"/"sendTransaction", the target address or transaction/quote id where applicable), log the successful response on exit, and log errors with full context (method, address/quote, and the returned error) before returning Err so callers can trace failures. Ensure logs use the crate's logging/tracing facility and include enough contextual fields to correlate requests and responses.src/openhuman/wallet/chains/evm.rs (1)
32-35: ⚡ Quick winAdd structured logs around nonce/gas/sendRawTransaction RPCs.
The only log here is the happy-path broadcast. Chain ID mismatches, nonce fetch failures, gas estimation failures, and RPC rejects currently lose the method/network/quote context the repo asks for.
As per coding guidelines "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."Also applies to: 104-165
🤖 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/chains/evm.rs` around lines 32 - 35, Add structured debug/trace logs around RPC entry/exit and error branches for EVM RPC interactions: instrument evm_balance (and the other EVM RPC wrappers in the same file, e.g., functions that call evm_rpc_call, nonce/gas/estimate/sendRawTransaction between lines ~104-165) to log method name ("eth_getBalance", nonce, gas estimate, sendRawTransaction), the EvmNetwork value, the address/tx hash/quote context, and the raw RPC response on success, and include the error/response body on failure; specifically, before calling evm_rpc_call log an entry with method and params, on successful hex_to_u256 conversion log the parsed U256 and method/network, and on any ?-propagated error capture and log the error with method/network/quote info to retain context for debugging.src/openhuman/wallet/chains/btc.rs (1)
84-109: ⚡ Quick winAdd structured logs around the Esplora RPCs.
native_balance(),fetch_utxos(), andbroadcast_raw_hex()only log the success path after broadcast. When Esplora calls fail, there is no method/address/quote context in the logs to explain which provider call broke.As per coding guidelines "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."Also applies to: 176-277
🤖 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/chains/btc.rs` around lines 84 - 109, Add structured tracing around Esplora RPC calls: instrument native_balance, fetch_utxos, and broadcast_raw_hex (and other Esplora RPC helpers in this file) with trace/debug logs on entry (include method name and key params: address or tx_hex), trace/debug on successful exit with returned info (e.g., confirmed/pending or UTXO count or txid), and error logs that include the method name, address/tx_hex context and the error string when rest_get_json/rest_post_text fail; use the existing logging/tracing crate conventions (trace/debug for entry/exit, error for failures) and ensure messages are structured (key/value pairs) so callers can correlate which provider and call failed.tests/json_rpc_e2e.rs (1)
3176-3281: ⚡ Quick winReturn the mock server handles so the tests can shut them down.
Each
start_mock_*helper drops theJoinHandlefromserve_on_ephemeral, so every new chain test leaves an axum server alive until process exit. That leaks tasks/listeners across the binary and makes later failures harder to reason about.🤖 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 3176 - 3281, The helpers (start_mock_evm_with_chain_id, start_mock_btc, start_mock_solana, start_mock_tron) currently drop the JoinHandle returned by serve_on_ephemeral, leaking running axum servers; change each helper to return that JoinHandle (alongside the SocketAddr and any broadcast Arc) so callers can shut servers down (abort() or await the handle). Locate serve_on_ephemeral usage in start_mock_evm_with_chain_id, start_mock_btc, start_mock_solana, start_mock_tron and modify their signatures/returns to include the JoinHandle type returned by serve_on_ephemeral, propagate the handle out, and update call sites/tests to call .abort() or .await on the handle to stop the server after each test.
🤖 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/chains/btc.rs`:
- Around line 63-79: The current validate_btc_address() enforces is_p2wpkh() and
is used for both from_address and to_address, which wrongly rejects valid
recipient types (e.g., Taproot bc1p or P2SH); split sender vs recipient
validation: keep a strict validate_btc_sender() (or validate_btc_from_address)
that performs the P2WPKH check on the parsed Address, and change
validate_btc_address() (or create validate_btc_recipient()) used for to_address
to only trim, parse, require mainnet via
Address::from_str(...).require_network(Network::Bitcoin) and return the trimmed
string without the is_p2wpkh() restriction so script_pubkey_for_addr() can
encode other valid script types; update call sites (from_address -> sender
validator, to_address -> recipient validator).
In `@src/openhuman/wallet/chains/evm.rs`:
- Around line 105-110: The nonce fetch uses eth_getTransactionCount with
"latest", which ignores pending transactions and can cause nonce collisions;
update the rpc_call_to invocation that assigns nonce_hex (the
eth_getTransactionCount call using &rpc_url and quote.from_address) to request
"pending" instead of "latest" so the returned nonce accounts for
already-submitted pending txs.
In `@src/openhuman/wallet/chains/solana.rs`:
- Around line 353-361: In the PreparedKind::TokenTransfer match arm (where you
compute mint via b58_to_pubkey, derive src_ata and dst_ata and call
build_spl_transfer_message), add logic to ensure the destination ATA exists
before creating the Transfer instruction: query the chain/state for dst_ata
existence and if it does not exist either prepend an associated token account
creation instruction to the message (create_associated_token_account for
dst_ata) or return a clear preflight error (e.g., "destination ATA not found for
mint <mint>") instead of blindly building the SPL Transfer; update the code
paths that call build_spl_transfer_message to accept/handle an initial ATA
creation instruction or the early error so first-time transfers to wallets
without an ATA succeed or fail with a clear message.
In `@src/openhuman/wallet/execution.rs`:
- Around line 767-787: The current flow uses get_quote(...) and only calls
take_quote(...) after broadcasting, allowing two concurrent callers to both get
the same quote and double-broadcast; fix it by atomically removing/marking the
quote as taken before sending: call take_quote(¶ms.quote_id) (use the
returned quote for the chain calls instead of get_quote/clone) to ensure only
one broadcaster proceeds, then perform the chain-specific calls
(execute_evm_quote / execute_btc_quote / execute_solana_quote /
execute_tron_quote) on that taken quote; if a broadcast fails and you need the
quote to remain retryable, restore it (via your quote-insert/restore helper) in
the error path so callers can retry, but do not delay removal until after the
chain call.
In `@src/openhuman/wallet/rpc.rs`:
- Around line 27-32: Replace the per-call client() builder with a process-wide
lazy reqwest::Client created via once_cell::sync::Lazy (or OnceCell) so all
RPC/REST calls reuse the same connection pool; modify the existing client()
function to return the shared client (or a reference) instead of building each
time. Additionally, add log::debug!/log::trace! statements (using the
"[wallet::rpc]" prefix and include method/URL host) immediately before every Err
return in transport failure branches, non-2xx HTTP status handling, RPC response
error cases, and JSON decode/missing-result branches so the error path logs
context before returning the formatted error string. Ensure references to the
client creation live in the client() function and the error logging is added in
the functions that call client() and process responses (the RPC/REST
request/response handling code).
In `@src/openhuman/wallet/schemas.rs`:
- Around line 239-244: The new FieldSchema "evmNetwork" was added to the
controller but the wallet_prepare_transfer tool params (wallet_prepare_transfer)
still reject unknown fields via additionalProperties: false and don't declare
evmNetwork, so calls through that tool can't use it; update the tool's parameter
schema in the prepare_transfer implementation (the schema construction for
wallet_prepare_transfer) to include an optional "evmNetwork" field with the same
type/shape (Option<String>/optional string) and the matching comment, ensuring
it is declared alongside the other fields so additionalProperties can remain
false while the new field is accepted.
In `@src/openhuman/wallet/test_support.rs`:
- Around line 55-77: setup_wallet_in currently mutates OPENHUMAN_WORKSPACE and
never restores it, causing test-order dependency; wrap the env change in a guard
that captures the previous value (std::env::var_os("OPENHUMAN_WORKSPACE")) and
on Drop restores it (if Some then std::env::set_var, else std::env::remove_var)
so restoration runs even if the async function returns early or errors;
implement a small local struct (e.g., WorkspaceEnvGuard) with a Drop impl, set
the env before creating the guard, and keep existing logic (encrypt_secret,
setup) unchanged so cleanup is deterministic.
In `@tests/json_rpc_e2e.rs`:
- Around line 3253-3281: The test currently doesn't verify which Tron endpoint
was called; modify the mock handlers (mock_tron_create and mock_tron_trigger) in
tests/json_rpc_e2e.rs to record per-route hits or capture the incoming request
body (e.g., by incrementing a shared Arc<Mutex<HashMap>> or pushing to a shared
Vec) created in start_mock_tron, then in the test
json_rpc_wallet_tron_trc20_prepare_execute_round_trips assert that
mock_tron_trigger was invoked (and the native TRX test asserts mock_tron_create
was invoked); also update scripts/test-rust-with-mock.sh if it runs any
RPC-method whitelist so the new recorded RPCs are included. Ensure you reference
the shared tracker from start_mock_tron so both handlers can update it and the
test can read and assert on it after the call.
- Around line 3195-3222: Update the BTC mock to validate the requested address
and broadcast payload: in mock_btc_utxo extract the path param (addr) from the
request (e.g., accept Path<String> or use axum::extract::Path) and assert it
equals E2E_TEST_MNEMONIC_ADDRS_BTC before returning state.utxo; in
mock_btc_broadcast assert the incoming body is non-empty and valid hex (or at
least matches r"^[0-9a-fA-F]+$") before pushing to state.broadcast_txs,
preserving the existing mutex push logic; apply the same changes for the other
BTC mock at the second location referenced (lines ~3387-3451) so tests fail if
the wrong address or a non-hex/empty tx is used.
- Around line 2965-2977: Replace the loose ">= 8" and single-base check with an
explicit assertion that the catalog contains the exact expected network
identifiers: iterate over the expected network id string list (e.g. the set of
EVM networks and non-EVM chains used by the UI) and assert list.iter().any(|a|
a.get("evmNetwork").and_then(Value::as_str) == Some(expected)) for each expected
id; update the same pattern in the other block referenced (lines 3665-3707) to
pin the full multi-chain catalog instead of a lower bound; also update
scripts/test-rust-with-mock.sh to add/verify the new RPC method names used by
the UI so the test harness returns the pinned responses the tests expect.
---
Outside diff comments:
In `@src/openhuman/wallet/execution.rs`:
- Around line 467-535: The EVM branch is hardcoded to
EvmNetwork::EthereumMainnet; update the WalletChain::Evm match to
destructure/extract the actual EVM network from account.chain (match
WalletChain::Evm(network) => ...) and pass that network into
chain_evm::evm_balance(...) and set the resulting BalanceInfo.evm_network to
Some(network) instead of always Some(EvmNetwork::EthereumMainnet); ensure you
update any equality check that set evm_network (the later if account.chain ==
WalletChain::Evm) to use the extracted network value so per-network balances
(Base/Arbitrum/Optimism/Polygon/etc.) are returned.
In `@src/openhuman/wallet/rpc.rs`:
- Around line 84-93: Before each early return in the RPC error branches (the
HTTP non-success branch using status.is_success(), the JSON decode error from
serde_json::from_str, and the JSON-RPC error branch that inspects
body.get("error")), add a debug/trace log that includes the method and a
redacted form of the request URL (e.g., call an existing redact helper or add
one like redact_url(&url) if none exists) and any safe error snippet (not raw
credentials); use log::debug! or tracing::trace! immediately before the Err(...)
return so each branch logs method + redacted URL and a short error context;
apply the same change to the other similar blocks mentioned (lines ~115-118,
~148-151, ~170-173) referencing the same symbols (status.is_success(),
serde_json::from_str, body.get("error")).
---
Nitpick comments:
In `@src/openhuman/wallet/chains/btc.rs`:
- Around line 84-109: Add structured tracing around Esplora RPC calls:
instrument native_balance, fetch_utxos, and broadcast_raw_hex (and other Esplora
RPC helpers in this file) with trace/debug logs on entry (include method name
and key params: address or tx_hex), trace/debug on successful exit with returned
info (e.g., confirmed/pending or UTXO count or txid), and error logs that
include the method name, address/tx_hex context and the error string when
rest_get_json/rest_post_text fail; use the existing logging/tracing crate
conventions (trace/debug for entry/exit, error for failures) and ensure messages
are structured (key/value pairs) so callers can correlate which provider and
call failed.
In `@src/openhuman/wallet/chains/evm.rs`:
- Around line 32-35: Add structured debug/trace logs around RPC entry/exit and
error branches for EVM RPC interactions: instrument evm_balance (and the other
EVM RPC wrappers in the same file, e.g., functions that call evm_rpc_call,
nonce/gas/estimate/sendRawTransaction between lines ~104-165) to log method name
("eth_getBalance", nonce, gas estimate, sendRawTransaction), the EvmNetwork
value, the address/tx hash/quote context, and the raw RPC response on success,
and include the error/response body on failure; specifically, before calling
evm_rpc_call log an entry with method and params, on successful hex_to_u256
conversion log the parsed U256 and method/network, and on any ?-propagated error
capture and log the error with method/network/quote info to retain context for
debugging.
In `@src/openhuman/wallet/chains/solana.rs`:
- Around line 78-87: The functions calling Solana RPCs (e.g., native_balance and
the routines handling getLatestBlockhash and sendTransaction) lack entry/exit
and error logs, which makes debugging provider failures hard; update each RPC
caller (references: native_balance, the getLatestBlockhash handler and
sendTransaction path in this module) to log at debug/trace level when an RPC
call is about to be made (include RPC method name like
"getBalance"/"getLatestBlockhash"/"sendTransaction", the target address or
transaction/quote id where applicable), log the successful response on exit, and
log errors with full context (method, address/quote, and the returned error)
before returning Err so callers can trace failures. Ensure logs use the crate's
logging/tracing facility and include enough contextual fields to correlate
requests and responses.
In `@src/openhuman/wallet/chains/tron.rs`:
- Around line 55-66: The native_balance function lacks structured logs around
the Tron RPC call and error branches; add debug/trace logs at entry (including
the address and that validate_tron_address passed), before the RPC call (include
method "getaccount", target URL and hex address from tron_address_to_hex), after
a successful response showing returned balance, and an error log on any early
returns or on rest_post_json failures; use the existing functions/native symbols
(native_balance, validate_tron_address, rpc_url_for_chain, tron_address_to_hex,
rest_post_json) and emit structured logs with context fields like
method="getaccount", address, hex_address, url, and balance so failures can be
diagnosed.
In `@tests/json_rpc_e2e.rs`:
- Around line 3176-3281: The helpers (start_mock_evm_with_chain_id,
start_mock_btc, start_mock_solana, start_mock_tron) currently drop the
JoinHandle returned by serve_on_ephemeral, leaking running axum servers; change
each helper to return that JoinHandle (alongside the SocketAddr and any
broadcast Arc) so callers can shut servers down (abort() or await the handle).
Locate serve_on_ephemeral usage in start_mock_evm_with_chain_id, start_mock_btc,
start_mock_solana, start_mock_tron and modify their signatures/returns to
include the JoinHandle type returned by serve_on_ephemeral, propagate the handle
out, and update call sites/tests to call .abort() or .await on the handle to
stop the server after each test.
🪄 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: 408d29f4-644f-4d92-8975-165657067b3f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsrc/openhuman/wallet/chains/btc.rssrc/openhuman/wallet/chains/evm.rssrc/openhuman/wallet/chains/mod.rssrc/openhuman/wallet/chains/solana.rssrc/openhuman/wallet/chains/tron.rssrc/openhuman/wallet/defaults.rssrc/openhuman/wallet/execution.rssrc/openhuman/wallet/mod.rssrc/openhuman/wallet/rpc.rssrc/openhuman/wallet/schemas.rssrc/openhuman/wallet/test_support.rstests/json_rpc_e2e.rs
- execute_prepared: take quote atomically before broadcasting; restore on failure. Prevents two concurrent confirmations from double-submitting the same quote. - EVM signer: use eth_getTransactionCount(..., "pending") so back-to-back confirmations don't collide on the same nonce. - BTC validation: split sender (P2WPKH required for signing) from recipient (any mainnet address — P2TR/P2SH recipients now accepted). - Solana SPL: preflight getAccountInfo on the destination ATA so we fail loudly with a clear error instead of burning the broadcast when the ATA doesn't exist yet. New unit test for the missing-ATA path. - wallet/rpc: share a process-wide reqwest::Client via once_cell::Lazy so connection pooling stays hot across calls. - wallet_prepare_transfer tool schema: declare evmNetwork so tool callers can target Base/Arbitrum/Optimism/Polygon (previously additionalProperties: false silently dropped it). - E2E tests: pin every expected EVM L2 + chain_id in network_defaults and supported_assets; assert BTC mock is queried for the BIP84-derived sender address; assert Tron native vs TRC20 hit the correct REST endpoints (no silent regression onto the wrong path); assert BTC broadcast body is hex. - test_support: expose WorkspaceEnvGuard so callers can opt in to env restoration; documented why the helper itself can't restore eagerly. 64 wallet unit tests + 8 wallet E2E tests pass.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/chains/btc.rs (1)
200-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify the derived signer matches
from_address.
validate_btc_sender_address()only provesfrom_addressis a mainnet P2WPKH string. If the stored derivation path/address pair drifts, this path will fetch UTXOs for one address and sign with another key, which turns into a late broadcast rejection instead of a clear early failure. Compare the derived sender address tofrom_addrright after key derivation and abort on mismatch.🐛 Proposed fix
let (private_key, public_key) = derive_btc_private_key(&mnemonic, &secret.derivation_path)?; + let derived_from = bitcoin::Address::p2wpkh(&public_key, Network::Bitcoin).to_string(); + if derived_from != from_addr { + return Err(format!( + "BTC key derivation mismatch: derived {} but expected {}", + derived_from, from_addr + )); + } let from_spk = script_pubkey_for_addr(&from_addr)?;🤖 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/chains/btc.rs` around lines 200 - 219, After deriving keys in derive_btc_private_key (right after calling derive_btc_private_key(&mnemonic, &secret.derivation_path)), compute the Bitcoin address corresponding to the derived public_key (same format as validate_btc_sender_address expects, e.g., mainnet P2WPKH) and compare it to from_addr; if they differ, return an Err with a clear message (e.g., "derived address X does not match requested from_addr Y") so the function fails early rather than signing with the wrong key. Ensure you use the same address-encoding logic used elsewhere (e.g., script_pubkey_for_addr/address-from-scriptPubKey helper) to generate the derived address for a correct comparison.
🧹 Nitpick comments (2)
src/openhuman/wallet/chains/solana.rs (1)
378-388: ⚡ Quick winLog the ATA preflight miss before returning.
This new branch is exactly the kind of hard-to-infer failure path the wallet logging guideline calls out. Emit a
debug!/trace!line withquote_id, mint, owner, and derived ATA before theErr(...)so these user-visible preflight failures are diagnosable.♻️ Proposed fix
if !account_exists(&pubkey_to_b58(&dst_ata)).await? { + debug!( + "{LOG_PREFIX} spl_preflight_missing_ata quote_id={} mint={} owner={} ata={}", + quote.quote_id, + mint_addr, + pubkey_to_b58(&to_pubkey), + pubkey_to_b58(&dst_ata) + ); return Err(format!( "SPL preflight: destination Associated Token Account does not exist for mint {} owner {}; create it before transferring", mint_addr, pubkey_to_b58(&to_pubkey) ));As per coding guidelines, "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 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/chains/solana.rs` around lines 378 - 388, Before returning the error in the SPL preflight branch (the block that checks account_exists(&pubkey_to_b58(&dst_ata)) and currently returns Err(...)), emit a debug/trace log that includes quote_id, mint_addr, pubkey_to_b58(&to_pubkey) (owner) and pubkey_to_b58(&dst_ata) (derived ATA). Locate the branch around the account_exists call in the same function and add a call to debug! or trace! with those named fields immediately before the Err(...) return so the failure is recorded for diagnostics.src/openhuman/wallet/execution.rs (1)
767-786: ⚡ Quick winAdd debug logs around the take/dispatch/restore lifecycle.
This branch now owns the hardest-to-reconstruct quote state transitions in the module. A small debug/trace log on entry and success would make confirmation-race and failed-broadcast reports much easier to diagnose.
🪵 Example
let quote = take_quote(¶ms.quote_id)?; let chain = quote.chain; let restorable = quote.clone(); + debug!( + "{LOG_PREFIX} execute_prepared start chain={} quote_id={} kind={:?}", + chain_str(chain), + quote.quote_id, + quote.kind + ); let result = match chain { WalletChain::Evm => chain_evm::execute_evm_quote(quote).await, WalletChain::Btc => chain_btc::execute_btc_quote(quote).await, WalletChain::Solana => chain_sol::execute_solana_quote(quote).await, WalletChain::Tron => chain_tron::execute_tron_quote(quote).await, @@ return Err(error); } }; + debug!( + "{LOG_PREFIX} execute_prepared success chain={} quote_id={} tx_hash={}", + chain_str(chain), + result.quote_id, + result.transaction_hash + );As per coding guidelines, "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 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 767 - 786, Add debug/trace logs around the take/dispatch/restore lifecycle: log a trace/debug right after take_quote(¶ms.quote_id) with the quote_id and chain, another debug/trace on dispatch entry before calling chain_evm::execute_evm_quote / chain_btc::execute_btc_quote / chain_sol::execute_solana_quote / chain_tron::execute_tron_quote, a debug on successful result before returning the value, and a debug/trace when restoring via store_quote(restorable) (include quote_id, chain_str(chain) and any error string). Use the module's existing logging macros/levels (debug or trace for entry/exit, debug for state transitions) and ensure messages reference quote_id and chain to aid debugging.
🤖 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 782-783: When restoring a failed quote, refresh its TTL before
persisting so it isn't immediately expired: update the restored quote's
expires_at_ms to now + the quote TTL (e.g., QUOTE_TTL_MS or 5 minutes) on the
restorable object and then call store_quote(restorable); in other words, set
restorable.expires_at_ms = current_time_ms() + QUOTE_TTL_MS (or invoke an
existing helper like refresh_quote_ttl(restorable) if available) prior to
store_quote(restorable) in the restore/failure path.
---
Outside diff comments:
In `@src/openhuman/wallet/chains/btc.rs`:
- Around line 200-219: After deriving keys in derive_btc_private_key (right
after calling derive_btc_private_key(&mnemonic, &secret.derivation_path)),
compute the Bitcoin address corresponding to the derived public_key (same format
as validate_btc_sender_address expects, e.g., mainnet P2WPKH) and compare it to
from_addr; if they differ, return an Err with a clear message (e.g., "derived
address X does not match requested from_addr Y") so the function fails early
rather than signing with the wrong key. Ensure you use the same address-encoding
logic used elsewhere (e.g., script_pubkey_for_addr/address-from-scriptPubKey
helper) to generate the derived address for a correct comparison.
---
Nitpick comments:
In `@src/openhuman/wallet/chains/solana.rs`:
- Around line 378-388: Before returning the error in the SPL preflight branch
(the block that checks account_exists(&pubkey_to_b58(&dst_ata)) and currently
returns Err(...)), emit a debug/trace log that includes quote_id, mint_addr,
pubkey_to_b58(&to_pubkey) (owner) and pubkey_to_b58(&dst_ata) (derived ATA).
Locate the branch around the account_exists call in the same function and add a
call to debug! or trace! with those named fields immediately before the Err(...)
return so the failure is recorded for diagnostics.
In `@src/openhuman/wallet/execution.rs`:
- Around line 767-786: Add debug/trace logs around the take/dispatch/restore
lifecycle: log a trace/debug right after take_quote(¶ms.quote_id) with the
quote_id and chain, another debug/trace on dispatch entry before calling
chain_evm::execute_evm_quote / chain_btc::execute_btc_quote /
chain_sol::execute_solana_quote / chain_tron::execute_tron_quote, a debug on
successful result before returning the value, and a debug/trace when restoring
via store_quote(restorable) (include quote_id, chain_str(chain) and any error
string). Use the module's existing logging macros/levels (debug or trace for
entry/exit, debug for state transitions) and ensure messages reference quote_id
and chain to aid debugging.
🪄 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: d8168240-6a5e-4f72-a2c8-2e6839ad6dbe
📒 Files selected for processing (8)
src/openhuman/tools/impl/wallet/prepare_transfer.rssrc/openhuman/wallet/chains/btc.rssrc/openhuman/wallet/chains/evm.rssrc/openhuman/wallet/chains/solana.rssrc/openhuman/wallet/execution.rssrc/openhuman/wallet/rpc.rssrc/openhuman/wallet/test_support.rstests/json_rpc_e2e.rs
If the chain call took longer than the 5-minute quote window (network timeouts, slow node), restoring the quote with its original expires_at_ms would hand back an immediately-expired entry — retry would fail with "quote expired". Reset expires_at_ms to now + QUOTE_TTL_MS on the restore path so the caller actually gets a usable retry window.
…complete # Conflicts: # app/src-tauri/Cargo.lock
…complete # Conflicts: # tests/json_rpc_e2e.rs
This was referenced May 26, 2026
senamakel
added a commit
to nocstah/openhuman
that referenced
this pull request
Sep 11, 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
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
chains/{evm,btc,solana,tron}.rsper-chain executors.execution.rsbecomes a thin dispatcher; preparation logic + quote storage stay shared.evmNetworkto prepare params + prepared quotes so a singleWalletChain::Evmcovers Ethereum + L2s, with chain_id cross-check at execute time to refuse cross-network signing.Problem
The wallet domain had stubs for BTC/Solana/Tron —
execute_preparedreturned "not implemented yet" for every non-EVM chain, and Base/Arbitrum/Optimism/Polygon weren't representable at all. Agents could prepare quotes but couldn't sign or broadcast on three of the four supported chains, and the EVM signer was hard-pinned to Ethereum mainnet.Solution
Per-chain executors under
chains/:m/44'/60'/...),EvmNetworkenum selects RPC/explorer/asset catalog per network. Chain_id from RPC must match the network on the quote or signing aborts.bitcoincrate, Esplora REST for UTXO/balance/broadcast, BIP143 segwit sighash + ECDSA signing, dust-aware change output.solana-sdk's ~150 transitive crates), native + SPL with off-curvefind_program_addressfor ATA derivation.Shared
test_supportmodule exposes the canonical test mnemonic + deterministic addresses so every chain test runs against the same wallet without re-deriving.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/coverage.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.## RelatedOPENHUMAN_WALLET_RPC_<CHAIN>env overridedocs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
evmNetworkdefaults toethereum_mainnetwhen omitted, preserving the previous behavior.Related
evmNetworkselector (separate PR)AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
wallet/multi-chain-completef537111b5b6fd164305e998ddc6e868f69367ef3Validation Run
pnpm --filter openhuman-app format:check— N/A: no frontend changespnpm typecheck— N/A: no frontend changescargo test --lib openhuman::wallet(63 pass) +cargo test --test json_rpc_e2e json_rpc_wallet(8 pass)cargo fmt+cargo check --bin openhuman-corecleanValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
wallet_execute_preparednow signs and broadcasts on BTC/Solana/Tron (previously returned "not implemented yet"). EVM gains L2 network selection viaevmNetworkparam.wallet_prepare_transfer+wallet_execute_preparedfor non-EVM chains will now actually broadcast (instead of erroring). EVM callers can target Base/Arbitrum/Optimism/Polygon.Parity Contract
evmNetworkparam) keep targeting Ethereum mainnet exactly as before. All previously-passing tests still pass.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Documentation
Tests
Chores