Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NFT and FT Standards #509

Merged
merged 3 commits into from
Dec 5, 2024
Merged

NFT and FT Standards #509

merged 3 commits into from
Dec 5, 2024

Conversation

fadeev
Copy link
Member

@fadeev fadeev commented Nov 27, 2024

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.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a "standards" section providing insights into contract standards on ZetaChain.
    • Added detailed metadata for "Universal NFT" and "Universal Token" standards.
    • Launched comprehensive guides for implementing Universal NFT and Universal Token standards, including cross-chain transfer processes.
  • Documentation

    • Enhanced documentation for developers with practical instructions and implementation details for NFT and token standards.

@fadeev fadeev requested review from bbbeeeee, brewmaster012 and a team as code owners November 27, 2024 14:28
Copy link

vercel bot commented Nov 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs-v2 ✅ Ready (Inspect) Visit Preview Dec 5, 2024 3:23pm

Copy link
Contributor

coderabbitai bot commented Nov 27, 2024

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

The 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 _meta.json file, along with a new JSON file specifically for standards metadata. Two new markdown files provide detailed descriptions of the Universal NFT and Universal Token standards, outlining their functionalities, architecture, and implementation details for developers.

Changes

File Path Change Summary
src/pages/developers/_meta.json Added a new section "standards" with properties "title" ("Contract Standards") and "description" ("Learn about the different contract standards available on ZetaChain and how to use them.")
src/pages/developers/standards/_meta.json Introduced a new JSON file containing metadata for "nft" (title: "Universal NFT", description: "The Universal NFT standard enables non-fungible tokens (ERC-721 NFT) to be minted on any chain and seamlessly transferred between connected chains.") and "token" (title: "Universal Token", description: "The Universal Token standard enables ERC-20 fungible tokens to be minted on any chain and seamlessly transferred between connected chains.")
src/pages/developers/standards/nft.mdx Created a new file detailing the Universal NFT standard, including its architecture, transfer processes, error handling, and deployment instructions.
src/pages/developers/standards/token.mdx Created a new file outlining the Universal Token standard, including its functionality, contract details, and similarities to the NFT standard.

Possibly related PRs

  • Component library #391: This PR modifies src/pages/developers/_meta.json, which is directly related to the main PR's changes in the same file, enhancing the metadata structure and content.
  • Gateway: Interacting with Universal Apps #433: This PR also updates src/pages/developers/_meta.json by adding a new section titled "chains," which complements the main PR's focus on enhancing metadata related to standards in ZetaChain.
  • Hello and Echo Contracts #485: This PR introduces changes to the Hello contract, which implements the UniversalContract interface, relevant to the main PR's focus on contract standards and interoperability within ZetaChain.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8db0fcd and 7768a82.

📒 Files selected for processing (1)
  • src/pages/developers/standards/_meta.json (1 hunks)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Add NatSpec documentation for the constructor parameters
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 07bc9c5 and f2f31d9.

📒 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: ⚠️ Potential issue

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:

  1. Use a combination of chainId, timestamp, and nonce
  2. 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.

Comment on lines +1 to +10
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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
import "@zetachain/standard-contracts/nft/contracts/zetachain/UniversalNFT.sol";
import "@zetachain/standard-contracts/token/contracts/universal/UniversalToken.sol";

Comment on lines +38 to +51
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)
{}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Comment on lines +18 to +29
constructor(
address payable gatewayAddress,
address owner,
string memory name,
string memory symbol,
uint256 gasLimit
)
UniversalToken(gatewayAddress, gasLimit)
Ownable(owner)
ERC20(name, symbol)
{}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Comment on lines 1 to 52
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)
{}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

Comment on lines +491 to +499
(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();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Comment on lines +76 to +96
```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)
{}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add router validation and documentation.

The Universal contract should include:

  1. Documentation about the Uniswap router's role in gas token swaps
  2. 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.

Suggested change
```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");
}
}
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f2f31d9 and 8db0fcd.

📒 Files selected for processing (1)
  • src/pages/developers/standards/token.mdx (1 hunks)
🔇 Additional comments (4)
src/pages/developers/standards/token.mdx (4)

1-10: Documentation needs enhancement with essential implementation details.

The current documentation, while providing a basic overview, would benefit from additional sections covering implementation details.


38-51: 🛠️ Refactor suggestion

Document the purpose of uniswapRouter parameter.

