Add delegation contract verification for EIP-7702 accounts #57
Conversation
WalkthroughThe change introduces delegation-aware minimal-account checks across core, executors, server, and tests. A new optional delegation contract parameter is threaded through DelegatedAccount and EoaAuthorizationCache, updating cache keys and call sites. Constants cleanup removes unused imports. Integration tests now supply delegation contract context. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller (Worker/Server/Tx)
participant Cache as EoaAuthorizationCache
participant DA as DelegatedAccount
note over Caller,DA: Delegation-aware minimal-account check
Caller->>Cache: is_minimal_account(&DelegatedAccount, delegation_contract: Option<Address>)
Cache->>Cache: Build key {eoa, chain, delegation_contract or ZERO}
alt Cache hit
Cache-->>Caller: cached is_minimal
else Cache miss
Cache->>DA: is_minimal_account(delegation_contract)
DA-->>Cache: is_minimal (delegation-aware)
Cache->>Cache: Store result under expanded key
Cache-->>Caller: is_minimal
end
sequenceDiagram
autonumber
actor Tx as Transaction Flow
participant DA as DelegatedAccount
note over Tx,DA: Authorization decision based on delegation-aware minimal check
Tx->>DA: is_minimal_account(Some(delegation_contract))
alt Minimal with respect to delegation
Tx-->>Tx: Early return (no auth added)
else Not minimal
Tx-->>Tx: Proceed to add authorization
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![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)
eip7702-core/tests/integration_tests.rs (1)
660-664: Fix panic before fetching delegation contract
setup.delegation_contractis stillNoneimmediately afterTestSetup::new(). Callingexpecthere will panic and fail the test before any delegation setup occurs. Fetch the bytecode (which populates the contract) before unwrapping the option.let mut setup = TestSetup::new().await?; -let delegation_contract = setup.delegation_contract.expect("Delegation contract should be set"); - -// Step 1: Fetch and set bytecode from Base Sepolia -setup.fetch_and_set_bytecode().await?; +// Step 1: Fetch and set bytecode from Base Sepolia +setup.fetch_and_set_bytecode().await?; + +let delegation_contract = setup.delegation_contract.expect("Delegation contract should be set");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
eip7702-core/src/constants.rs(0 hunks)eip7702-core/src/delegated_account.rs(2 hunks)eip7702-core/src/transaction.rs(1 hunks)eip7702-core/tests/integration_tests.rs(4 hunks)executors/src/eoa/authorization_cache.rs(2 hunks)executors/src/eoa/worker/mod.rs(1 hunks)server/src/execution_router/mod.rs(1 hunks)
💤 Files with no reviewable changes (1)
- eip7702-core/src/constants.rs
🧰 Additional context used
🧬 Code graph analysis (2)
eip7702-core/src/delegated_account.rs (1)
executors/src/eoa/authorization_cache.rs (1)
is_minimal_account(25-41)
eip7702-core/tests/integration_tests.rs (2)
eip7702-core/src/delegated_account.rs (1)
is_minimal_account(31-85)executors/src/eoa/authorization_cache.rs (1)
is_minimal_account(25-41)
🔇 Additional comments (2)
eip7702-core/src/delegated_account.rs (2)
76-82: Debug logging looks comprehensive.The updated trace includes all relevant context: EOA address, target address extracted from bytecode, the delegation contract being checked against, and the final delegation status.
31-31: Confirm and clarifyNonebehavior fordelegation_contract
All call sites now pass the new parameter, but several (inexecution_routerandworker) useNone, causingis_minimal_account(None)to unconditionally returntrueand the cache to key onAddress::ZERO. Verify this fallback matches intended semantics or require callers to supply an explicitdelegation_contract.
| let is_delegated = match delegation_contract { | ||
| Some(delegation_contract) => { | ||
| target_address == delegation_contract | ||
| } | ||
| None => { | ||
| true | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Reconsider the None case returning true by default.
When delegation_contract is None, the method returns true (assumes delegation exists), which seems semantically incorrect. This means "I don't know which contract to check against, so I'll assume it's delegated."
Additionally, the authorization cache (shown in relevant snippets) uses Address::ZERO as a fallback when delegation_contract is None, which could lead to cache key collisions—different delegation contracts would share the same cache entry.
Consider either:
- Returning an error when
delegation_contractisNoneand delegation verification is actually needed - Making
delegation_contracta required parameter instead ofOption<Address> - Documenting clearly why
Nonemeans "assume delegated"
Run the following script to understand how None is used across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find all call sites passing None to is_minimal_account
# Search for calls with None argument
rg -n -B 5 -A 3 '\.is_minimal_account\(\s*None\s*\)' --type rust
# Also check cache usage with Address::ZERO
rg -n -B 3 -A 3 'Address::ZERO' --type rust -g '*authorization_cache*'Length of output: 672
🏁 Script executed:
#!/bin/bash
# 1. Find definition of is_minimal_account
rg -n 'fn is_minimal_account' -C3 --type rust
# 2. Show lines 60-100 in delegated_account.rs to inspect None handling
sed -n '60,100p' eip7702-core/src/delegated_account.rs
# 3. Show lines 1-100 in authorization_cache.rs for call-site context
sed -n '1,100p' executors/src/eoa/authorization_cache.rsLength of output: 3615
Reconsider default behavior when delegation_contract is None.
- In
DelegatedAccount::is_minimal_account(eip7702-core/src/delegated_account.rs:67–74), returningtrueonNoneunconditionally bypasses the delegation check—any call without a contract will always succeed. Instead, either require a validdelegation_contractor return an error when it’s missing. - In
EoaAuthorizationCache(executors/src/eoa/authorization_cache.rs:35–38), usingunwrap_or(Address::ZERO)for the cache key collapses allNonecases into the same entry, risking collisions. Consider encoding theOptiondirectly in the key or using a distinct sentinel for “no contract.”
🤖 Prompt for AI Agents
In eip7702-core/src/delegated_account.rs around lines 67–74 and
executors/src/eoa/authorization_cache.rs around lines 35–38, the current logic
treats a missing delegation_contract as a successful delegation and collapses
all None cases into Address::ZERO for cache keys; update
DelegatedAccount::is_minimal_account to not unconditionally return true when
delegation_contract is None — either require Some and return false (or propagate
an Err) when it’s missing and adjust the function signature/call sites
accordingly so callers handle the missing contract explicitly; in
EoaAuthorizationCache stop using unwrap_or(Address::ZERO) — change the cache key
to encode the Option (e.g., use Option<Address> or a (bool, Address) tuple) or
choose a distinct sentinel value and document it to avoid collisions so each
None vs Some(contract) is cached separately.
d4mr
deleted the
Add_delegation_contract_verification_for_EIP-7702_accounts
branch
Summary by CodeRabbit