chore: Add base mainnet addresses for go live #373
Conversation
CryptoFewka
requested review from
andresaiello,
brewmaster012,
lumtis,
CharlieMc0,
fadeev,
skosito and
fbac
as code owners
|
Warning Rate limit exceeded@CryptoFewka has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request consist of adding a new blockchain network configuration for Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
CryptoFewka
changed the title
CryptoFewka
changed the title
![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
🧹 Outside diff range and nitpick comments (2)
v1/hardhat.config.ts (1)
50-57: LGTM! Consider adding API key and explanatory comment.The addition of the Base Mainnet configuration looks good and is consistent with other custom chain configurations. The chain ID and URLs are correct for Base Mainnet.
To further improve this change:
- Consider adding an API key entry for
base_mainnetin theapiKeysection above, similar to other networks.- It might be helpful to add a comment explaining the purpose of this addition, e.g., "// Base Mainnet configuration".
Here's a suggested modification:
etherscan: { apiKey: { amoy_testnet: process.env.POLYGONSCAN_API_KEY || "", base_testnet: process.env.BASESCAN_API_KEY || "", + base_mainnet: process.env.BASESCAN_API_KEY || "", // BSC bsc: process.env.BSCSCAN_API_KEY || "", // ... (other configurations) }, customChains: [ // ... (other custom chains) + // Base Mainnet configuration { chainId: 8453, network: "base_mainnet", urls: { apiURL: "https://api.basescan.org/api", browserURL: "https://basescan.org/", }, }, ], },v1/package.json (1)
92-92: Consider a patch version bump.Given the updates to dependencies, particularly the hardhat version, it might be appropriate to increment the patch version of this package (e.g., from 0.0.8 to 0.0.9). This would signal to users that there have been minor updates or bug fixes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
v1/data/addresses.jsonis excluded by!v1/data/**v1/data/addresses.mainnet.jsonis excluded by!v1/data/**
📒 Files selected for processing (4)
- v1/hardhat.config.ts (1 hunks)
- v1/lib/address.tools.ts (1 hunks)
- v1/lib/contracts.constants.ts (1 hunks)
- v1/package.json (2 hunks)
🔇 Additional comments (6)
v1/lib/contracts.constants.ts (1)
15-20: Verify salt numbers for Base MainnetThe addition of the
base_mainnetentry is good and follows the existing structure. However, there are a few points to consider:
- All salt numbers are currently set to "0". Is this intentional, or are these placeholders?
- Remember the comment above
SALT_NUMBERS: "dev: this values should be calculated using get-salt script". Have you run this script for the Base Mainnet values?- Using "0" for all contracts might lead to conflicts if these values are used for unique identification.
Please verify if "0" is the correct value for all contracts on Base Mainnet. If these are placeholders, update them with the correct values calculated using the get-salt script.
To help verify the uniqueness of salt numbers across networks, you can run the following script:
This script will help identify any potential issues with salt number uniqueness and the frequency of "0" values.
v1/lib/address.tools.ts (3)
70-70: LGTM: Addition of "base_mainnet" to ZetaProtocolMainNetwork typeThe addition of "base_mainnet" to the ZetaProtocolMainNetwork type is correct and aligns with the PR objective of adding base mainnet addresses. This change is consistent with the existing type declaration format and doesn't introduce any issues.
70-70: Summary: Successfully added base mainnet supportThe changes to add "base_mainnet" to both the ZetaProtocolMainNetwork type and the zetaProtocolMainNetworks array are correct and consistent. These additions successfully implement the PR objective of adding base mainnet addresses to the Zeta Protocol.
Key points:
- The type declaration is updated to include "base_mainnet".
- The constant array is updated to reflect the new type.
- Alphabetical order is maintained in the array, preserving readability and consistency.
These changes enhance the protocol's support for different networks without introducing any apparent issues or inconsistencies in the existing code structure.
Also applies to: 78-78
78-78: LGTM: Addition of "base_mainnet" to zetaProtocolMainNetworks arrayThe addition of "base_mainnet" to the zetaProtocolMainNetworks array is correct and consistent with the updated type declaration. The alphabetical order is maintained, which is good for readability and consistency.
To ensure that this change is properly reflected throughout the codebase, let's verify its usage:
✅ Verification successful
LGTM: Addition of "base_mainnet" to zetaProtocolMainNetworks array
The addition of
"base_mainnet"to thezetaProtocolMainNetworksarray has been successfully verified. All references tozetaProtocolMainNetworksare appropriately updated, and no hardcoded main network lists were found that require modification.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the updated zetaProtocolMainNetworks constant # Test: Search for references to zetaProtocolMainNetworks echo "Searching for references to zetaProtocolMainNetworks:" rg --type typescript "zetaProtocolMainNetworks" # Test: Search for any hardcoded lists of main networks that might need updating echo "Searching for potential hardcoded lists of main networks:" rg --type typescript -e "bsc_mainnet.*eth_mainnet.*polygon_mainnet"Length of output: 429
Script:
#!/bin/bash # Description: Verify the usage of the updated zetaProtocolMainNetworks constant # Test: Search for references to zetaProtocolMainNetworks in TypeScript files echo "Searching for references to zetaProtocolMainNetworks in .ts and .tsx files:" rg "zetaProtocolMainNetworks" --glob "**/*.ts" --glob "**/*.tsx" # Test: Search for any hardcoded lists of main networks that might need updating in TypeScript files echo "Searching for potential hardcoded lists of main networks in .ts and .tsx files:" rg -e "bsc_mainnet.*eth_mainnet.*polygon_mainnet" --glob "**/*.ts" --glob "**/*.tsx"Length of output: 821
v1/package.json (2)
93-95: Approve chokidar addition, but clarify its intended use.The addition of chokidar (a file watching library) suggests new functionality related to file system monitoring. This could be beneficial for development workflows or build processes.
Could you please provide more context on why chokidar was added and where it's being used in the project? This will help ensure it's being used appropriately and efficiently.
To help verify its usage, you can run the following script:
#!/bin/bash # Description: Check for chokidar usage in the project # Test: Search for chokidar imports or requires rg --type typescript --type javascript 'require\(["\']chokidar["\']\)|import .* from ["\']chokidar["\'']' # Test: Look for potential file watching setup rg --type typescript --type javascript 'watch\(|watchFile\('
50-50: Hardhat version update looks good, but verify compatibility.The update of hardhat from "^2.17.2" to "^2.22.12" is a good practice to keep dependencies current. However, it's important to ensure this update doesn't introduce any breaking changes or conflicts with other dependencies.
Please run the following script to check for any potential conflicts or issues:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
v1/hardhat.config.ts (1)
24-24: LGTM with a minor suggestion.The addition of the Base Mainnet API key is correct and follows the existing pattern. Using an environment variable for the API key is a good security practice.
Consider reordering the keys alphabetically to maintain consistency and improve readability:
apiKey: { amoy_testnet: process.env.POLYGONSCAN_API_KEY || "", + base_mainnet: process.env.BASESCAN_API_KEY || "", base_testnet: process.env.BASESCAN_API_KEY || "", - base_mainnet: process.env.BASESCAN_API_KEY || "", // BSC bsc: process.env.BSCSCAN_API_KEY || "",🧰 Tools
🪛 GitHub Check: lint
[failure] 24-24:
Expected object keys to be in ascending order. 'base_mainnet' should be before 'base_testnet'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- v1/hardhat.config.ts (2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
v1/hardhat.config.ts
[failure] 24-24:
Expected object keys to be in ascending order. 'base_mainnet' should be before 'base_testnet'
🔇 Additional comments (1)
v1/hardhat.config.ts (1)
Line range hint
1-95: Overall, the changes look good.The additions for Base Mainnet in the Hardhat configuration file are correct and consistent with the existing patterns. The use of environment variables for API keys is a good security practice. A minor suggestion was made to improve key ordering, and a verification step was recommended to ensure the accuracy of the Base Mainnet details.
|
Looks good to me. The CI failure is not related to the change. I will merge now. @fadeev do we still need the v1 generate to pass? |
I guess not. |
Summary by CodeRabbit
New Features
base_mainnetblockchain network, enhancing the existing network configurations.base_mainnet.base_mainnetnetwork within the SALT_NUMBERS configuration.Dependencies
hardhatpackage to a newer version.chokidar.