The Universal contract introduces a uniswapRouter parameter without explaining its purpose or requirements in the documentation.

Add documentation above the contract:

+/// @title Universal Token Contract for ZetaChain
+/// @notice Implements cross-chain token functionality with Uniswap integration
+/// @dev Extends UniversalToken with liquidity provision capabilities
 contract Universal is UniversalToken {

Likely invalid or redundant comment.


36-36: ⚠️ Potential issue

Incorrect import path for UniversalToken.

The import statement references the NFT contract instead of the token contract.

-import "@zetachain/standard-contracts/token/contracts/zetachain/UniversalNFT.sol";
+import "@zetachain/standard-contracts/token/contracts/zetachain/UniversalToken.sol";

Likely invalid or redundant comment.


15-15: ⚠️ Potential issue

Incorrect import path for UniversalToken.

The import statement references the NFT contract instead of the token contract.

-import "@zetachain/standard-contracts/token/contracts/evm/UniversalNFT.sol";
+import "@zetachain/standard-contracts/token/contracts/evm/UniversalToken.sol";

Likely invalid or redundant comment.

Comment on lines +17 to +29
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)
{}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add parameter validation in constructors.

Both contracts should validate their input parameters to prevent potential issues.

Add these validations to both constructors:

     constructor(...) {
+        require(gatewayAddress != address(0), "Invalid gateway address");
+        require(owner != address(0), "Invalid owner address");
+        require(bytes(name).length > 0, "Invalid name");
+        require(bytes(symbol).length > 0, "Invalid symbol");
+        require(gasLimit > 0, "Invalid gas limit");
         // ... rest of the constructor
     }

For the Universal contract, also add:

     constructor(...) {
         // ... other validations
+        require(uniswapRouter != address(0), "Invalid uniswap router address");
         // ... rest of the constructor
     }

Also applies to: 38-51

Comment on lines +11 to +52
```solidity filename="contracts/Connected.sol"
// SPDX-License-Identifier: MIT
pragma solidity 0.8.26;

import "@zetachain/standard-contracts/token/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/token/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)
{}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Import paths in token.mdx are incorrect

The code snippets in src/pages/developers/standards/token.mdx contain incorrect import paths:

  • Both contracts import UniversalNFT.sol but should import UniversalToken.sol since they inherit from UniversalToken
  • The imports reference the NFT contracts instead of token contracts

Locations requiring attention:

  • src/pages/developers/standards/token.mdx:
    // Incorrect
    import "@zetachain/standard-contracts/token/contracts/evm/UniversalNFT.sol";
    // Should be
    import "@zetachain/standard-contracts/token/contracts/evm/UniversalToken.sol";
    // Incorrect
    import "@zetachain/standard-contracts/token/contracts/zetachain/UniversalNFT.sol";
    // Should be
    import "@zetachain/standard-contracts/token/contracts/zetachain/UniversalToken.sol";
🔗 Analysis chain

Verify contract usage and import consistency.

Let's verify the usage of these contracts and import paths across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for contract usage
echo "Checking contract usage..."
rg -A 2 "contract.*is.*(?:Connected|Universal)"

# Verify import paths
echo "Checking import paths..."
rg "@zetachain/standard-contracts/(token|nft)/contracts/(evm|zetachain)/Universal(Token|NFT).sol"

# Check for potential inheritance chains
echo "Checking inheritance..."
ast-grep --pattern 'contract $_ is UniversalToken {
  $$$
}'

Length of output: 4429

Copy link
Member

@hernan-clich hernan-clich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK

{
"nft": {
"title": "Universal NFT",
"description": "Universal NFT."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we elaborate on what a 'Universal NFT' means? Users will most likely know what an NFT is, but what makes a Universal NFT different?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this piece from /developers/standards/nft works better as a brief description in the context of a card:

The Universal NFT standard enables non-fungible tokens (ERC-721 NFT) to be minted on any chain and seamlessly transferred between connected chains.

},
"token": {
"title": "Universal Token",
"description": "Universal fungible tokens."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same could be said in this case, this probably works better as a brief description for the card:

The Universal Token standard enables ERC-20 fungible tokens to be minted on any chain and seamlessly transferred between connected chains.

@fadeev fadeev merged commit a79ef4d into main Dec 5, 2024
3 of 4 checks passed
@fadeev fadeev deleted the standrds-nft-ft branch December 5, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants