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.

Sign up for GitHub

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

feat: change the nft metadata #192

Merged
merged 3 commits into from
Oct 17, 2024
Merged

feat: change the nft metadata #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13d64bb
feat: Change the NFT metadata
andresaiello Oct 4, 2024
41c3bab
add upgrade script
andresaiello Oct 4, 2024
61adba0
fix deploy script
andresaiello Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/zevm-app-contracts/contracts/xp-nft/test/xpNFTV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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:

  1. The contract name ZetaXPV2 suggests a major version change, which conflicts with the "1.0.1" version number.
  2. If this is indeed a new version with additional features, the version number should be increased, not decreased.
  3. 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.

}
}
50 changes: 50 additions & 0 deletions packages/zevm-app-contracts/contracts/xp-nft/xpNFT_V2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import "./xpNFT.sol";

contract ZetaXP_V2 is ZetaXP {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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

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();
}
Comment on lines +25 to +37

Check notice

Code scanning / Slither

Block timestamp Low

ZetaXP_V2._verifySetLevelSignature(ZetaXP_V2.SetLevelData) uses timestamp for comparisons
Dangerous comparisons:
- block.timestamp > data.signatureExpiration

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];
}
}
Comment on lines +6 to +50

Check warning

Code scanning / Slither

Conformance to Solidity naming conventions Warning

Contract ZetaXP_V2 is not in CapWords
35 changes: 35 additions & 0 deletions packages/zevm-app-contracts/scripts/xp-nft/upgrade-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { isProtocolNetworkName } from "@zetachain/protocol-contracts";
import { ethers, network, upgrades } from "hardhat";

import addresses from "../../data/addresses.json";
import { saveAddress } from "../address.helpers";
import { verifyContract } from "../explorer.helpers";

const networkName = network.name;

const upgradeZetaXP = async () => {
if (!isProtocolNetworkName(networkName)) throw new Error("Invalid network name");

//@ts-ignore
const nftAddress = addresses["zevm"][networkName].ZetaXP;
Comment on lines +13 to +14
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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;

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate nftAddress before proceeding

There's a risk of nftAddress being undefined if the address is missing for the given network, which could lead to runtime errors.

Add a check to ensure nftAddress is defined:

const nftAddress = addresses["zevm"][networkName].ZetaXP;
+ if (!nftAddress) {
+   throw new Error(`NFT address not found for network: ${networkName}`);
+ }
📝 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
const nftAddress = addresses["zevm"][networkName].ZetaXP;
const nftAddress = addresses["zevm"][networkName].ZetaXP;
if (!nftAddress) {
throw new Error(`NFT address not found for network: ${networkName}`);
}


const ZetaXPFactory = await ethers.getContractFactory("ZetaXP_V2");
const zetaXP = await upgrades.upgradeProxy(nftAddress, ZetaXPFactory);
const implementationAddress = await upgrades.erc1967.getImplementationAddress(zetaXP.address);
Comment on lines +16 to +18
Copy link

Choose a reason for hiding this comment

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

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

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


console.log("ZetaXP upgraded in:", zetaXP.address);
console.log("ZetaXP implementation deployed to:", implementationAddress);

saveAddress("ZetaXP", zetaXP.address, networkName);

await verifyContract(implementationAddress, []);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle verification failures gracefully

If contract verification fails, the script should handle it to prevent silent failures.

Consider wrapping the verification in a try-catch block:

+ try {
  await verifyContract(implementationAddress, []);
+ } catch (error) {
+   console.error("Contract verification failed:", error);
+   // Decide whether to exit or continue based on the importance
+ }
📝 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
await verifyContract(implementationAddress, []);
try {
await verifyContract(implementationAddress, []);
} catch (error) {
console.error("Contract verification failed:", error);
// Decide whether to exit or continue based on the importance
}

};

const main = async () => {
await upgradeZetaXP();
};

main().catch((error) => {
console.error(error);
process.exit(1);
});
Comment on lines +32 to +35
Copy link

Choose a reason for hiding this comment

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

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

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

36 changes: 36 additions & 0 deletions packages/zevm-app-contracts/test/xp-nft/test.helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,39 @@ export const getSignature = async (
const signature = await signer._signTypedData(domain, types, value);
return signature;
};

export const getSelLevelSignature = async (
chainId: number,
verifyingContract: string,
signer: SignerWithAddress,
signatureExpiration: number,
timestamp: number,
tokenId: number,
level: number
) => {
const domain = {
chainId: chainId,
name: "ZetaXP",
verifyingContract: verifyingContract,
version: "1",
};

const types = {
SetLevel: [
{ name: "tokenId", type: "uint256" },
{ name: "signatureExpiration", type: "uint256" },
{ name: "sigTimestamp", type: "uint256" },
{ name: "level", type: "uint256" },
],
};

const value = {
level,
sigTimestamp: timestamp,
signatureExpiration,
tokenId,
};
// Signing the data
const signature = await signer._signTypedData(domain, types, value);
return signature;
};
Loading
Loading