Virtual permit2 fetch for chains with different create2 paths.#187
Open
reednaa wants to merge 1 commit into
Open
Virtual permit2 fetch for chains with different create2 paths.#187reednaa wants to merge 1 commit into
reednaa wants to merge 1 commit into
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Permit2 override function abstraction src/input/escrow/InputSettlerEscrow.sol |
Remove hardcoded PERMIT2 constant and introduce _PERMIT2() as an internal pure virtual function. Update _openForWithPermit2 to call _PERMIT2().permitWitnessTransferFrom(...) instead of the constant reference. |
Permit2 override test suite test/input/escrow/InputSettlerEscrowPermit2Override.t.sol |
Add InputSettlerEscrowExposed and InputSettlerEscrowPermit2Override helper contracts that override _PERMIT2() to return an alternate address. Implement test_open_for_permit2_routes_through_overridden_address to verify escrowing occurs through the overridden Permit2 instance using vm.etch code injection. |
Snapshot parameter updates snapshots/inputSettler.json, snapshots/outputSettler.json |
Update preset parameter values for order IDs, escrow amounts, timestamp bounds, and coin-fill variants across all snapshot keys. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
- frangio
- pepebndc
Poem
🐰 Constants hardcoded, now they bend,
A virtual function becomes a friend,
Chain-specific Permits now can sway,
Tested through and through today!
Hop skip jump — override your way! 🎉
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title directly and clearly summarizes the main change: making Permit2 address overridable for chains with different CREATE2 paths. |
| Description check | ✅ Passed | The description comprehensively covers the purpose, implementation details, test coverage, and related issues, following the template structure with all critical sections addressed. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
permit2-virtual
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/input/escrow/InputSettlerEscrow.sol (1)
277-284:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix forge formatting failure on the chained Permit2 call.
forge fmt --checkis failing on Line 277-Line 284, which blocks CI. Reformat this call to match the formatter’s expected chained style.Proposed formatting-only fix
- _PERMIT2().permitWitnessTransferFrom( + _PERMIT2() + .permitWitnessTransferFrom( permitBatch, transferDetails, signer, Permit2WitnessType.Permit2WitnessHash(order), Permit2WitnessType.PERMIT2_PERMIT2_TYPESTRING, signature );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/input/escrow/InputSettlerEscrow.sol` around lines 277 - 284, The chained call to _PERMIT2().permitWitnessTransferFrom is misformatted and failing forge fmt; reformat the expression so the receiver and method call are split per the formatter's chained-call style: put _PERMIT2() then a dot and the method name on the next line, place each argument (permitBatch, transferDetails, signer, Permit2WitnessType.Permit2WitnessHash(order), Permit2WitnessType.PERMIT2_PERMIT2_TYPESTRING, signature) on its own line with trailing commas, and close with the matching ); ensuring indentation and no extra trailing spaces so forge fmt passes.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/input/escrow/InputSettlerEscrow.sol`:
- Around line 277-284: The chained call to _PERMIT2().permitWitnessTransferFrom
is misformatted and failing forge fmt; reformat the expression so the receiver
and method call are split per the formatter's chained-call style: put _PERMIT2()
then a dot and the method name on the next line, place each argument
(permitBatch, transferDetails, signer,
Permit2WitnessType.Permit2WitnessHash(order),
Permit2WitnessType.PERMIT2_PERMIT2_TYPESTRING, signature) on its own line with
trailing commas, and close with the matching ); ensuring indentation and no
extra trailing spaces so forge fmt passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: abfa99a0-0dcd-4783-bf36-4bb6300a3be9
📒 Files selected for processing (4)
snapshots/inputSettler.jsonsnapshots/outputSettler.jsonsrc/input/escrow/InputSettlerEscrow.soltest/input/escrow/InputSettlerEscrowPermit2Override.t.sol
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.
Description
Makes the Permit2 contract address overridable in InputSettlerEscrow so the settler can be deployed on chains where Permit2 does not live at the canonical address.
Previously the Permit2 address was a hardcoded constant (
0x000000000022D473030F116dDEE9F6B43aC78BA3). That address is the product of an EVM-specific deterministic CREATE2 deployment and cannot be reproduced on chains with different address derivation (e.g. Tron). This PR replaces the constant with aninternal pure virtualhook,_PERMIT2(), that defaults to the canonical address, and points the single call site in_openForWithPermit2at it.Subclasses can now override
_PERMIT2()to return a chain-specific Permit2 address — mirroring the existing_domainName()/_domainVersion()override pattern. There is no constructor change, so existing deterministic CREATE2 deployment addresses and salts on EVM chains are unaffected.Changes:
src/input/escrow/InputSettlerEscrow.sol—constant PERMIT2→_PERMIT2()virtual getter; call site updated.test/input/escrow/InputSettlerEscrowPermit2Override.t.sol(new) — proves a subclass override is honored and thatopenForcollects inputs end-to-end through the overridden Permit2 (fuzzed).snapshots/*.json— regenerated gas snapshots.Related Issues
Additional Notes
purebecause the default returns a literal; overrides returning a compile-time constant remain pure-compatible.Summary by CodeRabbit
Release Notes
Tests
Chores