Gateway testnet #207
Gateway testnet #207
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to multiple smart contracts and configuration files within the examples directory. Key changes include the renaming of the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🪛 actionlint.github/workflows/test.yaml19-19: shellcheck reported issue in this script: SC2038:warning:1:13: Use -print0/-0 or -exec + to allow for non-alphanumeric filenames (shellcheck) 19-19: shellcheck reported issue in this script: SC2086:info:3:31: Double quote to prevent globbing and word splitting (shellcheck) 🔇 Additional comments (3).github/workflows/test.yaml (3)
The workflow would benefit from path filters to run only when relevant files are modified. Please refer to the previous suggestion about adding path filters to the workflow triggers.
Enhance matrix setup reliability. The current shell script implementation has potential issues with non-alphanumeric filenames and quoting. Consider either fixing these issues or adopting the more robust github-script approach. Option 1: Fix the shell script issues: -test_dirs=$(find examples/*/scripts -type f -name 'test.sh' -exec dirname {} \; | xargs dirname)
-matrix_json=$(echo "$test_dirs" | jq -R '{"example-dir": .}' | jq -s . | jq -c .)
+test_dirs=$(find examples/*/scripts -type f -name 'test.sh' -print0 | xargs -0 dirname | xargs dirname)
+matrix_json=$(echo "${test_dirs}" | jq -R '{"example-dir": .}' | jq -s . | jq -c .)Option 2 (Recommended): Use the github-script approach as suggested in the previous review. Please refer to the previous suggestion about using github-script for a more robust implementation. 🧰 Tools🪛 actionlint19-19: shellcheck reported issue in this script: SC2038:warning:1:13: Use -print0/-0 or -exec + to allow for non-alphanumeric filenames (shellcheck) 19-19: shellcheck reported issue in this script: SC2086:info:3:31: Double quote to prevent globbing and word splitting (shellcheck)
Enhance test job with production-grade features. The test job would benefit from several production-ready improvements:
Please refer to the previous suggestion about enhancing the test job with production-grade features. Additionally, consider adding environment variables for the test configuration: test:
needs: setup-matrix
runs-on: ubuntu-latest
+ env:
+ NODE_ENV: test
+ TEST_NETWORK: localnet
strategy:
matrix:
include: ${{ fromJSON(needs.setup-matrix.outputs.matrix) }}
fail-fast: falseThank 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 (
|
|
I'm having issues with verification, so contracts are not yet verified. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 48
🧹 Outside diff range and nitpick comments (9)
examples/swap/tasks/deploy.ts (2)
38-43: Document the network context for these addresses.
Please add comments or update the parameter descriptions to specify which testnet (Base Sepolia, Polygon Amoy, or ZetaChain testnet) these addresses correspond to.
Line range hint 4-24: Add address validation in the deployment function.
The main function should validate the format of the contract addresses before deployment.
const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const network = hre.network.name as any;
+
+ // Validate contract addresses
+ if (!hre.ethers.utils.isAddress(args.systemContract)) {
+ throw new Error(`Invalid system contract address: ${args.systemContract}`);
+ }
+ if (!hre.ethers.utils.isAddress(args.gateway)) {
+ throw new Error(`Invalid gateway address: ${args.gateway}`);
+ }
const [signer] = await hre.ethers.getSigners();examples/hello/contracts/Hello.sol (1)
Line range hint 1-89: Enhance contract security with additional safeguards.
While the contract implements basic security measures, consider these improvements:
- Add events for critical operations in
callandwithdrawAndCall - Implement access control for external functions
- Add reentrancy guards for functions handling token transfers
Add the following improvements:
contract Hello is UniversalContract {
+ event CrossChainCallInitiated(address indexed sender, address indexed zrc20, uint256 amount);
+ event WithdrawAndCallInitiated(address indexed sender, address indexed zrc20, uint256 amount);
+
+ // Add reentrancy guard
+ bool private locked;
+
+ modifier noReentrant() {
+ require(!locked, "ReentrancyGuard: reentrant call");
+ locked = true;
+ _;
+ locked = false;
+ }
function call(
bytes memory receiver,
address zrc20,
bytes calldata message,
uint256 gasLimit,
RevertOptions memory revertOptions
- ) external {
+ ) external noReentrant {
(, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee))
revert TransferFailed();
IZRC20(zrc20).approve(address(gateway), gasFee);
+ emit CrossChainCallInitiated(msg.sender, zrc20, gasFee);
gateway.call(receiver, zrc20, message, gasLimit, revertOptions);
}
function withdrawAndCall(
bytes memory receiver,
uint256 amount,
address zrc20,
bytes calldata message,
uint256 gasLimit,
RevertOptions memory revertOptions
- ) external {
+ ) external noReentrant {
// ... existing code ...
+ emit WithdrawAndCallInitiated(msg.sender, zrc20, amount);
gateway.withdrawAndCall(
receiver,
amount,
zrc20,
message,
gasLimit,
revertOptions
);
}examples/swap/contracts/Swap.sol (1)
Line range hint 29-99: Critical: Add security measures for financial operations.
The contract handles critical financial operations but lacks several important security measures:
-
Reentrancy Protection:
- Add ReentrancyGuard from OpenZeppelin
- Implement checks-effects-interactions pattern
-
Token Approvals:
- Reset approvals to 0 before setting new values
- Consider using
safeApproveorsafeIncreaseAllowance
-
Input Validation:
- Add zero address checks for recipients
- Validate amount is non-zero
Apply these security improvements:
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
-contract Swap is UniversalContract {
+contract Swap is UniversalContract, ReentrancyGuard {
+ using SafeERC20 for IERC20;
// ... existing code ...
function swapAndWithdraw(
address inputToken,
uint256 amount,
address targetToken,
bytes memory recipient
- ) internal {
+ ) internal nonReentrant {
+ require(amount > 0, "Swap: amount must be positive");
+ require(targetToken != address(0), "Swap: invalid target token");
+ require(recipient.length > 0, "Swap: invalid recipient");
// ... existing code ...
if (gasZRC20 == targetToken) {
- IZRC20(gasZRC20).approve(address(gateway), outputAmount + gasFee);
+ IZRC20(gasZRC20).safeApprove(address(gateway), 0);
+ IZRC20(gasZRC20).safeApprove(address(gateway), outputAmount + gasFee);
} else {
- IZRC20(gasZRC20).approve(address(gateway), gasFee);
- IZRC20(targetToken).approve(address(gateway), outputAmount);
+ IZRC20(gasZRC20).safeApprove(address(gateway), 0);
+ IZRC20(gasZRC20).safeApprove(address(gateway), gasFee);
+ IZRC20(targetToken).safeApprove(address(gateway), 0);
+ IZRC20(targetToken).safeApprove(address(gateway), outputAmount);
}examples/nft/tasks/transfer.ts (2)
105-105: Consider making 'amount' an optional parameter with a default value
The amount parameter at line 105 is currently required, but in practice, there may be scenarios where specifying an amount isn't necessary. Making it optional with a sensible default can improve usability.
Adjust the parameter definition:
-.addParam("amount", "The amount of gas to transfer", "0");
+.addOptionalParam("amount", "The amount of gas to transfer", "0");34-35: Clarify the usage of 'withdrawGasFeeWithGasLimit' function
The destructuring assignment at line 34 may not be immediately clear to readers. Explicitly naming the unused variable can enhance readability.
Improve the code clarity:
-const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
+const [unusedVariable, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);Alternatively, if the first returned value is not needed, consider omitting it in the function if possible.
examples/swap/contracts/SwapToAnyToken.sol (1)
Line range hint 31-36: Ensure onCall function has appropriate access control
The onCall function is external and can be called by any address. To prevent unauthorized access and enhance security, it is advisable to restrict this function so that only the gateway contract can invoke it.
Apply this diff to add an onlyGateway modifier:
+ modifier onlyGateway() {
+ require(msg.sender == address(gateway), "Caller is not the gateway");
+ _;
+ }
function onCall(
MessageContext calldata context,
address zrc20,
uint256 amount,
bytes calldata message
- ) external virtual override {
+ ) external virtual override onlyGateway {examples/nft/contracts/Universal.sol (2)
59-63: Check Allowance Before 'transferFrom' to Ensure Successful Gas Fee Transfer
The transferCrossChain function assumes that msg.sender has set an adequate allowance for the contract to transfer the gas fee. Explicitly checking the allowance can provide a clearer error message and improve user experience.
Add an allowance check before attempting the transfer:
(, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit);
+ uint256 allowance = IZRC20(zrc20).allowance(msg.sender, address(this));
+ require(allowance >= gasFee, "Insufficient allowance for gas fee");
if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee))
revert TransferFailed();75-75: Emit Event After Successful Cross-Chain Transfer
Emitting an event after initiating a cross-chain transfer enhances transparency and allows external services to track these operations efficiently.
Add an event emission for the cross-chain transfer:
gateway.call(receiver, zrc20, encodedData, callOptions, revertOptions);
+ emit CrossChainTransferInitiated(msg.sender, receiver, tokenId, zrc20);
}
+event CrossChainTransferInitiated(address indexed sender, bytes indexed receiver, uint256 tokenId, address indexed zrc20);📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
examples/hello/yarn.lockis excluded by!**/yarn.lock,!**/*.lockexamples/nft/yarn.lockis excluded by!**/yarn.lock,!**/*.lockexamples/swap/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
- examples/hello/contracts/Hello.sol (1 hunks)
- examples/hello/package.json (2 hunks)
- examples/hello/tasks/deploy.ts (1 hunks)
- examples/nft/.eslintignore (1 hunks)
- examples/nft/.eslintrc.js (1 hunks)
- examples/nft/.gitignore (1 hunks)
- examples/nft/LICENSE (1 hunks)
- examples/nft/README.md (1 hunks)
- examples/nft/contracts/Connected.sol (1 hunks)
- examples/nft/contracts/Universal.sol (1 hunks)
- examples/nft/foundry.toml (1 hunks)
- examples/nft/hardhat.config.ts (1 hunks)
- examples/nft/package.json (1 hunks)
- examples/nft/scripts/test.sh (1 hunks)
- examples/nft/tasks/connectedSetCounterparty.ts (1 hunks)
- examples/nft/tasks/deploy.ts (1 hunks)
- examples/nft/tasks/mint.ts (1 hunks)
- examples/nft/tasks/transfer.ts (1 hunks)
- examples/nft/tasks/universalSetCounterparty.ts (1 hunks)
- examples/nft/tsconfig.json (1 hunks)
- examples/swap/contracts/Swap.sol (1 hunks)
- examples/swap/contracts/SwapToAnyToken.sol (1 hunks)
- examples/swap/package.json (3 hunks)
- examples/swap/tasks/deploy.ts (1 hunks)
✅ Files skipped from review due to trivial changes (6)
- examples/nft/.eslintignore
- examples/nft/.gitignore
- examples/nft/LICENSE
- examples/nft/README.md
- examples/nft/foundry.toml
- examples/nft/tsconfig.json
🧰 Additional context used
🪛 Shellcheck
examples/nft/scripts/test.sh
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (17)
examples/nft/hardhat.config.ts (2)
3-9: LGTM: Task imports are well-organized.
The imports are properly structured, including essential Hardhat tooling and ZetaChain-specific tasks.
14-19: Verify Solidity version compatibility.
The configuration uses Solidity 0.8.26, which is relatively recent. Ensure all imported contracts and dependencies are compatible with this version.
Let's verify the compatibility:
✅ Verification successful
Solidity version 0.8.26 is compatible with all contracts in the codebase
The verification shows that all contracts in the project are using compatible Solidity versions:
- Most contracts explicitly use
pragma solidity 0.8.26 - One contract uses
pragma solidity ^0.8.26which is compatible - Only test/mock contracts use different versions (0.8.7 and 0.5.16), which is acceptable as they are not part of the production codebase
The dependencies are also compatible:
- @ZetaChain packages are being used with recent versions (10.x-13.x)
- Hardhat version (2.17.2) supports Solidity 0.8.26
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential version compatibility issues in Solidity files
# Test: Search for pragma statements in all Solidity files
echo "Checking Solidity version requirements across the project:"
rg -t sol "pragma solidity"
# Test: Check package.json for relevant dependencies
echo -e "\nChecking hardhat and related package versions:"
fd -g "package.json" --exec cat {} | jq -r '.dependencies + .devDependencies | with_entries(select(.key | contains("hardhat") or contains("solidity") or contains("@zetachain")))'
Length of output: 2694
Script:
#!/bin/bash
# Let's try a different approach to find Solidity files and their pragma versions
echo "Searching for .sol files:"
fd -e sol
echo -e "\nChecking content of Solidity files:"
fd -e sol --exec cat {}
# Also check if there are any Solidity files in node_modules/@zetachain
echo -e "\nChecking @zetachain contracts:"
fd -e sol . "./node_modules/@zetachain" || true
Length of output: 27978
examples/nft/.eslintrc.js (3)
7-17: LGTM: Environment and parser configuration
The environment and parser configuration is comprehensive and well-structured, covering all necessary runtime environments (Node.js, Mocha) and properly configured for ES2021.
34-46: Verify TypeScript project path resolution
The import resolver configuration looks correct, but ensure the tsconfig.json path is accurate.
✅ Verification successful
TypeScript configuration path verified and correctly configured
The tsconfig.json file exists at the expected location (examples/nft/tsconfig.json), confirming that the import resolver configuration in .eslintrc.js is correctly set up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of tsconfig.json in the NFT example directory
if [ -f "examples/nft/tsconfig.json" ]; then
echo "tsconfig.json exists at the specified location"
else
echo "Warning: tsconfig.json not found at the specified location"
fi
Length of output: 142
1-6: 🛠️ Refactor suggestion
Consider using ES modules for consistency
While the configuration is well-typed with JSDoc, consider using ES modules for better consistency with modern JavaScript practices.
-const path = require("path");
+import path from "path";
/**
* @type {import("eslint").Linter.Config}
*/
-module.exports = {
+export default {Likely invalid or redundant comment.
examples/nft/tasks/connectedSetCounterparty.ts (1)
34-38: LGTM!
The task registration is well-structured with clear parameter descriptions and appropriate optional parameters.
examples/nft/tasks/deploy.ts (1)
1-3: LGTM: Imports are clean and properly typed
The imports are minimal and correctly typed using the Hardhat types.
examples/nft/tasks/universalSetCounterparty.ts (2)
1-3: LGTM: Imports are properly structured.
The necessary Hardhat dependencies are correctly imported.
35-40: LGTM: Task registration is well structured.
The task registration includes clear parameter descriptions and follows Hardhat's best practices.
examples/nft/tasks/mint.ts (3)
1-4: LGTM: Imports are appropriate and minimal.
The necessary dependencies for Hardhat task implementation are correctly imported.
44-52: LGTM: Task configuration is well-structured.
The task parameters are well-defined with clear descriptions and appropriate optionality.
1-52: Verify cross-chain contract compatibility.
Since this task is part of a cross-chain NFT system, let's verify the contract interfaces.
examples/nft/package.json (1)
44-45: Verify ethers.js and Hardhat version compatibility.
The current setup uses ethers.js v5.4.7 with Hardhat v2.17.2. Ensure these versions are compatible to avoid potential issues.
Let's check the compatibility:
#!/bin/bash
# Check Hardhat's peer dependency requirements for ethers
npm info [email protected] peerDependencies --jsonexamples/hello/package.json (1)
60-60: Verify compatibility with @zetachain/toolkit.
The @zetachain/protocol-contracts dependency has been updated to 10.0.0-rc11. Let's verify its compatibility with @zetachain/[email protected].
examples/swap/package.json (1)
10-11: Verify alignment with PR objectives.
The PR objectives mention enabling gateway functionality on specific testnets (Base Sepolia, Polygon Amoy, ZetaChain testnet), but the added scripts are for localnet deployment. Consider adding deployment scripts for the target testnets mentioned in the PR objectives.
Would you like me to help generate the deployment scripts for the specified testnets?
examples/nft/contracts/Universal.sol (2)
134-142: Ensure 'onRevert' Handles Missing or Malformed Data Gracefully
The onRevert function decodes data from context.revertMessage without validation. If the message is missing or malformed, it could cause the function to revert unexpectedly.
Add input validation to handle potential decoding errors:
function onRevert(RevertContext calldata context) external onlyGateway {
+ require(context.revertMessage.length > 0, "Empty revert message");
(uint256 tokenId, address sender, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
);28-28: Consider Adjusting Default 'gasLimit' Value
The default gasLimit is set to 700,000. Depending on the complexity of cross-chain operations, this value might be insufficient or excessive, potentially causing failures or unnecessary costs.
Review recent transaction gas consumption to determine an optimal default value. Adjust as necessary to balance reliability and cost-effectiveness.
| import "./tasks/deploy"; | ||
| import "./tasks/deploy"; |
There was a problem hiding this comment.
Remove duplicate import statement.
The import statement for "./tasks/deploy" appears twice, which is redundant and could lead to confusion.
Apply this diff to fix the duplication:
import "./tasks/deploy";
-import "./tasks/deploy";📝 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.
| import "./tasks/deploy"; | |
| import "./tasks/deploy"; | |
| import "./tasks/deploy"; |
| networks: { | ||
| ...getHardhatConfigNetworks(), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider environment-specific network configurations.
While using getHardhatConfigNetworks() is convenient, consider adding environment-specific overrides for gas settings and other network parameters.
Consider enhancing the network configuration:
networks: {
...getHardhatConfigNetworks(),
+ // Add environment-specific overrides
+ hardhat: {
+ gas: "auto",
+ gasPrice: "auto",
+ gasMultiplier: 1.2
+ }
},Committable suggestion was skipped due to low confidence.
| "Gateway address (default: ZetaChain Gateway on testnet)", | ||
| "0x6c533f7fe93fae114d0954697069df33c9b74fd7" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using environment-specific gateway addresses
While the updated gateway address is correct for the testnet, hardcoding it directly in the task definition might cause issues when deploying to different environments. Consider implementing a more flexible configuration approach.
Here's a suggested implementation using network-specific configuration:
+ const GATEWAY_ADDRESSES = {
+ baseSepolia: "0x6c533f7fe93fae114d0954697069df33c9b74fd7",
+ polygonAmoy: "0x...", // Add Polygon Amoy gateway address
+ zetaChainTestnet: "0x...", // Add ZetaChain testnet gateway address
+ };
task("deploy", "Deploy the contract", main)
.addFlag("json", "Output in JSON")
.addOptionalParam("name", "Contract to deploy", "Hello")
.addOptionalParam(
"gateway",
- "Gateway address (default: ZetaChain Gateway on testnet)",
- "0x6c533f7fe93fae114d0954697069df33c9b74fd7"
+ "Gateway address (defaults to network-specific gateway address)",
+ undefined,
types.string
);Then update the main function to use network-specific addresses:
const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const network = hre.network.name;
const gatewayAddress = args.gateway || GATEWAY_ADDRESSES[network];
if (!gatewayAddress) {
throw new Error(`No gateway address configured for network: ${network}`);
}
// ... rest of the deployment logic
const contract = await (factory as any).deploy(gatewayAddress);💡 Codebase verification
Different gateway addresses detected across examples
The hello and NFT examples are using different gateway addresses:
- Hello example:
0x6c533f7fe93fae114d0954697069df33c9b74fd7 - NFT example:
0x5FC8d32690cc91D4c39d9d3abcBD16989F875707
This inconsistency in gateway addresses between examples should be addressed to prevent confusion. Consider:
- Using the same gateway address across examples if they target the same network
- Adding clear documentation about which network each gateway address corresponds to
- Using environment variables or configuration files to manage network-specific addresses
🔗 Analysis chain
Verify gateway address usage across networks
Let's verify how this gateway address is being used across different networks to ensure consistency.
Let me check the gateway address configuration in the NFT example to get a complete picture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gateway address usage across the codebase
echo "Searching for gateway address references..."
rg -i "0x6c533f7fe93fae114d0954697069df33c9b74fd7"
echo "\nSearching for potential gateway configurations..."
rg -l "gateway.*address" --type ts
Length of output: 406
Script:
#!/bin/bash
# Check the gateway address configuration in the NFT example
echo "Content of examples/nft/tasks/deploy.ts:"
cat examples/nft/tasks/deploy.ts
Length of output: 1498
| @@ -35,10 +35,10 @@ task("deploy", "Deploy the contract", main) | |||
| .addOptionalParam( | |||
| "systemContract", | |||
| "System contract", | |||
| "0x610178dA211FEF7D417bC0e6FeD39F05609AD788" | |||
| "0xA51c1fc2f0D1a1b8494Ed1FE312d7C3a78Ed91C0" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using environment-based configuration for contract addresses.
The hardcoded addresses make it difficult to switch between different environments (local, testnet, mainnet). Consider refactoring to use environment-specific configuration files or environment variables.
- "systemContract",
- "System contract",
- "0xA51c1fc2f0D1a1b8494Ed1FE312d7C3a78Ed91C0"
+ "systemContract",
+ "System contract address (from config/<network>.json)",
+ process.env.SYSTEM_CONTRACT_ADDRESS || getNetworkConfig().systemContract
- "gateway",
- "Gateway address (default: ZetaChain Gateway)",
- "0x9A676e781A523b5d0C0e43731313A708CB607508"
+ "gateway",
+ "ZetaChain Gateway address (from config/<network>.json)",
+ process.env.GATEWAY_ADDRESS || getNetworkConfig().gatewayAlso applies to: 43-43
| plugins: [ | ||
| "@typescript-eslint", | ||
| "prettier", | ||
| "simple-import-sort", | ||
| "sort-keys-fix", | ||
| "typescript-sort-keys", | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error prevention plugins
While the current plugins focus on code organization and formatting, consider adding plugins for error prevention:
plugins: [
"@typescript-eslint",
"prettier",
"simple-import-sort",
"sort-keys-fix",
"typescript-sort-keys",
+ "eslint-plugin-import",
+ "eslint-plugin-promise",
+ "eslint-plugin-security"
],📝 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.
| plugins: [ | |
| "@typescript-eslint", | |
| "prettier", | |
| "simple-import-sort", | |
| "sort-keys-fix", | |
| "typescript-sort-keys", | |
| ], | |
| plugins: [ | |
| "@typescript-eslint", | |
| "prettier", | |
| "simple-import-sort", | |
| "sort-keys-fix", | |
| "typescript-sort-keys", | |
| "eslint-plugin-import", | |
| "eslint-plugin-promise", | |
| "eslint-plugin-security" | |
| ], |
| function safeMint(address to, string memory uri) public onlyOwner { | ||
| uint256 hash = uint256( | ||
| keccak256( | ||
| abi.encodePacked(address(this), block.number, _nextTokenId++) | ||
| ) | ||
| ); | ||
|
|
||
| uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; | ||
|
|
||
| _safeMint(to, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify Token ID Generation to Prevent Potential Collisions
The current token ID generation uses a hash of multiple parameters, which may introduce unnecessary complexity and a slight risk of collision. Using an incrementing counter ensures uniqueness and simplifies the implementation.
Apply this diff to simplify token ID generation:
function safeMint(address to, string memory uri) public onlyOwner {
- uint256 hash = uint256(
- keccak256(
- abi.encodePacked(address(this), block.number, _nextTokenId++)
- )
- );
-
- uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF;
+ uint256 tokenId = _nextTokenId++;
_safeMint(to, tokenId);
_setTokenURI(tokenId, uri);
}📝 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 safeMint(address to, string memory uri) public onlyOwner { | |
| uint256 hash = uint256( | |
| keccak256( | |
| abi.encodePacked(address(this), block.number, _nextTokenId++) | |
| ) | |
| ); | |
| uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; | |
| _safeMint(to, tokenId); | |
| _setTokenURI(tokenId, uri); | |
| } | |
| function safeMint(address to, string memory uri) public onlyOwner { | |
| uint256 tokenId = _nextTokenId++; | |
| _safeMint(to, tokenId); | |
| _setTokenURI(tokenId, uri); | |
| } |
| function onRevert(RevertContext calldata context) external { | ||
| (uint256 tokenId, address sender, string memory uri) = abi.decode( | ||
| context.revertMessage, | ||
| (uint256, address, string) | ||
| ); | ||
|
|
||
| _safeMint(sender, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| } |
There was a problem hiding this comment.
Restrict 'onRevert' Function to Authorized Caller
The onRevert function lacks access control, allowing any external account to invoke it and potentially mint tokens without authorization. This poses a significant security risk.
Apply this diff to secure the onRevert function:
function onRevert(RevertContext calldata context)
- external
+ external
+ onlyGateway
{
(uint256 tokenId, address sender, string memory uri) = abi.decode(
context.revertMessage,
(uint256, address, string)
);Committable suggestion was skipped due to low confidence.
examples/nft/contracts/Universal.sol
Outdated
| function onCall( | ||
| MessageContext calldata messageContext, | ||
| address zrc20, | ||
| uint256 amount, | ||
| bytes calldata message | ||
| ) external override { | ||
| if (keccak256(messageContext.origin) != keccak256(counterparty[zrc20])) | ||
| revert("Unauthorized"); | ||
|
|
||
| ( | ||
| uint256 tokenId, | ||
| address sender, | ||
| string memory uri, | ||
| address destination | ||
| ) = abi.decode(message, (uint256, address, string, address)); | ||
|
|
||
| if (destination == address(0)) { | ||
| _safeMint(sender, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| } else { | ||
| (, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit( | ||
| 700000 | ||
| ); | ||
|
|
||
| SwapHelperLib.swapExactTokensForTokens( | ||
| systemContract, | ||
| zrc20, | ||
| amount, | ||
| destination, | ||
| 0 | ||
| ); | ||
|
|
||
| IZRC20(destination).approve(address(gateway), gasFee); | ||
| gateway.call( | ||
| counterparty[destination], | ||
| destination, | ||
| abi.encode(tokenId, sender, uri), | ||
| CallOptions(700000, false), | ||
| RevertOptions(address(0), false, address(0), "", 0) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Enforce Access Control on 'onCall' Function
While the onCall function verifies the message origin, it's crucial to restrict function access to the gateway contract to prevent unauthorized calls that could compromise the contract's integrity.
Apply this diff to add the onlyGateway modifier to the onCall function:
+ modifier onlyGateway() {
+ require(msg.sender == address(gateway), "Unauthorized");
+ _;
+ }
function onCall(
MessageContext calldata messageContext,
address zrc20,
uint256 amount,
bytes calldata message
- ) external override {
+ ) external override onlyGateway {
if (keccak256(messageContext.origin) != keccak256(counterparty[zrc20]))
revert("Unauthorized");Committable suggestion was skipped due to low confidence.
| SystemContract public immutable systemContract = | ||
| SystemContract(0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Hardcoding 'systemContract' Address; Make It Configurable
Hardcoding the systemContract address limits the contract's flexibility across different networks or environments. Consider passing this address as a constructor parameter to enhance configurability and maintainability.
Apply this diff to make the systemContract address configurable:
- SystemContract public immutable systemContract =
- SystemContract(0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9);
+ SystemContract public immutable systemContract;
uint256 private _nextTokenId;
bool public isUniversal = true;
uint256 public gasLimit = 700000;
error TransferFailed();
constructor(
address payable gatewayAddress,
address initialOwner
+ , address systemContractAddress
) ERC721("MyToken", "MTK") Ownable(initialOwner) {
gateway = GatewayZEVM(gatewayAddress);
+ systemContract = SystemContract(systemContractAddress);
}Committable suggestion was skipped due to low confidence.
examples/nft/contracts/Universal.sol
Outdated
| if (keccak256(messageContext.origin) != keccak256(counterparty[zrc20])) | ||
| revert("Unauthorized"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Storage of Counterparty Addresses
Using keccak256 for byte-by-byte comparison can be gas-intensive. Storing the hashed values of counterparty addresses can optimize performance during comparisons.
Modify the storage structure and comparison logic:
- mapping(address => bytes) public counterparty;
+ mapping(address => bytes32) public counterpartyHash;
event CounterpartySet(address indexed zrc20, bytes indexed contractAddress);
function setCounterparty(
address zrc20,
bytes memory contractAddress
) external onlyOwner {
- counterparty[zrc20] = contractAddress;
+ counterpartyHash[zrc20] = keccak256(contractAddress);
emit CounterpartySet(zrc20, contractAddress);
}
function onCall(
MessageContext calldata messageContext,
address zrc20,
uint256 amount,
bytes calldata message
) external override onlyGateway {
- if (keccak256(messageContext.origin) != keccak256(counterparty[zrc20]))
+ if (keccak256(messageContext.origin) != counterpartyHash[zrc20])
revert("Unauthorized");Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (2)
examples/nft/tasks/transfer.ts (1)
63-98: LGTM: Well-structured task definition with comprehensive parameters.The task parameters are well-documented and properly typed. Consider adding parameter validation hints using the types system for the
gasAmountparameter.Apply this enhancement:
.addParam("gasAmount", "The amount of gas to transfer", "0"); + .addPositionalParam("gasAmount", "The amount of gas to transfer", "0", types.string, true);examples/nft/contracts/Universal.sol (1)
1-22: Add NatSpec documentation and interface definition.The contract lacks comprehensive documentation and a clearly defined interface. This makes it harder for other developers to understand and integrate with the contract.
Add NatSpec documentation:
+/// @title Universal NFT Contract +/// @notice Implements cross-chain NFT functionality using ZetaChain protocol +/// @dev Extends ERC721 with cross-chain transfer capabilities contract Universal is ERC721, ERC721Enumerable, ERC721URIStorage, Ownable, UniversalContract {Consider extracting the public interface to a separate file:
interface IUniversal { function setCounterparty(address zrc20, bytes memory contractAddress) external; function transferCrossChain(uint256 tokenId, address receiver, address destination) external; function safeMint(address to, string memory uri) external; // ... other public functions }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
examples/nft/contracts/Connected.sol(1 hunks)examples/nft/contracts/Universal.sol(1 hunks)examples/nft/scripts/test.sh(1 hunks)examples/nft/tasks/transfer.ts(1 hunks)
🧰 Additional context used
🪛 Shellcheck
examples/nft/scripts/test.sh
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (5)
examples/nft/scripts/test.sh (2)
27-34:
Improve contract deployment reliability.
The current implementation lacks proper validation of deployment success and address parsing.
Apply this diff to add robust error handling:
+function deploy_contract() {
+ local network=$1
+ local name=$2
+ local extra_args=${3:-""}
+ local result address
+
+ echo -e "\n🚀 Deploying contract on $network..."
+ if ! result=$(npx hardhat deploy --network "$network" $extra_args --json 2>&1); then
+ echo "❌ Deployment failed on $network: $result"
+ return 1
+ fi
+
+ if ! address=$(echo "$result" | jq -r '.contractAddress'); then
+ echo "❌ Failed to parse deployment result"
+ return 1
+ fi
+
+ if [[ ! "$address" =~ ^0x[a-fA-F0-9]{40}$ ]]; then
+ echo "❌ Invalid contract address: $address"
+ return 1
+ fi
+
+ echo "$address"
+}
+
-CONTRACT_ZETACHAIN=$(npx hardhat deploy --network localhost --json | jq -r '.contractAddress')
+CONTRACT_ZETACHAIN=$(deploy_contract "localhost" "Universal") || exit 1
echo -e "\n🚀 Deployed NFT contract on ZetaChain: $CONTRACT_ZETACHAIN"
-CONTRACT_ETHEREUM=$(npx hardhat deploy --name Connected --json --network localhost --gateway "$GATEWAY_ETHEREUM" | jq -r '.contractAddress')
+CONTRACT_ETHEREUM=$(deploy_contract "localhost" "Connected" "--gateway $GATEWAY_ETHEREUM") || exit 1
echo -e "🚀 Deployed NFT contract on EVM chain: $CONTRACT_ETHEREUM"
-CONTRACT_BNB=$(npx hardhat deploy --name Connected --json --network localhost --gateway "$GATEWAY_BNB" | jq -r '.contractAddress')
+CONTRACT_BNB=$(deploy_contract "localhost" "Connected" "--gateway $GATEWAY_BNB") || exit 1
echo -e "🚀 Deployed NFT contract on BNB chain: $CONTRACT_BNB"Likely invalid or redundant comment.
20-24:
Use environment variables for network configuration.
Hardcoded addresses make the script environment-specific and harder to maintain.
Apply this diff to improve configurability:
-ZRC20_ETHEREUM=0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe
-ZRC20_BNB=0x65a45c57636f9BcCeD4fe193A602008578BcA90b
-GATEWAY_ETHEREUM=0x610178dA211FEF7D417bC0e6FeD39F05609AD788
-GATEWAY_BNB=0x3Aa5ebB10DC797CAC828524e59A333d0A371443c
+: "${ZRC20_ETHEREUM:?'ZRC20_ETHEREUM environment variable is required'}"
+: "${ZRC20_BNB:?'ZRC20_BNB environment variable is required'}"
+: "${GATEWAY_ETHEREUM:?'GATEWAY_ETHEREUM environment variable is required'}"
+: "${GATEWAY_BNB:?'GATEWAY_BNB environment variable is required'}"Likely invalid or redundant comment.
examples/nft/tasks/transfer.ts (2)
1-4: LGTM: Imports are well-structured and appropriate.
The necessary dependencies are correctly imported for Hardhat task implementation and ZRC20 interaction.
55-59:
Fix undefined nftContract variable in console output.
The console output references args.nftContract which is undefined. The contract address is already available in args.from.
Apply this change:
console.log(`🚀 Successfully transferred NFT to the contract.
📜 Contract address: ${args.from}
-🖼 NFT Contract address: ${args.nftContract}
+🖼 NFT Contract address: ${args.from}
🆔 Token ID: ${args.tokenId}
🔗 Transaction hash: ${tx.hash}`);examples/nft/contracts/Universal.sol (1)
155-185: LGTM! Override implementations are correct.
The override functions properly implement the required functionality from parent contracts and maintain the expected behavior.
examples/nft/scripts/test.sh
Outdated
| function nft_balance() { | ||
| echo -e "\n🖼️ NFT Balance" | ||
| echo "---------------------------------------------" | ||
| local ZETACHAIN=$(cast call "$CONTRACT_ZETACHAIN" "balanceOf(address)(uint256)" "$SENDER") | ||
| local ETHEREUM=$(cast call "$CONTRACT_ETHEREUM" "balanceOf(address)(uint256)" "$SENDER") | ||
| local BNB=$(cast call "$CONTRACT_BNB" "balanceOf(address)(uint256)" "$SENDER") | ||
| echo "🟢 ZetaChain: $ZETACHAIN" | ||
| echo "🔵 EVM Chain: $ETHEREUM" | ||
| echo "🟡 BNB Chain: $BNB" | ||
| echo "---------------------------------------------" | ||
| } |
There was a problem hiding this comment.
Enhance error handling in the nft_balance function.
The current implementation needs improvement in error handling and variable declarations.
Apply this diff to improve reliability:
function nft_balance() {
echo -e "\n🖼️ NFT Balance"
echo "---------------------------------------------"
- local ZETACHAIN=$(cast call "$CONTRACT_ZETACHAIN" "balanceOf(address)(uint256)" "$SENDER")
- local ETHEREUM=$(cast call "$CONTRACT_ETHEREUM" "balanceOf(address)(uint256)" "$SENDER")
- local BNB=$(cast call "$CONTRACT_BNB" "balanceOf(address)(uint256)" "$SENDER")
+ local ZETACHAIN ETHEREUM BNB
+
+ if ! ZETACHAIN=$(cast call "$CONTRACT_ZETACHAIN" "balanceOf(address)(uint256)" "$SENDER"); then
+ echo "❌ Failed to fetch ZetaChain balance"
+ return 1
+ fi
+ if ! ETHEREUM=$(cast call "$CONTRACT_ETHEREUM" "balanceOf(address)(uint256)" "$SENDER"); then
+ echo "❌ Failed to fetch Ethereum balance"
+ return 1
+ fi
+ if ! BNB=$(cast call "$CONTRACT_BNB" "balanceOf(address)(uint256)" "$SENDER"); then
+ echo "❌ Failed to fetch BNB balance"
+ return 1
+ fi
echo "🟢 ZetaChain: $ZETACHAIN"
echo "🔵 EVM Chain: $ETHEREUM"
echo "🟡 BNB Chain: $BNB"
echo "---------------------------------------------"
}📝 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 nft_balance() { | |
| echo -e "\n🖼️ NFT Balance" | |
| echo "---------------------------------------------" | |
| local ZETACHAIN=$(cast call "$CONTRACT_ZETACHAIN" "balanceOf(address)(uint256)" "$SENDER") | |
| local ETHEREUM=$(cast call "$CONTRACT_ETHEREUM" "balanceOf(address)(uint256)" "$SENDER") | |
| local BNB=$(cast call "$CONTRACT_BNB" "balanceOf(address)(uint256)" "$SENDER") | |
| echo "🟢 ZetaChain: $ZETACHAIN" | |
| echo "🔵 EVM Chain: $ETHEREUM" | |
| echo "🟡 BNB Chain: $BNB" | |
| echo "---------------------------------------------" | |
| } | |
| function nft_balance() { | |
| echo -e "\n🖼️ NFT Balance" | |
| echo "---------------------------------------------" | |
| local ZETACHAIN ETHEREUM BNB | |
| if ! ZETACHAIN=$(cast call "$CONTRACT_ZETACHAIN" "balanceOf(address)(uint256)" "$SENDER"); then | |
| echo "❌ Failed to fetch ZetaChain balance" | |
| return 1 | |
| fi | |
| if ! ETHEREUM=$(cast call "$CONTRACT_ETHEREUM" "balanceOf(address)(uint256)" "$SENDER"); then | |
| echo "❌ Failed to fetch Ethereum balance" | |
| return 1 | |
| fi | |
| if ! BNB=$(cast call "$CONTRACT_BNB" "balanceOf(address)(uint256)" "$SENDER"); then | |
| echo "❌ Failed to fetch BNB balance" | |
| return 1 | |
| fi | |
| echo "🟢 ZetaChain: $ZETACHAIN" | |
| echo "🔵 EVM Chain: $ETHEREUM" | |
| echo "🟡 BNB Chain: $BNB" | |
| echo "---------------------------------------------" | |
| } |
🧰 Tools
🪛 Shellcheck
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 9-9: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
| echo -e "\n🔗 Setting counterparty contracts..." | ||
| npx hardhat connected-set-counterparty --network localhost --contract "$CONTRACT_ETHEREUM" --counterparty "$CONTRACT_ZETACHAIN" --json &>/dev/null | ||
| npx hardhat connected-set-counterparty --network localhost --contract "$CONTRACT_BNB" --counterparty "$CONTRACT_ZETACHAIN" --json &>/dev/null | ||
| npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_ETHEREUM" --zrc20 "$ZRC20_ETHEREUM" --json &>/dev/null | ||
| npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" --json &>/dev/null |
There was a problem hiding this comment.
Improve error visibility in counterparty setup.
Redirecting all output to /dev/null makes debugging difficult.
Apply this diff to improve error handling:
+function set_counterparty() {
+ local cmd=$1
+ shift
+ if ! npx hardhat "$cmd" "$@" --json 2>counterparty_errors.log >/dev/null; then
+ echo "❌ Failed to set counterparty: $cmd"
+ cat counterparty_errors.log
+ return 1
+ fi
+}
+
echo -e "\n🔗 Setting counterparty contracts..."
-npx hardhat connected-set-counterparty --network localhost --contract "$CONTRACT_ETHEREUM" --counterparty "$CONTRACT_ZETACHAIN" --json &>/dev/null
-npx hardhat connected-set-counterparty --network localhost --contract "$CONTRACT_BNB" --counterparty "$CONTRACT_ZETACHAIN" --json &>/dev/null
-npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_ETHEREUM" --zrc20 "$ZRC20_ETHEREUM" --json &>/dev/null
-npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" --json &>/dev/null
+set_counterparty "connected-set-counterparty" --network localhost --contract "$CONTRACT_ETHEREUM" --counterparty "$CONTRACT_ZETACHAIN" || exit 1
+set_counterparty "connected-set-counterparty" --network localhost --contract "$CONTRACT_BNB" --counterparty "$CONTRACT_ZETACHAIN" || exit 1
+set_counterparty "universal-set-counterparty" --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_ETHEREUM" --zrc20 "$ZRC20_ETHEREUM" || exit 1
+set_counterparty "universal-set-counterparty" --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" || exit 1📝 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.
| echo -e "\n🔗 Setting counterparty contracts..." | |
| npx hardhat connected-set-counterparty --network localhost --contract "$CONTRACT_ETHEREUM" --counterparty "$CONTRACT_ZETACHAIN" --json &>/dev/null | |
| npx hardhat connected-set-counterparty --network localhost --contract "$CONTRACT_BNB" --counterparty "$CONTRACT_ZETACHAIN" --json &>/dev/null | |
| npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_ETHEREUM" --zrc20 "$ZRC20_ETHEREUM" --json &>/dev/null | |
| npx hardhat universal-set-counterparty --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" --json &>/dev/null | |
| function set_counterparty() { | |
| local cmd=$1 | |
| shift | |
| if ! npx hardhat "$cmd" "$@" --json 2>counterparty_errors.log >/dev/null; then | |
| echo "❌ Failed to set counterparty: $cmd" | |
| cat counterparty_errors.log | |
| return 1 | |
| fi | |
| } | |
| echo -e "\n🔗 Setting counterparty contracts..." | |
| set_counterparty "connected-set-counterparty" --network localhost --contract "$CONTRACT_ETHEREUM" --counterparty "$CONTRACT_ZETACHAIN" || exit 1 | |
| set_counterparty "connected-set-counterparty" --network localhost --contract "$CONTRACT_BNB" --counterparty "$CONTRACT_ZETACHAIN" || exit 1 | |
| set_counterparty "universal-set-counterparty" --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_ETHEREUM" --zrc20 "$ZRC20_ETHEREUM" || exit 1 | |
| set_counterparty "universal-set-counterparty" --network localhost --contract "$CONTRACT_ZETACHAIN" --counterparty "$CONTRACT_BNB" --zrc20 "$ZRC20_BNB" || exit 1 |
examples/nft/scripts/test.sh
Outdated
| echo -e "\nTransferring NFT: ZetaChain → Ethereum..." | ||
| npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM" | ||
|
|
||
| nft_balance | ||
|
|
||
| echo -e "\nTransferring NFT: Ethereum → BNB..." | ||
| npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ETHEREUM" --to "$ZRC20_BNB" --gas-amount 0.1 | ||
|
|
||
| nft_balance | ||
|
|
||
| echo -e "\nTransferring NFT: BNB → ZetaChain..." | ||
| npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_BNB" | ||
|
|
There was a problem hiding this comment.
Enhance cross-chain transfer reliability.
Cross-chain transfers need robust error handling and timeout mechanisms.
Apply this diff to improve reliability:
+function transfer_nft() {
+ local from=$1
+ local to=$2
+ local token_id=$3
+ local gas_amount=${4:-""}
+ local max_attempts=3
+ local timeout=300
+
+ echo "🔄 Initiating transfer from $from..."
+
+ local attempt=1
+ while [ $attempt -le $max_attempts ]; do
+ if timeout $timeout npx hardhat transfer --network localhost --json \
+ --token-id "$token_id" --from "$from" ${to:+--to "$to"} ${gas_amount:+--gas-amount "$gas_amount"}; then
+ echo "✅ Transfer successful"
+ return 0
+ fi
+
+ echo "⚠️ Attempt $attempt failed. Retrying..."
+ ((attempt++))
+ sleep 5
+ done
+
+ echo "❌ Transfer failed after $max_attempts attempts"
+ return 1
+}
+
-echo -e "\nTransferring NFT: ZetaChain → Ethereum..."
-npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM"
+echo -e "\nTransferring NFT: ZetaChain → Ethereum..."
+transfer_nft "$CONTRACT_ZETACHAIN" "$ZRC20_ETHEREUM" "$NFT_ID" || exit 1
-echo -e "\nTransferring NFT: Ethereum → BNB..."
-npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ETHEREUM" --to "$ZRC20_BNB" --gas-amount 0.1
+echo -e "\nTransferring NFT: Ethereum → BNB..."
+transfer_nft "$CONTRACT_ETHEREUM" "$ZRC20_BNB" "$NFT_ID" "0.1" || exit 1
-echo -e "\nTransferring NFT: BNB → ZetaChain..."
-npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_BNB"
+echo -e "\nTransferring NFT: BNB → ZetaChain..."
+transfer_nft "$CONTRACT_BNB" "" "$NFT_ID" || exit 1📝 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.
| echo -e "\nTransferring NFT: ZetaChain → Ethereum..." | |
| npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM" | |
| nft_balance | |
| echo -e "\nTransferring NFT: Ethereum → BNB..." | |
| npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ETHEREUM" --to "$ZRC20_BNB" --gas-amount 0.1 | |
| nft_balance | |
| echo -e "\nTransferring NFT: BNB → ZetaChain..." | |
| npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_BNB" | |
| function transfer_nft() { | |
| local from=$1 | |
| local to=$2 | |
| local token_id=$3 | |
| local gas_amount=${4:-""} | |
| local max_attempts=3 | |
| local timeout=300 | |
| echo "🔄 Initiating transfer from $from..." | |
| local attempt=1 | |
| while [ $attempt -le $max_attempts ]; do | |
| if timeout $timeout npx hardhat transfer --network localhost --json \ | |
| --token-id "$token_id" --from "$from" ${to:+--to "$to"} ${gas_amount:+--gas-amount "$gas_amount"}; then | |
| echo "✅ Transfer successful" | |
| return 0 | |
| fi | |
| echo "⚠️ Attempt $attempt failed. Retrying..." | |
| ((attempt++)) | |
| sleep 5 | |
| done | |
| echo "❌ Transfer failed after $max_attempts attempts" | |
| return 1 | |
| } | |
| echo -e "\nTransferring NFT: ZetaChain → Ethereum..." | |
| transfer_nft "$CONTRACT_ZETACHAIN" "$ZRC20_ETHEREUM" "$NFT_ID" || exit 1 | |
| nft_balance | |
| echo -e "\nTransferring NFT: Ethereum → BNB..." | |
| transfer_nft "$CONTRACT_ETHEREUM" "$ZRC20_BNB" "$NFT_ID" "0.1" || exit 1 | |
| nft_balance | |
| echo -e "\nTransferring NFT: BNB → ZetaChain..." | |
| transfer_nft "$CONTRACT_BNB" "" "$NFT_ID" || exit 1 |
| const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit); | ||
| const zrc20 = new ethers.Contract(args.to, ZRC20ABI.abi, signer); | ||
| const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | ||
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | ||
| await zrc20TransferTx.wait(); |
There was a problem hiding this comment.
Add error handling for gas fee approval.
The ZRC20 approval transaction could fail, but there's no error handling.
Apply this change:
const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
- const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions);
- await zrc20TransferTx.wait();
+ try {
+ const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions);
+ await zrc20TransferTx.wait();
+ } catch (error) {
+ throw new Error(`Failed to approve gas fee: ${error.message}`);
+ }📝 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.
| const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit); | |
| const zrc20 = new ethers.Contract(args.to, ZRC20ABI.abi, signer); | |
| const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | |
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | |
| await zrc20TransferTx.wait(); | |
| const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit); | |
| const zrc20 = new ethers.Contract(args.to, ZRC20ABI.abi, signer); | |
| const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | |
| try { | |
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | |
| await zrc20TransferTx.wait(); | |
| } catch (error) { | |
| throw new Error(`Failed to approve gas fee: ${error.message}`); | |
| } |
| const main = async (args: any, hre: HardhatRuntimeEnvironment) => { | ||
| const { ethers } = hre; | ||
| const [signer] = await ethers.getSigners(); | ||
| const nftContract = await ethers.getContractAt("IERC721", args.from); | ||
| const approveTx = await nftContract | ||
| .connect(signer) | ||
| .approve(args.from, args.tokenId); | ||
| await approveTx.wait(); | ||
|
|
There was a problem hiding this comment.
Add input validation for critical parameters.
The function should validate the token ID and contract address before proceeding with the approval.
Apply this change:
const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
const { ethers } = hre;
const [signer] = await ethers.getSigners();
+ if (!ethers.utils.isAddress(args.from)) {
+ throw new Error("Invalid contract address");
+ }
+ if (!args.tokenId || isNaN(args.tokenId)) {
+ throw new Error("Invalid token ID");
+ }
const nftContract = await ethers.getContractAt("IERC721", args.from);📝 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.
| const main = async (args: any, hre: HardhatRuntimeEnvironment) => { | |
| const { ethers } = hre; | |
| const [signer] = await ethers.getSigners(); | |
| const nftContract = await ethers.getContractAt("IERC721", args.from); | |
| const approveTx = await nftContract | |
| .connect(signer) | |
| .approve(args.from, args.tokenId); | |
| await approveTx.wait(); | |
| const main = async (args: any, hre: HardhatRuntimeEnvironment) => { | |
| const { ethers } = hre; | |
| const [signer] = await ethers.getSigners(); | |
| if (!ethers.utils.isAddress(args.from)) { | |
| throw new Error("Invalid contract address"); | |
| } | |
| if (!args.tokenId || isNaN(args.tokenId)) { | |
| throw new Error("Invalid token ID"); | |
| } | |
| const nftContract = await ethers.getContractAt("IERC721", args.from); | |
| const approveTx = await nftContract | |
| .connect(signer) | |
| .approve(args.from, args.tokenId); | |
| await approveTx.wait(); |
| let contract; | ||
| try { | ||
| contract = await ethers.getContractAt("Universal", args.from); | ||
| await (contract as any).isUniversal(); | ||
| const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit); | ||
| const zrc20 = new ethers.Contract(args.to, ZRC20ABI.abi, signer); | ||
| const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | ||
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | ||
| await zrc20TransferTx.wait(); | ||
| } catch (e) { | ||
| contract = await ethers.getContractAt("Connected", args.from); | ||
| } |
There was a problem hiding this comment.
Enhance error handling in contract type detection.
The current try-catch block silently fails and defaults to Connected contract, which could mask configuration issues.
Apply this change:
let contract;
try {
contract = await ethers.getContractAt("Universal", args.from);
- await (contract as any).isUniversal();
+ const isUniversal = await (contract as any).isUniversal();
+ if (!isUniversal) {
+ throw new Error("Contract does not implement Universal interface");
+ }
const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit);
const zrc20 = new ethers.Contract(args.to, ZRC20ABI.abi, signer);
const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions);
await zrc20TransferTx.wait();
} catch (e) {
+ console.log("Falling back to Connected contract:", e.message);
contract = await ethers.getContractAt("Connected", args.from);
}📝 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.
| let contract; | |
| try { | |
| contract = await ethers.getContractAt("Universal", args.from); | |
| await (contract as any).isUniversal(); | |
| const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit); | |
| const zrc20 = new ethers.Contract(args.to, ZRC20ABI.abi, signer); | |
| const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | |
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | |
| await zrc20TransferTx.wait(); | |
| } catch (e) { | |
| contract = await ethers.getContractAt("Connected", args.from); | |
| } | |
| let contract; | |
| try { | |
| contract = await ethers.getContractAt("Universal", args.from); | |
| const isUniversal = await (contract as any).isUniversal(); | |
| if (!isUniversal) { | |
| throw new Error("Contract does not implement Universal interface"); | |
| } | |
| const gasLimit = hre.ethers.BigNumber.from(args.txOptionsGasLimit); | |
| const zrc20 = new ethers.Contract(args.to, ZRC20ABI.abi, signer); | |
| const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | |
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | |
| await zrc20TransferTx.wait(); | |
| } catch (e) { | |
| console.log("Falling back to Connected contract:", e.message); | |
| contract = await ethers.getContractAt("Connected", args.from); | |
| } |
| function transferCrossChain( | ||
| uint256 tokenId, | ||
| address receiver, | ||
| address destination | ||
| ) public { | ||
| string memory uri = tokenURI(tokenId); | ||
| _burn(tokenId); | ||
|
|
||
| (, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit( | ||
| gasLimit | ||
| ); | ||
| if ( | ||
| !IZRC20(destination).transferFrom(msg.sender, address(this), gasFee) | ||
| ) revert TransferFailed(); | ||
| IZRC20(destination).approve(address(gateway), gasFee); | ||
| bytes memory encodedData = abi.encode(tokenId, receiver, uri); | ||
|
|
||
| CallOptions memory callOptions = CallOptions(gasLimit, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| encodedData, | ||
| gasLimit | ||
| ); | ||
|
|
||
| gateway.call( | ||
| counterparty[destination], | ||
| destination, | ||
| encodedData, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
| } |
There was a problem hiding this comment.
Add access control and implement checks-effects-interactions pattern.
The transferCrossChain function has several security concerns:
- No access control to verify if caller owns or is approved for the token
- Potential reentrancy vulnerability due to external calls after state changes
Apply this diff to fix the issues:
- function transferCrossChain(
+ function transferCrossChain(
uint256 tokenId,
address receiver,
address destination
- ) public {
+ ) public {
+ require(_isApprovedOrOwner(_msgSender(), tokenId), "Not authorized");
+
+ // Effects
string memory uri = tokenURI(tokenId);
_burn(tokenId);
+ // Interactions
(, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit(
gasLimit
);Committable suggestion was skipped due to low confidence.
| function onCall( | ||
| MessageContext calldata context, | ||
| address zrc20, | ||
| uint256 amount, | ||
| bytes calldata message | ||
| ) external override { | ||
| if (keccak256(context.origin) != keccak256(counterparty[zrc20])) | ||
| revert("Unauthorized"); | ||
|
|
||
| ( | ||
| uint256 tokenId, | ||
| address sender, | ||
| string memory uri, | ||
| address destination | ||
| ) = abi.decode(message, (uint256, address, string, address)); | ||
|
|
||
| if (destination == address(0)) { | ||
| _safeMint(sender, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| } else { | ||
| (, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit( | ||
| 700000 | ||
| ); | ||
|
|
||
| SwapHelperLib.swapExactTokensForTokens( | ||
| systemContract, | ||
| zrc20, | ||
| amount, | ||
| destination, | ||
| 0 | ||
| ); | ||
|
|
||
| IZRC20(destination).approve(address(gateway), gasFee); | ||
| gateway.call( | ||
| counterparty[destination], | ||
| destination, | ||
| abi.encode(tokenId, sender, uri), | ||
| CallOptions(700000, false), | ||
| RevertOptions(address(0), false, address(0), "", 0) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use consistent gas limit and add input validation.
The onCall function uses a hardcoded gas limit (700000) instead of the contract's gasLimit variable. Also, missing validation for the amount parameter.
Apply these fixes:
function onCall(
MessageContext calldata context,
address zrc20,
uint256 amount,
bytes calldata message
) external override {
if (keccak256(context.origin) != keccak256(counterparty[zrc20]))
revert("Unauthorized");
+ require(amount > 0, "Invalid amount");
// ... existing code ...
(, uint256 gasFee) = IZRC20(destination).withdrawGasFeeWithGasLimit(
- 700000
+ gasLimit
);
// ... existing code ...
gateway.call(
counterparty[destination],
destination,
abi.encode(tokenId, sender, uri),
- CallOptions(700000, false),
+ CallOptions(gasLimit, false),
RevertOptions(address(0), false, address(0), "", 0)
);Committable suggestion was skipped due to low confidence.
| ) external payable returns (bytes4) { | ||
| if (messageContext.sender != counterparty) revert("Unauthorized"); | ||
|
|
||
| (uint256 tokenId, address receiver, string memory uri) = abi.decode( | ||
| message, | ||
| (uint256, address, string) | ||
| ); | ||
| _safeMint(receiver, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Ensure onCall returns the correct bytes4 value
The onCall function is declared to return a bytes4 but currently returns an empty string "". To maintain compliance with Solidity best practices and interface conventions, the function should return a meaningful bytes4 value.
Apply this diff to correct the return value:
function onCall(
MessageContext calldata messageContext,
bytes calldata message
) external payable returns (bytes4) {
if (messageContext.sender != counterparty) revert("Unauthorized");
// ...
- return "";
+ return bytes4(keccak256("onCall(MessageContext,bytes)"));
}Alternatively, if there is a specific selector expected by the calling contract or interface, return that selector to ensure proper interaction.
📝 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.
| ) external payable returns (bytes4) { | |
| if (messageContext.sender != counterparty) revert("Unauthorized"); | |
| (uint256 tokenId, address receiver, string memory uri) = abi.decode( | |
| message, | |
| (uint256, address, string) | |
| ); | |
| _safeMint(receiver, tokenId); | |
| _setTokenURI(tokenId, uri); | |
| return ""; | |
| } | |
| ) external payable returns (bytes4) { | |
| if (messageContext.sender != counterparty) revert("Unauthorized"); | |
| (uint256 tokenId, address receiver, string memory uri) = abi.decode( | |
| message, | |
| (uint256, address, string) | |
| ); | |
| _safeMint(receiver, tokenId); | |
| _setTokenURI(tokenId, uri); | |
| return bytes4(keccak256("onCall(MessageContext,bytes)")); | |
| } |
|
@andresaiello please, review. |
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (1)
examples/swap/scripts/test.sh (1)
1-4: Add script documentation and improve error handlingThe script would benefit from a header comment describing its purpose and usage. Additionally, consider adding error handling for required environment variables.
#!/bin/bash + +# Test script for swap functionality +# Usage: ./test.sh [localnet] +# Prerequisites: yarn, npx, hardhat set -e +set -o pipefail + +# Verify required tools +command -v npx >/dev/null 2>&1 || { echo "npx is required but not installed" >&2; exit 1; } +command -v yarn >/dev/null 2>&1 || { echo "yarn is required but not installed" >&2; exit 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
examples/nft/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.github/workflows/test.yaml(1 hunks)examples/hello/scripts/test.sh(1 hunks)examples/nft/package.json(1 hunks)examples/nft/scripts/test.sh(1 hunks)examples/swap/scripts/test.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/nft/package.json
🧰 Additional context used
🪛 actionlint
.github/workflows/test.yaml
18-18: shellcheck reported issue in this script: SC2038:warning:1:13: Use -print0/-0 or -exec + to allow for non-alphanumeric filenames
(shellcheck)
18-18: shellcheck reported issue in this script: SC2086:info:3:31: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 Shellcheck
examples/nft/scripts/test.sh
[warning] 10-10: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 11-11: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 12-12: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (1)
examples/swap/scripts/test.sh (1)
1-17: Verify gateway addresses in script
Given that this PR is about gateway testnet functionality, ensure the hardcoded addresses match the gateway addresses mentioned in the PR objectives.
| npx hardhat swap-from-evm --network localhost --receiver 0x84eA74d481Ee0A5332c457a4d796187F6Ba67fEB --amount 1 --target 0x9fd96203f7b22bCF72d9DCb40ff98302376cE09c --recipient 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repetitive swap commands
The swap commands contain hardcoded addresses and share similar parameters. Consider extracting these into variables and creating a reusable function.
+# Configuration
+RECEIVER_ADDRESS="0x84eA74d481Ee0A5332c457a4d796187F6Ba67fEB"
+RECIPIENT_ADDRESS="0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266"
+
+perform_swap() {
+ local target=$1
+ local erc20=${2:-""}
+ local cmd="npx hardhat swap-from-evm --network localhost --receiver $RECEIVER_ADDRESS --amount 1 --target $target --recipient $RECIPIENT_ADDRESS"
+ if [ -n "$erc20" ]; then
+ cmd="$cmd --erc20 $erc20"
+ fi
+ echo "Executing swap: $cmd"
+ eval "$cmd"
+}
+
+# Perform swaps
+perform_swap "0x9fd96203f7b22bCF72d9DCb40ff98302376cE09c"
+perform_swap "0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe" "0x0B306BF915C4d645ff596e518fAf3F9669b97016"Also applies to: 15-15
|
|
||
| npx hardhat swap-from-evm --network localhost --receiver 0x84eA74d481Ee0A5332c457a4d796187F6Ba67fEB --amount 1 --target 0x9fd96203f7b22bCF72d9DCb40ff98302376cE09c --recipient 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 | ||
|
|
||
| npx hardhat localnet-check |
There was a problem hiding this comment.
Add error handling for network check
The network check should include proper error handling and logging.
-npx hardhat localnet-check
+if ! npx hardhat localnet-check; then
+ echo "Network check failed" >&2
+ npx hardhat localnet-stop
+ exit 1
+fi📝 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.
| npx hardhat localnet-check | |
| if ! npx hardhat localnet-check; then | |
| echo "Network check failed" >&2 | |
| npx hardhat localnet-stop | |
| exit 1 | |
| fi |
|
|
||
| npx hardhat swap-from-evm --network localhost --receiver 0x84eA74d481Ee0A5332c457a4d796187F6Ba67fEB --amount 1 --target 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --recipient 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 --erc20 0x0B306BF915C4d645ff596e518fAf3F9669b97016 | ||
|
|
||
| npx hardhat localnet-stop |
There was a problem hiding this comment.
Add cleanup trap
Ensure proper cleanup even if the script fails or is interrupted.
+cleanup() {
+ echo "Cleaning up..."
+ npx hardhat localnet-stop || true
+}
+
+trap cleanup EXIT
-npx hardhat localnet-stop📝 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.
| npx hardhat localnet-stop | |
| cleanup() { | |
| echo "Cleaning up..." | |
| npx hardhat localnet-stop || true | |
| } | |
| trap cleanup EXIT |
| npx hardhat localnet --exit-on-error & sleep 10 | ||
| fi | ||
|
|
||
| yarn deploy:localnet |
There was a problem hiding this comment.
Add deployment verification
The deployment command should include verification of successful deployment.
-yarn deploy:localnet
+if ! yarn deploy:localnet; then
+ echo "Deployment failed" >&2
+ exit 1
+fi📝 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.
| yarn deploy:localnet | |
| if ! yarn deploy:localnet; then | |
| echo "Deployment failed" >&2 | |
| exit 1 | |
| fi |
| if [ "$1" = "localnet" ]; then | ||
| npx hardhat localnet --exit-on-error & sleep 10 | ||
| fi |
There was a problem hiding this comment.
Enhance local network startup reliability
The current implementation uses a fixed sleep duration which might be insufficient on slower systems. Consider implementing a more robust check for network availability.
if [ "$1" = "localnet" ]; then
- npx hardhat localnet --exit-on-error & sleep 10
+ npx hardhat localnet --exit-on-error &
+ NETWORK_PID=$!
+ echo "Waiting for local network to start..."
+ for i in {1..30}; do
+ if npx hardhat localnet-status &>/dev/null; then
+ echo "Local network is ready"
+ break
+ fi
+ if ! kill -0 $NETWORK_PID 2>/dev/null; then
+ echo "Local network failed to start" >&2
+ exit 1
+ fi
+ sleep 1
+ doneCommittable suggestion was skipped due to low confidence.
.github/workflows/test.yaml
Outdated
| name: Run Tests | ||
|
|
||
| on: | ||
| push: | ||
| workflow_dispatch: | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing workflow triggers for better control.
Add path filters and pull request trigger to optimize workflow execution:
on:
push:
+ paths:
+ - 'examples/**'
+ - '.github/workflows/test.yaml'
+ pull_request:
+ paths:
+ - 'examples/**'
+ - '.github/workflows/test.yaml'
workflow_dispatch:📝 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.
| name: Run Tests | |
| on: | |
| push: | |
| workflow_dispatch: | |
| name: Run Tests | |
| on: | |
| push: | |
| paths: | |
| - 'examples/**' | |
| - '.github/workflows/test.yaml' | |
| pull_request: | |
| paths: | |
| - 'examples/**' | |
| - '.github/workflows/test.yaml' | |
| workflow_dispatch: | |
There was a problem hiding this comment.
any reason to not include pull_request?
There was a problem hiding this comment.
@gartnera, including the pull_request trigger ensures that the workflow runs on pull request events as well. This helps catch issues during the code review process before merging changes. There is no reason not to include pull_request; in fact, it's advisable for comprehensive testing. Here's the updated workflow configuration:
name: Run Tests
on:
push:
paths:
- 'examples/**'
- '.github/workflows/test.yaml'
pull_request:
paths:
- 'examples/**'
- '.github/workflows/test.yaml'
workflow_dispatch:
.github/workflows/test.yaml
Outdated
| test: | ||
| needs: setup-matrix | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| include: ${{ fromJSON(needs.setup-matrix.outputs.matrix) }} | ||
| fail-fast: false | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Install Foundry | ||
| uses: foundry-rs/foundry-toolchain@v1 | ||
|
|
||
| - name: Run Test Script | ||
| run: | | ||
| cd "${{ matrix.example-dir }}" | ||
| yarn | ||
| chmod +x ./scripts/test.sh | ||
| ./scripts/test.sh localnet |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test job with production-grade features.
The test execution could benefit from several production-ready improvements:
test:
needs: setup-matrix
runs-on: ubuntu-latest
strategy:
matrix:
include: ${{ fromJSON(needs.setup-matrix.outputs.matrix) }}
fail-fast: false
+ max-parallel: 4
+ timeout-minutes: 30
steps:
- name: Checkout code
uses: actions/checkout@v3
+ - name: Setup Node.js
+ uses: actions/setup-node@v3
+ with:
+ node-version: '18'
+ cache: 'yarn'
+
- name: Install Foundry
uses: foundry-rs/foundry-toolchain@v1
+ with:
+ version: nightly
+ - name: Validate Test Script
+ run: |
+ if ! grep -q "#!/bin/bash" "${{ matrix.example-dir }}/scripts/test.sh"; then
+ echo "Error: test.sh must start with proper shebang"
+ exit 1
+ fi
+
- name: Run Test Script
run: |
cd "${{ matrix.example-dir }}"
yarn
chmod +x ./scripts/test.sh
- ./scripts/test.sh localnet
+ ./scripts/test.sh localnet || exit 1
+
+ - name: Upload Test Artifacts
+ if: always()
+ uses: actions/upload-artifact@v3
+ with:
+ name: test-results-${{ matrix.example-dir }}
+ path: |
+ ${{ matrix.example-dir }}/out/
+ ${{ matrix.example-dir }}/coverage/
+ retention-days: 7Key improvements:
- Added dependency caching for faster builds
- Added timeout limits to prevent hung jobs
- Added test artifact collection
- Added script validation
- Added proper error handling
- Limited parallel execution to prevent resource exhaustion
📝 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.
| test: | |
| needs: setup-matrix | |
| runs-on: ubuntu-latest | |
| strategy: | |
| matrix: | |
| include: ${{ fromJSON(needs.setup-matrix.outputs.matrix) }} | |
| fail-fast: false | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v3 | |
| - name: Install Foundry | |
| uses: foundry-rs/foundry-toolchain@v1 | |
| - name: Run Test Script | |
| run: | | |
| cd "${{ matrix.example-dir }}" | |
| yarn | |
| chmod +x ./scripts/test.sh | |
| ./scripts/test.sh localnet | |
| test: | |
| needs: setup-matrix | |
| runs-on: ubuntu-latest | |
| strategy: | |
| matrix: | |
| include: ${{ fromJSON(needs.setup-matrix.outputs.matrix) }} | |
| fail-fast: false | |
| max-parallel: 4 | |
| timeout-minutes: 30 | |
| steps: | |
| - name: Checkout code | |
| uses: actions/checkout@v3 | |
| - name: Setup Node.js | |
| uses: actions/setup-node@v3 | |
| with: | |
| node-version: '18' | |
| cache: 'yarn' | |
| - name: Install Foundry | |
| uses: foundry-rs/foundry-toolchain@v1 | |
| with: | |
| version: nightly | |
| - name: Validate Test Script | |
| run: | | |
| if ! grep -q "#!/bin/bash" "${{ matrix.example-dir }}/scripts/test.sh"; then | |
| echo "Error: test.sh must start with proper shebang" | |
| exit 1 | |
| fi | |
| - name: Run Test Script | |
| run: | | |
| cd "${{ matrix.example-dir }}" | |
| yarn | |
| chmod +x ./scripts/test.sh | |
| ./scripts/test.sh localnet || exit 1 | |
| - name: Upload Test Artifacts | |
| if: always() | |
| uses: actions/upload-artifact@v3 | |
| with: | |
| name: test-results-${{ matrix.example-dir }} | |
| path: | | |
| ${{ matrix.example-dir }}/out/ | |
| ${{ matrix.example-dir }}/coverage/ | |
| retention-days: 7 |
| NFT_ID=$(npx hardhat mint --network localhost --json --contract "$CONTRACT_ZETACHAIN" --token-uri https://example.com/nft/metadata/1 | jq -r '.tokenId') | ||
| echo -e "\nMinted NFT with ID: $NFT_ID on ZetaChain." |
There was a problem hiding this comment.
Add transaction verification after NFT operations.
The script should verify the success of NFT operations by checking the transaction receipts and final token ownership.
Consider implementing a verification function:
+function verify_nft_ownership() {
+ local token_id=$1
+ local expected_contract=$2
+ local max_attempts=10
+
+ echo "Verifying NFT ownership..."
+ for i in $(seq 1 $max_attempts); do
+ local owner
+ if ! owner=$(npx hardhat owner-of --network localhost --contract "$expected_contract" --token-id "$token_id" --json 2>/dev/null); then
+ echo "Attempt $i: Failed to verify ownership"
+ sleep 2
+ continue
+ fi
+
+ if [ "$owner" = "$SENDER" ]; then
+ echo "✅ NFT ownership verified"
+ return 0
+ fi
+ echo "Attempt $i: Waiting for ownership transfer..."
+ sleep 2
+ done
+
+ echo "❌ Failed to verify NFT ownership"
+ return 1
+}
+
NFT_ID=$(npx hardhat mint --network localhost --json --contract "$CONTRACT_ZETACHAIN" --token-uri https://example.com/nft/metadata/1 | jq -r '.tokenId')
echo -e "\nMinted NFT with ID: $NFT_ID on ZetaChain."
+verify_nft_ownership "$NFT_ID" "$CONTRACT_ZETACHAIN" || exit 1
echo -e "\nTransferring NFT: ZetaChain → Ethereum..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM"
+verify_nft_ownership "$NFT_ID" "$CONTRACT_ETHEREUM" || exit 1
echo -e "\nTransferring NFT: Ethereum → BNB..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_ETHEREUM" --to "$ZRC20_BNB" --gas-amount 0.1
+verify_nft_ownership "$NFT_ID" "$CONTRACT_BNB" || exit 1
echo -e "\nTransferring NFT: BNB → ZetaChain..."
npx hardhat transfer --network localhost --json --token-id "$NFT_ID" --from "$CONTRACT_BNB"
+verify_nft_ownership "$NFT_ID" "$CONTRACT_ZETACHAIN" || exit 1Also applies to: 57-58, 63-64, 69-70
| nft_balance | ||
| npx hardhat localnet-check | ||
|
|
||
| npx hardhat localnet-stop |
There was a problem hiding this comment.
Enhance cleanup process.
The current cleanup doesn't ensure proper termination of all processes or handle cleanup errors.
Consider implementing a more robust cleanup:
-npx hardhat localnet-stop
+function cleanup() {
+ echo "Cleaning up..."
+ if [ -n "$LOCALNET_PID" ]; then
+ if kill -0 "$LOCALNET_PID" 2>/dev/null; then
+ echo "Stopping localnet..."
+ if ! npx hardhat localnet-stop; then
+ echo "Failed to stop localnet gracefully, forcing stop..."
+ kill "$LOCALNET_PID"
+ fi
+ wait "$LOCALNET_PID" 2>/dev/null || true
+ fi
+ fi
+}
+
+trap cleanup EXIT
+cleanup📝 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.
| npx hardhat localnet-stop | |
| function cleanup() { | |
| echo "Cleaning up..." | |
| if [ -n "$LOCALNET_PID" ]; then | |
| if kill -0 "$LOCALNET_PID" 2>/dev/null; then | |
| echo "Stopping localnet..." | |
| if ! npx hardhat localnet-stop; then | |
| echo "Failed to stop localnet gracefully, forcing stop..." | |
| kill "$LOCALNET_PID" | |
| fi | |
| wait "$LOCALNET_PID" 2>/dev/null || true | |
| fi | |
| fi | |
| } | |
| trap cleanup EXIT | |
| cleanup |
| if [ "$1" = "localnet" ]; then | ||
| npx hardhat localnet --exit-on-error & sleep 10 | ||
| fi |
There was a problem hiding this comment.
Enhance localnet startup error handling.
The current implementation doesn't verify if the localnet started successfully. The arbitrary sleep might not be sufficient for the network to be ready.
Consider this more robust implementation:
-if [ "$1" = "localnet" ]; then
- npx hardhat localnet --exit-on-error & sleep 10
-fi
+if [ "$1" = "localnet" ]; then
+ echo "Starting localnet..."
+ npx hardhat localnet --exit-on-error &
+ LOCALNET_PID=$!
+
+ # Wait for localnet to be ready (max 30 seconds)
+ for i in {1..30}; do
+ if npx hardhat localnet-status &>/dev/null; then
+ echo "Localnet is ready"
+ break
+ fi
+ if ! kill -0 $LOCALNET_PID 2>/dev/null; then
+ echo "Localnet failed to start"
+ exit 1
+ fi
+ sleep 1
+ done
+
+ if ! npx hardhat localnet-status &>/dev/null; then
+ echo "Localnet failed to start within timeout"
+ kill $LOCALNET_PID
+ exit 1
+ fi
+fiCommittable suggestion was skipped due to low confidence.
.github/workflows/test.yaml
Outdated
| name: Run Tests | ||
|
|
||
| on: | ||
| push: | ||
| workflow_dispatch: | ||
|
|
There was a problem hiding this comment.
any reason to not include pull_request?
| test_dirs=$(find examples/*/scripts -type f -name 'test.sh' -exec dirname {} \; | xargs dirname) | ||
| matrix_json=$(echo "$test_dirs" | jq -R '{"example-dir": .}' | jq -s . | jq -c .) | ||
| echo "matrix=$matrix_json" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
might be cleaner to just write this in actions/github-script in javascript: https://github.com/actions/github-script
There was a problem hiding this comment.
The following actions use a deprecated Node.js version and will be forced to run on node20: actions/checkout@v3
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/test.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/test.yaml
18-18: shellcheck reported issue in this script: SC2038:warning:1:13: Use -print0/-0 or -exec + to allow for non-alphanumeric filenames
(shellcheck)
18-18: shellcheck reported issue in this script: SC2086:info:3:31: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/test.yaml (2)
3-5: Workflow triggers need enhancement.
The current trigger configuration could be optimized as suggested in the previous review.
23-43: Add production-grade features to test job.
The test job would benefit from several production-ready improvements as suggested in the previous review.
| - name: Set up Test Matrix | ||
| id: set-matrix | ||
| run: | | ||
| test_dirs=$(find examples/*/scripts -type f -name 'test.sh' -exec dirname {} \; | xargs dirname) | ||
| matrix_json=$(echo "$test_dirs" | jq -R '{"example-dir": .}' | jq -s . | jq -c .) | ||
| echo "matrix=$matrix_json" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance matrix setup using github-script.
Replace the shell script with a more robust JavaScript implementation using the github-script action.
- - name: Set up Test Matrix
- id: set-matrix
- run: |
- test_dirs=$(find examples/*/scripts -type f -name 'test.sh' -exec dirname {} \; | xargs dirname)
- matrix_json=$(echo "$test_dirs" | jq -R '{"example-dir": .}' | jq -s . | jq -c .)
- echo "matrix=$matrix_json" >> $GITHUB_OUTPUT
+ - name: Set up Test Matrix
+ id: set-matrix
+ uses: actions/github-script@v7
+ with:
+ script: |
+ const { execSync } = require('child_process');
+ try {
+ const testDirs = execSync('find examples/*/scripts -type f -name "test.sh"')
+ .toString()
+ .trim()
+ .split('\n')
+ .map(path => path.replace(/\/scripts\/test\.sh$/, ''))
+ .filter(Boolean);
+
+ if (!testDirs.length) {
+ throw new Error('No test directories found');
+ }
+
+ const matrix = {
+ 'example-dir': testDirs
+ };
+
+ core.setOutput('matrix', JSON.stringify(matrix));
+ } catch (error) {
+ core.setFailed(`Matrix setup failed: ${error.message}`);
+ }📝 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.
| - name: Set up Test Matrix | |
| id: set-matrix | |
| run: | | |
| test_dirs=$(find examples/*/scripts -type f -name 'test.sh' -exec dirname {} \; | xargs dirname) | |
| matrix_json=$(echo "$test_dirs" | jq -R '{"example-dir": .}' | jq -s . | jq -c .) | |
| echo "matrix=$matrix_json" >> $GITHUB_OUTPUT | |
| - name: Set up Test Matrix | |
| id: set-matrix | |
| uses: actions/github-script@v7 | |
| with: | |
| script: | | |
| const { execSync } = require('child_process'); | |
| try { | |
| const testDirs = execSync('find examples/*/scripts -type f -name "test.sh"') | |
| .toString() | |
| .trim() | |
| .split('\n') | |
| .map(path => path.replace(/\/scripts\/test\.sh$/, '')) | |
| .filter(Boolean); | |
| if (!testDirs.length) { | |
| throw new Error('No test directories found'); | |
| } | |
| const matrix = { | |
| 'example-dir': testDirs | |
| }; | |
| core.setOutput('matrix', JSON.stringify(matrix)); | |
| } catch (error) { | |
| core.setFailed(`Matrix setup failed: ${error.message}`); | |
| } |
🧰 Tools
🪛 actionlint
18-18: shellcheck reported issue in this script: SC2038:warning:1:13: Use -print0/-0 or -exec + to allow for non-alphanumeric filenames
(shellcheck)
18-18: shellcheck reported issue in this script: SC2086:info:3:31: Double quote to prevent globbing and word splitting
(shellcheck)
At the moment gateway is only enabled on Base Sepolia, Polygon Amoy and ZetaChain testnet.
Gateway on ZetaChain:
0x6c533f7fe93fae114d0954697069df33c9b74fd7Gateway on EVM chains:
0x0c487a766110c85d301d96e33579c5b317fa4995Hello Example
Call from Base to ZetaChain
https://sepolia.basescan.org/tx/0x133cdf3a06195de0a6bb89dd4761ca98d1301534b3c4987f0ff93c95c3fff78c
✅ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x133cdf3a06195de0a6bb89dd4761ca98d1301534b3c4987f0ff93c95c3fff78c
Call from ZetaChain to Base
Had to specify gas price — need to update the defaults to match testnet:
https://zetachain-testnet.blockscout.com/tx/0x19d476fa2c3d29ba41467ae7f2742541fd11e0b67d6548fe7655a3d40274323e
✅ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x19d476fa2c3d29ba41467ae7f2742541fd11e0b67d6548fe7655a3d40274323e
Withdraw And Call from ZetaChain to Base
https://zetachain-testnet.blockscout.com/tx/0x67099389ab6cb44ee03602d164320c615720b57762c5ddab042d65bdbe30c7a2
✅ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0x67099389ab6cb44ee03602d164320c615720b57762c5ddab042d65bdbe30c7a2
Transfers
Deposit from Base to ZetaChain
Not related to examples.
https://sepolia.basescan.org/tx/0xbb864575f2c4340e868946649c7a25f954b1d3804aed4f5b11bef09cb3d75ca4
✅ https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0xbb864575f2c4340e868946649c7a25f954b1d3804aed4f5b11bef09cb3d75ca4
Swap
Deposit and Call from Base to ZetaChain to Amoy
https://sepolia.basescan.org/tx/0xc81cc6d1383fb55bb99d0c672af1ab1db2dce003f3dc00a32e316e469733530c
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/0xc81cc6d1383fb55bb99d0c672af1ab1db2dce003f3dc00a32e316e469733530c
Added NFT example from #206
Depends on #207
Summary by CodeRabbit
Release Notes
New Features
.eslintignorefile to streamline linting processes.foundry.tomlconfiguration file for Foundry framework settings.v4.0.0-rc*.Bug Fixes
Documentation
Chores