test(contracts-rfq): gas bench for TokenZapV1
#3406
Conversation
WalkthroughThe changes introduce three new Solidity contracts aimed at enhancing testing capabilities for the vault system. The Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
packages/contracts-rfq/test/mocks/VaultManyArguments.sol (3)
11-12: Consider renaming the test exclusion functionWhile the empty function pattern for coverage exclusion is good, the name
testSimpleVaultMockmight be misleading as it's not actually testing anything related to SimpleVaultMock. Consider a more generic name likeexcludeFromCoverage.- function testSimpleVaultMock() external {} + function excludeFromCoverage() external {}
Line range hint
14-27: Add NatSpec documentation for function parametersThe deposit function lacks parameter documentation. Consider adding
@paramdescriptions to clarify the purpose of each parameter, especially for the encoded parameters.+ /// @param token The token address to deposit + /// @param encodedToken The ABI encoded token address for validation + /// @param amount The amount to deposit + /// @param user The user address for the deposit + /// @param encodedUser The ABI encoded user address for validation function deposit( address token, bytes memory encodedToken, uint256 amount, address user, bytes memory encodedUser )
Line range hint
29-42: Consider adding events and optimizing validation
- Consider emitting events in the deposit functions to track state changes, even in test contracts.
- The
_validateBytesfunction useskeccak256for comparison which is gas intensive. For testing purposes, consider using a simpler validation method.+ event Deposit(address indexed user, address indexed token, uint256 amount); function depositNoAmount(address user) external payable { _deposit(user, NATIVE_GAS_TOKEN, msg.value); + emit Deposit(user, NATIVE_GAS_TOKEN, msg.value); } function _validateBytes(address addr, bytes memory encoded) internal pure { - if (keccak256(abi.encode(addr)) != keccak256(encoded)) revert VaultManyArguments__SomeError(); + // For test contracts, we can use a simpler comparison + if (abi.encode(addr) != encoded) revert VaultManyArguments__SomeError(); }packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (1)
13-20: Consider documenting constants and using representative test amounts.Two suggestions for improvement:
- Add a comment explaining the significance of the
nativeGasTokenaddress (0xEeee...)- Consider if 0.1337 ether is a representative amount for gas benchmarking - you might want to test with different amounts to measure gas scaling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
packages/contracts-rfq/test/mocks/SimpleVaultMock.sol(1 hunks)packages/contracts-rfq/test/mocks/VaultManyArguments.sol(1 hunks)packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol(1 hunks)
🔇 Additional comments (7)
packages/contracts-rfq/test/mocks/SimpleVaultMock.sol (4)
1-4: LGTM! Appropriate license and modern Solidity version
The file uses a secure Solidity version and has proper licensing.
9-10: LGTM! Well-documented test exclusion pattern
The empty test function is properly documented and serves its purpose for coverage exclusion.
6-8: Verify the VaultMock implementation
The inheritance looks correct, but let's verify the base contract implementation to ensure proper integration.
✅ Verification successful
Base contract implementation is appropriate for testing purposes
The VaultMock implementation is a well-structured abstract contract that:
- Clearly documents its test-only purpose
- Implements basic vault functionality with deposit and balance tracking
- Uses SafeERC20 for secure token transfers
- Handles both ERC20 and native gas token deposits
- Has proper error handling
The inheritance in SimpleVaultMock is correct and safe to use for testing purposes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check VaultMock implementation and its inheritance chain
# Find and display VaultMock contract content
rg -p "contract VaultMock" --type sol
ast-grep --pattern 'contract VaultMock {
$$$
}'
Length of output: 119
Script:
#!/bin/bash
# Search for VaultMock contract definition using ripgrep without file type filter
rg -p "contract VaultMock"
# Find all Solidity files that might contain VaultMock
fd -e sol
# Search for imports of VaultMock to trace its location
rg "import.*VaultMock"
Length of output: 18975
Script:
#!/bin/bash
# Get the content of VaultMock.sol
cat "packages/contracts-rfq/test/mocks/VaultMock.sol"
Length of output: 910
12-14: Verify native token handling in deposit function
The function is marked as payable but delegates to _deposit without explicit ETH value handling. Please ensure:
- The parent's
_depositfunction properly handles native token deposits - Input validation is sufficient
- The
msg.valueis properly handled whentokenis the native token address
✅ Verification successful
Native token handling is properly implemented in deposit function
The internal _deposit function correctly handles native token deposits by:
- Checking if the token is
NATIVE_GAS_TOKEN(0xEeeee...) - Validating that
msg.valuematches the amount parameter when depositing native tokens - Using ERC20 transfer for non-native tokens
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check _deposit implementation in VaultMock for native token handling
# Find _deposit function implementation
ast-grep --pattern 'function _deposit(address $_, address $_, uint256 $_) {
$$$
}'
# Check for native token handling patterns
rg -p "msg.value" --type sol
Length of output: 143
Script:
#!/bin/bash
# Search for _deposit implementation and native token handling with correct file extension
ast-grep --pattern 'function _deposit(address $_, address $_, uint256 $_) {
$$$
}'
# Find files with .sol extension
fd -e sol
# Search for msg.value in .sol files
rg "msg.value" -A 3 $(fd -e sol)
# Look for native token handling patterns
rg "native|eth|weth" -i $(fd -e sol)
Length of output: 139251
Script:
#!/bin/bash
# Look for _deposit implementation in VaultMock
rg "_deposit" packages/contracts-rfq/test/mocks/VaultMock.sol -A 5
# Look for native token handling in VaultMock
rg "NATIVE|ETH|GAS_TOKEN" packages/contracts-rfq/test/mocks/VaultMock.sol
Length of output: 575
packages/contracts-rfq/test/mocks/VaultManyArguments.sol (1)
6-9: Well-structured contract declaration with clear documentation!
The contract follows good practices:
- Clear documentation about test-only usage
- Proper error naming convention with contract prefix
- Appropriate inheritance structure
packages/contracts-rfq/test/zaps/TokenZapV1.GasBench.t.sol (2)
1-12: LGTM! Contract setup and imports are well-structured.
The file uses the latest Solidity version and includes all necessary imports for testing.
22-37: Verify infinite approval simulation necessity.
While the infinite approval setup simulates a real-world scenario, consider adding a test case without infinite approval to benchmark the gas cost difference of the first deposit.
| function getVaultPayload(address token, uint256 amount) public view returns (bytes memory) { | ||
| return abi.encodeCall(vault.deposit, (token, amount, user)); | ||
| } | ||
|
|
||
| function getZapData(bytes memory originalPayload) public view returns (bytes memory) { | ||
| // Amount is the second argument of the deposit function. | ||
| return tokenZap.encodeZapData(address(vault), originalPayload, 4 + 32); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded offset with a calculated value.
The hardcoded offset 4 + 32 in getZapData is brittle and could break if the function signature changes. Consider:
- Adding a comment explaining the offset calculation
- Using ABI encoding utilities to calculate the offset dynamically
| function test_deposit_erc20() public { | ||
| bytes memory depositPayload = getVaultPayload(address(erc20), AMOUNT); | ||
| bytes memory zapData = getZapData(depositPayload); | ||
| // Transfer tokens to the zap contract first. | ||
| erc20.transfer(address(tokenZap), AMOUNT); | ||
| tokenZap.zap(address(erc20), AMOUNT, zapData); | ||
| // Check that the vault registered the deposit. | ||
| assertEq(vault.balanceOf(user, address(erc20)), AMOUNT); | ||
| } | ||
|
|
||
| function test_deposit_native() public { | ||
| bytes memory depositPayload = getVaultPayload(nativeGasToken, AMOUNT); | ||
| bytes memory zapData = getZapData(depositPayload); | ||
| tokenZap.zap{value: AMOUNT}(nativeGasToken, AMOUNT, zapData); | ||
| // Check that the vault registered the deposit. | ||
| assertEq(vault.balanceOf(user, nativeGasToken), AMOUNT); | ||
| } |
There was a problem hiding this comment.
Add explicit gas measurements for benchmarking.
For a gas benchmarking test suite, consider:
- Adding explicit gas measurement assertions
- Testing with different deposit amounts to understand gas scaling
- Adding comparison benchmarks against direct vault deposits
Example improvement:
function test_deposit_erc20() public {
+ uint256 gasStart = gasleft();
bytes memory depositPayload = getVaultPayload(address(erc20), AMOUNT);
bytes memory zapData = getZapData(depositPayload);
erc20.transfer(address(tokenZap), AMOUNT);
tokenZap.zap(address(erc20), AMOUNT, zapData);
+ uint256 gasUsed = gasStart - gasleft();
+ assertLt(gasUsed, MAX_EXPECTED_GAS);
assertEq(vault.balanceOf(user, address(erc20)), AMOUNT);
}Committable suggestion skipped: line range outside the PR's diff.
Deploying sanguine-fe with
|
| Latest commit: |
55329da
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8442555e.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-token-zap-gas-bench.sanguine-fe.pages.dev |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3406 +/- ##
=============================================
Coverage 42.36502% 42.36502%
=============================================
Files 87 87
Lines 3019 3019
Branches 82 82
=============================================
Hits 1279 1279
Misses 1737 1737
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:
|
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
SimpleVaultMockfor testing deposit functionality.VaultManyArgumentsfor enhanced error handling and deposit methods.TokenZapV1GasBenchmarkTestfor gas benchmarking of token deposits.Bug Fixes
Documentation