ERC-20 token transfer example and improvements to NFT #209
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several significant changes to the Changes
Possibly related PRs
Suggested reviewers
Warning Tool Failures:Tool Failure Count: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: 31
🧹 Outside diff range and nitpick comments (15)
examples/token/hardhat.config.ts (1)
14-19: Consider adding compiler optimization settings.The configuration would benefit from additional compiler settings to optimize gas usage and enable source maps for debugging.
Consider enhancing the configuration with these settings:
const config: HardhatUserConfig = { networks: { ...getHardhatConfigNetworks(), }, - solidity: "0.8.24", + solidity: { + version: "0.8.24", + settings: { + optimizer: { + enabled: true, + runs: 200, + }, + outputSelection: { + "*": { + "*": ["storageLayout"], + }, + }, + }, + }, };examples/token/.eslintrc.js (3)
7-17: Consider enhancing the base configuration for better TypeScript support.The current configuration could be improved by:
- Extending recommended TypeScript configurations
- Reconsidering the browser environment restriction
extends: [ - "plugin:prettier/recommended" + "plugin:prettier/recommended", + "plugin:@typescript-eslint/recommended", + "plugin:@typescript-eslint/recommended-requiring-type-checking" ], env: { - browser: false, + browser: true, es2021: true,
18-24: Enhance code quality with additional ESLint plugins.Consider adding security and best practice plugins to strengthen the codebase:
plugins: [ "@typescript-eslint", + "security", + "sonarjs", + "promise", "prettier", "simple-import-sort", "sort-keys-fix", "typescript-sort-keys", ],
34-46: Enhance module resolution with path aliases.Consider adding path alias support to improve import readability and maintainability:
"import/resolver": { typescript: { project: path.join(__dirname, "tsconfig.json"), + alwaysTryTypes: true }, + alias: { + map: [ + ["@contracts", "./contracts"], + ["@test", "./test"] + ], + extensions: [".ts", ".js", ".json"] + } },Make sure to add corresponding path aliases in your
tsconfig.json.examples/token/tasks/deploy.ts (2)
1-3: Consider adding strict typing for task arguments.Replace the
anytype with a proper interface to improve type safety and code maintainability.+interface DeployTaskArgs { + name: string; + gateway: string; + json: boolean; +} -const main = async (args: any, hre: HardhatRuntimeEnvironment) => { +const main = async (args: DeployTaskArgs, hre: HardhatRuntimeEnvironment) => {Also applies to: 4-4
34-41: Improve configuration management and parameter validation.The hardcoded gateway address and lack of contract name validation could cause issues in different environments.
Consider these improvements:
- Move the gateway address to a configuration file or environment variable.
- Add validation for the contract name parameter.
- Add network-specific gateway addresses.
+import { config } from "../config"; + +const SUPPORTED_CONTRACTS = ["Universal", "Connected"]; + task("deploy", "Deploy the NFT contract", main) .addFlag("json", "Output the result in JSON format") - .addOptionalParam("name", "The contract name to deploy", "Universal") + .addOptionalParam( + "name", + `The contract name to deploy (${SUPPORTED_CONTRACTS.join(", ")})`, + "Universal", + types.string + ) .addOptionalParam( "gateway", "Gateway address (default: ZetaChain Gateway)", - "0x5FC8d32690cc91D4c39d9d3abcBD16989F875707" + config.networks[hre.network.name]?.gatewayAddress );examples/token/tasks/universalSetCounterparty.ts (2)
1-3: Enhance type safety with proper parameter typing.The imports look good, but we should improve type safety for better maintainability and developer experience.
Add a properly typed interface for the task arguments:
interface UniversalSetCounterpartyArgs { contract: string; zrc20: string; counterparty: string; name: string; json: boolean; }
35-40: Add parameter validation in task definition.While the task parameters are well-defined, we should add validation at the task definition level.
Consider adding these validations:
task("universal-set-counterparty", "Sets the connected contract address", main) .addParam("contract", "The address of the deployed contract") .addParam("zrc20", "The ZRC20 address to link to the connected contract") .addParam("counterparty", "The address of the connected contract to set") .addOptionalParam("name", "The contract name to interact with", "Universal") - .addFlag("json", "Output the result in JSON format"); + .addFlag("json", "Output the result in JSON format") + .setAction(async (args, hre) => { + // Validate contract name + if (args.name && typeof args.name !== "string") { + throw new Error("Contract name must be a string"); + } + return main(args, hre); + });examples/token/tasks/mint.ts (1)
25-42: Improve output handling and null safetyThe output handling could be more robust and consistent.
Consider these improvements:
+ const output = { + contractAddress: args.contract, + mintTransactionHash: tx.hash, + recipient, + tokenId: tokenId?.toString() ?? 'unknown' + }; if (args.json) { - console.log( - JSON.stringify({ - contractAddress: args.contract, - mintTransactionHash: tx.hash, - recipient: recipient, - tokenURI: args.tokenUri, - tokenId: tokenId?.toString(), - }) - ); + console.log(JSON.stringify(output, null, 2)); } else { - console.log(`🚀 Successfully minted NFT. -📜 Contract address: ${args.contract} -👤 Recipient: ${recipient} -🆔 Token ID: ${tokenId?.toString()} -🔗 Transaction hash: ${tx.hash}`); + const { contractAddress, mintTransactionHash, recipient, tokenId } = output; + console.log( + [ + '🚀 Successfully minted NFT.', + `📜 Contract address: ${contractAddress}`, + `👤 Recipient: ${recipient}`, + `🆔 Token ID: ${tokenId}`, + `🔗 Transaction hash: ${mintTransactionHash}` + ].join('\n') + ); }examples/token/tasks/transfer.ts (1)
40-48: Enhance transfer execution reliability.The current implementation lacks address validation and transfer confirmation through events.
Consider these improvements:
-const receiver = args.receiver || signer.address; +const receiver = args.receiver ? ethers.utils.getAddress(args.receiver) : signer.address; +if (receiver === ethers.constants.AddressZero) { + throw new Error("Invalid receiver address"); +} -tx = await (contract as any).transferCrossChain( +const tx = await (contract as any).transferCrossChain( receiver, args.to, args.amount, { ...txOptions, value: gasAmount } ); -await tx.wait(); +const receipt = await tx.wait(); +const transferEvent = receipt.events?.find(e => e.event === 'CrossChainTransfer'); +if (!transferEvent) { + throw new Error('Transfer event not emitted - transaction may have failed silently'); +}examples/token/contracts/Connected.sol (3)
33-58: Validatemsg.valuebased ondestinationintransferCrossChainIn the
transferCrossChainfunction, whendestinationis the zero address, you callgateway.callwithout utilizingmsg.value. Conversely, whendestinationis non-zero, you callgateway.depositAndCall{value: msg.value}. To prevent unintended Ether transfers or failures due to insufficient funds, consider adding validation formsg.valuebased on thedestinationparameter.Apply this diff to add validation:
) external payable { + if (destination == address(0)) { + require(msg.value == 0, "No ETH should be sent when destination is zero address"); + } else { + require(msg.value > 0, "ETH required for depositAndCall"); + } _burn(msg.sender, amount); bytes memory encodedData = abi.encode(receiver, amount, destination);
72-73: Implement handling or logging in theonRevertfunctionThe
onRevertfunction is currently empty. For better observability and debugging, consider handling the revert scenario by adding logic or emitting an event that logs the revert context.Apply this diff to enhance the function:
function onRevert(RevertContext calldata context) external onlyGateway { + // Emit an event with revert details + emit CrossChainRevert(context.zetaTxSenderAddress, context.message); } +event CrossChainRevert(address sender, bytes message);This addition will help in tracking cross-chain call failures and diagnosing issues effectively.
29-31: Optimize the visibility of themintfunctionThe
mintfunction is marked aspublic, but if it's only intended to be called externally and not used internally or by derived contracts, changing the visibility toexternalcan save gas and optimize performance.Apply this diff to update the function visibility:
-function mint(address to, uint256 amount) public onlyOwner { +function mint(address to, uint256 amount) external onlyOwner { _mint(to, amount); }examples/token/contracts/Universal.sol (2)
118-118: Implement logic inonRevertfunctionThe
onRevertfunction is currently empty. To handle failed cross-chain transactions effectively, consider implementing logic to manage reversions, such as refunding tokens or notifying users of the failure.
19-19: Restrict exposure ofgasLimitvariableDeclaring
gasLimitas a public variable may expose internal configuration details. Consider making it private or internal, and provide controlled access through getter and setter functions with proper access modifiers.
📜 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/swap/yarn.lockis excluded by!**/yarn.lock,!**/*.lockexamples/token/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
examples/hello/package.json(1 hunks)examples/token/.eslintignore(1 hunks)examples/token/.eslintrc.js(1 hunks)examples/token/.gitignore(1 hunks)examples/token/LICENSE(1 hunks)examples/token/README.md(1 hunks)examples/token/contracts/Connected.sol(1 hunks)examples/token/contracts/Universal.sol(1 hunks)examples/token/foundry.toml(1 hunks)examples/token/hardhat.config.ts(1 hunks)examples/token/package.json(1 hunks)examples/token/scripts/test.sh(1 hunks)examples/token/tasks/connectedSetCounterparty.ts(1 hunks)examples/token/tasks/deploy.ts(1 hunks)examples/token/tasks/mint.ts(1 hunks)examples/token/tasks/transfer.ts(1 hunks)examples/token/tasks/universalSetCounterparty.ts(1 hunks)examples/token/tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- examples/hello/package.json
- examples/token/.eslintignore
- examples/token/.gitignore
- examples/token/LICENSE
- examples/token/README.md
- examples/token/foundry.toml
- examples/token/package.json
- examples/token/tsconfig.json
🧰 Additional context used
🪛 Shellcheck
examples/token/scripts/test.sh
[warning] 12-12: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 13-13: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (5)
examples/token/tasks/deploy.ts (1)
5-6: Verify cross-chain deployment compatibility.
Since this PR aims to demonstrate cross-chain token transfers, ensure the deployment task works correctly across all target networks (ZetaChain, Ethereum, and BNB Chain).
✅ Verification successful
Cross-chain deployment configuration is properly implemented
The deployment task is correctly configured for cross-chain compatibility:
- The network configuration is properly sourced from
@zetachain/networksusinggetHardhatConfigNetworks()in hardhat.config.ts - The deployment task correctly retrieves the network context using
hre.network.name - The codebase includes necessary ZetaChain protocol contracts and cross-chain gateway integrations
- The deployment script handles gateway addresses appropriately through the optional
gatewayparameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify network configurations and deployment parameters for all target chains
# Test: Check if network configurations exist for all target chains
rg -A 5 "networks.*=.*{" .
# Test: Check for chain-specific deployment parameters or overrides
rg -A 10 "deploy.*task" .
Length of output: 148
Script:
#!/bin/bash
# Let's fix the regex and try again with proper escaping
# Check network configurations
rg -A 5 "networks\s*=\s*\{" .
# Check hardhat config for network definitions
rg -A 10 "module\.exports\s*=\s*\{" hardhat.config.*
# Check deployment tasks and their network handling
rg -A 5 "task\(.*deploy" .
# Look for any network-specific logic in deployment files
fd "deploy" --type f --exec rg -l "network|chain"
Length of output: 2473
Script:
#!/bin/bash
# Let's examine the network handling in the token deployment task and related files
# Check the specific implementation of the token deployment
cat ./examples/token/tasks/deploy.ts
# Look for network configurations in the example directory
fd "hardhat.config" --type f --exec cat {} \; ./examples/token/
# Check for any chain-specific constants or configurations
rg -A 3 "chain|network" ./examples/token/
Length of output: 24751
examples/token/tasks/mint.ts (2)
1-4: LGTM: Clean and necessary imports
The imports are well-organized and include all required dependencies for the task implementation.
44-52: LGTM: Well-structured task definition
The task parameters are well-defined with clear descriptions and appropriate optionality.
examples/token/tasks/transfer.ts (1)
1-4: LGTM! Clean and focused imports.
The imports are well-organized and specific to the requirements.
examples/token/contracts/Connected.sol (1)
22-27:
Correct the usage of the Ownable constructor
The Ownable contract from OpenZeppelin does not accept an initialOwner parameter in its constructor. Instead, it sets the owner to msg.sender by default. To assign a different initial owner, you should call transferOwnership(initialOwner) within the constructor after initializing the parent contracts.
Apply this diff to fix the constructor:
) ERC20("MyToken", "MTK") {
gateway = GatewayEVM(gatewayAddress);
+ transferOwnership(initialOwner);
}Likely invalid or redundant comment.
| function onCall( | ||
| MessageContext calldata messageContext, | ||
| bytes calldata message | ||
| ) external payable onlyGateway returns (bytes4) { | ||
| if (messageContext.sender != counterparty) revert("Unauthorized"); | ||
| (address receiver, uint256 amount) = abi.decode( | ||
| message, | ||
| (address, uint256) | ||
| ); | ||
| _mint(receiver, amount); | ||
| } |
There was a problem hiding this comment.
Add the missing return statement in the onCall function
The onCall function declares a bytes4 return type but does not currently return any value. This will result in a compilation error and may cause unexpected behavior. Ensure that you return the appropriate selector as per the interface requirements of the GatewayEVM.
Apply this diff to add the missing return statement:
_mint(receiver, amount);
+ return _zetaclient.onCallSuccess(
+ messageContext.zetaTxSenderAddress,
+ messageContext.zetaTxOrigin,
+ messageContext.zetaValueAndGas
+ );
}Ensure that you replace the return statement with the correct value expected by the GatewayEVM interface.
📝 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 onCall( | |
| MessageContext calldata messageContext, | |
| bytes calldata message | |
| ) external payable onlyGateway returns (bytes4) { | |
| if (messageContext.sender != counterparty) revert("Unauthorized"); | |
| (address receiver, uint256 amount) = abi.decode( | |
| message, | |
| (address, uint256) | |
| ); | |
| _mint(receiver, amount); | |
| } | |
| function onCall( | |
| MessageContext calldata messageContext, | |
| bytes calldata message | |
| ) external payable onlyGateway returns (bytes4) { | |
| if (messageContext.sender != counterparty) revert("Unauthorized"); | |
| (address receiver, uint256 amount) = abi.decode( | |
| message, | |
| (address, uint256) | |
| ); | |
| _mint(receiver, amount); | |
| return _zetaclient.onCallSuccess( | |
| messageContext.zetaTxSenderAddress, | |
| messageContext.zetaTxOrigin, | |
| messageContext.zetaValueAndGas | |
| ); | |
| } |
| contract Universal is ERC20, Ownable, UniversalContract { | ||
| GatewayZEVM public immutable gateway; | ||
| SystemContract public immutable systemContract = | ||
| SystemContract(0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9); |
There was a problem hiding this comment.
Avoid hardcoding the system contract address
Hardcoding the systemContract address reduces flexibility and can lead to issues if the address changes across different networks or environments. Consider passing the address as a constructor parameter or providing a setter function with appropriate access control to allow for configuration.
| if (keccak256(messageContext.origin) != keccak256(counterparty[zrc20])) | ||
| revert("Unauthorized"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify origin verification in onCall function
Comparing keccak256 hashes of byte arrays is unnecessary and gas-inefficient. Since both messageContext.origin and counterparty[zrc20] are of type bytes, consider using a utility function to compare the byte arrays directly or, if possible, standardize on a fixed-size type like bytes32 for direct comparison. This will improve efficiency and readability.
| revert TransferFailed(); | ||
| IZRC20(zrc20).approve(address(gateway), gasFee); | ||
|
|
||
| bytes memory encodedData = abi.encode(receiver, amount); |
There was a problem hiding this comment.
Ensure consistent message encoding and decoding
In transferCrossChain, the message is encoded with two parameters:
bytes memory encodedData = abi.encode(receiver, amount);However, in onCall, the message is decoded expecting three parameters:
(address receiver, uint256 tokenAmount, address destination) = abi.decode(message, (address, uint256, address));This inconsistency will cause decoding errors. Ensure that the encoding and decoding formats match between these functions.
Apply this diff to align the encoding and decoding:
- If
destinationis intended to be part of the message:
function transferCrossChain(
address receiver,
address zrc20,
uint256 amount,
+ address destination
) public {
_burn(msg.sender, amount);
// ... [omitted code] ...
- bytes memory encodedData = abi.encode(receiver, amount);
+ bytes memory encodedData = abi.encode(receiver, amount, destination);
// ... [omitted code] ...
}- If
destinationis not required, adjust the decoding inonCall:
function onCall(
MessageContext calldata messageContext,
address zrc20,
uint256 amount,
bytes calldata message
) external override onlyGateway {
// ... [omitted code] ...
- (address receiver, uint256 tokenAmount, address destination) = abi.decode(message, (address, uint256, address));
+ (address receiver, uint256 tokenAmount) = abi.decode(message, (address, uint256));
// ... [omitted code] ...
}Also applies to: 92-93
| function transferCrossChain( | ||
| address receiver, | ||
| address zrc20, | ||
| uint256 amount | ||
| ) public { | ||
| _burn(msg.sender, amount); | ||
|
|
||
| (, uint256 gasFee) = IZRC20(zrc20).withdrawGasFeeWithGasLimit(gasLimit); | ||
| if (!IZRC20(zrc20).transferFrom(msg.sender, address(this), gasFee)) | ||
| revert TransferFailed(); | ||
| IZRC20(zrc20).approve(address(gateway), gasFee); | ||
|
|
||
| bytes memory encodedData = abi.encode(receiver, amount); | ||
|
|
||
| CallOptions memory callOptions = CallOptions(gasLimit, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| encodedData, | ||
| gasLimit | ||
| ); | ||
|
|
||
| gateway.call( | ||
| counterparty[zrc20], | ||
| zrc20, | ||
| encodedData, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
| } |
There was a problem hiding this comment.
Enhance security in transferCrossChain function
The transferCrossChain function performs multiple external calls and token transfers, which could introduce reentrancy risks. To strengthen security:
- Utilize the checks-effects-interactions pattern to order operations safely.
- Consider incorporating OpenZeppelin's
ReentrancyGuardto prevent reentrant calls. - Ensure that all external calls are properly validated and that state changes occur before these calls when appropriate.
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (9)
examples/token/contracts/shared/Events.sol (1)
4-24: Consider enhancing event parameters for better traceability.The events are well-structured with appropriate indexing on address parameters. However, consider adding these parameters to enhance traceability:
timestampto TokenTransfer events for temporal trackingchainIdto cross-chain transfer events for explicit chain identificationmetadataorreasonstring to TokenTransferReverted for debuggingHere's how you could enhance the events:
event TokenTransfer( address indexed destination, address indexed receiver, uint256 amount + uint256 timestamp, + uint256 chainId ); event TokenTransferReverted( address indexed sender, - uint256 amount + uint256 amount, + string reason );examples/token/contracts/Connected.sol (1)
11-16: Enhance error messages with more context.While the error definitions are good, they could be more descriptive to aid in debugging.
Consider this improvement:
- error InvalidAddress(); - error Unauthorized(); + error InvalidAddress(string reason); + error Unauthorized(address sender, address expected);Then update the usage:
- if (gatewayAddress == address(0) || owner == address(0)) - revert InvalidAddress(); + if (gatewayAddress == address(0)) + revert InvalidAddress("gateway address is zero"); + if (owner == address(0)) + revert InvalidAddress("owner address is zero"); - if (context.sender != counterparty) revert Unauthorized(); + if (context.sender != counterparty) + revert Unauthorized(context.sender, counterparty);examples/token/scripts/test.sh (1)
76-76: Improve cleanup handling.Add error handling for the network stop operation to ensure proper cleanup.
-npx hardhat localnet-stop +echo -e "\n🛑 Stopping local network..." +if ! npx hardhat localnet-stop; then + echo "Warning: Failed to stop local network gracefully" + # Attempt force cleanup + pkill -f "hardhat localnet" || true +fiexamples/token/contracts/Universal.sol (3)
1-2: Consider using a more flexible pragma versionThe current pragma
^0.8.26is very specific. While this ensures consistency, it might cause compatibility issues with other contracts or development environments. Consider using a more flexible range like^0.8.0unless there are specific features from 0.8.26 that are required.
14-28: Add NatSpec documentation for contract, variables, and errorsConsider adding comprehensive NatSpec documentation to improve code readability and maintainability. This should include:
- Contract description with
@titleand@notice- Public variable documentation with
@notice- Error documentation with
@noticeExample:
/// @title Universal Token Contract for Cross-chain Transfers /// @notice Implements ERC20 token with cross-chain transfer capabilities contract Universal is ERC20, Ownable2Step, UniversalContract, Events { /// @notice ZetaChain gateway interface for cross-chain communication GatewayZEVM public immutable gateway; /// @notice Error thrown when a transfer operation fails error TransferFailed();
93-95: Add event emission for mintingThe
mintfunction should emit an event to track token creation.function mint(address to, uint256 amount) public onlyOwner { + if (to == address(0)) revert InvalidAddress(); + if (amount == 0) revert("Zero amount mint"); _mint(to, amount); + emit TokenMinted(to, amount); }examples/token/tasks/deploy.ts (3)
7-12: Validate the presence of a signer more robustly.The current check may not cover all cases where a signer is invalid. Ensure that the signer object exists and has the necessary properties.
Modify the condition as follows:
-if (signer === undefined) { +if (!signer || !signer.address) {
24-37: Ensure consistent output formatting.To enhance maintainability and readability, consider using a structured logging library or utilities to format the output. This approach promotes consistency across different output formats (JSON and plain text).
For example, you might abstract the logging:
function logDeploymentResult(contract: Contract, network: string, signer: Signer, isJson: boolean) { const result = { contractAddress: contract.address, deployer: await signer.getAddress(), network: network, transactionHash: contract.deployTransaction.hash, }; if (isJson) { console.log(JSON.stringify(result)); } else { console.log(`🚀 Successfully deployed "${contract.contractName}" contract on ${network}. 📜 Contract address: ${result.contractAddress} 🔗 Transaction hash: ${result.transactionHash}`); } }And then call:
await logDeploymentResult(contract, network, signer, args.json);
40-50: Provide default values using environment variables or configuration.Hardcoding addresses and default parameter values can lead to issues when deploying to different environments. Use environment variables or a configuration file to manage default values dynamically.
For instance, utilize
process.env:-.addOptionalParam("gateway", "Gateway address (default: ZetaChain Gateway)", "0x5FC8d32690cc91D4c39d9d3abcBD16989F875707"); +.addOptionalParam("gateway", "Gateway address", process.env.GATEWAY_ADDRESS || "default_gateway_address");Ensure to document required environment variables in your README or project documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
examples/token/contracts/Connected.sol(1 hunks)examples/token/contracts/Universal.sol(1 hunks)examples/token/contracts/shared/Events.sol(1 hunks)examples/token/scripts/test.sh(1 hunks)examples/token/tasks/deploy.ts(1 hunks)examples/token/tasks/transfer.ts(1 hunks)
🧰 Additional context used
🪛 Shellcheck
examples/token/scripts/test.sh
[warning] 13-13: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 15-15: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (4)
examples/token/contracts/shared/Events.sol (2)
1-2: LGTM! Modern Solidity version selected.
The use of Solidity 0.8.26 is appropriate as it includes important safety features and optimizations.
5-8: Verify the byte array indexing implementation.
The contractAddress parameter is indexed as bytes. Ensure that the contract addresses across different chains are properly encoded and decoded when using this event.
✅ Verification successful
Byte array indexing implementation is correctly handled
The implementation shows proper handling of contract addresses across chains:
- In
Universal.sol, thecontractAddressis stored as bytes in thecounterpartymapping and emitted in the event - The event is emitted in a consistent manner when setting counterparty mappings
- The bytes format allows for flexibility in handling different address formats across chains
- Both NFT and Token implementations follow the same pattern for cross-chain address handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations that emit CounterpartyMappingSet
# to verify proper bytes encoding
rg -l "emit CounterpartyMappingSet" --type solidity
Length of output: 86
Script:
#!/bin/bash
# Search for implementations that emit CounterpartyMappingSet
# using ripgrep without file type filter
rg "emit CounterpartyMappingSet" -A 3
# Also search for any encoding/decoding of contract addresses
rg "contractAddress" -A 3
Length of output: 8774
examples/token/contracts/Connected.sol (1)
1-10: LGTM! Well-structured contract setup with secure inheritance choices.
The contract follows best practices by:
- Using a recent Solidity version with built-in overflow checks
- Implementing OpenZeppelin's Ownable2Step for secure ownership transfers
- Following a clear inheritance pattern
examples/token/contracts/Universal.sol (1)
29-46: LGTM! Well-structured constructor and modifier
The constructor properly validates inputs and the modifier has clear error messaging.
| return ""; | ||
| } | ||
|
|
||
| function onRevert(RevertContext calldata context) external onlyGateway {} |
There was a problem hiding this comment.
Implement revert handling logic.
The empty onRevert function could lead to lost tokens in case of cross-chain transfer failures. Consider implementing proper revert handling logic to ensure tokens can be recovered.
Would you like me to provide an implementation for handling cross-chain transfer reverts?
| function setCounterparty(address contractAddress) external onlyOwner { | ||
| counterparty = contractAddress; | ||
| emit CounterpartySet(contractAddress); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add address validation and improve event emission.
The function should validate the new counterparty address and include the old value in the event for better traceability.
Apply this improvement:
function setCounterparty(address contractAddress) external onlyOwner {
+ if (contractAddress == address(0)) revert InvalidAddress("counterparty address is zero");
+ address oldCounterparty = counterparty;
counterparty = contractAddress;
- emit CounterpartySet(contractAddress);
+ emit CounterpartySet(oldCounterparty, contractAddress);
}Committable suggestion skipped: line range outside the PR's diff.
| function onCall( | ||
| MessageContext calldata context, | ||
| bytes calldata message | ||
| ) external payable onlyGateway returns (bytes4) { | ||
| if (context.sender != counterparty) revert Unauthorized(); | ||
| (address receiver, uint256 amount) = abi.decode( | ||
| message, | ||
| (address, uint256) | ||
| ); | ||
| _mint(receiver, amount); | ||
| emit TokenTransferReceived(receiver, amount); | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add amount validation in onCall function.
The function should validate the minting amount to prevent potential issues.
Apply this improvement:
function onCall(
MessageContext calldata context,
bytes calldata message
) external payable onlyGateway returns (bytes4) {
if (context.sender != counterparty) revert Unauthorized();
(address receiver, uint256 amount) = abi.decode(
message,
(address, uint256)
);
+ if (receiver == address(0)) revert InvalidAddress("receiver is zero");
+ if (amount == 0) revert("amount cannot be zero");
_mint(receiver, amount);
emit TokenTransferReceived(receiver, amount);
return "";
}Committable suggestion skipped: line range outside the PR's diff.
| function transferCrossChain( | ||
| address destination, | ||
| address receiver, | ||
| uint256 amount | ||
| ) external payable { | ||
| _burn(msg.sender, amount); | ||
| bytes memory message = abi.encode(destination, receiver, amount); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| message, | ||
| 0 | ||
| ); | ||
|
|
||
| if (destination == address(0)) { | ||
| gateway.call(counterparty, message, revertOptions); | ||
| } else { | ||
| gateway.depositAndCall{value: msg.value}( | ||
| counterparty, | ||
| message, | ||
| revertOptions | ||
| ); | ||
| } | ||
| emit TokenTransfer(destination, receiver, amount); | ||
| } |
There was a problem hiding this comment.
Add security checks and follow CEI pattern.
The function has several security considerations that should be addressed:
- No validation if counterparty is set
- No validation of amount
- External calls after state modification (potential reentrancy)
Apply these security improvements:
function transferCrossChain(
address destination,
address receiver,
uint256 amount
) external payable {
+ if (counterparty == address(0)) revert InvalidAddress("counterparty not set");
+ if (receiver == address(0)) revert InvalidAddress("receiver is zero");
+ if (amount == 0) revert("amount cannot be zero");
+
+ // Cache msg.value to follow CEI pattern
+ uint256 value = msg.value;
_burn(msg.sender, amount);
bytes memory message = abi.encode(destination, receiver, amount);
RevertOptions memory revertOptions = RevertOptions(
address(this),
true,
address(0),
message,
0
);
if (destination == address(0)) {
gateway.call(counterparty, message, revertOptions);
} else {
- gateway.depositAndCall{value: msg.value}(
+ gateway.depositAndCall{value: value}(
counterparty,
message,
revertOptions
);
}
emit TokenTransfer(destination, receiver, amount);
}Committable suggestion skipped: line range outside the PR's diff.
| function transferCrossChain( | ||
| address destination, | ||
| address receiver, | ||
| uint256 amount | ||
| ) public { | ||
| if (receiver == address(0)) revert InvalidAddress(); | ||
| _burn(msg.sender, amount); | ||
|
|
||
| (, 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 message = abi.encode(receiver, amount); | ||
|
|
||
| CallOptions memory callOptions = CallOptions(gasLimit, false); | ||
|
|
||
| RevertOptions memory revertOptions = RevertOptions( | ||
| address(this), | ||
| true, | ||
| address(0), | ||
| message, | ||
| gasLimit | ||
| ); | ||
|
|
||
| gateway.call( | ||
| counterparty[destination], | ||
| destination, | ||
| message, | ||
| callOptions, | ||
| revertOptions | ||
| ); | ||
| emit TokenTransfer(destination, receiver, amount); | ||
| } |
There was a problem hiding this comment.
Add input validation and reentrancy protection
The transferCrossChain function needs additional security measures:
- Add a check for zero amount transfers
- Implement reentrancy protection using OpenZeppelin's
ReentrancyGuard - Consider implementing a maximum amount check
Apply this diff:
+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-contract Universal is ERC20, Ownable2Step, UniversalContract, Events {
+contract Universal is ERC20, Ownable2Step, UniversalContract, Events, ReentrancyGuard {
// ...
- function transferCrossChain(
+ function transferCrossChain(
address destination,
address receiver,
uint256 amount
- ) public {
+ ) public nonReentrant {
+ if (amount == 0) revert("Zero amount transfer");
if (receiver == address(0)) revert InvalidAddress();Committable suggestion skipped: line range outside the PR's diff.
| const [signer] = await hre.ethers.getSigners(); | ||
| if (signer === undefined) { | ||
| throw new Error( | ||
| `Wallet not found. Please, run "npx hardhat account --save" or set PRIVATE_KEY env variable (for example, in a .env file)` |
There was a problem hiding this comment.
Correct the command in the error message.
The command npx hardhat account --save may be incorrect or non-standard. Provide accurate instructions to prevent user confusion.
Update the error message:
-`Wallet not found. Please, run "npx hardhat account --save" or set PRIVATE_KEY env variable (for example, in a .env file)`
+`Wallet not found. Please set the PRIVATE_KEY environment variable (e.g., in a .env file) or ensure your Hardhat configuration includes a valid account.`📝 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.
| `Wallet not found. Please, run "npx hardhat account --save" or set PRIVATE_KEY env variable (for example, in a .env file)` | |
| `Wallet not found. Please set the PRIVATE_KEY environment variable (e.g., in a .env file) or ensure your Hardhat configuration includes a valid account.` |
| ); | ||
| } | ||
|
|
||
| const factory: any = await hre.ethers.getContractFactory(args.name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify the correct type for the contract factory.
Using any diminishes type checking for the contract factory. Replace any with the appropriate type.
Adjust the declaration:
-const factory: any = await hre.ethers.getContractFactory(args.name);
+const factory = await hre.ethers.getContractFactory(args.name);TypeScript can infer the type, or you can specify it explicitly if necessary.
📝 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 factory: any = await hre.ethers.getContractFactory(args.name); | |
| const factory = await hre.ethers.getContractFactory(args.name); |
| const contract = await factory.deploy( | ||
| args.gateway, | ||
| signer.address, | ||
| args.tokenName, | ||
| args.tokenSymbol, | ||
| ...(args.gasLimit ? [args.gasLimit] : []) | ||
| ); |
There was a problem hiding this comment.
Pass deployment overrides correctly to include gas limit.
The gas limit should be provided as an override, not as a constructor parameter. Including it in the constructor arguments may lead to unexpected behavior or deployment failures.
Refactor the deployment call:
- const contract = await factory.deploy(
- args.gateway,
- signer.address,
- args.tokenName,
- args.tokenSymbol,
- ...(args.gasLimit ? [args.gasLimit] : [])
- );
+ const deployArgs = [
+ args.gateway,
+ signer.address,
+ args.tokenName,
+ args.tokenSymbol,
+ ];
+
+ const deployOptions = args.gasLimit ? { gasLimit: args.gasLimit } : {};
+
+ const contract = await factory.deploy(...deployArgs, deployOptions);Committable suggestion skipped: line range outside the PR's diff.
| import { task, types } from "hardhat/config"; | ||
| import { HardhatRuntimeEnvironment } from "hardhat/types"; | ||
|
|
||
| const main = async (args: any, hre: HardhatRuntimeEnvironment) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define specific types for function parameters.
Using any for the args parameter reduces type safety and may lead to runtime errors. Consider defining a proper interface for args to enhance code reliability.
Apply this change:
-const main = async (args: any, hre: HardhatRuntimeEnvironment) => {
+interface DeployArgs {
+ json?: boolean;
+ tokenName?: string;
+ tokenSymbol?: string;
+ name?: string;
+ gasLimit?: string;
+ gateway?: string;
+}
+
+const main = async (args: DeployArgs, hre: HardhatRuntimeEnvironment) => {📝 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) => { | |
| interface DeployArgs { | |
| json?: boolean; | |
| tokenName?: string; | |
| tokenSymbol?: string; | |
| name?: string; | |
| gasLimit?: string; | |
| gateway?: string; | |
| } | |
| const main = async (args: DeployArgs, hre: HardhatRuntimeEnvironment) => { |
|
In this PR I've also modified the NFT example (events and parameter order) to make the two tutorials more consistent. |
fadeev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
examples/nft/contracts/Connected.sol (1)
Line range hint
1-156: Consider gas optimization for cross-chain operations.Cross-chain operations can be expensive. Consider implementing the following optimizations:
- Cache the URI data off-chain and only transfer a reference/hash in the cross-chain message
- Implement batch transfer functionality to amortize cross-chain messaging costs
- Consider implementing a message compression mechanism for URIs
Would you like assistance in implementing any of these optimizations?
examples/nft/contracts/Universal.sol (3)
128-132: Consider optimizing variable declarations for gas efficiency.The message decoding and event emissions follow the correct parameter order. However, we can optimize gas usage by combining related operations.
Consider this optimization:
- address destination, - address sender, - uint256 tokenId, - string memory uri + address destination, address sender, + uint256 tokenId, string memory uriAlso applies to: 137-137, 155-155, 159-159
164-166: Consider enhancing error handling with specific revert reasons.While the revert handling is functionally correct, consider adding specific error messages for different failure scenarios to improve debugging.
Add custom errors:
+ error InvalidRevertMessage(); + error TokenRecoveryFailed(); function onRevert(RevertContext calldata context) external onlyGateway { + if (context.revertMessage.length == 0) revert InvalidRevertMessage(); (address sender, uint256 tokenId, string memory uri) = abi.decode( context.revertMessage, (address, uint256, string) );Also applies to: 171-171
Line range hint
1-171: Consider implementing additional security measures.While the contract has good security foundations, consider these enhancements:
- Add reentrancy guards for external calls
- Implement emergency pause functionality
- Add events for critical state changes (e.g., gasLimit updates)
Would you like me to provide implementation details for these security enhancements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
examples/nft/contracts/Connected.sol(3 hunks)examples/nft/contracts/Universal.sol(4 hunks)examples/nft/contracts/shared/Events.sol(1 hunks)examples/token/hardhat.config.ts(1 hunks)examples/token/scripts/test.sh(1 hunks)examples/token/tasks/connectedSetCounterparty.ts(1 hunks)examples/token/tasks/mint.ts(1 hunks)examples/token/tasks/transfer.ts(1 hunks)examples/token/tasks/universalSetCounterparty.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/token/hardhat.config.ts
- examples/token/tasks/connectedSetCounterparty.ts
- examples/token/tasks/mint.ts
- examples/token/tasks/transfer.ts
- examples/token/tasks/universalSetCounterparty.ts
🧰 Additional context used
🪛 Shellcheck
examples/token/scripts/test.sh
[warning] 12-12: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 13-13: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 14-14: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (2)
examples/nft/contracts/shared/Events.sol (1)
8-29: LGTM! Parameter ordering follows a consistent pattern.
The reordering of event parameters follows a logical and consistent pattern across all transfer-related events:
- Network/destination parameters first
- Address-related parameters (sender/receiver)
- Token-specific parameters
- Additional metadata (uri)
This standardization improves code readability and maintains proper indexing for efficient event filtering.
examples/nft/contracts/Connected.sol (1)
68-68: Verify cross-chain message protocol compatibility.
The parameter ordering in message encoding and event emission has been changed. This modification requires careful verification to ensure compatibility with counterparty contracts across all chains.
Let's verify the counterparty contract implementation:
Also applies to: 88-88
✅ Verification successful
The previous ast-grep patterns might have been too specific. Let's try a broader search to understand the message encoding/decoding patterns and cross-chain message handling across the codebase.
Parameter reordering is consistent with cross-chain message protocol
The codebase analysis shows that the message encoding/decoding patterns are consistent across the contracts:
-
In Connected.sol (the contract under review):
- Encoding:
abi.encode(destination, receiver, tokenId, uri) - Decoding:
abi.decode(message, (address, uint256, string))
- Encoding:
-
In Universal.sol (the counterparty contract):
- Encoding:
abi.encode(receiver, tokenId, uri) - Decoding:
abi.decode(message, (address, address, uint256, string))
- Encoding:
The parameter ordering is properly aligned between the contracts, with Universal.sol correctly handling the additional destination parameter in the cross-chain message protocol.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for message encoding/decoding patterns in other contracts
ast-grep --pattern 'abi.encode($$$destination$$$, $$$receiver$$$, $$$tokenId$$$, $$$uri$$$)'
ast-grep --pattern 'abi.decode($$$, ($$$address$$$, $$$uint256$$$, $$$string$$$))'
Length of output: 179
Script:
#!/bin/bash
# Let's search for all abi.encode and abi.decode patterns in the codebase
rg "abi\.(encode|decode)" -A 3 -B 3
# Also search for any files that might contain cross-chain message handling
fd -e sol | xargs rg "(onCall|onRevert|transferCrossChain)" -A 5 -B 5
Length of output: 25867
| address indexed destination, | ||
| address indexed receiver, | ||
| uint256 indexed tokenId, | ||
| string uri | ||
| ); | ||
| event TokenTransferReceived( | ||
| uint256 indexed tokenId, | ||
| address indexed receiver, | ||
| uint256 indexed tokenId, | ||
| string uri | ||
| ); | ||
| event TokenTransferReverted( | ||
| uint256 indexed tokenId, | ||
| address indexed sender, | ||
| uint256 indexed tokenId, | ||
| string uri | ||
| ); | ||
| event CounterpartySet(address indexed zrc20, bytes indexed contractAddress); | ||
| event TokenTransferToDestination( | ||
| uint256 indexed tokenId, | ||
| address indexed sender, | ||
| address indexed destination, | ||
| address indexed sender, | ||
| uint256 indexed tokenId, | ||
| string uri | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Parameter mismatch detected in TokenTransfer event between declaration and usage
The verification reveals inconsistencies in event parameter ordering:
- In
examples/nft/contracts/Universal.sol,TokenTransferis emitted with parameters(receiver, destination, tokenId, uri) - However, in
Events.sol, it's declared as(destination, receiver, tokenId, uri) - Similar parameter ordering should be maintained across the codebase to prevent event handling issues
This mismatch could lead to incorrect event handling by external listeners and should be corrected to maintain consistency with the event declaration in Events.sol.
🔗 Analysis chain
Verify event parameter alignment with external contracts.
The parameter reordering appears to align with changes in the Connected and Universal contracts mentioned in the summary. However, it's crucial to verify this alignment to prevent any potential event handling issues.
Let me gather more information about the event declarations to complete the verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify event parameter ordering consistency across related contracts
# Expected: Parameter ordering should match in event emissions and declarations
# Search for event emissions in related contracts
echo "Checking event emissions in contracts:"
rg -A 2 "emit Token(Transfer|TransferReceived|TransferReverted|TransferToDestination)"
# Search for interface definitions that might be listening to these events
echo "Checking interface definitions:"
ast-grep --pattern 'interface $_ {
$$$
event Token$_(address indexed $_, $$$);
$$$
}'
Length of output: 2702
Script:
#!/bin/bash
# Search for event declarations in all contracts
echo "Checking event declarations in contracts:"
rg -A 3 "event Token(Transfer|TransferReceived|TransferReverted|TransferToDestination)" --type sol
# Search specifically in the shared Events.sol files
echo -e "\nChecking Events.sol files:"
fd "Events.sol" --exec cat {}
Length of output: 1911
| balance | ||
|
|
||
| echo -e "\nTransferring token: ZetaChain → Ethereum..." | ||
| npx hardhat transfer --network localhost --json --amount 10 --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM" |
There was a problem hiding this comment.
Add transaction receipt validation for token transfers.
The current implementation doesn't validate the transaction receipts. Consider adding receipt validation to ensure transfers are successful.
-npx hardhat transfer --network localhost --json --amount 10 --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM"
+# Function to validate transaction receipt
+validate_receipt() {
+ local receipt=$1
+ local status
+ status=$(echo "$receipt" | jq -r '.status')
+ if [ "$status" != "1" ]; then
+ echo "Transaction failed with status: $status"
+ exit 1
+ fi
+}
+
+# Execute transfer with receipt validation
+receipt=$(npx hardhat transfer --network localhost --json --amount 10 --from "$CONTRACT_ZETACHAIN" --to "$ZRC20_ETHEREUM")
+validate_receipt "$receipt"Also applies to: 64-64, 70-70
| npx hardhat localnet-check | ||
| balance | ||
|
|
||
| npx hardhat localnet-stop |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance network cleanup reliability.
The cleanup process should handle potential failures and ensure all resources are properly released.
-npx hardhat localnet-stop
+cleanup() {
+ echo "Cleaning up..."
+ if ! npx hardhat localnet-stop; then
+ echo "Warning: Failed to stop local network gracefully"
+ # Force cleanup as fallback
+ pkill -f "hardhat localnet" || true
+ 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 | |
| cleanup() { | |
| echo "Cleaning up..." | |
| if ! npx hardhat localnet-stop; then | |
| echo "Warning: Failed to stop local network gracefully" | |
| # Force cleanup as fallback | |
| pkill -f "hardhat localnet" || true | |
| fi | |
| } | |
| trap cleanup EXIT | |
| cleanup |
| (address sender, uint256 tokenId, string memory uri) = abi.decode( | ||
| context.revertMessage, | ||
| (uint256, address, string) | ||
| (address, uint256, string) | ||
| ); | ||
|
|
||
| _safeMint(sender, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| emit TokenTransferReverted(tokenId, sender, uri); | ||
| emit TokenTransferReverted(sender, tokenId, uri); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add address validation in onRevert function.
For consistency with onCall and other functions, add validation for the decoded sender address.
(address sender, uint256 tokenId, string memory uri) = abi.decode(
context.revertMessage,
(address, uint256, string)
);
+if (sender == address(0)) revert InvalidAddress();
_safeMint(sender, tokenId);📝 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.
| (address sender, uint256 tokenId, string memory uri) = abi.decode( | |
| context.revertMessage, | |
| (uint256, address, string) | |
| (address, uint256, string) | |
| ); | |
| _safeMint(sender, tokenId); | |
| _setTokenURI(tokenId, uri); | |
| emit TokenTransferReverted(tokenId, sender, uri); | |
| emit TokenTransferReverted(sender, tokenId, uri); | |
| (address sender, uint256 tokenId, string memory uri) = abi.decode( | |
| context.revertMessage, | |
| (address, uint256, string) | |
| ); | |
| if (sender == address(0)) revert InvalidAddress(); | |
| _safeMint(sender, tokenId); | |
| _setTokenURI(tokenId, uri); | |
| emit TokenTransferReverted(sender, tokenId, uri); |
| (address receiver, uint256 tokenId, string memory uri) = abi.decode( | ||
| message, | ||
| (uint256, address, string) | ||
| (address, uint256, string) | ||
| ); | ||
|
|
||
| _safeMint(receiver, tokenId); | ||
| _setTokenURI(tokenId, uri); | ||
| emit TokenTransferReceived(tokenId, receiver, uri); | ||
| emit TokenTransferReceived(receiver, tokenId, uri); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add address validation in onCall function.
While the parameter reordering is correct, consider adding validation for the decoded receiver address to maintain consistency with other functions that check for zero addresses.
(address receiver, uint256 tokenId, string memory uri) = abi.decode(
message,
(address, uint256, string)
);
+if (receiver == address(0)) revert InvalidAddress();
_safeMint(receiver, tokenId);Committable suggestion skipped: line range outside the PR's diff.
| @@ -79,7 +79,7 @@ contract Universal is | |||
| !IZRC20(destination).transferFrom(msg.sender, address(this), gasFee) | |||
| ) revert TransferFailed(); | |||
| IZRC20(destination).approve(address(gateway), gasFee); | |||
| bytes memory message = abi.encode(tokenId, receiver, uri); | |||
| bytes memory message = abi.encode(receiver, tokenId, uri); | |||
There was a problem hiding this comment.
💡 Codebase verification
Parameter ordering inconsistency detected between Universal and Connected contracts
The parameter ordering differs between the contracts:
Universal.sol:abi.encode(receiver, tokenId, uri)Connected.sol:abi.encode(destination, receiver, tokenId, uri)
This inconsistency in message encoding could lead to cross-chain transfer failures. The Connected contract expects an additional destination parameter in the encoded message that Universal contract does not provide.
🔗 Analysis chain
Parameter reordering looks good, verify cross-chain compatibility.
The reordering of parameters in message encoding and event emission maintains internal consistency. The sequence (receiver, tokenId, uri) is now uniform throughout the contract.
Let's verify the parameter order consistency in connected contracts:
Also applies to: 102-102
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for message encoding patterns in connected contracts
rg -A 2 "abi\.encode.*receiver.*tokenId.*uri" --type sol
Length of output: 86
Script:
#!/bin/bash
# Search for message encoding patterns in all files
rg -A 2 "abi\.encode.*receiver.*tokenId.*uri"
# Also search for message decoding patterns
rg -A 2 "abi\.decode.*receiver.*tokenId.*uri"
# Search for any abi.encode patterns to analyze parameter ordering
rg -A 2 "abi\.encode"
Length of output: 3355
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
examples/token/tasks/transfer.ts(1 hunks)
🔇 Additional comments (2)
examples/token/tasks/transfer.ts (2)
36-41:
Add validation for cross-chain transfer
The cross-chain transfer lacks validation for critical parameters and proper error handling.
Consider this implementation:
- tx = await (contract as any).transferCrossChain(
- args.to,
- receiver,
- args.amount,
- { ...txOptions, value: gasAmount }
- );
+ if (!contract.transferCrossChain) {
+ throw new Error("Contract does not support cross-chain transfers");
+ }
+
+ if (!ethers.utils.isAddress(args.to)) {
+ throw new Error("Invalid destination address");
+ }
+
+ tx = await contract.transferCrossChain(
+ args.to,
+ receiver,
+ args.amount,
+ { ...txOptions, value: gasAmount }
+ ).catch((error: Error) => {
+ throw new Error(`Cross-chain transfer failed: ${error.message}`);
+ });Likely invalid or redundant comment.
67-68: Review default gas values
The default gas values might need adjustment based on the network and contract complexity:
- Gas price: 10 Gwei might be too low for some networks
- Gas limit: 7M might be excessive for token transfers
Let's analyze typical gas usage:
Also applies to: 73-74
✅ Verification successful
The previous search might have failed due to incorrect pattern matching. Let's try a broader search to understand the gas configuration context and transfer implementations in the codebase.
Gas configuration values are appropriate for the use case
Based on the codebase analysis:
- The 10 Gwei (10000000000 wei) gas price is used consistently across examples and is reasonable for test/development environments
- The 7M gas limit is appropriate as the contracts perform complex operations:
- Cross-chain transfers via ZetaChain gateway
- Gas fee calculations using
withdrawGasFeeWithGasLimit - Multiple contract interactions in a single transaction
The values align with similar configurations in other parts of the codebase (e.g., NFT, Hello, and Swap examples) and are suitable for the cross-chain token transfer operations being performed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check typical gas usage for token transfers
rg -A 2 "transferCrossChain.*\{" | grep -A 2 "gasLimit"
Length of output: 55
Script:
#!/bin/bash
# Let's search for gas-related configurations and transfer functions
rg -i "gas.*limit|gas.*price" -A 2
# Also search for transfer-related functions to understand the context
ast-grep --pattern 'function $_ ($$$) {
$$$
transfer$$$($$$)
$$$
}'
Length of output: 17023
| .addFlag("callOnRevert", "Whether to call on revert") | ||
| .addOptionalParam( | ||
| "revertAddress", | ||
| "The address to call on revert", | ||
| "0x0000000000000000000000000000000000000000" | ||
| ) | ||
| .addOptionalParam("revertMessage", "The message to send on revert", "0x") | ||
| .addOptionalParam( | ||
| "onRevertGasLimit", | ||
| "The gas limit for the revert transaction", | ||
| 7000000, | ||
| types.int | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused revert parameters
The following parameters are defined but never used in the implementation:
callOnRevertrevertAddressrevertMessageonRevertGasLimit
Either implement the revert functionality or remove these unused parameters to avoid confusion.
| } | ||
| }; | ||
|
|
||
| task("transfer", "Transfer and lock an NFT", main) |
There was a problem hiding this comment.
Fix inconsistent task description
The task description mentions "Transfer and lock an NFT" but the implementation is for ERC-20 tokens.
Update the task description:
-task("transfer", "Transfer and lock an NFT", main)
+task("transfer", "Transfer ERC-20 tokens cross-chain", main)📝 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.
| task("transfer", "Transfer and lock an NFT", main) | |
| task("transfer", "Transfer ERC-20 tokens cross-chain", main) |
| const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | ||
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | ||
| await zrc20TransferTx.wait(); | ||
| const tokenApprove = await contract.approve(args.from, value); | ||
| await tokenApprove.wait(); |
There was a problem hiding this comment.
Add proper error handling for token approvals
The approval transactions lack proper error handling and validation. A failed approval could lead to a stuck transaction.
Consider this implementation:
- const [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
- const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions);
- await zrc20TransferTx.wait();
- const tokenApprove = await contract.approve(args.from, value);
- await tokenApprove.wait();
+ const [success, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit);
+ if (!success) {
+ throw new Error("Failed to calculate gas fee");
+ }
+
+ const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions);
+ const zrc20Receipt = await zrc20TransferTx.wait();
+ if (!zrc20Receipt.status) {
+ throw new Error("ZRC20 approval failed");
+ }
+
+ const tokenApprove = await contract.approve(args.from, value);
+ const tokenReceipt = await tokenApprove.wait();
+ if (!tokenReceipt.status) {
+ throw new Error("Token approval failed");
+ }📝 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 [, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | |
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | |
| await zrc20TransferTx.wait(); | |
| const tokenApprove = await contract.approve(args.from, value); | |
| await tokenApprove.wait(); | |
| const [success, gasFee] = await zrc20.withdrawGasFeeWithGasLimit(gasLimit); | |
| if (!success) { | |
| throw new Error("Failed to calculate gas fee"); | |
| } | |
| const zrc20TransferTx = await zrc20.approve(args.from, gasFee, txOptions); | |
| const zrc20Receipt = await zrc20TransferTx.wait(); | |
| if (!zrc20Receipt.status) { | |
| throw new Error("ZRC20 approval failed"); | |
| } | |
| const tokenApprove = await contract.approve(args.from, value); | |
| const tokenReceipt = await tokenApprove.wait(); | |
| if (!tokenReceipt.status) { | |
| throw new Error("Token approval failed"); | |
| } |
Depends on zeta-chain/localnet#52
Summary by CodeRabbit
Release Notes
New Features
ConnectedandUniversal.README.mddetailing compatibility with localnet versionv4.0.0-rc*.Configuration Changes
.gitignore,.eslintignore,foundry.toml,hardhat.config.ts,tsconfig.json, andLICENSE.package.jsonto include scripts for testing and linting.Documentation
README.mdto include new features and usage instructions.README.mdspecifying compatibility with localnet versionv4.0.0-rc*.