added test cases for ERC1271 delegated wallet signature verification#1627
added test cases for ERC1271 delegated wallet signature verification#1627
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR adds comprehensive support for multiple permit standards (EIP-2612, EIP-3009, Permit2) to the Permit2Proxy contract, introduces new token permit interfaces, adds fork-based ERC-1271 tests on Base and Arbitrum chains, updates script helpers, and provides extensive documentation for integration workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Test Coverage ReportLine Coverage: 86.77% (2979 / 3433 lines) |
![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
🤖 Fix all issues with AI agents
In `@test/solidity/CoinbaseERC1271Fork.t.sol`:
- Around line 394-407: The test currently asserts exact balance equality after
calling IPermit2Proxy(PERMIT2_PROXY_ARBITRUM).callDiamondWithPermit2 by checking
IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM) == amount which is
fragile; change it to record uint256 before =
IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM) prior to
vm.prank(wallet) and the call, then assert that
IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM) == before + amount (or
assertEq(delta, amount)) so you verify the balance delta rather than exact
absolute balance.
🧹 Nitpick comments (2)
test/solidity/CoinbaseERC1271Fork.t.sol (2)
83-88: Minor:UNISWAP_ARBITRUMandUSDC_ARBITRUMare not labeled.Other constants like
USDT_ARBITRUM,PERMIT2, etc. are labeled for trace readability, butUNISWAP_ARBITRUMandUSDC_ARBITRUM(used in the swap test) are missing labels.Suggested addition
vm.label(PERMIT2, "PERMIT2"); vm.label(PERMIT2_PROXY_ARBITRUM, "PERMIT2_PROXY_ARBITRUM"); + vm.label(UNISWAP_ARBITRUM, "UNISWAP_ARBITRUM"); + vm.label(USDC_ARBITRUM, "USDC_ARBITRUM");
251-264: Missing result-length guard (unlike the first test).
testFork_CoinbaseERC1271_IsValidSignature_ReturnsMagicValue(line 224) checksresult.length < 4before decoding, but this test accessesbytes4(result)directly. For consistency and clearer diagnostics on failure, consider adding the same guard.Suggested fix
if (!success) revert IsValidSignatureCallFailed(); + if (result.length < 4) revert InvalidResultLength(result.length); assertEq(
| vm.prank(wallet); | ||
| IPermit2Proxy(PERMIT2_PROXY_ARBITRUM).callDiamondWithPermit2( | ||
| diamondCalldata, | ||
| permit, | ||
| signature | ||
| ); | ||
|
|
||
| // Proxy should have received the USDT (then approved diamond); assert proxy balance or diamond call succeeded. | ||
| assertEq( | ||
| IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM), | ||
| amount, | ||
| "Permit2Proxy should hold USDT after permit transfer" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Exact-balance assertion is fragile — should assert balance delta instead.
The proxy may already hold some USDT from real Arbitrum transactions at the forked block. assertEq(balance, amount) will fail if there is any pre-existing balance. Record the balance before the call and assert the increase.
Suggested fix
+ uint256 proxyUsdtBefore = IERC20(USDT_ARBITRUM).balanceOf(
+ PERMIT2_PROXY_ARBITRUM
+ );
+
vm.prank(wallet);
IPermit2Proxy(PERMIT2_PROXY_ARBITRUM).callDiamondWithPermit2(
diamondCalldata,
permit,
signature
);
- // Proxy should have received the USDT (then approved diamond); assert proxy balance or diamond call succeeded.
- assertEq(
- IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM),
+ // Proxy should have received the USDT (then approved diamond).
+ assertEq(
+ IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM) -
+ proxyUsdtBefore,
amount,
"Permit2Proxy should hold USDT after permit transfer"
);🤖 Prompt for AI Agents
In `@test/solidity/CoinbaseERC1271Fork.t.sol` around lines 394 - 407, The test
currently asserts exact balance equality after calling
IPermit2Proxy(PERMIT2_PROXY_ARBITRUM).callDiamondWithPermit2 by checking
IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM) == amount which is
fragile; change it to record uint256 before =
IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM) prior to
vm.prank(wallet) and the call, then assert that
IERC20(USDT_ARBITRUM).balanceOf(PERMIT2_PROXY_ARBITRUM) == before + amount (or
assertEq(delta, amount)) so you verify the balance delta rather than exact
absolute balance.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@opcodes-selectors.md`:
- Line 46: The entry listing "`0x55299b49` | `UpgradeFailed()` (event)" is
incorrect because `UpgradeFailed()` is a custom error selector (4-byte) not an
event (32-byte topic); update the document to classify `0x55299b49` as a custom
error selector for `UpgradeFailed()` (or remove the "(event)" label), and ensure
any surrounding explanation in opcodes-selectors.md clarifies that 4-byte
selectors map to functions/errors while events use 32-byte topic hashes.
- Around line 79-82: The description for the excluded selector 0x4e487b71 is
incorrect: replace the primary label "Error(string)" with "Panic(uint256)" and
optionally add the correct selector for Error(string) (0x08c379a0) for clarity;
update the explanatory sentence around the 0x4e487b71 entry (and keep the
existing parenthetical) so it consistently identifies 0x4e487b71 as
Panic(uint256) and not Error(string).
In `@test/solidity/BaseERC1271Fork.t.sol`:
- Around line 463-467: Replace the exact-balance assertion that checks
IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) == amount with a delta check:
read and store the pre-transfer balance from
IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) into a variable (e.g.,
preBalance), execute the permit transfer, then assert that the new balance minus
preBalance equals the expected amount (or use assertTrue(newBalance >=
preBalance + amount) if overflow-protection needed). Update the assertion that
references PERMIT2_PROXY_BASE, amount, and IERC20/USDT_BASE accordingly so the
test verifies an increase rather than an absolute equality.
- Around line 227-257: The Base test's _fundWalletAndApprovePermit2() issues
only a single approve(PERMIT2, max) via BATCH_EXECUTE_SELECTOR which breaks for
Tether-like tokens that require approve(0) before changing non-zero allowances;
update _fundWalletAndApprovePermit2() to perform two batch execute calls from
wallet: first encode/execute approve(PERMIT2, 0) and handle its ok/returnData
exactly like the existing call, then encode/execute approve(PERMIT2,
type(uint256).max) and keep the existing success checks and final balance
assertion; mirror the CoinbaseERC1271Fork.t.sol pattern/comments to document
"USDT requires approve(0) before approve(max)".
🧹 Nitpick comments (4)
script/playgroundHelpers.sh (1)
2076-2078: Minor:-sSflags are negated by2>/dev/null.
curl -sSmeans "silent but show errors on stderr", but2>/dev/nulldiscards stderr, making-Sa no-op. If error visibility on curl failures is intentional (per-S), remove2>/dev/null; otherwise simplify tocurl -s -L.Optional simplification
- RESPONSE=$(curl -sS -L "${FOURBYTE_URL}/?hex_signature=${SELECTOR}" 2>/dev/null || echo '{"count":0}') + RESPONSE=$(curl -s -L "${FOURBYTE_URL}/?hex_signature=${SELECTOR}" 2>/dev/null || echo '{"count":0}').cursor/commands/analyze-unverified-contract.md (1)
72-83: Section labels (D, B, C, A) are confusing — consider sequential numbering.The text says "Use this order for speed" but labels them D → B → C → A instead of 1 → 2 → 3 → 4. This makes it hard to follow the intended priority. Renumber to match the described execution order.
test/solidity/CoinbaseERC1271Fork.t.sol (1)
50-67: Significant code duplication withBaseERC1271Fork.t.sol— consider a shared base contract.Both test files duplicate: EIP-712 type hashes,
_permit2Digest,_encodeSignature,_signPermit2Permit,_signPermit2PermitForSpender,_replaySafeHashFromWallet,_buildSwapTokensGenericCalldata, and all four interfaces (IERC20,IPermit2ProxyView,IPermit2Proxy,IDiamondLoupe).Extract shared logic into a common base (e.g.
test/solidity/utils/BaseERC1271Test.sol) that bothCoinbaseERC1271ForkandBaseERC1271Forkinherit from. Chain-specific constants and delegation setup would remain in the concrete contracts.Also applies to: 148-179, 353-401, 571-595
test/solidity/BaseERC1271Fork.t.sol (1)
120-156:_fetchDomainFromDelegatorsilently falls through ifchainIdfrom the decoded domain doesn't match the fork's chain — consider a sanity check.If
eip712Domain()returns a stale or incorrectchainId(e.g., the implementation was deployed on a different chain and returns a hardcoded value),fetchedDomainSeparatorwill be built with the wrong chain ID, and all subsequent ERC-1271 signature verifications will silently fail. Adding arequire(chainId == block.chainid)or a warning log would make debugging easier if the fork block or delegation target changes.
opcodes-selectors.md
Outdated
| ## Excluded | ||
|
|
||
| - **`0xffffffff`** – Used as sentinel/mask in the bytecode (not a function selector). | ||
| - **`0x4e487b71`** – `Error(string)`; used for revert encoding, not dispatch. (4byte also lists as `Panic(uint256)`.) |
There was a problem hiding this comment.
0x4e487b71 is Panic(uint256), not Error(string).
The excluded-selector note says this is Error(string), but Error(string) is 0x08c379a0. The selector 0x4e487b71 is actually Panic(uint256) (as your own parenthetical note acknowledges). Fix the primary description to avoid confusion.
Suggested fix
-- **`0x4e487b71`** – `Error(string)`; used for revert encoding, not dispatch. (4byte also lists as `Panic(uint256)`.)
+- **`0x4e487b71`** – `Panic(uint256)`; used for compiler-generated panic reverts (e.g. arithmetic overflow), not function dispatch.📝 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.
| ## Excluded | |
| - **`0xffffffff`** – Used as sentinel/mask in the bytecode (not a function selector). | |
| - **`0x4e487b71`** – `Error(string)`; used for revert encoding, not dispatch. (4byte also lists as `Panic(uint256)`.) | |
| ## Excluded | |
| - **`0xffffffff`** – Used as sentinel/mask in the bytecode (not a function selector). | |
| - **`0x4e487b71`** – `Panic(uint256)`; used for compiler-generated panic reverts (e.g. arithmetic overflow), not function dispatch. |
🤖 Prompt for AI Agents
In `@opcodes-selectors.md` around lines 79 - 82, The description for the excluded
selector 0x4e487b71 is incorrect: replace the primary label "Error(string)" with
"Panic(uint256)" and optionally add the correct selector for Error(string)
(0x08c379a0) for clarity; update the explanatory sentence around the 0x4e487b71
entry (and keep the existing parenthetical) so it consistently identifies
0x4e487b71 as Panic(uint256) and not Error(string).
| function _fundWalletAndApprovePermit2() internal { | ||
| uint256 amount = 100 * 1e6; // 100 USDT (6 decimals); use 1000 if whale has sufficient balance | ||
| vm.prank(USDT_WHALE_BASE); | ||
| if (!IERC20(USDT_BASE).transfer(wallet, amount)) | ||
| revert WhaleTransferFailed(); | ||
|
|
||
| bytes memory approveCalldata = abi.encodeWithSelector( | ||
| bytes4(keccak256("approve(address,uint256)")), | ||
| PERMIT2, | ||
| type(uint256).max | ||
| ); | ||
| BatchExecuteCall[] memory calls = new BatchExecuteCall[](1); | ||
| calls[0] = BatchExecuteCall({ | ||
| target: USDT_BASE, | ||
| value: uint256(0), | ||
| data: approveCalldata | ||
| }); | ||
|
|
||
| vm.prank(wallet); | ||
| (bool ok, bytes memory returnData) = wallet.call( | ||
| abi.encodeWithSelector(BATCH_EXECUTE_SELECTOR, calls) | ||
| ); | ||
| if (!ok) { | ||
| if (returnData.length > 0) LibUtil.revertWith(returnData); | ||
| revert ApproveViaExecuteFailed(); | ||
| } | ||
|
|
||
| uint256 walletBalance = IERC20(USDT_BASE).balanceOf(wallet); | ||
| if (walletBalance != amount) | ||
| revert WalletShouldHaveUSDT(amount, walletBalance); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Base USDT address references
rg "0xfde4C96c8593536E31F229EA8f37b2ADa2699bb2|USDT_BASE" test/solidity/ --type sol -B2 -A2Repository: lifinance/contracts
Length of output: 90
🏁 Script executed:
# Search for approve patterns around USDT in test files, specifically looking for approve(0) patterns
rg "approve.*\(.*0.*\).*approve|approve.*0.*then|approve.*0.*before" test/solidity/ --type sol -B2 -A2Repository: lifinance/contracts
Length of output: 90
🏁 Script executed:
# Look at the BaseERC1271Fork.t.sol file to see if there are other USDT handling patterns and the Arbitrum reference
cat -n test/solidity/BaseERC1271Fork.t.sol | head -100Repository: lifinance/contracts
Length of output: 5684
🏁 Script executed:
# Search for Arbitrum USDT handling to compare with Base
rg "USDT.*Arbitrum|arbitrum.*USDT|0x.*USDT" test/solidity/ --type sol -B3 -A3 | head -60Repository: lifinance/contracts
Length of output: 90
🏁 Script executed:
# Check if there's a reference to how the Arbitrum version handles approve(0)
rg -n "_fundWalletAndApprovePermit2|approve.*0.*max|Arbitrum" test/solidity/BaseERC1271Fork.t.sol -B3 -A3Repository: lifinance/contracts
Length of output: 2010
🏁 Script executed:
fd "CoinbaseERC1271Fork" test/Repository: lifinance/contracts
Length of output: 102
🏁 Script executed:
# Search for approve patterns in test files (without -type since it was failing)
grep -r "approve.*0.*approve\|approve.*0.*type" test/ --include="*.sol" -nRepository: lifinance/contracts
Length of output: 837
🏁 Script executed:
# Look for USDT approvals in Base-related tests
grep -r "0xfde4C96c8593536E31F229EA8f37b2ADa2699bb2\|USDT_BASE" test/ --include="*.sol" -B5 -A5Repository: lifinance/contracts
Length of output: 14496
🏁 Script executed:
# Find CoinbaseERC1271Fork and examine its approve pattern
find test/ -name "*CoinbaseERC1271*" -o -name "*Coinbase*Fork*"Repository: lifinance/contracts
Length of output: 102
🏁 Script executed:
# Search for approve(0) patterns specifically in USDT contexts
grep -r "approve.*0\)" test/ --include="*.sol" -B3 -A1 | grep -A3 -B3 "USDT\|0xfde4"Repository: lifinance/contracts
Length of output: 86
🏁 Script executed:
# Get the _fundWalletAndApprovePermit2 function from CoinbaseERC1271Fork.t.sol
sed -n '184,220p' test/solidity/CoinbaseERC1271Fork.t.solRepository: lifinance/contracts
Length of output: 1473
🏁 Script executed:
# Look for approve(0) pattern in the Arbitrum version
grep -n "approve.*0\|approve.*max" test/solidity/CoinbaseERC1271Fork.t.sol -B2 -A2Repository: lifinance/contracts
Length of output: 444
🏁 Script executed:
# Check if the tests in BaseERC1271Fork actually run successfully by looking for test execution patterns
cat -n test/solidity/BaseERC1271Fork.t.sol | tail -100Repository: lifinance/contracts
Length of output: 3824
🏁 Script executed:
# Search for any comments or documentation about Base USDT vs Arbitrum USDT differences
grep -r "Base.*USDT\|USDT.*Base\|0xfde4" . --include="*.sol" --include="*.md" --include="*.ts" -B2 -A2Repository: lifinance/contracts
Length of output: 6464
🏁 Script executed:
# Check the full context around the _fundWalletAndApprovePermit2 in both files side-by-side to understand the difference
echo "=== ARBITRUM VERSION ===" && sed -n '184,250p' test/solidity/CoinbaseERC1271Fork.t.sol && echo -e "\n=== BASE VERSION ===" && sed -n '227,257p' test/solidity/BaseERC1271Fork.t.solRepository: lifinance/contracts
Length of output: 3719
Add approve(0) before approve(max) in _fundWalletAndApprovePermit2() to match the Arbitrum pattern.
Base USDT (0xfde4...) is Tether's token, which has the same non-standard approve() behavior as Arbitrum USDT. The parallel CoinbaseERC1271Fork.t.sol correctly implements two sequential calls—reset to 0, then set to max—with an explicit comment: "USDT requires approve(0) before approve(max)". The Base version skips the zero reset and only issues one batch execute call. This will fail if the wallet ever has a non-zero allowance to Permit2. Update _fundWalletAndApprovePermit2() to issue two batch execute calls: one for approve(PERMIT2, 0), then one for approve(PERMIT2, type(uint256).max).
🤖 Prompt for AI Agents
In `@test/solidity/BaseERC1271Fork.t.sol` around lines 227 - 257, The Base test's
_fundWalletAndApprovePermit2() issues only a single approve(PERMIT2, max) via
BATCH_EXECUTE_SELECTOR which breaks for Tether-like tokens that require
approve(0) before changing non-zero allowances; update
_fundWalletAndApprovePermit2() to perform two batch execute calls from wallet:
first encode/execute approve(PERMIT2, 0) and handle its ok/returnData exactly
like the existing call, then encode/execute approve(PERMIT2, type(uint256).max)
and keep the existing success checks and final balance assertion; mirror the
CoinbaseERC1271Fork.t.sol pattern/comments to document "USDT requires approve(0)
before approve(max)".
| assertEq( | ||
| IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE), | ||
| amount, | ||
| "Permit2Proxy should hold USDT after permit transfer" | ||
| ); |
There was a problem hiding this comment.
Same fragile exact-balance assertion as the Coinbase test — use a delta check.
Same issue as CoinbaseERC1271Fork.t.sol line 446: the proxy may already hold USDT at the forked block. Record balance before and assert the increase.
Suggested fix
+ uint256 proxyUsdtBefore = IERC20(USDT_BASE).balanceOf(
+ PERMIT2_PROXY_BASE
+ );
+
vm.prank(wallet);
IPermit2Proxy(PERMIT2_PROXY_BASE).callDiamondWithPermit2(
diamondCalldata,
permit,
signature
);
- assertEq(
- IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE),
+ assertEq(
+ IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) -
+ proxyUsdtBefore,
amount,
"Permit2Proxy should hold USDT after permit transfer"
);🤖 Prompt for AI Agents
In `@test/solidity/BaseERC1271Fork.t.sol` around lines 463 - 467, Replace the
exact-balance assertion that checks
IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) == amount with a delta check:
read and store the pre-transfer balance from
IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) into a variable (e.g.,
preBalance), execute the permit transfer, then assert that the new balance minus
preBalance equals the expected amount (or use assertTrue(newBalance >=
preBalance + amount) if overflow-protection needed). Update the assertion that
references PERMIT2_PROXY_BASE, amount, and IERC20/USDT_BASE accordingly so the
test verifies an increase rather than an absolute equality.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.cursor/rules/400-solidity-tests.mdc:
- Around line 17-19: Update the naming rule to accept the repository’s existing
fork and failure prefixes: add `testFork_` and `testFail_` to the allowed
prefixes list and document an exception that `testFork_` tests may contain
additional internal underscores (e.g., allow names like
`testFork_BaseERC1271_IsValidSignature_ReturnsMagicValue`), and update the
examples in the rule text to reflect these additions so the rule no longer
conflicts with the tests in this PR.
In `@src/Interfaces/IERC20TransferWithAuthorization.sol`:
- Around line 9-10: The NatSpec `@dev` comment in
IERC20TransferWithAuthorization.sol is broken by a regular '//' on the
continuation line; change the comment prefix for the continuation from '//' to
'///' so the entire explanation about transferWithAuthorization and
receiveWithAuthorization is included in generated docs (look for the comment
referencing transferWithAuthorization and receiveWithAuthorization and replace
the second-line '//' with '///').
In `@test/solidity/BaseERC1271Fork.t.sol`:
- Line 90: The test file disables all fork tests by setting the internal
constant _FORK_TESTS_DISABLED = true which causes each test to return
immediately; replace this pattern by either removing the constant and enabling
tests, switch the flag to false, or (preferably) change the per-test
early-return pattern to use Foundry's skipping mechanism (e.g., call
vm.skip(true) or use a Foundry profile/label) and add a short comment
documenting the reason for skipping so fork tests run or are explicitly skipped
instead of silently being disabled by _FORK_TESTS_DISABLED in
BaseERC1271Fork.t.sol.
In `@test/solidity/Periphery/Permit2Proxy/BaseERC1271Fork.t.sol`:
- Line 5: The import path uses a direct lib/ path instead of the remapped alias;
update the import of ISignatureTransfer so it uses the remapping
`permit2/interfaces/ISignatureTransfer.sol` (replace the existing `import {
ISignatureTransfer } from "lib/Permit2/src/interfaces/ISignatureTransfer.sol";`
with the remapped path) to match the project's remappings and other tests.
- Around line 89-93: The constant _FORK_TESTS_DISABLED is currently true which
causes setUp() to return early and disables all testFork_* tests; change
_FORK_TESTS_DISABLED to false (or replace it with a dynamic check using an
environment flag via vm.envOr or similar) so setUp() creates the fork state and
allows testFork_* functions to run, ensuring you update the constant or the
conditional in setUp() accordingly (refer to _FORK_TESTS_DISABLED and setUp()).
- Around line 481-485: Instead of asserting the absolute balance equals amount,
capture the pre-call balance and assert the delta equals amount: read
IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) into a beforeBalance variable
before the permit transfer, perform the transfer, then read afterBalance and
replace assertEq(IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE), amount, ...)
with assertEq(afterBalance - beforeBalance, amount, ...) so the test passes even
if PERMIT2_PROXY_BASE had a pre-existing USDT balance.
In `@test/solidity/Periphery/Permit2Proxy/CoinbaseERC1271Fork.t.sol`:
- Line 5: Change the import to use the permit2 remapping instead of the
hardcoded lib path: update the import that references ISignatureTransfer (the
import line importing ISignatureTransfer) to use "permit2/..." (match the same
remapping used in BaseERC1271Fork.t.sol) so the import reads from the remapped
permit2 package rather than "lib/Permit2/...".
🧹 Nitpick comments (10)
src/Interfaces/IERC20Permit7597.sol (1)
4-7: Consider adding a reference link to ERC-7597.Per coding guidelines, when copying from or shadowing an external protocol, add a short NatSpec note with a link referencing the original. A link to the ERC-7597 specification would help future readers.
test/solidity/utils/ERC1271Wallet.sol (1)
14-14:ownercan beimmutable.
owneris set once in the constructor and never modified. Making itimmutablewould be consistent withDELEGATE_TOand slightly cheaper on reads.Proposed fix
- address public owner; + address public immutable owner;test/solidity/BaseERC1271Fork.t.sol (1)
606-609: InlineIERC20interface is duplicated across test files.This same minimal
IERC20interface (andIPermit2ProxyView) is also defined intest/solidity/Periphery/Permit2Proxy.t.sol. Consider extracting a shared test utility interface (e.g., intest/solidity/utils/Interfaces.sol) and importing it from both files to reduce duplication.As per coding guidelines: "Flag duplicate code that should use existing helpers/libraries."
test/solidity/Periphery/Permit2Proxy.t.sol (1)
1134-1180: Double-use revert test: consider addingassertBalanceChangeor an explicit assertion after the first call.The first
callDiamondWithEIP3009Signaturecall (line 1154) is expected to succeed but has no assertion verifying it did. If it silently fails for an unexpected reason, the second call'svm.expectRevertmight match an unrelated revert, making the test pass vacuously. A brief assertion (e.g., verifying the nonce was consumed or the balance changed) after the first call would strengthen this test.src/Periphery/Permit2Proxy.sol (2)
78-127: Parameter naming: public function parameters lack underscore prefix.The existing
callDiamondWithEIP2612Signature(v,r,s)and all new public functions (callDiamondWithEIP3009Signature, etc.) use parameters liketokenAddress,amount,deadlineinstead of_tokenAddress,_amount,_deadline. This deviates from the codebase convention (e.g.callDiamondWithPermit2at Line 273 uses_diamondCalldata,_permit,_signature).Since the existing v,r,s overload already shipped without underscores this is likely intentional for consistency within the EIP-2612/3009 family, but it creates an inconsistency with the Permit2 functions in the same contract.
13-17: Consider adding bug-bounty custody documentation.Per coding guidelines, contracts in
src/should explicitly document whether they hold/custody funds. This proxy does transiently hold tokens (betweentransferFrom/receiveWithAuthorizationand the diamond call). A brief NatSpec note like "This contract transiently holds tokens during permit+bridge flows; funds should not persist beyond a single transaction" would clarify intent for auditors and bug bounty hunters.test/solidity/Periphery/Permit2Proxy/Permit2Proxy.t.sol (2)
360-413: Mid-test fork switch replaces allsetUp()state — fragile test isolation.
test_CanExecuteCalldataUsingEip2612CoinbaseWalletUsdccallsvm.createSelectForkat Line 365, which switches from the mainnet fork (set insetUp) to an Arbitrum fork. After this,permit2Proxy,uniPermit2, and all state fromsetUp()are stale. The test correctly creates a new localproxyWithBytes, but if someone later adds assertions referencing the originalpermit2Proxy, they'd get unexpected results.Consider adding a brief comment at the fork-switch explaining that setUp state is intentionally replaced, or extracting this into a separate test contract (similar to the
CoinbaseERC1271Forkpattern).
1029-1032: Constant defined between helper functions — minor organizational nit.
EIP3009_RECEIVE_WITH_AUTHORIZATION_TYPEHASHat Line 1029 is wedged between the_setCoinbaseDelegationAndInitializehelper and the_generateEIP3009ReceiveWithAuthorizationDigesthelper. Consider moving it up to the constants section (near Line 22-46) for consistency.test/solidity/Periphery/Permit2Proxy/BaseERC1271Fork.t.sol (1)
606-627: Interfaces duplicated across fork test files.
IERC20,IPermit2ProxyView,IPermit2Proxy, andIDiamondLoupeare defined identically in bothBaseERC1271Fork.t.solandCoinbaseERC1271Fork.t.sol. Consider extracting them into a shared test helper (e.g.,test/solidity/utils/ForkTestInterfaces.sol) to reduce duplication. That said, per coding guidelines, test interfaces may be defined inline, so this is optional.test/solidity/Periphery/Permit2Proxy/CoinbaseERC1271Fork.t.sol (1)
76-98:setUp()always creates a fork — tests will fail or be slow withoutETH_NODE_URI_ARBITRUM.Unlike
BaseERC1271Forkwhich has_FORK_TESTS_DISABLED, this test always forks Arbitrum at block 410M. If the RPC is unavailable or slow, all tests in this contract will fail or timeout. Consider adding a similar guard or usingvm.envOrto conditionally skip, especially since this is a draft PR.
| - **Exactly one underscore** in the function name: between the prefix and the test name (e.g. `test_CanExecuteCalldata`, `testRevert_WhenCalledWithInvalidCalldata`). No extra underscores inside the test name. | ||
| - **Prefixes:** `test_` (success), `testRevert_` (failure), `testBase_` (base). | ||
| - **Test name (after the underscore):** CamelCase, no internal underscores (e.g. `CanExecuteCalldataUsingEip2612SignatureUsdc`, not `can_execute_calldata_using_eip2612_signature_usdc`). |
There was a problem hiding this comment.
New naming rule contradicts test names in this PR.
The rule mandates exactly one underscore and only three prefixes (test_, testRevert_, testBase_), but the fork tests added in this same PR use testFork_ as a prefix with multiple internal underscores (e.g., testFork_BaseERC1271_IsValidSignature_ReturnsMagicValue). Additionally, the repo historically uses testFail_ per its style guide.
Consider either:
- Adding
testFork_(andtestFail_) to the allowed prefixes, or - Renaming the fork tests to comply (e.g.,
test_ForkBaseERC1271IsValidSignatureReturnsMagicValue).
🤖 Prompt for AI Agents
In @.cursor/rules/400-solidity-tests.mdc around lines 17 - 19, Update the naming
rule to accept the repository’s existing fork and failure prefixes: add
`testFork_` and `testFail_` to the allowed prefixes list and document an
exception that `testFork_` tests may contain additional internal underscores
(e.g., allow names like
`testFork_BaseERC1271_IsValidSignature_ReturnsMagicValue`), and update the
examples in the rule text to reflect these additions so the rule no longer
conflicts with the tests in this PR.
| /// @dev transferWithAuthorization is not used by LI.FI since anyone may call this on the token, | ||
| // so it can be front-run. We only call receiveWithAuthorization. |
There was a problem hiding this comment.
NatSpec continuation uses // instead of ///.
Line 10 switches from /// (NatSpec) to // (regular comment), so the rest of the @dev explanation won't appear in generated docs. Use /// for the continuation.
Proposed fix
/// `@dev` transferWithAuthorization is not used by LI.FI since anyone may call this on the token,
- // so it can be front-run. We only call receiveWithAuthorization.
+ /// so it can be front-run. We only call receiveWithAuthorization.🤖 Prompt for AI Agents
In `@src/Interfaces/IERC20TransferWithAuthorization.sol` around lines 9 - 10, The
NatSpec `@dev` comment in IERC20TransferWithAuthorization.sol is broken by a
regular '//' on the continuation line; change the comment prefix for the
continuation from '//' to '///' so the entire explanation about
transferWithAuthorization and receiveWithAuthorization is included in generated
docs (look for the comment referencing transferWithAuthorization and
receiveWithAuthorization and replace the second-line '//' with '///').
| bytes32 internal fetchedMessageTypeHash; | ||
|
|
||
| /// @dev Set true to deactivate fork tests (e.g. for coverage); set false to re-enable. No vm.skip (older forge-std). | ||
| bool internal constant _FORK_TESTS_DISABLED = true; |
There was a problem hiding this comment.
All fork tests are disabled — no test coverage is provided by this file.
_FORK_TESTS_DISABLED = true causes every test to return immediately in its first line. While this may be intentional for CI/coverage runs, it means the PR doesn't actually execute any of these tests. Consider using a more standard approach like vm.skip(true) or a Foundry profile/label to selectively run fork tests, and document the intention clearly.
🤖 Prompt for AI Agents
In `@test/solidity/BaseERC1271Fork.t.sol` at line 90, The test file disables all
fork tests by setting the internal constant _FORK_TESTS_DISABLED = true which
causes each test to return immediately; replace this pattern by either removing
the constant and enabling tests, switch the flag to false, or (preferably)
change the per-test early-return pattern to use Foundry's skipping mechanism
(e.g., call vm.skip(true) or use a Foundry profile/label) and add a short
comment documenting the reason for skipping so fork tests run or are explicitly
skipped instead of silently being disabled by _FORK_TESTS_DISABLED in
BaseERC1271Fork.t.sol.
| pragma solidity ^0.8.17; | ||
|
|
||
| import { Test } from "forge-std/Test.sol"; | ||
| import { ISignatureTransfer } from "lib/Permit2/src/interfaces/ISignatureTransfer.sol"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Import path should use the permit2/ remapping instead of a direct lib/ path.
Line 5 uses lib/Permit2/src/interfaces/ISignatureTransfer.sol while the rest of the codebase (and the other test files in this PR) use the permit2/interfaces/ISignatureTransfer.sol remapping. As per coding guidelines, use remappings defined in remappings.txt.
Suggested fix
-import { ISignatureTransfer } from "lib/Permit2/src/interfaces/ISignatureTransfer.sol";
+import { ISignatureTransfer } from "permit2/interfaces/ISignatureTransfer.sol";📝 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.
| import { ISignatureTransfer } from "lib/Permit2/src/interfaces/ISignatureTransfer.sol"; | |
| import { ISignatureTransfer } from "permit2/interfaces/ISignatureTransfer.sol"; |
🤖 Prompt for AI Agents
In `@test/solidity/Periphery/Permit2Proxy/BaseERC1271Fork.t.sol` at line 5, The
import path uses a direct lib/ path instead of the remapped alias; update the
import of ISignatureTransfer so it uses the remapping
`permit2/interfaces/ISignatureTransfer.sol` (replace the existing `import {
ISignatureTransfer } from "lib/Permit2/src/interfaces/ISignatureTransfer.sol";`
with the remapped path) to match the project's remappings and other tests.
| /// @dev Set true to deactivate fork tests (e.g. for coverage); set false to re-enable. No vm.skip (older forge-std). | ||
| bool internal constant _FORK_TESTS_DISABLED = true; | ||
|
|
||
| function setUp() public { | ||
| if (_FORK_TESTS_DISABLED) return; |
There was a problem hiding this comment.
All fork tests are disabled — ensure this is intentional for the draft PR.
_FORK_TESTS_DISABLED = true means none of the testFork_* functions in this file will execute. Combined with the early return in setUp(), no fork state is created. This is fine for a draft, but should be toggled to false (or replaced with a more dynamic mechanism like vm.envOr) before merging.
🤖 Prompt for AI Agents
In `@test/solidity/Periphery/Permit2Proxy/BaseERC1271Fork.t.sol` around lines 89 -
93, The constant _FORK_TESTS_DISABLED is currently true which causes setUp() to
return early and disables all testFork_* tests; change _FORK_TESTS_DISABLED to
false (or replace it with a dynamic check using an environment flag via vm.envOr
or similar) so setUp() creates the fork state and allows testFork_* functions to
run, ensuring you update the constant or the conditional in setUp() accordingly
(refer to _FORK_TESTS_DISABLED and setUp()).
| assertEq( | ||
| IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE), | ||
| amount, | ||
| "Permit2Proxy should hold USDT after permit transfer" | ||
| ); |
There was a problem hiding this comment.
Exact-balance assertion is fragile — assert the balance delta instead.
Same issue as flagged in the Coinbase fork test: the deployed PERMIT2_PROXY_BASE may already hold USDT at the forked block. assertEq(balance, amount) will fail if there's any pre-existing balance. Record the balance before the call and assert the increase.
Suggested fix
+ uint256 proxyUsdtBefore = IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE);
+
vm.prank(wallet);
IPermit2Proxy(PERMIT2_PROXY_BASE).callDiamondWithPermit2(
diamondCalldata,
permit,
signature
);
assertEq(
- IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE),
+ IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) - proxyUsdtBefore,
amount,
"Permit2Proxy should hold USDT after permit transfer"
);🤖 Prompt for AI Agents
In `@test/solidity/Periphery/Permit2Proxy/BaseERC1271Fork.t.sol` around lines 481
- 485, Instead of asserting the absolute balance equals amount, capture the
pre-call balance and assert the delta equals amount: read
IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE) into a beforeBalance variable
before the permit transfer, perform the transfer, then read afterBalance and
replace assertEq(IERC20(USDT_BASE).balanceOf(PERMIT2_PROXY_BASE), amount, ...)
with assertEq(afterBalance - beforeBalance, amount, ...) so the test passes even
if PERMIT2_PROXY_BASE had a pre-existing USDT balance.
| pragma solidity ^0.8.17; | ||
|
|
||
| import { Test } from "forge-std/Test.sol"; | ||
| import { ISignatureTransfer } from "lib/Permit2/src/interfaces/ISignatureTransfer.sol"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Import path should use the permit2/ remapping.
Same as BaseERC1271Fork.t.sol — use the remapping for consistency.
Suggested fix
-import { ISignatureTransfer } from "lib/Permit2/src/interfaces/ISignatureTransfer.sol";
+import { ISignatureTransfer } from "permit2/interfaces/ISignatureTransfer.sol";🤖 Prompt for AI Agents
In `@test/solidity/Periphery/Permit2Proxy/CoinbaseERC1271Fork.t.sol` at line 5,
Change the import to use the permit2 remapping instead of the hardcoded lib
path: update the import that references ISignatureTransfer (the import line
importing ISignatureTransfer) to use "permit2/..." (match the same remapping
used in BaseERC1271Fork.t.sol) so the import reads from the remapped permit2
package rather than "lib/Permit2/...".
There was a problem hiding this comment.
This file will be helpful for testing once we have a verified delegator contract or more info about how it works
2 participants
Which Jira task belongs to this PR?
https://lifi.atlassian.net/browse/LF-15521
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)