feat(contracts-rfq): optional strict slippage for SynapseIntentPreviewer
#3467
Conversation
WalkthroughThis pull request introduces a comprehensive update to the Synapse protocol's contracts, focusing on enhancing the Changes
Sequence DiagramsequenceDiagram
participant User
participant SynapseIntentPreviewer
participant TokenZapV1
participant TargetContract
User->>SynapseIntentPreviewer: previewIntent(swapQuoter, forwardTo, slippageWei, tokenIn, tokenOut, amountIn)
SynapseIntentPreviewer-->>User: Generate steps with slippage
User->>TokenZapV1: zap(token, amount, zapData)
TokenZapV1->>TokenZapV1: Validate minFinalBalance
TokenZapV1->>TargetContract: Execute transaction
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
|
Deploying sanguine-fe with
|
| Latest commit: |
2d3565a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://977bfa4e.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-syn-intent-previewer-st.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3467 +/- ##
============================================
Coverage ? 98.89241%
============================================
Files ? 11
Lines ? 632
Branches ? 0
============================================
Hits ? 625
Misses ? 7
Partials ? 0
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: 0
🧹 Nitpick comments (5)
packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (1)
Line range hint
244-244: Typographical error in function name_createHandleHativeStepsThe function name
_createHandleHativeStepscontains a typographical error. It should be_createHandleNativeSteps.Apply this diff to correct the typo and update all references:
-function _createHandleHativeSteps( +function _createHandleNativeSteps(Ensure that all calls to this function are updated accordingly, such as:
-steps = _createHandleHativeSteps(tokenIn, tokenOut, amountIn, forwardTo); +steps = _createHandleNativeSteps(tokenIn, tokenOut, amountIn, forwardTo);packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultActions.sol (2)
7-7: Fix typo in documentation commentChange "woth" to "both" in the comment.
-/// as it includes woth WETH and Pool functions. +/// as it includes both WETH and Pool functions.
8-36: Interface design looks good but could benefit from additional documentationThe interface correctly defines WETH and Pool functions with appropriate parameters for security (deadlines) and slippage protection. Consider adding:
@devcomments explaining the expected behavior of each function@paramdescriptions for function parameters@returndescriptions for return valuespackages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (2)
45-46: Add documentation for thestrictOutstate variable.Consider adding a comment explaining that this boolean controls whether exact output amounts are enforced during swaps and liquidity operations.
+ /// @notice Controls whether exact output amounts are enforced (true) or ignored (false) during operations bool public strictOut;
653-659: Document the rationale for disabling strict output in intermediate steps.In double-step operations, the intermediate step explicitly sets
useExactAmountOuttofalsewhile the final step uses thestrictOutvalue. This behavior should be documented to explain why intermediate steps don't enforce exact outputs.// step0: tokenA -> weth, always no forwaring, no strictOut + // Note: Intermediate steps don't enforce exact outputs to allow for price movements ISynapseIntentRouter.StepParams memory expectedStep0 = ISynapseIntentRouter.StepParams({Also applies to: 690-696
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol(2 hunks)packages/contracts-rfq/contracts/legacy/router/interfaces/IDefaultActions.sol(1 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(12 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json(2 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json(2 hunks)packages/contracts-rfq/devops.json(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.StrictOut.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol(21 hunks)
🔇 Additional comments (10)
packages/contracts-rfq/devops.json (2)
3-3: Verify the intentional removal of auto-gas-1559 flags
The auto-gas-1559 flags have been removed for Arbitrum, Base, Blast, Mainnet, and Optimism networks. While these networks support EIP-1559, removing automatic gas price estimation might affect the deployment process.
Run the following script to check for any deployment-related changes or documentation:
Consider documenting:
- The rationale behind removing automatic gas estimation
- How gas prices should be handled during deployments for these networks
- Whether any additional configuration is required
Also applies to: 5-5, 6-6, 14-14, 18-18
Line range hint 1-27: Verify configuration consistency across environments
The changes maintain different deployment flags for different networks (e.g., --legacy, --slow, --skip-simulation). Ensure these configurations are consistent with the new strict slippage implementation.
Run the following script to check for related configuration patterns:
✅ Verification successful
Let me gather more information about the configuration patterns to verify the consistency with the strict slippage implementation.
Let me search for more specific information about the deployment configurations and slippage handling.
Let me search for more specific information about the deployment flags and configurations.
Based on the search results, I can now provide a conclusive response about the deployment flags and their consistency with slippage implementation.
Network-specific deployment flags are correctly configured for transaction handling
The deployment flags in devops.json are properly configured to handle different network characteristics:
--legacyflag is used for networks that require legacy transaction types (Aurora, Boba, Canto, Dogechain, Harmony, Metis, Scroll)--slowflag is used for networks that need additional confirmation time (Aurora, Boba, Canto, Cronos, DFK, Dogechain, Harmony, Moonbeam, Moonriver, Polygon)--skip-simulationflag is used for networks where transaction simulation may be unreliable (Boba, Klaytn, Moonbeam, Moonriver)--auto-gas-1559is used for EIP-1559 compatible networks (Arbitrum, Base, Blast, Cronos, DFK, Klaytn, Mainnet, Moonbeam, Moonriver, Optimism, Polygon)
These flags are independent of slippage implementation as they handle network-specific transaction submission behaviors rather than application-level slippage calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related configuration patterns across deployment files
# Search for network-specific configurations
fd -t f "devops.json" -X jq -r '.chains'
# Check for any related deployment scripts
fd -t f "deploy.*" -X cat {}
Length of output: 107243
Script:
#!/bin/bash
# Search for slippage-related configurations and implementations
rg -A 5 -B 5 "slippage"
# Search for any deployment scripts that handle network-specific configurations
rg -A 5 -B 5 "legacy|slow|skip-simulation"
# Search for any test files that verify network-specific behaviors
fd -t f "test.*" -X cat {}
Length of output: 78832
Script:
#!/bin/bash
# Search for slippage-related code and configurations
rg -A 5 -B 5 "minDy|slippage"
# Search for deployment-related files and configurations
fd -t f "deploy.*config.*" -X cat {}
# Search for any test files that verify network-specific behaviors
fd -t f "test.*" -e sol -X cat {}
Length of output: 83912
Script:
#!/bin/bash
# Search for deployment configuration files and their usage
fd -t f "devops.json|deploy.*config" -X cat {}
# Search for any files that define or use the deployment flags
rg -A 5 -B 5 "legacy|slow|skip-simulation"
# Search for any deployment scripts that handle network-specific behaviors
fd -t f "deploy.*" -e sol -X cat {}
Length of output: 69857
packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (2)
60-60: Confirm logic for lastStepMinAmountOut assignment
The assignment of lastStepMinAmountOut using strictOut ? amountOut : 0 appears correct. However, ensure that amountOut is reliably computed and that this logic aligns with the intended behavior for strict and non-strict output scenarios.
35-35: Consider potential ABI compatibility issues due to function signature change
Adding the new parameter strictOut to the previewIntent function changes its signature. This modification may impact any external contracts or applications that rely on the previous function signature. Ensure that all dependent contracts and interfaces are updated accordingly to prevent potential integration issues.
Run the following script to locate all references to previewIntent and verify that they are updated:
packages/contracts-rfq/test/router/SynapseIntentPreviewer.StrictOut.t.sol (1)
6-11: New test contract correctly sets up strict output scenarios
The SynapseIntentPreviewerStrictOutTest contract extends SynapseIntentPreviewerTest and appropriately sets strictOut = true in the overridden setUp function. This ensures that tests will properly evaluate the strict output conditions introduced by the new parameter.
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol (1)
24-24:
Ensure all implementations of ISynapseIntentPreviewer are updated
Introducing the strictOut parameter to the previewIntent function modifies the interface. All contracts that implement ISynapseIntentPreviewer must update their implementations to match the new function signature to maintain compatibility.
Run the following script to identify implementations that need updating:
✅ Verification successful
Implementation is already updated with the new parameter
The only implementation of ISynapseIntentPreviewer is in SynapseIntentPreviewer.sol, and it already includes the strictOut parameter in its previewIntent function signature and uses it correctly in the implementation (line 43 and line 63 where it sets lastStepMinAmountOut based on the strictOut value).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all contracts implementing `ISynapseIntentPreviewer` to ensure they are updated.
# Search for contracts that implement `ISynapseIntentPreviewer` in Solidity files.
rg -t solidity 'contract.*is.*ISynapseIntentPreviewer'
Length of output: 89
Script:
#!/bin/bash
# Search for contracts that implement ISynapseIntentPreviewer using ripgrep without file type filter
rg "contract.*implements.*ISynapseIntentPreviewer|contract.*is.*ISynapseIntentPreviewer"
# Also search for any imports of ISynapseIntentPreviewer to find potential implementations
rg "import.*ISynapseIntentPreviewer"
Length of output: 407
Script:
#!/bin/bash
# Let's check the implementation of previewIntent in SynapseIntentPreviewer.sol
cat packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol
Length of output: 14953
packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (1)
2-6: Verify deployment and ABI changes
The deployment configuration has been properly updated with:
- New contract address
- New transaction details
- Addition of
strictOutparameter in the correct position
Please ensure:
- The deployment transaction is finalized on Optimism
- The new contract address is updated in all relevant documentation
Also applies to: 36-40
✅ Verification successful
Let me verify the ABI changes by checking the contract interface.
Let me check the transaction receipt and contract implementation to verify the deployment.
Based on the provided context, I can now verify the deployment:
Deployment verified successfully
The transaction receipt shows:
- Status: "0x1" (success)
- Contract deployment is finalized at block 129285056
- The implementation matches the contract in the codebase (SynapseIntentPreviewer.sol)
- The deployment files are properly organized in network-specific directories
- The new
strictOutparameter is correctly implemented in the contract interface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the deployment transaction is finalized
curl -s -X POST -H "Content-Type: application/json" --data '{
"jsonrpc":"2.0",
"method":"eth_getTransactionReceipt",
"params":["0xa525aa65479aae46377a908d4451af3951e7ae05ba98609beadbae051222a744"],
"id":1
}' https://mainnet.optimism.io
Length of output: 1437
Script:
#!/bin/bash
# Find and check the interface file for SynapseIntentPreviewer
fd -e sol -e ts "SynapseIntentPreviewer" --exec cat {}
Length of output: 43903
Script:
#!/bin/bash
# Check if the contract address matches the implementation
ast-grep --pattern 'contract SynapseIntentPreviewer'
Length of output: 54
Script:
#!/bin/bash
# Check the transaction receipt and contract implementation
rg -l "SynapseIntentPreviewer" --type sol
Length of output: 69
Script:
#!/bin/bash
# Find all files related to SynapseIntentPreviewer
fd "SynapseIntentPreviewer"
Length of output: 448
packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json (1)
2-6: Verify cross-chain deployment consistency
The contract address matches across both Optimism and Arbitrum deployments, suggesting deterministic deployment. This is good practice for cross-chain consistency.
Please ensure:
- The deployment transaction is finalized on Arbitrum
- The deterministic deployment process is documented
Consider adding deployment documentation explaining the deterministic deployment process and cross-chain address consistency.
Also applies to: 36-40
packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (2)
76-88: LGTM! Well-structured helper function.
The helper function effectively encapsulates the common preview intent call pattern, making the test cases more maintainable.
124-137: Consider adding explicit test cases for both true and false useExactAmountOut values.
While the functions correctly handle the useExactAmountOut parameter, there aren't explicit test cases verifying the behavior for both true and false values independently. This could leave edge cases untested.
Also applies to: 188-201, 260-268
github-actions
bot
added
size/m
and removed
size/s
needs-go-generate-services/rfq
labels
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1)
Line range hint
1-177: LGTM! Consider adding documentation for the new parameters.The test implementation is well-structured with good separation of concerns and comprehensive test scenarios. However, it would be helpful to add documentation comments explaining the purpose and expected behavior of the new
forwardToandminFwdAmountparameters in the test scenarios.Add documentation comments above the
createFixturesfunction explaining:
- The purpose of
forwardToandminFwdAmountparameters- Why certain values were chosen for different test scenarios
- What edge cases are being tested
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (1)
129-130: Zero forwarding in getDepositZapData
Again,address(0)andminFwdAmount = 0mean no forwarding. Confirm that no forward calls are actually needed here.packages/contracts-rfq/script/DeploySIR.s.sol (1)
30-37: Enhance error detail when the deployment fails the code check.
Currently, an assert(false) halts execution. You could add a revert with a more descriptive message.- assert(false); + revert(string.concat(contractName, " runtime code mismatch at ", vm.toString(deployedAt)));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol(2 hunks)packages/contracts-rfq/contracts/libs/ZapDataV1.sol(5 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(17 hunks)packages/contracts-rfq/contracts/zaps/TokenZapV1.sol(4 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/script/DeploySIR.s.sol(2 hunks)packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol(2 hunks)packages/contracts-rfq/test/integration/TokenZapV1.t.sol(2 hunks)packages/contracts-rfq/test/libs/ZapDataV1.t.sol(8 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.HighSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.NoSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.WithSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol(28 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(16 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol(1 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.t.sol(22 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/contracts-rfq/contracts/interfaces/ISynapseIntentPreviewer.sol
- packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol
🔇 Additional comments (133)
packages/contracts-rfq/test/router/SynapseIntentPreviewer.HighSlippage.t.sol (3)
2-2: Confirmed Solidity compiler version is acceptable.
Using pragma solidity 0.8.24 is fine for a typical modern Solidity environment. There are no apparent issues with compiler features here.
6-6: New contract extension appears sound.
Inheriting from SynapseIntentPreviewerTest is a logical choice for adding specialized test scenarios dealing with high slippage.
7-12: Ensure coverage of capping logic in the parent contract.
By setting slippageWei to 1e20, you're testing the scenario where slippage is out of bounds and should be capped at 100%. Confirm there is a dedicated test (or assertion) verifying that the actual slippage used by the SynapseIntentPreviewer is bounded.
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (22)
81-82: Confirm usage of zero address and zero minFwdAmount
Setting forwardTo = address(0) and minFwdAmount = 0 ensures no forwarding by default. Verify that this test scenario intentionally requires no tokens to be forwarded.
86-95: Validate placeholder zero amount
Using abi.encodeCall(weth.withdraw, (0)) as a placeholder is fine, as the actual withdrawal amount will be injected. Ensure all tests accurately reflect this logic so that the correct values are passed at runtime.
99-107: New signature for getSwapZapData
The new parameters forwardTo and minFwdAmount align with the updated forwarding logic. The approach looks consistent with the rest of the contract.
Also applies to: 116-117
256-271: Revert test for insufficient forward amount
This is a solid test to confirm TokenZapV1__ForwardAmountBelowMin is triggered properly. Good coverage.
Line range hint 461-475: getSwapForwardERC20Steps includes minFwdAmount
Adding minFwdAmount ensures correct minimum forwarding behavior. The approach remains consistent with the rest of the suite.
510-510: Deadline test for swapForwardERC20
Ensures the deadline logic is properly validated. No issues noted.
515-515: Last step amount verification
Checks the revert when final step amount is insufficient. This is important negative testing.
519-522: Test for insufficient forward amount
The new test ensures we revert if actual forward is below minFwdAmount. Good negative coverage.
523-523: No changes here
No additional code changes in this line.
531-531: Full balance swap
Verifies the “full balance” approach works as expected. No issues found.
546-546: Full balance plus extra funds
Ensures logic holds with extra funds. Looks good.
559-559: Deadline revert check
Checks for revert on missed deadline. Good negative case coverage.
564-564: Last step insufficient revert
Confirmed coverage for insufficient final step.
568-572: Forward amount insufficient with full balance
Expands on negative testing for the full balance scenario. Thorough approach.
573-573: No changes in this line
No further comments.
589-590: Overload calls with default minFwdAmount
Uses AMOUNT / TOKEN_PRICE as a default. Matches the test design for partial unwrap.
592-599: Extended signature for getSwapUnwrapForwardNativeSteps
Including minFwdAmount is consistent with the updated forward logic.
607-607: Zero forward data for the swap step
Setting forwardTo: address(0) and minFwdAmount: 0 within the swap step is consistent with a no-forward scenario.
614-614: Unwrap step with dynamic forward
This step uses the newly introduced minFwdAmount, ensuring the user receives the correct minimum.
711-717: Forward amount insufficient (exactAmount0 scenario)
Properly tests revert when forward amount is below the required threshold.
772-778: Forward amount insufficient (exactAmount1 scenario)
Again, thorough negative testing to ensure correctness with partial amounts.
828-834: Forward amount insufficient (fullBalances scenario)
Good coverage for the full balance forward logic.
packages/contracts-rfq/test/router/SynapseIntentPreviewer.NoSlippage.t.sol (1)
1-12: No-slippage test contract
Setting slippageWei to zero ensures revert on any price change. This is a valid scenario for specific swaps.
packages/contracts-rfq/test/router/SynapseIntentPreviewer.WithSlippage.t.sol (1)
1-13: With-slippage test contract
swapMinAmountOut and slippageWei are adjusted to assert correct behavior under 1% slippage. Implementation is straightforward.
packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json (4)
2-2: Updated contract address and receipt
New deployment address and receipt hash reflect an updated deployment to Arbitrum. Ensure this aligns with environment config.
Also applies to: 5-6
36-40: Added slippageWei parameter
The ABI now includes slippageWei. This aligns with the code updates that control minimal forward amounts based on slippage.
118-122: ZapDataV1__ForwardParamsIncorrect error
Provides clarity for forward parameter issues. Good to see additional error coverage.
128-132: ZapDataV1__PayloadLengthAboveMax error
Enhances error handling for invalid payload lengths. No further comments.
packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json (4)
2-2: New contract address on Optimism
Configuration updated for the Optimism network. Confirm correctness in your environment.
Also applies to: 5-6
36-40: Updated ABI to include slippage
Mirrors the changes in the Arbitrum deployment. Ensures consistency across environments.
118-122: ZapDataV1__ForwardParamsIncorrect
Ensures the contract reverts properly when forward parameter checks fail.
128-132: ZapDataV1__PayloadLengthAboveMax
Expanded error coverage for out-of-bounds payload scenarios.
packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol (1)
223-227: LGTM! Named parameters improve code clarity.
The change to use named parameters enhances readability and maintainability by making the parameter meanings explicit.
packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (4)
45-46: LGTM! Well-defined state variables for slippage control.
The new state variables swapMinAmountOut and slippageWei are appropriately initialized and support the PR's objective of implementing optional strict slippage.
76-88: LGTM! Well-designed helper functions.
The new helper functions:
previewIntent: Provides a clean wrapper aroundsip.previewIntentwith slippage supportgetAdjustedMinFwdAmount: Implements a clear utility function for forward amount adjustment
Both functions follow good design principles and are well integrated into the test suite.
Also applies to: 107-109
128-141: LGTM! Consistent function signature updates.
The functions have been systematically updated to include the minAmountOut parameter, maintaining consistency across all zap-related functions while supporting the PR's slippage control objective.
Also applies to: 196-209, 266-278, 334-334, 385-385
162-166: LGTM! Comprehensive test coverage for forwarding scenarios.
The test assertions thoroughly verify the behavior of minFwdAmount in both forwarding and non-forwarding scenarios, ensuring the new slippage control functionality works as expected.
Also applies to: 233-237, 301-305, 353-357, 404-408
packages/contracts-rfq/script/DeploySIR.s.sol (3)
4-6: Imports for new contracts look good.
They appear consistent with usage in deployment code, ensuring the correct dependencies are accessible.
21-24: Consider adding salts for create2 in a follow-up.
The TODO reference to separate salts may add further determinism or collision protection.
25-28: Deployment verification is well-designed.
Comparing the deployed code hash to the expected runtime code adds reliability.
packages/contracts-rfq/test/libs/ZapDataV1.t.sol (24)
23-31: New parameter minFwdAmount in encodeZapData
This addition aligns with the updated signature. Ensure that all calls handle this parameter consistently throughout.
37-37: Added minFwdAmount field
Ensure that allocation of fuzzing values covers edge cases (e.g., zero and very large amounts).
48-49: Logical assumptions for finalToken and forwardTo
These constraint checks are correct to avoid mismatched forward parameters.
57-58: Delegated encoding with new parameter
Good that harness.encodeV1 is used for better coverage.
64-64: Validation of minFwdAmount
This assertion ensures the test logic is coherent.
69-70: Comparison against manual encoding
Great way to confirm the correctness of the updated encodeZapData.
77-77: Ensure coverage for no-amount scenario
Further tests for zero and non-zero minFwdAmount under no-amount cases would be beneficial.
87-88: Forwarding constraints
Similar to earlier lines, the test ensures correct behavior when forwardTo is zero.
91-91: Check usage of revert reasons
All new custom errors should remain consistent with the library usage.
97-97: Double check final token usage
It’s good to see the test verifying correct final token extraction.
101-104: Verifying manual encoding
A helpful final check that compares with your encodeZapData logic for no-amount scenario.
109-110: Target address must be non-zero
The revert check is correct, preventing calls into the zero address.
112-116: Forwarding parameters revert checks
Good coverage for invalid forward parameters.
117-121: minFwdAmount with no forwarding
Excellent negative test for ensuring minFwdAmount must be zero if no forwarding address is set.
122-124: Exceeding payload length
Comprehensive coverage for out-of-range payload size.
130-130: Check for invalid encoding
The test ensures that the amount position is within the payload.
145-145: Encoded zap data assertion
Manually encoded data is re-checked, ensuring consistent behavior.
148-148: Expect revert for invalid encode
Good negative test.
164-164: Unsupported version check
Ensures version mismatch triggers a revert as expected.
178-178: Manual re-encoding with custom version
Proper for verifying the UnsupportedVersion error.
188-188: No-amount scenario with unsupported version
Well-structured boundary test.
199-199: Ensuring revert for unknown version
Extends coverage to no-amount scenario.
207-207: Validate length checks
Ensures partial or truncated data triggers the InvalidEncoding revert.
Line range hint 493-494: No-amount roundtrip
Neat test confirming correct encode/decode flow for no-amount usage.
packages/contracts-rfq/contracts/libs/ZapDataV1.sol (9)
17-19: Additional fields in packed struct
Offsets look updated to include minFwdAmount. Keep an eye on potential alignment issues.
25-27: Offset constants
Correctly reflect the shift after adding a uint256 field.
31-32: New custom error
ZapDataV1__ForwardParamsIncorrect() is a concise name. Maintains clarity.
63-64: Added docstring for minFwdAmount_
Clear explanation of how forwarding thresholds function.
74-74: Include uint256 minFwdAmount_ in the signature
Ensures the library matches new forward usage across the codebase.
83-84: Check payload length vs AMOUNT_NOT_PRESENT
Prevents exceeding uint16 bounds. Nicely handled.
89-96: Forwarding constraints
Accurately verifying that if forwarding is requested, final token is non-zero and vice versa. Good defensive programming.
97-97: Return statement
Glad to see a fully packed encoding with the new param included.
124-130: Getter for minFwdAmount
Straightforward assembly usage, well commented.
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (6)
29-30: Fine-grained revert conditions
Streamlines error checking for forward param and min amounts.
89-93: Refactored _forwardToken call
Centralizing the logic with minimal parameters is easier to maintain.
121-122: Extended encodeZapData
Inclusion of minFwdAmount fosters consistent forward constraints.
128-129: Forward address handling
Zero vs. non-zero forward address logic is clear.
170-175: Zero forwardTo + non-zero minFwdAmount
Appropriately reverts to prevent unintended token locking.
177-179: Sufficient balance check
Ensures correct revert if the final token balance is below minFwdAmount.
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (37)
59-59: getZapDataDeposit updated
Inclusion of the zero minFwdAmount ensures deposit tests remain unchanged.
63-64: Retaining no-amount approach
Callers can set amountPosition to payload.length for clarity.
184-184: New parameter for unwrap
Places the amount at index 4, consistent with the WETHMock call.
191-192: getZapDataUnwrapAndForward signature extended
Supports forwarding logic with a minFwdAmount.
203-204: Structured param passing
Code is quite readable while bridging the new min forward amount.
210-210: Wrap call uses type(uint256).max
This indicates no amount reference in the payload.
214-222: getZapDataWrapAndForward function
Enables WETH deposit followed by forwarding. Proper separation of responsibilities.
228-229: Forward-to logic extended
Consistent usage of new param in encodeZapData.
277-277: minFwdAmount test
Confirms that user address correctly receives tokens if amount meets the threshold.
287-287: Placeholder approach
Ensures the test runs with approximate amounts; logic remains stable.
300-300: Extra funds scenario
Good coverage to confirm leftover tokens are applied to forwarding.
314-314: Again verifying leftover usage
Ensures final tokens plus extras are forwarded.
324-324: Forwarding to contract
Ensures contract addresses can also receive native tokens.
334-334: Test forwarding with placeholder
Maintains consistency between approximate and actual amounts.
347-347: Extra funds on contract forward
Test confirms the leftover logic remains consistent.
361-361: Reiterating leftover usage
Same approach validating leftover usage for a different function variant.
412-412: Wrap and forward test
Ensures WETH is transferred to user if minFwdAmount requirements are met.
424-424: Extra funds usage
Verifies that surplus balances do not break the forward logic.
438-439: Zero forward parameters
No final token forwarding is requested here. Straight deposit.
579-580: EOA target with non-empty payload
Check for revert if target is an EOA. Neat negative test.
594-595: Ensures EOA target reverts
Again verifying correct revert path for calls into EOA.
618-618: Revert on forward to a non-payable contract
Matches expectations for an internal call failing.
642-647: Less than minimum forward revert
Correctly tested. Avoids partial under-threshold forward.
653-658: Zero final token + forward
Ensures the correct revert if final token is not specified when forwarding.
659-671: Wrap call with zero forward
Tests that zero minFwdAmount means no forwarding while final token is non-zero.
672-680: Forward requires non-zero forwardTo
As expected, reverts if there's a minFwdAmount but no forwarding address.
685-686: Incorrect final token
Testing ForwardAmountBelowMin revert if the wrong token is produced.
690-695: Another under-min scenario
Checks logic consistency for unwrap flow.
702-708: Zero final token revert
Again verifying TokenZapV1__ForwardParamsIncorrect for missing final token.
709-721: Zero forward address with zero min
Correctly results in no forwarding, leftover balance remains.
723-732: Zero forward address with non-zero min
Same revert scenario as prior.
737-739: Mismatch in final token
Ensures we revert due to insufficient final token returned.
750-751: EOA target with non-empty payload
Reiterates intangible call revert coverage for native transfers.
767-768: Exceeding max payload
Confirms a revert from the library for payload length above AMOUNT_NOT_PRESENT.
775-775: Zero target
Ensures it triggers a revert at encode time, consistent with runtime checks.
780-782: Zero final token with forwardTo
Properly reverts if forwarding was requested but final token is zero.
784-787: No forwardTo with minFwdAmount
Matches the library’s ForwardParamsIncorrect logic.
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (3)
22-22: Include parameter validation for minFwdAmount_.
Although the contract delegates actual validation to ZapDataV1.validateV1(), consider adding checks or clarifications in this harness to ensure minFwdAmount_ is always consistent with the overall logic. This can help reveal potential edge cases during testing.
30-30: Confirm compatibility of arguments with ZapDataV1.encodeV1.
You've updated the function call to pass the minFwdAmount_ parameter, ensuring alignment with the corresponding library function. This looks correct. Double-check that all tests adequately cover scenarios where minFwdAmount_ is zero and non-zero.
45-47: Good addition for extracting minFwdAmount.
The new minFwdAmount function helps isolate the retrieval of this specific field, improving clarity in tests. Make sure that tests validate both expected values and error conditions for out-of-range values.
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (1)
45-45: Ensure the proper usage of new parameter in encodeZapData.
The additional minFwdAmount parameter is set to 0 in this gas benchmark test, which could differ from typical production usage. Confirm that this accurately reflects the real-world scenario you’re benchmarking, or consider testing multiple non-zero configurations.
packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (5)
2-2: Address, hash, and block number updates detected.
These deployment details look consistent with a redeployment. Ensure that downstream references (docs, scripts, etc.) are updated accordingly.
Also applies to: 5-6
83-87: New minFwdAmount parameter in ABI.
This parameter ensures precise control over minimum forwarding amounts in zap operations. Confirm that all relevant calling code sets and validates this parameter correctly.
168-173: Refined error handling with TokenZapV1__ForwardParamsIncorrect.
This addition clarifies invalid pointer usage or forward parameters. Ensure test coverage includes these new error conditions.
191-195: New error ZapDataV1__ForwardParamsIncorrect.
This error indicates an additional check in the underlying library. Verify that callers handle or expect this error properly, especially in cross-contract interactions.
201-205: New error ZapDataV1__PayloadLengthAboveMax.
Enforcing payload length constraints helps prevent potential data-based attacks. Confirm that larger or malformed payloads are rejected as expected.
packages/contracts-rfq/deployments/optimism/TokenZapV1.json (5)
2-2: Deployment details: Address, hash, and blockNumber changed.
These fields reflect a redeployment on Optimism. Confirm all references (documentation, configuration, etc.) remain in sync after the redeployment.
Also applies to: 5-6
83-87: Parameter minFwdAmount introduced in ABI.
Having a dedicated field for the minimal forward amount ensures consistent usage across all environment deployments. Check for uniform usage in client-side code.
168-173: Error handling extended with TokenZapV1__ForwardParamsIncorrect.
Align tests to cover potential misconfigurations or invalid forward parameters that could trigger this error.
191-195: ZapDataV1__ForwardParamsIncorrect introduced.
Matches the changes in the ZapDataV1 library. Validate cross-file usage and ensure thorough negative tests to confirm correct error raising.
201-205: Enhanced validation via ZapDataV1__PayloadLengthAboveMax.
This new error helps guard against oversized or corrupted input data. Encourage comprehensive payload boundary testing.
| forwardTo: address(0), | ||
| minFwdAmount: 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test cases for non-zero forward amounts.
While the changes consistently add the new minFwdAmount parameter across all test scenarios, they all use default values (forwardTo: address(0), minFwdAmount: 0). To ensure robust testing of the new functionality:
- Add test cases with non-zero
minFwdAmountvalues to verify the minimum forwarding amount validation - Add test cases with non-zero
forwardToaddresses to verify the forwarding functionality - Add test cases that should trigger the new error conditions (
TokenZapV1__ForwardAmountBelowMinandTokenZapV1__ForwardParamsIncorrect)
Would you like me to help generate additional test cases to cover these scenarios?
Also applies to: 104-105, 113-114, 122-123, 130-131
github-actions
bot
added
size/l
and removed
size/m
needs-go-generate-services/rfq
labels
| function minFinalBalance(bytes calldata encodedZapData) internal pure returns (uint256 minFinalBalance_) { | ||
| // Load 32 bytes from the offset. No shift is applied, as we need the full 256 bits. | ||
| assembly { | ||
| minFinalBalance_ := calldataload(add(encodedZapData.offset, OFFSET_MIN_FINAL_BALANCE)) | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Assembly usage Warning
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1)
92-93: 🛠️ Refactor suggestionEnhance test coverage for
minFinalBalanceparameter.While the new parameter has been added consistently across all test scenarios, they all use the default value (0). To ensure robust testing of the slippage functionality:
- Add test cases with non-zero
minFinalBalancevalues- Add test cases that trigger the new
TokenZapV1__FinalBalanceBelowMinerror- Add edge cases for balance validation
Additionally, as noted in previous reviews, consider adding test cases for non-zero forward amounts.
Would you like me to help generate additional test cases to cover these scenarios?
Also applies to: 104-105, 113-114, 122-123, 130-131
🧹 Nitpick comments (6)
packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (1)
24-25: Consider using unchecked math for gas optimization.The slippage calculation is correct, but since we're working with a percentage that's always less than 100%, the multiplication and division could be performed in an unchecked block to save gas.
- minFinalBalance = amountOut - (amountOut * slippageWei) / MAX_SLIPPAGE; + unchecked { + minFinalBalance = amountOut - (amountOut * slippageWei) / MAX_SLIPPAGE; + }Also applies to: 63-67
packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (1)
Line range hint
370-377: Consider adding edge case tests for slippage calculationsWhile the current tests cover basic success and revert cases for minimum final balance checks, consider adding tests for:
- Edge cases around slippage calculation precision
- Maximum possible slippage values
- Rounding behavior with very small amounts
Also applies to: 720-725
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (3)
Line range hint
188-201: Add input validation for minFinalBalanceThe encoding functions accept
minFinalBalancewithout validation. Consider adding checks for:
- Maximum reasonable values
- Relationship with amount parameter
- Consistency with token decimals
Also applies to: 214-230
660-676: Enhance error messages for better debuggingWhile the error handling is comprehensive, consider making the error messages more descriptive:
- Include actual and expected values in
FinalBalanceBelowMinerror- Add details about which token failed validation in
FinalTokenNotSpecifiedAlso applies to: 720-738
527-530: Consider making test helpers more flexibleThe test helpers could be improved by:
- Adding optional parameters with sensible defaults
- Supporting batch operations for multiple test cases
- Providing utility functions for common test scenarios
Also applies to: 538-539
packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json (1)
2-2: Deployed address update
The contract address has been updated to a new location. Ensure that dependent services or applications identify the correct target for interactions when upgrading to this new deployment.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol(0 hunks)packages/contracts-rfq/contracts/libs/ZapDataV1.sol(5 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(17 hunks)packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/contracts/zaps/TokenZapV1.sol(4 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json(3 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json(4 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json(3 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/optimism/TokenZapV1.json(4 hunks)packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol(2 hunks)packages/contracts-rfq/test/integration/TokenZapV1.t.sol(2 hunks)packages/contracts-rfq/test/libs/ZapDataV1.t.sol(8 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.HighSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.NoSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.WithSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol(27 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol(7 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(32 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.t.sol(22 hunks)
💤 Files with no reviewable changes (1)
- packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.NoSlippage.t.sol
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.WithSlippage.t.sol
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.HighSlippage.t.sol
- packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol
- packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol
- packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json
- packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json
- packages/contracts-rfq/test/libs/ZapDataV1.t.sol
🔇 Additional comments (123)
packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (3)
83-87: New parameter added to enhance slippage control.The addition of
minFinalBalanceparameter toencodeZapDataaligns with the PR objectives for implementing optional strict slippage control.
168-173: Enhanced error handling for better validation.The error handling has been improved with:
- Renamed error for clarity:
TokenZapV1__FinalBalanceBelowMin- New error for parameter validation:
TokenZapV1__FinalTokenNotSpecified- Additional ZapDataV1 library errors for better validation
Also applies to: 191-195, 201-205
2-6: Verify contract deployment on Arbitrum.The contract has been redeployed with updated functionality. Please ensure the deployment was successful and the contract is operating as expected on Arbitrum.
✅ Verification successful
Contract deployment on Arbitrum is verified and successful
The contract deployment at address
0x6C6FA1cE8160bb680f7a1dd2068c7302bA2a9eaBon Arbitrum has been verified. The contract has bytecode deployed, confirming it's a valid smart contract with the expected functionality including:
- Token zapping operations
- Enhanced error handling
- Balance validation features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify contract deployment and code size curl -s -X POST -H "Content-Type: application/json" --data '{ "jsonrpc": "2.0", "id": 1, "method": "eth_getCode", "params": ["0x6C6FA1cE8160bb680f7a1dd2068c7302bA2a9eaB", "latest"] }' https://arb1.arbitrum.io/rpc | jqLength of output: 10361
packages/contracts-rfq/deployments/optimism/TokenZapV1.json (1)
2-6: Verify contract deployment on Optimism.The contract has been redeployed with updated functionality. Please ensure the deployment was successful and the contract is operating as expected on Optimism.
✅ Verification successful
Contract deployment on Optimism is verified and successful
The contract at address
0x6C6FA1cE8160bb680f7a1dd2068c7302bA2a9eaBhas been successfully deployed on Optimism mainnet. The contract bytecode is present and matches the expected TokenZapV1 implementation, containing all the necessary functionality including:
- Token approval and transfer methods
- ETH handling (
0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee)- Zap operations with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify contract deployment and code size curl -s -X POST -H "Content-Type: application/json" --data '{ "jsonrpc": "2.0", "id": 1, "method": "eth_getCode", "params": ["0x6C6FA1cE8160bb680f7a1dd2068c7302bA2a9eaB", "latest"] }' https://mainnet.optimism.io | jqLength of output: 10360
packages/contracts-rfq/contracts/libs/ZapDataV1.sol (4)
17-19: LGTM! Well-structured layout changes.The struct layout is properly documented with clear field positions, and the offsets are correctly calculated based on the field sizes. The changes maintain backward compatibility by appending the new field.
Also applies to: 22-27
31-32: LGTM! Clear and descriptive error types.The new error types are well-named and provide clear feedback for validation failures.
58-66: LGTM! Well-documented parameter requirements.The documentation clearly explains the purpose and requirements of the new
minFinalBalanceparameter, including its relationship with thefinalTokenparameter.
126-133: LGTM! Consistent implementation of data extraction.The
minFinalBalancefunction follows the established pattern for data extraction, using inline assembly efficiently and safely.packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (3)
29-30: LGTM! Clear error handling improvements.The new error types provide more specific error conditions, improving the contract's error handling capabilities.
121-124: LGTM! Well-documented parameter addition.The
minFinalBalanceparameter is properly documented and integrated into the function signature.Also applies to: 130-131
172-183: LGTM! Robust balance checking implementation.The function efficiently handles both forwarding and minimum balance checks with proper error handling and optimizations through early returns.
packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol (1)
75-81: LGTM! Consistent parameter propagation.The
minFinalBalanceparameter is consistently propagated through all step creation functions and properly used in the ZapDataV1 encoding.Also applies to: 109-121, 134-145, 153-159, 202-202, 240-240, 272-277, 283-285, 308-308, 338-338, 365-365
packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol (3)
45-46: Well-structured state variables for slippage control!The state variables are appropriately initialized with
swapMinFinalBalance = 0andslippageWei = 1e18(100%), which aligns with the PR objectives for testing both strict and relaxed slippage scenarios.
76-88: Clean wrapper implementation for slippage control!The
previewIntentwrapper function elegantly encapsulates the slippage parameter while maintaining a clean interface for test cases. This approach makes it easy to modify slippage behavior across all tests.
Line range hint
450-456: Well-structured test cases for wrap operations!The tests thoroughly verify:
- Minimum final balance enforcement
- Balance checks after operations
- Proper revert behavior when conditions aren't met
Also applies to: 660-664
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol (1)
26-27: Clarity in documentation updates
This documentation clarifies that slippage checks are not performed by the router, indicating that all such checks must be handled within the Zap steps themselves. This is beneficial for users who may otherwise assume the router enforces slippage constraints.packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol (1)
44-44: Validation no longer enforced by router
By removing the parameter previously used to enforce minimum amounts, the router delegates all final step validation responsibility to ZapRecipient logic or external checks. Confirm that downstream components fully address any need for last step amount constraints to avoid unexpected behavior.packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json (1)
5-6: Block metadata revisions
The new transaction hash and block number reference a different deployment. Validate that indexes and historical logs remain consistent in your block explorer or indexing services to avoid referencing stale data.packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json (2)
2-2: Address alignment across networks
The updated address here mirrors the address from the Arbitrum deployment, unifying cross-chain references. Confirm that cross-chain monitoring tools point to this new address on Optimism.
5-6: Transaction receipt consistency
The updated transaction hash and block number indicate a new contract deployment. Verify that event logs, transaction indexing, and block explorers are aligned with these new references to avoid confusion about the contract’s lifecycle.packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (103)
81-82: No issues found with these new fields.
They align well with the zero minFinalBalance use case.
86-95: New parameters ingetUnwrapZapData
The usage of a zero placeholder for the withdrawal amount and specifyingminFinalBalancelooks consistent with the intended design.
99-107: Enhancement ingetSwapZapData
IntroducingforwardToandminFinalBalanceoffers more fine-grained control. The payload encoding is correct.Also applies to: 116-117
129-130: AddedforwardToandminFinalBalancewith default values
Setting them toaddress(0)and0is consistent with a no-forward scenario.
152-154:checkRevertMsgValueAboveExpectedWithERC20
The revert check for excessivemsg.valueis properly handled.
157-163:checkRevertsMsgValueAboveExpectedWithNative
Tests both “only msg.value too high” and “both amountIn & msg.value too high” scenarios.
166-172:checkRevertsMsgValueBelowExpectedWithNative
Accurately tests cases wheremsg.valueoramountInis insufficient.
175-177:checkRevertDeadlineExceeded
Proper revert testing for past-deadline scenarios. Looks solid.
180-187:checkRevertFinalBalanceInsufficient
Introduces a final-balance check. Reverting withTokenZapV1__FinalBalanceBelowMinis consistent.
205-205: Line 205
The call tocompleteUserIntentwith standard params is fine.
217-217: Line 217
Ensuresdeadlinechecks are tested for depositing ERC20.
222-222: Line 222
Revert test for excessivemsg.valuein deposit ERC20. Implementation is correct.
227-227: Line 227
completeUserIntentwithFULL_BALANCEdeposit scenario is validated.
235-235: Line 235
FULL_BALANCEdeposit plus extra funds is tested. Good coverage.
242-242: Line 242
Deadline exceed check. Approach is consistent with prior patterns.
247-247: Line 247
Excessmsg.valuerevert test. No issues.
265-265: Line 265
Checks exact native deposit. Implementation aligns well with deposit logic.
277-277: Line 277
Revert test for deadline-exceeded with native deposit. Straightforward.
282-282: Line 282
Msg.value above expected revert is correctly verified.
287-287: Line 287
Checks revert if msg.value is below expected for native deposit. Good coverage.
292-292: Line 292
VerifiesFULL_BALANCEdeposit for native token. Works with standard usage.
300-300: Line 300
Ensures extra funds usage inFULL_BALANCEdeposit. Good test scenario.
307-307: Line 307
Deadline exceed check. Consistent with prior patterns.
312-312: Line 312
Tests revert ifmsg.valueis too high with full balance deposit.
317-317: Line 317
Checks revert ifmsg.valueis below expected in full balance deposit.
322-325:getSwapForwardERC20Steps
IncorporatingminFinalBalanceparameter is aligned with final balance checks. No issues.Also applies to: 336-336
343-344: Lines 343-344
TestsexactAmountswap from WETH to ERC20. Proper verification of final user balance.
352-353: Lines 352-353
Confirms that extra funds are accounted for in a forward swap scenario.
359-360: Lines 359-360
Revert test for missed deadline in swap. Straightforward.
363-365: Lines 363-365
Revert test for insufficient final balance. Good coverage forminFinalBalance.
369-370: Lines 369-370
Revert if message value is unexpectedly high for swap. Clear approach.
375-376: Lines 375-376
TestsFULL_BALANCEfor WETH -> ERC20 swap. Implementation is consistent.
384-385: Lines 384-385
Checks extra funds usage inFULL_BALANCEswap scenario. Looks good.
391-392: Lines 391-392
Standard deadline revert test. No concerns.
395-398: Lines 395-398
Ensures revert if final ERC20 balance is belowminFinalBalance. Correct logic.
402-403: Lines 402-403
Revert test with an unexpectedmsg.value. Straightforward check.
415-426:getSwapUnwrapForwardNativeSteps
Supports multi-step flows with a newminFinalBalance. Implementation is coherent.Also applies to: 434-441
450-450: Line 450
Checks user’s final native balance after swap+unwrap. All good.
460-460: Line 460
Verifies extra funds are captured in final user balance for swap+unwrap.
468-468: Line 468
Deadline check for swap+unwrap scenario. No issues noted.
474-474: Line 474
Ensures revert ifmsg.valueis above expected in swap+unwrap.
481-481: Line 481
Handles a zero “exact amount” scenario for unwrap step. Implementation is consistent.
491-491: Line 491
Confirms extra funds usage with zero-amount unwrap is properly handled.
498-498: Line 498
Fails if deadline exceeded for swap+unwrap with full balance.
501-501: Lines 501, 503-505
Revert test covering insufficient final balance. Great coverage.Also applies to: 503-505
510-510: Line 510
Checks revert if msg.value is unexpectedly high in swap+unwrap zero-amount scenario.
517-517: Line 517
Basic exact-amount swap+unwrap. No issues.
532-532: Line 532
Confirms extra funds scenario for swap+unwrap steps. Good negative test approach.
540-540: Line 540
Deadline revert test again, remains consistent.
543-543: Lines 543, 545-547
Revert if final balance is below minimum after swap+unwrap. Spot on.Also applies to: 545-547
553-553: Line 553
Checks revert with an overly large msg.value for swap+unwrap with exact amount.
560-560: Line 560
Validates full balances scenario in swap+unwrap. Implementation is correct.
570-570: Line 570
Extra funds with swap+unwrap full balances. Good coverage.
577-577: Line 577
Deadline revert with full balances. Consistent approach.
580-580: Lines 580, 582-584
Reverts on final balance shortfall in a multi-step flow. Good thoroughness.Also applies to: 582-584
589-589: Line 589
Revert ifmsg.valueis above expected. Checking correctness for cost coverage.
608-608: Line 608
UsesgetSwapZapDatafor ERC20 swap in multi-step deposit. Implementation aligns with minFinalBalance logic.
623-623: Line 623
Tests exact swap+deposit scenario, verifying final vault balance.
636-636: Line 636
Deadline exceed revert coverage. No issues.
642-642: Line 642
Ensures revert ifmsg.valueis too high in swap+deposit ERC20.
648-648: Line 648
Zero-amount deposit step validated. Implementation is clear.
657-657: Line 657
Tests “extra funds” usage in final deposit step. Looks correct.
664-664: Line 664
Deadline revert check. Clear logic.
669-669: Line 669
Excess msg.value revert in swap+deposit flow. Appropriately tested.
675-675: Line 675
Tests partial swap with exact deposit. Implementation is consistent.
689-689: Line 689
Deadline check for partial swap scenario. Good coverage.
695-695: Line 695
Excess msg.value revert test. Consistent with other scenarios.
701-701: Line 701
Verifies full-balance swap & deposit. Implementation is coherent.
710-710: Line 710
Confirms extra funds usage for both swap and deposit steps. Good thorough testing.
717-717: Line 717
Deadline revert scenario. No issues.
722-722: Line 722
Revert ifmsg.valueis above expected for full balances scenario.
755-755: Line 755
Wrap+deposit exact amounts test. Implementation looks straightforward.
767-767: Line 767
Deadline exceed revert for wrap+deposit. Good approach.
772-772: Line 772
Tests revert with msg.value above expected for wrap+deposit scenario.
777-777: Line 777
Zero-amount deposit after wrap. No issues.
786-786: Line 786
Ensures extra funds are recognized for full balance deposit step.
793-793: Line 793
Deadline revert verifying no partial success. Correct.
798-798: Line 798
Excess msg.value revert for wrap+deposit. Consistent tests.
803-803: Line 803
Revert check for insufficient msg.value in wrap+deposit. Good negative coverage.
808-808: Line 808
Exact-amount deposit after full wrap. Implementation on track.
821-821: Line 821
Deadline revert scenario for wrap+deposit with partial step. No issues.
826-826: Line 826
Revert if msg.value is too high in exact wrap deposit. Proper check.
831-831: Line 831
Revert for an insufficient msg.value in wrap deposit scenario. Thorough.
836-836: Line 836
Testing full-balance wrap+deposit is correct.
845-845: Line 845
Ensures extra funds usage in both wrap and deposit steps. Good coverage.
852-852: Line 852
Deadline revert with wrap+deposit full balances. No issues found.
857-857: Line 857
Excess msg.value revert test. Straightforward.
862-862: Line 862
Revert if insufficient msg.value in wrap+deposit full balance. Thorough negative path check.
881-881: Line 881
getUnwrapZapDatausage for WETH->ETH step ingetUnwrapDepositNativeSteps. Perfectly aligned.
895-895: Line 895
Exact amounts in unwrap+deposit. Verified.
907-907: Line 907
Deadline exceed revert for unwrap+deposit. Appropriately tested.
912-912: Line 912
Excessmsg.valuerevert with WETH unwrap. No issues.
917-917: Line 917
Zero-amount deposit step in an unwrap scenario. Implementation is consistent.
926-926: Line 926
Confirms extra funds usage for final deposit. Good coverage.
933-933: Line 933
Deadline revert in unwrap+deposit for full balance. Fine.
938-938: Line 938
Excess msg.value revert. Matches the overall revert testing logic.
943-943: Line 943
Exact-amount deposit after WETH unwrap. Implementation matches tested logic.
956-956: Line 956
Deadline exceed revert with unwrap+deposit. Straightforward.
961-961: Line 961
Excess msg.value revert with exact deposit. Clear coverage.
966-966: Line 966
Full-balance unwrap+deposit scenario tested. Looks correct.
975-975: Line 975
Confirms extra funds usage with full-balance unwrap+deposit.
982-982: Line 982
Deadline exceed revert for full-balance unwrap+deposit. Good negative test.
987-987: Line 987
Excess msg.value revert test with full-balance unwrap+deposit.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/contracts-rfq/contracts/libs/ZapDataV1.sol (1)
58-66: Consider enhancing parameter documentation.The documentation for
minFinalBalance_could be more specific about its relationship with slippage protection./// @param minFinalBalance_ The minimum balance of `finalToken` to have after performing the Zap action. - /// This parameter could be used to implement slippage protection for a Zap action that does - /// not automatically transfer the token to the intended recipient. Otherwise, it must be - /// set to 0. + /// When used for slippage protection, this value represents the minimum acceptable output + /// amount after accounting for price impact and fees. For actions that automatically + /// transfer tokens to the recipient, this must be set to 0.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol(0 hunks)packages/contracts-rfq/contracts/libs/ZapDataV1.sol(5 hunks)packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol(17 hunks)packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol(1 hunks)packages/contracts-rfq/contracts/zaps/TokenZapV1.sol(4 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json(3 hunks)packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json(4 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json(3 hunks)packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json(1 hunks)packages/contracts-rfq/deployments/optimism/TokenZapV1.json(4 hunks)packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol(2 hunks)packages/contracts-rfq/test/integration/TokenZapV1.t.sol(2 hunks)packages/contracts-rfq/test/libs/ZapDataV1.t.sol(8 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.HighSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.NoSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.WithSlippage.t.sol(1 hunks)packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol(27 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol(7 hunks)packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol(32 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.t.sol(22 hunks)
💤 Files with no reviewable changes (1)
- packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouterErrors.sol
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.WithSlippage.t.sol
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.NoSlippage.t.sol
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.HighSlippage.t.sol
- packages/contracts-rfq/deployments/optimism/SynapseIntentPreviewer.json
- packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol
- packages/contracts-rfq/deployments/arbitrum/SynapseIntentPreviewer.json
- packages/contracts-rfq/test/router/SynapseIntentRouter.BalanceChecks.t.sol
- packages/contracts-rfq/test/libs/ZapDataV1.t.sol
👮 Files not reviewed due to content moderation or server errors (3)
- packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
- packages/contracts-rfq/contracts/router/SynapseIntentPreviewer.sol
- packages/contracts-rfq/test/router/SynapseIntentPreviewer.t.sol
🧰 Additional context used
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
[warning] 172-189: Dangerous strict equalities
TokenZapV1._checkBalanceAndForward(address,address,uint256) (contracts/zaps/TokenZapV1.sol#172-189) uses a dangerous strict equality:
- amount == 0 || amount < minFinalBalance (contracts/zaps/TokenZapV1.sol#181)
packages/contracts-rfq/contracts/libs/ZapDataV1.sol
[warning] 127-132: Assembly usage
ZapDataV1.minFinalBalance(bytes) (contracts/libs/ZapDataV1.sol#127-132) uses assembly
- INLINE ASM (contracts/libs/ZapDataV1.sol#129-131)
🔇 Additional comments (19)
packages/contracts-rfq/deployments/optimism/TokenZapV1.json (1)
2-6: LGTM! Contract deployment and ABI updates are consistent.The changes correctly reflect:
- Updated contract deployment details
- New
minFinalBalanceparameter inencodeZapData- New error types for validation
Also applies to: 83-87, 168-170, 171-174, 191-195, 201-205
packages/contracts-rfq/contracts/libs/ZapDataV1.sol (3)
17-19: LGTM! Struct layout and offsets are correctly defined.The changes maintain proper tight packing of the struct fields while adding the new
minFinalBalancefield.Also applies to: 22-27
91-98: LGTM! Comprehensive validation rules forfinalToken.The validation logic correctly ensures that
finalTokenis specified when eitherforwardToorminFinalBalanceis non-zero.
127-132: Assembly usage is justified for gas optimization.The assembly code follows the same pattern as other field accessors, optimizing gas usage when reading from calldata. The implementation is safe as it:
- Uses the correct offset calculation
- Properly loads the full 256 bits needed for uint256
🧰 Tools
🪛 GitHub Check: Slither
[warning] 127-132: Assembly usage
ZapDataV1.minFinalBalance(bytes) (contracts/libs/ZapDataV1.sol#127-132) uses assembly
- INLINE ASM (contracts/libs/ZapDataV1.sol#129-131)packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (4)
59-64: LGTM: Function signatures consistently updated to includeminFinalBalanceparameterThe changes consistently add the
minFinalBalanceparameter andforwardToaddress to theencodeZapDatacalls across all helper functions. This aligns with the PR objectives for implementing optional strict slippage.Also applies to: 81-82, 94-95, 116-117, 129-130
370-377: LGTM: Test coverage for zeroforwardToscenariosGood test coverage for scenarios where
forwardTois zero, verifying that tokens remain in the TokenZap contract both with and withoutminFinalBalancerequirements.Also applies to: 450-456, 458-464
660-664: LGTM: Test coverage for insufficient final balanceComprehensive test coverage for scenarios where the final balance is below the minimum required, verifying that the contract correctly reverts with
TokenZapV1__FinalBalanceBelowMin.Also applies to: 720-725
674-676: LGTM: Test coverage for zero final tokenGood test coverage for scenarios where the final token is not specified, verifying that the contract correctly reverts with
TokenZapV1__FinalTokenNotSpecified.Also applies to: 736-738, 762-764, 775-776
packages/contracts-rfq/test/router/SynapseIntentRouter.t.sol (3)
86-96: LGTM: Function signatures consistently updated to includeminFinalBalanceparameterThe changes consistently add the
minFinalBalanceparameter to thegetUnwrapZapDataandgetSwapZapDatafunctions. The placeholder zero amount in the payload is a good practice as it will be replaced with the actual amount during execution.Also applies to: 99-117
180-187: LGTM: Helper function for testing insufficient final balanceGood addition of the
checkRevertFinalBalanceInsufficienthelper function to verify that the contract correctly reverts when the final balance is below the minimum required.
363-365: LGTM: Test coverage for insufficient final balanceComprehensive test coverage for scenarios where the final balance is below the minimum required, across different swap and unwrap operations.
Also applies to: 395-398, 501-505, 543-547, 580-584
packages/contracts-rfq/deployments/arbitrum/TokenZapV1.json (4)
83-87: New parameter aligns with PR objectives.The addition of
minFinalBalanceparameter toencodeZapDataaligns with the PR's goal of implementing optional strict slippage. This parameter will allow for precise control over minimum output amounts, supporting both 0% and 100% slippage scenarios as described in the PR objectives.
168-170: Error handling improvements for balance and token validation.The renaming of
TokenZapV1__FinalTokenBalanceZerotoTokenZapV1__FinalBalanceBelowMinbetter reflects its purpose, and the addition ofTokenZapV1__FinalTokenNotSpecifiedimproves input validation.Also applies to: 171-174
191-195: Verify error handling consistency across contracts.New errors
ZapDataV1__FinalTokenNotSpecifiedandZapDataV1__PayloadLengthAboveMaxsuggest shared validation logic with the ZapDataV1 library. Let's verify the consistency of error handling across related contracts.Also applies to: 201-205
✅ Verification successful
Error handling is consistently implemented between TokenZapV1 and ZapDataV1
The verification shows that error handling is properly synchronized between the contracts:
ZapDataV1.soldefines the core validation errors:
ZapDataV1__FinalTokenNotSpecifiedZapDataV1__PayloadLengthAboveMaxZapDataV1__TargetZeroAddress- Plus library-specific errors:
ZapDataV1__InvalidEncoding,ZapDataV1__UnsupportedVersion
TokenZapV1.solmirrors these errors with its own namespace:
TokenZapV1__FinalTokenNotSpecifiedTokenZapV1__PayloadLengthAboveMaxTokenZapV1__TargetZeroAddress- Plus contract-specific error:
TokenZapV1__FinalBalanceBelowMinThis demonstrates proper error handling consistency where both contracts maintain their own namespaced versions of shared validation errors while also having their specific error cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent error handling across related contracts # Search for these error definitions in other contracts ast-grep --pattern 'error ZapDataV1__FinalTokenNotSpecified' ast-grep --pattern 'error ZapDataV1__PayloadLengthAboveMax' # Check for any other related error definitions rg -A 1 'error.*TokenZapV1__|error.*ZapDataV1__'Length of output: 1268
2-2: Verify contract deployment on Arbitrum.The contract has been redeployed to a new address with updated transaction details. Let's verify the deployment status.
Also applies to: 5-6
packages/contracts-rfq/contracts/interfaces/ISynapseIntentRouter.sol (1)
26-27: Clarify user responsibility for slippage checks
The newly added lines explicitly state that the router no longer performs slippage checks. This shift in responsibility ensures a more flexible design but could expose users to slippage risks if their Zap steps do not handle slippage correctly.Be sure to confirm with downstream code that slippage handling is performed where necessary to avoid unexpected losses in volatile markets.
packages/contracts-rfq/contracts/router/SynapseIntentRouter.sol (1)
44-44: Confirm the removal of minimum amount checks for last step
Line 44 now callscompleteIntentdirectly without validating the last step against a minimum amount threshold. While this simplifies logic, there's potential risk if the last step receives an unexpectedly small amount (e.g., in slippage extremes).Please confirm that this behavior meets all functional requirements and adequately protects users from scenarios in which amounts become insufficient in the final step.
packages/contracts-rfq/deployments/optimism/SynapseIntentRouter.json (1)
2-2: Validate updated deployment details
The contract address, transaction hash, and block number have changed in this JSON deployment file. Ensure these values reflect the newly deployed contract and confirm that the revision is correct, particularly if any external integrations rely on stable references to older addresses or on past transaction details.In addition, confirm that any dependent services, frontends, or scripts are updated to reference this new deployment address if they interact directly with the contract.
Also applies to: 5-6
packages/contracts-rfq/deployments/arbitrum/SynapseIntentRouter.json (1)
2-2: Check the addressed deployment consistency
As with the Optimism deployment, the updated address, transaction hash, and block number should be verified for correctness. Confirm that these details accurately represent the newly deployed contract on Arbitrum.Be sure to synchronize references in any environment variables, frontends, or documentation to this new address.
Also applies to: 5-6
| forwardTo: address(0), | ||
| minFinalBalance: 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for minFinalBalance parameter.
While the changes consistently add the new minFinalBalance parameter, all test scenarios use the default value (0). To ensure robust testing:
- Add test cases with non-zero
minFinalBalancevalues - Add test cases that trigger the new error condition
TokenZapV1__FinalBalanceBelowMin
Would you like me to help generate additional test cases to cover these scenarios?
Also applies to: 104-105, 113-114, 122-123, 130-131
Description
Updates the
SynapseIntentPreviewerto generate the intent steps for following scenarios:Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Technical Updates
The changes primarily focus on providing more granular control and safety mechanisms for token swaps and forwarding across different blockchain environments.