strict v2 account identity: registry-only address resolution#17
Merged
tac0turtle merged 11 commits intomainfrom Feb 27, 2026
Merged
strict v2 account identity: registry-only address resolution#17tac0turtle merged 11 commits intomainfrom
tac0turtle merged 11 commits intomainfrom
Conversation
The duplicate grpc_addr field in TestappRunCustom clashed with the same field in NativeRunConfigArgs, causing a runtime panic. Use the existing --grpc-addr from NativeRunConfigArgs wired through NodeConfig. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaces numeric AccountId with a 32-byte canonical form; adds an EOA/contract registry and runtime registration APIs; introduces a storage-backed StateQuerier for RPC balance/nonce; rewrites genesis, mempool, and STF flows to resolve accounts from ETH addresses and register runtime contracts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Registry as EOA Registry
participant Env as Environment
participant Storage as Storage
Client->>Registry: resolve_or_create_eoa_account(eth_address)
rect rgba(100, 150, 255, 0.5)
Note over Registry: Lookup Phase
Registry->>Env: lookup_account_id_in_env(eth_address)
Env-->>Registry: Option<AccountId>
alt Not Found
Registry->>Storage: lookup_account_id_in_storage(eth_address)
Storage-->>Registry: Option<AccountId>
end
end
alt Account Exists
Registry-->>Client: Return existing AccountId
else Create New
rect rgba(100, 200, 100, 0.5)
Note over Registry: Create & Register Phase
Registry->>Env: derive_eth_eoa_account_id(eth_address)
Env-->>Registry: new AccountId
Registry->>Env: register_account_at_id(account_id, code_id, init_msg)
Env-->>Registry: Success
end
rect rgba(200, 100, 100, 0.5)
Note over Registry: Persist Phase
Registry->>Storage: set_mapping(eth_address, account_id)
Storage-->>Registry: Success
end
Registry-->>Client: Return newly created AccountId
end
sequenceDiagram
participant RPC as RPC Client
participant Provider as ChainStateProvider
participant Querier as StorageStateQuerier
participant Storage as Storage
participant Registry as EOA Registry
RPC->>Provider: get_balance(eth_address)
alt State Querier Present
Provider->>Querier: get_balance(eth_address)
Querier->>Registry: lookup_account_id_in_storage(eth_address)
Registry->>Storage: query mapping key
Storage-->>Registry: AccountId
Querier->>Storage: read balance (token_account_id, account_id)
Storage-->>Querier: balance u128
Querier-->>Provider: U256
else No Querier
Provider-->>RPC: NotImplemented / fallback
end
Provider-->>RPC: balance result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ntid # Conflicts: # bin/evd/src/main.rs # bin/testapp/Cargo.toml # bin/testapp/src/lib.rs # bin/testapp/src/main.rs # crates/app/node/src/config.rs # crates/app/node/src/lib.rs # docker/evd/Dockerfile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
bin/testapp/tests/mempool_e2e.rs (1)
260-267:⚠️ Potential issue | 🟠 MajorMake this E2E test deterministic.
Lines 265-266 use
OsRng, which makes failures hard to reproduce and debug across CI runs.As per coding guidelines:
**/tests/**/*.rs: Use evolve_simulator for deterministic end-to-end testing with SimulatorConfig::default() and seed configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 260 - 267, The test_token_transfer_e2e currently uses non-deterministic OsRng when calling SigningKey::random (alice_key and bob_key), so make the test deterministic by creating a deterministic simulator via evolve_simulator with SimulatorConfig::default() and a fixed seed, and derive the signing keys from that seeded RNG (or from a deterministic seed value) instead of OsRng; update references to alice_key, bob_key and get_address to use those deterministically-derived keys so the end-to-end test is repeatable across CI.
🧹 Nitpick comments (7)
crates/testing/simulator/src/eth_eoa.rs (1)
36-37: Consider preallocating vector capacity.The
.to_vec()call allocates exactly the size ofaccount_id.as_bytes()(32 bytes), thenpush(0u8)may trigger a reallocation. As per coding guidelines, minimize memory allocation and preallocate when possible—similar towarming.rswhich usesVec::with_capacity(32).♻️ Suggested optimization
- let mut nonce_key = account_id.as_bytes().to_vec(); + let mut nonce_key = Vec::with_capacity(33); + nonce_key.extend_from_slice(&account_id.as_bytes()); nonce_key.push(0u8); // ... - let mut addr_key = account_id.as_bytes().to_vec(); + let mut addr_key = Vec::with_capacity(33); + addr_key.extend_from_slice(&account_id.as_bytes()); addr_key.push(1u8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/testing/simulator/src/eth_eoa.rs` around lines 36 - 37, The allocation for nonce_key currently uses account_id.as_bytes().to_vec() then pushes a byte which can reallocate; change to preallocate capacity (e.g. Vec::with_capacity(account_id.as_bytes().len() + 1)) then copy the bytes (via extend_from_slice or equivalent) and push the trailing 0u8 so no reallocation occurs—refer to nonce_key and account_id.as_bytes() in this file and the preallocation pattern used in warming.rs for guidance.docker/testapp/Dockerfile (1)
9-11: Improve Docker layer caching for faster rebuilds.
COPY . .beforecargo buildinvalidates dependency layers on almost any source change. Consider a manifest-first orcargo-chefflow to avoid recompiling the full dependency graph each build.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/testapp/Dockerfile` around lines 9 - 11, The Dockerfile currently does COPY . . before running cargo build -p evolve_testapp --release which invalidates dependency layers on any source change; change to a manifest-first build (or cargo-chef) flow: first COPY only Cargo.toml and Cargo.lock (and workspace manifests if any), run cargo fetch/cargo build --release for dependencies to create a cached layer, then COPY the src/ and other sources and run cargo build -p evolve_testapp --release; alternatively implement cargo-chef’s plan/build steps (cargo chef prepare -> COPY recipe -> cargo chef cook -> COPY source -> cargo build) to isolate dependency compilation from app source changes.crates/app/stf/src/lib.rs (1)
1253-1373: Consider decomposingapply_txto improve readability.The
apply_txfunction is ~120 lines, exceeding the 70-line guideline. While the logic is correct and the complexity is inherent to the transaction lifecycle, consider extracting phases into helper methods:
resolve_sender_phase()bootstrap_sender_phase()validate_tx_phase()execute_tx_phase()This would improve readability and make each phase easier to test independently.
As per coding guidelines: "Keep functions to less than 70 lines to maintain bounded complexity"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/stf/src/lib.rs` around lines 1253 - 1373, The apply_tx function is too large—split its clear phases into small helpers to improve readability and testability: extract a resolve_sender_phase that takes (&mut ExecutionState, codes, &mut GasCounter, &Tx, BlockContext) and returns Result<ResolvedSender, TxError> using Invoker::new_for_begin_block and tx.resolve_sender_account; extract bootstrap_sender_phase that accepts the resolved sender, tx.sender_bootstrap(), state, codes, &mut GasCounter and calls Invoker::new_for_begin_block and register_account_at_id (returning Result<(), TxError>); extract validate_tx_phase that constructs Invoker::new_for_validate_tx and calls self.tx_validator.validate_tx (returning Result<InvokerValidateCtx, TxError>); and extract execute_tx_phase that converts validate ctx via ctx.into_new_exec(resolved_sender), resolves recipient, calls ctx.do_exec and PostTx::after_tx_executed and returns the final (Response, gas_used, events) tuple; then have apply_tx orchestrate these helpers, preserve existing error handling and gas_counter usage, and keep apply_tx under 70 lines.crates/rpc/chain-index/src/integration.rs (1)
252-267: Consider extracting shared address resolution logic.The
from/toresolution logic here is identical tobuild_stored_transaction(lines 195-211). Consider extracting helper functions likeresolve_sender_address(tx)andresolve_recipient_address(tx)to reduce duplication.♻️ Optional refactor to reduce duplication
fn resolve_sender_address<Tx: Transaction>(tx: &Tx) -> Address { tx.sender_eth_address() .map(Address::from) .unwrap_or_else(|| account_id_to_address(tx.sender())) } fn resolve_recipient_address<Tx: Transaction>(tx: &Tx) -> Option<Address> { if let Some(recipient) = tx.recipient_eth_address() { Some(Address::from(recipient)) } else { let recipient = tx.recipient(); if recipient == AccountId::invalid() { None } else { Some(account_id_to_address(recipient)) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/chain-index/src/integration.rs` around lines 252 - 267, Extract the duplicated from/to address resolution into shared helper functions used by both this block and build_stored_transaction: implement resolve_sender_address<Tx: Transaction>(tx: &Tx) that uses tx.sender_eth_address(), Address::from and account_id_to_address(tx.sender()), and resolve_recipient_address<Tx: Transaction>(tx: &Tx) that returns Option<Address> by checking tx.recipient_eth_address(), tx.recipient(), comparing to AccountId::invalid(), and using account_id_to_address where appropriate; replace the inline logic in this function and in build_stored_transaction with calls to these helpers.bin/testapp/tests/mempool_e2e.rs (1)
261-447: Splittest_token_transfer_e2einto smaller helpers.This test bundles setup, signing, submission, block production, and assertions in one large function, which is above the project’s complexity bound.
As per coding guidelines:
**/*.rs: Keep functions to less than 70 lines to maintain bounded complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/tests/mempool_e2e.rs` around lines 261 - 447, Split the large test_token_transfer_e2e into small helpers: extract genesis/setup into a helper (e.g., setup_genesis) that builds gas_config, calls build_mempool_stf, do_eth_genesis, applies genesis_changes and returns (stf, storage, genesis_accounts, codes); extract tx construction into build_transfer_tx that takes (alice_key, chain_id, token_address, bob_account_id, transfer_amount) and returns raw_tx using create_signed_tx and compute_selector; extract mempool submission + block production into submit_and_produce_block which decodes via EthGateway::decode_and_verify, adds to mempool, calls dev.produce_block_from_mempool and returns block_result and tx_hash; and extract final assertions into assert_post_block_state that reads read_nonce and read_token_balance and asserts expected balances; update test_token_transfer_e2e to orchestrate these helpers (calling build_mempool_stf, setup_genesis, build_transfer_tx, submit_and_produce_block, assert_post_block_state) so each function remains under the 70-line complexity bound while keeping calls to create_signed_tx, do_eth_genesis, produce_block_from_mempool, read_nonce and read_token_balance intact.crates/rpc/chain-index/src/provider.rs (1)
378-412: Prefer explicit RPC errors whenstate_querieris not configured.Returning zero/empty defaults here can hide configuration mistakes and serve misleading state responses.
Suggested pattern
async fn get_balance(&self, address: Address, _block: Option<u64>) -> Result<U256, RpcError> { - match &self.state_querier { - Some(q) => q.get_balance(address).await, - None => Ok(U256::ZERO), - } + let q = self + .state_querier + .as_ref() + .ok_or_else(|| RpcError::NotImplemented("state querier not configured".to_string()))?; + q.get_balance(address).await }Apply the same pattern to
get_transaction_count,call, andestimate_gas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/chain-index/src/provider.rs` around lines 378 - 412, The current implementations of get_balance, get_transaction_count, call, and estimate_gas return silent defaults when self.state_querier is None; change each to return an explicit RpcError indicating that the state_querier is not configured instead of Ok defaults. Locate the match arms in get_balance, get_transaction_count, call, and estimate_gas that check self.state_querier and replace the None => Ok(...) branches with returning an Err(RpcError) (use the crate's RpcError constructor or variant) containing a clear message like "state_querier not configured" so callers receive an explicit RPC error when the provider is misconfigured.bin/evd/src/main.rs (1)
166-418: Splitrun_nodeinto smaller units before expanding this path further.This function now contains storage init, genesis, RPC wiring, block callback logic, and shutdown handling in one place. The added state querier/genesis wiring increases cognitive load and change risk.
As per coding guidelines, "Keep functions to less than 70 lines to maintain bounded complexity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/evd/src/main.rs` around lines 166 - 418, The run_node function is too large and should be split into smaller helper functions; extract responsibility areas (storage & QmdbStorage init, genesis/load logic, RPC server setup, chain index creation, the OnBlockExecuted closure, and shutdown/save logic) into clearly named functions like init_storage_and_genesis(storage_config, genesis_config) -> (storage, genesis_result, initial_height), init_chain_index(config) -> Option<Arc<PersistentChainIndex>>, start_rpc_server(config, storage, mempool, genesis_result) -> Option<RpcHandle>, build_on_block_executed(storage, chain_index, parent_hash, current_height, callback_chain_id, executor_config) -> OnBlockExecuted, and run_server_with_shutdown(server, rpc_handle, storage, chain_state) -> (); move the local variables currently captured (e.g., storage, chain_index, parent_hash, current_height, genesis_result, mempool, executor_config, callback_max_gas, callback_indexing_enabled) into the signatures so closures no longer capture the whole outer context, update run_node to coordinate these smaller functions (each under ~70 lines), and ensure error handling/logging is preserved when moving logic out of run_node.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/evd/src/main.rs`:
- Around line 644-657: Tests mutate the global process environment via
EnvVarGuard::set and its Drop impl, causing flakiness when tests run in
parallel; fix this by serializing env-var mutations: introduce a static global
lock (e.g., a Mutex or RwLock) and acquire it when creating the guard (inside
EnvVarGuard::set or its constructor) and release it in the Drop implementation
so only one test can mutate env vars at a time, ensuring EnvVarGuard::set and
impl Drop for EnvVarGuard use that lock to enforce exclusive access.
In `@bin/testapp/src/main.rs`:
- Around line 196-206: The code currently panics on malformed addresses by
calling parse_address().expect(...) while building funded_accounts; change this
to return a validation error instead of panicking: replace the map/expect block
so parse_address() errors are propagated (e.g. map each acc to
acc.parse_address().map(|addr| (addr.into_array(), acc.balance)) and then
collect::<Result<Vec<([u8;20], u128)>, _>>()? or otherwise convert the parse
error into your function's error type and use the ? operator). References:
funded_accounts variable construction and parse_address() — ensure the calling
function returns Result so malformed ETH addresses produce a recoverable error
rather than aborting.
- Around line 111-115: Change load_genesis_config to return a
Result<Option<EvdGenesisConfig>, anyhow::Error> (or your project's common error
type) instead of panicking: replace the current path.map(... expect(...)) with
code that attempts EvdGenesisConfig::load(p) and propagates any load error (use
.map(|p| EvdGenesisConfig::load(p)).transpose()? or use match/ ? on the Result
from EvdGenesisConfig::load), and log a clear error via tracing if needed so
malformed or missing genesis data returns a controlled startup error rather than
calling expect.
In `@crates/app/node/src/lib.rs`:
- Around line 435-445: The JSON-RPC ChainStateProvider is being given the
StateQuerier (state_querier) via .with_state_querier(...), but the gRPC
ChainStateProvider created later in run_dev_node_with_rpc is not—causing
inconsistent state query behavior between protocols; fix it by passing the same
state_querier to the gRPC provider: locate the ChainStateProvider construction
used for the gRPC server and call
.with_state_querier(Arc::clone(&state_querier)) (or state_querier.clone()) on
it, matching the pattern used in run_dev_node_with_rpc_and_mempool_eth and
ensuring the Arc<dyn StateQuerier> type is used.
In `@crates/app/tx/eth/src/eoa_registry.rs`:
- Around line 152-160: Define a typed error with define_error! for the 0x5A
conflict (e.g., a name like AddressAccountConflict) and replace all direct
evolve_core::ErrorCode::new(0x5A) uses with that new error constant; locate the
conflict checks in eoa_registry.rs (the blocks that call
lookup_account_id_in_env and lookup_address_in_env and the other conflict paths
around the account/address validation) and return the defined error instead of
ErrorCode::new(0x5A) so the magic number is centralized and registered at
compile time.
In `@crates/app/tx/eth/src/mempool.rs`:
- Around line 123-135: Replace the inline numeric error codes in
resolve_recipient_account with named error constants defined via define_error!
in your error module; specifically, add two constants (e.g.,
ERR_SYSTEM_LOOKUP_MISSING and ERR_SYSTEM_RECIPIENT_NOT_FOUND) registered with
define_error! using the appropriate 0x40-0x7F system range, then update
resolve_recipient_account to return those constants (use the constant
constructors instead of evolve_core::ErrorCode::new(0x50) and ..new(0x52)) so
the function references the compile-time-registered error names rather than raw
literals.
In `@crates/rpc/chain-index/src/querier.rs`:
- Around line 94-98: Add a helper (e.g., resolve_account_id) that first calls
lookup_account_id_in_storage and if that returns None calls
lookup_contract_account_id_in_storage, returning the resolved account id (or
None) and propagating storage errors via map_err (RpcError::InternalError).
Replace the direct lookup_account_id_in_storage calls in get_balance() and
get_transaction_count() to use this helper so runtime contract accounts
registered via register_runtime_contract_account are found; preserve the
existing error handling pattern (map_err(...)? and Ok(U256::ZERO) when None).
In `@docker/testapp/Dockerfile`:
- Around line 13-25: The runtime stage currently runs the container as root; add
a non-root user and switch to it in that stage: create a dedicated user/group
(e.g., appuser), ensure ownership/permissions for WORKDIR /app and the binary
/usr/local/bin/testapp are set to that user (chown/chmod as needed), and add a
USER appuser line before the ENTRYPOINT so the container executes
/usr/local/bin/testapp as the non-root user; update the runtime stage in the
Dockerfile accordingly and ensure the copied binary is accessible to that user.
---
Outside diff comments:
In `@bin/testapp/tests/mempool_e2e.rs`:
- Around line 260-267: The test_token_transfer_e2e currently uses
non-deterministic OsRng when calling SigningKey::random (alice_key and bob_key),
so make the test deterministic by creating a deterministic simulator via
evolve_simulator with SimulatorConfig::default() and a fixed seed, and derive
the signing keys from that seeded RNG (or from a deterministic seed value)
instead of OsRng; update references to alice_key, bob_key and get_address to use
those deterministically-derived keys so the end-to-end test is repeatable across
CI.
---
Nitpick comments:
In `@bin/evd/src/main.rs`:
- Around line 166-418: The run_node function is too large and should be split
into smaller helper functions; extract responsibility areas (storage &
QmdbStorage init, genesis/load logic, RPC server setup, chain index creation,
the OnBlockExecuted closure, and shutdown/save logic) into clearly named
functions like init_storage_and_genesis(storage_config, genesis_config) ->
(storage, genesis_result, initial_height), init_chain_index(config) ->
Option<Arc<PersistentChainIndex>>, start_rpc_server(config, storage, mempool,
genesis_result) -> Option<RpcHandle>, build_on_block_executed(storage,
chain_index, parent_hash, current_height, callback_chain_id, executor_config) ->
OnBlockExecuted, and run_server_with_shutdown(server, rpc_handle, storage,
chain_state) -> (); move the local variables currently captured (e.g., storage,
chain_index, parent_hash, current_height, genesis_result, mempool,
executor_config, callback_max_gas, callback_indexing_enabled) into the
signatures so closures no longer capture the whole outer context, update
run_node to coordinate these smaller functions (each under ~70 lines), and
ensure error handling/logging is preserved when moving logic out of run_node.
In `@bin/testapp/tests/mempool_e2e.rs`:
- Around line 261-447: Split the large test_token_transfer_e2e into small
helpers: extract genesis/setup into a helper (e.g., setup_genesis) that builds
gas_config, calls build_mempool_stf, do_eth_genesis, applies genesis_changes and
returns (stf, storage, genesis_accounts, codes); extract tx construction into
build_transfer_tx that takes (alice_key, chain_id, token_address,
bob_account_id, transfer_amount) and returns raw_tx using create_signed_tx and
compute_selector; extract mempool submission + block production into
submit_and_produce_block which decodes via EthGateway::decode_and_verify, adds
to mempool, calls dev.produce_block_from_mempool and returns block_result and
tx_hash; and extract final assertions into assert_post_block_state that reads
read_nonce and read_token_balance and asserts expected balances; update
test_token_transfer_e2e to orchestrate these helpers (calling build_mempool_stf,
setup_genesis, build_transfer_tx, submit_and_produce_block,
assert_post_block_state) so each function remains under the 70-line complexity
bound while keeping calls to create_signed_tx, do_eth_genesis,
produce_block_from_mempool, read_nonce and read_token_balance intact.
In `@crates/app/stf/src/lib.rs`:
- Around line 1253-1373: The apply_tx function is too large—split its clear
phases into small helpers to improve readability and testability: extract a
resolve_sender_phase that takes (&mut ExecutionState, codes, &mut GasCounter,
&Tx, BlockContext) and returns Result<ResolvedSender, TxError> using
Invoker::new_for_begin_block and tx.resolve_sender_account; extract
bootstrap_sender_phase that accepts the resolved sender, tx.sender_bootstrap(),
state, codes, &mut GasCounter and calls Invoker::new_for_begin_block and
register_account_at_id (returning Result<(), TxError>); extract
validate_tx_phase that constructs Invoker::new_for_validate_tx and calls
self.tx_validator.validate_tx (returning Result<InvokerValidateCtx, TxError>);
and extract execute_tx_phase that converts validate ctx via
ctx.into_new_exec(resolved_sender), resolves recipient, calls ctx.do_exec and
PostTx::after_tx_executed and returns the final (Response, gas_used, events)
tuple; then have apply_tx orchestrate these helpers, preserve existing error
handling and gas_counter usage, and keep apply_tx under 70 lines.
In `@crates/rpc/chain-index/src/integration.rs`:
- Around line 252-267: Extract the duplicated from/to address resolution into
shared helper functions used by both this block and build_stored_transaction:
implement resolve_sender_address<Tx: Transaction>(tx: &Tx) that uses
tx.sender_eth_address(), Address::from and account_id_to_address(tx.sender()),
and resolve_recipient_address<Tx: Transaction>(tx: &Tx) that returns
Option<Address> by checking tx.recipient_eth_address(), tx.recipient(),
comparing to AccountId::invalid(), and using account_id_to_address where
appropriate; replace the inline logic in this function and in
build_stored_transaction with calls to these helpers.
In `@crates/rpc/chain-index/src/provider.rs`:
- Around line 378-412: The current implementations of get_balance,
get_transaction_count, call, and estimate_gas return silent defaults when
self.state_querier is None; change each to return an explicit RpcError
indicating that the state_querier is not configured instead of Ok defaults.
Locate the match arms in get_balance, get_transaction_count, call, and
estimate_gas that check self.state_querier and replace the None => Ok(...)
branches with returning an Err(RpcError) (use the crate's RpcError constructor
or variant) containing a clear message like "state_querier not configured" so
callers receive an explicit RPC error when the provider is misconfigured.
In `@crates/testing/simulator/src/eth_eoa.rs`:
- Around line 36-37: The allocation for nonce_key currently uses
account_id.as_bytes().to_vec() then pushes a byte which can reallocate; change
to preallocate capacity (e.g. Vec::with_capacity(account_id.as_bytes().len() +
1)) then copy the bytes (via extend_from_slice or equivalent) and push the
trailing 0u8 so no reallocation occurs—refer to nonce_key and
account_id.as_bytes() in this file and the preallocation pattern used in
warming.rs for guidance.
In `@docker/testapp/Dockerfile`:
- Around line 9-11: The Dockerfile currently does COPY . . before running cargo
build -p evolve_testapp --release which invalidates dependency layers on any
source change; change to a manifest-first build (or cargo-chef) flow: first COPY
only Cargo.toml and Cargo.lock (and workspace manifests if any), run cargo
fetch/cargo build --release for dependencies to create a cached layer, then COPY
the src/ and other sources and run cargo build -p evolve_testapp --release;
alternatively implement cargo-chef’s plan/build steps (cargo chef prepare ->
COPY recipe -> cargo chef cook -> COPY source -> cargo build) to isolate
dependency compilation from app source changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
bin/evd/src/main.rsbin/testapp/src/eth_eoa.rsbin/testapp/src/genesis_config.rsbin/testapp/src/lib.rsbin/testapp/src/main.rsbin/testapp/src/sim_testing.rsbin/testapp/tests/mempool_e2e.rsbin/txload/src/main.rscrates/app/node/src/config.rscrates/app/node/src/lib.rscrates/app/sdk/core/src/lib.rscrates/app/sdk/core/src/low_level.rscrates/app/sdk/core/src/runtime_api.rscrates/app/sdk/core/src/storage_api.rscrates/app/sdk/standards/evolve_authentication/src/lib.rscrates/app/sdk/stf_traits/src/lib.rscrates/app/sdk/testing/src/lib.rscrates/app/stf/src/handlers.rscrates/app/stf/src/lib.rscrates/app/tx/eth/src/eoa_registry.rscrates/app/tx/eth/src/lib.rscrates/app/tx/eth/src/mempool.rscrates/app/tx/eth/src/traits.rscrates/app/tx/eth/tests/integration_tests.rscrates/rpc/chain-index/src/integration.rscrates/rpc/chain-index/src/lib.rscrates/rpc/chain-index/src/provider.rscrates/rpc/chain-index/src/querier.rscrates/rpc/types/Cargo.tomlcrates/rpc/types/src/lib.rscrates/testing/debugger/src/breakpoints.rscrates/testing/debugger/src/inspector.rscrates/testing/debugger/src/trace.rscrates/testing/simulator/src/eth_eoa.rsdocker/testapp/Dockerfile
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
bin/evd/src/main.rs (1)
585-588:⚠️ Potential issue | 🟠 MajorAvoid panic on malformed custom-genesis ETH addresses.
Using
expect("invalid address in genesis config")here turns bad input into a process panic instead of a controlled startup/init error path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/evd/src/main.rs` around lines 585 - 588, Replace the panic-producing expect on parse_address() with proper error handling: instead of calling acc.parse_address().expect("invalid address in genesis config"), propagate or return a Result/Error (or log and exit cleanly) when parse_address() fails, including the offending acc (or its identifier) in the error context; ensure the surrounding function that constructs the (addr.into_array(), acc.balance) tuple returns a Result so callers can handle malformed genesis addresses rather than aborting the process.
🧹 Nitpick comments (2)
crates/app/node/src/lib.rs (1)
629-633: Consider addingHasTokenAccountIdbound for consistency.Unlike
run_dev_node,run_dev_node_with_rpc, andrun_dev_node_with_rpc_and_mempool_eth, this function'sGtype doesn't requireHasTokenAccountId. While the warning at lines 720-723 explains that RPC with genericTxtypes isn't fully supported, adding the bound would maintain API consistency across all dev node runners. If a user enables RPC with this function, balance queries would silently fail rather than being blocked at compile time.♻️ Suggested change
- G: BorshSerialize + BorshDeserialize + Clone + Debug + Send + Sync + 'static, + G: BorshSerialize + BorshDeserialize + Clone + Debug + HasTokenAccountId + Send + Sync + 'static,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/node/src/lib.rs` around lines 629 - 633, The generic parameter G for the function lacks the HasTokenAccountId bound, causing inconsistency with run_dev_node, run_dev_node_with_rpc, and run_dev_node_with_rpc_and_mempool_eth and allowing RPC-enabled builds to compile while balance queries would later fail; update the G type bound in the function signature (the generic list containing Tx, Codes, S, Stf, G) to include HasTokenAccountId (i.e., add + HasTokenAccountId to the existing G bounds) so the compiler enforces token-account support at compile time.crates/rpc/chain-index/src/querier.rs (1)
119-127: Stub implementations are reasonable placeholders.The
call()andestimate_gas()methods are clearly marked with TODOs. The hardcoded 21000 gas is a reasonable default (base ETH transfer cost), and emptyBytesis a safe fallback for unimplemented call results.Would you like me to open an issue to track implementing
call()andestimate_gas()via STF?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rpc/chain-index/src/querier.rs` around lines 119 - 127, The current stub implementations of call(&self, _request: &CallRequest) and estimate_gas(&self, _request: &CallRequest) must be replaced with real STF-backed logic: implement call by invoking the state transition function query path (STF::query or equivalent) with the incoming CallRequest payload, convert the STF response bytes into the returned Bytes value and map any STF errors into RpcError; implement estimate_gas by executing the call in the STF in “dry-run”/simulation mode with gas accounting enabled (track gas used during execution) and return the measured u64 gas value instead of the hardcoded 21000, also mapping execution failures to RpcError. Ensure you reference the CallRequest parameter, use the module/type that exposes STF::query and the STF gas-tracking execution API, and add appropriate error mapping and logging where the current stubs return Ok(Bytes::new()) and Ok(21000).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/evd/src/main.rs`:
- Around line 697-707: EnvVarGuard currently stores the MutexGuard for
ENV_VAR_LOCK for the whole guard lifetime, causing deadlocks when multiple
EnvVarGuard::set calls are created; change the design so set only locks briefly
to read the old value and set the new one, then drops the lock before returning
(i.e., remove the _guard field from the EnvVarGuard struct), and implement Drop
for EnvVarGuard that locks ENV_VAR_LOCK when restoring the old value; update
EnvVarGuard::set, the struct fields, and the Drop impl to reference
ENV_VAR_LOCK, EnvVarGuard, and set accordingly.
In `@bin/testapp/src/main.rs`:
- Around line 243-247: The GenesisAccounts struct is being populated with the
token contract account for both participants—set alice and bob to their actual
EOA genesis accounts (replace token.0 with alice_acc.0 and bob_acc.0
respectively) and verify the other fields (e.g., atom, scheduler) use the
intended account variables (e.g., atom_acc.0 or token.0 and scheduler_acc.0) so
callers receive real genesis participant IDs instead of the token contract.
---
Duplicate comments:
In `@bin/evd/src/main.rs`:
- Around line 585-588: Replace the panic-producing expect on parse_address()
with proper error handling: instead of calling
acc.parse_address().expect("invalid address in genesis config"), propagate or
return a Result/Error (or log and exit cleanly) when parse_address() fails,
including the offending acc (or its identifier) in the error context; ensure the
surrounding function that constructs the (addr.into_array(), acc.balance) tuple
returns a Result so callers can handle malformed genesis addresses rather than
aborting the process.
---
Nitpick comments:
In `@crates/app/node/src/lib.rs`:
- Around line 629-633: The generic parameter G for the function lacks the
HasTokenAccountId bound, causing inconsistency with run_dev_node,
run_dev_node_with_rpc, and run_dev_node_with_rpc_and_mempool_eth and allowing
RPC-enabled builds to compile while balance queries would later fail; update the
G type bound in the function signature (the generic list containing Tx, Codes,
S, Stf, G) to include HasTokenAccountId (i.e., add + HasTokenAccountId to the
existing G bounds) so the compiler enforces token-account support at compile
time.
In `@crates/rpc/chain-index/src/querier.rs`:
- Around line 119-127: The current stub implementations of call(&self, _request:
&CallRequest) and estimate_gas(&self, _request: &CallRequest) must be replaced
with real STF-backed logic: implement call by invoking the state transition
function query path (STF::query or equivalent) with the incoming CallRequest
payload, convert the STF response bytes into the returned Bytes value and map
any STF errors into RpcError; implement estimate_gas by executing the call in
the STF in “dry-run”/simulation mode with gas accounting enabled (track gas used
during execution) and return the measured u64 gas value instead of the hardcoded
21000, also mapping execution failures to RpcError. Ensure you reference the
CallRequest parameter, use the module/type that exposes STF::query and the STF
gas-tracking execution API, and add appropriate error mapping and logging where
the current stubs return Ok(Bytes::new()) and Ok(21000).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
bin/evd/src/main.rsbin/testapp/src/main.rsbin/testapp/tests/mempool_e2e.rscrates/app/node/src/lib.rscrates/app/stf/src/lib.rscrates/app/tx/eth/src/eoa_registry.rscrates/app/tx/eth/src/error.rscrates/app/tx/eth/src/mempool.rscrates/rpc/chain-index/src/integration.rscrates/rpc/chain-index/src/provider.rscrates/rpc/chain-index/src/querier.rscrates/testing/simulator/src/eth_eoa.rsdocker/testapp/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/testing/simulator/src/eth_eoa.rs
- docker/testapp/Dockerfile
Comment on lines
697
to
707
| impl EnvVarGuard { | ||
| fn set(key: &'static str, value: &str) -> Self { | ||
| let guard = ENV_VAR_LOCK.lock().expect("env var lock poisoned"); | ||
| let old = std::env::var(key).ok(); | ||
| std::env::set_var(key, value); | ||
| Self { | ||
| key, | ||
| old, | ||
| _guard: guard, | ||
| } | ||
| } |
There was a problem hiding this comment.
EnvVarGuard can deadlock when setting multiple vars in one test.
Each EnvVarGuard::set holds the global mutex for the guard lifetime. Creating _alice_addr, _bob_addr, _alice_bal, _bob_bal sequentially in the same test attempts recursive locking and can block indefinitely.
Also applies to: 771-780
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/evd/src/main.rs` around lines 697 - 707, EnvVarGuard currently stores the
MutexGuard for ENV_VAR_LOCK for the whole guard lifetime, causing deadlocks when
multiple EnvVarGuard::set calls are created; change the design so set only locks
briefly to read the old value and set the new one, then drops the lock before
returning (i.e., remove the _guard field from the EnvVarGuard struct), and
implement Drop for EnvVarGuard that locks ENV_VAR_LOCK when restoring the old
value; update EnvVarGuard::set, the struct fields, and the Drop impl to
reference ENV_VAR_LOCK, EnvVarGuard, and set accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/app/sdk/core/src/lib.rs (1)
66-69: Alignas_bytes()documentation and semantics: return&[u8; 32]instead of owned copy.The docs say "Borrow raw canonical bytes", but the signature returns an owned
[u8; 32], creating unnecessary copying and duplicatingto_bytes(). The proposed change aligns documentation with semantics. All ~130+ existing call sites remain compatible since they already borrow the returned value or convert it to owned types.♻️ Proposed API/doc alignment
- /// Borrow raw canonical bytes. - pub const fn as_bytes(&self) -> [u8; 32] { - self.0 - } + /// Borrow raw canonical bytes. + pub const fn as_bytes(&self) -> &[u8; 32] { + &self.0 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/sdk/core/src/lib.rs` around lines 66 - 69, The method as_bytes currently returns an owned [u8; 32] but its docstring says "Borrow raw canonical bytes"; change its signature to return a reference (&[u8; 32]) and return &self.0 so the method truly borrows without copying. Update the signature in lib.rs for the method named as_bytes and its body to return &self.0; no caller changes should be required since existing call sites that borrow or convert will remain compatible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/app/sdk/core/src/lib.rs`:
- Around line 98-105: The current AccountId::inner() unconditionally drops the
upper 16 bytes which can collapse distinct non-numeric canonical IDs into the
same u128; fix by making inner() defensive: check the high 16 bytes of self.0
(bytes 0..16) for any non-zero values and refuse/propagate an error instead of
silently truncating — e.g. change inner() to return Result<u128, _> (or panic
with a clear message) when those high bytes are non-zero, otherwise perform the
u128::from_be_bytes on the low 16 bytes as before; update all call sites of
AccountId::inner() to handle the new Result/panic semantics.
---
Nitpick comments:
In `@crates/app/sdk/core/src/lib.rs`:
- Around line 66-69: The method as_bytes currently returns an owned [u8; 32] but
its docstring says "Borrow raw canonical bytes"; change its signature to return
a reference (&[u8; 32]) and return &self.0 so the method truly borrows without
copying. Update the signature in lib.rs for the method named as_bytes and its
body to return &self.0; no caller changes should be required since existing call
sites that borrow or convert will remain compatible.
Comment on lines
+98
to
105
| /// Legacy extractor for compatibility where a numeric ID is still required. | ||
| pub const fn inner(&self) -> u128 { | ||
| self.0 | ||
| u128::from_be_bytes([ | ||
| self.0[16], self.0[17], self.0[18], self.0[19], self.0[20], self.0[21], self.0[22], | ||
| self.0[23], self.0[24], self.0[25], self.0[26], self.0[27], self.0[28], self.0[29], | ||
| self.0[30], self.0[31], | ||
| ]) | ||
| } |
There was a problem hiding this comment.
Guard AccountId::inner() against lossy canonical-ID truncation.
inner() always drops the upper 16 bytes. For non-numeric canonical IDs, distinct AccountId values can collapse to the same u128, which is unsafe for compatibility paths.
🛡️ Proposed compatibility-safe API
impl AccountId {
+ /// Legacy extractor that only succeeds for numeric-compat IDs
+ /// (upper 16 bytes must be zero).
+ pub fn try_inner(&self) -> Option<u128> {
+ if self.0[..16].iter().any(|b| *b != 0) {
+ return None;
+ }
+ Some(self.inner())
+ }
+
/// Legacy extractor for compatibility where a numeric ID is still required.
pub const fn inner(&self) -> u128 {
u128::from_be_bytes([
self.0[16], self.0[17], self.0[18], self.0[19], self.0[20], self.0[21], self.0[22],
self.0[23], self.0[24], self.0[25], self.0[26], self.0[27], self.0[28], self.0[29],
self.0[30], self.0[31],
])
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/app/sdk/core/src/lib.rs` around lines 98 - 105, The current
AccountId::inner() unconditionally drops the upper 16 bytes which can collapse
distinct non-numeric canonical IDs into the same u128; fix by making inner()
defensive: check the high 16 bytes of self.0 (bytes 0..16) for any non-zero
values and refuse/propagate an error instead of silently truncating — e.g.
change inner() to return Result<u128, _> (or panic with a clear message) when
those high bytes are non-zero, otherwise perform the u128::from_be_bytes on the
low 16 bytes as before; update all call sites of AccountId::inner() to handle
the new Result/panic semantics.
This was referenced Feb 27, 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.
Overview
Summary by CodeRabbit
New Features
Bug Fixes