NFT and FT Standards #509
NFT and FT Standards #509
Conversation
fadeev
requested review from
bbbeeeee,
brewmaster012 and
a team
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes in this pull request involve the addition of new metadata and documentation regarding contract standards on ZetaChain. A new section titled "standards" has been introduced in the existing Changes
Possibly related PRs
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: 8
🧹 Outside diff range and nitpick comments (4)
src/pages/developers/_meta.json (1)
14-17: LGTM with a minor suggestion for the description.The new standards section is well-structured and logically placed. Consider enhancing the description to highlight ZetaChain's unique cross-chain capabilities.
"standards": { "title": "Contract Standards", - "description": "Learn about the different contract standards available on ZetaChain and how to use them." + "description": "Learn about ZetaChain's cross-chain compatible contract standards and how to leverage them for omnichain interoperability." },src/pages/developers/standards/token.mdx (1)
13-13: Consider using a more flexible Solidity version constraint.Using such a specific version (0.8.26) might cause compatibility issues. Consider using a version range that includes recent stable releases.
-pragma solidity 0.8.26; +pragma solidity ^0.8.0;src/pages/developers/standards/nft.mdx (2)
3-52: Consider enhancing security documentation.The documentation would benefit from a dedicated security considerations section that covers:
- Token ID collision prevention strategies
- Cross-chain transfer security implications
- Potential attack vectors and mitigations
- Gas fee considerations and DOS prevention
Would you like me to help draft a security considerations section?
55-74: Enhance contract documentation and version flexibility.Consider the following improvements:
- Add NatSpec documentation for the constructor parameters
- Consider using a version range (e.g.,
^0.8.0) unless there's a specific reason for 0.8.26// SPDX-License-Identifier: MIT -pragma solidity 0.8.26; +pragma solidity ^0.8.0; +/// @notice Connected NFT contract for cross-chain transfers +/// @dev Implements the Universal NFT standard for connected chains contract Connected is UniversalNFT { + /// @notice Initializes the Connected NFT contract + /// @param gatewayAddress The address of the ZetaChain gateway + /// @param owner The address of the contract owner + /// @param name The name of the NFT collection + /// @param symbol The symbol of the NFT collection + /// @param gasLimit The gas limit for cross-chain transfers constructor(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/pages/developers/_meta.json(1 hunks)src/pages/developers/standards/_meta.json(1 hunks)src/pages/developers/standards/nft.mdx(1 hunks)src/pages/developers/standards/token.mdx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/pages/developers/standards/_meta.json
🧰 Additional context used
🪛 LanguageTool
src/pages/developers/standards/nft.mdx
[misspelling] ~98-~98: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... Universal contract constructor accepts a Uniswap router address. It is recommend...
(EN_A_VS_AN)
[uncategorized] ~366-~366: Possible missing comma found.
Context: ...d with the gas limit on the destination chain and isArbitraryCall (the second param...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~369-~369: Possible missing article found.
Context: ...estination chain, but without providing address of the universal contract making the ca...
(AI_HYDRA_LEO_MISSING_THE)
[formatting] ~372-~372: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ... isArbitraryCall to false is important, because the contract on a connected chain must ...
(COMMA_BEFORE_BECAUSE)
[grammar] ~404-~404: The word ‘withdraw’ is not a noun. Did you mean “withdrawal”?
Context: ... connected chain. First, it queries the withdraw fee on the destination chain. Then, it ...
(PREPOSITION_VERB)
[uncategorized] ~651-~651: Possible missing comma found.
Context: ...To transfer an NFT to another connected chain the destination address must be the ZRC...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~655-~655: Possible missing comma found.
Context: ...C-20 ETH address. When transferring to ZetaChain a no asset gateway.call is executed, ...
(AI_HYDRA_LEO_MISSING_COMMA)
[formatting] ~655-~655: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...in a no asset gateway.call is executed, because cross-chain calls to ZetaChain do not r...
(COMMA_BEFORE_BECAUSE)
🔇 Additional comments (1)
src/pages/developers/standards/nft.mdx (1)
344-349:
Improve token ID generation mechanism.
The current token ID generation using block.number could be manipulated by miners/validators. Consider using a more robust mechanism:
- Use a combination of chainId, timestamp, and nonce
- Implement a UUID-like generation system
-uint256 hash = uint256(
- keccak256(
- abi.encodePacked(address(this), block.number, _nextTokenId++)
- )
-);
+uint256 hash = uint256(
+ keccak256(
+ abi.encodePacked(
+ block.chainid,
+ block.timestamp,
+ address(this),
+ _nextTokenId++,
+ msg.sender
+ )
+ )
+);Likely invalid or redundant comment.
| The Universal Token standard enables ERC-20 fungible tokens to be minted on any | ||
| chain and seamlessly transferred between connected chains. | ||
|
|
||
| When transferring tokens between chains, a token is burned on the source chain. | ||
| The token's amount is sent in a message to the token contract on the destination | ||
| chain, where a corresponding token is minted. | ||
|
|
||
| The Universal Token standard works the same way as [Universal | ||
| NFT](/developers/standards/nft). | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Documentation needs enhancement with essential implementation details.
The current documentation, while concise, would benefit from additional sections covering:
- List of supported chains/networks
- Gas fee handling across chains
- Security considerations and best practices
- Implementation examples with code snippets
- Event specifications for monitoring transfers
Would you like me to help draft these additional documentation sections?
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.26; | ||
|
|
||
| import "@zetachain/standard-contracts/nft/contracts/evm/UniversalNFT.sol"; |
There was a problem hiding this comment.
Incorrect import path for UniversalToken.
The import statement references the NFT contract instead of the token contract.
-import "@zetachain/standard-contracts/nft/contracts/evm/UniversalNFT.sol";
+import "@zetachain/standard-contracts/token/contracts/evm/UniversalToken.sol";📝 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 "@zetachain/standard-contracts/nft/contracts/evm/UniversalNFT.sol"; | |
| import "@zetachain/standard-contracts/token/contracts/evm/UniversalToken.sol"; |
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.26; | ||
|
|
||
| import "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol"; |
There was a problem hiding this comment.
Incorrect import path and chain-specific location.
The import statement references the NFT contract and uses a chain-specific path.
-import "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol";
+import "@zetachain/standard-contracts/token/contracts/universal/UniversalToken.sol";📝 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 "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol"; | |
| import "@zetachain/standard-contracts/token/contracts/universal/UniversalToken.sol"; |
| contract Universal is UniversalToken { | ||
| constructor( | ||
| address payable gatewayAddress, | ||
| address owner, | ||
| string memory name, | ||
| string memory symbol, | ||
| uint256 gasLimit, | ||
| address uniswapRouter | ||
| ) | ||
| UniversalToken(gatewayAddress, gasLimit, uniswapRouter) | ||
| Ownable(owner) | ||
| ERC20(name, symbol) | ||
| {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation and comprehensive documentation.
The contract needs input validation and documentation explaining the purpose of uniswapRouter.
+/// @notice Creates a new Universal token contract for ZetaChain
+/// @param gatewayAddress The address of the ZetaChain gateway contract
+/// @param owner The address that will own this contract
+/// @param name The name of the token
+/// @param symbol The symbol of the token
+/// @param gasLimit The gas limit for cross-chain operations
+/// @param uniswapRouter The address of the Uniswap router for liquidity operations
constructor(
address payable gatewayAddress,
address owner,
string memory name,
string memory symbol,
uint256 gasLimit,
address uniswapRouter
- )
+ ) public {
+ require(uniswapRouter != address(0), "Invalid uniswap router address");
UniversalToken(gatewayAddress, gasLimit, uniswapRouter)
Ownable(owner)
ERC20(name, symbol)
{}Committable suggestion skipped: line range outside the PR's diff.
| constructor( | ||
| address payable gatewayAddress, | ||
| address owner, | ||
| string memory name, | ||
| string memory symbol, | ||
| uint256 gasLimit | ||
| ) | ||
| UniversalToken(gatewayAddress, gasLimit) | ||
| Ownable(owner) | ||
| ERC20(name, symbol) | ||
| {} | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add NatSpec documentation and explicit visibility modifier.
The constructor lacks documentation explaining its parameters and their requirements.
+/// @notice Creates a new Connected token contract
+/// @param gatewayAddress The address of the ZetaChain gateway contract
+/// @param owner The address that will own this contract
+/// @param name The name of the token
+/// @param symbol The symbol of the token
+/// @param gasLimit The gas limit for cross-chain operations
- constructor(
+ constructor public (
address payable gatewayAddress,
address owner,
string memory name,
string memory symbol,
uint256 gasLimit
)Committable suggestion skipped: line range outside the PR's diff.
| The Universal Token standard enables ERC-20 fungible tokens to be minted on any | ||
| chain and seamlessly transferred between connected chains. | ||
|
|
||
| When transferring tokens between chains, a token is burned on the source chain. | ||
| The token's amount is sent in a message to the token contract on the destination | ||
| chain, where a corresponding token is minted. | ||
|
|
||
| The Universal Token standard works the same way as [Universal | ||
| NFT](/developers/standards/nft). | ||
|
|
||
| ```solidity filename="contracts/Connected.sol" | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.26; | ||
|
|
||
| import "@zetachain/standard-contracts/nft/contracts/evm/UniversalNFT.sol"; | ||
|
|
||
| contract Connected is UniversalToken { | ||
| constructor( | ||
| address payable gatewayAddress, | ||
| address owner, | ||
| string memory name, | ||
| string memory symbol, | ||
| uint256 gasLimit | ||
| ) | ||
| UniversalToken(gatewayAddress, gasLimit) | ||
| Ownable(owner) | ||
| ERC20(name, symbol) | ||
| {} | ||
| } | ||
| ``` | ||
|
|
||
| ```solidity filename="contracts/Universal.sol" | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.26; | ||
|
|
||
| import "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol"; | ||
|
|
||
| contract Universal is UniversalToken { | ||
| constructor( | ||
| address payable gatewayAddress, | ||
| address owner, | ||
| string memory name, | ||
| string memory symbol, | ||
| uint256 gasLimit, | ||
| address uniswapRouter | ||
| ) | ||
| UniversalToken(gatewayAddress, gasLimit, uniswapRouter) | ||
| Ownable(owner) | ||
| ERC20(name, symbol) | ||
| {} | ||
| } | ||
| ``` |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect import paths detected in token implementation
The code shows inconsistency in imports. Both Connected.sol and Universal.sol are importing from @zetachain/standard-contracts/nft/contracts/*/UniversalNFT.sol while implementing UniversalToken. This appears to be incorrect as NFT-related imports are being used for token contracts.
• contracts/Connected.sol: Incorrect import path @zetachain/standard-contracts/nft/contracts/evm/UniversalNFT.sol
• contracts/Universal.sol: Incorrect import path @zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol
The imports should be from the token-specific paths instead of NFT paths to ensure proper implementation of the Universal Token standard.
🔗 Analysis chain
Verify cross-chain security measures.
Let's ensure consistent security patterns are used across similar cross-chain implementations.
Let me analyze the imports and security patterns to verify the cross-chain implementation consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar cross-chain token implementations
ast-grep --pattern 'contract $_ is UniversalToken {
$$$
}'
# Search for security-related patterns in cross-chain contracts
rg -l "require\(msg\.sender == gatewayAddress"
Length of output: 111
Script:
#!/bin/bash
# Search for similar cross-chain token implementations in the imports
rg -l "UniversalToken"
# Search for files containing gateway-related security patterns
rg -l "gateway.*address"
# Look for any token-related contracts
fd -e sol -x cat {} \; | grep -A 5 "contract.*Token"
# Check imports in the token contracts
rg "import.*@zetachain/standard-contracts"
Length of output: 3150
| (address gasZRC20, uint256 gasFee) = IZRC20(destination) | ||
| .withdrawGasFeeWithGasLimit(gasLimitAmount); | ||
| if (destination != gasZRC20) revert InvalidAddress(); | ||
| if ( | ||
| !IZRC20(destination).transferFrom(msg.sender, address(this), gasFee) | ||
| ) revert TransferFailed(); | ||
| if (!IZRC20(destination).approve(address(gateway), gasFee)) { | ||
| revert ApproveFailed(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize gas fee handling.
The current implementation makes separate calls for gas fee calculation and token approval. Consider combining these operations:
-(address gasZRC20, uint256 gasFee) = IZRC20(destination)
- .withdrawGasFeeWithGasLimit(gasLimitAmount);
-if (destination != gasZRC20) revert InvalidAddress();
-if (
- !IZRC20(destination).transferFrom(msg.sender, address(this), gasFee)
-) revert TransferFailed();
-if (!IZRC20(destination).approve(address(gateway), gasFee)) {
- revert ApproveFailed();
-}
+function _handleGasFee(address destination) internal returns (uint256) {
+ (address gasZRC20, uint256 gasFee) = IZRC20(destination)
+ .withdrawGasFeeWithGasLimit(gasLimitAmount);
+ if (destination != gasZRC20) revert InvalidAddress();
+
+ IZRC20 token = IZRC20(destination);
+ if (!token.transferFrom(msg.sender, address(this), gasFee))
+ revert TransferFailed();
+ if (!token.approve(address(gateway), gasFee))
+ revert ApproveFailed();
+
+ return gasFee;
+}Committable suggestion skipped: line range outside the PR's diff.
| ```solidity filename="contracts/Universal.sol" | ||
| // SPDX-License-Identifier: MIT | ||
| pragma solidity 0.8.26; | ||
|
|
||
| import "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol"; | ||
|
|
||
| contract Universal is UniversalNFT { | ||
| constructor( | ||
| address payable gatewayAddress, | ||
| address owner, | ||
| string memory name, | ||
| string memory symbol, | ||
| uint256 gasLimit, | ||
| address uniswapRouter | ||
| ) | ||
| UniversalNFT(gatewayAddress, gasLimit, uniswapRouter) | ||
| Ownable(owner) | ||
| ERC721(name, symbol) | ||
| {} | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add router validation and documentation.
The Universal contract should include:
- Documentation about the Uniswap router's role in gas token swaps
- Validation that the router address is a valid Uniswap V2 router
// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;
+import "@uniswap/v2-core/contracts/interfaces/IUniswapV2Factory.sol";
+import "@uniswap/v2-periphery/contracts/interfaces/IUniswapV2Router02.sol";
+/// @notice Universal NFT contract for cross-chain operations
+/// @dev Implements the Universal NFT standard on ZetaChain
contract Universal is UniversalNFT {
constructor(
address payable gatewayAddress,
address owner,
string memory name,
string memory symbol,
uint256 gasLimit,
address uniswapRouter
)
UniversalNFT(gatewayAddress, gasLimit, uniswapRouter)
Ownable(owner)
ERC721(name, symbol)
- {}
+ {
+ // Validate that the router is a valid Uniswap V2 router
+ try IUniswapV2Router02(uniswapRouter).factory() returns (address factory) {
+ require(factory != address(0), "Invalid Uniswap router");
+ } catch {
+ revert("Invalid Uniswap router");
+ }
+ }📝 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.
| ```solidity filename="contracts/Universal.sol" | |
| // SPDX-License-Identifier: MIT | |
| pragma solidity 0.8.26; | |
| import "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol"; | |
| contract Universal is UniversalNFT { | |
| constructor( | |
| address payable gatewayAddress, | |
| address owner, | |
| string memory name, | |
| string memory symbol, | |
| uint256 gasLimit, | |
| address uniswapRouter | |
| ) | |
| UniversalNFT(gatewayAddress, gasLimit, uniswapRouter) | |
| Ownable(owner) | |
| ERC721(name, symbol) | |
| {} | |
| } | |
| ``` | |
| // SPDX-License-Identifier: MIT | |
| pragma solidity 0.8.26; | |
| import "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol"; | |
| import "@uniswap/v2-core/contracts/interfaces/IUniswapV2Factory.sol"; | |
| import "@uniswap/v2-periphery/contracts/interfaces/IUniswapV2Router02.sol"; | |
| /// @notice Universal NFT contract for cross-chain operations | |
| /// @dev Implements the Universal NFT standard on ZetaChain | |
| contract Universal is UniversalNFT { | |
| constructor( | |
| address payable gatewayAddress, | |
| address owner, | |
| string memory name, | |
| string memory symbol, | |
| uint256 gasLimit, | |
| address uniswapRouter | |
| ) | |
| UniversalNFT(gatewayAddress, gasLimit, uniswapRouter) | |
| Ownable(owner) | |
| ERC721(name, symbol) | |
| { | |
| // Validate that the router is a valid Uniswap V2 router | |
| try IUniswapV2Router02(uniswapRouter).factory() returns (address factory) { | |
| require(factory != address(0), "Invalid Uniswap router"); | |
| } catch { | |
| revert("Invalid Uniswap router"); | |
| } | |
| } | |
| } |
Right now the Token page is pretty small, because Token works exactly the same way as NFT. Over time there will be more features that will make these standards different, for example, lock & mint and rate limiting mechanisms for Token.