fix: nft testnet script #221
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to deployment scripts for smart contracts in the NFT example directory. Key changes include an adjustment to the deployment gas limit for 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
examples/nft/scripts/testnet.sh (2)
8-8: Consider parameterizing contract addresses and optimizing gas limitsThe deployment command contains hardcoded addresses and a high gas limit. Consider:
- Moving gateway and uniswap router addresses to environment variables or a config file
- Validating if 10M gas limit is necessary, as it could lead to higher deployment costs
-UNIVERSAL=$(npx hardhat deploy --name Universal --network zeta_testnet --deploy-gas-limit 10000000 --gateway 0x6c533f7fe93fae114d0954697069df33c9b74fd7 --uniswap-router 0x2ca7d64A7EFE2D62A725E2B35Cf7230D6677FfEe --gas-limit 500000 --json | jq -r '.contractAddress') +# Load configuration +source .env.testnet + +UNIVERSAL=$(npx hardhat deploy \ + --name Universal \ + --network zeta_testnet \ + --deploy-gas-limit "${DEPLOY_GAS_LIMIT:-5000000}" \ + --gateway "${ZETA_GATEWAY_ADDRESS}" \ + --uniswap-router "${ZETA_UNISWAP_ROUTER}" \ + --gas-limit 500000 \ + --json | jq -r '.contractAddress')
Line range hint
1-21: Consider restructuring for improved maintainability and reliabilityThis deployment script handles critical contract deployments and linking across multiple networks. Consider the following architectural improvements:
- Add a comprehensive header comment explaining the deployment flow
- Split the script into functions for each deployment phase
- Create a separate configuration file for network-specific addresses
- Add logging for better deployment tracking
- Implement rollback mechanisms for failed deployments
Would you like me to provide an example of this improved structure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
examples/nft/scripts/testnet.sh(1 hunks)examples/nft/tasks/deploy.ts(2 hunks)
🔇 Additional comments (3)
examples/nft/scripts/testnet.sh (1)
9-10: Verify gateway addresses for different networks
The script uses the same gateway address (0x0c487a766110c85d301d96e33579c5b317fa4995) for both Base Sepolia and BSC testnet networks. This seems unusual and should be verified.
examples/nft/tasks/deploy.ts (2)
25-25: LGTM: Explicit gas limit in deployment options
The addition of an explicit gas limit in the deployment options enhances control over the deployment process.
Line range hint 1-72: Verify gas limit values across environments
The deployment gas limits should be verified across different networks to ensure they are sufficient for contract deployment.
✅ Verification successful
Let me analyze the network configurations and gas limits to provide a comprehensive verification of the gas limit values across environments.
Let me analyze the configuration files and network settings to verify the gas limit values across different environments.
Gas limits are appropriately configured across environments
The verification reveals that gas limits are properly handled across different environments:
- Default deployment gas limit is set to 1,000,000 units (line 53 in deploy.ts)
- Network-specific gas limits are managed through:
- Testnet deployments use explicit gas limits (e.g., 10,000,000 for zeta_testnet, 500,000 for other networks)
- Local development uses configurable gas limits through CLI parameters
- Production networks inherit configurations from @zetachain/networks package
The gas limit values are appropriately configured for different deployment scenarios and can be overridden when needed through command-line arguments, providing the necessary flexibility while maintaining safe defaults.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for gas limit configurations across deployment scripts
# Search for gas limit configurations in deployment scripts
echo "Searching for gas limit configurations in deployment scripts..."
rg -n "gasLimit|gas.*limit" --type ts
# Search for network-specific configurations
echo "Searching for network-specific configurations..."
fd -e json -e js -e ts --exec grep -l "network.*config" {}
Length of output: 4177
Script:
#!/bin/bash
# Let's check for network-specific configurations and hardhat config files
echo "Searching for hardhat config files..."
fd "hardhat.config" --type ts --type js
echo "Checking content of hardhat config files..."
for config in $(fd "hardhat.config" --type ts --type js); do
echo "=== Content of $config ==="
cat "$config"
done
echo "Searching for network-specific gas configurations..."
rg -n "networks.*gas" --type ts --type js
Length of output: 761
Script:
#!/bin/bash
# Let's try a different approach to find configuration files
echo "Searching for hardhat config files..."
find . -name "hardhat.config.*"
echo "Checking content of found config files..."
for config in $(find . -name "hardhat.config.*"); do
echo "=== Content of $config ==="
cat "$config"
done
# Let's also check for any network-specific settings in the examples
echo "Checking for network configurations in examples..."
rg -n "network" examples/ --type-not md
Length of output: 24519
examples/nft/scripts/testnet.sh
Outdated
| npx hardhat connected-set-universal --network base_sepolia --contract "$CONNECTED_BASE" --universal "$UNIVERSAL" --json | ||
|
|
||
| npx hardhat connected-set-counterparty --network bsc_testnet --contract "$CONNECTED_BNB" --counterparty "$UNIVERSAL" --json | ||
| npx hardhat connected-set-universal --network bsc_testnet --contract "$CONNECTED_BNB" --universal "$UNIVERSAL" --json | ||
|
|
||
| npx hardhat universal-set-counterparty --network zeta_testnet --contract "$UNIVERSAL" --counterparty "$CONNECTED_BASE" --zrc20 "$ZRC20_BASE" --json | ||
| npx hardhat universal-set-connected --network zeta_testnet --contract "$UNIVERSAL" --connected "$CONNECTED_BASE" --zrc20 "$ZRC20_BASE" --json | ||
|
|
||
| npx hardhat universal-set-counterparty --network zeta_testnet --contract "$UNIVERSAL" --counterparty "$CONNECTED_BNB" --zrc20 "$ZRC20_BNB" --json No newline at end of file | ||
| npx hardhat universal-set-connected --network zeta_testnet --contract "$UNIVERSAL" --connected "$CONNECTED_BNB" --zrc20 "$ZRC20_BNB" --json |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add deployment validation and error handling
The contract linking commands lack validation and error handling. Consider adding checks between steps to ensure successful deployment and linking.
+# Function to verify contract deployment
+verify_deployment() {
+ local contract_address=$1
+ local network=$2
+
+ if [[ ! $contract_address =~ ^0x[a-fA-F0-9]{40}$ ]]; then
+ echo "Error: Invalid contract address for $network"
+ exit 1
+ fi
+}
+
+# Verify deployments
+verify_deployment "$CONNECTED_BASE" "base_sepolia"
+verify_deployment "$CONNECTED_BNB" "bsc_testnet"
+
npx hardhat connected-set-universal --network base_sepolia --contract "$CONNECTED_BASE" --universal "$UNIVERSAL" --json
+
+# Verify successful linking
+if [ $? -ne 0 ]; then
+ echo "Error: Failed to link Base Sepolia contract"
+ exit 1
+fiAlso consider moving ZRC20 addresses to a configuration file for better maintainability.
Committable suggestion skipped: line range outside the PR's diff.
| .addOptionalParam( | ||
| "deployGasLimit", | ||
| "Gas limit for deploy transaction", | ||
| "5000000" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Convert deployGasLimit default value to number type
The default value for deployGasLimit is defined as a string "5000000", but should be a number for consistency with other gas-related parameters and to prevent potential type coercion issues.
Apply this change:
.addOptionalParam(
"deployGasLimit",
"Gas limit for deploy transaction",
- "5000000"
+ 5000000,
+ types.int
)📝 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.
| .addOptionalParam( | |
| "deployGasLimit", | |
| "Gas limit for deploy transaction", | |
| "5000000" | |
| ) | |
| .addOptionalParam( | |
| "deployGasLimit", | |
| "Gas limit for deploy transaction", | |
| 5000000, | |
| types.int | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
examples/nft/scripts/localnet.sh (1)
33-34: Consider using a variable for gas limit values.The deployment gas limit is repeated across different commands. Consider defining it as a variable at the start of the script for better maintainability.
#!/bin/bash set -e set -x +DEPLOY_GAS_LIMIT=10000000 if [ "$1" = "start" ]; then npx hardhat localnet --exit-on-error & sleep 10; fiThen use it in the deployment commands:
-CONTRACT_ETHEREUM=$(npx hardhat deploy --name Connected --json --network localhost --deploy-gas-limit 10000000 --gateway "$GATEWAY_ETHEREUM" | jq -r '.contractAddress') +CONTRACT_ETHEREUM=$(npx hardhat deploy --name Connected --json --network localhost --deploy-gas-limit $DEPLOY_GAS_LIMIT --gateway "$GATEWAY_ETHEREUM" | jq -r '.contractAddress')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
examples/nft/scripts/localnet.sh(1 hunks)
🔇 Additional comments (2)
examples/nft/scripts/localnet.sh (2)
36-37: Clarify the purpose of dual gas limit parameters.
The BNB contract deployment uses both --deploy-gas-limit 10000000 and --gas-limit 1000000. This dual configuration might cause confusion:
- What's the difference between these parameters?
- Why is this needed only for BNB chain deployment?
Consider adding comments to explain the purpose of each gas limit parameter.
Let's check if this pattern is documented or used elsewhere:
30-31: Verify the deployment gas limit value for ZetaChain contract.
The deployment gas limit of 10,000,000 seems high. While this might work, it's important to ensure this value is optimized for your contract size and complexity.
Let's analyze the contract size and complexity:
✅ Verification successful
The previous results show the contract structure, but we need to analyze the specific NFT contract being deployed and its deployment configuration to make a proper assessment of the gas limit. This will help us understand if 10M gas limit is justified.
The gas limit of 10,000,000 for NFT contract deployment is justified
The analysis reveals that the NFT contract is complex and requires a high gas limit due to:
- Multiple inheritance from OpenZeppelin contracts (ERC721, ERC721Enumerable, ERC721URIStorage, ERC721Burnable)
- Complex cross-chain functionality with gateway interactions
- Storage initialization for mappings and state variables
- Constructor parameters initialization and validation
The default deployment gas limit in the Hardhat task is 5,000,000, and given the contract's complexity, the increase to 10,000,000 is a reasonable safety margin to ensure successful deployment on ZetaChain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check contract size and complexity
# Look for the contract file and analyze its size
fd -e sol | while read -r file; do
echo "Analyzing $file..."
# Count lines of code (excluding comments and empty lines)
cat "$file" | grep -v '^[[:space:]]*$' | grep -v '^[[:space:]]*//' | wc -l
# Look for potential complex operations that might require high gas
rg -n "for|while|mapping|struct" "$file"
done
Length of output: 4098
Script:
#!/bin/bash
# Let's specifically analyze the NFT contracts that are being deployed
echo "Analyzing NFT contract content..."
cat examples/nft/contracts/Universal.sol
echo -e "\nChecking for complex operations in NFT contracts..."
rg -n "onZetaMessage|onZetaRevert|transfer|mint" examples/nft/contracts/Universal.sol
echo -e "\nChecking hardhat deploy task to understand gas usage..."
fd -e ts -e js | grep -i "deploy" | xargs cat
Length of output: 15494
Summary by CodeRabbit
New Features
Bug Fixes
Documentation