feat(contracts-rfq): Token Zap [SLT-389] #3352
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce several new Solidity files, including a library ( Changes
Possibly related PRs
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3352 +/- ##
===================================================
+ Coverage 3.10368% 42.32681% +39.22313%
===================================================
Files 44 87 +43
Lines 2932 3017 +85
Branches 0 82 +82
===================================================
+ Hits 91 1277 +1186
+ Misses 2838 1737 -1101
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Deploying sanguine-fe with
|
| Latest commit: |
678dcb5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://491cf1c4.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-token-zap.sanguine-fe.pages.dev |
ChiTimesChi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (11)
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (1)
7-9: Add NatSpec documentation for public functions
The functions lack documentation explaining their purpose, parameters, and expected behavior. This is particularly important for:
validateV1: What constitutes valid data?payload: How is the amount integrated into the encoded data?
Example NatSpec for payload:
/// @notice Generates the final payload by incorporating the actual amount into the encoded zap data
/// @param encodedZapData The pre-encoded zap data with amount placeholder
/// @param amount The actual amount to be used in the payload
/// @return The final payload with the actual amount insertedAlso applies to: 31-33
packages/contracts-rfq/test/mocks/VaultManyArguments.sol (2)
8-8: Consider using a more descriptive error name.
The error name VaultManyArguments__SomeError is vague. Consider renaming it to better describe the specific error condition it represents, such as VaultManyArguments__InvalidEncodedData since it's used in the _validateBytes function.
- error VaultManyArguments__SomeError();
+ error VaultManyArguments__InvalidEncodedData();30-32: Remove unnecessary payable modifier.
Since this function always reverts, the payable modifier is unnecessary.
- function depositWithRevert() external payable {
+ function depositWithRevert() external {
revert VaultManyArguments__SomeError();
}packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1)
15-23: Consider adding comments explaining the magic numbers.
While the constants are well-defined, it would be helpful to add comments explaining:
- The significance of the chain IDs (1337 and 7331)
- The reasoning behind the specific amounts (1 ether and 0.9999 ether)
- The meaning of ZAP_NATIVE value (123_456)
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (2)
11-30: Consider adding more test scenarios in setUp.
While the basic setup is good, consider adding test scenarios with:
- Tokens with different decimals (e.g., 6, 8, 18)
- Multiple users for testing access control
- Edge case amounts (e.g., very small/large values)
101-143: Add gas usage assertions for native token operations.
The native token tests should include gas usage assertions to ensure optimal gas consumption, especially important for bridging operations.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol (2)
98-101: Ensure consistent naming convention for test functions
The function test_relay_depositTokenRevertParams_revert contains _revert twice in its name. Consider renaming it for clarity and consistency.
Apply this diff to rename the function:
-function test_relay_depositTokenRevertParams_revert() public {
+function test_relay_depositTokenRevert() public {115-118: Ensure consistent naming convention for test functions
The function test_relay_depositNativeRevertParams_revert contains _revert twice in its name. Consider renaming it for clarity and consistency.
Apply this diff to rename the function:
-function test_relay_depositNativeRevertParams_revert() public {
+function test_relay_depositNativeRevert() public {packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (1)
14-14: Document the purpose of NATIVE_GAS_TOKEN constant
While the placeholder address 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE is often used to represent native gas tokens, adding a comment or NatSpec documentation explaining its usage can improve code clarity.
packages/contracts-rfq/test/libs/ZapDataV1.t.sol (2)
51-51: Use EXPECTED_VERSION constant in version assertions
To enhance maintainability and consistency, consider using the EXPECTED_VERSION constant instead of hardcoding the version number in the assertEq statements.
Apply this diff to update the assertions:
- assertEq(harness.version(zapData), 1);
+ assertEq(harness.version(zapData), EXPECTED_VERSION);Also applies to: 65-65
106-106: Use EXPECTED_VERSION constant in version checks
Replace the hardcoded version number with the EXPECTED_VERSION constant in the vm.assume statements to improve consistency.
Apply this diff to update the version checks:
- vm.assume(version != 1);
+ vm.assume(version != EXPECTED_VERSION);Also applies to: 125-125
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- packages/contracts-rfq/contracts/libs/ZapDataV1.sol (1 hunks)
- packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (1 hunks)
- packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (1 hunks)
- packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol (1 hunks)
- packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1 hunks)
- packages/contracts-rfq/test/libs/ZapDataV1.t.sol (1 hunks)
- packages/contracts-rfq/test/mocks/VaultManyArguments.sol (1 hunks)
- packages/contracts-rfq/test/mocks/VaultMock.sol (1 hunks)
- packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-10-14T14:48:01.520Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
[warning] 28-48: Unused return
TokenZapV1.zap(address,uint256,bytes) (contracts/zaps/TokenZapV1.sol#28-48) ignores return value by Address.functionCallWithValue({target:target,data:payload,value:msg.value}) (contracts/zaps/TokenZapV1.sol#46)
🔇 Additional comments (30)
packages/contracts-rfq/test/mocks/VaultMock.sol (3)
1-8: LGTM: Contract setup follows best practices.
The contract correctly:
- Specifies SPDX license
- Uses a recent Solidity version
- Imports SafeERC20 for secure token handling
- Is marked as abstract as intended for inheritance
- Uses proper OpenZeppelin imports
10-12: 🛠️ Refactor suggestion
Consider using ImmutableX pattern for NATIVE_GAS_TOKEN.
While the current implementation is functional, consider using the immutable pattern for the NATIVE_GAS_TOKEN address to save gas in test environments:
- address internal constant NATIVE_GAS_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
+ address internal immutable NATIVE_GAS_TOKEN;Then initialize it in the constructor. This would allow for easier testing with different native token addresses if needed.
Likely invalid or redundant comment.
16-23: Verify reentrancy protection in inheriting contracts.
The _deposit function handles external calls (safeTransferFrom) before state changes. While this follows CEI (Checks-Effects-Interactions) pattern internally, inheriting contracts need to ensure they maintain reentrancy protection when implementing deposit logic.
Additionally, consider adding events for deposits to aid in testing and monitoring:
+ event Deposited(address indexed user, address indexed token, uint256 amount);
function _deposit(address user, address token, uint256 amount) internal {
if (token == NATIVE_GAS_TOKEN) {
if (msg.value != amount) revert VaultMock__AmountIncorrect();
} else {
IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
}
balanceOf[user][token] += amount;
+ emit Deposited(user, token, amount);
}Let's verify the inheritance chain to ensure proper reentrancy protection:
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (3)
1-4: Verify Solidity version compatibility across deployment chains
The contract uses Solidity 0.8.24, which is a very recent version. While this version includes important security features and optimizations, ensure that all target deployment chains (mentioned in PR objectives: ETH, BNB, AVAX) have full support for this version.
6-6: LGTM! Well-structured test harness
The contract follows best practices for test harnesses:
- Clear naming convention
- Single responsibility (testing ZapDataV1 library)
- Pure functions for deterministic testing
11-21: 🛠️ Refactor suggestion
Consider adding input validation for encodeV1
The encodeV1 function accepts parameters without validation. Consider adding checks for:
amountPosition_bounds relative topayload_lengthtarget_being a non-zero addresspayload_minimum length
packages/contracts-rfq/test/mocks/VaultManyArguments.sol (1)
1-7: LGTM! Clear contract setup with appropriate warnings.
The contract follows best practices with proper license, pragma version, and clear documentation about its testing purpose.
packages/contracts-rfq/test/integration/TokenZapV1.t.sol (4)
1-13: LGTM! Well-organized imports and appropriate pragma version.
The file structure follows best practices with clear organization of imports and uses a stable Solidity version.
25-44: LGTM! Well-organized state variables.
The state variables are logically grouped and use appropriate visibility modifiers.
119-159: LGTM! Well-implemented utility functions.
The utility functions make good use of Solidity's encoding functions and provide clear interfaces for test scenarios.
46-58: Verify the implementation of mintTokens().
The mintTokens() function is declared but not implemented in this abstract contract. Ensure that all concrete implementations provide the necessary token minting logic.
✅ Verification successful
mintTokens() is correctly implemented across test hierarchy
The abstract mintTokens() function in TokenZapV1.t.sol is properly implemented by its concrete test cases:
FastBridgeV2.TokenZapV1.Src.t.sol: Implements minting for source chain testingFastBridgeV2.TokenZapV1.Dst.t.sol: Implements minting for destination chain testing
Both implementations provide the necessary token minting logic and state setup required for their respective test scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for concrete implementations of TokenZapV1IntegrationTest
ast-grep --pattern 'contract $_ is TokenZapV1IntegrationTest {
$$$
function mintTokens() $_ {
$$$
}
$$$
}'
Length of output: 120
Script:
#!/bin/bash
# Let's first find the base test contract
fd "TokenZapV1IntegrationTest" --type f
# Then let's look for any implementations of mintTokens
rg "function mintTokens" -A 5
Length of output: 5670
packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (4)
1-9: LGTM: Imports and pragma are appropriate.
The pragma version is up-to-date (0.8.24) and all necessary test dependencies are imported.
145-165: LGTM: Comprehensive encoding/decoding tests.
The roundtrip tests effectively verify the encoding/decoding logic with both standard and edge cases.
62-99: Consider testing with fee-on-transfer tokens.
The ERC20 tests cover standard scenarios but miss some important edge cases:
- Fee-on-transfer tokens
- Rebasing tokens
- Tokens with transfer hooks
1-212: Verify integration with FastBridgeV2.
Given the PR objective of integrating with FastBridgeV2, we should verify the interaction points.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (7)
8-18: Well-defined BridgeRequested event
The BridgeRequested event is properly structured with appropriate indexed parameters, accurately capturing all necessary details for the bridging operation.
20-23: Correct initialization of test environment
The setUp function correctly sets the chain ID using vm.chainId(SRC_CHAIN_ID) before invoking the parent setUp(), ensuring the test environment mirrors the source chain.
25-30: Accurate token minting and approval
The mintTokens function effectively mints the required tokens to the user and sets the necessary approval for the fastBridge contract to handle the tokens.
32-41: Proper simulation of user-initiated bridge transactions
The bridge function appropriately uses vm.prank to simulate a transaction from the user's perspective, correctly handling both token and native asset transfers based on the isToken flag.
43-64: Accurate event expectation setup for testing
The expectEventBridgeRequested function correctly encodes the bridge transaction and sets up the expected event emission, ensuring that the tests can accurately verify event logs.
66-74: Effective balance verification after bridging
The checkBalances function accurately verifies that the user's balance decreases and the fastBridge contract's balance increases appropriately, confirming the correct transfer of assets during the bridge operation.
76-110: Comprehensive test coverage for bridging scenarios
The suite of test functions provides thorough coverage of various bridging scenarios, including token and native asset transfers, handling of deposit parameters, and revert cases. This ensures robust validation of the bridging functionality.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol (5)
20-23: Initialization in setUp function is correct
The chain ID is correctly set, and the base setup is invoked appropriately.
25-30: Token minting and approval logic is appropriate in mintTokens
The relayer's balance is correctly set up, and token approvals are properly configured.
32-42: relay function implementation correctly handles transaction relaying
The function encodes the bridge transaction and correctly invokes the fastBridge.relay function with appropriate parameters.
44-64: Event expectation in expectEventBridgeRelayed is properly set up
The function correctly configures the expected event emission with all the required parameters.
66-82: Balance verification in checkBalances function is thorough
The function accurately asserts balances based on whether tokens or native assets are used.
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (2)
74-81: Verify that amountPosition and payload.length fit within uint16 bounds
When casting amountPosition to uint16, ensure that both amountPosition and payload.length do not exceed the maximum value of uint16 to prevent truncation or overflow errors.
28-48: zap function implementation looks correct
The zap function properly validates the Zap data, handles token approvals, manages native gas tokens, constructs the payload with the correct amount, and forwards the call to the target contract with the appropriate msg.value.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 28-48: Unused return
TokenZapV1.zap(address,uint256,bytes) (contracts/zaps/TokenZapV1.sol#28-48) ignores return value by Address.functionCallWithValue({target:target,data:payload,value:msg.value}) (contracts/zaps/TokenZapV1.sol#46)
packages/contracts-rfq/test/libs/ZapDataV1.t.sol (1)
9-143: Comprehensive test coverage and clean implementation
The test contract provides thorough coverage of various scenarios for the ZapDataV1 library, including both valid and invalid cases. The implementation is clean and follows best practices, ensuring robust validation of encoding and decoding functionalities.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (14)
packages/contracts-rfq/test/mocks/VaultMock.sol (1)
10-12: Consider adding documentation for NATIVE_GAS_TOKEN.
While the sentinel value 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE is commonly used to represent native gas tokens (ETH/BNB/AVAX), it would be helpful to document this convention for clarity.
- address internal constant NATIVE_GAS_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
+ /// @dev Sentinel value representing native gas tokens (ETH/BNB/AVAX)
+ /// This is a widely adopted convention in DeFi protocols
+ address internal constant NATIVE_GAS_TOKEN = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (2)
6-6: Add NatSpec documentation for the contract.
While the contract structure is good, adding NatSpec documentation would improve clarity about its testing purpose and relationship with the ZapDataV1 library.
+/// @title ZapDataV1Harness
+/// @notice Test harness for exposing internal functions of ZapDataV1 library
+/// @dev Used for unit testing the encoding and validation of zap data
contract ZapDataV1Harness {7-9: Add NatSpec documentation for public functions.
While the functions are straightforward wrappers, adding NatSpec documentation would improve testability and maintainability.
+ /// @notice Validates the encoded zap data format
+ /// @param encodedZapData The encoded zap data to validate
function validateV1(bytes calldata encodedZapData) public pure {
ZapDataV1.validateV1(encodedZapData);
}
+ /// @notice Encodes zap data into a bytes array
+ /// @param amountPosition_ Position where amount should be inserted
+ /// @param target_ Target contract address
+ /// @param payload_ Function call payload
+ /// @return encodedZapData The encoded zap data
function encodeV1(
uint16 amountPosition_,
address target_,
bytes memory payload_
)Also applies to: 11-21
packages/contracts-rfq/test/mocks/VaultManyArguments.sol (2)
26-28: Add validation for zero value deposits.
Consider adding a check to ensure msg.value > 0 to prevent unnecessary zero-value transactions.
function depositNoAmount(address user) external payable {
+ if (msg.value == 0) revert VaultManyArguments__SomeError();
_deposit(user, NATIVE_GAS_TOKEN, msg.value);
}30-32: Remove unnecessary payable modifier.
Since this function always reverts and doesn't use msg.value, the payable modifier is unnecessary.
- function depositWithRevert() external payable {
+ function depositWithRevert() external {
revert VaultManyArguments__SomeError();
}packages/contracts-rfq/test/integration/TokenZapV1.t.sol (3)
23-23: Consider documenting the ZAP_NATIVE constant.
The magic number 123_456 would benefit from a comment explaining its significance in the test scenarios.
- uint256 internal constant ZAP_NATIVE = 123_456;
+ // Arbitrary amount of native tokens to be sent along with the zap transaction
+ uint256 internal constant ZAP_NATIVE = 123_456;46-58: Consider adding deployment validations in setUp.
While the setup is well-structured, consider adding assertions to verify successful contract deployments and role assignments.
function setUp() public virtual {
fastBridge = new FastBridgeV2(address(this));
+ require(address(fastBridge) != address(0), "FastBridge deployment failed");
fastBridge.grantRole(fastBridge.RELAYER_ROLE(), relayer);
+ require(fastBridge.hasRole(fastBridge.RELAYER_ROLE(), relayer), "Role assignment failed");149-159: Add documentation for deposit payload helper functions.
The helper functions would benefit from NatSpec documentation explaining their purpose and parameters.
+ /// @notice Creates a payload for depositing tokens into the vault
+ /// @param token The token address to deposit (can be native token address)
+ /// @return The encoded deposit function call
function getDepositPayload(address token) public view returns (bytes memory) {
return abi.encodeCall(dstVault.deposit, (token, abi.encode(token), DST_AMOUNT, user, abi.encode(user)));
}
+ /// @notice Creates a payload for deposits that don't require an amount parameter
+ /// @return The encoded deposit function call
function getDepositNoAmountPayload() public view returns (bytes memory) {
return abi.encodeCall(dstVault.depositNoAmount, (user));
}
+ /// @notice Creates a payload that will cause the deposit to revert (for testing)
+ /// @return The encoded deposit function call
function getDepositRevertPayload() public view returns (bytes memory) {
return abi.encodeCall(dstVault.depositWithRevert, ());
}packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (5)
12-12: Document test constants and magic values.
Consider adding comments to explain:
- The significance of the
AMOUNTvalue (0.987 ether) - The special address
0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeEused for native gas tokens
- uint256 internal constant AMOUNT = 0.987 ether;
+ /// @dev Non-round amount to ensure proper decimal handling
+ uint256 internal constant AMOUNT = 0.987 ether;
- address internal nativeGasToken = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;
+ /// @dev Special address representing native gas tokens (ETH/BNB/AVAX)
+ address internal nativeGasToken = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;Also applies to: 19-19
44-47: Document the calculation of amount position in payload.
The magic number 4 + 32 * 2 needs explanation. It represents the position of the amount parameter in the encoded function call.
function getZapData(bytes memory originalPayload) public view returns (bytes memory) {
- // Amount is the third argument of the deposit function
+ // Position calculation:
+ // 4 bytes: function selector
+ // 32 bytes: first parameter (token address)
+ // 32 bytes: second parameter (encoded token)
+ // Next 32 bytes: amount parameter
return tokenZap.encodeZapData(address(vault), originalPayload, 4 + 32 * 2);
}62-99: Add test case for zero amount zap.
The test suite should verify that the contract correctly handles zero amount transactions, as they might be valid in some protocols.
Consider adding:
function test_zap_erc20_zeroAmount() public {
bytes memory zapData = getZapData(getVaultPayload(address(erc20), 0));
bytes4 returnValue = tokenZap.zap(address(erc20), 0, zapData);
assertEq(returnValue, tokenZap.zap.selector);
assertEq(vault.balanceOf(user, address(erc20)), 0);
}101-143: Add test for handling unexpected native token receipts.
Consider adding a test case where the contract receives native tokens through selfdestruct to ensure it doesn't affect the zap operation.
Consider adding:
function test_zap_native_selfdestructFunds() public {
// Setup a contract that can selfdestruct and send ETH
address payable selfDestructContract = payable(makeAddr("selfDestruct"));
deal(selfDestructContract, AMOUNT);
// Receive unexpected funds
vm.prank(selfDestructContract);
(bool success,) = address(tokenZap).call{value: AMOUNT}("");
require(success, "Transfer failed");
// Should not affect normal zap operation
test_zap_native_placeholderZero();
}145-165: Add specific edge cases to encoding tests.
While the property-based test is good, consider adding specific edge cases like max uint256 values and special addresses.
Consider adding:
function test_encodeZapData_maxValues() public {
address maxAddress = address(uint160(type(uint160).max));
uint256 maxAmount = type(uint256).max;
test_encodeZapData_roundtrip(maxAddress, maxAmount, maxAmount);
}packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (1)
16-17: Enhance error messages for better debugging
The custom errors TokenZapV1__AmountIncorrect and TokenZapV1__PayloadLengthAboveMax could include additional information to aid in debugging, such as the expected versus actual amounts or payload lengths.
Consider modifying the errors to include more context:
-error TokenZapV1__AmountIncorrect();
+error TokenZapV1__AmountIncorrect(uint256 expected, uint256 actual);
-error TokenZapV1__PayloadLengthAboveMax();
+error TokenZapV1__PayloadLengthAboveMax(uint256 maxLength, uint256 actualLength);And update where the errors are used accordingly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- packages/contracts-rfq/contracts/libs/ZapDataV1.sol (1 hunks)
- packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (1 hunks)
- packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (1 hunks)
- packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol (1 hunks)
- packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (1 hunks)
- packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1 hunks)
- packages/contracts-rfq/test/libs/ZapDataV1.t.sol (1 hunks)
- packages/contracts-rfq/test/mocks/VaultManyArguments.sol (1 hunks)
- packages/contracts-rfq/test/mocks/VaultMock.sol (1 hunks)
- packages/contracts-rfq/test/zaps/TokenZapV1.t.sol (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (1)
Learnt from: ChiTimesChi
PR: synapsecns/sanguine#3284
File: packages/contracts-rfq/test/FastBridgeV2.Src.t.sol:919-993
Timestamp: 2024-10-14T14:48:01.520Z
Learning: In Solidity test files for FastBridgeV2 (e.g., `packages/contracts-rfq/test/FastBridgeV2.Src.t.sol`), code duplication in test functions is acceptable to maintain readability and maintainability, even if it contradicts DRY principles.
🪛 GitHub Check: Slither
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol
[warning] 28-48: Unused return
TokenZapV1.zap(address,uint256,bytes) (contracts/zaps/TokenZapV1.sol#28-48) ignores return value by Address.functionCallWithValue({target:target,data:payload,value:msg.value}) (contracts/zaps/TokenZapV1.sol#46)
🔇 Additional comments (25)
packages/contracts-rfq/test/mocks/VaultMock.sol (1)
1-8: LGTM! Solid contract foundation.
The contract setup follows best practices with:
- Recent Solidity version
- Proper use of SafeERC20 for token operations
- Appropriate abstract modifier for a base contract
packages/contracts-rfq/test/harnesses/ZapDataV1Harness.sol (2)
1-4: LGTM! Well-structured file setup.
The file uses appropriate licensing, latest stable Solidity version, and correct import syntax.
31-33: Verify amount parameter bounds.
The payload function accepts an unbounded uint256 amount parameter. Consider adding validation to ensure the amount fits within the expected bounds of the target protocol.
packages/contracts-rfq/test/mocks/VaultManyArguments.sol (2)
1-8: Well-structured contract setup with clear documentation.
The contract is properly documented with a clear warning about its test-only nature. Good practice to explicitly state "DO NOT USE IN PRODUCTION" for mock contracts.
10-24: Add input validation for edge cases.
While the function validates encoded data, consider adding these safety checks:
- Zero address validation for
tokenanduser - When
tokenis native gas token, validate thatamountmatchesmsg.value - When
tokenis ERC20, ensuremsg.valueis zero
Let's check if VaultMock has these validations:
packages/contracts-rfq/test/integration/TokenZapV1.t.sol (1)
1-14: LGTM! Well-structured imports and setup.
The file uses the latest Solidity version and has a clean organization of imports, properly separating core contracts from test mocks.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Src.t.sol (7)
7-18: Well-Defined Event BridgeRequested
The declaration of the BridgeRequested event accurately captures all necessary parameters for logging bridge requests, facilitating effective event handling and debugging.
20-23: Proper Override of setUp Function
The setUp function correctly overrides the parent function, sets the chain ID using vm.chainId(SRC_CHAIN_ID), and calls super.setUp() to ensure proper test environment initialization.
25-30: Correct Token Minting and Approval in mintTokens
The mintTokens function effectively mints tokens to the user and sets up the necessary token approvals for the fastBridge contract. The use of vm.prank(user) correctly simulates the user's context for the approval transaction.
32-41: Accurate Implementation of bridge Function
The bridge function accurately handles the bridging logic by pranking as the user and invoking fastBridge.bridge with the correct parameters and value based on the isToken flag.
43-64: Proper Setup for Event Expectation in expectEventBridgeRequested
The expectEventBridgeRequested function correctly encodes the bridge transaction, computes the transaction ID, and sets up the expected event emission using vm.expectEmit. The event parameters are precisely constructed to match the expected BridgeRequested event.
66-74: Effective Balance Verification in checkBalances
The checkBalances function accurately asserts the user's and fastBridge's balances for both token and native asset scenarios, ensuring the correctness of the bridging operation's impact on balances.
76-110: Comprehensive Test Cases Covering Various Bridging Scenarios
The test functions (test_bridge_depositTokenParams, test_bridge_depositTokenWithZapNativeParams, etc.) thoroughly cover different bridging scenarios, including token deposits, native asset deposits, and cases with various parameters. Each test function correctly sets up expectations, invokes the bridge, and verifies balances.
packages/contracts-rfq/test/integration/FastBridgeV2.TokenZapV1.Dst.t.sol (6)
8-18: Event BridgeRelayed is correctly defined
The BridgeRelayed event is correctly declared with appropriate indexed parameters for efficient filtering and logging of bridge transactions.
20-23: setUp function correctly initializes test environment
The setUp function properly overrides the parent setup, sets the chain ID to DST_CHAIN_ID, and ensures the test environment is correctly configured.
25-30: mintTokens function effectively prepares token balances
The mintTokens function successfully mints tokens to the relayer and sets up the necessary approvals for the fastBridge contract.
66-82: Balance assertions accurately validate fund transfers
The checkBalances function correctly asserts the balances of involved addresses, ensuring that tokens or native assets are precisely transferred and accounted for in the dstVault.
84-118: Test functions provide comprehensive coverage of relay scenarios
The test functions effectively cover various scenarios for both token and native asset transfers, including:
- Successful deposits with tokens and native assets.
- Testing with additional
zapNativeamounts. - Expected reverts when interacting with the
VaultManyArgumentscontract.
This comprehensive testing ensures robust validation of the relay functionality.
32-42:
Verify Ether value sent in fastBridge.relay when relaying tokens
In the relay function, when isToken is true, the call to fastBridge.relay sends paramsV2.zapNative as value. This could result in unintended Ether being sent alongside token transfers. Please verify if this behavior is intentional and aligns with the expected functionality.
If unintentional, consider adjusting the value assignment:
fastBridge.relay{value: isToken ? paramsV2.zapNative : DST_AMOUNT}(encodedBridgeTx);To:
fastBridge.relay{value: isToken ? 0 : DST_AMOUNT}(encodedBridgeTx);packages/contracts-rfq/contracts/libs/ZapDataV1.sol (6)
27-35: Proper validation in validateV1 function
The validateV1 function effectively ensures that the encoded ZapData is valid by checking the length and confirming the version matches the expected constant. This safeguards against improperly formatted data.
37-65: Efficient encoding in encodeV1 function
The encodeV1 function correctly encodes the ZapData struct by tightly packing the fields. It includes appropriate validation to ensure the amountPosition_ is within the bounds of the payload_, preventing potential out-of-bounds errors.
67-73: Accurate version extraction in version function
The inline assembly in the version function correctly extracts the version field from the encoded data. The use of assembly here is justified for efficient data retrieval.
75-81: Correct target address retrieval in target function
The target function uses inline assembly to accurately extract the target address from the encoded ZapData. This implementation is efficient and reliable.
83-108: Robust payload handling in payload function
The payload function properly handles the case where the amount is present or not in the original payload. It safely replaces the placeholder amount with the actual amount when necessary, ensuring the integrity of the data passed to the target contract.
110-117: Safe extraction of amount position in _amountPosition function
The inline assembly in _amountPosition correctly retrieves the amount position from the encoded data. This aids in accurately modifying the payload when replacing the token amount.
| function checkERC20HappyPath(bytes memory zapData, uint256 msgValue) public { | ||
| // Transfer tokens to the zap contract first | ||
| erc20.transfer(address(tokenZap), AMOUNT); | ||
| bytes4 returnValue = tokenZap.zap{value: msgValue}(address(erc20), AMOUNT, zapData); | ||
| assertEq(returnValue, tokenZap.zap.selector); | ||
| // Check that the vault registered the deposit | ||
| assertEq(vault.balanceOf(user, address(erc20)), AMOUNT); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance ERC20 test utility with balance checks.
Consider adding initial balance checks to ensure test preconditions and verify that no tokens remain in the zap contract after the operation.
function checkERC20HappyPath(bytes memory zapData, uint256 msgValue) public {
+ uint256 initialBalance = erc20.balanceOf(address(tokenZap));
// Transfer tokens to the zap contract first
erc20.transfer(address(tokenZap), AMOUNT);
+ assertEq(erc20.balanceOf(address(tokenZap)), initialBalance + AMOUNT, "Pre-zap balance check failed");
bytes4 returnValue = tokenZap.zap{value: msgValue}(address(erc20), AMOUNT, zapData);
assertEq(returnValue, tokenZap.zap.selector);
// Check that the vault registered the deposit
assertEq(vault.balanceOf(user, address(erc20)), AMOUNT);
+ assertEq(erc20.balanceOf(address(tokenZap)), initialBalance, "Post-zap balance check failed");
}📝 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 checkERC20HappyPath(bytes memory zapData, uint256 msgValue) public { | |
| // Transfer tokens to the zap contract first | |
| erc20.transfer(address(tokenZap), AMOUNT); | |
| bytes4 returnValue = tokenZap.zap{value: msgValue}(address(erc20), AMOUNT, zapData); | |
| assertEq(returnValue, tokenZap.zap.selector); | |
| // Check that the vault registered the deposit | |
| assertEq(vault.balanceOf(user, address(erc20)), AMOUNT); | |
| } | |
| function checkERC20HappyPath(bytes memory zapData, uint256 msgValue) public { | |
| uint256 initialBalance = erc20.balanceOf(address(tokenZap)); | |
| // Transfer tokens to the zap contract first | |
| erc20.transfer(address(tokenZap), AMOUNT); | |
| assertEq(erc20.balanceOf(address(tokenZap)), initialBalance + AMOUNT, "Pre-zap balance check failed"); | |
| bytes4 returnValue = tokenZap.zap{value: msgValue}(address(erc20), AMOUNT, zapData); | |
| assertEq(returnValue, tokenZap.zap.selector); | |
| // Check that the vault registered the deposit | |
| assertEq(vault.balanceOf(user, address(erc20)), AMOUNT); | |
| assertEq(erc20.balanceOf(address(tokenZap)), initialBalance, "Post-zap balance check failed"); | |
| } |
| function zap(address token, uint256 amount, bytes calldata zapData) external payable returns (bytes4) { | ||
| // Check that the ZapData is valid before decoding it | ||
| zapData.validateV1(); | ||
| address target = zapData.target(); | ||
| // Approve the target contract to spend the token. TokenZapV1 does not custody any tokens outside of the | ||
| // zap action, so we can approve the arbitrary target contract. | ||
| if (token == NATIVE_GAS_TOKEN) { | ||
| // No approvals are needed for the native gas token, just check that the amount is correct | ||
| if (msg.value != amount) revert TokenZapV1__AmountIncorrect(); | ||
| } else { | ||
| // Issue the approval only if the current allowance is less than the required amount | ||
| if (IERC20(token).allowance(address(this), target) < amount) { | ||
| IERC20(token).forceApprove(target, type(uint256).max); | ||
| } | ||
| } | ||
| // Perform the Zap action, forwarding full msg.value to the target contract | ||
| // Note: this will bubble up any revert from the target contract | ||
| bytes memory payload = zapData.payload(amount); | ||
| Address.functionCallWithValue({target: target, data: payload, value: msg.value}); | ||
| return this.zap.selector; | ||
| } |
There was a problem hiding this comment.
Consider adding reentrancy protection to the zap function
The zap function allows external calls to arbitrary contracts via functionCallWithValue. To prevent potential reentrancy attacks, it's advisable to add reentrancy protection using OpenZeppelin's ReentrancyGuard.
Apply this diff to add reentrancy protection:
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-contract TokenZapV1 is IZapRecipient {
+contract TokenZapV1 is IZapRecipient, ReentrancyGuard {
-function zap(address token, uint256 amount, bytes calldata zapData) external payable returns (bytes4) {
+function zap(address token, uint256 amount, bytes calldata zapData) external payable nonReentrant returns (bytes4) {Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 28-48: Unused return
TokenZapV1.zap(address,uint256,bytes) (contracts/zaps/TokenZapV1.sol#28-48) ignores return value by Address.functionCallWithValue({target:target,data:payload,value:msg.value}) (contracts/zaps/TokenZapV1.sol#46)
🛠️ Refactor suggestion
Emit an event after successful zap execution
Emitting an event upon successful execution of the zap function can improve transparency and facilitate off-chain tracking and debugging.
Apply this diff to add an event and emit it:
+event ZapExecuted(address indexed sender, address token, uint256 amount, address target);
function zap(address token, uint256 amount, bytes calldata zapData) external payable nonReentrant returns (bytes4) {
// existing code
Address.functionCallWithValue({target: target, data: payload, value: msg.value});
+ emit ZapExecuted(msg.sender, token, amount, target);
return this.zap.selector;
}Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Slither
[warning] 28-48: Unused return
TokenZapV1.zap(address,uint256,bytes) (contracts/zaps/TokenZapV1.sol#28-48) ignores return value by Address.functionCallWithValue({target:target,data:payload,value:msg.value}) (contracts/zaps/TokenZapV1.sol#46)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol(1 hunks)
🔇 Additional comments (1)
packages/contracts-rfq/contracts/zaps/TokenZapV1.sol (1)
103-114: decodeZapData Function Implementation is Correct
The decodeZapData function effectively decodes the Zap Data and replaces the placeholder amount with the actual amount when necessary. The use of validateV1 ensures that the zapData is correctly formatted, enhancing reliability and security.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
github-actions
bot
added
needs-go-generate-services/rfq
and removed
needs-go-generate-services/rfq
labels
ChiTimesChi
force-pushed
the
feat/token-zap
branch
from
9a0acdb to
678dcb5
Compare
Description
Minimal implementation of a contract that facilitates the generic Zap action for ERC20 tokens or native gas tokens (ETH/BNB/AVAX/others).
This enables atomic workflows like:
FastBridgeV2.A single permisionless
TokenZapcontract could be used for any DeFi protocol and any FastBridge (V2+) deployment, assuming:Full Workflow
Let's suppose Alice wants to deposit USDC into a Vault contract on Arbitrum. Alice has 100 USDC to start with on Optimism. The Arbitrum Vault exposes a following function to make the deposits:
depositFunds(address token, uint256 amount, address onBehalfOf). Following steps need to be done (abstracted away from the Alice by the FE):payload = abi.encodeCall(vault.depositFunds, (arbUSDC, 0, alice))amountwithinpayload:amountPosition = 4 + 32 * 1 = 36TokenZapexposes an off-chain getter:zapData = tokenZap.encodeZapData(arbVault, payload, amountPosition)zapNative = 0- this should be changed only when a Zap with ERC20 requires an additional native payment (e.g. token bridged via native bridge)dest_amountfrom step (5).Summary by CodeRabbit
Summary by CodeRabbit
New Features
ZapDataV1library for efficient data management and validation.TokenZapV1contract enabling token "Zap" actions with secure transfers.FastBridgeV2functionality related to token bridging.VaultManyArgumentsmock contract for enhanced testing scenarios.Bug Fixes
TokenZapV1for incorrect amounts and payload lengths.Tests
TokenZapV1andZapDataV1ensuring functionality and error handling.