feat(ev-deployer): part 3 - add Permit2 contract support#182
Open
feat(ev-deployer): part 3 - add Permit2 contract support#182
Conversation
…e patching Add Hyperlane monorepo as git submodule pinned to @hyperlane-xyz/core@11.0.3. Implement bytecode patching utility for Solidity immutable variables and MerkleTreeHook genesis alloc generation with mailbox/localDomain/deployedBlock patching. This is the foundation for embedding Hyperlane contracts at genesis.
# Conflicts: # Cargo.lock
…deployer-merkle-tree-hook
The MerkleTreeHook bytecode test needs OpenZeppelin dependencies from the Hyperlane monorepo, which are managed by soldeer.
Verify bytecode, storage slots, and patched immutables (mailbox, localDomain, deployedBlock) for the MerkleTreeHook contract via RPC.
Rustdoc interprets [644], [578], and [32] as intra-doc links, causing the docs CI job to fail with -D warnings.
Add Uniswap Permit2 as a genesis-deployable contract with EIP-712 immutable patching (_CACHED_CHAIN_ID, _CACHED_DOMAIN_SEPARATOR).
📝 WalkthroughWalkthroughAdds Permit2 support and immutable bytecode patching to the ev-deployer toolchain, wires Permit2 into config, manifest, genesis allocation, and CLI, and refactors FeeVault from Hyperlane bridging to local fee distribution; updates tests, docs, and adds a permit2 git submodule. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Deployment Config
participant Genesis as Genesis Builder
participant Permit2 as Permit2 Module
participant Patcher as Immutables Patcher
participant Alloc as Genesis Alloc
Config->>Genesis: Load deploy config (permit2 optional)
Genesis->>Permit2: build(permit2_config, chain_id)
Permit2->>Permit2: load embedded runtime bytecode with placeholders
Permit2->>Patcher: patch_u256(bytecode, _CACHED_CHAIN_ID, chain_id)
Patcher->>Patcher: write chain_id bytes into bytecode offsets
Permit2->>Permit2: compute domain_separator (EIP‑712)
Permit2->>Patcher: patch_bytes(bytecode, _CACHED_DOMAIN_SEPARATOR, domain_separator)
Patcher->>Patcher: write domain separator into bytecode offsets
Permit2->>Genesis: return GenesisContract(address, patched_code)
Genesis->>Alloc: insert permit2 GenesisContract into alloc map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
randygrok
changed the title
|
Claude finished @randygrok's task —— View job PR Review - Comprehensive AnalysisI've completed a thorough review of PR #182 implementing Permit2 contract support and FeeVault refactoring. Here's my assessment: Todo List:
SummaryThis PR successfully adds Uniswap Permit2 contract support while refactoring FeeVault from Hyperlane cross-chain bridging to local fee distribution. The implementation includes sophisticated bytecode patching for EIP-712 immutables and comprehensive testing. Code Quality Assessment ⭐⭐⭐⭐⭐Strengths:
Security Review 🔒Positive Security Practices:
Security Concerns Identified:
Bug Analysis 🐛Critical Issues Found:
Test Coverage Gap:
Performance Considerations ⚡
Implementation QualityPermit2 Integration:
FeeVault Refactoring:
RecommendationsHigh Priority
Medium Priority
Low Priority
CodeRabbit Review AlignmentThe CodeRabbit automated review identified similar issues, particularly around:
I concur with most CodeRabbit findings and have provided additional context around security implications and implementation quality. Overall AssessmentThis is a well-engineered PR that successfully implements complex bytecode patching while maintaining code quality and security standards. The implementation demonstrates strong understanding of EVM internals and careful attention to testing. Recommendation: APPROVE with requested fixes for the identified bugs, particularly the genesis collision issue. |
randygrok
changed the title
…racts Add three Hyperlane core contracts to ev-deployer so the full messaging stack can be embedded at genesis without post-deploy transactions. - Mailbox: core messaging hub with localDomain/deployedBlock immutables - NoopIsm: stateless ISM that accepts all messages (for devnet) - ProtocolFee: post-dispatch hook with MAX_PROTOCOL_FEE immutable
…lean ci build The embedded bytecodes were compiled with --extra-output storageLayout which subtly altered the output. Regenerated from a clean ci profile build to match what forge produces without extra flags.
…noop_ism, protocol_fee)
- 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
…s validation Merge ev-deployer-part1-core into ev-deployer-merkle-tree-hook, resolving config.rs conflicts by keeping both the MerkleTreeHook/ProtocolFee validations and the new duplicate-address check between AdminProxy and FeeVault.
FeeVault will be added in a separate PR to keep this one focused on AdminProxy only.
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.
Default to an empty ContractsConfig when the section is omitted, so a minimal config only needs [chain].
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.
…deployer-merkle-tree-hook
…xy and Permit2 Remove FeeVault, Mailbox, MerkleTreeHook, NoopIsm, and ProtocolFee contracts along with their config, validation, tests, docs, and e2e checks. This scopes the permit2 branch to only Part 1 (core) and Part 3 (Permit2) functionality.
Resolve conflicts keeping AdminProxy + Permit2 only (no Hyperlane). Remove leftover Hyperlane soldeer CI step from workflow.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
.gitmodules (1)
4-6: Remove the unused Hyperlane submodule to reduce supply-chain surface.The
contracts/lib/hyperlane-monoreposubmodule has no references in the codebase and is out of scope for this Permit2-focused PR. Removing it will reduce dependencies without impacting functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitmodules around lines 4 - 6, Remove the unused Hyperlane submodule by deleting its entry from .gitmodules (the submodule named "contracts/lib/hyperlane-monorepo" with path contracts/lib/hyperlane-monorepo and URL), then remove the submodule from the repository state and working tree (ensure the contracts/lib/hyperlane-monorepo directory and any .git/modules/contracts/lib/hyperlane-monorepo metadata are removed and the submodule is removed from the index) so no references remain in the repo; commit the change to .gitmodules and the removal so the dependency is fully gone.bin/ev-deployer/tests/e2e_genesis.sh (1)
80-83: Check alloc keys structurally instead of grepping full JSON text.
grep -qican false-pass if the address appears outsideallockeys. Parse JSON and assert keys directly.🔍 Proposed robustness update
-grep -qi "000000000000000000000000000000000000Ad00" "$GENESIS" \ - || fail "AdminProxy address not found in genesis" -grep -qi "000000000022D473030F116dDEE9F6B43aC78BA3" "$GENESIS" \ - || fail "Permit2 address not found in genesis" +python3 - "$GENESIS" <<'PY' || fail "required contract addresses not found in genesis alloc" +import json, sys +with open(sys.argv[1], "r", encoding="utf-8") as f: + alloc = json.load(f).get("alloc", {}) +keys = {k.lower().removeprefix("0x") for k in alloc.keys()} +required = { + "000000000000000000000000000000000000ad00", + "000000000022d473030f116ddee9f6b43ac78ba3", +} +missing = sorted(required - keys) +if missing: + raise SystemExit(f"missing alloc keys: {missing}") +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 80 - 83, Replace the fragile grep checks that search the whole GENESIS JSON with structured JSON key checks: use jq to extract the keys under the top-level "alloc" object from $GENESIS and assert that the specific addresses "000000000000000000000000000000000000Ad00" and "000000000022D473030F116dDEE9F6B43aC78BA3" are present as alloc keys (normalize both extracted keys and expected addresses to the same case before comparing). Update the two checks that currently call grep -qi to instead call jq to list alloc keys and then call the existing fail function if the expected address is not found. Ensure you reference GENESIS and the exact addresses shown when implementing the jq-based assertions.
🤖 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/README.md`:
- Around line 32-37: Replace the truncated canonical Permit2 address in the
README section `[contracts.permit2]` (the `address` field description) with the
full canonical Permit2 address; update the description string so it contains the
complete address (e.g., 0x000000000022D473030F116dDEE9F6B43aC78BA3) to allow
accurate copy/paste for configs and avoid the `...` truncation.
In `@bin/ev-deployer/src/config.rs`:
- Around line 44-49: The Permit2Config currently accepts a zero address; add a
runtime validation to reject Address::zero() for Permit2 by implementing a
validation method (e.g., Permit2Config::validate) or a TryFrom/try_into
conversion that is called after deserialization and returns an error if
self.address == Address::zero(), and ensure whatever config-loading code invokes
this validation so parsing fails early with a clear error when
contracts.permit2.address is zero.
In `@bin/ev-deployer/src/contracts/immutables.rs`:
- Around line 25-35: The patch_bytes function currently assumes each
ImmutableRef length equals the 32-byte value but doesn't enforce it, causing
copy_from_slice to panic with unclear errors; add an explicit check inside the
loop that r.length == value.len() (or assert_eq!(r.length, 32, "...")) before
the bounds assert/copy so failures are deterministic and self-explanatory,
referencing the ImmutableRef fields (r.start, r.length) and value length in the
assertion message.
In `@bin/ev-deployer/src/genesis.rs`:
- Around line 20-23: The insertion of the Permit2 contract can silently
overwrite an existing entry because insert_contract uses Map::insert; update
insert_contract (or the call site) to detect duplicate deploy-config addresses
and fail instead of replacing: before calling insert_contract (or within
insert_contract), check whether the allocation map already contains the
contract's address (use the same unique identifier produced by
contracts::permit2::build, e.g., the contract.address or contract.id), and if it
does return an error or panic with a clear message indicating a duplicate
configured contract address (include the conflicting address and the two
contract identifiers); alternatively use Map::entry and on Occupied(_) return
Err to prevent the silent overwrite.
In `@contracts/lib/permit2`:
- Line 1: The contracts/lib/permit2 submodule is not checked out to the pinned
commit cc56ad0f3439c502c246fc5cfcc3db92bb8b7219 referenced in
bin/ev-deployer/src/contracts/permit2.rs; update the submodule to the correct
commit by running git submodule update --init --recursive and then git -C
contracts/lib/permit2 fetch && git -C contracts/lib/permit2 checkout
cc56ad0f3439c502c246fc5cfcc3db92bb8b7219, verify the commit matches the
abbreviated cc56ad0 in bin/ev-deployer/src/contracts/permit2.rs, and commit the
updated submodule reference so the repo index points to that exact commit before
merging.
---
Nitpick comments:
In @.gitmodules:
- Around line 4-6: Remove the unused Hyperlane submodule by deleting its entry
from .gitmodules (the submodule named "contracts/lib/hyperlane-monorepo" with
path contracts/lib/hyperlane-monorepo and URL), then remove the submodule from
the repository state and working tree (ensure the
contracts/lib/hyperlane-monorepo directory and any
.git/modules/contracts/lib/hyperlane-monorepo metadata are removed and the
submodule is removed from the index) so no references remain in the repo; commit
the change to .gitmodules and the removal so the dependency is fully gone.
In `@bin/ev-deployer/tests/e2e_genesis.sh`:
- Around line 80-83: Replace the fragile grep checks that search the whole
GENESIS JSON with structured JSON key checks: use jq to extract the keys under
the top-level "alloc" object from $GENESIS and assert that the specific
addresses "000000000000000000000000000000000000Ad00" and
"000000000022D473030F116dDEE9F6B43aC78BA3" are present as alloc keys (normalize
both extracted keys and expected addresses to the same case before comparing).
Update the two checks that currently call grep -qi to instead call jq to list
alloc keys and then call the existing fail function if the expected address is
not found. Ensure you reference GENESIS and the exact addresses shown when
implementing the jq-based assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77369cc7-cbce-43ea-aae9-f89bda6a02c0
📒 Files selected for processing (14)
.gitmodulesbin/ev-deployer/README.mdbin/ev-deployer/examples/devnet.tomlbin/ev-deployer/src/config.rsbin/ev-deployer/src/contracts/immutables.rsbin/ev-deployer/src/contracts/mod.rsbin/ev-deployer/src/contracts/permit2.rsbin/ev-deployer/src/genesis.rsbin/ev-deployer/src/init_template.tomlbin/ev-deployer/src/main.rsbin/ev-deployer/src/output.rsbin/ev-deployer/tests/e2e_genesis.shcontracts/lib/hyperlane-monorepocontracts/lib/permit2
Comment on lines
+32
to
+37
| #### `[contracts.permit2]` | ||
|
|
||
| | Field | Type | Description | | ||
| |-----------|---------|----------------------------------------------------------| | ||
| | `address` | address | Address to deploy at (canonical: `0x000000000022D473...`) | | ||
|
|
There was a problem hiding this comment.
Use the full canonical Permit2 address in the config reference.
Line 36 truncates the canonical address with ..., which is error-prone for copy/paste in config docs.
📝 Proposed doc fix
-| `address` | address | Address to deploy at (canonical: `0x000000000022D473...`) |
+| `address` | address | Address to deploy at (canonical: `0x000000000022D473030F116dDEE9F6B43aC78BA3`) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-deployer/README.md` around lines 32 - 37, Replace the truncated
canonical Permit2 address in the README section `[contracts.permit2]` (the
`address` field description) with the full canonical Permit2 address; update the
description string so it contains the complete address (e.g.,
0x000000000022D473030F116dDEE9F6B43aC78BA3) to allow accurate copy/paste for
configs and avoid the `...` truncation.
Comment on lines
+44
to
+49
| /// `Permit2` configuration (Uniswap token approval manager). | ||
| #[derive(Debug, Deserialize)] | ||
| pub(crate) struct Permit2Config { | ||
| /// Address to deploy at. | ||
| pub address: Address, | ||
| } |
There was a problem hiding this comment.
Validate contracts.permit2.address is non-zero.
Right now a zero Permit2 address is accepted, which can produce an invalid deployment target without early feedback.
Suggested fix
fn validate(&self) -> eyre::Result<()> {
if let Some(ref ap) = self.contracts.admin_proxy {
eyre::ensure!(
!ap.owner.is_zero(),
"admin_proxy.owner must not be the zero address"
);
}
+
+ if let Some(ref p2) = self.contracts.permit2 {
+ eyre::ensure!(
+ !p2.address.is_zero(),
+ "permit2.address 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 44 - 49, The Permit2Config
currently accepts a zero address; add a runtime validation to reject
Address::zero() for Permit2 by implementing a validation method (e.g.,
Permit2Config::validate) or a TryFrom/try_into conversion that is called after
deserialization and returns an error if self.address == Address::zero(), and
ensure whatever config-loading code invokes this validation so parsing fails
early with a clear error when contracts.permit2.address is zero.
Comment on lines
+25
to
+35
| pub(crate) fn patch_bytes(bytecode: &mut [u8], refs: &[ImmutableRef], value: &[u8; 32]) { | ||
| for r in refs { | ||
| assert!( | ||
| r.start + r.length <= bytecode.len(), | ||
| "immutable ref out of bounds: start={} length={} bytecode_len={}", | ||
| r.start, | ||
| r.length, | ||
| bytecode.len() | ||
| ); | ||
| bytecode[r.start..r.start + r.length].copy_from_slice(value); | ||
| } |
There was a problem hiding this comment.
Enforce immutable word-size invariant explicitly.
patch_bytes accepts arbitrary length, but copy_from_slice(value) requires exactly 32 bytes. Add an explicit check so failures are deterministic and self-explanatory.
Suggested fix
pub(crate) fn patch_bytes(bytecode: &mut [u8], refs: &[ImmutableRef], value: &[u8; 32]) {
for r in refs {
+ assert!(
+ r.length == value.len(),
+ "immutable ref length mismatch: expected={} got={} start={}",
+ value.len(),
+ r.length,
+ r.start
+ );
assert!(
r.start + r.length <= bytecode.len(),
"immutable ref out of bounds: start={} length={} bytecode_len={}",
r.start,
r.length,
bytecode.len()
);
bytecode[r.start..r.start + r.length].copy_from_slice(value);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub(crate) fn patch_bytes(bytecode: &mut [u8], refs: &[ImmutableRef], value: &[u8; 32]) { | |
| for r in refs { | |
| assert!( | |
| r.start + r.length <= bytecode.len(), | |
| "immutable ref out of bounds: start={} length={} bytecode_len={}", | |
| r.start, | |
| r.length, | |
| bytecode.len() | |
| ); | |
| bytecode[r.start..r.start + r.length].copy_from_slice(value); | |
| } | |
| pub(crate) fn patch_bytes(bytecode: &mut [u8], refs: &[ImmutableRef], value: &[u8; 32]) { | |
| for r in refs { | |
| assert!( | |
| r.length == value.len(), | |
| "immutable ref length mismatch: expected={} got={} start={}", | |
| value.len(), | |
| r.length, | |
| r.start | |
| ); | |
| assert!( | |
| r.start + r.length <= bytecode.len(), | |
| "immutable ref out of bounds: start={} length={} bytecode_len={}", | |
| r.start, | |
| r.length, | |
| bytecode.len() | |
| ); | |
| bytecode[r.start..r.start + r.length].copy_from_slice(value); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/ev-deployer/src/contracts/immutables.rs` around lines 25 - 35, The
patch_bytes function currently assumes each ImmutableRef length equals the
32-byte value but doesn't enforce it, causing copy_from_slice to panic with
unclear errors; add an explicit check inside the loop that r.length ==
value.len() (or assert_eq!(r.length, 32, "...")) before the bounds assert/copy
so failures are deterministic and self-explanatory, referencing the ImmutableRef
fields (r.start, r.length) and value length in the assertion message.
Comment on lines
+20
to
+23
| if let Some(ref p2_config) = config.contracts.permit2 { | ||
| let contract = contracts::permit2::build(p2_config, config.chain.chain_id); | ||
| insert_contract(&mut alloc, &contract); | ||
| } |
There was a problem hiding this comment.
Prevent silent alloc overwrite when two configured contracts share one address.
At Line 20-23, adding Permit2 means two configured contracts can now collide. insert_contract uses Map::insert, so one entry is silently replaced instead of failing.
🛠️ Proposed fix (fail on duplicate deploy-config addresses)
-pub(crate) fn build_alloc(config: &DeployConfig) -> Value {
+pub(crate) fn build_alloc(config: &DeployConfig) -> eyre::Result<Value> {
let mut alloc = Map::new();
if let Some(ref ap_config) = config.contracts.admin_proxy {
let contract = contracts::admin_proxy::build(ap_config);
- insert_contract(&mut alloc, &contract);
+ insert_contract(&mut alloc, &contract)?;
}
if let Some(ref p2_config) = config.contracts.permit2 {
let contract = contracts::permit2::build(p2_config, config.chain.chain_id);
- insert_contract(&mut alloc, &contract);
+ insert_contract(&mut alloc, &contract)?;
}
- Value::Object(alloc)
+ Ok(Value::Object(alloc))
}
@@
- let alloc = build_alloc(config);
+ let alloc = build_alloc(config)?;
@@
-fn insert_contract(alloc: &mut Map<String, Value>, contract: &GenesisContract) {
+fn insert_contract(alloc: &mut Map<String, Value>, contract: &GenesisContract) -> eyre::Result<()> {
let addr_key = normalize_addr(&format!("{}", contract.address));
@@
- alloc.insert(addr_key, Value::Object(entry));
+ if alloc.insert(addr_key.clone(), Value::Object(entry)).is_some() {
+ eyre::bail!("duplicate deploy address in config: {addr_key}");
+ }
+ Ok(())
}🤖 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 20 - 23, The insertion of the
Permit2 contract can silently overwrite an existing entry because
insert_contract uses Map::insert; update insert_contract (or the call site) to
detect duplicate deploy-config addresses and fail instead of replacing: before
calling insert_contract (or within insert_contract), check whether the
allocation map already contains the contract's address (use the same unique
identifier produced by contracts::permit2::build, e.g., the contract.address or
contract.id), and if it does return an error or panic with a clear message
indicating a duplicate configured contract address (include the conflicting
address and the two contract identifiers); alternatively use Map::entry and on
Occupied(_) return Err to prevent the silent overwrite.
| @@ -0,0 +1 @@ | |||
| Subproject commit cc56ad0f3439c502c246fc5cfcc3db92bb8b7219 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the submodule commit exists in the official Uniswap/permit2 repository
# and check if it corresponds to a tagged release
COMMIT="cc56ad0f3439c502c246fc5cfcc3db92bb8b7219"
# Check if commit exists in Uniswap/permit2 via GitHub API
echo "=== Checking commit existence in Uniswap/permit2 ==="
gh api repos/Uniswap/permit2/commits/$COMMIT --jq '.sha, .commit.message' 2>/dev/null || echo "Commit not found or API error"
# List tags to see if this commit corresponds to a release
echo ""
echo "=== Checking tags in Uniswap/permit2 ==="
gh api repos/Uniswap/permit2/tags --jq '.[].name' 2>/dev/null | head -10
# Check the commit date
echo ""
echo "=== Commit date ==="
gh api repos/Uniswap/permit2/commits/$COMMIT --jq '.commit.committer.date' 2>/dev/null || echo "Could not retrieve date"Repository: evstack/ev-reth
Length of output: 564
🏁 Script executed:
#!/bin/bash
# Read the submodule reference file
echo "=== Content of contracts/lib/permit2 ==="
cat -n contracts/lib/permit2
# Check if .gitmodules exists and contains permit2 configuration
echo ""
echo "=== Checking .gitmodules for permit2 ==="
if [ -f .gitmodules ]; then
grep -A 3 "permit2" .gitmodules 2>/dev/null || echo ".gitmodules exists but no permit2 entry found"
else
echo ".gitmodules not found"
fi
# Read the referenced Rust file to verify documentation
echo ""
echo "=== Content of bin/ev-deployer/src/contracts/permit2.rs (lines 30-60) ==="
head -60 bin/ev-deployer/src/contracts/permit2.rs | tail -31Repository: evstack/ev-reth
Length of output: 19741
🏁 Script executed:
#!/bin/bash
# Get the actual submodule commit reference
echo "=== Checking git submodule reference ==="
cd contracts/lib/permit2 2>/dev/null && git rev-parse HEAD 2>/dev/null || echo "Not a git repo or not accessible"
# Alternative: check from parent repo
cd - > /dev/null 2>&1
echo ""
echo "=== Checking submodule commit via git config/status ==="
git config -f .gitmodules submodule.contracts/lib/permit2.url
git ls-files --stage contracts/lib/permit2
# Check git log if available
echo ""
echo "=== Attempting to verify commit documentation match ==="
echo "Documented commit abbreviation in Rust code: cc56ad0"
echo "Full hash claim in review: cc56ad0f3439c502c246fc5cfcc3db92bb8b7219"
echo "Matches first 7 chars: $([ 'cc56ad0' = 'cc56ad0f3439c502c246fc5cfcc3db92bb8b7219' ] && echo 'YES' || echo 'First 7 chars match: YES')"Repository: evstack/ev-reth
Length of output: 530
Submodule reference contains a critical inconsistency.
The git submodule index shows contracts/lib/permit2 is pinned to commit cc56ad0f3439c502c246fc5cfcc3db92bb8b7219, which matches the abbreviated reference cc56ad0 documented in bin/ev-deployer/src/contracts/permit2.rs (line 34). However, the actual checked-out state of the submodule is at a different commit (c32b6339dc92100bf8b6dc19976c7bad03dbb155), indicating the submodule has not been properly initialized to the pinned reference.
Additionally, the review comment incorrectly describes the submodule file structure—git submodules do not store commit hashes as plaintext in a file but rather in the git index.
Action required: Ensure the submodule is checked out to the pinned commit cc56ad0f3439c502c246fc5cfcc3db92bb8b7219 before merging this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/lib/permit2` at line 1, The contracts/lib/permit2 submodule is not
checked out to the pinned commit cc56ad0f3439c502c246fc5cfcc3db92bb8b7219
referenced in bin/ev-deployer/src/contracts/permit2.rs; update the submodule to
the correct commit by running git submodule update --init --recursive and then
git -C contracts/lib/permit2 fetch && git -C contracts/lib/permit2 checkout
cc56ad0f3439c502c246fc5cfcc3db92bb8b7219, verify the commit matches the
abbreviated cc56ad0 in bin/ev-deployer/src/contracts/permit2.rs, and commit the
updated submodule reference so the repo index points to that exact commit before
merging.
Remove IHypNativeMinter interface and all Hyperlane-specific fields (destinationDomain, recipientAddress) from FeeVault. The contract now uses direct ETH transfers to a configurable bridgeRecipient instead of Hyperlane's transferRemote(). sendToCelestia() renamed to distribute(). Also removes the hyperlane-monorepo git submodule and updates all deployment scripts, tests, and documentation.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/FeeVault.sol (1)
96-100:⚠️ Potential issue | 🟡 MinorSetting
bridgeShareBpsto 0 can blockdistribute()ifminimumAmount > 0.
setBridgeShare(0)is allowed and would makebridgeAmount = 0on every call. IfminimumAmount > 0, line 56 will always revert with "minimum amount not met". While the owner can fix this by adjusting either setting, it could cause temporary operational issues.💡 Optional: Add validation or documentation
function setBridgeShare(uint256 _bridgeShareBps) external onlyOwner { require(_bridgeShareBps <= 10000, "FeeVault: invalid bps"); + // Note: Setting to 0 means all funds go to otherRecipient. + // If minimumAmount > 0, distribute() will revert until minimumAmount is set to 0. bridgeShareBps = _bridgeShareBps; emit BridgeShareUpdated(_bridgeShareBps); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/FeeVault.sol` around lines 96 - 100, setBridgeShare currently allows setting bridgeShareBps to 0 which makes bridgeAmount always 0 and can cause distribute() to revert on the "minimum amount not met" check; modify the setBridgeShare(uint256 _bridgeShareBps) function to reject zero values (e.g., require(_bridgeShareBps > 0, "FeeVault: bridge share cannot be zero")) or alternatively update distribute() to skip the minimumAmount check when computed bridgeAmount == 0; reference setBridgeShare, bridgeShareBps, distribute(), bridgeAmount, and minimumAmount when making the change and keep the BridgeShareUpdated event emission.
🧹 Nitpick comments (2)
contracts/src/FeeVault.sol (1)
61-69: Consider documenting the push-transfer DoS risk.The
distribute()function uses push transfers tootherRecipientandbridgeRecipient. If either recipient is a contract that reverts on receive (intentionally or due to gas limits), the vault cannot be drained. Since recipients are owner-controlled, this is a moderate risk, but worth documenting.A pull-based withdrawal pattern would mitigate this, though it adds complexity. If push-transfer is intentional (fail-fast behavior), consider adding a comment explaining the design decision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/FeeVault.sol` around lines 61 - 69, The distribute() function in FeeVault uses push transfers to otherRecipient and bridgeRecipient which can cause a DoS if a recipient contract reverts or consumes excessive gas; either switch to a pull-based pattern (add withdrawOther()/withdrawBridge() functions that let recipients claim their balances and store owed amounts in state) or keep the push model but add a clear comment in distribute() documenting the DoS risk and the intentional fail-fast design; reference the distribute(), otherRecipient, bridgeRecipient, and FeeVault symbols when applying the change so reviewers can locate the adjustment quickly.contracts/test/FeeVault.t.sol (1)
77-96: Consider adding a test forotherRecipientzero address revert.The
test_Distribute_Split5050test assumesotherRecipientis set (fromsetUp). There's no explicit test covering the revert case wherebridgeShareBps < 10000butotherRecipientis zero (which would hit line 62 inFeeVault.sol: "other recipient not set").💡 Suggested test case
function test_Distribute_OtherRecipientNotSet() public { FeeVault freshVault = new FeeVault(owner, minAmount, fee, 5000, address(0)); // 50% split, no otherRecipient freshVault.setBridgeRecipient(bridgeRecipient); (bool success,) = address(freshVault).call{value: 2 ether}(""); require(success); vm.prank(user); vm.deal(user, fee); vm.expectRevert("FeeVault: other recipient not set"); freshVault.distribute{value: fee}(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/test/FeeVault.t.sol` around lines 77 - 96, Add a unit test that verifies FeeVault.distribute reverts when bridgeShareBps < 10000 but otherRecipient is unset: instantiate a fresh FeeVault (use the constructor to set bridgeShareBps to 5000 and otherRecipient to address(0)), call setBridgeRecipient(bridgeRecipient), fund the vault, then vm.prank(user) with vm.deal(user, fee) and assert vm.expectRevert("FeeVault: other recipient not set") before calling freshVault.distribute{value: fee}(); this will cover the otherRecipient zero-address revert path referenced by otherRecipient and bridgeShareBps in FeeVault.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/src/FeeVault.sol`:
- Around line 96-100: setBridgeShare currently allows setting bridgeShareBps to
0 which makes bridgeAmount always 0 and can cause distribute() to revert on the
"minimum amount not met" check; modify the setBridgeShare(uint256
_bridgeShareBps) function to reject zero values (e.g., require(_bridgeShareBps >
0, "FeeVault: bridge share cannot be zero")) or alternatively update
distribute() to skip the minimumAmount check when computed bridgeAmount == 0;
reference setBridgeShare, bridgeShareBps, distribute(), bridgeAmount, and
minimumAmount when making the change and keep the BridgeShareUpdated event
emission.
---
Nitpick comments:
In `@contracts/src/FeeVault.sol`:
- Around line 61-69: The distribute() function in FeeVault uses push transfers
to otherRecipient and bridgeRecipient which can cause a DoS if a recipient
contract reverts or consumes excessive gas; either switch to a pull-based
pattern (add withdrawOther()/withdrawBridge() functions that let recipients
claim their balances and store owed amounts in state) or keep the push model but
add a clear comment in distribute() documenting the DoS risk and the intentional
fail-fast design; reference the distribute(), otherRecipient, bridgeRecipient,
and FeeVault symbols when applying the change so reviewers can locate the
adjustment quickly.
In `@contracts/test/FeeVault.t.sol`:
- Around line 77-96: Add a unit test that verifies FeeVault.distribute reverts
when bridgeShareBps < 10000 but otherRecipient is unset: instantiate a fresh
FeeVault (use the constructor to set bridgeShareBps to 5000 and otherRecipient
to address(0)), call setBridgeRecipient(bridgeRecipient), fund the vault, then
vm.prank(user) with vm.deal(user, fee) and assert vm.expectRevert("FeeVault:
other recipient not set") before calling freshVault.distribute{value: fee}();
this will cover the otherRecipient zero-address revert path referenced by
otherRecipient and bridgeShareBps in FeeVault.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0de4fb0f-e0bf-459e-956d-154b3189d4e4
⛔ Files ignored due to path filters (1)
contracts/foundry.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.claude/skills/contracts.md.gitmodulescontracts/README.mdcontracts/script/DeployFeeVault.s.solcontracts/script/GenerateFeeVaultAlloc.s.solcontracts/src/FeeVault.solcontracts/test/AdminProxy.t.solcontracts/test/FeeVault.t.soldocs/contracts/fee_vault.mddocs/guide/fee-systems.md
💤 Files with no reviewable changes (1)
- contracts/test/AdminProxy.t.sol
✅ Files skipped from review due to trivial changes (2)
- .gitmodules
- .claude/skills/contracts.md
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)
contracts/README.md (1)
45-45:⚠️ Potential issue | 🟡 MinorResolve contradiction in configuration guidance.
Line 45 says all configuration is set via constructor arguments, but Line 58 requires post-deployment
setBridgeRecipient(). Please soften Line 45 to reflect that not all operational config is constructor-based.Suggested doc tweak
-All configuration is set via constructor arguments at deploy time: +Most configuration is set via constructor arguments at deploy time:Also applies to: 58-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/README.md` at line 45, The README currently states "All configuration is set via constructor arguments" but later requires calling setBridgeRecipient(), so update the wording to reflect that most configuration is constructor-based while some operational parameters may need post-deployment calls; specifically mention the constructor-based config and call out setBridgeRecipient() as an example of a required post-deploy operation, and adjust the sentence around the constructor description (reference: setBridgeRecipient()) to avoid the absolute "All" phrasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contracts/fee_vault.md`:
- Line 40: The docs currently state that a `callFee` is "required" but the
contract check `msg.value >= callFee` permits `callFee = 0`; update the wording
around `callFee` and the distribution trigger (references: callFee, msg.value >=
callFee, distribution function) to state that `callFee` is the minimum required
payment (it can be zero), so callers must send at least `callFee` and when
`callFee == 0` no payment is needed; revise the lines that say "required" to
something like "minimum call fee (may be zero)" and add a short clarifying
sentence that `msg.value >= callFee` enforces this behaviour.
---
Outside diff comments:
In `@contracts/README.md`:
- Line 45: The README currently states "All configuration is set via constructor
arguments" but later requires calling setBridgeRecipient(), so update the
wording to reflect that most configuration is constructor-based while some
operational parameters may need post-deployment calls; specifically mention the
constructor-based config and call out setBridgeRecipient() as an example of a
required post-deploy operation, and adjust the sentence around the constructor
description (reference: setBridgeRecipient()) to avoid the absolute "All"
phrasing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0290d52-ca80-4745-a35c-df1e43e35a95
📒 Files selected for processing (3)
contracts/README.mddocs/contracts/fee_vault.mddocs/guide/fee-systems.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/guide/fee-systems.md
| - **Caller Incentive/Fee**: A `callFee` is required to trigger the bridge function. | ||
| - **Automatic Splitting**: Funds are split automatically upon calling `distribute`. No manual withdrawal is required for the secondary recipient. | ||
| - **Minimum Threshold**: A `minimumAmount` ensures that distribution only occurs when it is economically viable. | ||
| - **Caller Incentive/Fee**: A `callFee` is required to trigger the distribution function. |
There was a problem hiding this comment.
Clarify callFee semantics for zero-fee configs.
Line 40 says callFee is “required,” but Line 52 checks msg.value >= callFee. With callFee = 0, no payment is required. Please reword this to avoid operator confusion.
Suggested doc tweak
-- **Caller Incentive/Fee**: A `callFee` is required to trigger the distribution function.
+- **Caller Incentive/Fee**: A caller may need to pay `callFee` to trigger distribution (no payment is needed when `callFee` is `0`).Also applies to: 50-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contracts/fee_vault.md` at line 40, The docs currently state that a
`callFee` is "required" but the contract check `msg.value >= callFee` permits
`callFee = 0`; update the wording around `callFee` and the distribution trigger
(references: callFee, msg.value >= callFee, distribution function) to state that
`callFee` is the minimum required payment (it can be zero), so callers must send
at least `callFee` and when `callFee == 0` no payment is needed; revise the
lines that say "required" to something like "minimum call fee (may be zero)" and
add a short clarifying sentence that `msg.value >= callFee` enforces this
behaviour.
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
2 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.
Summary
_CACHED_CHAIN_ID,_CACHED_DOMAIN_SEPARATOR) at genesis timecontracts/lib/permit2git submodule for source verificationDOMAIN_SEPARATOR()correctnessTest plan
cargo test -p ev-deployer— 32 passed)DOMAIN_SEPARATOR()via RPCSummary by CodeRabbit
New Features
Tests
Documentation