refactor: add template logic back to universal NFT/FT #249
Conversation
📝 WalkthroughWalkthroughThis pull request introduces new upgradeable NFT and token contracts. Two NFT contracts (EVMUniversalNFT and ZetaChainUniversalNFT) and two token contracts (EVMUniversalToken and ZetaChainUniversalToken) now extend multiple OpenZeppelin upgradeable contracts. Each contract features an initializer function to set immutable parameters; functions for minting, pausing/unpausing, and upgrade authorization; and overrides for core functionality to ensure compatibility with the extended libraries. Changes
Sequence Diagram(s)sequenceDiagram
participant Owner as Contract Owner
participant NFT as NFT Contract
Owner->>NFT: initialize(initialOwner, name, symbol, gatewayAddress, gas)
Owner->>NFT: safeMint(to, uri)
Owner->>NFT: pause() / unpause()
NFT-->>Owner: Confirmation & state update
sequenceDiagram
participant Owner as Contract Owner
participant Token as Token Contract
Owner->>Token: initialize(initialOwner, name, symbol, gatewayAddress, gas[, uniswapRouterAddress])
Owner->>Token: mint(to, amount)
Owner->>Token: pause() / unpause()
Token-->>Owner: Confirmation & state update
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
🪧 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: 1
🧹 Nitpick comments (12)
examples/token/contracts/EVMUniversalToken.sol (3)
14-22: Adopt a documented inheritance diagram or short remarks.
This multi-inheritance approach is perfectly valid. However, documenting each parent contract’s purpose (e.g.,ERC20Upgradeablefor standard token logic,OwnableUpgradeablefor restricted access, etc.) would help future maintainers quickly grasp your design.
43-49: Pause and unpause functionality is clear.
Leveraging the built-in pausing from OpenZeppelin is a solid approach. Make sure to confirm that external integrations handle paused states gracefully.
51-53: Uncapped token supply by design.
Be aware that allowing the owner to mint at will can lead to infinite supply. If this is intended, proceed. Otherwise, consider implementing a supply cap or additional logic.examples/nft/contracts/EVMUniversalNFT.sol (3)
2-11: Stay mindful of pinned Solidity and imported library versions.
Similar to the token contract, you're pinning compiler and library versions. This fosters consistency but should be revisited periodically to incorporate patches and enhancements.
27-27: Tracking_nextTokenIdis straightforward.
_nextTokenIdis used when generating token IDs via hashing. This is a valid approach, but ensure the ID creation logic meets your desired uniqueness requirements.
51-66: Secure minting process but deterministic ID approach.
Using a hash of(address(this), block.number, _nextTokenId++)provides semi-unique IDs, but it’s not truly unpredictable. Miners could potentially influence block properties if a specific ID is valuable. If full unpredictability is essential, consider VRF-based randomness.examples/token/contracts/ZetaChainUniversalToken.sol (3)
2-2: Solidity pinned to0.8.26.
As with the other contracts, revisit this periodically to pull in later compiler optimizations or security patches.
58-60: Minting logic.
Similar toEVMUniversalToken, ensure you intend for an unbounded supply. Otherwise, enforce a capacity check.
76-76:receive()fallback is helpful.
Accepting Ether directly can be beneficial for cross-chain bridging fees. Confirm that it’s used intentionally.examples/nft/contracts/ZetaChainUniversalNFT.sol (3)
52-67: Consider a purely incremental or externally verifiable token ID approach.
While hashing withaddress(this),block.number, and_nextTokenIdproduces a pseudo-unique ID, collisions remain theoretically possible. A deterministic counter-based ID or a more robust randomness mechanism (e.g., Chainlink VRF) might be preferable, depending on requirements.-uint256 hash = uint256( - keccak256( - abi.encodePacked(address(this), block.number, _nextTokenId++) - ) -); -uint256 tokenId = hash & 0x00FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; +uint256 tokenId = _nextTokenId++;
129-132: Pause function is clear and straightforward.
This ensures the contract can be halted for maintenance or risk mitigation. Be sure to include this in test coverage.
133-135: Unpause function mirrors pause functionality.
Having both pause and unpause ensures flexible contract lifecycle management. Similarly, remember to cover unpause logic in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/nft/contracts/EVMUniversalNFT.sol(1 hunks)examples/nft/contracts/ZetaChainUniversalNFT.sol(1 hunks)examples/token/contracts/EVMUniversalToken.sol(1 hunks)examples/token/contracts/ZetaChainUniversalToken.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: slither (examples/swap, swap.sarif)
- GitHub Check: slither (examples/call, call.sarif)
- GitHub Check: slither (examples/hello, hello.sarif)
- GitHub Check: test (examples/call)
🔇 Additional comments (34)
examples/token/contracts/EVMUniversalToken.sol (5)
23-26: Constructor pattern is correct for UUPS.
Disabling initializers in the constructor is a recommended safeguard against accidental re-initialization. No issues here.
28-41: Initialization parameters seem comprehensive.
Theinitialize()function covers naming, ownership, and gateway logic. This is a robust setup. Just ensure you re-run initializer tests if you modify the contract signature in the future.
55-57: Upgrade authorization logic is secure.
Restricting_authorizeUpgradetoonlyOwneris standard for UUPS-proxied contracts. No concerns here.
61-67: Override_updateensures compatibility.
This override pattern is typical when combiningERC20withPausable. Good job invokingsuper._updateto preserve parent functionality.
2-9:✅ Verification successful
Ensure continued alignment with future OpenZeppelin updates.
Pinning Solidity to0.8.26and importing specific OpenZeppelin versions helps maintain consistency, but keep an eye on potential security updates or improvements in newer releases. A periodic review to upgrade dependencies is advisable.You can run a quick check of newer compiler and OpenZeppelin versions if you'd like to confirm you are on the recommended stable release.
🏁 Script executed:
#!/bin/bash # Check for the latest stable Solidity release from GitHub curl -s https://api.github.com/repos/ethereum/solidity/releases | jq -r '.[0].tag_name' # Check for newer minor versions of OpenZeppelin rg "openzeppelin" -A 2Length of output: 24113
Action: Periodically Review and Validate Dependency Versions
After verification, the code inexamples/token/contracts/EVMUniversalToken.solremains consistent with the current pinned OpenZeppelin packages (v5.2.0 as seen in the yarn lock files). However, note that while Solidity is explicitly pinned to0.8.26, the latest stable release is now0.8.29. Although explicit version pinning helps ensure compatibility and reproducibility, you may want to:
- Consider Testing an Upgrade: Evaluate the impact of upgrading the Solidity version from
0.8.26to0.8.29to take advantage of the latest bug fixes and security improvements.- Monitor OpenZeppelin Updates: Keep an eye on future updates to OpenZeppelin contracts that might offer enhanced features or critical security patches.
- Maintain Consistency: Continue pinning versions to avoid unexpected changes unless thorough testing confirms that an upgrade is safe.
This periodic review strategy will help maintain a balanced approach between stability and benefiting from the latest improvements.
examples/nft/contracts/EVMUniversalNFT.sol (10)
13-14: UniversalNFTCore import is suitable.
Inheriting fromUniversalNFTCorecentralizes bridging and cross-chain features. This is a clean approach.
16-25: Multi-contract inheritance is well-structured.
Declaring upgradeable NFT functionalities (Enumerable, Burnable, Pausable) in one place simplifies reference for future audits.
29-32: Constructor properly disables initializers.
This pattern is consistent with UUPS best practices. No issues noted.
34-49: Initialization covers essential fields.
ThegatewayAddressandgasparameters are set alongside standard NFT initializers. This design is succinct.
68-74: Pause/unpause logic is standard.
As with your token contract, ensure external callers handle paused states robustly.
76-79: UUPS upgrade guard.
Restricting_authorizeUpgradetoonlyOwneris straightforward.
82-96: Override_updatefor multiple inheritance.
Good use ofsuper._updateto unify the logic fromERC721Upgradeable,ERC721EnumerableUpgradeable, andERC721PausableUpgradeable.
98-104: Consistent override of_increaseBalance.
Applying the enumerability approach here helps maintain the correct token balance count across multiple parent classes.
105-118: Robust token URI resolution.
Deferring to the parent implementations is typical and ensures uniform handling of token URIs.
120-134: MergedsupportsInterfacechecks.
Combining the parent interface IDs in one function is best practice. This ensures runtime consistency of all inherited interfaces.examples/token/contracts/ZetaChainUniversalToken.sol (7)
17-18: Import ofUniversalTokenCoreis appropriate.
This is consistent with the design of your other universal token contracts.
20-28: Multi-inheritance preserves key functionalities.
Composing multiple OpenZeppelin modules plusUniversalTokenCorehelps unify cross-chain token features with standard token capabilities.
30-32: Constructor logic correct.
Disabling initializers is crucial. No issues here.
34-48: Initialization approach mirrors EVMUniversalToken.
Inclusion of theuniswapRouterAddressparameter for cross-chain or cross-token swaps is a good extension. Ensure to test this thoroughly with your bridging/gateway logic.
50-56: Pausing and unpausing are standard.
Matches your other implementations, providing consistent admin control.
62-64: Owner-only upgrade guard.
Limits upgrade initiation to the contract owner, which is the canonical approach for UUPS upgrades.
68-74: Override_updatefor multi-parent synergy.
Maintains compatibility across all inherited functionalities, with no apparent conflicts.examples/nft/contracts/ZetaChainUniversalNFT.sol (12)
4-11: Use of upgradeable contract imports looks appropriate.
All selected imports are consistent with OpenZeppelin's upgradeable contract patterns. Ensure they are pinned to specific versions in your dependency files to avoid unexpected breakage on future releases.
13-15: UniversalNFTCore import is consistent with project goals.
The import statement indicates clear intent to layer universal NFT functionality on top of OpenZeppelin. No issues observed.
27-27: Simple private variable storage is valid.
_nextTokenIdsuits a straightforward minting flow. No immediate concerns.
29-32: Constructor disabling initializers is standard for upgradeable contracts.
This is a recommended best practice to prevent direct contract usage without initialization. Good job.
34-50: Initialization logic is comprehensive.
All relevant OpenZeppelin initializers are invoked, and universal core is properly set up. Be mindful of upgrading from one version to another and double-check that no re-initialization is required elsewhere.
69-85: Override of_updateis well-structured.
This override references multiple parents. Ensure the final call tosuper._updateis indeed capturing the combined logic fromERC721Upgradeable,ERC721EnumerableUpgradeable, andERC721PausableUpgradeablewithout unintended side effects.
87-92: Balance tracking override aligns with enumerability requirements.
The implemented override callssuper._increaseBalance, ensuring the enumerability and standard ERC721 logic remain consistent. No issues found.
94-107:tokenURIoverride merges multiple parents.
Relying onsuper.tokenURI(tokenId)addresses conflicts betweenERC721,ERC721URIStorage, andUniversalNFTCore. This looks correct; ensure metadata expansion or modifications happen consistently if you add any specialized logic.
109-123:supportsInterfaceoverride properly integrates multiple interfaces.
The combined overrides handle potential collisions from multiple inherited contracts. Good approach to maintain broad compatibility.
125-127: Owner-restricted_authorizeUpgradeis standard for UUPS.
Restricting upgrades to the contract owner is a common and appropriate security measure for UUPS proxies.
137-137:receive()function properly accepts ETH for gas payments.
No concerns. Make sure to handle possible reentrancy if any subsequent logic is introduced in future expansions.
16-26:❓ Verification inconclusive
Verify inheritance ordering for potential override conflicts.
Multiple parent contracts share hooks or internal functions (e.g.,_beforeTokenTransfer). Confirm that the final override resolutions—including those fromUniversalNFTCore—are correct and consistent with your design.
🏁 Script executed:
#!/bin/bash # Identify all definitions of _beforeTokenTransfer or similar hooks rg -A 5 "function _beforeTokenTransfer"Length of output: 41
Action: Validate Inheritance Override Order for Hook Functions
It appears that our automated search did not return any instances of
_beforeTokenTransferin the codebase, which makes it difficult to automatically confirm that the intended override chaining—especially involvingUniversalNFTCore—is correctly implemented. Please perform a manual review of the following:
- Verify that any custom override of
_beforeTokenTransferin the inherited contracts (e.g., in OpenZeppelin’s ERC721 modules andUniversalNFTCore) are properly chained with explicitoverridedeclarations.- Confirm that if
UniversalNFTCoreor any other module introduces additional logic for_beforeTokenTransfer, it calls the relevant parent implementations viasuper._beforeTokenTransfer.- Review the final inheritance ordering in
ZetaChainUniversalNFT.solto ensure that the combined logic from all parent contracts executes as designed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/token/contracts/ZetaChainUniversalToken.sol (1)
75-75: Consider documenting the purpose of the receive function.While the receive function allows the contract to accept Ether, it lacks documentation explaining its intended purpose. Consider adding a comment explaining when and why this function would be used in the context of this token.
- receive() external payable {} + /// @notice Allows the contract to receive Ether, necessary for gas token swaps and cross-chain operations + receive() external payable {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/token/contracts/ZetaChainUniversalToken.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (examples/token)
- GitHub Check: test (examples/swap)
- GitHub Check: test (examples/nft)
- GitHub Check: test (examples/call)
- GitHub Check: slither (examples/swap, swap.sarif)
- GitHub Check: slither (examples/call, call.sarif)
- GitHub Check: slither (examples/hello, hello.sarif)
🔇 Additional comments (8)
examples/token/contracts/ZetaChainUniversalToken.sol (8)
2-2: Appropriate use of fixed Solidity version.Using a fixed version (
0.8.26) rather than a floating version (^0.8.26) is preferable for production contracts as it ensures consistent behavior across builds and deployments.
4-17: Well-structured imports following a logical pattern.The imports are organized in a logical manner, starting with ZetaChain-specific dependencies followed by OpenZeppelin upgradeable components. This organization enhances code readability and maintainability.
19-27: Robust inheritance structure for upgradeable token.The contract now follows a comprehensive inheritance pattern that incorporates multiple OpenZeppelin upgradeable components alongside the ZetaChain-specific
UniversalTokenCore. This approach provides a solid foundation for an upgradeable token with pause functionality and owner controls.
28-31: Correctly implemented constructor pattern for upgradeable contracts.The constructor with
_disableInitializers()follows the recommended OpenZeppelin pattern for upgradeable contracts, preventing the initializer from being called during deployment.
33-47: Well-structured initializer with comprehensive parameter set.The initializer function properly initializes all parent contracts and captures all necessary parameters for token configuration. The use of descriptive parameter names and comments enhances code clarity.
49-59: Appropriate owner-restricted token management functions.The pause, unpause, and mint functions follow security best practices by restricting access to the contract owner. These administrative controls provide essential functionality for managing the token lifecycle.
61-63: Secure upgrade authorization mechanism.The
_authorizeUpgradefunction correctly restricts upgrade capabilities to the contract owner, aligning with security best practices for upgradeable contracts.
67-73: Properly implemented override for ERC20 and Pausable compatibility.The
_updateoverride correctly handles the interaction between ERC20 and Pausable functionality, ensuring that both aspects of the contract work harmoniously.
|
@zeta-chain/smart-contracts please, review. |
Universal NFT/FT example were basically one-liner imports from standard-contracts. Made it easier to keep them always up-to-date, but also kind of useless, because for any sort of changes, you'd have to copy and paste the underlying code.
Now, the NFT/FT example is a copy of:
https://github.com/zeta-chain/standard-contracts/blob/main/contracts/token/contracts/zetachain/UniversalToken.sol
In other words, when a new user runs
They will get a standard OpenZeppelin ERC-721 with ZetaChain features.
If we change the template, we'd have to manually keep these examples updated.
Summary by CodeRabbit
New NFT Contracts
New Token Contracts