feat(contracts-rfq): Synapse Intent Router #3433
Conversation
WalkthroughThe pull request introduces several new files and functionalities related to the Synapse Intent Router. It adds an interface, Changes
Possibly related PRs
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol (2)
2-2: Consider updating the Solidity pragma for consistencyThe current Solidity pragma is
^0.8.4, while other files likeSynapseIntentRouter.solusepragma solidity 0.8.24. Aligning the Solidity version across all related contracts can prevent potential compilation issues and ensure compatibility.
18-18: Define the "SIR" acronym upon first useThe abbreviation "SIR" is used without prior definition. For clarity, consider defining "SIR" (Synapse Intent Router) when it is first mentioned in the documentation.
packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol (2)
19-19: Add documentation for theNATIVE_GAS_TOKENspecial addressIncluding a comment explaining that
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeEis used as a placeholder for the native gas token (e.g., ETH) can improve code readability and help other developers understand its purpose.
68-70: Optimize input validation usingrequirestatementsFor consistency and readability, consider using
requirestatements instead ofif...revertfor input validations:- if (block.timestamp > deadline) revert SIR__DeadlineExceeded(); - if (length == 0) revert SIR__StepsNotProvided(); + require(block.timestamp <= deadline, "SIR__DeadlineExceeded"); + require(length != 0, "SIR__StepsNotProvided");packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol (1)
2-2: Synchronize Solidity version pragma for consistencyThe Solidity pragma is set to
^0.8.4, whereas other contracts likeSynapseIntentRouter.solusepragma solidity 0.8.24. Aligning the Solidity versions across all contracts can prevent compilation issues and maintain consistency.packages/contracts-rfq/test/mocks/WETHMock.sol (2)
21-25: Clarify the use ofvm.dealin themintfunctionThe
mintfunction utilizesvm.dealto adjust the contract's balance for testing purposes. Sincevmis a testing utility provided by Foundry, consider adding a comment to emphasize that this function should not be used in production code.
22-23: Prevent potential overflow in balance calculationWhen calculating
newBalance, addingamounttoaddress(this).balancecould potentially overflow. Although unlikely in tests, consider adding a check to prevent overflow:uint256 newBalance = address(this).balance + amount; require(newBalance >= address(this).balance, "Overflow in newBalance calculation");packages/contracts-rfq/test/mocks/PoolMock.sol (3)
23-24: Enhance documentation clarityThe comment could be more explicit about why an empty test function prevents coverage reporting.
- /// @notice We include an empty "test" function so that this contract does not appear in the coverage report. + /// @notice Empty function with "test" prefix to exclude this mock contract from coverage reports, + /// as test files starting with "test" are typically excluded from coverage analysis.
26-28: Consider adding access controlWhile this is a mock contract, consider adding access control to prevent unauthorized ratio changes during tests.
+ address private owner; + + constructor(address token0_, address token1_) { + token0 = token0_; + token1 = token1_; + owner = msg.sender; + } + + modifier onlyOwner() { + require(msg.sender == owner, "PoolMock: caller is not the owner"); + _; + } - function setRatioWei(uint256 ratioWei_) external { + function setRatioWei(uint256 ratioWei_) external onlyOwner { ratioWei = ratioWei_; }
30-43: Add explicit overflow checks for ratio calculationsWhile Solidity 0.8+ includes overflow checks, consider adding explicit checks for better testing control.
function swap(uint256 amountIn, address tokenIn) external returns (uint256 amountOut) { address tokenOut; + unchecked { + // Prevent overflow in intermediate calculations + require(amountIn <= type(uint256).max / ratioWei, "PoolMock: amount too large"); + } if (tokenIn == token0) { tokenOut = token1; amountOut = amountIn * ratioWei / 1e18; } else if (tokenIn == token1) { tokenOut = token0; amountOut = amountIn * 1e18 / ratioWei; } else { revert PoolMock__TokenNotSupported(); } + require(amountOut > 0, "PoolMock: zero amount out"); IERC20(tokenIn).safeTransferFrom(msg.sender, address(this), amountIn); IERC20(tokenOut).safeTransfer(msg.sender, amountOut); }packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol (2)
31-62: Consider adding edge case testsWhile the current tests cover basic scenarios, consider adding tests for:
- Zero amount deposits
- Maximum uint256 amounts
- Amounts that could cause overflow in intermediate calculations
66-206: Document magic numbers and constantsConsider adding documentation for constants like
TOKEN_PRICE,FULL_BALANCE, andAMOUNTto explain their significance in the test scenarios.+ /// @notice Standard test amount, chosen to be large enough to test meaningful operations + /// but small enough to avoid overflow concerns uint256 constant AMOUNT = /* current value */; + + /// @notice Mock token price used for swap calculations uint256 constant TOKEN_PRICE = /* current value */; + + /// @notice Represents the full balance available for testing uint256 constant FULL_BALANCE = /* current value */;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol(1 hunks)packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/test/mocks/PoolMock.sol(1 hunks)packages/contracts-rfq/test/mocks/WETHMock.sol(2 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(1 hunks)
🔇 Additional comments (3)
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (1)
20-1035: Well-structured and comprehensive test suite
The test suite is extensive and covers multiple scenarios, ensuring that the SynapseIntentRouter functions correctly under various conditions. The use of helper functions and thorough checks contributes to the maintainability and reliability of the tests.
packages/contracts-rfq/test/mocks/PoolMock.sol (1)
1-15: LGTM! Well-structured contract declarations
Good use of SafeERC20 for secure token operations and appropriate use of immutable variables for token addresses.
packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol (1)
1-28: LGTM! Well-implemented test helper function
The completeUserIntent override correctly implements balance checks using completeIntentWithBalanceChecks and properly simulates user context with vm.prank.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3433 +/- ##
============================================
Coverage ? 98.91008%
============================================
Files ? 11
Lines ? 734
Branches ? 0
============================================
Hits ? 726
Misses ? 8
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol
Dismissed
Show dismissed
Hide dismissed
packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol
Dismissed
Show dismissed
Hide dismissed
Deploying sanguine-fe with
|
| Latest commit: |
e28e56e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8796b3b0.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-syn-intent-router.sanguine-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/contracts-rfq/script/DeploySIR.s.sol (2)
8-9: Consider adding documentation for contract name constants.While the constant names are clear, adding NatSpec documentation would help explain their purpose and relationship in the deployment process.
+ /// @notice Name of the Synapse Intent Router contract to be deployed string public constant LATEST_SIR = "SynapseIntentRouter"; + /// @notice Name of the Token Zap contract to be deployed string public constant LATEST_ZAP = "TokenZapV1";
11-12: Consider a more explicit test exclusion approach.While the empty test function works for excluding from coverage, consider using coverage ignore pragmas or configuration for better maintainability.
- /// @notice We include an empty "test" function so that this contract does not appear in the coverage report. - function testDeploySIR() external {} + // coverage-skip-start + /// @dev Mock test function for tooling compatibility + function testDeploySIR() external pure returns (bool) { + return true; + } + // coverage-skip-endpackages/contracts-rfq/deployments/optimism/TokenZapV1.json (2)
9-12: Consider adding explicit handling for receive functionThe contract includes a payable receive function but doesn't specify how received funds are handled. This could lead to locked funds if not properly managed.
Consider adding events and explicit handling for received funds, or document why the receive function is necessary.
156-170: Consolidate duplicate zero address errorsThere are two separate errors for zero address validation:
TokenZapV1__TargetZeroAddressZapDataV1__TargetZeroAddressThis duplication could lead to inconsistent error handling and confusion for users.
Consider consolidating these into a single error type and ensuring consistent usage throughout the contract.
packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json (1)
77-131: Consider refactoring to reduce code duplicationThe
completeIntentWithBalanceChecksfunction shares significant code withcompleteIntent. Consider implementing a private helper function for the common logic to improve maintainability and reduce the risk of divergent implementations.packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (1)
1-183: Consider upgrade strategy and integration testingAs this contract is part of the Synapse Intent Router system:
- Document the upgrade strategy for future versions
- Ensure comprehensive integration tests with other system components
- Consider implementing events for better transaction tracking and debugging
packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json (1)
132-209: Consider enhancing error informationWhile the error types are well-defined, some could benefit from additional context parameters:
SIR__AmountInsufficient: Add expected and actual amountsSIR__DeadlineExceeded: Add current time and deadlineSIR__UnspentFunds: Add token address and remaining amount
This would improve debugging and user experience.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json(1 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/optimism/TokenZapV1.json(1 hunks)packages/contracts-rfq/script/DeploySIR.s.sol(1 hunks)
🔇 Additional comments (13)
packages/contracts-rfq/script/DeploySIR.s.sol (2)
1-7: LGTM! Modern Solidity practices applied.
The contract uses:
- Latest Solidity version (0.8.24)
- Proper SPDX license
- Explicit imports
- Appropriate inheritance
14-18:
Critical: Verify constructor arguments and create2 salt implementation.
Several concerns need to be addressed:
- Empty constructor arguments might not be suitable if the contracts require initialization
- Missing create2 salts could lead to address collisions
- No post-deployment verification
Let's verify if these contracts require constructor arguments:
Consider adding:
- Constructor argument validation
- Deterministic create2 salt generation
- Post-deployment verification steps
- Documentation about expected deployed addresses
Example enhancement:
function run() external broadcastWithHooks {
- // TODO: create2 salts
- deployAndSave({contractName: LATEST_SIR, constructorArgs: "", deployCodeFunc: cbDeployCreate2});
- deployAndSave({contractName: LATEST_ZAP, constructorArgs: "", deployCodeFunc: cbDeployCreate2});
+ bytes32 sirSalt = _generateSalt(LATEST_SIR);
+ bytes32 zapSalt = _generateSalt(LATEST_ZAP);
+
+ address sir = deployAndSave({
+ contractName: LATEST_SIR,
+ constructorArgs: _encodeSIRConstructorArgs(),
+ salt: sirSalt,
+ deployCodeFunc: cbDeployCreate2
+ });
+
+ address zap = deployAndSave({
+ contractName: LATEST_ZAP,
+ constructorArgs: _encodeZapConstructorArgs(),
+ salt: zapSalt,
+ deployCodeFunc: cbDeployCreate2
+ });
+
+ _verifyDeployment(sir, zap);
}packages/contracts-rfq/deployments/optimism/TokenZapV1.json (2)
1-183: Verify security considerations for token handling and payload execution
The contract handles both tokens and native currency, and executes arbitrary payloads. Please ensure:
- All token operations use SafeERC20
- Payload execution is properly validated and sandboxed
- Reentrancy protection is in place
✅ Verification successful
Based on the examination of the TokenZapV1 contract source code, I can now provide a final response:
Security measures are properly implemented
The contract implements all necessary security measures:
- Uses SafeERC20 for token operations (
using SafeERC20 for IERC20) - Validates payload through
ZapDataV1.validateV1()before execution - Validates target address is non-zero (
if (target == address(0)) revert TokenZapV1__TargetZeroAddress()) - Uses Address library's safe call methods for execution
- Implements payload length validation in
encodeZapData
The contract is stateless by design and any leftover funds can be claimed by anyone, which is clearly documented. This is an intentional design choice rather than a security issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for security patterns in the codebase
# Check for reentrancy guards
echo "Checking for reentrancy protection..."
rg -l "nonReentrant|ReentrancyGuard"
# Check for SafeERC20 usage
echo "Checking for SafeERC20 usage..."
rg -l "SafeERC20"
# Check for validation patterns
echo "Checking for payload validation..."
rg -A 5 "function zap.*payable"
Length of output: 13093
Script:
#!/bin/bash
# Let's examine the TokenZapV1 contract implementation for security measures
# Check the full TokenZapV1 contract implementation
echo "Checking TokenZapV1 implementation..."
ast-grep --pattern 'contract TokenZapV1 {
$$$
}'
# Check for validation functions
echo "Checking validation functions..."
rg -A 10 "validateV1"
# Check for token transfer patterns
echo "Checking token transfer patterns..."
rg -A 5 "transfer.*\(.*token"
Length of output: 92472
Script:
#!/bin/bash
# Let's examine the TokenZapV1 contract source code
echo "Checking TokenZapV1 source code..."
fd -t f "TokenZapV1.sol" -x cat {}
# Check for UniversalToken library usage (which often includes SafeERC20)
echo "Checking UniversalToken usage..."
rg -A 5 "UniversalToken"
# Check for payload size validation
echo "Checking payload validation..."
rg -A 5 "TokenZapV1__PayloadLengthAboveMax"
Length of output: 114444
1-7: Verify contract deployment on Optimism block explorer
The contract has been deployed but should be verified on the block explorer for transparency and auditability.
packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json (4)
8-21: LGTM: Native gas token view function
The NATIVE_GAS_TOKEN function is correctly implemented as a view function returning the native token address.
132-209: LGTM: Well-structured error definitions
The error definitions are comprehensive and follow best practices:
- Clear prefix convention with "SIR__"
- Good coverage of failure scenarios
- Proper integration of OpenZeppelin standard errors
22-76: Consider adding maximum steps limit
The completeIntent function accepts an unbounded array of steps, which could potentially lead to DOS attacks through excessive gas consumption. Consider adding a maximum limit to the number of steps allowed.
1-7: Verify contract deployment on Arbiscan
The contract is deployed on Arbitrum but should be verified on Arbiscan for transparency and auditability. Also, confirm if the empty constructor arguments ("0x") are intentional.
✅ Verification successful
Based on the code review from the test files and contract implementation shown earlier, I can now generate the final response:
Empty constructor arguments are correct for this contract
The empty constructor arguments ("0x") are intentional as the contract doesn't require initialization. The contract implementation shows:
- No constructor parameters are needed
- No initialization function exists
- Only immutable state variables (NATIVE_GAS_TOKEN constant and internal FULL_BALANCE constant)
- All functionality is self-contained in the contract implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the contract is verified on Arbiscan
# Note: Using curl instead of direct Arbiscan API to avoid API key requirements
curl -s "https://api.arbiscan.io/api/v1/contract/$address/info" | jq '.result[0].ABI' | grep -q "null" && echo "Contract not verified" || echo "Contract verified"
Length of output: 195
Script:
#!/bin/bash
# Let's check if there are any constructor arguments defined in the source code
fd "SynapseIntentRouter" --type f --extension sol --exec cat {}
# Also check for any initialization functions
ast-grep --pattern 'function initialize($$$) { $$$ }'
Length of output: 62862
Script:
#!/bin/bash
# Let's check the actual contract implementation to verify constructor
ast-grep --pattern 'contract SynapseIntentRouter $$$'
Length of output: 55
Script:
#!/bin/bash
# Let's check the contract implementation with a more specific pattern
ast-grep --pattern 'contract SynapseIntentRouter is ISynapseIntentRouter, ISynapseIntentRouterErrors {
$$$
}'
Length of output: 113
packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (3)
113-181: LGTM: Well-defined error handling
The error definitions are comprehensive and well-structured:
- Standard OpenZeppelin errors for address and ERC20 operations
- Custom errors for zap-specific scenarios
- Clear naming conventions with proper namespacing
1-7: Verify contract deployment and source code verification
Please ensure the contract is verified on Arbiscan for transparency and auditability. The empty constructor arguments ("0x") should be validated.
✅ Verification successful
Contract deployment verification successful
The contract is verified on Arbiscan (status: 1) and the empty constructor arguments ("0x") are correct as the contract has no explicit constructor. The contract inherits from IZapRecipient and uses the default constructor. The deployment information is valid and complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and source code verification status
# Check if contract is verified on Arbiscan
curl -s "https://api.arbiscan.io/api?module=contract&action=getabi&address=0xcae5baD754dC99cDA1d58415C5eeeb92e1279F69" | jq -r '.status'
Length of output: 138
Script:
#!/bin/bash
# Let's verify the constructor arguments by checking the contract source code
# First, find the contract source file
fd "TokenZapV1.sol" --type f
# Then check if there's a constructor defined in the contract
if [ -f "packages/contracts-rfq/contracts/TokenZapV1.sol" ]; then
ast-grep --pattern 'constructor($$$) {
$$$
}' packages/contracts-rfq/contracts/TokenZapV1.sol
fi
Length of output: 142
Script:
#!/bin/bash
# Let's check the actual contract source code for constructor
cat packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
Length of output: 8886
8-112: Review native token handling security
The contract interface includes native token handling capabilities:
- Payable
receive()function NATIVE_GAS_TOKEN()view function- Payable
zap()function
Ensure proper native token handling is implemented in the contract to prevent locked funds.
✅ Verification successful
Based on the implementation in TokenZapV1.sol, I can now generate the final response:
Native token handling is properly implemented
The contract has secure native token handling mechanisms:
- Payable
receive()function with clear documentation about leftover funds - Explicit handling of native tokens in
zap()function usingAddress.sendValueandAddress.functionCallWithValue - Clear documentation warning users about sending exact amounts to avoid leftover funds
- Balance checks performed within the OpenZeppelin's Address library functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for native token handling patterns
# Look for native token handling patterns in the codebase
ast-grep --pattern 'function withdraw'
ast-grep --pattern 'msg.value'
Length of output: 71
Script:
#!/bin/bash
# Search for the contract implementation file
fd -e sol TokenZapV1
# Search for native token handling patterns in Solidity files
rg -U "receive\(\).*payable" --type sol
rg -U "function\s+withdraw" --type sol
rg "msg\.value" --type sol -A 3
Length of output: 590
Script:
#!/bin/bash
# Check the implementation file content
cat packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
# Search for native token handling patterns without file type restriction
rg -U "receive\(\).*payable"
rg -U "function\s+withdraw"
rg "msg\.value" -A 3
Length of output: 76098
packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json (2)
1-7: Verify contract deployment on Optimism
Let's verify the contract deployment and initialization state.
✅ Verification successful
Contract deployment verified successfully on Optimism
The verification results show:
- Contract is deployed and has valid bytecode at 0x57203c65DeA2ded4EE4E303a9494bee04df030BF
- Deployment transaction (0xf68cf0c65d39291cf7b293228ae1664ca8fb0b2afb32e6ed1ecbac80a38f4771) was successful with status 0x1
- Empty constructor args are valid as the contract initialization is handled through function calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and initialization on Optimism
# Check if the contract is deployed and verify its code size
curl -s -X POST --data '{
"jsonrpc":"2.0",
"method":"eth_getCode",
"params":["0x57203c65DeA2ded4EE4E303a9494bee04df030BF", "latest"],
"id":1
}' https://mainnet.optimism.io | jq .result
# Verify transaction receipt
curl -s -X POST --data '{
"jsonrpc":"2.0",
"method":"eth_getTransactionReceipt",
"params":["0xf68cf0c65d39291cf7b293228ae1664ca8fb0b2afb32e6ed1ecbac80a38f4771"],
"id":1
}' https://mainnet.optimism.io | jq .result
Length of output: 9737
8-131: Consider adding events for important state changes
The contract interface lacks events for tracking important operations. Consider adding events to log:
- Intent completion status
- Step execution details
- Balance check results
This would improve transparency and make it easier to track contract activity off-chain.
Verify access control implementation
The contract interface shows payable functions without visible access control modifiers. Please ensure proper access controls are implemented in the contract code.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (1)
996-1034: RefactortoArrayfunctions to reduce duplicationThe
toArrayfunctions for creating arrays with 1, 2, or 3StepParamsinstances can be refactored using a single function that accepts a dynamic array. This will enhance maintainability and reduce code duplication.Consider using variable-length arrays as follows:
- function toArray(ISynapseIntentRouter.StepParams memory a) internal pure returns (ISynapseIntentRouter.StepParams[] memory arr) { - arr = new ISynapseIntentRouter.StepParams[](1); - arr[0] = a; - return arr; - } - - function toArray(ISynapseIntentRouter.StepParams memory a, ISynapseIntentRouter.StepParams memory b) internal pure returns (ISynapseIntentRouter.StepParams[] memory arr) { - arr = new ISynapseIntentRouter.StepParams[](2); - arr[0] = a; - arr[1] = b; - return arr; - } - - function toArray(ISynapseIntentRouter.StepParams memory a, ISynapseIntentRouter.StepParams memory b, ISynapseIntentRouter.StepParams memory c) internal pure returns (ISynapseIntentRouter.StepParams[] memory arr) { - arr = new ISynapseIntentRouter.StepParams[](3); - arr[0] = a; - arr[1] = b; - arr[2] = c; - return arr; - } + function toArray(ISynapseIntentRouter.StepParams[] memory steps) internal pure returns (ISynapseIntentRouter.StepParams[] memory) { + return steps; + }Usage example:
- steps = toArray(step1, step2); + steps = toArray(new ISynapseIntentRouter.StepParams[](2)); + steps[0] = step1; + steps[1] = step2;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(1 hunks)
🔇 Additional comments (1)
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (1)
37-42: Ensure withExtraFunds modifier handles token balances correctly
In the withExtraFunds modifier, extra funds are minted and dealt to tokenZap. Verify that this handling is appropriate for the test scenarios and that tokenZap correctly manages these balances without side effects.
Run the following script to check the token balances of tokenZap:
✅ Verification successful
The withExtraFunds modifier correctly handles token balances
The code analysis shows that the withExtraFunds modifier is properly implemented and thoroughly tested:
- It correctly mints extra ERC20 and WETH tokens to the tokenZap contract
- It properly deals native ETH to the tokenZap address
- The test suite includes comprehensive test cases that verify:
- Extra funds have no effect when using exact amounts
- Extra funds are correctly used with "full balance" instructions
- Balance checks properly revert when extra funds remain unspent
The modifier's behavior is consistent with its intended purpose of testing scenarios with additional funds.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that tokenZap has the correct extra funds.
# Check ERC20 balance
cast call $(cat address.txt) "balanceOf(address)(uint256)" $(cat tokenZapAddress.txt)
# Check WETH balance
cast call $(cat wethAddress.txt) "balanceOf(address)(uint256)" $(cat tokenZapAddress.txt)
Length of output: 565
Script:
#!/bin/bash
# Let's analyze the test file and related code to understand the token balance handling
# First, let's see the full test file content
cat packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol
# Find any other occurrences of withExtraFunds in the codebase
rg "withExtraFunds" -A 5 -B 5
# Look for tokenZap usage and initialization
rg "tokenZap" -A 5 -B 5
Length of output: 73954
| function checkRevertAmountInsufficient( | ||
| uint256 msgValue, | ||
| uint256 lastStepAmountIn, | ||
| ISynapseIntentRouter.StepParams[] memory steps | ||
| ) | ||
| public | ||
| { | ||
| vm.expectRevert(SIR__AmountInsufficient.selector); | ||
| completeUserIntent({ | ||
| msgValue: msgValue, | ||
| amountIn: AMOUNT, | ||
| minLastStepAmountIn: lastStepAmountIn + 1, | ||
| deadline: block.timestamp, | ||
| steps: steps | ||
| }); | ||
| } |
There was a problem hiding this comment.
Potential off-by-one error in checkRevertAmountInsufficient usage
The checkRevertAmountInsufficient function adds 1 to lastStepAmountIn when calling completeUserIntent, but the test functions already increment lastStepAmountIn before passing it. This double increment results in testing with a minLastStepAmountIn that is two units higher than intended, potentially causing incorrect test outcomes.
Apply this diff to correct the lastStepAmountIn parameter in the test functions:
- checkRevertAmountInsufficient({msgValue: 0, lastStepAmountIn: AMOUNT + 1, steps: steps});
+ checkRevertAmountInsufficient({msgValue: 0, lastStepAmountIn: AMOUNT, steps: steps});Make similar adjustments in all affected test functions that call checkRevertAmountInsufficient.
Also applies to: 270-273, 312-315, 358-361, 405-408, 473-476, 520-523, 558-561, 605-608, 668-671, 711-714, 751-753, 798-801, 864-867, 908-910, 942-944, 984-987
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
packages/contracts-rfq/test/mocks/DefaultPoolMock.sol (3)
10-11: Consider using coverage ignore pragmas instead of empty test function.Instead of using an empty test function to exclude from coverage reports, consider using standard coverage ignore pragmas:
- /// @notice We include an empty "test" function so that this contract does not appear in the coverage report. - function testDefaultPoolMock() external {} + /// @custom:coverage ignore-start + /// @custom:coverage ignore-end
34-46: Add documentation to swapStorage function.Add natspec documentation for testing clarity:
+ /// @notice Mock implementation to return swap pool parameters + /// @return initialA Initial amplification coefficient + /// @return futureA Future amplification coefficient + /// @return initialATime Time when ramp up/down starts + /// @return futureATime Time when ramp up/down ends + /// @return swapFee Swap fee percentage + /// @return adminFee Admin fee percentage + /// @return lpToken Address of LP token function swapStorage() external view returns (
48-54: Add documentation to getToken function.Add natspec documentation for testing clarity:
+ /// @notice Returns mock token address for given index + /// @dev Addresses will be overridden by vm.mockCall in tests + /// @param index Token index (must be less than TOKENS) + /// @return token Mock token address function getToken(uint8 index) external pure returns (address token) {packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (1)
116-116: Enhance code readability by replacing magic numbers with constantsIn the calculations for
amountPosition_, magic numbers like4 + 32 * 2are used. Consider defining named constants for these offsets to improve code readability and maintainability.Also applies to: 160-160, 275-275
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol (1)
2-2: Ensure consistent Solidity pragma version between interface and implementationThe interface uses
pragma solidity ^0.8.4, while the implementation usespragma solidity 0.8.24. For consistency and to avoid potential compatibility issues, consider aligning the Solidity versions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol(1 hunks)packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol(1 hunks)packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol(1 hunks)packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol(1 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(1 hunks)packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol(1 hunks)packages/contracts-rfq/test/mocks/DefaultPoolMock.sol(1 hunks)packages/contracts-rfq/test/mocks/SwapQuoterMock.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultPool.sol
- packages/contracts-rfq/contracts/legacy/router/interfaces/IWETH9.sol
- packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultExtendedPool.sol
- packages/contracts-rfq/test/mocks/SwapQuoterMock.sol
🔇 Additional comments (2)
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (1)
7-8: LGTM! Well-structured test harness constants.
The addition of these public constants provides good test accessibility to the internal values from ZapDataV1, which will be useful for validating the Synapse Intent Router's handling of Zap operations.
packages/contracts-rfq/test/mocks/DefaultPoolMock.sol (1)
1-8: LGTM! Contract setup follows best practices.
The contract setup is well-structured with appropriate license, solidity version, and interface import.
| function calculateSwap( | ||
| uint8 tokenIndexFrom, | ||
| uint8 tokenIndexTo, | ||
| uint256 dx | ||
| ) | ||
| external | ||
| view | ||
| returns (uint256 amountOut) | ||
| {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation to calculateSwap function.
Similar to swap function, add parameter validation and documentation:
+ /// @notice Mock implementation of swap calculation
+ /// @param tokenIndexFrom Index of token to swap from
+ /// @param tokenIndexTo Index of token to swap to
+ /// @param dx Amount of tokens to swap
+ /// @return amountOut Expected amount of tokens to receive
function calculateSwap(
uint8 tokenIndexFrom,
uint8 tokenIndexTo,
uint256 dx
)
external
view
returns (uint256 amountOut)
- {}
+ {
+ require(tokenIndexFrom < TOKENS, "Invalid input token");
+ require(tokenIndexTo < TOKENS, "Invalid output token");
+ require(tokenIndexFrom != tokenIndexTo, "Cannot swap same token");
+ }📝 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.
| function calculateSwap( | |
| uint8 tokenIndexFrom, | |
| uint8 tokenIndexTo, | |
| uint256 dx | |
| ) | |
| external | |
| view | |
| returns (uint256 amountOut) | |
| {} | |
| /// @notice Mock implementation of swap calculation | |
| /// @param tokenIndexFrom Index of token to swap from | |
| /// @param tokenIndexTo Index of token to swap to | |
| /// @param dx Amount of tokens to swap | |
| /// @return amountOut Expected amount of tokens to receive | |
| function calculateSwap( | |
| uint8 tokenIndexFrom, | |
| uint8 tokenIndexTo, | |
| uint256 dx | |
| ) | |
| external | |
| view | |
| returns (uint256 amountOut) | |
| { | |
| require(tokenIndexFrom < TOKENS, "Invalid input token"); | |
| require(tokenIndexTo < TOKENS, "Invalid output token"); | |
| require(tokenIndexFrom != tokenIndexTo, "Cannot swap same token"); | |
| } |
| function swap( | ||
| uint8 tokenIndexFrom, | ||
| uint8 tokenIndexTo, | ||
| uint256 dx, | ||
| uint256 minDy, | ||
| uint256 deadline | ||
| ) | ||
| external | ||
| returns (uint256 amountOut) | ||
| {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation to swap function.
The function should validate token indices and include natspec documentation for testing clarity:
+ /// @notice Mock implementation of token swap
+ /// @param tokenIndexFrom Index of token to swap from
+ /// @param tokenIndexTo Index of token to swap to
+ /// @param dx Amount of tokens to swap
+ /// @param minDy Minimum amount of tokens to receive
+ /// @param deadline Timestamp after which swap is invalid
+ /// @return amountOut Amount of tokens received
function swap(
uint8 tokenIndexFrom,
uint8 tokenIndexTo,
uint256 dx,
uint256 minDy,
uint256 deadline
)
external
returns (uint256 amountOut)
- {}
+ {
+ require(tokenIndexFrom < TOKENS, "Invalid input token");
+ require(tokenIndexTo < TOKENS, "Invalid output token");
+ require(tokenIndexFrom != tokenIndexTo, "Cannot swap same token");
+ require(block.timestamp <= deadline, "Deadline expired");
+ }📝 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.
| function swap( | |
| uint8 tokenIndexFrom, | |
| uint8 tokenIndexTo, | |
| uint256 dx, | |
| uint256 minDy, | |
| uint256 deadline | |
| ) | |
| external | |
| returns (uint256 amountOut) | |
| {} | |
| /// @notice Mock implementation of token swap | |
| /// @param tokenIndexFrom Index of token to swap from | |
| /// @param tokenIndexTo Index of token to swap to | |
| /// @param dx Amount of tokens to swap | |
| /// @param minDy Minimum amount of tokens to receive | |
| /// @param deadline Timestamp after which swap is invalid | |
| /// @return amountOut Amount of tokens received | |
| function swap( | |
| uint8 tokenIndexFrom, | |
| uint8 tokenIndexTo, | |
| uint256 dx, | |
| uint256 minDy, | |
| uint256 deadline | |
| ) | |
| external | |
| returns (uint256 amountOut) | |
| { | |
| require(tokenIndexFrom < TOKENS, "Invalid input token"); | |
| require(tokenIndexTo < TOKENS, "Invalid output token"); | |
| require(tokenIndexFrom != tokenIndexTo, "Cannot swap same token"); | |
| require(block.timestamp <= deadline, "Deadline expired"); | |
| } |
| } else if (params.action == Action.RemoveLiquidity) { | ||
| steps = _createRemoveLiquiditySteps(tokenIn, tokenOut, params); | ||
| } else { | ||
| steps = _createHandleHativeSteps(tokenIn, tokenOut, amountIn); |
There was a problem hiding this comment.
Fix typo in function name: _createHandleHativeSteps should be _createHandleNativeSteps
There is a typo in the function name _createHandleHativeSteps. It should be _createHandleNativeSteps to correctly reflect that it handles native tokens.
Apply the following diff to correct the function name:
-function _createHandleHativeSteps(
+function _createHandleNativeSteps(And update the function call accordingly:
-steps = _createHandleHativeSteps(tokenIn, tokenOut, amountIn);
+steps = _createHandleNativeSteps(tokenIn, tokenOut, amountIn);Also applies to: 205-205
| function _verifyLpToken(address pool, address token) internal view { | ||
| (,,,,,, address lpToken) = IDefaultExtendedPool(pool).swapStorage(); | ||
| if (lpToken != token) revert SIP__PoolTokenMismatch(); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/contracts-rfq/script/DeploySIR.s.sol (2)
8-10: Add documentation about deployment dependenciesConsider adding NatSpec documentation to explain:
- The relationship between these contracts
- Any required deployment order
- Why these specific contract implementations were chosen
+ /// @notice The main Synapse Intent Router contract that handles intent execution string public constant LATEST_SIR = "SynapseIntentRouter"; + /// @notice Preview contract that simulates intent execution string public constant LATEST_SIP = "SynapseIntentPreviewer"; + /// @notice Token Zap implementation used by the router for token operations string public constant LATEST_ZAP = "TokenZapV1";
12-13: Consider proper test coverage configurationInstead of using an empty test function to exclude from coverage, consider:
- Using coverage configuration files to exclude deployment scripts
- Or adding the
@devtag to mark this as a development contractpackages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (1)
83-112: Consider adding parameter validation errorsThe current error set could be enhanced with additional validation errors for the
previewIntentfunction parameters:
SIP__InvalidSwapQuoterSIP__InvalidAmountSIP__TokensIdentical
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json(1 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json(1 hunks)packages/contracts-rfq/script/DeploySIR.s.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json
🔇 Additional comments (5)
packages/contracts-rfq/script/DeploySIR.s.sol (2)
1-4: LGTM! Modern Solidity version choice.
Using Solidity 0.8.24 is a good choice as it includes the latest security features and optimizations.
15-20:
Critical deployment configuration issues need attention
Several concerns with the deployment implementation:
- Empty constructor arguments (
"") might cause initialization issues if these contracts require parameters - The TODO for create2 salts needs to be addressed as it affects address predictability
- No validation of successful deployment
Let's verify if these contracts require constructor parameters:
Consider these improvements:
- Add deployment validation checks
- Implement proper error handling
- Add events for deployment tracking
- Document the expected create2 addresses once salts are implemented
Would you like assistance in implementing these improvements?
packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (3)
9-21: LGTM! Function is properly defined
The NATIVE_GAS_TOKEN view function is correctly specified with appropriate return type and visibility.
1-7: Verify contract on Optimism block explorer
The contract has been deployed successfully, but it's important to ensure it's verified on the Optimism block explorer for transparency and auditability.
✅ Verification successful
Contract is verified on Optimism block explorer
The contract at address 0xE184826D4aBC2798134abE7e2Fd72156827Fc7EA is successfully verified on the Optimism block explorer. The API response status of "1" confirms the verification status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the contract is verified on Optimism block explorer
# Note: Using curl instead of direct etherscan API as it's a deployment artifact check
curl -s "https://api-optimistic.etherscan.io/api?module=contract&action=getabi&address=0xE184826D4aBC2798134abE7e2Fd72156827Fc7EA" | jq -r '.status'
Length of output: 150
22-82: Verify swapQuoter address validation
The previewIntent function accepts a swapQuoter address parameter which could pose security risks if not properly validated. Ensure that:
- The implementation validates the
swapQuoteraddress against a whitelist - The
msgValuein StepParams is properly bounded to prevent excessive value transfers
…on'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
| 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 _createAddLiquiditySteps( | ||
| address tokenIn, | ||
| address tokenOut, | ||
| DefaultParams memory params, | ||
| address forwardTo | ||
| ) | ||
| internal | ||
| view | ||
| returns (ISynapseIntentRouter.StepParams[] memory steps) | ||
| { | ||
| address pool = params.pool; | ||
| if (pool == address(0)) revert SIP__PoolZeroAddress(); | ||
| // Sanity check tokenIn vs tokenIndexFrom. | ||
| if (IDefaultPool(pool).getToken(params.tokenIndexFrom) != tokenIn) revert SIP__PoolTokenMismatch(); | ||
| // Sanity check tokenOut vs pool's LP token. | ||
| _verifyLpToken(pool, tokenOut); | ||
| // Figure out how many tokens does the pool support. | ||
| uint256[] memory amounts; | ||
| for (uint8 i = 0;; i++) { | ||
| // solhint-disable-next-line no-empty-blocks | ||
| try IDefaultExtendedPool(pool).getToken(i) returns (address) { | ||
| // Token exists, continue. | ||
| } catch { | ||
| // No more tokens, allocate the array using the correct size. | ||
| amounts = new uint256[](i); | ||
| break; | ||
| } | ||
| } | ||
| return _toStepsArray( | ||
| ISynapseIntentRouter.StepParams({ | ||
| token: tokenIn, | ||
| amount: FULL_BALANCE, | ||
| msgValue: 0, | ||
| zapData: ZapDataV1.encodeV1({ | ||
| target_: pool, | ||
| finalToken_: tokenOut, | ||
| forwardTo_: forwardTo, | ||
| // addLiquidity(amounts, minToMint, deadline) | ||
| payload_: abi.encodeCall(IDefaultExtendedPool.addLiquidity, (amounts, 0, type(uint256).max)), | ||
| // amountIn is encoded within `amounts` at `TOKEN_IN_INDEX`, `amounts` is encoded after | ||
| // (amounts.offset, minToMint, deadline, amounts.length). | ||
| amountPosition_: 4 + 32 * 4 + 32 * uint16(params.tokenIndexFrom) | ||
| }) | ||
| }) | ||
| ); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| 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
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (18)
packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol (10)
43-45: Consider refactoring to reduce code duplicationThe function
test_depositERC20_exactAmount_extraFunds_revert_unspentERC20callstest_depositERC20_exactAmount_revert_unspentERC20but adds thewithExtraFundsmodifier. To improve maintainability and reduce duplication, consider parameterizing the tests or consolidating them into a single function with varying inputs.
60-62: Reduce redundancy in test functionsSimilar to the previous suggestion,
test_depositNative_exactAmount_extraFunds_revert_unspentNativecallstest_depositNative_exactAmount_revert_unspentNativewith thewithExtraFundsmodifier. Refactoring these tests can enhance readability and maintainability.
93-95: Refactor repeated test patternsThe test function
test_swapDepositERC20_exactAmounts_extraFunds_revert_unspentERC20duplicates the call totest_swapDepositERC20_exactAmounts_revert_unspentERC20with an additional modifier. Consolidating these tests could simplify the codebase.
96-98: Streamline test implementations
test_swapDepositERC20_exactAmounts_extraFunds_revert_unspentWETHrepeats the pattern of calling another test with thewithExtraFundsmodifier. Consider combining these tests or using a parameterized approach to minimize repetition.
139-142: Consolidate repetitive tests for clarityThe function
test_wrapDepositWETH_exactAmounts_extraFunds_revert_unspentNativemirrorstest_wrapDepositWETH_exactAmounts_revert_unspentNativewith an added modifier. Refactoring can improve code clarity and reduce duplication.
143-145: Optimize test structure to reduce duplicationSimilarly,
test_wrapDepositWETH_exactAmounts_extraFunds_revert_unspentWETHduplicates another test function's logic. Streamlining these tests can enhance maintainability.
184-187: Avoid redundant test functionsThe function
test_unwrapDepositNative_exactAmounts_extraFunds_revert_unspentWETHcallstest_unwrapDepositNative_exactAmounts_revert_unspentWETHwith thewithExtraFundsmodifier. Refactoring these could reduce redundancy.
188-190: Simplify repetitive test cases
test_unwrapDepositNative_exactAmounts_extraFunds_revert_unspentNativefollows the same pattern of calling another test with an additional modifier. Consolidating these tests can improve the test suite's organization.
234-237: Enhance test suite by reducing repetitionThe test
test_swapUnwrapForwardNative_exactAmounts_extraFunds_revert_unspentERC20repeats another test's logic withwithExtraFunds. Refactoring can make the test suite more concise.
238-240: Refactor similar tests for maintainability
test_swapUnwrapForwardNative_exactAmounts_extraFunds_revert_unspentWETHduplicates existing test logic. Consider refactoring to improve code maintainability.packages/contracts-rfq/contracts/libs/ZapDataV1.sol (1)
Line range hint
69-80: Add validation forfinalToken_whenforwardTo_is setWhile the documentation specifies that
finalToken_must be a non-zero address whenforwardTo_is non-zero, theencodeV1function does not enforce this constraint. To prevent misuse and ensure data integrity, please add a validation check withinencodeV1to enforce this requirement.Apply the following diff to add the necessary 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(); + // Final token needs to be specified if forwarding is required. + 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_); }packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (1)
212-215: Address unused return value in_verifyLpTokenThe
_verifyLpTokenfunction ignores the returned values fromswapStorage(), except forlpToken. This might trigger static analysis warnings about unused return values.If the other returned values are not needed, consider using an explicit variable to ignore them:
(, , , , , , address lpToken) = IDefaultExtendedPool(pool).swapStorage();Alternatively, you can suppress the warning by acknowledging the unused variables.
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (2)
172-174: Potential issue with deprecatedboundfunctionIn the test
test_zap_native_msgValueLowerThanAmount_extraNative, theboundfunction is used:amountPosition = bound(amountPosition, payload.length, type(uint256).max);Ensure that
boundis the appropriate function for your testing framework. If it's deprecated, consider usingvm.assumeor other assertion methods provided by Forge.
557-567: Use consistent error handling for zero address checksIn the test
test_zap_wrapForward_revert_zeroFinalToken, the errorTokenZapV1.TokenZapV1__TokenZeroAddressis expected whenfinalTokenis zero. Verify that all tests and functions consistently use this error for zero address validations to maintain uniform error handling across the codebase.packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol (1)
6-29: LGTM! Well-structured interface with comprehensive documentation.The interface is well-designed with clear NatSpec documentation and proper error handling strategy. The view function is gas-efficient and the parameter naming is descriptive.
Consider:
- Adding events to track preview requests for better observability
- Including a version number (e.g.,
ISynapseIntentPreviewerV1) to support future upgradespackages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json (1)
23-87: Consider adding input validation for previewIntentThe
previewIntentfunction takes multiple address parameters but lacks explicit validation. While some validations might be handled internally, consider adding explicit checks for:
- Zero addresses for
swapQuoter,forwardTo,tokenIn, andtokenOut- Token compatibility between
tokenInandtokenOutpackages/contracts-rfq/test/libs/ZapDataV1.t.sol (2)
34-35: Enhance test coverage for edge casesWhile the basic roundtrip tests are well implemented, consider adding specific test cases for:
- Address collision scenarios (when finalToken equals forwardTo)
- Invalid address patterns
- Gas optimization scenarios with repeated addresses
+function test_roundtrip_withAmount_addressCollision( + address finalToken, + address target, + uint256 amount, + bytes memory prefix, + bytes memory postfix +) public view { + test_roundtrip_withAmount( + finalToken, + finalToken, // Same address as finalToken + target, + amount, + prefix, + postfix + ); +}Also applies to: 57-58, 68-73, 85-86
174-175: Verify minimum valid ZapData length calculationThe test correctly validates the minimum length, but consider adding a constant for better maintainability.
+ // Minimum length = version(2) + amountPosition(2) + finalToken(20) + forwardTo(20) + target(20) + uint256 constant MINIMUM_ZAPDATA_LENGTH = 64; + bytes memory minimumValidZapData = encodeZapData(EXPECTED_VERSION, type(uint16).max, address(0), address(0), address(0), ""); + assertEq(minimumValidZapData.length, MINIMUM_ZAPDATA_LENGTH);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol(1 hunks)packages/contracts-rfq/contracts/libs/ZapDataV1.sol(4 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(1 hunks)packages/contracts-rfq/contracts/zaps/TokenZapV1.sol(6 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json(1 hunks)packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json(1 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json(1 hunks)packages/contracts-rfq/deployments/optimism/TokenZapV1.json(1 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(1 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(1 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol(1 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.t.sol(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol
🔇 Additional comments (18)
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (3)
7-8: LGTM! Constants are properly defined.
The constants are correctly imported from the ZapDataV1 library, ensuring consistency between the test harness and the actual implementation.
32-38: LGTM! Getter functions follow established patterns.
The new getter functions finalToken and forwardTo are implemented consistently with the existing getters, properly delegating to the ZapDataV1 library.
15-25: Verify consistency with ZapDataV1 library implementation.
The function signature has been updated correctly with the new parameters. Let's verify the consistency with the library implementation.
✅ Verification successful
Parameter signature and types match perfectly between harness and library
The verification shows that the encodeV1 function in the harness matches exactly with the library implementation in terms of:
- Parameter order: amountPosition_, finalToken_, forwardTo_, target_, payload_
- Parameter types: uint16, address, address, address, bytes memory
- Return type: bytes memory
The only difference is the visibility modifier (public vs internal) which is appropriate as the harness is meant to expose the library's functionality for testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the ZapDataV1 library's encodeV1 function signature matches this harness
ast-grep --pattern 'function encodeV1(
uint16 $_,
address $_,
address $_,
address $_,
bytes memory $_
) $$$'
Length of output: 120
Script:
#!/bin/bash
# Let's try a broader search to find the ZapDataV1 library implementation
rg -l "library ZapDataV1"
# Then let's look at the content of the file containing the library
rg -A 10 "function encodeV1"
Length of output: 1842
packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol (3)
8-27: completeUserIntent function override is correctly implemented
The completeUserIntent function properly overrides the base implementation and correctly calls router.completeIntentWithBalanceChecks with the appropriate parameters, ensuring accurate test execution.
127-128: Ensure AMOUNT is greater than zero to prevent underflow
The calculation uint256 amountReduced = AMOUNT - 1; may underflow if AMOUNT is zero. Verify that AMOUNT is initialized to a value greater than zero in the test setup to avoid underflow errors.
80-81: Confirm that AMOUNT * TOKEN_PRICE exceeds one to prevent underflow
In uint256 amountReduced = AMOUNT * TOKEN_PRICE - 1;, ensure that the product of AMOUNT and TOKEN_PRICE is greater than one to avoid underflow. This verification will prevent potential runtime errors during testing.
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (1)
226-241:
Potential off-by-one error in checkRevertAmountInsufficient usage
In the checkRevertAmountInsufficient function (lines 226-241), minLastStepAmountIn is set to lastStepAmountIn + 1. Therefore, when calling this function from your test cases, you should pass lastStepAmountIn without incrementing it. However, in several test functions, lastStepAmountIn is passed as AMOUNT + 1, resulting in a double increment. This could cause the tests to check for an amount that is two units higher than intended, potentially leading to incorrect test outcomes.
Apply this diff to correct the lastStepAmountIn parameter in the test functions:
For example, in test_depositERC20_exactAmount_revert_lastStepAmountInsufficient:
function test_depositERC20_exactAmount_revert_lastStepAmountInsufficient() public {
ISynapseIntentRouter.StepParams[] memory steps = getDepositERC20Steps(AMOUNT);
- checkRevertAmountInsufficient({msgValue: 0, lastStepAmountIn: AMOUNT + 1, steps: steps});
+ checkRevertAmountInsufficient({msgValue: 0, lastStepAmountIn: AMOUNT, steps: steps});
}Please make similar adjustments in all affected test functions that call checkRevertAmountInsufficient.
Affected functions include but are not limited to:
test_depositERC20_fullBalance_revert_lastStepAmountInsufficient(lines 321-324)test_depositNative_exactAmount_revert_lastStepAmountInsufficient(lines 367-370)test_depositNative_fullBalance_revert_lastStepAmountInsufficient(lines 414-417)test_swapForwardERC20_exactAmount_revert_lastStepAmountInsufficient(lines 481-484)test_swapForwardERC20_fullBalance_revert_lastStepAmountInsufficient(lines 525-528)test_swapUnwrapForwardNative_exactAmounts_revert_lastStepAmountInsufficient(lines 600-603)test_swapUnwrapForwardNative_exactAmount0_revert_lastStepAmountInsufficient(lines 649-652)test_swapUnwrapForwardNative_exactAmount1_revert_lastStepAmountInsufficient(lines 704-707)test_swapUnwrapForwardNative_fullBalances_revert_lastStepAmountInsufficient(lines 752-755)test_swapDepositERC20_exactAmounts_revert_lastStepAmountInsufficient(lines 817-820)test_swapDepositERC20_exactAmount0_revert_lastStepAmountInsufficient(lines 864-867)test_swapDepositERC20_exactAmount1_revert_lastStepAmountInsufficient(lines 902-905)test_swapDepositERC20_fullBalances_revert_lastStepAmountInsufficient(lines 949-952)test_wrapDepositWETH_exactAmounts_revert_lastStepAmountInsufficient(lines 1012-1015)test_wrapDepositWETH_exactAmount0_revert_lastStepAmountInsufficient(lines 1055-1058)test_unwrapDepositNative_exactAmounts_revert_lastStepAmountInsufficient(lines 1208-1211)test_unwrapDepositNative_exactAmount0_revert_lastStepAmountInsufficient(lines 1251-1254)test_unwrapDepositNative_exactAmount1_revert_lastStepAmountInsufficient(lines 1285-1288)test_unwrapDepositNative_fullBalances_revert_lastStepAmountInsufficient(lines 1332-1335)
packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (1)
66-73: Fix typo in function name: _createHandleHativeSteps should be _createHandleNativeSteps
There is a typo in the function name _createHandleHativeSteps. It should be _createHandleNativeSteps to accurately reflect its purpose of handling native tokens. This typo exists in both the function definition and where it's called. Please update the function name and all references accordingly.
Apply the following diff to correct the typo:
// Update function call in previewIntent:
if (params.action == Action.RemoveLiquidity) {
steps = _createRemoveLiquiditySteps(tokenIn, tokenOut, params, forwardTo);
} else {
- steps = _createHandleHativeSteps(tokenIn, tokenOut, amountIn, forwardTo);
+ steps = _createHandleNativeSteps(tokenIn, tokenOut, amountIn, forwardTo);
}
// Correct function definition:
- function _createHandleHativeSteps(
+ function _createHandleNativeSteps(
address tokenIn,
address tokenOut,
uint256 amountIn,
address forwardTo
)
internal
pure
returns (ISynapseIntentRouter.StepParams[] memory steps)
{
// Function implementation
}Also applies to: 218-239
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (1)
45-45: Consider adding test cases for non-zero addresses.
While using address(0) works for basic testing, it might mask potential issues. Consider adding test cases with non-zero addresses for finalToken and forwardTo parameters to ensure the encoding works correctly in all scenarios.
Let's verify if there are any tests covering non-zero addresses:
packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (2)
88-122: LGTM! Comprehensive error definitions.
The error types are well-defined and cover various failure scenarios:
- Pool-related errors (token mismatch, zero address)
- Input validation errors (empty params, native token checks)
- Zap-specific errors (invalid encoding, target address)
2-7: Verify empty constructor arguments.
The contract is deployed with empty constructor arguments ("0x"). Please verify if this is intentional and aligns with the contract's implementation.
Let's check the contract's constructor:
✅ Verification successful
Empty constructor arguments are correct
The contract implementation shows that there is no explicit constructor defined, which means it uses the default empty constructor. Therefore, the empty constructor arguments ("0x") in the deployment configuration are correct and intentional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for constructor in the contract implementation
ast-grep --pattern 'constructor($$$) {
$$$
}'
Length of output: 49
Script:
#!/bin/bash
# Let's first find the contract file
fd "SynapseIntentPreviewer" --type f --extension sol
# Then we'll examine its content
rg -l "contract SynapseIntentPreviewer" --type sol
Length of output: 336
Script:
#!/bin/bash
# Let's examine the contract implementation
cat packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol
Length of output: 14138
packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json (2)
94-122: Add events for important state changes
The contract lacks events to track important operations and state changes. Consider adding events for:
- Intent preview requests
- Failed operations with their respective error codes
2-2: Verify contract address on Arbitrum block explorer
The contract is deployed at 0x9519E8D136d0a89d7e10D1a66C97249E0135544B. Let's verify its existence and code verification status.
✅ Verification successful
Contract is deployed and verified on Arbitrum
The contract at 0x9519E8D136d0a89d7e10D1a66C97249E0135544B exists on Arbitrum and its code is verified. The API response shows the contract's ABI which includes:
- Error definitions for SIP and ZapDataV1
- View function
NATIVE_GAS_TOKEN - Main function
previewIntentfor handling swap previews
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and code verification status
# Expected: Contract exists and is verified
# Check if contract exists and is verified on Arbiscan
curl -s "https://api.arbiscan.io/api?module=contract&action=getabi&address=0x9519E8D136d0a89d7e10D1a66C97249E0135544B"
Length of output: 1860
packages/contracts-rfq/deployments/optimism/TokenZapV1.json (2)
69-83: Verify amountPosition bounds in encodeZapData
The encodeZapData function accepts an amountPosition parameter without apparent bounds checking. This could lead to out-of-bounds access when the position is applied to the payload.
94-122: Consider implementing reentrancy protection for zap function
The zap function is marked as payable and likely performs external calls. Ensure proper reentrancy protection is implemented to prevent potential attacks.
packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (1)
2-2: Verify contract address on Arbitrum block explorer
The contract is deployed at 0x6327797F149a75D506aFda46D5fCE6E74fC409D5. Let's verify its existence and code verification status.
✅ Verification successful
Contract is deployed and verified on Arbitrum
The contract at 0x6327797F149a75D506aFda46D5fCE6E74fC409D5 exists and is verified on Arbitrum, as confirmed by the successful API response containing the contract's ABI. The ABI shows it's a TokenZapV1 contract with expected functionality including:
zapfunction for token swappingencodeZapDataanddecodeZapDatafunctions- Native gas token support
- Error handling for various edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify contract deployment and code verification status
# Expected: Contract exists and is verified
# Check if contract exists and is verified on Arbiscan
curl -s "https://api.arbiscan.io/api?module=contract&action=getabi&address=0x6327797F149a75D506aFda46D5fCE6E74fC409D5"
Length of output: 2864
packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1)
87-89: 🛠️ Refactor suggestion
Consider testing non-zero values for finalToken and forwardTo parameters
All test cases initialize finalToken and forwardTo with address(0). While this tests the basic functionality, it doesn't validate the behavior when these parameters contain actual addresses.
Consider adding test cases with non-zero addresses to validate:
- Token forwarding functionality
- Final token conversion scenarios
- Edge cases with invalid addresses
depositTokenParams.zapData = dstZap.encodeZapData({
target: address(dstVault),
payload: getDepositPayload(address(dstToken)),
amountPosition: 4 + 32 * 2,
- finalToken: address(0),
- forwardTo: address(0)
+ finalToken: address(dstToken),
+ forwardTo: user
});Also applies to: 98-100, 106-108, 114-116, 121-123
packages/contracts-rfq/test/libs/ZapDataV1.t.sol (1)
21-22: LGTM! Comprehensive parameter encoding implementation
The encoding function correctly handles the new parameters while maintaining backward compatibility with the existing structure.
Also applies to: 30-30
| function test_swapUnwrapForwardNative_exactAmount1_extraFunds_revertWithBalanceChecks() | ||
| public | ||
| virtual | ||
| withExtraFunds | ||
| { | ||
| uint256 initialBalance = user.balance; | ||
| uint256 amountSwap = AMOUNT / TOKEN_PRICE; | ||
| ISynapseIntentRouter.StepParams[] memory steps = getSwapUnwrapForwardNativeSteps(FULL_BALANCE, amountSwap); | ||
| completeUserIntent({ | ||
| msgValue: 0, | ||
| amountIn: AMOUNT, | ||
| minLastStepAmountIn: amountSwap, | ||
| deadline: block.timestamp, | ||
| steps: steps | ||
| }); | ||
| // Check the user native balance | ||
| assertEq(user.balance, initialBalance + amountSwap + EXTRA_FUNDS); | ||
| } |
There was a problem hiding this comment.
Inconsistent test function name and behavior in test_swapUnwrapForwardNative_exactAmount1_extraFunds_revertWithBalanceChecks
The function test_swapUnwrapForwardNative_exactAmount1_extraFunds_revertWithBalanceChecks (lines 678-695) suggests that it should test for a revert due to balance checks when extra funds are present. However, the function executes completeUserIntent without expecting any revert, and it asserts that the user's balance increases, indicating a successful execution. This inconsistency between the test function's name and its behavior may cause confusion.
Consider renaming the function to accurately reflect its behavior, such as test_swapUnwrapForwardNative_exactAmount1_extraFunds_allowsExtraFunds, or update the test to properly verify that a revert occurs due to balance checks when extra funds are present.
| /// Last action is "use exact amount", so extra funds have no effect. | ||
| function test_wrapDepositWETH_exactAmount1_extraFunds_revertWithBalanceChecks() public virtual withExtraFunds { | ||
| test_wrapDepositWETH_exactAmount1(); | ||
| } |
There was a problem hiding this comment.
Inconsistent test function name and behavior in test_wrapDepositWETH_exactAmount1_extraFunds_revertWithBalanceChecks
Similarly, the function test_wrapDepositWETH_exactAmount1_extraFunds_revertWithBalanceChecks (lines 1084-1087) implies that it should test for a revert due to balance checks with extra funds. However, it simply calls test_wrapDepositWETH_exactAmount1(), which does not expect a revert and indicates a successful execution. This inconsistency may lead to misunderstandings about the test's purpose.
Please consider renaming the function to match its actual behavior or modifying it to correctly test for a revert due to balance checks when extra funds are involved.
| /// Last action is "use exact amount", so extra funds have no effect. | ||
| function test_unwrapDepositNative_exactAmount1_extraFunds_revertWithBalanceChecks() public virtual withExtraFunds { | ||
| test_unwrapDepositNative_exactAmount1(); | ||
| } |
There was a problem hiding this comment.
Inconsistent test function name and behavior in test_unwrapDepositNative_exactAmount1_extraFunds_revertWithBalanceChecks
The test function test_unwrapDepositNative_exactAmount1_extraFunds_revertWithBalanceChecks (lines 1275-1278) suggests it should test for a revert due to balance checks when extra funds are present. However, it calls test_unwrapDepositNative_exactAmount1(), which does not expect a revert and indicates that extra funds have no effect. This discrepancy between the function name and its behavior could be misleading.
Recommend renaming the function to accurately reflect its actual behavior or updating the test logic to properly verify a revert due to balance checks when extra funds are present.
| revert TokenZapV1__PayloadLengthAboveMax(); | ||
| } | ||
| // Final token needs to be specified if forwarding is required. | ||
| if (forwardTo != address(0) && finalToken == address(0)) { | ||
| revert TokenZapV1__TokenZeroAddress(); | ||
| } |
There was a problem hiding this comment.
Ensure amountPosition fits within uint16 before casting
In the encodeZapData function, amountPosition is accepted as a uint256 but later cast to uint16 without a strict upper bound check. This could lead to unexpected truncation if amountPosition exceeds uint16 maximum value. To prevent potential issues, please add a validation to ensure amountPosition fits within the uint16 range before casting.
Apply the following diff to enforce the check:
if (amountPosition >= payload.length) {
amountPosition = ZapDataV1.AMOUNT_NOT_PRESENT;
+ } else if (amountPosition > type(uint16).max) {
+ revert TokenZapV1__AmountPositionTooLarge();
}
- // At this point, we have checked that both `amountPosition` and `payload.length` fit in uint16.
+ // At this point, we have ensured that `amountPosition` fits within uint16.
return ZapDataV1.encodeV1({
amountPosition_: uint16(amountPosition),
finalToken_: finalToken,
forwardTo_: forwardTo,
target_: target,
payload_: payload
});Also, define the new error:
error TokenZapV1__AmountPositionTooLarge();| /// @notice Forwards the proceeds of the Zap action to the specified non-zero recipient. | ||
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reentrancy protection when forwarding tokens
The _forwardToken function performs external calls to untrusted addresses (forwardTo) which could introduce reentrancy risks. Although the contract is stateless, it's good practice to implement reentrancy guards to enhance security.
Consider adding the nonReentrant modifier from OpenZeppelin's ReentrancyGuard to the zap function and any other functions that transfer funds to external addresses.
Description
Synapse Intent Router (or just SIR) is a contract that completes the user intents using a series of generic Zap steps (see #3352 for more details about Zaps). Actions like swapping, depositing, bridging, sending tokens and many others are supported as long as they could be represented as a Zap.
Contract exposes two entry points with minor differences:
completeIntentandcompleteIntentWithBalanceChecks:completeIntentWithBalanceChecksalso verifies that no leftover tokens are left in the auxilaryZapRecipientcontract used for performing the Zap steps.Summary by CodeRabbit
New Features
ISynapseIntentRouterinterface for executing complex token transfers with balance checks.ISynapseIntentRouterErrorsinterface.SynapseIntentRoutercontract to manage and execute "zap" operations.WETHMockcontract to support token minting.ISynapseIntentPreviewerinterface for previewing token swaps.SynapseIntentRouterandTokenZapV1on Arbitrum and Optimism networks.TokenZapV1contract with improved error handling and new functionality for token forwarding.Bug Fixes
SynapseIntentRouterfor better user experience.Tests
SynapseIntentRouter, validating functionality and error conditions.SynapseIntentPreviewercontract to ensure proper functionality across various scenarios.TokenZapV1to reflect new parameters and functionalities.