feat: Enable XP NFT to be transferable #199
Conversation
|
Caution Review failedThe pull request is closed. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/zevm-app-contracts/test/xp-nft/xp-nft-v3.tsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: prettier.resolveConfig.sync is not a function 📝 WalkthroughWalkthroughThis pull request introduces modifications to the XP NFT contract series, focusing on enhancing the flexibility and functionality of token transfers and versioning. The changes span three contract files ( Changes
Sequence DiagramsequenceDiagram
participant Owner as NFT Owner
participant Contract as ZetaXP_V3
participant NewOwner as New Owner
Owner->>Contract: moveTagToToken(tokenId, tag)
Contract-->>Owner: Validate ownership
Owner->>Contract: Transfer NFT
Contract->>Contract: Update tag mappings
Contract->>Contract: Emit TagUpdated event
NewOwner->>Contract: Receive NFT with associated tag
Possibly Related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 0
🧹 Nitpick comments (2)
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V3.sol (1)
13-22: Solid tag-handling in the_transferoverride.
Your logic correctly clears the tag from the sender's map and assigns it to the recipient only if no existing token is mapped to that tag. The assumption that the tag doesn't collide for the new owner is sensible, but consider how you'd handle accidental collisions in the future.packages/zevm-app-contracts/test/xp-nft/xp-nft-v3.ts (1)
18-46: Setup and initialization are clear.
You effectively deploy the proxy, upgrade it, and preparesampleNFT. Provide more nuance in naming constants and variables if the code grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol(1 hunks)packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol(1 hunks)packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V3.sol(1 hunks)packages/zevm-app-contracts/test/xp-nft/xp-nft-v3.ts(1 hunks)
🔇 Additional comments (8)
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V3.sol (3)
7-7: Event usage looks appropriate.
Defining TagUpdated with indexed parameters is helpful for efficient event filtering.
9-11: Versioning strategy seems correct.
Overriding the version to "3.0.0" aligns with the new contract's functionality.
24-40: Robust moveTagToToken function.
The ownership check on line 27 is crucial. The function logically clears the old token tag and updates the new token's mapping. The early return on line 31 is lean and prevents redundant work.
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol (1)
23-23: Allowing further overrides of version().
Marking version() as virtual extends flexibility for new implementations.
packages/zevm-app-contracts/test/xp-nft/xp-nft-v3.ts (3)
1-16: Imports and constants are well-structured.
No apparent issues. All references to Hardhat, Chai, and ethers are standard best practices for a test suite.
48-74: Helper function design.
The mintNFT utility streamlines repeated mint logic. Ensuring you handle block timing, signature expiration, and robust validations is well done.
75-113: End-to-end tag updating test.
This test thoroughly verifies the minted token ownership, transfer, and subsequent tag reassociations. Great coverage for critical logic.
packages/zevm-app-contracts/contracts/xp-nft/xpNFT.sol (1)
156-156: Deferring transfer to subclasses is well-intentioned.
By overriding _transfer to revert, you ensure that only derived contracts can customize the transfer logic. This is critical given your new _transfer in V3.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Guillermo Maiolo <guille.maiolo@gmail.com>
Summary
Summary by CodeRabbit
New Features
Improvements
Tests