Merge Swap and SwapToAnyToken into a single example #223
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes 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: 7
🧹 Outside diff range and nitpick comments (10)
examples/swap/contracts/SwapCompanion.sol (1)
14-16: Add input validation for the constructor parametergatewayAddress.To prevent potential misconfiguration, it's advisable to check that
gatewayAddressis not the zero address.Apply this diff to add the validation:
function SwapCompanion(address payable gatewayAddress) { + require(gatewayAddress != address(0), "Invalid gateway address"); gateway = GatewayEVM(gatewayAddress); }examples/swap/tasks/zetachainSwap.ts (2)
Line range hint
7-25: Enhance error handling and input validationThe main function lacks proper error handling and input validation which is crucial for production deployments.
Consider implementing this more robust version:
const main = async (args: any, hre: HardhatRuntimeEnvironment) => { const [signer] = await hre.ethers.getSigners(); + if (!signer) throw new Error("No signer available"); + + // Validate input parameters + if (!args.contract || !args.zrc20 || !args.amount || !args.target || !args.recipient) { + throw new Error("Missing required parameters"); + } const factory = await hre.ethers.getContractFactory("Swap"); const contract = factory.attach(args.contract); const zrc20 = new ethers.Contract(args.zrc20, ZRC20.abi, signer); + + try { + const amount = parseEther(args.amount); - const amount = parseEther(args.amount); + console.log(`Approving ${args.amount} tokens...`); + const approval = await zrc20.approve(args.contract, amount, { + gasLimit: 300000 // Add appropriate gas limit + }); + await approval.wait(); - const approval = await zrc20.approve(args.contract, amount); - await approval.wait(); + console.log(`Swapping tokens...`); + const tx = await contract.swap( + args.zrc20, + amount, + args.target, + ethers.utils.arrayify(args.recipient), + JSON.parse(args.withdraw), + { gasLimit: 500000 } // Add appropriate gas limit + ); - const tx = await contract.swap( - args.zrc20, - amount, - args.target, - ethers.utils.arrayify(args.recipient), - JSON.parse(args.withdraw) - ); - - await tx.wait(); - console.log(`Transaction hash: ${tx.hash}`); + const receipt = await tx.wait(); + console.log(`Transaction hash: ${tx.hash}`); + return receipt; + } catch (error) { + console.error(`Failed to execute swap:`, error); + throw error; + } };
Line range hint
32-43: Add type safety to task parametersThe task parameters lack proper TypeScript interfaces which could lead to runtime errors.
Consider adding a type-safe interface:
interface SwapTaskArgs { contract: string; amount: string; zrc20: string; target: string; recipient: string; withdraw: boolean; json: boolean; } task("zetachain-swap", "Swap tokens from ZetaChain", async (args: SwapTaskArgs, hre: HardhatRuntimeEnvironment) => { return main(args, hre); })examples/swap/tasks/deployCompanion.ts (1)
39-43: Avoid hardcoding gateway addressesThe hardcoded gateway address could cause issues when deploying to different networks.
Consider implementing network-specific configuration:
const GATEWAY_ADDRESSES = { testnet: "0x0c487a766110c85d301d96e33579c5b317fa4995", mainnet: "<mainnet_address>", // Add other networks as needed }; // In task definition .addOptionalParam( "gateway", "Gateway address", (args, hre) => GATEWAY_ADDRESSES[hre.network.name] || args.gateway, )examples/swap/tasks/companionSwap.ts (1)
49-61: Enhance task parameter validationThe task parameters need additional validation and documentation.
Consider adding parameter validation:
interface CompanionSwapArgs { contract: string; universalContract: string; withdraw: boolean; erc20?: string; target: string; amount: string; recipient: string; } task("companion-swap", "Swap native gas tokens") .addParam("contract", "Address of the deployed companion contract") .addParam("universalContract", "Address of the universal contract") .addOptionalParam("withdraw", "Withdraw to destination chain", true, types.boolean) .addOptionalParam("erc20", "Optional ERC20 token address for token swaps") .addParam("target", "Target ZRC20 token address") .addParam("amount", "Amount to swap (in token decimals)") .addParam("recipient", "Recipient address on the destination chain") .setAction(async (args: CompanionSwapArgs, hre: HardhatRuntimeEnvironment) => { // Validate addresses if (!ethers.utils.isAddress(args.contract)) throw new Error("Invalid contract address"); if (!ethers.utils.isAddress(args.universalContract)) throw new Error("Invalid universal contract address"); if (args.erc20 && !ethers.utils.isAddress(args.erc20)) throw new Error("Invalid ERC20 address"); if (!ethers.utils.isAddress(args.target)) throw new Error("Invalid target address"); if (!ethers.utils.isAddress(args.recipient)) throw new Error("Invalid recipient address"); return main(args, hre); });examples/swap/tasks/evmGatewaySwap.ts (2)
40-40: Consider extracting hardcoded values to configurationThe task name change is appropriate, but consider moving the hardcoded gateway address to a configuration file for better maintainability.
Line range hint
40-69: Add parameter documentation for better developer experienceConsider adding JSDoc comments to document the expected format and units for parameters like:
amount: Expected units (wei/ether)gasPrice: Units (wei/gwei)gasLimit: Recommended rangestarget: Expected format of ZRC-20 addressexamples/swap/package.json (2)
25-25: Consider using caret version for @openzeppelin/hardhat-upgradesUsing a fixed version (
1.28.0) might miss important security updates. Consider using^1.28.0to receive patch and minor updates.
Line range hint
11-11: Update deploy script to remove SwapToAnyToken referenceThe
deploy-any:localnetscript still references the oldSwapToAnyTokencontract which is being merged intoSwap.- "deploy-any:localnet": "npx hardhat compile --force && npx hardhat deploy --name SwapToAnyToken --network localhost --gateway 0x9A676e781A523b5d0C0e43731313A708CB607508"examples/swap/scripts/localnet.sh (1)
14-14: Remove unused ZRC20_USDC variableThe
ZRC20_USDCvariable is defined but never used in the script.-ZRC20_USDC=$(jq -r '.addresses[] | select(.type=="ZRC-20 USDC on 97") | .address' localnet.json)🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 14-14: ZRC20_USDC appears unused. Verify use (or export if used externally).
(SC2034)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
examples/swap/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
examples/swap/contracts/Swap.sol(6 hunks)examples/swap/contracts/SwapCompanion.sol(1 hunks)examples/swap/contracts/SwapToAnyToken.sol(0 hunks)examples/swap/hardhat.config.ts(1 hunks)examples/swap/package.json(1 hunks)examples/swap/scripts/localnet.sh(1 hunks)examples/swap/tasks/companionSwap.ts(1 hunks)examples/swap/tasks/deploy.ts(1 hunks)examples/swap/tasks/deployCompanion.ts(1 hunks)examples/swap/tasks/evmGatewaySwap.ts(1 hunks)examples/swap/tasks/zetachainSwap.ts(2 hunks)
💤 Files with no reviewable changes (1)
- examples/swap/contracts/SwapToAnyToken.sol
🧰 Additional context used
🪛 Shellcheck (0.10.0)
examples/swap/scripts/localnet.sh
[warning] 14-14: ZRC20_USDC appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (5)
examples/swap/contracts/Swap.sol (1)
Line range hint 55-68: Verify the usage of __Ownable_init(owner) in the initialize function.
Ensure that the __Ownable_init function accepts an owner parameter. If it doesn't, you should initialize with __Ownable_init() and then transfer ownership to the designated owner using transferOwnership(owner).
Run the following script to check if __Ownable_init accepts an owner parameter:
✅ Verification successful
Let me analyze the repository structure to locate the OpenZeppelin contracts and verify the __Ownable_init function signature.
Let me search for the OpenZeppelin's Ownable contract implementation and its initialization patterns.
The initialization of OwnableUpgradeable with an owner parameter is correct and follows the latest OpenZeppelin pattern.
The search results confirm that the contract is using @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol, and the initialization pattern __Ownable_init(owner) is being used correctly. This is the recommended approach for initializing upgradeable contracts with immediate ownership assignment, as it combines the initialization and ownership transfer in a single atomic operation, which is more gas-efficient and secure than initializing and then transferring ownership in separate steps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if __Ownable_init accepts an owner parameter.
# Search for the definition of __Ownable_init in OpenZeppelin contracts.
rg -A 3 'function __Ownable_init' node_modules/@openzeppelin/contracts-upgradeable
Length of output: 173
Script:
#!/bin/bash
# Let's try searching in the repository for OpenZeppelin imports and contract definitions
rg -l "Ownable" --type sol
# Then let's examine the content of any relevant files
fd -e sol -x cat {} \;
Length of output: 39832
Script:
#!/bin/bash
# Search for package.json to understand dependencies
cat package.json 2>/dev/null || echo "No package.json found"
# Search for hardhat.config.js/ts to understand project structure
fd "hardhat.config" -x cat {}
Length of output: 3325
Script:
#!/bin/bash
# Search specifically for Ownable contract inheritance and initialization
rg -A 5 "contract.*Ownable" --type sol
rg "__Ownable_init" --type sol
Length of output: 127
Script:
#!/bin/bash
# Search for OpenZeppelin dependencies in node_modules
fd -H "Ownable" -t f node_modules/@openzeppelin -x cat {}
# Search for Ownable imports and usage in the codebase
rg -p "import.*Ownable|__Ownable_init" .
Length of output: 794
Script:
#!/bin/bash
# Search for package-lock.json or yarn.lock to identify exact OpenZeppelin version
cat package-lock.json 2>/dev/null || cat yarn.lock 2>/dev/null
# Try to find Ownable contract in the local vendor directory if it exists
fd -H "OwnableUpgradeable.sol" . -x cat {}
Length of output: 170
examples/swap/hardhat.config.ts (1)
2-5: The updated task imports and plugin configurations are appropriate.
The inclusion of new tasks and plugin imports aligns with the extended functionalities introduced. No issues found.
Also applies to: 13-15
examples/swap/tasks/deploy.ts (1)
22-28: Streamline the deployment output by always providing JSON.
Removing the conditional JSON output simplifies the deployment process and ensures consistent output formatting.
examples/swap/tasks/companionSwap.ts (1)
35-43: 🛠️ Refactor suggestion
Optimize native gas swap implementation
The native gas swap implementation could be optimized.
Consider these optimizations:
} else {
const value = hre.ethers.utils.parseEther(args.amount);
+
+ // Validate native balance
+ const balance = await signer.getBalance();
+ if (balance.lt(value.add(ethers.utils.parseEther("0.01")))) { // Include gas estimate
+ throw new Error("Insufficient balance for swap and gas");
+ }
+
+ // Add slippage protection
+ const minReceived = value.mul(95).div(100); // 5% slippage
+
tx = await contract.swapNativeGas(
args.universalContract,
args.target,
args.recipient,
args.withdraw,
+ minReceived,
{ value }
);Likely invalid or redundant comment.
examples/swap/scripts/localnet.sh (1)
4-5: Excellent error handling improvements
The addition of set -x and set -o pipefail enhances debugging capabilities and ensures proper error propagation in pipelines.
| function swapERC20( | ||
| address universalSwapContract, | ||
| address targetToken, | ||
| bytes memory recipient, | ||
| uint256 amount, | ||
| address asset, | ||
| bool withdraw | ||
| ) public { | ||
| IERC20(asset).safeTransferFrom(msg.sender, address(this), amount); | ||
| if (!IERC20(asset).approve(address(gateway), amount)) { | ||
| revert ApprovalFailed(); | ||
| } | ||
| gateway.depositAndCall( | ||
| universalSwapContract, | ||
| amount, | ||
| asset, | ||
| abi.encode(targetToken, recipient, withdraw), | ||
| RevertOptions(msg.sender, false, address(0), "", 0) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use safeApprove instead of approve to ensure safe token approvals.
Since you're utilizing SafeERC20, it's recommended to use safeApprove instead of approve. This handles non-standard ERC20 tokens that do not return a boolean value and eliminates the need for manual error handling.
Apply this diff to modify the approval:
IERC20(asset).safeTransferFrom(msg.sender, address(this), amount);
-if (!IERC20(asset).approve(address(gateway), amount)) {
- revert ApprovalFailed();
-}
+IERC20(asset).safeApprove(address(gateway), amount);Also, remove the ApprovalFailed error definition if it's no longer used:
-error ApprovalFailed();📝 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 swapERC20( | |
| address universalSwapContract, | |
| address targetToken, | |
| bytes memory recipient, | |
| uint256 amount, | |
| address asset, | |
| bool withdraw | |
| ) public { | |
| IERC20(asset).safeTransferFrom(msg.sender, address(this), amount); | |
| if (!IERC20(asset).approve(address(gateway), amount)) { | |
| revert ApprovalFailed(); | |
| } | |
| gateway.depositAndCall( | |
| universalSwapContract, | |
| amount, | |
| asset, | |
| abi.encode(targetToken, recipient, withdraw), | |
| RevertOptions(msg.sender, false, address(0), "", 0) | |
| ); | |
| } | |
| function swapERC20( | |
| address universalSwapContract, | |
| address targetToken, | |
| bytes memory recipient, | |
| uint256 amount, | |
| address asset, | |
| bool withdraw | |
| ) public { | |
| IERC20(asset).safeTransferFrom(msg.sender, address(this), amount); | |
| IERC20(asset).safeApprove(address(gateway), amount); | |
| gateway.depositAndCall( | |
| universalSwapContract, | |
| amount, | |
| asset, | |
| abi.encode(targetToken, recipient, withdraw), | |
| RevertOptions(msg.sender, false, address(0), "", 0) | |
| ); | |
| } |
| function swap( | ||
| address inputToken, | ||
| uint256 amount, | ||
| address targetToken, | ||
| bytes memory recipient, | ||
| 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 | ||
| ); | ||
| emit TokenSwap( | ||
| msg.sender, | ||
| recipient, | ||
| inputToken, | ||
| targetToken, | ||
| amount, | ||
| out | ||
| ); | ||
| withdraw( | ||
| Params({ | ||
| target: targetToken, | ||
| to: recipient, | ||
| withdraw: withdrawFlag | ||
| }), | ||
| msg.sender, | ||
| gasFee, | ||
| gasZRC20, | ||
| out, | ||
| inputToken | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use safeTransferFrom for secure token transfers in the swap function.
To handle tokens that do not return a boolean value and to ensure robust error handling, use safeTransferFrom from the SafeERC20 library instead of transferFrom.
Apply this diff to modify the token transfer:
+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
...
+using SafeERC20 for IERC20;
...
function swap(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient,
bool withdrawFlag
) public {
- bool success = IZRC20(inputToken).transferFrom(
- msg.sender,
- address(this),
- amount
- );
- if (!success) {
- revert TransferFailed();
- }
+ IERC20(inputToken).safeTransferFrom(
+ msg.sender,
+ address(this),
+ amount
+ );Remove the TransferFailed error definition if it's no longer used:
-error TransferFailed();Committable suggestion skipped: line range outside the PR's diff.
| if (params.withdraw) { | ||
| if (gasZRC20 == params.target) { | ||
| if (!IZRC20(gasZRC20).approve(address(gateway), out + gasFee)) { | ||
| revert ApprovalFailed(); | ||
| } | ||
| } else { | ||
| if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) { | ||
| revert ApprovalFailed(); | ||
| } | ||
| if (!IZRC20(params.target).approve(address(gateway), out)) { | ||
| revert ApprovalFailed(); | ||
| } | ||
| } | ||
| gateway.withdraw( | ||
| abi.encodePacked(params.to), | ||
| out, | ||
| params.target, | ||
| RevertOptions({ | ||
| revertAddress: address(this), | ||
| callOnRevert: true, | ||
| abortAddress: address(0), | ||
| revertMessage: abi.encode(sender, inputToken), | ||
| onRevertGasLimit: gasLimit | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use safeApprove in the withdraw function to ensure safe approvals.
Replace direct approve calls with safeApprove to handle tokens that may not return a boolean value, ensuring the contract operates securely.
Apply this diff to update the approval logic:
if (params.withdraw) {
if (gasZRC20 == params.target) {
- if (!IZRC20(gasZRC20).approve(address(gateway), out + gasFee)) {
- revert ApprovalFailed();
- }
+ IERC20(gasZRC20).safeApprove(address(gateway), out + gasFee);
} else {
- if (!IZRC20(gasZRC20).approve(address(gateway), gasFee)) {
- revert ApprovalFailed();
- }
- if (!IZRC20(params.target).approve(address(gateway), out)) {
- revert ApprovalFailed();
- }
+ IERC20(gasZRC20).safeApprove(address(gateway), gasFee);
+ IERC20(params.target).safeApprove(address(gateway), out);
}
gateway.withdraw(
abi.encodePacked(params.to),
out,
params.target,
RevertOptions({
revertAddress: address(this),
callOnRevert: true,
abortAddress: address(0),
revertMessage: abi.encode(sender, inputToken),
onRevertGasLimit: gasLimit
})
);
}Remove the ApprovalFailed error definition if it's no longer used:
-error ApprovalFailed();Committable suggestion skipped: line range outside the PR's diff.
| const contract = await hre.upgrades.deployProxy( | ||
| factory as any, | ||
| [args.gateway, args.uniswapRouter, args.gasLimit, signer.address], | ||
| { kind: "uups" } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any type casting for improved type safety.
Casting the contract factory to any can undermine the benefits of TypeScript's type checking. Instead, explicitly define the type to maintain type safety.
Apply this diff to correct the type:
-const contract = await hre.upgrades.deployProxy(
- factory as any,
+const contract = await hre.upgrades.deployProxy(
+ factory,
[args.gateway, args.uniswapRouter, args.gasLimit, signer.address],
{ kind: "uups" }
);Ensure that the contract factory is properly typed, which might involve updating your imports:
+const factory = await hre.ethers.getContractFactory(args.name, signer);| const factory = await hre.ethers.getContractFactory(args.name); | ||
| const contract = await (factory as any).deploy(args.gateway); | ||
| await contract.deployed(); |
There was a problem hiding this comment.
Add deployment verification and contract size check
The deployment process lacks important production safeguards.
Consider implementing these safety checks:
const factory = await hre.ethers.getContractFactory(args.name);
+
+ // Check contract size
+ const bytecode = factory.bytecode;
+ const size = bytecode.length / 2 - 1;
+ if (size > 24576) { // 24KB limit
+ throw new Error(`Contract size is ${size} bytes, exceeding the 24KB limit`);
+ }
+
const contract = await (factory as any).deploy(args.gateway);
await contract.deployed();
+
+ // Verify deployment
+ const code = await hre.ethers.provider.getCode(contract.address);
+ if (code === "0x") throw new Error("Contract deployment failed");
+
+ // Verify on block explorer if not on local network
+ if (network !== "localhost" && network !== "hardhat") {
+ await hre.run("verify:verify", {
+ address: contract.address,
+ constructorArguments: [args.gateway],
+ });
+ }Committable suggestion skipped: line range outside the PR's diff.
| if (args.erc20) { | ||
| const erc20Contract = new ethers.Contract( | ||
| args.erc20, | ||
| ERC20_ABI.abi, | ||
| signer | ||
| ); | ||
| const decimals = await erc20Contract.decimals(); | ||
| const value = hre.ethers.utils.parseUnits(args.amount, decimals); | ||
| const approveTx = await erc20Contract | ||
| .connect(signer) | ||
| .approve(args.contract, value); | ||
| await approveTx.wait(); | ||
|
|
||
| tx = await contract.swapERC20( | ||
| args.universalContract, | ||
| args.target, | ||
| args.recipient, | ||
| args.amount, | ||
| args.erc20, | ||
| args.withdraw | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Add value validation and slippage protection
The ERC20 swap flow lacks important safety checks.
Consider implementing these safeguards:
if (args.erc20) {
const erc20Contract = new ethers.Contract(
args.erc20,
ERC20_ABI.abi,
signer
);
const decimals = await erc20Contract.decimals();
const value = hre.ethers.utils.parseUnits(args.amount, decimals);
+
+ // Validate balance
+ const balance = await erc20Contract.balanceOf(signer.address);
+ if (balance.lt(value)) {
+ throw new Error(`Insufficient balance. Required: ${args.amount}, Available: ${
+ ethers.utils.formatUnits(balance, decimals)
+ }`);
+ }
+
+ // Add slippage protection
+ const minReceived = value.mul(95).div(100); // 5% slippage
const approveTx = await erc20Contract
.connect(signer)
.approve(args.contract, value);
await approveTx.wait();
tx = await contract.swapERC20(
args.universalContract,
args.target,
args.recipient,
args.amount,
args.erc20,
args.withdraw,
+ minReceived,
+ { gasLimit: 500000 }
);Committable suggestion skipped: line range outside the PR's diff.
| CONTRACT_SWAP=$(npx hardhat deploy --name Swap --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" | jq -r '.contractAddress') | ||
| COMPANION=$(npx hardhat deploy-companion --gateway "$GATEWAY_ETHEREUM" --network localhost --json | jq -r '.contractAddress') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for contract deployments
Add checks to ensure contract addresses are successfully retrieved before proceeding with subsequent operations.
CONTRACT_SWAP=$(npx hardhat deploy --name Swap --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" | jq -r '.contractAddress')
+if [ -z "$CONTRACT_SWAP" ]; then
+ echo "Error: Failed to deploy Swap contract"
+ exit 1
+fi
COMPANION=$(npx hardhat deploy-companion --gateway "$GATEWAY_ETHEREUM" --network localhost --json | jq -r '.contractAddress')
+if [ -z "$COMPANION" ]; then
+ echo "Error: Failed to deploy Companion contract"
+ exit 1
+fiAlso applies to: 24-41
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
examples/swap/scripts/localnet.sh (1)
13-14: Clean up unused variablesThe
ZRC20_USDCvariable is declared but never used in the script.Remove the unused variable declaration if it's not needed for future use.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 14-14: ZRC20_USDC appears unused. Verify use (or export if used externally).
(SC2034)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
examples/swap/hardhat.config.ts(1 hunks)examples/swap/scripts/localnet.sh(1 hunks)examples/swap/tasks/evmSwap.ts(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
examples/swap/scripts/localnet.sh
[warning] 14-14: ZRC20_USDC appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (5)
examples/swap/hardhat.config.ts (1)
2-5: LGTM! Verify task file existence.
The new task imports align well with the PR objectives, following a consistent naming convention.
✅ Verification successful
The previous script failed because the tasks directory wasn't found at the expected location. Let's verify the existence of these files across the repository first, and then we can determine their actual location and validate the imports accordingly.
✓ Task imports verified and correctly located
All imported task files are present in their expected location at examples/swap/tasks/:
- companionSwap.ts
- deployCompanion.ts
- zetachainSwap.ts
- evmSwap.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all imported task files exist
for task in companionSwap deployCompanion zetachainSwap evmSwap; do
if ! fd -t f "^${task}.ts$" tasks/; then
echo "Missing task file: ${task}.ts"
exit 1
fi
done
Length of output: 287
Script:
#!/bin/bash
# Let's first check if the tasks directory exists and its location
fd -t d "tasks$"
# Then let's check for these specific files in the repository
fd -t f "companionSwap\.ts|deployCompanion\.ts|zetachainSwap\.ts|evmSwap\.ts$"
Length of output: 349
examples/swap/tasks/evmSwap.ts (1)
40-40: Verify task name changes across the codebase
The task name change from "swap-from-evm" to "evm-swap" appears correct and aligns with the PR objectives.
Let's verify the task name usage across the codebase:
✅ Verification successful
Task name change is consistent across the codebase
The verification confirms that:
- No references to the old task name "swap-from-evm" exist in the codebase
- The new task name "evm-swap" is consistently used in:
- Task definition (examples/swap/tasks/evmSwap.ts)
- Local development scripts (examples/swap/scripts/localnet.sh)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old task name
rg "swap-from-evm" --type ts --type sh
# Search for consistent usage of the new task name
rg "evm-swap" --type ts --type sh
Length of output: 277
examples/swap/scripts/localnet.sh (3)
4-5: Improved error handling and debugging capabilities
The addition of set -x and set -o pipefail enhances script reliability by:
- Enabling command tracing for better debugging
- Ensuring pipeline failures are properly caught
21-22:
Add error handling for contract deployments
The contract deployment commands should include error checking to ensure successful deployment before proceeding.
Apply this diff:
CONTRACT_SWAP=$(npx hardhat deploy --name Swap --network localhost --gateway "$GATEWAY_ZETACHAIN" --uniswap-router "$UNISWAP_ROUTER" | jq -r '.contractAddress')
+if [ -z "$CONTRACT_SWAP" ]; then
+ echo "Error: Failed to deploy Swap contract"
+ exit 1
+fi
COMPANION=$(npx hardhat deploy-companion --gateway "$GATEWAY_ETHEREUM" --network localhost --json | jq -r '.contractAddress')
+if [ -z "$COMPANION" ]; then
+ echo "Error: Failed to deploy Companion contract"
+ exit 1
+fi24-41: Verify contract interactions across different swap scenarios
The script tests multiple swap scenarios:
- Companion swap with native token
- Companion swap with ERC20
- EVM swap with withdrawal
- EVM swap without withdrawal
- ZetaChain swap
The test coverage looks comprehensive.
Let's verify the contract interactions:
Also applies to: 45-60, 64-69
✅ Verification successful
Let me analyze the contract interactions by first identifying the relevant files and then examining their contents. The previous pattern might have been too specific, so I'm starting with a broader search to locate the swap-related contracts and their implementations.
Now that we've identified the relevant contract files, let's examine their contents to verify the contract interactions and event emissions for the different swap scenarios mentioned in the script.
Contract interactions for swap scenarios are properly implemented
The contract implementations confirm that all swap scenarios mentioned in the script are correctly supported:
- Companion swap with native token:
SwapCompanion.swapNativeGas() - Companion swap with ERC20:
SwapCompanion.swapERC20() - EVM swap with withdrawal:
Swap.swap()withwithdrawFlag = true - EVM swap without withdrawal:
Swap.swap()withwithdrawFlag = false - ZetaChain swap:
Swap.onCall()handles cross-chain swaps
The contracts include proper event emissions via TokenSwap event, comprehensive error handling, and secure token transfer mechanisms using OpenZeppelin's SafeERC20.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify contract interfaces match the expected function calls
ast-grep --pattern 'interface $_ {
$$$
companion$_($$$) {
$$$
}
$$$
}'
# Verify event emissions for these operations
ast-grep --pattern 'event $_(
$$$
address $_,
$$$
)'
Length of output: 145
Script:
#!/bin/bash
# Search for contract files related to swaps
fd -e sol | grep -i "swap"
# Search for companion swap function implementations
rg -l "companion.*swap" --type sol
# Look for swap-related events
rg "event.*Swap" --type sol
# Search for test files related to swaps
fd -e js -e ts | grep -i "swap.*test"
Length of output: 264
Script:
#!/bin/bash
# Examine the Swap contract content
cat examples/swap/contracts/Swap.sol
# Examine the SwapCompanion contract content
cat examples/swap/contracts/SwapCompanion.sol
# Look for test files that might verify these scenarios
fd . examples/swap/test/
Length of output: 9956
| { version: "0.8.7" }, | ||
| { version: "0.8.26" }, | ||
| ], | ||
| compilers: [{ version: "0.8.20" }, { version: "0.8.26" }], |
There was a problem hiding this comment.
💡 Codebase verification
Compiler versions are misaligned with contract requirements
The configured compiler versions (0.8.20, 0.8.26) in hardhat.config.ts don't align with actual usage. All Solidity contracts in the codebase exclusively use version 0.8.26 (either fixed or ^0.8.26), making the 0.8.20 compiler configuration redundant.
- Remove version 0.8.20 from the compilers array in
hardhat.config.ts - Keep only
{ version: "0.8.26" }to match the codebase requirements
🔗 Analysis chain
LGTM! Consider security implications.
The streamlined compiler versions (0.8.20, 0.8.26) are appropriate for OpenZeppelin upgradeable contracts. The removal of older versions reduces potential security vulnerabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Solidity files using pragma versions
echo "Checking Solidity pragma versions:"
fd -e sol . | xargs rg "pragma solidity" -A 1
Length of output: 1287
| import "@openzeppelin/hardhat-upgrades"; | ||
| import "@nomiclabs/hardhat-ethers"; |
There was a problem hiding this comment.
💡 Codebase verification
Required Hardhat plugins not found in dependencies
The imported plugins @openzeppelin/hardhat-upgrades and @nomiclabs/hardhat-ethers are not listed in either dependencies or devDependencies of package.json. These plugins are essential for:
- Implementing the UUPS upgradeable pattern
- Managing contract deployments and interactions
Please ensure to:
- Install
@openzeppelin/hardhat-upgradesfor UUPS support - Install
@nomiclabs/hardhat-ethersas it's a peer dependency - Use compatible versions that work with your Hardhat setup
🔗 Analysis chain
LGTM! Verify plugin compatibility.
The addition of OpenZeppelin upgrades and ethers plugins is necessary for implementing the UUPS upgradeable pattern mentioned in the PR objectives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package.json for compatible versions
if [ -f package.json ]; then
echo "Plugin versions:"
jq '.dependencies["@openzeppelin/hardhat-upgrades"], .dependencies["@nomiclabs/hardhat-ethers"]' package.json
fi
Length of output: 190
Script:
#!/bin/bash
# Check devDependencies in package.json
echo "Checking devDependencies:"
jq '.devDependencies["@openzeppelin/hardhat-upgrades"], .devDependencies["@nomiclabs/hardhat-ethers"]' package.json
# List all dependencies containing hardhat or openzeppelin
echo -e "\nAll related dependencies:"
jq '.dependencies, .devDependencies | with_entries(select(.key | test("hardhat|openzeppelin")))' package.json
Length of output: 423
The Swap has grown in size (due to revert logic) and the difference between Swap and SwapToAnyToken has become insignificant in terms of. LOC and complexity. I'm going to be merging both examples/tutorials into a single Swap tutorial to make the dev ex a bit more streamlined.
Made Swap upgradeable.
Added a companion Swap contract primarily to be called from ThirdWeb.
Added Bitcoin mainnet chain ID.
Added TokenSwap event.
Summary by CodeRabbit
New Features
SwapCompanioncontract for facilitating token swaps.companion-swapto streamline token swaps.Improvements
Swapcontract with upgradeable and ownership features.Bug Fixes
Chores