feat: Checking LZ OFTLimits on transferTokenLayerZero (DEV-1140)#235
Open
TheMj0ln1r wants to merge 4 commits intofix/rate-limit-for-transferTokenLayerZerofrom
Open
feat: Checking LZ OFTLimits on transferTokenLayerZero (DEV-1140)#235TheMj0ln1r wants to merge 4 commits intofix/rate-limit-for-transferTokenLayerZerofrom
TheMj0ln1r wants to merge 4 commits intofix/rate-limit-for-transferTokenLayerZerofrom
Conversation
Summary by OctaneNew ContractsNo new contracts were added. Updated Contracts
🔗 Commit Hash: b324168 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThe changes enforce OFT (Omnichain Fungible Token) limit boundaries in the LayerZero library's transfer function by decoding and validating minimum and maximum amount limits before token transfer execution. Comprehensive test coverage is added to verify boundary-case scenarios across multiple test suites. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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
🤖 Fix all issues with AI agents
In `@test/mainnet-fork/LayerZero.t.sol`:
- Around line 647-741: The test is operating against mainnet fixtures but runs
on the Arbitrum fork; replace references to mainnetController, rateLimits,
almProxy, usdt (and SPARK_PROXY when impersonating) with the corresponding
foreign-controller fixtures (e.g., foreignController, foreignRateLimits,
foreignAlmProxy, foreignUsdt, and foreign SPARK_PROXY variable) so the setup,
rate-limit mutation, deals, and transferTokenLayerZero calls target the Arbitrum
deployment; update calls to ILayerZero(USDT_OFT) to use the foreign token
variable and ensure setRateLimitData, setLayerZeroRecipient, deal() and
transferTokenLayerZero invocations reference the foreign fixtures and relayer as
in the other forked tests.
Comment on lines
+647
to
+741
| function test_transferTokenLayerZero_OFTLimitBoundary() external { | ||
| bytes32 target = bytes32(uint256(uint160(makeAddr("layerZeroRecipient")))); | ||
|
|
||
| vm.startPrank(SPARK_PROXY); | ||
|
|
||
| rateLimits.setRateLimitData( | ||
| keccak256(abi.encode( | ||
| mainnetController.LIMIT_LAYERZERO_TRANSFER(), | ||
| USDT_OFT, | ||
| destinationEndpointId | ||
| )), | ||
| type(uint256).max, | ||
| 0 | ||
| ); | ||
|
|
||
| mainnetController.setLayerZeroRecipient(destinationEndpointId, target); | ||
|
|
||
| vm.stopPrank(); | ||
|
|
||
| // Setup token balances | ||
| deal(address(usdt), address(almProxy), type(uint256).max); | ||
| deal(relayer, 1 ether); // Gas cost for LayerZero | ||
|
|
||
| bytes memory options = OptionsBuilder.newOptions().addExecutorLzReceiveOption(200_000, 0); | ||
|
|
||
| // NOTE: amount under OFTLimit minAmountLD boundary test can't be done with USDT_OFT | ||
| // USDT_OFT OFTLimit.minAmountLD = 0, So, we can't pass amount less than 0 | ||
|
|
||
| // amount over OFTLimit maxAmountLD boundary | ||
|
|
||
| SendParam memory sendParams = SendParam({ | ||
| dstEid : destinationEndpointId, | ||
| to : target, | ||
| amountLD : 18446744073709551616, // type(uint64) + 1 | ||
| minAmountLD : 18446744073709551616, // type(uint64) + 1 | ||
| extraOptions : options, | ||
| composeMsg : "", | ||
| oftCmd : "" | ||
| }); | ||
|
|
||
| MessagingFee memory fee = ILayerZero(USDT_OFT).quoteSend(sendParams, false); | ||
|
|
||
| // OFTLimits remains same | ||
| (OFTLimit memory limits, ,) = ILayerZero(USDT_OFT).quoteOFT(sendParams); | ||
|
|
||
| vm.prank(relayer); | ||
| vm.expectRevert("LayerZeroLib/amount-above-max"); | ||
| mainnetController.transferTokenLayerZero{value: fee.nativeFee}( | ||
| USDT_OFT, | ||
| limits.maxAmountLD + 1, | ||
| destinationEndpointId | ||
| ); | ||
|
|
||
| // amount at OFTLimit minAmountLD boundary | ||
|
|
||
| sendParams = SendParam({ | ||
| dstEid : destinationEndpointId, | ||
| to : target, | ||
| amountLD : 0, | ||
| minAmountLD : 0, | ||
| extraOptions : options, | ||
| composeMsg : "", | ||
| oftCmd : "" | ||
| }); | ||
|
|
||
| fee = ILayerZero(USDT_OFT).quoteSend(sendParams, false); | ||
|
|
||
| vm.prank(relayer); | ||
| mainnetController.transferTokenLayerZero{value: fee.nativeFee}( | ||
| USDT_OFT, | ||
| limits.minAmountLD, | ||
| destinationEndpointId | ||
| ); | ||
|
|
||
| // amount at OFTLimit maxAmountLD boundary | ||
|
|
||
| sendParams = SendParam({ | ||
| dstEid : destinationEndpointId, | ||
| to : target, | ||
| amountLD : type(uint64).max, | ||
| minAmountLD : type(uint64).max, | ||
| extraOptions : options, | ||
| composeMsg : "", | ||
| oftCmd : "" | ||
| }); | ||
|
|
||
| fee = ILayerZero(USDT_OFT).quoteSend(sendParams, false); | ||
|
|
||
| vm.prank(relayer); | ||
| mainnetController.transferTokenLayerZero{value: fee.nativeFee}( | ||
| USDT_OFT, | ||
| limits.maxAmountLD, | ||
| destinationEndpointId | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix Arbitrum fork boundary test to use foreign controller fixtures.
This block configures and calls mainnetController/rateLimits/almProxy/usdt on the Arbitrum fork, so it targets the wrong deployment and will likely fail or become a no-op. Use the foreign controller fixtures instead.
Proposed fix
- vm.startPrank(SPARK_PROXY);
+ vm.startPrank(SPARK_EXECUTOR);
- rateLimits.setRateLimitData(
+ foreignRateLimits.setRateLimitData(
keccak256(abi.encode(
- mainnetController.LIMIT_LAYERZERO_TRANSFER(),
+ foreignController.LIMIT_LAYERZERO_TRANSFER(),
USDT_OFT,
destinationEndpointId
)),
type(uint256).max,
0
);
- mainnetController.setLayerZeroRecipient(destinationEndpointId, target);
+ foreignController.setLayerZeroRecipient(destinationEndpointId, target);
- deal(address(usdt), address(almProxy), type(uint256).max);
+ deal(USDT0, address(foreignAlmProxy), type(uint256).max);
- mainnetController.transferTokenLayerZero{value: fee.nativeFee}(
+ foreignController.transferTokenLayerZero{value: fee.nativeFee}(
USDT_OFT,
limits.maxAmountLD + 1,
destinationEndpointId
);
- mainnetController.transferTokenLayerZero{value: fee.nativeFee}(
+ foreignController.transferTokenLayerZero{value: fee.nativeFee}(
USDT_OFT,
limits.minAmountLD,
destinationEndpointId
);
- mainnetController.transferTokenLayerZero{value: fee.nativeFee}(
+ foreignController.transferTokenLayerZero{value: fee.nativeFee}(
USDT_OFT,
limits.maxAmountLD,
destinationEndpointId
);🤖 Prompt for AI Agents
In `@test/mainnet-fork/LayerZero.t.sol` around lines 647 - 741, The test is
operating against mainnet fixtures but runs on the Arbitrum fork; replace
references to mainnetController, rateLimits, almProxy, usdt (and SPARK_PROXY
when impersonating) with the corresponding foreign-controller fixtures (e.g.,
foreignController, foreignRateLimits, foreignAlmProxy, foreignUsdt, and foreign
SPARK_PROXY variable) so the setup, rate-limit mutation, deals, and
transferTokenLayerZero calls target the Arbitrum deployment; update calls to
ILayerZero(USDT_OFT) to use the foreign token variable and ensure
setRateLimitData, setLayerZeroRecipient, deal() and transferTokenLayerZero
invocations reference the foreign fixtures and relayer as in the other forked
tests.
TheMj0ln1r
changed the base branch from
dev
to
fix/rate-limit-for-transferTokenLayerZero
TheMj0ln1r
force-pushed
the
dev-1140-lz-oft-limit-checks
branch
from
b324168 to
5aebe7e
Compare
Overview
🔗 Commit Hash: b324168 |
lucas-manuel
added
Status: Revisions Needed
and removed
Status: Ready for Review
labels
|
Coverage after merging dev-1140-lz-oft-limit-checks into fix/rate-limit-for-transferTokenLayerZero will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.