feat(ev-deployer): part 1 - add ev-deployer CLI for genesis contract allocation#167
Merged
feat(ev-deployer): part 1 - add ev-deployer CLI for genesis contract allocation#167
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (runs CLI)
participant FS as Filesystem (config / genesis.json)
participant CLI as ev-deployer
participant Node as ev-reth (RPC)
rect rgba(200,200,255,0.5)
Dev->>CLI: run `ev-deployer genesis --merge-into base.json --config devnet.toml`
CLI->>FS: read `devnet.toml`
CLI->>CLI: build contract GenesisContract (address, code, storage)
CLI->>FS: read `base.json`
CLI->>FS: merge alloc entries (check collisions, apply --force if set)
CLI->>FS: write merged genesis.json
end
rect rgba(200,255,200,0.5)
Dev->>Node: start `ev-reth --genesis merged.json`
Node->>Dev: exposes JSON-RPC
Dev->>Node: curl eth_getCode / eth_getStorageAt
Node->>Dev: return code and storage => verify matches expected
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
|
Claude finished @randygrok's task —— View job PR Review - ev-deployer CLI ImplementationI've completed a comprehensive review of the ev-deployer implementation. Here's my detailed feedback: Todo List:
Overall Assessment ✅This is a well-designed and implemented CLI tool. The architecture is clean, code quality is high, and the test coverage is good. The implementation follows Rust best practices and includes appropriate error handling. Strengths📐 Architecture & Design
🔒 Security
🧪 Test Coverage
Issues Found🟠 Major Issues (Already Addressed by CodeRabbit)
🟡 Minor IssuesCode Robustness:
Test Quality:
Performance Considerations ⚡
Code Quality Assessment 📊Rust Best Practices: ✅ Excellent
Documentation: ✅ Good
Error Handling: ✅ Excellent
Security Analysis 🔐✅ No security concerns identified:
Recommendations
SummaryThis is a high-quality implementation that follows excellent engineering practices. The modular design makes it easy to extend with additional contracts, and the comprehensive test suite provides confidence in the implementation. The issues identified are minor and mostly already addressed by the automated review feedback. The tool successfully achieves its goal of providing a declarative way to generate genesis configurations for ev-reth contracts, with good UX through the CLI interface and proper safety guards for merge operations. Recommendation: ✅ Approve - This PR is ready to merge after addressing the CodeRabbit feedback. |
randygrok
changed the title
randygrok
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ev_deployer.yml:
- Around line 6-13: The pull_request path filters in ev_deployer.yml only
include 'contracts/src/**', 'contracts/foundry.toml', and 'bin/ev-deployer/**',
so edits to root Cargo.toml, Cargo.lock, or the workflow itself won't trigger EV
Deployer CI and can break cargo test -p ev-deployer; update the
pull_request.paths block (the pull_request.paths entry in
.github/workflows/ev_deployer.yml) to also include the root-level files like
'Cargo.toml', 'Cargo.lock', and the workflow file (e.g.
'.github/workflows/ev_deployer.yml') so changes to those files will trigger the
workflow.
In `@bin/ev-deployer/README.md`:
- Around line 86-90: The README's description of the merge behavior is
misleading: update the text around the ev-deployer genesis command (referencing
the --merge-into and --output flags) to remove the phrase "modifies the `alloc`
field in-place" and explicitly state that the CLI does not mutate the original
file but writes the merged result to the path provided by --output (or to stdout
if --output is omitted); ensure the example command and accompanying sentence
clearly say the original genesis.json is unchanged and the merged output is
written to genesis-out.json (or stdout).
In `@bin/ev-deployer/src/config.rs`:
- Around line 83-101: The validate() method currently doesn't ensure contract
addresses are unique, allowing admin_proxy.address == fee_vault.address which
later causes silent alloc overwrites; add a check in validate() that if both
self.contracts.admin_proxy and self.contracts.fee_vault are Some, their .address
fields must not be equal and fail fast (use eyre::ensure or eyre::bail) with a
clear error like "admin_proxy.address must not equal fee_vault.address"; update
the validate() function to perform this uniqueness assertion alongside the
existing owner and bridge_share_bps checks.
In `@bin/ev-deployer/src/contracts/admin_proxy.rs`:
- Around line 68-73: The current normalization of forge output (building
forge_hex from output.stdout) uses unwraps and will panic on invalid UTF-8 or if
the "0x" prefix is missing; change it to handle errors safely by first
converting output.stdout with String::from_utf8 and propagating or logging the
error instead of unwrap, then perform trimming and lowercase on the resulting
string and remove a leading "0x" non-panically (e.g., use
strip_prefix("0x").unwrap_or(&s) or check starts_with and slice) to produce
forge_hex; update the code that assigns forge_hex (the variable constructed from
output.stdout) to return a Result or fallback value rather than panicking.
In `@bin/ev-deployer/src/contracts/fee_vault.rs`:
- Around line 171-176: The current normalization of forge output into forge_hex
uses multiple unwraps and can panic if the output is not valid UTF-8 or if the
"0x" prefix is missing; change it to a tolerant normalization that avoids
unwraps by using String::from_utf8_lossy on output.stdout and
trim_start_matches("0x") instead of strip_prefix/unwrap, e.g. build forge_hex
from
String::from_utf8_lossy(&output.stdout).trim().trim_start_matches("0x").to_lowercase()
so it safely handles missing "0x" and invalid UTF-8.
In `@bin/ev-deployer/src/main.rs`:
- Around line 71-75: When --merge-into is provided but no explicit --output is
given, the merged genesis currently only goes to stdout; change the control flow
so that when merge_into is Some(genesis_path) and output is None you persist the
merged result back to genesis_path. Concretely, after calling
genesis::merge_into(&cfg, genesis_path, force) (or using the merged value from
genesis::build_alloc when merging), write the resulting genesis bytes/string to
the same genesis_path (or call an existing genesis write helper) instead of only
printing to stdout; update the branch that handles merge_into (and the similar
branch around lines 79–84) to perform that write when output is unspecified.
In `@bin/ev-deployer/tests/e2e_genesis.sh`:
- Around line 35-38: The curl RPC calls that post JSON-RPC using RPC_URL with
variables method and params are currently blocking; update both curl invocations
(the ones that build the JSON payload with "$RPC_URL" and -d
"{\"jsonrpc\":\"2.0\",\"method\":\"$method\",\"params\":$params,\"id\":1}" and
the similar second call) to include connection and overall timeouts (for example
--connect-timeout 5 --max-time 10) so a stalled node/socket won’t hang CI; keep
the existing headers and piping to python3 unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 442c5b5a-2b3c-4deb-af81-3a72dbdffa67
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcontracts/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ev_deployer.ymlCargo.tomlbin/ev-deployer/Cargo.tomlbin/ev-deployer/README.mdbin/ev-deployer/examples/devnet.tomlbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/admin_proxy.rsbin/ev-deployer/src/contracts/fee_vault.rsbin/ev-deployer/src/contracts/mod.rsbin/ev-deployer/src/genesis.rsbin/ev-deployer/src/main.rsbin/ev-deployer/src/output.rsbin/ev-deployer/tests/e2e_genesis.shcontracts/foundry.tomljustfile
Comment on lines
+171
to
+176
| let forge_hex = String::from_utf8(output.stdout) | ||
| .unwrap() | ||
| .trim() | ||
| .strip_prefix("0x") | ||
| .unwrap() | ||
| .to_lowercase(); |
There was a problem hiding this comment.
Use non-panicking forge hex normalization in test.
Line 174/175 can panic if forge output unexpectedly omits 0x. Prefer tolerant normalization.
Suggested fix
- let forge_hex = String::from_utf8(output.stdout)
- .unwrap()
- .trim()
- .strip_prefix("0x")
- .unwrap()
- .to_lowercase();
+ let forge_hex = String::from_utf8(output.stdout)
+ .unwrap()
+ .trim()
+ .trim_start_matches("0x")
+ .to_lowercase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-deployer/src/contracts/fee_vault.rs` around lines 171 - 176, The
current normalization of forge output into forge_hex uses multiple unwraps and
can panic if the output is not valid UTF-8 or if the "0x" prefix is missing;
change it to a tolerant normalization that avoids unwraps by using
String::from_utf8_lossy on output.stdout and trim_start_matches("0x") instead of
strip_prefix/unwrap, e.g. build forge_hex from
String::from_utf8_lossy(&output.stdout).trim().trim_start_matches("0x").to_lowercase()
so it safely handles missing "0x" and invalid UTF-8.
Comment on lines
+71
to
+75
| let result = if let Some(ref genesis_path) = merge_into { | ||
| genesis::merge_into(&cfg, genesis_path, force)? | ||
| } else { | ||
| genesis::build_alloc(&cfg) | ||
| }; |
There was a problem hiding this comment.
--merge-into without --output drops merged result to stdout only.
When users pass --merge-into alone, the merged genesis is not written back to the source file. That makes merge behavior easy to misuse and lose.
Suggested fix (persist merge target by default)
- if let Some(ref out_path) = output {
- std::fs::write(out_path, &json)?;
- eprintln!("Wrote alloc to {}", out_path.display());
- } else {
- println!("{json}");
- }
+ if let Some(target_path) = output.as_ref().or(merge_into.as_ref()) {
+ std::fs::write(target_path, &json)?;
+ eprintln!("Wrote alloc to {}", target_path.display());
+ } else {
+ println!("{json}");
+ }Also applies to: 79-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-deployer/src/main.rs` around lines 71 - 75, When --merge-into is
provided but no explicit --output is given, the merged genesis currently only
goes to stdout; change the control flow so that when merge_into is
Some(genesis_path) and output is None you persist the merged result back to
genesis_path. Concretely, after calling genesis::merge_into(&cfg, genesis_path,
force) (or using the merged value from genesis::build_alloc when merging), write
the resulting genesis bytes/string to the same genesis_path (or call an existing
genesis write helper) instead of only printing to stdout; update the branch that
handles merge_into (and the similar branch around lines 79–84) to perform that
write when output is unspecified.
- Expand CI workflow path triggers to include Cargo.toml, Cargo.lock,
and the workflow file itself
- Fix README merge behavior description (not in-place)
- Validate contract addresses are unique in config
- Use trim_start_matches("0x") instead of strip_prefix().unwrap() in
bytecode verification tests
- Add curl timeouts to e2e test RPC calls to prevent CI hangs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
bin/ev-deployer/src/contracts/fee_vault.rs (1)
171-175:⚠️ Potential issue | 🟡 MinorAvoid panic in forge output decoding path.
Line 171 still uses
String::from_utf8(output.stdout).unwrap(), which can panic on unexpected bytes. Use lossy decoding to keep this parity test tolerant.Proposed fix
- let forge_hex = String::from_utf8(output.stdout) - .unwrap() + let forge_hex = String::from_utf8_lossy(&output.stdout) .trim() .trim_start_matches("0x") .to_lowercase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-deployer/src/contracts/fee_vault.rs` around lines 171 - 175, The current forge_hex construction uses String::from_utf8(output.stdout).unwrap() which can panic on invalid UTF-8; change the decoding to a lossy form (e.g. use String::from_utf8_lossy on output.stdout) before trim/trim_start_matches/to_lowercase so the parity test tolerates unexpected bytes and avoids panics when building forge_hex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@bin/ev-deployer/src/contracts/fee_vault.rs`:
- Around line 171-175: The current forge_hex construction uses
String::from_utf8(output.stdout).unwrap() which can panic on invalid UTF-8;
change the decoding to a lossy form (e.g. use String::from_utf8_lossy on
output.stdout) before trim/trim_start_matches/to_lowercase so the parity test
tolerates unexpected bytes and avoids panics when building forge_hex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7550381-d587-49ad-b622-1fca28fe6555
📒 Files selected for processing (6)
.github/workflows/ev_deployer.ymlbin/ev-deployer/README.mdbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/admin_proxy.rsbin/ev-deployer/src/contracts/fee_vault.rsbin/ev-deployer/tests/e2e_genesis.sh
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/ev_deployer.yml
- bin/ev-deployer/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/ev-deployer/src/config.rs
- bin/ev-deployer/src/contracts/admin_proxy.rs
- bin/ev-deployer/tests/e2e_genesis.sh
tac0turtle
reviewed
Mar 23, 2026
| ev-deployer compute-address --config deploy.toml --contract admin_proxy | ||
| ``` | ||
|
|
||
| ## Contracts |
Contributor
There was a problem hiding this comment.
are these optional to deploy?
Contributor
Author
There was a problem hiding this comment.
I think I am gonna move them into ev-deployer directly.
tac0turtle
reviewed
Mar 23, 2026
Contributor
tac0turtle
left a comment
tac0turtle
left a comment
There was a problem hiding this comment.
LGTM, left one comment on optionality but otherwise super nice job
tac0turtle
approved these changes
Mar 23, 2026
FeeVault will be added in a separate PR to keep this one focused on AdminProxy only.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bin/ev-deployer/tests/e2e_genesis.sh (2)
79-83: Use structured JSON validation for alloc presence instead of substring grep.
grepcan false-positive on incidental matches and does not prove the address exists as an alloc key.Suggested refactor
-# Quick sanity: address should be in the alloc -grep -q "000000000000000000000000000000000000Ad00" "$GENESIS" \ - || fail "AdminProxy address not found in genesis" +python3 - "$GENESIS" <<'PY' || fail "AdminProxy address not found in genesis alloc" +import json, sys +genesis_path = sys.argv[1] +with open(genesis_path, "r", encoding="utf-8") as f: + data = json.load(f) +alloc = data.get("alloc", {}) +target = "000000000000000000000000000000000000ad00" +keys = {k.lower()[2:] if k.lower().startswith("0x") else k.lower() for k in alloc.keys()} +if target not in keys: + raise SystemExit(1) +PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-deployer/tests/e2e_genesis.sh` around lines 79 - 83, Replace the fragile substring grep check that looks for "000000000000000000000000000000000000Ad00" in "$GENESIS" with a structured JSON key existence check using a JSON parser (e.g., jq) to assert the address is present under the "alloc" object; update the test to call jq on "$GENESIS" to test .alloc["0x000000000000000000000000000000000000Ad00"] (or the canonical address form used elsewhere) and call fail() if jq indicates the key is missing, otherwise call pass() — locate this logic around the existing GENESIS variable reference and the fail/pass calls in the e2e_genesis.sh test.
109-125: Derive expected AdminProxy values from test input instead of hardcoding literals.Duplicating
ADMIN_PROXY/owner as constants makes this flow less sensitive to regressions in config ingestion. Prefer sourcing expected values from the same deploy input (or generated addresses manifest) before asserting RPC state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-deployer/tests/e2e_genesis.sh` around lines 109 - 125, The test currently hardcodes expected AdminProxy values; change it to derive expectations from the test inputs by using the existing ADMIN_OWNER (and ADMIN_PROXY) variables rather than literal addresses: build expected_owner_slot by taking ADMIN_OWNER, stripping the 0x, lowercasing it and left-padding with zeros to 64 hex chars and prefixing 0x, then compare that to admin_slot0 (normalized to lowercase) produced by rpc_call; keep using ADMIN_PROXY when fetching admin_code and use fail/pass as before. Ensure you reference the symbols ADMIN_OWNER, ADMIN_PROXY, admin_code, admin_slot0, expected_owner_slot, rpc_call and fail so the logic remains robust to changes in deploy inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/ev-deployer/tests/e2e_genesis.sh`:
- Around line 79-83: Replace the fragile substring grep check that looks for
"000000000000000000000000000000000000Ad00" in "$GENESIS" with a structured JSON
key existence check using a JSON parser (e.g., jq) to assert the address is
present under the "alloc" object; update the test to call jq on "$GENESIS" to
test .alloc["0x000000000000000000000000000000000000Ad00"] (or the canonical
address form used elsewhere) and call fail() if jq indicates the key is missing,
otherwise call pass() — locate this logic around the existing GENESIS variable
reference and the fail/pass calls in the e2e_genesis.sh test.
- Around line 109-125: The test currently hardcodes expected AdminProxy values;
change it to derive expectations from the test inputs by using the existing
ADMIN_OWNER (and ADMIN_PROXY) variables rather than literal addresses: build
expected_owner_slot by taking ADMIN_OWNER, stripping the 0x, lowercasing it and
left-padding with zeros to 64 hex chars and prefixing 0x, then compare that to
admin_slot0 (normalized to lowercase) produced by rpc_call; keep using
ADMIN_PROXY when fetching admin_code and use fail/pass as before. Ensure you
reference the symbols ADMIN_OWNER, ADMIN_PROXY, admin_code, admin_slot0,
expected_owner_slot, rpc_call and fail so the logic remains robust to changes in
deploy inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16ba6325-c7f2-4619-9c3a-119a04863a7f
📒 Files selected for processing (8)
bin/ev-deployer/README.mdbin/ev-deployer/examples/devnet.tomlbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/mod.rsbin/ev-deployer/src/genesis.rsbin/ev-deployer/src/main.rsbin/ev-deployer/src/output.rsbin/ev-deployer/tests/e2e_genesis.sh
✅ Files skipped from review due to trivial changes (2)
- bin/ev-deployer/src/contracts/mod.rs
- bin/ev-deployer/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- bin/ev-deployer/src/output.rs
- bin/ev-deployer/examples/devnet.toml
- bin/ev-deployer/src/main.rs
- bin/ev-deployer/src/config.rs
- bin/ev-deployer/src/genesis.rs
Keep only the CLI framework (config, genesis merge, manifest output) without any contract implementations. Contracts will be added in subsequent PRs.
This reverts commit 089ef22.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
bin/ev-deployer/src/genesis.rs (1)
109-118: Cover the collision paths in tests.This test only proves that an existing
allocentry survives whenbuild_alloc()returns{}. It does not exercise theaddress collisionerror or theforceoverwrite branch, which are the core behaviors of this module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-deployer/src/genesis.rs` around lines 109 - 118, Add tests that exercise the address-collision and force-overwrite branches of merge_into: create a temp genesis file containing an alloc entry for a specific address, then call merge_into(&test_config(), tmp.path(), false) with build_alloc() returning a colliding address and assert it returns an error describing the collision; then call merge_into(&test_config(), tmp.path(), true) and assert the alloc entry is replaced/updated (e.g., the value written reflects the new build_alloc result). Reference the existing test merge_into_existing_genesis and the merge_into function (and the code path that checks/overwrites alloc entries) to add two new tests — one expecting a collision error and one verifying force=true overwrites the existing alloc entry.
🤖 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/ev-deployer/src/genesis.rs`:
- Around line 31-35: Normalize alloc keys before checking collisions and
inserting: convert both the incoming key (addr from new_alloc) and existing keys
in genesis_alloc to a canonical address form (e.g., lowercase hex without 0x or
a checksum-normalized form) before calling genesis_alloc.contains_key(addr) and
before genesis_alloc.insert(...). In practice, compute a canonical_addr for each
addr (and ensure genesis_alloc is keyed by canonical forms or compare against
canonicalized existing keys) inside the loop that iterates new_alloc (the block
using variables addr and entry) so collision detection and insertion use the
same normalized representation.
In `@bin/ev-deployer/src/main.rs`:
- Around line 93-98: The ComputeAddress subcommand currently ignores the
provided config and always bails with "unknown contract"; update the
Command::ComputeAddress handling to first load and validate the config (use the
passed-in config path instead of _config_path so errors are surfaced) and
propagate any config file errors, then match on the contract name and return a
clear "not implemented yet" error for unsupported contracts (or remove/gate the
subcommand for part 1 if you prefer). Specifically, replace the unconditional
eyre::bail! in the Command::ComputeAddress arm with code that attempts to
read/parse the config, reports config load failures, and only then returns an
explicit not-implemented error for the contract branch of compute-address.
---
Nitpick comments:
In `@bin/ev-deployer/src/genesis.rs`:
- Around line 109-118: Add tests that exercise the address-collision and
force-overwrite branches of merge_into: create a temp genesis file containing an
alloc entry for a specific address, then call merge_into(&test_config(),
tmp.path(), false) with build_alloc() returning a colliding address and assert
it returns an error describing the collision; then call
merge_into(&test_config(), tmp.path(), true) and assert the alloc entry is
replaced/updated (e.g., the value written reflects the new build_alloc result).
Reference the existing test merge_into_existing_genesis and the merge_into
function (and the code path that checks/overwrites alloc entries) to add two new
tests — one expecting a collision error and one verifying force=true overwrites
the existing alloc entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c54e065-9fde-42e2-8e3e-9a85c37a9ff9
📒 Files selected for processing (8)
bin/ev-deployer/README.mdbin/ev-deployer/examples/devnet.tomlbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/mod.rsbin/ev-deployer/src/genesis.rsbin/ev-deployer/src/main.rsbin/ev-deployer/src/output.rsbin/ev-deployer/tests/e2e_genesis.sh
✅ Files skipped from review due to trivial changes (3)
- bin/ev-deployer/examples/devnet.toml
- bin/ev-deployer/README.md
- bin/ev-deployer/src/config.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/ev-deployer/src/contracts/mod.rs
- bin/ev-deployer/tests/e2e_genesis.sh
- bin/ev-deployer/src/output.rs
Default to an empty ContractsConfig when the section is omitted, so a minimal config only needs [chain].
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/ev-deployer/src/config.rs (1)
50-60: Consider validating thatadmin_proxy.addressis non-zero.The validation ensures
owneris not the zero address, butaddress(the deployment target) is not checked. A zero address would place the contract at0x0, which is almost certainly unintended.Suggested fix
fn validate(&self) -> eyre::Result<()> { if let Some(ref ap) = self.contracts.admin_proxy { + eyre::ensure!( + !ap.address.is_zero(), + "admin_proxy.address must not be the zero address" + ); eyre::ensure!( !ap.owner.is_zero(), "admin_proxy.owner must not be the zero address" ); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ev-deployer/src/config.rs` around lines 50 - 60, The validate() method currently checks admin_proxy.owner but not admin_proxy.address; update validate (the fn validate(&self) -> eyre::Result<()>) to also ensure the admin_proxy.address is not the zero address by adding an eyre::ensure!(!ap.address.is_zero(), "admin_proxy.address must not be the zero address") alongside the existing owner check so both ap.owner and ap.address are validated.
🤖 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/ev-deployer/src/main.rs`:
- Around line 52-53: The help text for the --contract arg mentions "fee_vault"
but only "admin_proxy" is handled, so either remove "fee_vault" from the
docstring or implement support for it in the command handling; to fix, either
update the doc comment above the #[arg(long)] so it lists only "admin_proxy", or
add a matching branch in the ComputeAddress logic (e.g., the match handling the
contract name in ComputeAddress) that recognizes "fee_vault" and
computes/returns the correct address/behavior for it; ensure the symbol
ComputeAddress (and whatever enum/handler that matches contract names) is
updated accordingly so help text and runtime behavior remain consistent.
---
Nitpick comments:
In `@bin/ev-deployer/src/config.rs`:
- Around line 50-60: The validate() method currently checks admin_proxy.owner
but not admin_proxy.address; update validate (the fn validate(&self) ->
eyre::Result<()>) to also ensure the admin_proxy.address is not the zero address
by adding an eyre::ensure!(!ap.address.is_zero(), "admin_proxy.address must not
be the zero address") alongside the existing owner check so both ap.owner and
ap.address are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f93c772-ab31-42a1-b97a-0d88098125c9
📒 Files selected for processing (9)
bin/ev-deployer/README.mdbin/ev-deployer/examples/devnet.tomlbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/admin_proxy.rsbin/ev-deployer/src/contracts/mod.rsbin/ev-deployer/src/genesis.rsbin/ev-deployer/src/main.rsbin/ev-deployer/src/output.rsbin/ev-deployer/tests/e2e_genesis.sh
✅ Files skipped from review due to trivial changes (3)
- bin/ev-deployer/examples/devnet.toml
- bin/ev-deployer/src/contracts/mod.rs
- bin/ev-deployer/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- bin/ev-deployer/src/output.rs
- bin/ev-deployer/tests/e2e_genesis.sh
- bin/ev-deployer/src/genesis.rs
Generates a TOML config template with all supported contracts commented out and documented.
…ence Move Init subcommand first in help output and remove leftover fee_vault mention from --contract flag doc.
Canonicalize address keys (lowercase, strip 0x) before comparing during merge, so collisions are detected regardless of case or prefix in the existing genesis file.
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
3 participants
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.
Part of #119
Implements Part 1 (Core) of EV Deployer — a CLI tool for generating genesis alloc entries for ev-reth contracts via declarative TOML configuration.
The tool reads a TOML config file, encodes contract bytecode and storage state, and generates Ethereum-compatible genesis alloc JSON. Supports merging into existing genesis files with collision detection.
CLI commands
initgenesiscompute-addressgenesisflags--config— path to deploy TOML--output— write JSON to file (stdout if omitted)--merge-into— merge alloc entries into an existing genesis JSON--force— allow overwriting existing addresses when merging--addresses-out— export a JSON manifest of contract name → addressContracts supported
admin_proxyWhat's included
init,genesis,compute-address)[chain]required,[contracts]optional)FeeVault and other contracts will be added in follow-up PRs.