-
Notifications
You must be signed in to change notification settings - Fork 26
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.
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: added mainnet chain configs #689
Conversation
WalkthroughThe pull request introduces modifications to several files, primarily focusing on the release workflow and the implementation of blockchain network constants. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🔇 Additional comments (9)packages/deploy/src/chains/mod.rs (1)
The modular structure cleanly separates different chain types (devnets, mainnets, testnets) while maintaining a unified interface through the common ChainInfo type. packages/deploy/src/chains/testnets.rs (2)
The addition of "-testnet" suffix to chain names (andromeda-testnet, stargaze-testnet) improves clarity and aligns with the PR objective of differentiating chain types. Also applies to: 24-24
The .github/workflows/release.yml (1)
Setting releases as drafts and prereleases is a good safety measure, especially when introducing mainnet chain configurations. This ensures proper review before production deployment. packages/deploy/src/chains/devnets.rs (1)
The devnet configurations are well-structured with clear separation between network info and chain info. Good practice using constants for maintainability. packages/deploy/src/contracts.rs (1)
Good architectural decision to separate core OS contracts from application contracts. The implementation is clean and uses consistent patterns. Also applies to: 14-16, 31-31, 88-96 packages/deploy/src/chains/mainnets.rs (1)
The gas prices vary significantly between chains:
Also applies to: 33-33, 50-50, 67-67 packages/deploy/src/os.rs (2)
The new publish function is well-implemented with:
Also applies to: 184-186
The module name change from "ibc_registry" to "ibc-registry" aligns with the naming convention but might affect existing deployments. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
contracts/os/andromeda-kernel/src/interface.rs (1)
41-50
: Add documentation for ADOMetadata implementationThe implementation looks good but would benefit from documentation explaining the purpose of these metadata fields and their usage.
Consider adding documentation:
+/// Implements ADOMetadata for KernelContract to provide standardized +/// access to contract metadata such as name and version. impl<Chain> ADOMetadata for KernelContract<Chain> { + /// Returns the contract identifier fn name() -> String { CONTRACT_ID.to_string() } + /// Returns the contract version from Cargo.toml fn version() -> String { let version = env!("CARGO_PKG_VERSION"); version.to_string() } }packages/deploy/src/chains/devnets.rs (2)
13-13
: Fix inconsistent chain ID naming patternThe chain IDs have inconsistent naming:
localosmosisa-1
localwasma-1
localandromedaa-1
(has double 'a')Consider standardizing to a consistent pattern.
Also applies to: 30-30, 47-47
17-18
: Consider infrastructure redundancy for devnetsAll devnet services (GRPC, LCD) are hosted on a single IP (164.90.212.168). While acceptable for development, consider:
- Adding fallback endpoints
- Documenting the infrastructure setup
- Adding health check endpoints
This helps prevent development bottlenecks if the single endpoint fails.
Also applies to: 34-35, 51-52
packages/deploy/src/contracts.rs (1)
88-96
: Add documentation for OS contracts functionConsider adding documentation to explain:
- Purpose of OS contracts vs regular contracts
- When to use os_contracts() vs all_contracts()
- Requirements or dependencies between these contracts
Suggested documentation:
+/// Returns a list of core Operating System contracts that are fundamental to the Andromeda protocol. +/// These contracts (ADODB, Kernel, VFS, Economics, IBC Registry) provide the base infrastructure +/// and should be deployed before application-specific contracts. pub fn os_contracts() -> Vec<DeployableContract> {packages/deploy/src/chains/mainnets.rs (2)
14-14
: Consider adding documentation for gas denomination unitsAdd comments to clarify the units for gas prices (e.g., price per gas unit) to help maintainers understand and update these values correctly.
Also applies to: 31-31, 48-48, 65-65
193-205
: Consider documenting the significance of chain orderAdd a comment above the
MAINNET_CHAINS
array to explain if the order of chains has any significance for the system.
🛑 Comments failed to post (4)
packages/deploy/src/chains/mod.rs (1)
10-25:
⚠️ Potential issueCritical: Improve error handling in get_chain function
The current implementation has two potential panic points:
- Unwrap on chain lookup (line 23)
- Panic on duplicate chain names (line 17)
Consider returning a Result type instead to handle errors gracefully.
Here's a suggested implementation:
-pub fn get_chain(chain: String) -> ChainInfo { +pub fn get_chain(chain: String) -> Result<ChainInfo, String> { let all_chains: Vec<ChainInfo> = [MAINNET_CHAINS, TESTNET_CHAINS, DEVNET_CHAINS].concat(); let unique_chain_names: std::collections::HashSet<&str> = all_chains .iter() .map(|c| c.network_info.chain_name) .collect(); if unique_chain_names.len() != all_chains.len() { - panic!("Duplicate chain names found in ChainInfo"); + return Err("Duplicate chain names found in ChainInfo".to_string()); } all_chains .iter() .find(|c| c.chain_id == chain || c.network_info.chain_name == chain) - .unwrap() - .clone() + .map(|c| c.clone()) + .ok_or_else(|| format!("Chain '{}' not found", chain)) }📝 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.pub fn get_chain(chain: String) -> Result<ChainInfo, String> { let all_chains: Vec<ChainInfo> = [MAINNET_CHAINS, TESTNET_CHAINS, DEVNET_CHAINS].concat(); let unique_chain_names: std::collections::HashSet<&str> = all_chains .iter() .map(|c| c.network_info.chain_name) .collect(); if unique_chain_names.len() != all_chains.len() { return Err("Duplicate chain names found in ChainInfo".to_string()); } all_chains .iter() .find(|c| c.chain_id == chain || c.network_info.chain_name == chain) .map(|c| c.clone()) .ok_or_else(|| format!("Chain '{}' not found", chain)) }
packages/deploy/src/chains/mainnets.rs (3)
119-119:
⚠️ Potential issueFix typo in Chihuahua GRPC URL
The GRPC URL contains a typo: "chiahuahua" instead of "chihuahua"
- grpc_urls: &["https://chiahuahua-grpc.polkachu.com:21290"], + grpc_urls: &["https://chihuahua-grpc.polkachu.com:21290"],📝 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.grpc_urls: &["https://chihuahua-grpc.polkachu.com:21290"],
98-98: 💡 Codebase verification
Chain ID should be updated to "desmos-mainnet"
The current chain ID "desmos" is incorrect. The API endpoint shows that the correct chain ID for Desmos mainnet is "desmos-mainnet".
- Update line 98 in
packages/deploy/src/chains/mainnets.rs
to usechain_id: "desmos-mainnet"
🔗 Analysis chain
Verify Desmos chain ID format
Other chains follow the "-1" suffix pattern (e.g., "juno-1", "chihuahua-1"), but Desmos uses just "desmos". Please verify if this is the correct chain ID.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Desmos chain ID curl -s "https://api.mainnet.desmos.network/cosmos/base/tendermint/v1beta1/node_info" | jq -r '.default_node_info.network'Length of output: 135
186-186: 💡 Codebase verification
Based on the search results from the codebase, I can now provide a final response:
Archway's gas price appears to be incorrectly configured
Comparing gas prices across all chains in the codebase:
- Most chains have gas prices between 0.001 and 1.0
- Archway's value (140000000000.0) is orders of magnitude higher than any other chain
- Similar chains like Nibiru and Coreum use 0.025 and 0.0625 respectively
The current value appears to be a misconfiguration as it's significantly out of range compared to all other chains in the codebase. Consider adjusting it to be in line with other Cosmos chains.
🔗 Analysis chain
Verify Archway's unusually high gas price
The gas price for Archway (140000000000.0) is significantly higher than other chains. Please verify if this is the correct value and consider adding a comment explaining why it differs so much from other chains.
Let me try another approach to verify the gas price by checking other files in the codebase for context and comparing with other chains.
Let me try one more verification by checking Archway's documentation or configuration files in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Archway's gas price curl -s "https://archway-api.polkachu.com/cosmos/base/node/v1beta1/params" | jq '.min_gas_price'Length of output: 99
Script:
#!/bin/bash # Search for gas price configurations across the codebase rg -A 2 "gas_price:" # Also check for any comments or documentation about Archway gas prices rg -i "archway.*gas.*price"Length of output: 4651
Script:
#!/bin/bash # Search for any Archway-related configuration files or documentation fd -e md -e txt -e rs "archway" --exec grep -i -A 3 -B 3 "gas|price|fee" # Also check for any commit messages or configuration changes related to Archway gas price git log --oneline -S "140000000000" -- packages/deploy/src/chains/mainnets.rsLength of output: 72
(cherry picked from commit 6533d5d)
Co-authored-by: Connor Barr <[email protected]>
Motivation
These changes add configs for all active mainnet chains. There are also changes to chain names to differentiate their type and a uniqueness check to prevent overlaps.
Implementation
mainnets.rs
fileSummary by CodeRabbit
New Features
publish
method for the Operating System Deployment, enhancing contract publishing capabilities.os_contracts
to return deployable contracts specific to the operating system context.Bug Fixes
Chores