feat: Improve XP Gov #194
feat: Improve XP Gov #194
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (3)
packages/zevm-app-contracts/scripts/xp-nft/gov-query-proposals.ts (1)
1-38: Consider architectural improvements for maintainability.
The script would benefit from:
- Separation into reusable utilities
- Configuration management
- TypeScript interfaces for governance types
Consider creating:
- A shared utilities module for governance queries
- A configuration file for network-specific parameters
- Type definitions for governance entities
packages/zevm-app-contracts/scripts/xp-nft/deploy-gov.ts (1)
62-65: Enhance error logging for better debugging
In the main function's error handler, providing additional context can aid in debugging deployment issues more effectively.
Apply this diff to improve error messages:
main().catch((error) => {
- console.error(error);
+ console.error('Error during deployment:', error);
process.exit(1);
});packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (1)
Line range hint 45-52: Handle NFT levels beyond 3 explicitly in _getVotes function
Currently, the _getVotes function returns a voting weight of 0 for any NFT levels other than 1, 2, or 3 due to the final else clause. If higher NFT levels are introduced in the future, they will unintentionally have no voting weight. Consider updating the function to handle additional levels appropriately or enforcing level restrictions to prevent unexpected behavior.
Apply this diff to handle levels greater than 3:
function _getVotes(
address account,
uint256 blockNumber,
bytes memory params
) internal view override returns (uint256) {
uint256 tokenId = xpNFT.tokenByUserTag(account, tagValidToVote);
uint256 level = xpNFT.getLevel(tokenId);
require(level > 0, "ZetaXPGov: invalid NFT level");
- if (level == 1) {
- return 1; // Rosegold
- } else if (level == 2) {
- return 2; // Black
- } else if (level == 3) {
- return 3; // Green
- } else {
- return 0; // Silver cannot vote
+ if (level == 0) {
+ return 0; // Silver cannot vote
+ } else {
+ return level; // Voting weight equals NFT level
}
}Alternatively, if NFT levels are strictly limited to 1, 2, or 3, enforce this constraint:
function _getVotes(
address account,
uint256 blockNumber,
bytes memory params
) internal view override returns (uint256) {
uint256 tokenId = xpNFT.tokenByUserTag(account, tagValidToVote);
uint256 level = xpNFT.getLevel(tokenId);
- require(level > 0, "ZetaXPGov: invalid NFT level");
+ require(level > 0 && level <= 3, "ZetaXPGov: invalid NFT level");
if (level == 1) {
return 1; // Rosegold
} else if (level == 2) {
return 2; // Black
} else if (level == 3) {
return 3; // Green
}
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (1 hunks)
- packages/zevm-app-contracts/data/addresses.json (1 hunks)
- packages/zevm-app-contracts/scripts/address.helpers.ts (1 hunks)
- packages/zevm-app-contracts/scripts/xp-nft/deploy-gov.ts (1 hunks)
- packages/zevm-app-contracts/scripts/xp-nft/gov-query-proposals.ts (1 hunks)
- packages/zevm-app-contracts/scripts/xp-nft/gov-set-level.ts (1 hunks)
🔇 Additional comments (5)
packages/zevm-app-contracts/scripts/address.helpers.ts (1)
9-10: Verify the impact of network parameter addition.
The function signature change might affect existing callers that rely on the old signature.
Let's verify the usage:
✅ Verification successful
The network parameter addition is safe and backward compatible
The function signature change from getZEVMAppAddress(address: string) to getZEVMAppAddress(address: string, network: string = "zeta_testnet") is backward compatible since:
- The new parameter has a default value of "zeta_testnet"
- All current usages found in the codebase call the function with a single argument:
getZEVMAppAddress("rewardDistributorFactory")getZEVMAppAddress("zetaSwap")getZEVMAppAddress("withdrawERC20")- etc.
Only the XP-NFT related scripts explicitly use the network parameter, which appears to be the intended use case for this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to getZEVMAppAddress
rg -l 'getZEVMAppAddress\(' | xargs rg -A 2 'getZEVMAppAddress\('
Length of output: 3983
packages/zevm-app-contracts/data/addresses.json (2)
12-14: LGTM! Address entries are properly formatted
The new contract addresses are correctly formatted and maintain consistency with the existing structure.
13-14: Verify contract deployment and initialization
The addresses look valid, but let's verify the contract deployment and initialization:
✅ Verification successful
Contract deployment verification successful
Both contracts are successfully deployed and contain valid bytecode at the specified addresses:
- TimelockController: 0x26F16EB99F195F1f401b0Afcafa2FEbb9427CE09
- ZetaXPGov: 0x7B01E3060B9aAb92E3b981DB699330bB582a53C0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the contracts are deployed and contain the expected code
# Test 1: Check if the addresses contain contract code
echo "Checking TimelockController deployment..."
curl -s -X POST -H "Content-Type: application/json" --data '{
"jsonrpc":"2.0",
"method":"eth_getCode",
"params":["0x26F16EB99F195F1f401b0Afcafa2FEbb9427CE09", "latest"],
"id":1
}' https://zetachain-athens-evm.blockpi.network/v1/rpc/public
echo "Checking ZetaXPGov deployment..."
curl -s -X POST -H "Content-Type: application/json" --data '{
"jsonrpc":"2.0",
"method":"eth_getCode",
"params":["0x7B01E3060B9aAb92E3b981DB699330bB582a53C0", "latest"],
"id":1
}' https://zetachain-athens-evm.blockpi.network/v1/rpc/public
# Expected: Both responses should return contract bytecode (non-empty hex string)
# If either returns "0x", the contract is not deployed at that address
Length of output: 52481
packages/zevm-app-contracts/scripts/xp-nft/gov-set-level.ts (1)
34-58: Add network verification to prevent accidental deployments.
Given the sensitive nature of governance operations, additional safety checks should be implemented.
Add network verification:
const callGov = async () => {
const [signer] = await ethers.getSigners();
+ // Verify network
+ const network = await ethers.provider.getNetwork();
+ const expectedChainId = process.env.EXPECTED_CHAIN_ID;
+ if (expectedChainId && network.chainId !== parseInt(expectedChainId)) {
+ throw new Error(`Wrong network: expected chainId ${expectedChainId}, got ${network.chainId}`);
+ }
if (!isProtocolNetworkName(networkName)) throw new Error("Invalid network name");packages/zevm-app-contracts/scripts/xp-nft/deploy-gov.ts (1)
39-39: Validate tag encoding for correctness
Ensure that the encoded tag generated by encodeTag("XP_NFT") matches the expected value. This is critical for contract interactions that depend on consistent identifiers.
Run the following script to confirm the encoded tag:
✅ Verification successful
Tag encoding implementation is consistent but should be centralized
The encodeTag function is consistently implemented across multiple files as:
const encodeTag = (tag: string) => ethers.utils.keccak256(ethers.utils.defaultAbiCoder.encode(["string"], [tag]));- The encoding logic is correct but duplicated across files:
- packages/zevm-app-contracts/scripts/xp-nft/deploy-gov.ts
- packages/zevm-app-contracts/scripts/xp-nft/gov-set-level.ts
- packages/zevm-app-contracts/test/xp-nft/zeta-xp-gov.ts
- packages/zevm-app-contracts/test/xp-nft/xp-nft.ts
- packages/zevm-app-contracts/test/xp-nft/xp-nft-v2.ts
- packages/zevm-app-contracts/test/xp-nft/xp-nft-v2-back.ts
Consider moving this function to a shared utility file to maintain consistency and facilitate future updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the encoded tag matches the expected value.
# Expected encoded tag value (replace with the known correct value)
EXPECTED_TAG="0x..."
# Test: Output the encoded tag.
node -e "
const ethers = require('ethers');
const tag = 'XP_NFT';
const encoded = ethers.utils.defaultAbiCoder.encode(['string'], [tag]);
const hash = ethers.utils.keccak256(encoded);
console.log('Encoded Tag:', hash);
if (hash !== '$EXPECTED_TAG') {
throw new Error('Encoded tag does not match expected value.');
}
"
Length of output: 1135
Script:
#!/bin/bash
# Let's first check the implementation of encodeTag function
rg -A 5 "function encodeTag"
# Also search for any tests or usage of encodeTag
rg "encodeTag\("
# Look for any constants or configurations related to XP_NFT tag
rg "XP_NFT"
Length of output: 2255
Script:
#!/bin/bash
# Let's find the implementation of encodeTag function
ast-grep --pattern 'function encodeTag'
# Also check for any utility files that might contain this function
fd encodeTag
# Look for any test files that might verify the encoding
fd -e test.ts -e spec.ts | xargs rg "describe.*encodeTag"
Length of output: 116
Script:
#!/bin/bash
# Let's look for imports to find where encodeTag is defined
rg "import.*encodeTag" -A 2
# Search for any files that might contain the utility functions
fd -e ts -e js | xargs rg "export.*encodeTag"
# Look in common utility directories
fd utils -t d | xargs rg "encodeTag"
Length of output: 2234
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (1 hunks)
- packages/zevm-app-contracts/data/addresses.json (1 hunks)
- packages/zevm-app-contracts/scripts/xp-nft/deploy-gov.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/data/addresses.json
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (3 hunks)
- packages/zevm-app-contracts/data/addresses.json (1 hunks)
- packages/zevm-app-contracts/scripts/xp-nft/gov-set-level.ts (1 hunks)
- packages/zevm-app-contracts/test/xp-nft/zeta-xp-gov.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/zevm-app-contracts/data/addresses.json
- packages/zevm-app-contracts/scripts/xp-nft/gov-set-level.ts
🧰 Additional context used
🪛 GitHub Check: Slither
packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol
[warning] 38-38: Conformance to Solidity naming conventions
Parameter ZetaXPGov.setQuorumPercentage(uint256)._quorumPercentage (contracts/xp-nft/ZetaXPGov.sol#38) is not in mixedCase
[warning] 42-42: Conformance to Solidity naming conventions
Parameter ZetaXPGov.setMinLevelToPropose(uint256)._minLevelToPropose (contracts/xp-nft/ZetaXPGov.sol#42) is not in mixedCase
🔇 Additional comments (1)
packages/zevm-app-contracts/contracts/xp-nft/ZetaXPGov.sol (1)
46-49: Handle potential errors when retrieving tokenId and NFT level
andresaiello
deleted the
andy/pd-6855-improve-xp-gov-contracts-and-redeploy
branch
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Chores