Add Docker Compose stack for evd and update branch changes#16
Merged
tac0turtle merged 5 commits intomainfrom Feb 27, 2026
Merged
Add Docker Compose stack for evd and update branch changes#16tac0turtle merged 5 commits intomainfrom
tac0turtle merged 5 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds Docker support for running evd (and optional ev-node), refactors genesis initialization from async to synchronous across evd/testapp, introduces custom genesis config loading, and adds optional gRPC server startup alongside JSON‑RPC in the dev node. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (JSON-RPC / gRPC)
participant RPC as JSON-RPC Server
participant GRPC as gRPC Server
participant Provider as Chain State Provider
participant Storage as Storage / Mempool
alt RPC enabled and grpc_addr set
Client->>RPC: JSON-RPC request
Client->>GRPC: gRPC request
par
RPC->>Provider: state query
GRPC->>Provider: state query
and
Provider->>Storage: fetch data
Storage-->>Provider: data
end
par
Provider-->>RPC: response
Provider-->>GRPC: response
and
RPC-->>Client: JSON-RPC response
GRPC-->>Client: gRPC response
end
else RPC only
Client->>RPC: JSON-RPC request
RPC->>Provider: state query
Provider->>Storage: fetch data
Storage-->>Provider: data
Provider-->>RPC: response
RPC-->>Client: JSON-RPC response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
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>
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
crates/app/node/src/config.rs (1)
70-78:⚠️ Potential issue | 🟠 MajorPopulate
grpc_addrfromself.parsed_grpc_addr()into_rpc_config().The method sets
grpc_addr: Nonebut should populate it fromself.parsed_grpc_addr(), consistent with howhttp_addris populated fromself.parsed_rpc_addr(). The only caller (inbin/testapp/src/main.rs) immediately reassignsgrpc_addrtoSome(config.parsed_grpc_addr()), making this boilerplate unnecessary. Sinceparsed_grpc_addr()guarantees a valid address, it should be set by default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/node/src/config.rs` around lines 70 - 78, In to_rpc_config() update the grpc_addr field to use the parsed_grpc_addr() result rather than None: inside the RpcConfig construction in the Config::to_rpc_config method, set grpc_addr: Some(self.parsed_grpc_addr()) so the RPC config mirrors how http_addr is populated from parsed_rpc_addr(); this removes the need for callers (e.g., in bin/testapp::main) to reassign grpc_addr.
🧹 Nitpick comments (5)
bin/testapp/Cargo.toml (1)
37-38: Consider centralizingserdeandserde_jsonto[workspace.dependencies]for consistency.Currently, these dependencies are not centralized in the workspace. All crates in the repository directly version them (version = "1.0"), which is consistent but not optimal for long-term maintainability. Adding them to
[workspace.dependencies]in the rootCargo.tomland updating all crates to useworkspace = truewould keep versions synchronized across the entire project.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/Cargo.toml` around lines 37 - 38, Centralize serde and serde_json by adding them to the root [workspace.dependencies] (e.g., serde = "1.0" and serde_json = "1.0") and then update this crate's Cargo.toml (the entries for serde and serde_json in bin/testapp/Cargo.toml) to use workspace = true instead of specifying version = "1.0"; ensure you remove the version keys here so the crate references the workspace-managed serde and serde_json.bin/testapp/src/main.rs (1)
113-118: Minor duplication withbin/evd/src/main.rs.This
load_genesis_confighelper is identical to the one inevd. Consider moving it toevolve_testapp::genesis_configorevolve_nodeif more binaries need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/src/main.rs` around lines 113 - 118, The helper function load_genesis_config (which calls EvdGenesisConfig::load and logs the path) is duplicated across binaries—move it into a shared module (e.g., create a new public function in evolve_testapp::genesis_config or in evolve_node), make it pub fn load_genesis_config(path: Option<&str>) -> Option<EvdGenesisConfig>, update the binaries to import and call that single shared function, and delete the duplicate definitions; ensure the new module exposes the same signature and preserves the tracing::info and expect behavior so callers compile without other changes.bin/evd/src/main.rs (1)
502-561: Consider extracting shared genesis logic.This
run_custom_genesisimplementation is nearly identical tobin/testapp/src/main.rs:run_custom_genesis(Lines 186-250). Both:
- Parse and filter funded accounts
- Initialize EOAs in a loop via
EthEoaAccountRef::initialize- Build balances vector with
address_to_account_id- Initialize token and scheduler
The main difference is the return type (
EvdGenesisResultvsGenesisAccounts). Consider extracting the shared initialization logic into a helper inevolve_testappthat returns a tuple or generic struct, which each binary can then wrap into its specific result type.🤖 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 502 - 561, run_custom_genesis duplicates shared genesis setup (parsing funded accounts, calling EthEoaAccountRef::initialize, building balances with address_to_account_id, calling TokenRef::initialize and SchedulerRef::initialize) that also exists in bin/testapp's run_custom_genesis; extract that common logic into a new helper in evolve_testapp (e.g., prepare_genesis_resources or build_genesis_state) which accepts the genesis_config, storage/callable stf env or a closure to run env initializers and returns a small common struct/tuple (token id, scheduler id, balances/any shared state or the raw GenesisOutput::changes) so each binary's run_custom_genesis (the one here returning EvdGenesisResult and the other returning GenesisAccounts) can call the helper and then wrap/convert the returned tuple into their specific result types (EvdGenesisResult / GenesisAccounts) and update callers to use the helper instead of duplicating the initialization steps.crates/app/node/src/lib.rs (1)
861-885: gRPC server lacks graceful shutdown handling.The gRPC server is spawned with
tokio::spawnbut the task handle is not retained, unlike the JSON-RPC server which is properly stopped during shutdown (lines 979-982). This means:
- The gRPC server won't receive a graceful shutdown signal
- In-flight gRPC requests may be abruptly terminated during shutdown
- Any startup errors are logged but the node continues running
♻️ Proposed fix to track gRPC server for graceful shutdown
tracing::info!("Starting gRPC server on {}", grpc_addr); - let grpc_server = GrpcServer::with_subscription_manager( + let grpc_server_task = GrpcServer::with_subscription_manager( grpc_config, grpc_state_provider, Arc::clone(&subscriptions), ); - tokio::spawn(async move { + let grpc_handle = tokio::spawn(async move { - if let Err(e) = grpc_server.serve().await { + if let Err(e) = grpc_server_task.serve().await { tracing::error!("gRPC server error: {}", e); } }); + // Store grpc_handle to abort during shutdownThen during shutdown (around line 920), abort the gRPC task:
grpc_handle.abort();Alternatively, if
GrpcServersupports graceful shutdown, consider integrating it with the existingcontext_for_shutdownsignal.🤖 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 861 - 885, The gRPC server is being spawned without keeping its task handle so it cannot be shutdown gracefully; modify the block that creates GrpcServer (grpc_server) so you capture the JoinHandle (e.g., let grpc_handle = tokio::spawn(...)) instead of discarding it, then store that handle alongside the JSON-RPC handle and on shutdown call grpc_handle.abort() (or, if GrpcServer supports a graceful shutdown API, hook its shutdown future into the existing context_for_shutdown signal so serve() is awaited with that cancelation). Ensure references to GrpcServer::with_subscription_manager, grpc_server.serve(), and the new grpc_handle are updated accordingly.docker/evd/Dockerfile (1)
13-25: Consider running the container as a non-root user for improved security.The container runs as root by default, which is flagged by Trivy (DS-0002). Running as a non-root user reduces the attack surface if the container is compromised.
🔒 Proposed fix to add a non-root user
FROM debian:bookworm-slim AS runtime RUN apt-get update \ && apt-get install -y --no-install-recommends ca-certificates \ && rm -rf /var/lib/apt/lists/* +RUN useradd --create-home --uid 1000 evd + WORKDIR /app COPY --from=builder /app/target/release/evd /usr/local/bin/evd +RUN chown -R evd:evd /app +USER evd + EXPOSE 8545 50051 ENTRYPOINT ["/usr/local/bin/evd"]Note: Ensure the volume mount path (
/var/lib/evolve/dataper docker-compose.yml) is writable by the non-root user, or adjust the path/permissions accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/evd/Dockerfile` around lines 13 - 25, The Dockerfile currently runs the runtime image as root; add a non-root user and group during the image build (e.g., create a dedicated user via a RUN step after installing ca-certificates), chown the app WORKDIR and any runtime-writable data directory (the directory used for persistent volume mounts) to that user, and switch to that user with a USER instruction before the ENTRYPOINT so /usr/local/bin/evd still gets copied by COPY but the process does not run as root; ensure ownership changes occur after COPY so the non-root user can write to the data directory at runtime.
🤖 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/testapp/src/main.rs`:
- Around line 236-240: GenesisAccounts is being constructed with alice, bob, and
atom all set to token.0 (the token account) — change the construction in the
custom genesis builder so that alice and bob are set to the first two funded EOA
account IDs (the same IDs used by run_default_genesis), while atom remains set
to token.0; locate the GenesisAccounts initialization (the block that currently
sets alice: token.0, bob: token.0, atom: token.0, scheduler: scheduler_acc.0)
and replace alice and bob with the appropriate funded account identifiers (e.g.,
funded_accounts[0].0 and funded_accounts[1].0 or whatever variable holds the
first two EOA IDs in this scope) so downstream code receives real user account
IDs.
---
Outside diff comments:
In `@crates/app/node/src/config.rs`:
- Around line 70-78: In to_rpc_config() update the grpc_addr field to use the
parsed_grpc_addr() result rather than None: inside the RpcConfig construction in
the Config::to_rpc_config method, set grpc_addr: Some(self.parsed_grpc_addr())
so the RPC config mirrors how http_addr is populated from parsed_rpc_addr();
this removes the need for callers (e.g., in bin/testapp::main) to reassign
grpc_addr.
---
Nitpick comments:
In `@bin/evd/src/main.rs`:
- Around line 502-561: run_custom_genesis duplicates shared genesis setup
(parsing funded accounts, calling EthEoaAccountRef::initialize, building
balances with address_to_account_id, calling TokenRef::initialize and
SchedulerRef::initialize) that also exists in bin/testapp's run_custom_genesis;
extract that common logic into a new helper in evolve_testapp (e.g.,
prepare_genesis_resources or build_genesis_state) which accepts the
genesis_config, storage/callable stf env or a closure to run env initializers
and returns a small common struct/tuple (token id, scheduler id, balances/any
shared state or the raw GenesisOutput::changes) so each binary's
run_custom_genesis (the one here returning EvdGenesisResult and the other
returning GenesisAccounts) can call the helper and then wrap/convert the
returned tuple into their specific result types (EvdGenesisResult /
GenesisAccounts) and update callers to use the helper instead of duplicating the
initialization steps.
In `@bin/testapp/Cargo.toml`:
- Around line 37-38: Centralize serde and serde_json by adding them to the root
[workspace.dependencies] (e.g., serde = "1.0" and serde_json = "1.0") and then
update this crate's Cargo.toml (the entries for serde and serde_json in
bin/testapp/Cargo.toml) to use workspace = true instead of specifying version =
"1.0"; ensure you remove the version keys here so the crate references the
workspace-managed serde and serde_json.
In `@bin/testapp/src/main.rs`:
- Around line 113-118: The helper function load_genesis_config (which calls
EvdGenesisConfig::load and logs the path) is duplicated across binaries—move it
into a shared module (e.g., create a new public function in
evolve_testapp::genesis_config or in evolve_node), make it pub fn
load_genesis_config(path: Option<&str>) -> Option<EvdGenesisConfig>, update the
binaries to import and call that single shared function, and delete the
duplicate definitions; ensure the new module exposes the same signature and
preserves the tracing::info and expect behavior so callers compile without other
changes.
In `@crates/app/node/src/lib.rs`:
- Around line 861-885: The gRPC server is being spawned without keeping its task
handle so it cannot be shutdown gracefully; modify the block that creates
GrpcServer (grpc_server) so you capture the JoinHandle (e.g., let grpc_handle =
tokio::spawn(...)) instead of discarding it, then store that handle alongside
the JSON-RPC handle and on shutdown call grpc_handle.abort() (or, if GrpcServer
supports a graceful shutdown API, hook its shutdown future into the existing
context_for_shutdown signal so serve() is awaited with that cancelation). Ensure
references to GrpcServer::with_subscription_manager, grpc_server.serve(), and
the new grpc_handle are updated accordingly.
In `@docker/evd/Dockerfile`:
- Around line 13-25: The Dockerfile currently runs the runtime image as root;
add a non-root user and group during the image build (e.g., create a dedicated
user via a RUN step after installing ca-certificates), chown the app WORKDIR and
any runtime-writable data directory (the directory used for persistent volume
mounts) to that user, and switch to that user with a USER instruction before the
ENTRYPOINT so /usr/local/bin/evd still gets copied by COPY but the process does
not run as root; ensure ownership changes occur after COPY so the non-root user
can write to the data directory at runtime.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.dockerignoreREADME.mdbin/evd/Cargo.tomlbin/evd/src/main.rsbin/testapp/Cargo.tomlbin/testapp/src/genesis_config.rsbin/testapp/src/lib.rsbin/testapp/src/main.rscrates/app/node/Cargo.tomlcrates/app/node/src/config.rscrates/app/node/src/lib.rsdocker-compose.ev-node.ymldocker-compose.ymldocker/evd/Dockerfile
💤 Files with no reviewable changes (1)
- bin/evd/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/evd/src/main.rs (1)
510-533:⚠️ Potential issue | 🔴 CriticalAdd validation for duplicate Ethereum addresses in genesis config.
The
EvdGenesisConfig::validate()method checks that addresses parse correctly but does not validate uniqueness. If the genesis config contains duplicateeth_addressentries,EthEoaAccountRef::initializewill be called multiple times for the same address, and duplicate balances will be passed toTokenRef::initialize. This can result in incorrect total supply or unexpected state management.Add a uniqueness check in
EvdGenesisConfig::validate()to reject configurations with duplicate addresses.🤖 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 510 - 533, EvdGenesisConfig::validate() currently only checks address parsing but must also reject duplicate Ethereum addresses to prevent multiple calls to EthEoaAccountRef::initialize and duplicate TokenRef::initialize balances; update validate() to parse each genesis_config.eth_address (or the same field used to build funded_accounts), insert the parsed [u8;20] (or canonical address type) into a HashSet and return an Err if insert returns false (duplicate found) with a clear error message referencing the offending address, ensuring downstream code that builds funded_accounts and calls EthEoaAccountRef::initialize / TokenRef::initialize can assume unique addresses.
♻️ Duplicate comments (1)
bin/testapp/src/main.rs (1)
236-240:⚠️ Potential issue | 🟠 MajorCustom genesis returns incorrect
alice/bobaccount IDs.Lines 237-238 set
aliceandbobtotoken.0, so both fields point to the token account instead of EOA account IDs. This breaks parity with default genesis output and can mislead downstream consumers.🛠️ Targeted fix
let scheduler_acc = SchedulerRef::initialize(vec![], vec![], env)?.0; scheduler_acc.update_begin_blockers(vec![], env)?; + let alice_id = funded_accounts + .first() + .map(|(eth_addr, _)| { + address_to_account_id(alloy_primitives::Address::from(*eth_addr)) + }) + .unwrap_or(PLACEHOLDER_ACCOUNT); + let bob_id = funded_accounts + .get(1) + .map(|(eth_addr, _)| { + address_to_account_id(alloy_primitives::Address::from(*eth_addr)) + }) + .unwrap_or(alice_id); + Ok(GenesisAccounts { - alice: token.0, - bob: token.0, + alice: alice_id, + bob: bob_id, atom: token.0, scheduler: scheduler_acc.0, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/testapp/src/main.rs` around lines 236 - 240, The GenesisAccounts construction is assigning alice and bob to token.0 (making them point to the token account) instead of their EOA IDs; update the fields in the GenesisAccounts initializer so alice uses alice_acc.0 and bob uses bob_acc.0 (leave token and scheduler as token.0 and scheduler_acc.0 respectively) — look for the GenesisAccounts instantiation near where token, alice_acc, bob_acc, and scheduler_acc are declared and swap token.0 for alice/bob to the corresponding alice_acc.0 and bob_acc.0.
🧹 Nitpick comments (2)
crates/app/node/src/config.rs (1)
70-78: Consider wiringgrpc.addrfromNodeConfigtoRpcConfig.The
NodeConfighas agrpcsection with address configuration (self.grpc.addr), butto_rpc_configalways setsgrpc_addr: None. Users configuringgrpc.addrin their YAML config might expect gRPC to be enabled automatically.If gRPC should require explicit opt-in, this is fine as-is. Otherwise, consider:
♻️ Proposed change to wire gRPC config
pub fn to_rpc_config(&self) -> RpcConfig { RpcConfig { http_addr: self.parsed_rpc_addr(), chain_id: self.chain.chain_id, enabled: self.rpc.enabled, enable_block_indexing: self.rpc.enable_block_indexing, - grpc_addr: None, + grpc_addr: Some(self.parsed_grpc_addr()), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/app/node/src/config.rs` around lines 70 - 78, The to_rpc_config function currently sets RpcConfig.grpc_addr to None; update to wire the NodeConfig grpc address into RpcConfig by reading self.grpc.addr (and/or self.grpc.enabled if gRPC should be opt-in) and assign the appropriate Option into RpcConfig.grpc_addr in the to_rpc_config method; locate the to_rpc_config function in config.rs and replace the hardcoded None with the NodeConfig value (e.g., Some/cloned or parsed self.grpc.addr) or conditionally set None when self.grpc.enabled is false to preserve explicit opt-in behavior.crates/app/node/src/lib.rs (1)
861-862: gRPC support is only wired inrun_dev_node_with_rpc_and_mempool_eth.The gRPC server integration is only implemented in this function. The other RPC-enabled function
run_dev_node_with_rpc(lines 387-526) does not checkrpc_config.grpc_addr, so gRPC won't start even if configured.If this is intentional (gRPC only supported with mempool-based ETH transactions), consider documenting this limitation in
RpcConfig::grpc_addr. Otherwise, consider adding gRPC support torun_dev_node_with_rpcas well.🤖 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 861 - 862, The RPC gRPC launch is only performed in run_dev_node_with_rpc_and_mempool_eth, but run_dev_node_with_rpc does not inspect RpcConfig::grpc_addr so a configured gRPC server will never start; either document this limitation on RpcConfig::grpc_addr or add the same gRPC start logic to run_dev_node_with_rpc: locate the conditional that checks rpc_config.grpc_addr (the grpc_addr binding and server-start sequence used in run_dev_node_with_rpc_and_mempool_eth) and copy/adapt that block into run_dev_node_with_rpc (ensuring any required variables and async tasks are wired the same way), or alternatively add a comment on RpcConfig::grpc_addr documenting that gRPC is only supported by run_dev_node_with_rpc_and_mempool_eth.
🤖 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/testapp/src/main.rs`:
- Around line 113-117: The code currently panics on invalid user input by
calling expect in load_genesis_config (and the other expect calls that parse
user-provided addresses around lines 204–207); change these to proper error
handling: in load_genesis_config replace the expect on EvdGenesisConfig::load
with non-panicking behavior (either change the function signature to return
Result<Option<EvdGenesisConfig>, E> and propagate the error, or keep Option and
catch the load error, log it via tracing::error! and return None), and similarly
replace the expect used for parsing addresses with a fallible parse that returns
a Result/Option and propagate or return a user-facing error instead of
panicking; ensure you update callers to handle the new Result/Option return
values so no user input can cause a process panic.
In `@crates/app/node/src/lib.rs`:
- Around line 880-885: The gRPC server task is spawned with tokio::spawn and its
JoinHandle is dropped so it never gets shut down gracefully; change the spawn
site where grpc_server.serve() is launched to capture and store the JoinHandle
(e.g., grpc_handle) instead of discarding it, make it an Option<JoinHandle<...>>
alongside the JSON-RPC handle, and during shutdown (after the tokio::select!
block where handle.stop() is called for the JSON-RPC server) call abort() on the
stored grpc_handle and log the stop (e.g., tracing::info!("gRPC server
stopped")) to ensure the gRPC task is terminated cleanly.
In `@docker/evd/Dockerfile`:
- Around line 13-25: The runtime stage runs /usr/local/bin/evd as root; create a
dedicated unprivileged user (e.g., "evd" or "evduser") in the runtime stage,
ensure ownership of runtime files (/app and /usr/local/bin/evd) is transferred
to that user, set a safe HOME if needed, and switch to that user with USER
before the ENTRYPOINT; update the runtime stage (the stage that currently COPYs
/app/target/release/evd and defines ENTRYPOINT) to add the user (using
groupadd/useradd or addgroup/adduser from apt if required), chown the binary and
workdir to the new user, and then add USER evd (or chosen name) so the container
no longer runs as root.
---
Outside diff comments:
In `@bin/evd/src/main.rs`:
- Around line 510-533: EvdGenesisConfig::validate() currently only checks
address parsing but must also reject duplicate Ethereum addresses to prevent
multiple calls to EthEoaAccountRef::initialize and duplicate
TokenRef::initialize balances; update validate() to parse each
genesis_config.eth_address (or the same field used to build funded_accounts),
insert the parsed [u8;20] (or canonical address type) into a HashSet and return
an Err if insert returns false (duplicate found) with a clear error message
referencing the offending address, ensuring downstream code that builds
funded_accounts and calls EthEoaAccountRef::initialize / TokenRef::initialize
can assume unique addresses.
---
Duplicate comments:
In `@bin/testapp/src/main.rs`:
- Around line 236-240: The GenesisAccounts construction is assigning alice and
bob to token.0 (making them point to the token account) instead of their EOA
IDs; update the fields in the GenesisAccounts initializer so alice uses
alice_acc.0 and bob uses bob_acc.0 (leave token and scheduler as token.0 and
scheduler_acc.0 respectively) — look for the GenesisAccounts instantiation near
where token, alice_acc, bob_acc, and scheduler_acc are declared and swap token.0
for alice/bob to the corresponding alice_acc.0 and bob_acc.0.
---
Nitpick comments:
In `@crates/app/node/src/config.rs`:
- Around line 70-78: The to_rpc_config function currently sets
RpcConfig.grpc_addr to None; update to wire the NodeConfig grpc address into
RpcConfig by reading self.grpc.addr (and/or self.grpc.enabled if gRPC should be
opt-in) and assign the appropriate Option into RpcConfig.grpc_addr in the
to_rpc_config method; locate the to_rpc_config function in config.rs and replace
the hardcoded None with the NodeConfig value (e.g., Some/cloned or parsed
self.grpc.addr) or conditionally set None when self.grpc.enabled is false to
preserve explicit opt-in behavior.
In `@crates/app/node/src/lib.rs`:
- Around line 861-862: The RPC gRPC launch is only performed in
run_dev_node_with_rpc_and_mempool_eth, but run_dev_node_with_rpc does not
inspect RpcConfig::grpc_addr so a configured gRPC server will never start;
either document this limitation on RpcConfig::grpc_addr or add the same gRPC
start logic to run_dev_node_with_rpc: locate the conditional that checks
rpc_config.grpc_addr (the grpc_addr binding and server-start sequence used in
run_dev_node_with_rpc_and_mempool_eth) and copy/adapt that block into
run_dev_node_with_rpc (ensuring any required variables and async tasks are wired
the same way), or alternatively add a comment on RpcConfig::grpc_addr
documenting that gRPC is only supported by
run_dev_node_with_rpc_and_mempool_eth.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.dockerignoreREADME.mdbin/evd/Cargo.tomlbin/evd/src/main.rsbin/testapp/Cargo.tomlbin/testapp/src/genesis_config.rsbin/testapp/src/lib.rsbin/testapp/src/main.rscrates/app/node/Cargo.tomlcrates/app/node/src/config.rscrates/app/node/src/lib.rsdocker-compose.ev-node.ymldocker-compose.ymldocker/evd/Dockerfile
💤 Files with no reviewable changes (1)
- bin/evd/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/evd/src/main.rs (1)
511-513: Prefer propagating custom-genesis parse errors instead of panicking.Line 513 uses
expect("invalid address in genesis config"). Today this is mostly guarded upstream, but keeping this path fallible (Result) makes the binary more robust to future call-site changes.As per coding guidelines, "Fail fast with assertions for invariants and explicit error handling in Rust."
Also applies to: 459-468
🤖 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 511 - 513, Replace the panic-causing expect on genesis_config.funded_accounts() with fallible error propagation: change the code in main (or the function that parses the custom genesis) to return a Result and use the ? operator (or map_err to provide a contextual error) on genesis_config.funded_accounts() so invalid addresses produce an Err instead of aborting; apply the same change to the other similar sites around the parse code (the block referenced at lines 459-468) so all genesis parsing errors propagate to the caller with contextual error messages rather than calling expect.
🤖 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/node/src/config.rs`:
- Line 76: NodeConfig currently always sets grpc_addr to
Some(self.parsed_grpc_addr()), which forces gRPC on; change the assignment so
grpc_addr remains optional by only populating it when the config indicates gRPC
is enabled (e.g., use the Option from parsed_grpc_addr or wrap
parsed_grpc_addr() with .ok() / conditional mapping). Update the code that
constructs grpc_addr (the grpc_addr field in NodeConfig and any use of
parsed_grpc_addr()) to produce an Option so grpc_addr is None when not
configured and Some(addr) only when parsed/explicitly enabled.
---
Nitpick comments:
In `@bin/evd/src/main.rs`:
- Around line 511-513: Replace the panic-causing expect on
genesis_config.funded_accounts() with fallible error propagation: change the
code in main (or the function that parses the custom genesis) to return a Result
and use the ? operator (or map_err to provide a contextual error) on
genesis_config.funded_accounts() so invalid addresses produce an Err instead of
aborting; apply the same change to the other similar sites around the parse code
(the block referenced at lines 459-468) so all genesis parsing errors propagate
to the caller with contextual error messages rather than calling expect.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Cargo.tomlbin/evd/src/main.rsbin/testapp/Cargo.tomlbin/testapp/src/genesis_config.rsbin/testapp/src/lib.rsbin/testapp/src/main.rscrates/app/node/src/config.rscrates/app/node/src/lib.rsdocker/evd/Dockerfile
🚧 Files skipped from review as they are similar to previous changes (4)
- docker/evd/Dockerfile
- bin/testapp/src/lib.rs
- bin/testapp/Cargo.toml
- crates/app/node/src/lib.rs
| chain_id: self.chain.chain_id, | ||
| enabled: self.rpc.enabled, | ||
| enable_block_indexing: self.rpc.enable_block_indexing, | ||
| grpc_addr: Some(self.parsed_grpc_addr()), |
There was a problem hiding this comment.
grpc_addr is no longer optional in config-driven startup
Line 76 always sets grpc_addr to Some(...), so gRPC becomes effectively always enabled for NodeConfig consumers. That conflicts with the “optional when configured” behavior and removes a disable path.
Suggested fix
diff --git a/crates/app/node/src/config.rs b/crates/app/node/src/config.rs
@@
- self.grpc
- .addr
- .parse::<SocketAddr>()
- .map_err(|e| format!("invalid grpc.addr '{}': {e}", self.grpc.addr))?;
+ if !self.grpc.addr.trim().is_empty() {
+ self.grpc
+ .addr
+ .parse::<SocketAddr>()
+ .map_err(|e| format!("invalid grpc.addr '{}': {e}", self.grpc.addr))?;
+ }
@@
- grpc_addr: Some(self.parsed_grpc_addr()),
+ grpc_addr: (!self.grpc.addr.trim().is_empty())
+ .then(|| self.parsed_grpc_addr()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/app/node/src/config.rs` at line 76, NodeConfig currently always sets
grpc_addr to Some(self.parsed_grpc_addr()), which forces gRPC on; change the
assignment so grpc_addr remains optional by only populating it when the config
indicates gRPC is enabled (e.g., use the Option from parsed_grpc_addr or wrap
parsed_grpc_addr() with .ok() / conditional mapping). Update the code that
constructs grpc_addr (the grpc_addr field in NodeConfig and any use of
parsed_grpc_addr()) to produce an Option so grpc_addr is None when not
configured and Some(addr) only when parsed/explicitly enabled.
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
Documentation
Chores