feat: change the nft metadata #192
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files related to the 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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| contract ZetaXP_V2 is ZetaXP { | ||
| bytes32 private constant SETLEVEL_TYPEHASH = | ||
| keccak256("SetLevel(uint256 tokenId,uint256 signatureExpiration,uint256 sigTimestamp,uint256 level)"); | ||
|
|
||
| struct SetLevelData { | ||
| uint256 tokenId; | ||
| bytes signature; | ||
| uint256 signatureExpiration; | ||
| uint256 sigTimestamp; | ||
| uint256 level; | ||
| } | ||
|
|
||
| mapping(uint256 => uint256) public levelByTokenId; | ||
| event LevelSet(address indexed sender, uint256 indexed tokenId, uint256 level); | ||
|
|
||
| function version() public pure override returns (string memory) { | ||
| return "2.0.0"; | ||
| } | ||
|
|
||
| function _verifySetLevelSignature(SetLevelData memory data) private view { | ||
| bytes32 structHash = keccak256( | ||
| abi.encode(SETLEVEL_TYPEHASH, data.tokenId, data.signatureExpiration, data.sigTimestamp, data.level) | ||
| ); | ||
| bytes32 constructedHash = _hashTypedDataV4(structHash); | ||
|
|
||
| if (!SignatureChecker.isValidSignatureNow(signerAddress, constructedHash, data.signature)) { | ||
| revert InvalidSigner(); | ||
| } | ||
|
|
||
| if (block.timestamp > data.signatureExpiration) revert SignatureExpired(); | ||
| if (data.sigTimestamp <= lastUpdateTimestampByTokenId[data.tokenId]) revert OutdatedSignature(); | ||
| } | ||
|
|
||
| function setLevel(SetLevelData memory data) external { | ||
| _verifySetLevelSignature(data); | ||
|
|
||
| levelByTokenId[data.tokenId] = data.level; | ||
| lastUpdateTimestampByTokenId[data.tokenId] = data.sigTimestamp; | ||
| emit LevelSet(msg.sender, data.tokenId, data.level); | ||
| } | ||
|
|
||
| function getLevel(uint256 tokenId) external view returns (uint256) { | ||
| return levelByTokenId[tokenId]; | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Conformance to Solidity naming conventions Warning
| function _verifySetLevelSignature(SetLevelData memory data) private view { | ||
| bytes32 structHash = keccak256( | ||
| abi.encode(SETLEVEL_TYPEHASH, data.tokenId, data.signatureExpiration, data.sigTimestamp, data.level) | ||
| ); | ||
| bytes32 constructedHash = _hashTypedDataV4(structHash); | ||
|
|
||
| if (!SignatureChecker.isValidSignatureNow(signerAddress, constructedHash, data.signature)) { | ||
| revert InvalidSigner(); | ||
| } | ||
|
|
||
| if (block.timestamp > data.signatureExpiration) revert SignatureExpired(); | ||
| if (data.sigTimestamp <= lastUpdateTimestampByTokenId[data.tokenId]) revert OutdatedSignature(); | ||
| } |
Check notice
Code scanning / Slither
Block timestamp Low
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (13)
packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.sol (1)
Discrepancy Confirmed: PR does not implement on-chain data support in
xpNFTV2.sol.
- Only the version number was updated in
packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.sol.🔗 Analysis chain
Line range hint
1-10: Discrepancy between PR objectives and implemented changes.The PR objectives mention updating the NFT to support on-chain data. However, the only change in this file is a version number update. There appears to be a significant discrepancy between the stated objectives and the actual implementation.
To verify if on-chain data support has been added elsewhere in the codebase, please run the following script:
If the script doesn't return any relevant results, consider updating the PR description to accurately reflect the changes made, or implement the on-chain data support as originally intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for new on-chain data related functionality in the codebase # Test 1: Search for new functions or state variables related to on-chain data echo "Searching for new on-chain data related functionality:" rg --type solidity -i '(function|mapping|struct).*(?i)(onchain|chain|data)' --ignore-file packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.sol # Test 2: Search for modifications to existing functions that might indicate on-chain data support echo "Searching for modifications to existing functions:" rg --type solidity -i '(function).*\{' --ignore-file packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.solLength of output: 1414
Script:
#!/bin/bash # Description: Search for new on-chain data related functionality in the codebase # Test 1: Search for new functions or state variables related to on-chain data echo "Searching for new on-chain data related functionality:" rg --glob '*.sol' -i '(function|mapping|struct).*(?i)(onchain|chain|data)' # Test 2: Search for modifications to existing functions that might indicate on-chain data support echo "Searching for modifications to existing functions:" rg --glob '*.sol' -i 'function\s+\w+\s*\(.*\)\s*\{'Length of output: 13858
packages/zevm-app-contracts/test/xp-nft/test.helpers.ts (1)
51-85: Approve implementation with minor suggestions for improvement.The
getSelLevelSignaturefunction is well-implemented and correctly uses EIP-712 typed data signing. It aligns with the existinggetSignaturefunction in structure and methodology. However, consider the following enhancements:
- Extract the version string to a constant for consistency:
const ZETA_XP_VERSION = "1";
- Add JSDoc comments for improved documentation:
/** * Generates a signature for setting the level of a specific NFT. * @param chainId - The ID of the blockchain network * @param verifyingContract - The address of the contract that will verify the signature * @param signer - The signer's address * @param signatureExpiration - The expiration timestamp for the signature * @param timestamp - The current timestamp * @param tokenId - The ID of the NFT * @param level - The new level to set for the NFT * @returns A promise that resolves to the generated signature */ export const getSelLevelSignature = async ( // ... (parameters) ) => { // ... (function body) };These changes will enhance code maintainability and readability.
packages/zevm-app-contracts/test/xp-nft/xp-nft-v2.ts (1)
18-46: Test suite setup is comprehensive and well-structured.The setup for the test suite is thorough, including contract deployment, upgrading, and preparation of sample data. The use of
beforeEachensures a clean state for each test, which is a best practice.To improve readability, consider extracting the contract deployment and upgrading logic into a separate function:
async function deployAndUpgradeContract() { const zetaXPFactory = await ethers.getContractFactory("ZetaXP"); const zetaXP = await upgrades.deployProxy(zetaXPFactory, [ "ZETA NFT", "ZNFT", ZETA_BASE_URL, signer.address, signer.address, ]); await zetaXP.deployed(); const ZetaXPFactory = await ethers.getContractFactory("ZetaXP_V2"); return await upgrades.upgradeProxy(zetaXP.address, ZetaXPFactory); } // In beforeEach: zetaXP = await deployAndUpgradeContract();This change would make the
beforeEachhook more concise and easier to understand.packages/zevm-app-contracts/scripts/xp-nft/upgrade-v2.ts (2)
11-11: Clarify error message for invalid networkThe current error message is generic. Providing the invalid network name can aid in debugging.
Consider updating the error message:
if (!isProtocolNetworkName(networkName)) - throw new Error("Invalid network name"); + throw new Error(`Invalid network name: ${networkName}`);
20-21: Use template literals for consistent loggingUsing template literals enhances readability and consistency in log messages.
Modify the console logs:
- console.log("ZetaXP upgraded in:", zetaXP.address); - console.log("ZetaXP implementation deployed to:", implementationAddress); + console.log(`ZetaXP upgraded at: ${zetaXP.address}`); + console.log(`ZetaXP implementation deployed to: ${implementationAddress}`);packages/zevm-app-contracts/test/xp-nft/xp-nft-v2-back.ts (8)
13-13: Correct 'Back compatibility' to 'Backward compatibility' in test descriptionThe test description should be updated to "XP NFT V2 Contract Backward compatibility test" to use the standard terminology.
294-294: Correct test description grammarUpdate the test description to "Should revert if non-owner wants to update base URI" for clarity and correct grammar.
299-299: Improve test description for clarityUpdate the test description to "Should revert if transfer is attempted" for better clarity.
328-328: Improve test description for clarityUpdate the test description to "Should revert if the same signature is used twice" for clarity.
358-358: Correct test description grammarUpdate the test description to "Should revert if user already has the tag" for grammatical correctness.
461-461: Correct typo in test descriptionUpdate the test description to "Should revert if signature expired" to fix the typo.
550-550: Clarify test descriptionUpdate the test description to "Should accept updating NFT with the same tag" for improved clarity.
609-609: Improve test description for clarityUpdate the test description to "Should revert if trying to update NFT with an already used tag" for better clarity and understanding.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.sol (1 hunks)
- packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol (1 hunks)
- packages/zevm-app-contracts/scripts/xp-nft/upgrade-v2.ts (1 hunks)
- packages/zevm-app-contracts/test/xp-nft/test.helpers.ts (1 hunks)
- packages/zevm-app-contracts/test/xp-nft/xp-nft-v2-back.ts (1 hunks)
- packages/zevm-app-contracts/test/xp-nft/xp-nft-v2.ts (1 hunks)
- packages/zevm-app-contracts/test/xp-nft/xp-nft.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol
[warning] 6-50: Conformance to Solidity naming conventions
Contract ZetaXP_V2 (contracts/xp-nft/xpNFT_V2.sol#6-50) is not in CapWords
[notice] 25-37: Block timestamp
ZetaXP_V2._verifySetLevelSignature(ZetaXP_V2.SetLevelData) (contracts/xp-nft/xpNFT_V2.sol#25-37) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > data.signatureExpiration (contracts/xp-nft/xpNFT_V2.sol#35)
🔇 Additional comments (6)
packages/zevm-app-contracts/test/xp-nft/xp-nft-v2.ts (2)
1-9: Imports and constants are well-structured.The necessary modules and types are correctly imported, and the constants are appropriately defined. This segment adheres to best practices in terms of organization and naming conventions.
1-119: Summary of review for xp-nft-v2.tsThe test suite for the XP NFT V2 contract is well-structured and covers key functionalities. The suggested improvements focus on:
- Enhancing type safety in utility functions.
- Improving readability of the test setup.
- Reducing code duplication in test cases.
Implementing these changes will result in a more robust, maintainable, and efficient test suite. Overall, the file provides a solid foundation for ensuring the correct behavior of the XP NFT V2 contract.
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol (4)
7-9: Correct implementation of EIP-712 type hashThe
SETLEVEL_TYPEHASHconstant and its usage in the signature verification process are correctly implemented according to EIP-712 standards. This ensures secure and standardized message hashing for signature verification.
25-37: Signature verification logic is properly implementedThe
_verifySetLevelSignaturefunction correctly implements EIP-712 typed data hashing and utilizesSignatureChecker.isValidSignatureNowfor signature verification. It includes essential checks for signature expiration and ensures that the providedsigTimestampis newer than the last update, effectively preventing replay attacks.🧰 Tools
🪛 GitHub Check: Slither
[notice] 25-37: Block timestamp
ZetaXP_V2._verifySetLevelSignature(ZetaXP_V2.SetLevelData) (contracts/xp-nft/xpNFT_V2.sol#25-37) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > data.signatureExpiration (contracts/xp-nft/xpNFT_V2.sol#35)
39-45: Securely updates token levels with proper validationThe
setLevelfunction appropriately calls_verifySetLevelSignatureto validate the signature before updating thelevelByTokenIdmapping and emitting theLevelSetevent. This ensures that only authorized updates occur, maintaining the integrity of the token levels.
47-49: Efficient retrieval of token levelsThe
getLevelfunction provides a straightforward and efficient way to retrieve the level associated with a giventokenId.
| @@ -5,6 +5,6 @@ import "../xpNFT.sol"; | |||
|
|
|||
| contract ZetaXPV2 is ZetaXP { | |||
| function version() public pure override returns (string memory) { | |||
| return "2.0.0"; | |||
| return "1.0.1"; | |||
There was a problem hiding this comment.
Inconsistent version numbering requires attention.
The version number has been changed from "2.0.0" to "1.0.1", which is contrary to conventional semantic versioning practices. This change appears to be a regression in version number, which could lead to confusion in dependency management and contract upgrades.
Consider the following points:
- The contract name
ZetaXPV2suggests a major version change, which conflicts with the "1.0.1" version number. - If this is indeed a new version with additional features, the version number should be increased, not decreased.
- If this is a minor update or patch, consider renaming the contract to reflect the minor change.
Proposed solution:
- return "1.0.1";
+ return "2.0.0";Alternatively, if this is indeed a minor update to version 1:
- contract ZetaXPV2 is ZetaXP {
+ contract ZetaXPV1_0_1 is ZetaXP {Committable suggestion was skipped due to low confidence.
| const encodeTag = (tag: string) => ethers.utils.keccak256(ethers.utils.defaultAbiCoder.encode(["string"], [tag])); | ||
|
|
||
| const getTokenIdFromRecipient = (receipt: any): number => { | ||
| //@ts-ignore | ||
| return receipt.events[0].args?.tokenId; | ||
| }; |
There was a problem hiding this comment.
Consider using a type-safe approach for getTokenIdFromRecipient.
The @ts-ignore comment in getTokenIdFromRecipient suggests a potential type-related issue. To improve type safety and maintainability, consider defining a more specific type for the receipt parameter.
Here's a suggested improvement:
interface TransactionReceipt {
events?: Array<{
args?: {
tokenId?: ethers.BigNumber;
};
}>;
}
const getTokenIdFromRecipient = (receipt: TransactionReceipt): number => {
const tokenId = receipt.events?.[0]?.args?.tokenId;
if (!tokenId) {
throw new Error("TokenId not found in transaction receipt");
}
return tokenId.toNumber();
};This approach provides better type safety and error handling.
| const mintNFT = async (nft: NFT) => { | ||
| const currentBlock = await ethers.provider.getBlock("latest"); | ||
| const sigTimestamp = currentBlock.timestamp; | ||
| const signatureExpiration = sigTimestamp + 1000; | ||
|
|
||
| const signature = await getSignature( | ||
| HARDHAT_CHAIN_ID, | ||
| zetaXP.address, | ||
| signer, | ||
| signatureExpiration, | ||
| sigTimestamp, | ||
| nft.to, | ||
| nft | ||
| ); | ||
|
|
||
| const nftParams: NFTSigned = { | ||
| ...sampleNFT, | ||
| sigTimestamp, | ||
| signature, | ||
| signatureExpiration, | ||
| } as NFTSigned; | ||
|
|
||
| const tx = await zetaXP.mintNFT(nftParams); | ||
| const receipt = await tx.wait(); | ||
| return getTokenIdFromRecipient(receipt); | ||
| }; | ||
|
|
||
| it("Should update NFT level", async () => { | ||
| const tokenId = await mintNFT(sampleNFT); | ||
|
|
||
| const currentBlock = await ethers.provider.getBlock("latest"); | ||
| const sigTimestamp = currentBlock.timestamp; | ||
| const signatureExpiration = sigTimestamp + 1000; | ||
| const level = 3; | ||
|
|
||
| const signature = await getSelLevelSignature( | ||
| HARDHAT_CHAIN_ID, | ||
| zetaXP.address, | ||
| signer, | ||
| signatureExpiration, | ||
| sigTimestamp, | ||
| tokenId, | ||
| level | ||
| ); | ||
|
|
||
| await zetaXP.setLevel({ level, sigTimestamp, signature, signatureExpiration, tokenId }); | ||
| const onchainLevel = await zetaXP.getLevel(tokenId); | ||
| expect(onchainLevel).to.be.eq(level); | ||
| }); | ||
|
|
||
| it("Should emit event on set level", async () => { | ||
| const tokenId = await mintNFT(sampleNFT); | ||
|
|
||
| const currentBlock = await ethers.provider.getBlock("latest"); | ||
| const sigTimestamp = currentBlock.timestamp; | ||
| const signatureExpiration = sigTimestamp + 1000; | ||
| const level = 3; | ||
|
|
||
| const signature = await getSelLevelSignature( | ||
| HARDHAT_CHAIN_ID, | ||
| zetaXP.address, | ||
| signer, | ||
| signatureExpiration, | ||
| sigTimestamp, | ||
| tokenId, | ||
| level | ||
| ); | ||
|
|
||
| const tx = zetaXP.setLevel({ level, sigTimestamp, signature, signatureExpiration, tokenId }); | ||
| await expect(tx).to.emit(zetaXP, "LevelSet").withArgs(signer.address, tokenId, level); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring common test setup logic.
The test cases effectively cover important contract functionality. However, there's noticeable code duplication in the setup for each test, which could be refactored to improve maintainability.
Consider extracting the common setup logic into a helper function:
async function setupLevelUpdate(tokenId: number, level: number) {
const currentBlock = await ethers.provider.getBlock("latest");
const sigTimestamp = currentBlock.timestamp;
const signatureExpiration = sigTimestamp + 1000;
const signature = await getSelLevelSignature(
HARDHAT_CHAIN_ID,
zetaXP.address,
signer,
signatureExpiration,
sigTimestamp,
tokenId,
level
);
return { level, sigTimestamp, signature, signatureExpiration, tokenId };
}
// In your tests:
it("Should update NFT level", async () => {
const tokenId = await mintNFT(sampleNFT);
const updateParams = await setupLevelUpdate(tokenId, 3);
await zetaXP.setLevel(updateParams);
const onchainLevel = await zetaXP.getLevel(tokenId);
expect(onchainLevel).to.be.eq(updateParams.level);
});
it("Should emit event on set level", async () => {
const tokenId = await mintNFT(sampleNFT);
const updateParams = await setupLevelUpdate(tokenId, 3);
const tx = zetaXP.setLevel(updateParams);
await expect(tx).to.emit(zetaXP, "LevelSet").withArgs(signer.address, tokenId, updateParams.level);
});This refactoring reduces code duplication and improves the overall maintainability of the test suite.
| //@ts-ignore | ||
| const nftAddress = addresses["zevm"][networkName].ZetaXP; |
There was a problem hiding this comment.
Avoid using @ts-ignore; enhance type safety
Suppressing TypeScript errors with @ts-ignore can hide potential issues. It's better to address the root cause by properly typing addresses.
Define an appropriate interface for addresses or cast it with a known type. For example:
- //@ts-ignore
const nftAddress = addresses["zevm"][networkName].ZetaXP;
+ const nftAddress = (addresses as { [key: string]: any })["zevm"][networkName].ZetaXP;Or, better yet, define a detailed type for addresses:
interface Addresses {
zevm: {
[networkName: string]: {
ZetaXP: string;
};
};
}
const addressesData = addresses as Addresses;
const nftAddress = addressesData.zevm[networkName].ZetaXP;| main().catch((error) => { | ||
| console.error(error); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in main execution
Currently, any error causes the script to exit without specifying the context. Providing more details aids in troubleshooting.
Update the error log to include contextual information:
main().catch((error) => {
- console.error(error);
+ console.error("An unexpected error occurred during the upgrade script execution:", error);
process.exit(1);
});📝 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.
| main().catch((error) => { | |
| console.error(error); | |
| process.exit(1); | |
| }); | |
| main().catch((error) => { | |
| console.error("An unexpected error occurred during the upgrade script execution:", error); | |
| process.exit(1); | |
| }); |
| const ZetaXPFactory = await ethers.getContractFactory("ZetaXP_V2"); | ||
| const zetaXP = await upgrades.upgradeProxy(nftAddress, ZetaXPFactory); | ||
| const implementationAddress = await upgrades.erc1967.getImplementationAddress(zetaXP.address); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the upgrade process
The upgrade operation may fail due to various reasons (e.g., network issues, incorrect contract code). Implementing error handling will make the script more robust.
Wrap the upgrade steps in a try-catch block:
+ try {
const ZetaXPFactory = await ethers.getContractFactory("ZetaXP_V2");
const zetaXP = await upgrades.upgradeProxy(nftAddress, ZetaXPFactory);
const implementationAddress = await upgrades.erc1967.getImplementationAddress(zetaXP.address);
+ } catch (error) {
+ console.error("Upgrade failed:", error);
+ process.exit(1);
+ }📝 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 ZetaXPFactory = await ethers.getContractFactory("ZetaXP_V2"); | |
| const zetaXP = await upgrades.upgradeProxy(nftAddress, ZetaXPFactory); | |
| const implementationAddress = await upgrades.erc1967.getImplementationAddress(zetaXP.address); | |
| try { | |
| const ZetaXPFactory = await ethers.getContractFactory("ZetaXP_V2"); | |
| const zetaXP = await upgrades.upgradeProxy(nftAddress, ZetaXPFactory); | |
| const implementationAddress = await upgrades.erc1967.getImplementationAddress(zetaXP.address); | |
| } catch (error) { | |
| console.error("Upgrade failed:", error); | |
| process.exit(1); | |
| } |
|
|
||
| import "./xpNFT.sol"; | ||
|
|
||
| contract ZetaXP_V2 is ZetaXP { |
There was a problem hiding this comment.
Rename the contract to adhere to Solidity naming conventions
The contract name ZetaXP_V2 includes an underscore, which does not align with Solidity's naming conventions. It's recommended to use CapWords (PascalCase) without underscores for contract names. Consider renaming the contract to ZetaXPV2 for consistency and clarity.
Apply this diff to rename the contract:
-contract ZetaXP_V2 is ZetaXP {
+contract ZetaXPV2 is ZetaXP {📝 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.
| contract ZetaXP_V2 is ZetaXP { | |
| contract ZetaXPV2 is ZetaXP { |
🧰 Tools
🪛 GitHub Check: Slither
[warning] 6-50: Conformance to Solidity naming conventions
Contract ZetaXP_V2 (contracts/xp-nft/xpNFT_V2.sol#6-50) is not in CapWords
| @@ -359,7 +359,7 @@ describe("XP NFT Contract test", () => { | |||
| const zetaXPV2 = await upgrades.upgradeProxy(zetaXP.address, ZetaXPFactory); | |||
|
|
|||
| const version2 = await zetaXPV2.version(); | |||
| await expect(version2).to.be.eq("2.0.0"); | |||
| await expect(version2).to.be.eq("1.0.1"); | |||
There was a problem hiding this comment.
💡 Codebase verification
Version Number Inconsistencies Detected
The version number "2.0.0" was found in the following location and needs to be updated to "1.0.1":
packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol: return "2.0.0";
🔗 Analysis chain
Ensure Version Number Consistency
The expected version after upgrading the contract has been changed from "2.0.0" to "1.0.1". Please verify that this change aligns with your versioning strategy and that all references to the version number are updated consistently across the codebase.
Here is a script to check for any remaining references to "2.0.0":
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for occurrences of the version number "2.0.0" in the codebase.
# Expected result: No occurrences of "2.0.0" should be found; all should be updated to "1.0.1".
# Command to search for "2.0.0" in the codebase:
rg '2\.0\.0'
Length of output: 8627
| const signature = await getSignature( | ||
| HARDHAT_CHAIN_ID, | ||
| zetaXP.address, | ||
| signer, | ||
| signatureExpiration, | ||
| sigTimestamp, | ||
| sampleNFT.to, | ||
| sampleNFT | ||
| ); | ||
|
|
||
| const nftParams: NFTSigned = { | ||
| ...sampleNFT, | ||
| sigTimestamp, | ||
| signature, | ||
| signatureExpiration, | ||
| } as NFTSigned; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated code for signature generation and NFT parameters
The code for generating signatures and constructing nftParams is repeated across multiple test cases. Consider refactoring this logic into helper functions to improve code maintainability and reduce duplication.
| const tokenURI2 = await zetaXP.tokenURI(1); | ||
| await expect(tokenURI2).to.be.eq(`${ZETA_BASE_URL}v2/1`); | ||
| }); |
There was a problem hiding this comment.
Fix token ID in token URI retrieval
In line 290, you are retrieving tokenURI2 for token ID 1 instead of 2. Update the token ID to 2 to correctly verify the token URI for the second minted token.
Apply this diff to fix the issue:
const tokenURI2 = await zetaXP.tokenURI(
- 1
+ 2
);
await expect(tokenURI2).to.be.eq(`${ZETA_BASE_URL}v2/2`);📝 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 tokenURI2 = await zetaXP.tokenURI(1); | |
| await expect(tokenURI2).to.be.eq(`${ZETA_BASE_URL}v2/1`); | |
| }); | |
| const tokenURI2 = await zetaXP.tokenURI(2); | |
| await expect(tokenURI2).to.be.eq(`${ZETA_BASE_URL}v2/2`); | |
| }); |
Summary
Summary by CodeRabbit
New Features
ZetaXP_V2contract with enhanced level management capabilities.LevelSetfor tracking level changes.Bug Fixes
Tests