Add revert handling to the Swap example #219
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
examples/swap/tasks/deploy.ts (2)
49-53: Enhance gasLimit parameter documentationWhile the parameter is correctly defined, the documentation could be more specific.
.addOptionalParam( "gasLimit", - "Gas limit for the transaction", + "Gas limit for the transaction in gas units (minimum recommended: 1,000,000)", 1000000, types.int );
Line range hint
1-53: Consider adding dynamic gas estimationCurrently, the gas limit is hardcoded with a default value. For production deployments, consider implementing dynamic gas estimation based on network conditions and contract complexity.
You could:
- Add a gas estimation function that simulates the deployment
- Use the estimated value as the default instead of a fixed number
- Add a safety margin (e.g., 20%) to the estimated value
examples/swap/scripts/localnet.sh (3)
12-16: Remove unused GATEWAY_ETHEREUM variableThe
GATEWAY_ETHEREUMvariable is declared but never used in the script. Consider removing it to maintain code cleanliness.ZRC20_BNB=$(jq -r '.addresses[] | select(.type=="ZRC-20 BNB on 97") | .address' localnet.json) -GATEWAY_ETHEREUM=$(jq -r '.addresses[] | select(.type=="gatewayEVM" and .chain=="ethereum") | .address' localnet.json) GATEWAY_ZETACHAIN=$(jq -r '.addresses[] | select(.type=="gatewayZEVM" and .chain=="zetachain") | .address' localnet.json)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 13-13: GATEWAY_ETHEREUM appears unused. Verify use (or export if used externally).
(SC2034)
18-23: Add error handling for contract deploymentsWhile the deployments are well-structured, consider adding error handling to ensure the script fails gracefully if deployments are unsuccessful.
+if ! CONTRACT_SWAP=$(npx hardhat deploy --name Swap --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress'); then + echo "Failed to deploy Swap contract" + exit 1 +fi -CONTRACT_SWAP=$(npx hardhat deploy --name Swap --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress') echo -e "\n🚀 Deployed Swap contract on ZetaChain: $CONTRACT_SWAP" +if ! CONTRACT_SWAPTOANY=$(npx hardhat deploy --name SwapToAnyToken --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress'); then + echo "Failed to deploy SwapToAnyToken contract" + exit 1 +fi -CONTRACT_SWAPTOANY=$(npx hardhat deploy --name SwapToAnyToken --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" --json | jq -r '.contractAddress') echo -e "\n🚀 Deployed SwapToAny contract on ZetaChain: $CONTRACT_SWAPTOANY"
24-71: Document the purpose of each swap operationThe script includes multiple swap operations with different configurations, but their specific purposes are not documented. Consider adding comments to explain the test scenarios being covered.
+# Test 1: Swap with immediate withdrawal npx hardhat swap-from-evm \ --network localhost \ --receiver "$CONTRACT_SWAP" \ --amount 1 \ --target "$ZRC20_BNB" \ --recipient "$SENDER" npx hardhat localnet-check +# Test 2: Swap without immediate withdrawal npx hardhat swap-from-evm \ --network localhost \ --receiver "$CONTRACT_SWAP" \ --amount 1 \ --target "$ZRC20_BNB" \ --recipient "$SENDER" \ --withdraw false +# Test 3: SwapToAnyToken with immediate withdrawal npx hardhat swap-from-evm \ --network localhost \ --receiver "$CONTRACT_SWAPTOANY" \ --amount 1 \ --target "$ZRC20_BNB" \ --recipient "$SENDER" +# Test 4: SwapToAnyToken without immediate withdrawal npx hardhat swap-from-evm \ --network localhost \ --receiver "$CONTRACT_SWAPTOANY" \ --amount 1 \ --target "$ZRC20_BNB" \ --recipient "$SENDER" \ --withdraw false +# Test 5: Cross-chain swap from ZetaChain to Ethereum npx hardhat swap-from-zetachain \ --network localhost \ --contract "$CONTRACT_SWAPTOANY" \ --amount 1 \ --zrc20 "$ZRC20_BNB" \ --target "$ZRC20_ETHEREUM" \ --recipient "$SENDER"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
examples/swap/contracts/Swap.sol(3 hunks)examples/swap/contracts/SwapToAnyToken.sol(2 hunks)examples/swap/scripts/localnet.sh(1 hunks)examples/swap/tasks/deploy.ts(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
examples/swap/scripts/localnet.sh
[warning] 13-13: GATEWAY_ETHEREUM appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (7)
examples/swap/tasks/deploy.ts (2)
1-1: LGTM: Clean import addition
The addition of types import is appropriate for type-safe parameter definition.
17-18: Verify gas limit sufficiency for revert scenarios
The gas limit parameter is now properly passed to the contract constructor. However, we should verify if the default value of 1,000,000 gas units is sufficient for complex operations involving reverts.
✅ Verification successful
Gas limit value is appropriate for complex operations
Based on the contract analysis, the gas limit of 1,000,000 units is sufficient for the contract's operations. The contract includes multiple gas-intensive operations:
- Complex swap operations through Uniswap router
- Multiple token approvals and transfers
- Revert handling with message encoding/decoding
- Cross-chain message passing via gateway
The gas limit aligns with similar production patterns and provides adequate headroom for these operations, including revert scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gas-consuming operations in the contract
rg -A 5 "withdraw|revert|transfer|call" --type solidity
# Look for existing gas estimations or similar deployments
rg -A 5 "gasLimit|gas:" --type yaml --type json
Length of output: 140
Script:
#!/bin/bash
# Search for Solidity files first
fd -e sol
# Search for gas-consuming operations in all files (without file type filter)
rg -A 5 "withdraw|revert|transfer|call"
# Look for gas configurations in TypeScript/JavaScript files
rg -A 5 "gasLimit|gas:" -g "*.{ts,js}"
# Look for deployment configurations
fd -e json -e yaml | xargs cat
Length of output: 131156
Script:
#!/bin/bash
# Search for Solidity files in the swap example to analyze gas usage
fd -e sol -p examples/swap/contracts | xargs cat
# Look for gas limit configurations in the deployment tasks
rg -A 5 "gasLimit.*=.*[0-9]+" examples/swap/tasks/deploy.ts
Length of output: 12435
examples/swap/scripts/localnet.sh (1)
Line range hint 1-71: Add test case for revert handling
The PR's main objective is to add revert handling for failed withdrawals, but the script doesn't explicitly test this scenario. Consider adding a test case where the withdrawal is expected to fail and trigger the onRevert function.
Would you like me to provide an example test case that verifies the revert handling functionality?
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 11-11: ZRC20_USDC appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 13-13: GATEWAY_ETHEREUM appears unused. Verify use (or export if used externally).
(SC2034)
examples/swap/contracts/SwapToAnyToken.sol (4)
19-19: LGTM: Added gasLimit as an immutable state variable
The addition of the uint256 public immutable gasLimit; enhances the contract by ensuring the gasLimit remains constant after deployment.
23-24: LGTM: Added custom errors for ApprovalFailed and TransferFailed
Defining specific errors improves clarity and gas efficiency during error handling.
80-85: Refactoring enhances modularity and readability
The use of handleGasAndSwap function improves code organization by separating concerns and reusing logic.
203-226: Revert handling logic is correctly implemented
The onRevert function appropriately handles reversion scenarios by decoding the revert message, recalculating the swap, and withdrawing the assets back to the sender. The usage of gasLimit ensures that gas fees are managed effectively.
| fallback() external payable {} | ||
|
|
||
| receive() external payable {} |
There was a problem hiding this comment.
Ensure fallback and receive functions revert as intended
The fallback and receive functions are currently empty, which means they will accept Ether sent to the contract without any action. According to the PR objectives, these functions should revert when called to handle cases where the withdraw function fails on the destination chain. This ensures that any unexpected Ether transfers are properly rejected and the onRevert function is triggered appropriately.
Apply this diff to make the functions revert when called:
-fallback() external payable {}
+fallback() external payable {
+ revert("");
+}
-receive() external payable {}
+receive() external payable {
+ revert("");
+}📝 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.
| fallback() external payable {} | |
| receive() external payable {} | |
| fallback() external payable { | |
| revert(""); | |
| } | |
| receive() external payable { | |
| revert(""); | |
| } |
| constructor( | ||
| address payable gatewayAddress, | ||
| address uniswapRouterAddress, | ||
| uint256 gasLimitAmount | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for gasLimitAmount in the constructor
It's recommended to validate the gasLimitAmount parameter in the constructor to ensure it's within an acceptable range (e.g., greater than zero). This prevents potential misconfigurations that could lead to failed transactions due to insufficient gas limits.
Apply this suggested change:
constructor(
address payable gatewayAddress,
address uniswapRouterAddress,
uint256 gasLimitAmount
) {
+ if (gasLimitAmount == 0) {
+ revert InvalidGasLimit();
+ }
if (gatewayAddress == address(0) || uniswapRouterAddress == address(0))
revert InvalidAddress();
uniswapRouter = uniswapRouterAddress;
gateway = GatewayZEVM(gatewayAddress);
gasLimit = gasLimitAmount;
}Add a new error declaration:
+ error InvalidGasLimit();Also applies to: 40-40
| bool success = IWETH9(params.target).transfer( | ||
| address(uint160(bytes20(params.to))), | ||
| out | ||
| ); | ||
| if (!success) { | ||
| revert TransferFailed(); | ||
| } |
There was a problem hiding this comment.
Use IZRC20 interface instead of IWETH9 for token transfers
In the withdraw function, when params.withdraw is false, the contract attempts to transfer tokens using the IWETH9 interface:
bool success = IWETH9(params.target).transfer(
address(uint160(bytes20(params.to))),
out
);Unless params.target is guaranteed to be an IWETH9 compliant token, this may result in unexpected behavior. It is recommended to use the IZRC20 interface for standard token transfers to ensure compatibility with general ERC20 tokens.
Apply the following change:
-bool success = IWETH9(params.target).transfer(
+bool success = IZRC20(params.target).transfer(
address(uint160(bytes20(params.to))),
out
);| fallback() external payable {} | ||
|
|
||
| receive() external payable {} |
There was a problem hiding this comment.
Ensure fallback and receive functions revert as per PR objectives
According to the PR objectives, the fallback and receive functions should revert when called to handle situations where the withdraw function may fail on the destination chain. The current implementation adds empty functions that do not revert. To align with the intended functionality, these functions should be modified to revert upon invocation.
Apply the following changes:
-fallback() external payable {}
+fallback() external payable {
+ revert("");
+}
-receive() external payable {}
+receive() external payable {
+ revert("");
+}📝 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.
| fallback() external payable {} | |
| receive() external payable {} | |
| fallback() external payable { | |
| revert(""); | |
| } | |
| receive() external payable { | |
| revert(""); | |
| } |
| uint256 swapAmount; | ||
|
|
||
| if (withdraw) { | ||
| (gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee(); | ||
| (gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee(); | ||
|
|
||
| if (gasZRC20 == inputToken) { | ||
| swapAmount = amount - gasFee; | ||
| } else { | ||
| inputForGas = SwapHelperLib.swapTokensForExactTokens( | ||
| uniswapRouter, | ||
| inputToken, | ||
| gasFee, | ||
| gasZRC20, | ||
| amount | ||
| ); | ||
| swapAmount = amount - inputForGas; | ||
| } | ||
| if (gasZRC20 == inputToken) { | ||
| swapAmount = amount - gasFee; | ||
| } else { | ||
| inputForGas = SwapHelperLib.swapTokensForExactTokens( | ||
| uniswapRouter, | ||
| inputToken, | ||
| gasFee, | ||
| gasZRC20, | ||
| amount | ||
| ); | ||
| swapAmount = amount - inputForGas; | ||
| } | ||
|
|
||
| uint256 outputAmount = SwapHelperLib.swapExactTokensForTokens( | ||
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | ||
| uniswapRouter, | ||
| inputToken, | ||
| swapAmount, | ||
| targetToken, | ||
| 0 | ||
| ); | ||
| return (out, gasZRC20, gasFee); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure swapAmount is validated to be greater than zero
The swapAmount might be zero if gasFee equals amount or inputForGas equals amount, leading to potential issues when performing swaps with zero amount. It's prudent to add a check to ensure that swapAmount > 0 before proceeding.
Implement the following check:
if (gasZRC20 == inputToken) {
swapAmount = amount - gasFee;
+ require(swapAmount > 0, "Swap amount must be greater than zero");
} else {
inputForGas = SwapHelperLib.swapTokensForExactTokens(
uniswapRouter,
inputToken,
gasFee,
gasZRC20,
amount
);
swapAmount = amount - inputForGas;
+ require(swapAmount > 0, "Swap amount must be greater than zero");
}📝 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.
| uint256 swapAmount; | |
| if (withdraw) { | |
| (gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee(); | |
| (gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee(); | |
| if (gasZRC20 == inputToken) { | |
| swapAmount = amount - gasFee; | |
| } else { | |
| inputForGas = SwapHelperLib.swapTokensForExactTokens( | |
| uniswapRouter, | |
| inputToken, | |
| gasFee, | |
| gasZRC20, | |
| amount | |
| ); | |
| swapAmount = amount - inputForGas; | |
| } | |
| if (gasZRC20 == inputToken) { | |
| swapAmount = amount - gasFee; | |
| } else { | |
| inputForGas = SwapHelperLib.swapTokensForExactTokens( | |
| uniswapRouter, | |
| inputToken, | |
| gasFee, | |
| gasZRC20, | |
| amount | |
| ); | |
| swapAmount = amount - inputForGas; | |
| } | |
| uint256 outputAmount = SwapHelperLib.swapExactTokensForTokens( | |
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | |
| uniswapRouter, | |
| inputToken, | |
| swapAmount, | |
| targetToken, | |
| 0 | |
| ); | |
| return (out, gasZRC20, gasFee); | |
| } | |
| uint256 swapAmount; | |
| (gasZRC20, gasFee) = IZRC20(targetToken).withdrawGasFee(); | |
| if (gasZRC20 == inputToken) { | |
| swapAmount = amount - gasFee; | |
| require(swapAmount > 0, "Swap amount must be greater than zero"); | |
| } else { | |
| inputForGas = SwapHelperLib.swapTokensForExactTokens( | |
| uniswapRouter, | |
| inputToken, | |
| gasFee, | |
| gasZRC20, | |
| amount | |
| ); | |
| swapAmount = amount - inputForGas; | |
| require(swapAmount > 0, "Swap amount must be greater than zero"); | |
| } | |
| uint256 out = SwapHelperLib.swapExactTokensForTokens( | |
| uniswapRouter, | |
| inputToken, | |
| swapAmount, | |
| targetToken, | |
| 0 | |
| ); | |
| return (out, gasZRC20, gasFee); | |
| } |
| function swap( | ||
| address inputToken, | ||
| uint256 amount, | ||
| address targetToken, | ||
| bytes memory recipient, | ||
| bool withdraw | ||
| ) internal { | ||
| bool withdrawFlag | ||
| ) public { | ||
| bool success = IZRC20(inputToken).transferFrom( | ||
| msg.sender, | ||
| address(this), | ||
| amount | ||
| ); | ||
| if (!success) { | ||
| revert TransferFailed(); | ||
| } | ||
|
|
||
| (uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap( | ||
| inputToken, | ||
| amount, | ||
| targetToken | ||
| ); | ||
|
|
||
| withdraw( | ||
| Params({ | ||
| target: targetToken, | ||
| to: recipient, | ||
| withdraw: withdrawFlag | ||
| }), | ||
| msg.sender, | ||
| gasFee, | ||
| gasZRC20, | ||
| out, | ||
| inputToken | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider adding reentrancy protection to the swap function
The swap function involves token transfers and external calls, which could potentially be exploited in a reentrancy attack. It is advisable to add reentrancy protection using OpenZeppelin's ReentrancyGuard by inheriting from it and applying the nonReentrant modifier to the swap function.
Apply the following changes:
- Import
ReentrancyGuardand inherit from it:
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-contract SwapToAnyToken is UniversalContract {
+contract SwapToAnyToken is UniversalContract, ReentrancyGuard {
address public immutable uniswapRouter;
GatewayZEVM public gateway;
uint256 constant BITCOIN = 18332;
uint256 public immutable gasLimit;- Apply the
nonReentrantmodifier to theswapfunction:
function swap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient,
bool withdrawFlag
- ) public {
+ ) public nonReentrant {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function swap( | |
| address inputToken, | |
| uint256 amount, | |
| address targetToken, | |
| bytes memory recipient, | |
| bool withdraw | |
| ) internal { | |
| bool withdrawFlag | |
| ) public { | |
| bool success = IZRC20(inputToken).transferFrom( | |
| msg.sender, | |
| address(this), | |
| amount | |
| ); | |
| if (!success) { | |
| revert TransferFailed(); | |
| } | |
| (uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap( | |
| inputToken, | |
| amount, | |
| targetToken | |
| ); | |
| withdraw( | |
| Params({ | |
| target: targetToken, | |
| to: recipient, | |
| withdraw: withdrawFlag | |
| }), | |
| msg.sender, | |
| gasFee, | |
| gasZRC20, | |
| out, | |
| inputToken | |
| ); | |
| } | |
| import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | |
| contract SwapToAnyToken is UniversalContract, ReentrancyGuard { | |
| address public immutable uniswapRouter; | |
| GatewayZEVM public gateway; | |
| uint256 constant BITCOIN = 18332; | |
| uint256 public immutable gasLimit; | |
| function swap( | |
| address inputToken, | |
| uint256 amount, | |
| address targetToken, | |
| bytes memory recipient, | |
| bool withdrawFlag | |
| ) public nonReentrant { | |
| bool success = IZRC20(inputToken).transferFrom( | |
| msg.sender, | |
| address(this), | |
| amount | |
| ); | |
| if (!success) { | |
| revert TransferFailed(); | |
| } | |
| (uint256 out, address gasZRC20, uint256 gasFee) = handleGasAndSwap( | |
| inputToken, | |
| amount, | |
| targetToken | |
| ); | |
| withdraw( | |
| Params({ | |
| target: targetToken, | |
| to: recipient, | |
| withdraw: withdrawFlag | |
| }), | |
| msg.sender, | |
| gasFee, | |
| gasZRC20, | |
| out, | |
| inputToken | |
| ); | |
| } |
Added revert handling to the swap contract to handle scenarios where
withdrawfunction fails on destination chain.To test add this:
And then withdraw tokens to the same swap contract on localnet, it will revert and trigger
onRevert, which will send tokens back to the original sender.Summary by CodeRabbit
New Features
SwapandSwapToAnyTokencontracts.Bug Fixes
Chores