fix(contracts-rfq): add forwardTo to ZapData for Zap Actions that don't forward assets to the user
#3451
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to several smart contracts and their respective interfaces to introduce a new parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SynapseIntentPreviewer
participant TokenZapV1
User->>SynapseIntentPreviewer: previewIntent(swapQuoter, forwardTo, tokenIn, tokenOut, amountIn)
SynapseIntentPreviewer->>TokenZapV1: zap(token, amount, zapData)
TokenZapV1->>TokenZapV1: encodeZapData(target, payload, amountPosition, finalToken, forwardTo)
TokenZapV1->>User: forwardToken(finalToken, forwardTo)
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
ChiTimesChi
changed the title
forrwardTo to ZapData for Zap Actions that don't forward assets to the userforwardTo to ZapData for Zap Actions that don't forward assets to the user
Deploying sanguine-fe with
|
| Latest commit: |
4b925f9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f54631c5.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-fbv2-zap-data-funds-reci.sanguine-fe.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function finalToken(bytes calldata encodedZapData) internal pure returns (address finalToken_) { | ||
| // Load 32 bytes from the offset and shift it 96 bits to the right to get the highest 160 bits. | ||
| assembly { | ||
| finalToken_ := shr(96, calldataload(add(encodedZapData.offset, OFFSET_FINAL_TOKEN))) | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Assembly usage Warning
| function forwardTo(bytes calldata encodedZapData) internal pure returns (address forwardTo_) { | ||
| // Load 32 bytes from the offset and shift it 96 bits to the right to get the highest 160 bits. | ||
| assembly { | ||
| forwardTo_ := shr(96, calldataload(add(encodedZapData.offset, OFFSET_FORWARD_TO))) | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Assembly usage Warning
| function _forwardToken(address token, address forwardTo) internal { | ||
| // Check the token address and its balance to be safely forwarded. | ||
| if (token == address(0)) revert TokenZapV1__TokenZeroAddress(); | ||
| uint256 amount = token == NATIVE_GAS_TOKEN ? address(this).balance : IERC20(token).balanceOf(address(this)); | ||
| if (amount == 0) revert TokenZapV1__FinalTokenBalanceZero(); | ||
| // Forward the full balance of the final token to the specified recipient. | ||
| if (token == NATIVE_GAS_TOKEN) { | ||
| Address.sendValue({recipient: payable(forwardTo), amount: amount}); | ||
| } else { | ||
| IERC20(token).safeTransfer(forwardTo, amount); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Dangerous strict equalities Medium
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/syn-intent-router #3451 +/- ##
================================================================
- Coverage 98.71087% 98.57904% -0.13183%
================================================================
Files 11 11
Lines 543 563 +20
================================================================
+ Hits 536 555 +19
- Misses 7 8 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (1)
45-45: Consider adding test cases for non-zero forwardTo address.While this file focuses on gas benchmarking, it would be valuable to add test cases that verify the gas consumption when
forwardTois set to a non-zero address, as this represents a real-world usage scenario described in the PR objectives.Example test case:
+ function test_deposit_erc20_with_forward() public { + address forwardTo = makeAddr("forwardTo"); + bytes memory depositPayload = getVaultPayload(address(erc20), AMOUNT); + bytes memory zapData = tokenZap.encodeZapData( + address(vault), + depositPayload, + 4 + 32, + address(0), + forwardTo + ); + erc20.transfer(address(tokenZap), AMOUNT); + tokenZap.zap(address(erc20), AMOUNT, zapData); + // Check that the vault registered the deposit for forwardTo + assertEq(vault.balanceOf(forwardTo, address(erc20)), AMOUNT); + }packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1)
Line range hint
166-168: Update helper functions to supportforwardToparameterThe
getDepositPayloadfunction currently hardcodes the user address as the recipient. To properly test the forwarding functionality, consider parameterizing the recipient address:-function getDepositPayload(address token) public view returns (bytes memory) { - return abi.encodeCall(dstVault.deposit, (token, abi.encode(token), DST_AMOUNT, user, abi.encode(user))); +function getDepositPayload( + address token, + address recipient +) public view returns (bytes memory) { + return abi.encodeCall( + dstVault.deposit, + (token, abi.encode(token), DST_AMOUNT, recipient, abi.encode(recipient)) + ); +}packages/contracts-rfq/contracts/libs/ZapDataV1.sol (1)
91-98: Review the use of inline assembly for address extractionThe functions
finalTokenandforwardTocorrectly use inline assembly to extract address values from the encoded data. However, to enhance readability and maintainability, consider adding comments within the assembly blocks to explain the logic.Apply this diff to include explanatory comments:
function finalToken(bytes calldata encodedZapData) internal pure returns (address finalToken_) { // Load 32 bytes from the offset and shift it 96 bits to the right to get the highest 160 bits. assembly { + // calldataload reads 32 bytes from the specified position + // We shift right by 96 bits (12 bytes) to obtain the address from the lower 20 bytes finalToken_ := shr(96, calldataload(add(encodedZapData.offset, OFFSET_FINAL_TOKEN))) } } function forwardTo(bytes calldata encodedZapData) internal pure returns (address forwardTo_) { // Load 32 bytes from the offset and shift it 96 bits to the right to get the highest 160 bits. assembly { + // Similar to finalToken_, extract the address from the correct position forwardTo_ := shr(96, calldataload(add(encodedZapData.offset, OFFSET_FORWARD_TO))) } }This will make it easier for future developers to understand the purpose of each operation within the assembly blocks.
Also applies to: 99-106
🧰 Tools
🪛 GitHub Check: Slither
[warning] 92-97: Assembly usage
ZapDataV1.finalToken(bytes) (contracts/libs/ZapDataV1.sol#92-97) uses assembly
- INLINE ASM (contracts/libs/ZapDataV1.sol#94-96)packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (1)
170-182: Address the potential strict equality issue in_forwardTokenThe usage of
if (amount == 0)in_forwardTokenis flagged due to strict equality comparison, which can be problematic ifamountcan be a small value subject to rounding errors. Ensure that the logic correctly handles scenarios where the amount is negligibly small.Consider using
require(amount > 0, "Amount must be greater than zero")if a zero amount should revert, or handle zero amounts gracefully if appropriate.🧰 Tools
🪛 GitHub Check: Slither
[warning] 171-182: Dangerous strict equalities
TokenZapV1._forwardToken(address,address) (contracts/zaps/TokenZapV1.sol#171-182) uses a dangerous strict equality:
- amount == 0 (contracts/zaps/TokenZapV1.sol#175)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (3)
24-24: Typographical error in error name:SIP__NoOpForwardNotSupportedThe error name
SIP__NoOpForwardNotSupportedmay contain a typo in "Hative" in the function_createHandleHativeSteps.Review and correct the error name or any related comments to ensure consistency and clarity.
31-34: Update interface documentation forpreviewIntentmethodThe
previewIntentmethod signature has changed with the addition of theforwardToparameter. Ensure that the interface and any inherited documentation reflect this change to maintain consistency.Update the NatSpec comments to include the
forwardToparameter and explain its purpose.
221-232: Typo in function name_createHandleHativeStepsThe function
_createHandleHativeStepsappears to have a typo. It likely should be_createHandleNativeSteps.Apply this diff to correct the function name:
- function _createHandleHativeSteps( + function _createHandleNativeSteps( address tokenIn, address tokenOut, uint256 amountIn, address forwardTo ) internal pure returns (ISynapseIntentRouter.StepParams[] memory steps) { if (tokenIn == NATIVE_GAS_TOKEN) { // tokenOut is Wrapped Native return _toStepsArray( _createWrapNativeStep({wrappedNative: tokenOut, msgValue: amountIn, forwardTo: forwardTo}) ); } // Sanity check tokenOut if (tokenOut != NATIVE_GAS_TOKEN) revert SIP__TokenNotNative(); // tokenIn is Wrapped Native return _toStepsArray(_createUnwrapNativeStep({wrappedNative: tokenIn, forwardTo: forwardTo})); }Update all references to this function throughout the contract to reflect the corrected name.
Also applies to: 237-237
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (2)
181-208: Add test cases for wrapping and unwrapping with forwardingNew functions
getZapDataUnwrap,getZapDataUnwrapAndForward,getZapDataWrap, andgetZapDataWrapAndForwardhave been introduced. However, corresponding test cases that utilize these functions should be added to verify that wrapping and unwrapping operations with forwarding work as intended.Consider adding test cases that:
- Verify successful wrapping and forwarding to a recipient.
- Test unwrapping and forwarding, both to EOAs and contracts.
- Check for failure scenarios when
finalTokenorforwardToare misuse.This will enhance the test coverage for the new functionality.
Line range hint
426-438: Use bounded values in property-based testsIn the test
test_encodeZapData_roundtrip, ensure that theamountparameter is within a reasonable range to prevent potential overflows or unexpected behavior during encoding and decoding.Consider bounding the
amountparameter usingbound(amount, minValue, maxValue);to ensure it stays within safe limits during testing.packages/contracts-rfq/test/libs/ZapDataV1.t.sol (1)
166-175: Consider extracting common test data setupThe test data setup for
invalidEncodedZapDatais repeated across multiple test cases. Consider extracting it into a helper function to improve maintainability.+ function createInvalidEncodedZapData( + uint16 version, + uint16 amountPosition, + address finalToken, + address forwardTo, + address target, + bytes memory payload + ) internal pure returns (bytes memory) { + return encodeZapData(version, amountPosition, finalToken, forwardTo, target, payload); + }packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (1)
593-650: Consider adding test cases for failed intermediate stepsWhile the test coverage for successful double-step intents is comprehensive, consider adding test cases for scenarios where intermediate steps fail, such as:
- Failed swap before unwrap
- Failed wrap before swap
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol(2 hunks)packages/contracts-rfq/contracts/libs/ZapDataV1.sol(4 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(15 hunks)packages/contracts-rfq/contracts/zaps/TokenZapV1.sol(6 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json(3 hunks)packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json(4 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json(3 hunks)packages/contracts-rfq/deployments/optimism/TokenZapV1.json(4 hunks)packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol(1 hunks)packages/contracts-rfq/test/integration/TokenZapV1.t.sol(2 hunks)packages/contracts-rfq/test/libs/ZapDataV1.t.sol(6 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol(12 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(5 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol(1 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.t.sol(11 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/libs/ZapDataV1.sol
[warning] 92-97: Assembly usage
ZapDataV1.finalToken(bytes) (contracts/libs/ZapDataV1.sol#92-97) uses assembly
- INLINE ASM (contracts/libs/ZapDataV1.sol#94-96)
[warning] 100-105: Assembly usage
ZapDataV1.forwardTo(bytes) (contracts/libs/ZapDataV1.sol#100-105) uses assembly
- INLINE ASM (contracts/libs/ZapDataV1.sol#102-104)
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
[warning] 171-182: Dangerous strict equalities
TokenZapV1._forwardToken(address,address) (contracts/zaps/TokenZapV1.sol#171-182) uses a dangerous strict equality:
- amount == 0 (contracts/zaps/TokenZapV1.sol#175)
🔇 Additional comments (49)
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (1)
45-45: LGTM! The test correctly adapts to the new interface.
The modification properly handles the new encodeZapData parameters by passing address(0) for both finalToken and forwardTo, which represents the baseline case for gas benchmarking.
packages/contracts-rfq/deployments/optimism/TokenZapV1.json (3)
73-82: LGTM! New parameters align with PR objectives.
The addition of finalToken and forwardTo parameters to encodeZapData properly implements the forwarding functionality described in the PR objectives. The parameter types are correctly defined as addresses.
161-165: LGTM! New error types enhance validation.
The added error types improve the contract's error handling:
TokenZapV1__FinalTokenBalanceZero: Ensures operations result in non-zero token balanceTokenZapV1__TokenZeroAddress: Validates token address input
These follow the existing error naming convention and provide clear error messages.
Also applies to: 176-180
2-2: Verify the new contract deployment on Optimism.
The contract has been redeployed with a new address. Please ensure the deployment was successful and the contract is properly verified on Optimism block explorer.
Also applies to: 5-6
✅ Verification successful
Contract deployment verified successfully on Optimism
The contract at address 0x6327797F149a75D506aFda46D5fCE6E74fC409D5 is:
- Successfully deployed and verified on Optimism
- Contains the expected ABI with new parameters and error types
- Deployed at block
129029951which is confirmed to exist on the network
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and bytecode on Optimism
# Check if contract exists and is verified on Optimism
curl -s "https://api-optimistic.etherscan.io/api?module=contract&action=getabi&address=0x6327797F149a75D506aFda46D5fCE6E74fC409D5" | jq .
# Verify block number exists
curl -s "https://api-optimistic.etherscan.io/api?module=block&action=getblockreward&blockno=129029951" | jq .
Length of output: 3333
packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json (3)
31-35: LGTM: forwardTo parameter addition
The new forwardTo parameter is correctly added to the previewIntent function with proper type definition. This change aligns with the PR objectives to support forwarding assets to specified addresses.
88-92: LGTM: Error handling enhancement
The addition of SIP__NoOpForwardNotSupported error type enhances the contract's error handling capabilities for cases where forwarding is attempted on no-op intents.
2-2: Verify contract deployment on Arbitrum
The contract has been redeployed to a new address with updated transaction details. This is expected due to the interface changes.
Also applies to: 5-6
packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (3)
2-2: Contract redeployment verified.
The contract has been redeployed to a new address with updated transaction details, which is expected given the functional changes.
Also applies to: 5-6
161-165: New error types enhance input validation.
The addition of two new error types improves the contract's robustness:
TokenZapV1__FinalTokenBalanceZero: Ensures there are tokens to forwardTokenZapV1__TokenZeroAddress: Validates token addresses
This is a good practice for providing clear error messages.
Also applies to: 176-180
73-82: New parameters align with PR objectives.
The addition of finalToken and forwardTo parameters to encodeZapData properly implements the requirement to support forwarding assets to a specified address.
Let's verify the integration with related contracts:
✅ Verification successful
Parameters are correctly integrated across the codebase
The verification confirms that the finalToken and forwardTo parameters in TokenZapV1.json are properly aligned with the ZapDataV1 library implementation. The library shows these parameters at the correct offsets (OFFSET_FINAL_TOKEN and OFFSET_FORWARD_TO), and the TokenZapV1 contract properly implements the IZapRecipient interface to handle these parameters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of ZapData encoding across the codebase
# Check for matching parameter updates in related files
rg -A 10 "encodeZapData|encodeV1" --type sol
Length of output: 74
Script:
#!/bin/bash
# Search for Solidity files with .sol extension and look for encodeZapData or encodeV1
rg -t sol -g "*.sol" -A 10 "encodeZapData|encodeV1"
# Also check for any interface definitions or implementations
fd -e sol -x cat {} | rg -A 10 "interface.*Zap|contract.*Zap"
Length of output: 14537
Script:
#!/bin/bash
# Let's check the ZapDataV1 library implementation to verify parameter handling
fd -e sol -x cat {} | rg -A 20 "library ZapDataV1"
# Also check for any TokenZapV1 contract implementations
fd -e sol -x cat {} | rg -A 20 "contract TokenZapV1"
Length of output: 4420
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (20)
79-81: 'getWrapZapData' function updated with 'finalToken' and 'forwardTo'.
The addition of finalToken and forwardTo parameters in the getWrapZapData function enhances the flexibility of token zapping operations, aligning with the PR objectives.
85-92: 'getUnwrapZapData' function now accepts 'forwardTo' parameter.
The modification to include forwardTo in getUnwrapZapData correctly allows proceeds to be forwarded to a specified address, as intended.
96-105: Updated 'getSwapZapData' to incorporate 'forwardTo'.
The inclusion of the forwardTo parameter in getSwapZapData ensures that swapped tokens can be directed to a specified recipient, which is consistent with the enhancement goals.
115-117: Inclusion of 'finalToken' and 'forwardTo' in 'getDepositZapData'.
Adding finalToken and forwardTo parameters to getDepositZapData improves the modularity and reusability of the code, facilitating better control over token destinations.
429-444: New function 'getSwapForwardERC20Steps' implemented effectively.
The getSwapForwardERC20Steps function correctly sets up the steps for swapping WETH to ERC20 tokens and forwarding them to the user, aligning with the new forwarding functionality.
447-459: Test 'test_swapForwardERC20_exactAmount' added for swapping and forwarding.
The test accurately verifies that the user receives the correct amount of ERC20 tokens after swapping and that the forwarding mechanism works as expected.
462-474: Extra funds handling in 'test_swapForwardERC20_exactAmount_extraFunds' is correct.
The test confirms that extra funds are appropriately utilized when forwarding tokens, ensuring no discrepancies in user balances.
477-479: Deadline exceedance test 'test_swapForwardERC20_exactAmount_revert_deadlineExceeded' is properly implemented.
The test ensures the contract correctly reverts the transaction when the deadline is exceeded, adhering to expected behavior.
481-484: Insufficient amount test 'test_swapForwardERC20_exactAmount_revert_lastStepAmountInsufficient' functions correctly.
This test validates that the contract reverts when the last step amount is insufficient, maintaining transaction integrity.
486-489: Message value check in 'test_swapForwardERC20_exactAmount_revert_msgValueAboveExpected' is accurate.
The test correctly handles scenarios where msgValue is above expected, ensuring proper error handling.
491-503: Full balance swap in 'test_swapForwardERC20_fullBalance' operates as intended.
The test verifies that swapping the full balance works correctly and that tokens are forwarded appropriately.
506-518: Handling extra funds in 'test_swapForwardERC20_fullBalance_extraFunds' is correctly validated.
This test ensures extra funds are accounted for in full balance swaps and that user balances reflect the correct amounts.
520-534: Revert conditions in 'swapForwardERC20' tests are properly checked.
The added tests comprehensively cover revert scenarios, such as deadline exceedance and insufficient amounts, ensuring robust error handling.
538-561: Function 'getSwapUnwrapForwardNativeSteps' added for swapping, unwrapping, and forwarding.
The implementation correctly sets up the steps to swap ERC20 tokens to WETH, unwrap WETH to native tokens, and forward them to the user.
563-576: Test 'test_swapUnwrapForwardNative_exactAmounts' verifies swapping, unwrapping, and forwarding.
The test accurately confirms that the user receives the correct amount of native tokens, ensuring the steps function cohesively.
579-592: Extra funds handling in 'test_swapUnwrapForwardNative_exactAmounts_extraFunds' is validated.
The test ensures that extra funds are correctly utilized in the last forward instruction, updating the user's balance appropriately.
677-695: Test 'test_swapUnwrapForwardNative_exactAmount1_extraFunds_revertWithBalanceChecks' correctly handles extra funds.
The test demonstrates that extra funds are processed when no balance checks are performed, and the user's native balance reflects the additional amount.
780-782: 'zapData' modification in 'getSwapDepositERC20Steps' aligns with updates.
The use of getSwapZapData with address(0) for forwardTo parameter is appropriate for the deposit operation.
Line range hint 780-856: Swap and deposit ERC20 tests added and implemented correctly.
The suite of tests for swapping WETH to ERC20 tokens and depositing them ensures all scenarios, including extra funds and revert conditions, are thoroughly covered.
1173-1175: 'zapData' in 'getUnwrapDepositNativeSteps' correctly updated with 'forwardTo'.
The use of getUnwrapZapData with address(0) for forwardTo parameter aligns with the intended functionality of depositing native tokens.
packages/contracts-rfq/contracts/libs/ZapDataV1.sol (1)
15-18: Offsets in struct definition and constants are updated correctly
The updated offsets for finalToken, forwardTo, and target in the struct definition and their corresponding constants accurately reflect the sizes and positions of the new fields. This ensures that the data is packed correctly and can be decoded reliably.
Also applies to: 22-25
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (2)
51-51:
Redundant check for zero address in zap function
The check if (token == address(0)) revert TokenZapV1__TokenZeroAddress(); may be redundant if the zap function is intended to support native gas tokens (e.g., ETH), which are represented by the constant NATIVE_GAS_TOKEN. Since NATIVE_GAS_TOKEN is set to a specific address, this check could prevent the use of native tokens.
Consider revising the check to allow token to be either an ERC20 address or NATIVE_GAS_TOKEN:
- if (token == address(0)) revert TokenZapV1__TokenZeroAddress();
+ if (token == address(0) && msg.value == 0) revert TokenZapV1__TokenZeroAddress();Alternatively, ensure that NATIVE_GAS_TOKEN is not equal to address(0) to differentiate it from an invalid token address.
Likely invalid or redundant comment.
87-91: Ensure _forwardToken handles zero balance scenarios safely
The _forwardToken function assumes that there is a positive balance of finalToken. If the balance is zero, it reverts with TokenZapV1__FinalTokenBalanceZero(). Consider whether reverting is the desired behavior when there is no balance to forward, or if it should silently succeed.
Run the following script to check for any calls to _forwardToken where the balance might be zero:
Verify that in all code paths leading to _forwardToken, the balance of finalToken is expected to be greater than zero, and that reverting on zero balance aligns with the intended contract logic.
packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (1)
66-72: Handle the default case explicitly in intent creation
In the previewIntent function, after handling Action.Swap, Action.AddLiquidity, and Action.RemoveLiquidity, the else block calls _createHandleHativeSteps. Ensure that this function correctly handles any other possible actions or defaults to a safe state.
Review the logic in _createHandleHativeSteps to confirm it appropriately handles unexpected actions or tokens.
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (2)
57-59: Ensure test helpers reflect new parameters
The test helper functions getZapDataDeposit and getZapDataDepositNoAmount have been updated to include finalToken and forwardTo parameters. Ensure that all calls to these functions in the test cases supply appropriate values to thoroughly test the new functionality.
Also applies to: 62-63
264-309: Ensure tests handle extra funds correctly
In the tests test_zap_unwrapForwardNativeEOA_placeholderZero_extraFunds and similar, extra tokens are transferred to tokenZap before the zap operation. Ensure that these tests assert not only the successful completion but also verify that the extra funds are handled as expected, preventing any unintended side effects.
Review the tests to confirm that:
- Extra funds do not cause unexpected behavior.
- The final balances of
tokenZap, the recipient, and any involved tokens are as intended.
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol (2)
11-12: LGTM! Well-documented parameter addition.
The documentation for the new forwardTo parameter is clear and comprehensive, including the important note about using address(0) when forwarding is not required.
21-21: Verify integration with existing implementations.
The parameter placement in the function signature is logical, but ensure all implementations of this interface are updated accordingly.
✅ Verification successful
Implementation is aligned with the interface
The implementation in SynapseIntentPreviewer.sol matches exactly with the interface definition in ISynapseIntentPreviewer.sol, including the forwardTo parameter placement. No integration issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all implementations of previewIntent to ensure they're updated
ast-grep --pattern 'function previewIntent(address $_, address $_, address $_, address $_, uint256 $_)'
Length of output: 105
Script:
#!/bin/bash
# Search for interface definition
echo "Interface definition:"
rg -l "ISynapseIntentPreviewer"
# Search for any implementations of previewIntent function
echo -e "\nImplementations:"
rg "function previewIntent" -A 5
Length of output: 1462
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (2)
16-17: LGTM! Consistent parameter addition.
The new parameters finalToken_ and forwardTo_ are correctly added to the encodeV1 function, maintaining consistency with the main library implementation.
Also applies to: 25-25
32-38: LGTM! Well-structured getter functions.
The new getter functions finalToken and forwardTo follow the established pattern and correctly delegate to the underlying library.
packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (3)
31-35: LGTM! ABI updated correctly.
The ABI has been properly updated to include the new forwardTo parameter with correct type and positioning.
88-92: LGTM! Appropriate error handling.
The new error type SIP__NoOpForwardNotSupported has been added to handle invalid forwarding scenarios.
2-2: Verify deployment transaction.
The contract has been redeployed with a new address. Ensure the deployment transaction was successful and the contract is verified on the Optimism block explorer.
Also applies to: 5-6
✅ Verification successful
Let me verify the contract deployment by checking the contract code at the specified address.
Contract deployment verified successfully
The deployment transaction (0x928a7db8741fb992934302f73e076f7630075151384529b538cb133e797c4bac) was successful with status "0x1", and the contract at address 0x9519E8D136d0a89d7e10D1a66C97249E0135544B contains the expected bytecode, confirming a successful deployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Get deployment block information
curl -s "https://api-optimistic.etherscan.io/api?module=proxy&action=eth_getTransactionReceipt&txhash=0x928a7db8741fb992934302f73e076f7630075151384529b538cb133e797c4bac"
Length of output: 1355
Script:
#!/bin/bash
# Get contract code at the specified address
curl -s "https://api-optimistic.etherscan.io/api?module=proxy&action=eth_getCode&address=0x9519E8D136d0a89d7e10D1a66C97249E0135544B&tag=latest"
Length of output: 15438
packages/contracts-rfq/test/libs/ZapDataV1.t.sol (3)
21-30: LGTM: Proper encoding of new parameters
The encodeZapData function correctly encodes the new finalToken and forwardTo parameters in the packed data structure.
53-64: LGTM: Comprehensive roundtrip testing
The test cases thoroughly verify the encoding and decoding of both finalToken and forwardTo parameters, including edge cases with zero addresses.
Also applies to: 81-90
114-126: LGTM: Proper error handling verification
The test cases properly verify error conditions for invalid encodings and parameter combinations.
packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol (1)
208-257: LGTM: Comprehensive test coverage for unspent funds
The new test cases thoroughly verify the handling of unspent funds in swap and unwrap operations, including scenarios with forwarding. The test coverage includes:
- Unspent ERC20 tokens
- Unspent WETH
- Extra funds scenarios
- Balance checks with forwarding
packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (2)
43-43: LGTM: Clean test setup and helper functions
The test setup and helper functions are well-organized with proper handling of the new forwardTo parameter. The separation between noForward and withForward test cases improves test clarity.
Also applies to: 107-141
Line range hint 364-399: LGTM: Proper validation of no-op intents with forwarding
The test cases correctly verify that forwarding is not supported for no-op intents, with proper error handling for both token and native currency scenarios.
| amountPosition: 4 + 32 * 2, | ||
| finalToken: address(0), | ||
| forwardTo: address(0) |
There was a problem hiding this comment.
Add test coverage for non-zero forwardTo and finalToken addresses
The test cases only cover scenarios where both finalToken and forwardTo are set to address(0). Given that the PR's main objective is to support forwarding assets to a specified address, we should add test coverage for:
- Non-zero
forwardToaddresses to verify the forwarding functionality - Different combinations of
finalTokenandforwardTovalues - Edge cases like forwarding to the same address as the user
Here's a suggested test structure to add:
function test_ForwardToSpecifiedAddress() public {
address forwardTo = makeAddr("ForwardTo");
bytes memory zapData = dstZap.encodeZapData({
target: address(dstVault),
payload: getDepositPayload(address(dstToken)),
amountPosition: 4 + 32 * 2,
finalToken: address(dstToken),
forwardTo: forwardTo
});
// Add assertions to verify tokens are forwarded correctly
}
function test_ForwardToZeroAddress() public {
// Test that tokens are sent to user when forwardTo is address(0)
}
function test_ForwardToWithDifferentFinalToken() public {
// Test forwarding with finalToken != depositToken
}Also applies to: 98-100, 106-108, 114-116, 121-123
| /// @param finalToken_ The token produced as a result of the Zap action (ERC20 or native gas token). | ||
| /// A zero address value signals that the Zap action doesn't result in any asset per se, | ||
| /// like bridging or depositing into a vault without an LP token. | ||
| /// Note: this parameter must be set to a non-zero value if the `forwardTo_` parameter is | ||
| /// set to a non-zero value. | ||
| /// @param forwardTo_ The address to which `finalToken` should be forwarded. This parameter is required only | ||
| /// if the Zap action does not automatically transfer the token to the intended recipient. | ||
| /// Otherwise, it must be set to address(0). |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure finalToken_ and forwardTo_ parameters are validated appropriately
The addition of finalToken_ and forwardTo_ parameters enhances the flexibility of the encodeV1 function. However, there should be explicit validations to ensure that if forwardTo_ is not the zero address, then finalToken_ must also be a non-zero address, as mentioned in the documentation. Consider adding a require statement to enforce this constraint.
Apply this diff to include the validation:
function encodeV1(
uint16 amountPosition_,
address finalToken_,
address forwardTo_,
address target_,
bytes memory payload_
)
internal
pure
returns (bytes memory encodedZapData)
{
if (target_ == address(0)) revert ZapDataV1__TargetZeroAddress();
+ if (forwardTo_ != address(0) && finalToken_ == address(0)) revert ZapDataV1__FinalTokenZeroAddress();
// Amount is encoded in [amountPosition_ .. amountPosition_ + 32), which should be within the payload.
if (amountPosition_ != AMOUNT_NOT_PRESENT && (uint256(amountPosition_) + 32 > payload_.length)) {
revert ZapDataV1__InvalidEncoding();
}
return abi.encodePacked(VERSION, amountPosition_, finalToken_, forwardTo_, target_, payload_);
}This ensures that the function reverts if forwardTo_ is set without a corresponding finalToken_, preventing potential runtime errors.
Also applies to: 66-67
| /// @param finalToken The token produced as a result of the Zap action (ERC20 or native gas token). | ||
| /// A zero address value signals that the Zap action doesn't result in any asset per se, | ||
| /// like bridging or depositing into a vault without an LP token. | ||
| /// Note: this parameter must be set to a non-zero value if the `forwardTo` parameter is | ||
| /// set to a non-zero value. | ||
| /// @param forwardTo The address to which `finalToken` should be forwarded. This parameter is required only | ||
| /// if the Zap action does not automatically transfer the token to the intended recipient. | ||
| /// Otherwise, it must be set to address(0). |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for finalToken and forwardTo in encodeZapData
While the documentation mentions that finalToken must be non-zero if forwardTo is set, the function does not enforce this constraint. Consider adding a require statement to ensure data integrity.
Apply this diff to include the validation:
function encodeZapData(
address target,
bytes memory payload,
uint256 amountPosition,
address finalToken,
address forwardTo
)
external
pure
returns (bytes memory)
{
if (payload.length > ZapDataV1.AMOUNT_NOT_PRESENT) {
revert TokenZapV1__PayloadLengthAboveMax();
}
+ // Ensure finalToken is non-zero if forwardTo is set
+ if (forwardTo != address(0) && finalToken == address(0)) {
+ revert TokenZapV1__FinalTokenZeroAddress();
+ }
// External integrations do not need to understand the specific `AMOUNT_NOT_PRESENT` semantics.
// Therefore, they can specify any value greater than or equal to `payload.length` to indicate
// that the amount is not present in the payload.
if (amountPosition >= payload.length) {
amountPosition = ZapDataV1.AMOUNT_NOT_PRESENT;
}
// At this point, we have checked that both `amountPosition` and `payload.length` fit in uint16.
return ZapDataV1.encodeV1({
amountPosition_: uint16(amountPosition),
finalToken_: finalToken,
forwardTo_: forwardTo,
target_: target,
payload_: payload
});
}This ensures that incorrect combinations of finalToken and forwardTo are prevented at the time of encoding.
Also applies to: 133-136
| error TokenZapV1__FinalTokenBalanceZero(); | ||
| error TokenZapV1__PayloadLengthAboveMax(); | ||
| error TokenZapV1__TargetZeroAddress(); | ||
| error TokenZapV1__TokenZeroAddress(); |
There was a problem hiding this comment.
Add missing error definition for TokenZapV1__FinalTokenZeroAddress
The TokenZapV1__FinalTokenZeroAddress() error is referenced in the contract but not defined. Ensure that this error is properly declared to maintain consistency and clarity in error handling.
Apply this diff to declare the missing error:
error TokenZapV1__FinalTokenBalanceZero();
error TokenZapV1__PayloadLengthAboveMax();
error TokenZapV1__TargetZeroAddress();
+ error TokenZapV1__FinalTokenZeroAddress();
error TokenZapV1__TokenZeroAddress();This addition will prevent potential compilation errors and improve error reporting.
📝 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.
| error TokenZapV1__FinalTokenBalanceZero(); | |
| error TokenZapV1__PayloadLengthAboveMax(); | |
| error TokenZapV1__TargetZeroAddress(); | |
| error TokenZapV1__TokenZeroAddress(); | |
| error TokenZapV1__FinalTokenBalanceZero(); | |
| error TokenZapV1__PayloadLengthAboveMax(); | |
| error TokenZapV1__TargetZeroAddress(); | |
| error TokenZapV1__FinalTokenZeroAddress(); | |
| error TokenZapV1__TokenZeroAddress(); |
* feat: scaffold IntentRouter * feat: initial implementation * test: single step * feat: scaffold checked/unchecked `completeIntent` * feat: implement balance checks * test: add cases where TokenZap has non-zero initial amount * test: double step cases (erc20 and/or native combos) * feat: SIR, TokenZap deployment script * deploy: test deploys for SIR * test: update to #3434 changes * feat: scaffold SIP * test: coverage for SIP * feat: Synapse Intent Previewer * feat: update deploy script, deploy SIP * fix(contracts-rfq): add `forwardTo` to ZapData for Zap Actions that don't forward assets to the user (#3451) * feat: scaffold finalToken, forwardTo in ZapDataV1 * feat: add finalToken, forwardTo * feat: scaffold TokenZap.encodeZapData * test: update existing tests * test: forwardTo scenarios * feat: token zeor address checks * feat:scaffold token forwarding * test: more revert cases * feat: final token forwarding * test: forwardTo behaviour in SIP * feat: support optional `forwardTo` in SIP * deploy: redeploy SIP, TokenZap
Description
This PR enhances
TokenZapV1andZapDataV1by adding an optional feature to transfer the proceeds of the Zap Action to the specifiedforwardToaddress. This is useful for creating Zap Actions for contracts that send the funds back tomsg.senderrather than the specified address, example beingIDefaultPoolpool contracts that are used as the source of liquidity on some of the chains.Summary by CodeRabbit
New Features
Bug Fixes
Tests