feat: adding NetworkConfig #225
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThis pull request introduces a network configuration system for the Prism project. The changes primarily focus on modifying the configuration management in the CLI, adding new dependencies, and updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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: 2
🧹 Nitpick comments (7)
crates/cli/src/main.rs (1)
27-39: Improve error handling in configuration loading.The command handling has been simplified, but the error handling could be more specific.
Consider using a custom error type instead of converting all errors to
std::io::Error:- let config = - load_config(args.clone()).map_err(|e| Error::new(ErrorKind::Other, e.to_string()))?; + let config = load_config(args.clone()).map_err(|e| Error::new( + ErrorKind::InvalidData, + format!("Failed to load configuration: {}", e) + ))?;crates/tests/src/lib.rs (1)
80-83: Expand test coverage for NetworkConfig.While the test has been updated for the new LightClient constructor, it doesn't verify the network configuration functionality comprehensively.
Consider adding test cases for:
- Invalid network configurations
- Edge cases in start height values
- Verifying key validation scenarios
crates/node_types/lightclient/src/lightclient.rs (2)
27-36: Good architectural improvement, but needs documentation.The simplified constructor makes the LightClient more modular and less coupled to specific DA layer implementations.
Consider adding documentation to explain:
- The expected format of
start_height- Validation requirements for
prover_pubkey- Format requirements for
sp1_vkey
3-3: Consider adding logging for configuration values.Adding debug logs for configuration values would help with troubleshooting.
use prism_da::DataAvailabilityLayer; +use log::debug;crates/cli/src/cfg.rs (2)
218-239: Consider refactoring the celestia_config construction for better readability.The current implementation is verbose. Consider extracting this into a separate method or using the builder pattern.
- let celestia_config = match config.da_layer { - DALayerOption::Celestia => Some(CelestiaConfig { - connection_string: args - .celestia - .celestia_client - .unwrap_or(default_celestia_config.connection_string), - start_height: args - .celestia - .celestia_start_height - .unwrap_or(default_celestia_config.start_height), - snark_namespace_id: args - .celestia - .snark_namespace_id - .unwrap_or(default_celestia_config.snark_namespace_id), - operation_namespace_id: args - .celestia - .operation_namespace_id - .unwrap_or(default_celestia_config.operation_namespace_id), - }), - DALayerOption::InMemory => None, - }; + let celestia_config = config.da_layer.match_map(|| CelestiaConfig { + connection_string: args.celestia.celestia_client + .unwrap_or(default_celestia_config.connection_string), + start_height: args.celestia.celestia_start_height + .unwrap_or(default_celestia_config.start_height), + snark_namespace_id: args.celestia.snark_namespace_id + .unwrap_or(default_celestia_config.snark_namespace_id), + operation_namespace_id: args.celestia.operation_namespace_id + .unwrap_or(default_celestia_config.operation_namespace_id), + });
295-299: Improve error message for missing Celestia configuration.The error message could be more descriptive to help with troubleshooting.
- .context("Celestia configuration not found")?; + .context("Celestia configuration not found. Ensure it is properly set in the network configuration")?;crates/cli/Cargo.toml (1)
Line range hint
17-38: Consider organizing dependencies into categories and auditing necessity.While using workspace-level version management is good, consider:
- Organizing dependencies into categories using comments (e.g.,
# Networking,# Async, etc.) for better maintainability- Auditing if all dependencies are directly needed for the NetworkConfig feature
Example organization:
[dependencies] +# Network & Celestia lumina-node = { workspace = true } +celestia-types = { workspace = true } + +# Async & Runtime async-trait = { workspace = true } tokio = { workspace = true } + +# Logging log = { workspace = true } pretty_env_logger = { workspace = true } # ... continue organizing other dependencies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.toml(1 hunks)crates/cli/Cargo.toml(2 hunks)crates/cli/src/cfg.rs(9 hunks)crates/cli/src/lib.rs(1 hunks)crates/cli/src/main.rs(3 hunks)crates/node_types/lightclient/src/lightclient.rs(2 hunks)crates/tests/src/lib.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: clippy
- GitHub Check: integration-test
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
🔇 Additional comments (7)
crates/cli/src/lib.rs (1)
2-2: LGTM! Network module addition aligns with PR objectives.The new
networkmodule addition is clean and follows the existing module structure.crates/cli/src/main.rs (2)
110-122: LGTM! Good extraction of signing key logic.The
get_signing_keyfunction is well-structured and provides clear error messages.
41-53: Verify the verifying key consistency.The LightClient initialization now uses the network configuration's verifying key directly, but we should verify its compatibility with the ProverClient's verification key.
crates/cli/src/cfg.rs (3)
8-8: LGTM! The new imports are well-organized and necessary.The added imports support the new NetworkConfig feature and base64 encoding/decoding functionality.
Also applies to: 10-10, 28-28
47-48: Verify base64 validation handling.The field expects a base64-encoded string. Let's verify the validation handling:
✅ Verification successful
Base64 validation is properly implemented.
The verifying key field is validated through a robust base64 validation system that includes proper error handling, length validation, and base64 decoding checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for base64 validation handling rg -A 3 "from_base64" .Length of output: 23301
109-109: Verify NetworkConfig initialization across different scenarios.The NetworkConfig is initialized with default values. Let's verify the initialization:
Also applies to: 118-118
Cargo.toml (1)
104-107: Verify version compatibility and security implications.
- Version compatibility concerns:
lumina-node(0.8.0) matchescelestia-rpc(0.8.0)- But
celestia-typesis at 0.9.0 which might cause compatibility issues- Security considerations:
- Pinned versions prevent automatic security updates
Run this script to check for known vulnerabilities and compatibility:
✅ Verification successful
Dependencies are compatible with no security concerns
The version differences follow semantic versioning principles:
- celestia-types 0.9.0 should be backward compatible with 0.8.0 components
- No critical security vulnerabilities found in the specified versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for known vulnerabilities in these dependencies gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: CARGO, package: "lumina-node") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } } ' # Check if celestia-types 0.9.0 is compatible with lumina-node 0.8.0 cargo tree -p celestia-types -p lumina-node --depth=0Length of output: 928
Script:
#!/bin/bash # Clone RustSec database git clone --depth 1 https://github.com/RustSec/advisory-db.git cd advisory-db # Search for relevant advisories echo "=== Searching for lumina-node advisories ===" find . -type f -name "*.md" -exec grep -l "lumina-node" {} \; echo "=== Searching for libp2p advisories ===" find . -type f -name "*.md" -exec grep -l "libp2p" {} \; cd .. # Check dependency specifications echo "=== Checking dependency specifications ===" rg -A 5 "lumina-node|celestia-|libp2p" Cargo.tomlLength of output: 1151
Script:
#!/bin/bash # Check content of recent libp2p advisory cd advisory-db echo "=== Recent libp2p advisory ===" cat crates/libp2p-tokio-socks5/RUSTSEC-2024-0334.md echo "=== Checking version requirements ===" # Check if there are any version requirements in source files cd .. rg -B2 -A2 "celestia.*0\.9|celestia.*0\.8|lumina.*0\.8" --type rustLength of output: 953
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/cli/src/network.rs (2)
15-22: Consider improving documentation and encapsulation.While the struct design is good, consider these improvements:
- Add more comprehensive documentation for the struct and each field
- Consider making fields private with public getters to prevent invalid state mutations
Example documentation:
/// Configuration for a network instance including Celestia network settings and verification keys pub struct NetworkConfig { /// The type of network (Specter or Custom) network: Network, /// The specific Celestia network configuration celestia_network: CelestiaNetwork, /// The verifying key of the prover used for verification of proofs verifying_key: Option<VerifyingKey>, /// Additional Celestia-specific configuration parameters celestia_config: Option<CelestiaConfig>, }
24-33: Consider more descriptive default network name.The current default network name "custom" is quite generic. Consider using a more descriptive name like "local" or "development" to better indicate its purpose.
- network: Network::Custom("custom".to_string()), + network: Network::Custom("local".to_string()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/cli/src/network.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: integration-test
- GitHub Check: build-and-push-image
🔇 Additional comments (4)
crates/cli/src/network.rs (4)
1-8: LGTM! Well-organized imports.The imports are clean, necessary, and logically grouped.
9-13: LGTM! Well-designed enum for network types.The
Networkenum provides good extensibility with itsCustomvariant while maintaining type safety for known networks.
1-71: Verify WASM compatibility of dependencies.@sebasti810 For WASM compatibility review:
- The code structure looks WASM-compatible
- Please verify that
prism_serde::base64implementation works in WASM context- Consider adding WASM-specific tests
Let's check if the dependencies have WASM features:
✅ Verification successful
Base64 implementation is WASM-compatible ✅
The base64 implementation in
prism-serdeuses pure Rust code with WASM-compatible dependencies. No platform-specific code or unsafe operations are present. The implementation relies on the standardbase64crate which is known to work well in WASM context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for wasm-related features in Cargo.toml files rg -l 'wasm|target.*wasm' --glob 'Cargo.toml' # Check if prism_serde has WASM-specific code rg -l 'wasm|cfg.*wasm' crates/prism_serde/src/Length of output: 158
Script:
#!/bin/bash # Find the correct path for prism_serde fd prism_serde # Once found, let's examine its Cargo.toml and source files fd -t f . -e rs -e toml --full-path ./prism-serde # Look for base64 implementation rg -l "base64" --type rustLength of output: 405
Script:
#!/bin/bash # Check the base64 implementation cat crates/serde/src/base64.rs # Check the serde crate's Cargo.toml for dependencies cat crates/serde/Cargo.toml # Look for any cfg or target specific code rg "cfg\(" crates/serde/src/Length of output: 1558
46-71:⚠️ Potential issueMove hardcoded values to constants and improve error handling.
Several improvements are needed:
- Hardcoded values should be constants
- Base64 key decoding uses unwrap() which could panic
- Magic numbers and strings should be documented
Consider this safer implementation:
impl Network { const SPECTER_VERIFYING_KEY: &'static str = "L2ilppK59Kq3aAMB/wpxdVGaI53DHPMdY6fcRodyFaA="; const SPECTER_START_HEIGHT: u64 = 4180975; const SPECTER_SNARK_NAMESPACE: &'static str = "000000000000000000000000000000000000707269736d5350457330"; const SPECTER_OPERATION_NAMESPACE: &'static str = "000000000000000000000000000000000000707269736d5350456f30"; pub fn config(&self) -> Result<NetworkConfig, anyhow::Error> { match self { Network::Specter => { let key = VerifyingKey::from_base64(Self::SPECTER_VERIFYING_KEY) .map_err(|e| anyhow::anyhow!("Failed to decode verifying key: {}", e))?; Ok(NetworkConfig { network: Network::Specter, celestia_network: CelestiaNetwork::Mocha, verifying_key: Some(key), celestia_config: Some(CelestiaConfig { start_height: Self::SPECTER_START_HEIGHT, snark_namespace_id: Self::SPECTER_SNARK_NAMESPACE.to_string(), operation_namespace_id: Self::SPECTER_OPERATION_NAMESPACE.to_string(), ..CelestiaConfig::default() }), }) }, Network::Custom(id) => Ok(NetworkConfig { network: Network::Custom(id.clone()), ..Default::default() }), } } }Additionally, let's verify if these hardcoded values are documented elsewhere:
✅ Verification successful
Consider moving hardcoded values to constants for better maintainability
The verification confirms that these values are network-specific constants without external configuration. While the original review's suggestions were good, we can simplify the recommendation:
- Move the hardcoded values to named constants for better maintainability and documentation
- Consider adding doc comments explaining the significance of these values
- The
unwrap()usage appears intentional as this is network configuration code, but adding a comment explaining why panic is acceptable would be helpfulThe suggestion to change the return type to
Resultmight be too disruptive since other code depends on this configuration being infallible.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation about these magic values rg -i "4180975|707269736d5350457330|707269736d5350456f30" --type mdLength of output: 69
Script:
#!/bin/bash # Search for base64 key pattern and network configurations ast-grep --pattern 'VerifyingKey::from_base64($_)' # Search for potential config files and environment variables fd -e toml -e yaml -e env . -x cat {} # Search for other network-related configurations rg -A 5 "impl Network"Length of output: 12923
|
yes ser, that should work :) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/cli/src/cfg.rs (1)
260-268:⚠️ Potential issueHandle potential network name parsing failure gracefully.
The network configuration uses
unwrap()which could panic if the network name is invalid.
🧹 Nitpick comments (3)
crates/cli/src/cfg.rs (3)
47-48: Enhance documentation for verifying_key field.While the current documentation mentions the base64 encoding requirement, it would be helpful to add:
- The purpose of the verifying key in the context of epoch signatures
- The expected format before base64 encoding
- Whether this overrides any network-specific default key
182-184: Improve warning message for missing verifying key.The current warning message could be more informative by:
- Explaining the security implications of missing key verification
- Providing guidance on how to set up a verifying key
- Mentioning that network-specific default keys are used if available
260-268: Consider separating network identification from DA layer configuration.The
NetworkConfigstructure currently combines network identification with Celestia-specific configuration. Consider splitting these concerns to:
- Make it easier to add support for other DA layers in the future
- Improve modularity and maintainability
- Keep network identification independent of specific DA layer implementations
Example structure:
pub struct NetworkConfig { network: Network, verifying_key: Option<VerifyingKey>, da_layer_config: DALayerConfig, } pub enum DALayerConfig { Celestia(CelestiaNetworkConfig), // Future DA layers can be added here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(1 hunks)crates/cli/src/cfg.rs(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unused dependencies
- GitHub Check: integration-test
- GitHub Check: unit-test
- GitHub Check: build-and-push-image
🔇 Additional comments (1)
crates/cli/src/cfg.rs (1)
115-122:⚠️ Potential issueHandle potential network name parsing failure gracefully.
The
initializemethod usesunwrap()which could panic if the network name is invalid.Apply this diff to handle the error gracefully:
- fn initialize(path: &str, network_name: &str) -> Self { + fn initialize(path: &str, network_name: &str) -> Result<Self> { - Config { + Ok(Config { webserver: Some(WebServerConfig::default()), - network: Network::from_str(network_name).unwrap().config(), + network: Network::from_str(network_name) + .map_err(|e| anyhow::anyhow!("Invalid network name '{}': {}", network_name, e))? + .config(), da_layer: DALayerOption::default(), db: StorageBackend::RocksDB(RocksDBConfig::new(&format!("{}data", path))), - } + }) }Likely invalid or redundant comment.
distractedm1nd
force-pushed
the
network_config
branch
from
48e1814 to
c6fe320
Compare
distractedm1nd
force-pushed
the
network_config
branch
from
c6fe320 to
f33f8b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/cli/src/cfg.rs (1)
260-268:⚠️ Potential issueVerify error handling for network name parsing.
The
unwrap()call on the result ofNetwork::from_strcould panic if the network name is invalid.Apply this diff to handle the error gracefully:
- network: Network::from_str(&args.network_name.unwrap_or_default()).unwrap(), + network: Network::from_str(&args.network_name.unwrap_or_default()) + .map_err(|e| anyhow::anyhow!("Invalid network name: {}", e))?,crates/cli/src/main.rs (1)
54-79:⚠️ Potential issueRace condition in prover initialization still exists.
The prover generates a new verifying key after initialization, but there's no mechanism to ensure this key is propagated to the network configuration for other components.
Consider this approach:
Commands::Prover(_) => { let db = initialize_db(&config).map_err(|e| Error::new(ErrorKind::Other, e.to_string()))?; let signing_key = get_signing_key()?; let verifying_key = signing_key.verifying_key(); + + // Update network config with new verifying key + config.network.update_verifying_key(verifying_key.clone()) + .map_err(|e| Error::new(ErrorKind::Other, e.to_string()))?; info!( "prover's verifying key: {}", verifying_key.to_bytes().to_base64() );
🧹 Nitpick comments (5)
crates/cli/src/cfg.rs (3)
47-48: Improve documentation for the verifying key format.While the documentation mentions that the verifying key should be base64-encoded, it would be helpful to provide an example format or pattern to guide users.
- /// Prover's verifying key, used to verify epoch signatures. Expected to be a base64-encoded string. + /// Prover's verifying key, used to verify epoch signatures. + /// Expected to be a base64-encoded string. + /// Example format: "VkVSS0VZMXpoYnM2NVl3PT0="
299-303: Consider using the?operator for better error handling.The current code uses
context()to handle the error, but the?operator would be more idiomatic here since we're already returning aResult.- let celestia_conf = config - .network - .celestia_config - .clone() - .context("Celestia configuration not found")?; + let celestia_conf = config.network.celestia_config.clone()?;
162-166: Simplify the network name handling.The code clones the
argsunnecessarily and uses a verbose unwrap chain.- ensure_config_file_exists( - &home_path, - &args.clone().network_name.unwrap_or("custom".to_string()), - ) - .context("Failed to ensure config file exists")?; + let network_name = args.network_name.as_deref().unwrap_or("custom"); + ensure_config_file_exists(&home_path, network_name) + .context("Failed to ensure config file exists")?;crates/cli/src/main.rs (2)
Line range hint
80-104: Improve error handling for verifying key retrieval in FullNode.The error message could be more descriptive about where it expects to find the prover verifying key.
- .ok_or_else(|| Error::new(ErrorKind::NotFound, "prover verifying key not found"))?; + .ok_or_else(|| Error::new( + ErrorKind::NotFound, + "prover verifying key not found in network configuration", + ))?;
110-122: Add documentation and improve error handling.The new helper function would benefit from:
- Documentation explaining the key retrieval process
- More specific error handling for different failure scenarios
+/// Retrieves the signing key from the system's keychain. +/// +/// # Returns +/// - Ok(SigningKey): The signing key if successfully retrieved and validated +/// - Err: If the key cannot be retrieved from keychain or if the key format is invalid fn get_signing_key() -> std::io::Result<SigningKey> { let signing_key_chain = KeyStoreType::KeyChain(KeyChain) .get_signing_key() - .map_err(|e| Error::new(ErrorKind::Other, e.to_string()))?; + .map_err(|e| Error::new(ErrorKind::NotFound, format!("Failed to retrieve key from keychain: {}", e)))?; SigningKey::from_algorithm_and_bytes(CryptoAlgorithm::Ed25519, signing_key_chain.as_bytes()) .map_err(|e| { Error::new( - ErrorKind::InvalidInput, + ErrorKind::InvalidData, format!("Invalid signing key: {}", e), ) }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(1 hunks)crates/cli/Cargo.toml(2 hunks)crates/cli/src/cfg.rs(11 hunks)crates/cli/src/lib.rs(1 hunks)crates/cli/src/main.rs(3 hunks)crates/cli/src/network.rs(1 hunks)crates/node_types/lightclient/src/lightclient.rs(2 hunks)crates/tests/src/lib.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/cli/src/lib.rs
- Cargo.toml
- crates/tests/src/lib.rs
- crates/cli/src/network.rs
- crates/cli/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unused dependencies
- GitHub Check: unit-test
- GitHub Check: clippy
- GitHub Check: build-and-push-image
- GitHub Check: integration-test
🔇 Additional comments (5)
crates/node_types/lightclient/src/lightclient.rs (1)
27-35: LGTM! Good refactoring of the constructor.The changes simplify the
LightClientinitialization by directly accepting thestart_heightparameter instead of extracting it from a config object. This improves separation of concerns and makes the code more flexible and easier to test.crates/cli/src/cfg.rs (1)
263-266: Verify the verifying key parsing logic.The code attempts to parse the verifying key from base64 but silently ignores parsing errors. We should verify if this is the intended behavior.
✅ Verification successful
Verifying key parsing behavior is correct
The silent error handling is intentional here as it implements a fallback pattern - if the user-provided key is invalid, it gracefully falls back to the network configuration's key. The underlying
VerifyingKey::from_base64implementation properly validates the key format and length.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances where verifying key parsing errors are handled rg -A 3 "from_base64"Length of output: 23533
crates/cli/src/main.rs (3)
2-2: LGTM! Network module addition aligns with PR objectives.The addition of the network module is consistent with the PR's goal of introducing NetworkConfig functionality.
41-53: Verify verifying key consistency in LightClient.The LightClient uses two different verifying keys:
- From network config (
verifying_key)- From ProverClient setup (
vk)Ensure these keys are consistent and validate their relationship.
27-29: Verify command handling robustness.The current match pattern assumes all commands have args. Consider:
- Adding a default case to handle potential future commands
- Adding explicit error handling for invalid command types
Closes #219
Also, @smuu, this would make #217 obsolete I think, because it handles the conversion directly inside of the config
@sebasti810 does this work for you for WASM?
Summary by CodeRabbit
Summary by CodeRabbit
Dependencies
lumina-nodeto workspace dependencies.async-trait,serde,tokio, and logging utilities.Configuration
networkfield.Library Changes
networkmodule to the CLI library.Networkenum andNetworkConfigstruct.