fix(ante): address association parity across EVM ante paths#3793
fix(ante): address association parity across EVM ante paths#3793codchen wants to merge 1 commit into
Conversation
Follow-up to #3716. The authorization-authority address association added there landed in the EVMPreprocessDecorator ante path but not in the legacyabci ante path (EvmDeliverTxAnte / EvmCheckTxAnte), which associated only the tx sender. Add ante.AssociateAuthorizationAuthorities and invoke it from the deliver-tx path (mirrored in check-tx for mempool consistency) so both paths associate the same addresses before EVM execution. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3793 +/- ##
==========================================
- Coverage 60.20% 59.24% -0.97%
==========================================
Files 2328 2236 -92
Lines 194502 184016 -10486
==========================================
- Hits 117104 109024 -8080
+ Misses 66849 65296 -1553
+ Partials 10549 9696 -853
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview Adds Adds Reviewed by Cursor Bugbot for commit e932f96. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e932f96. Configure here.
| write() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicated authority association logic
Low Severity
AssociateAuthorizationAuthorities reimplements the same AuthorityToPreAssociate + cache-context association loop already in EVMPreprocessDecorator.associateAuthorizationAuthorities. app/ante already reuses other x/evm/ante helpers, and this path is state-machine-critical, so a second copy raises divergence risk if one side is fixed later.
Reviewed by Cursor Bugbot for commit e932f96. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds ante.AssociateAuthorizationAuthorities to the legacyabci ante paths (EvmDeliverTxAnte/EvmCheckTxAnte), bringing EIP-7702 authorization-authority pre-association to parity with the existing EVMPreprocessDecorator. The change is a faithful, well-documented mirror of the existing logic, reuses the same validated helper, and is covered by a new test; no blocking issues found.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. Codex reported no material issues.
- Consider a follow-up refactor to share a single implementation of the authority pre-association loop between
app/anteandx/evm/ante(the two now-duplicated bodies must stay in lockstep since the PR's own note stresses the paths must behave identically); the sharedhelpers.AuthorityToPreAssociatealready covers the validation, but the cache-context/write loop is duplicated. - The added test exercises
AssociateAuthorizationAuthoritiesdirectly but not via the fullEvmDeliverTxAnte/EvmCheckTxAnteentry points; an end-to-end assertion that CheckTx's association is not committed (matching the mempool-only intent) would strengthen coverage.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found in my review, but given this is app-hash-breaking, consensus-critical ante-handler code that must ship in lockstep with #3716, I'd like a human to confirm the coordinated-release plan before merge.
Extended reasoning...
Overview
The PR adds ante.AssociateAuthorizationAuthorities and calls it from both EvmDeliverTxAnte and EvmCheckTxAnte in app/ante/, so the legacyabci ante path pre-associates EIP-7702 SetCode authorization authorities the same way x/evm/ante.EVMPreprocessDecorator already does. I compared the new function line-by-line against EVMPreprocessDecorator.associateAuthorizationAuthorities (x/evm/ante/preprocess.go) and it is a faithful mirror: same use of helpers.AuthorityToPreAssociate for the chain-id/nonce/code checks, same per-authority cache-context isolation so a single failing authority can't affect others or reject the tx, and the same call ordering relative to sender association. helpers.AuthorityToPreAssociate itself already guards against replay of a foreign-chain authorization and against re-mapping an already-associated authority.
Security risks
This is squarely address-association / consensus-state code: a bug here can misassign an EVM<->Sei address mapping, which the PR's own commit message notes can eventually orphan staking/distribution state and halt the chain. I didn't find a defect in this diff, but the blast radius of a mistake in this area is a chain halt, not a contained bug.
Level of scrutiny
The PR is labeled app-hash-breaking and the description explicitly states it must be released in the same coordinated upgrade as #3716 because the two ante paths need to behave identically. That coordination requirement is an operational/release-process concern that isn't something I can verify from the diff alone (e.g., whether #3716 is scheduled for the same upgrade, whether both PRs land before the same tag is cut). Combined with the app-hash-breaking label, this warrants a human sign-off even though the code change itself looks correct and well-tested.
Other factors
Test coverage is solid: TestAssociateAuthorizationAuthorities exercises both the matching-chain-association and foreign-chain-skip cases, mirroring the existing x/evm/ante tests for the same behavior. The bug-hunting pass found nothing, and my own read didn't surface a logic issue in the new code or its integration points.
1 participant
Summary
Follow-up to #3716, aligning EVM address-association handling between the two ante paths:
EVMPreprocessDecoratorpath already pre-associates EIP-7702SetCodeauthorization authorities (not just the tx sender) before execution.EvmDeliverTxAnte/EvmCheckTxAnte) previously associated only the sender.This adds
ante.AssociateAuthorizationAuthorities, invoked fromEvmDeliverTxAnte(after sender association, before fee-charging / execution) and mirrored inEvmCheckTxAnte. It reuses the existinghelpers.AuthorityToPreAssociatevalidation, so both paths associate an identical set of addresses.Notes
EvmCheckTxAnteruns on a throwaway cache and does not apply authorizations — its call is purely for mempool-state consistency with the deliver path.Testing
GOWORK=off go test ./app/ -run 'TestAssociateAuthorizationAuthorities|TestSetCodeTxRequiresAuthorityAssociation'— passGOWORK=off go test -race ./app/ante/...— passTestAssociateAuthorizationAuthorities(matching-chain authority associated; foreign-chain authority skipped).🤖 Generated with Claude Code