feat: load app with only testnet config #16
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the application state management and error handling mechanisms across multiple files. The Changes
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
src/context.rs (1)
38-44: Improved error handling, but consider enhancing user feedbackThe addition of error handling for
Config::load()is a good improvement. It prevents the application from running with an invalid configuration, which aligns with the PR objective of loading the app with only testnet config.However, consider the following enhancements:
- Use
eprintln!instead ofprintln!for error messages, as errors should typically be written to stderr.- Provide more detailed error information to help users troubleshoot configuration issues.
- Consider a more graceful error handling approach, especially if this code might be used in a library context.
Here's a suggested improvement:
let config = match Config::load() { Ok(config) => config, Err(e) => { eprintln!("Failed to load config: {}", e); eprintln!("Please check your configuration file and ensure it's correctly formatted."); eprintln!("If the issue persists, contact support or check the documentation."); std::process::exit(1); } };This change provides more context and guidance to the user while still preventing the application from running with an invalid configuration.
src/config.rs (1)
63-65: Consider changing the error log to a warning when.envfile is missingSince the application can proceed using environment variables even if the
.envfile is missing, logging an error might not be appropriate. Consider logging a warning instead to indicate that while the.envfile was not loaded, the application will continue.Apply this diff to make the change:
- tracing::error!( + tracing::warn!( ?err, "Failed to load .env file. Continuing with environment variables." );src/ui/network_chooser_screen.rs (1)
54-54: Consider handling unexpectedNetworkvariants explicitlyUsing
unreachable!()assumes that all possibleNetworkvariants have been covered. If new variants are added to theNetworkenum in the future, this could lead to unexpected panics.Consider handling the default case explicitly or returning an error indicating an unsupported network.
🛑 Comments failed to post (6)
src/config.rs (1)
94-102:
⚠️ Potential issueCorrect the conditional logic to ensure all missing configurations are logged
The current use of
else ifstatements means that only one warning will be logged, even if one of the configurations is missing. To log warnings for each missing configuration, replace theelse ifstatements with separateifstatements.Apply this diff to fix the issue:
if mainnet_config.is_none() && testnet_config.is_none() { return Err(ConfigError::NoValidConfigs); } - else if mainnet_config.is_none() { + if mainnet_config.is_none() { tracing::warn!("Failed to load mainnet configuration"); } - else if testnet_config.is_none() { + if testnet_config.is_none() { tracing::warn!( "Failed to load testnet configuration, but successfully loaded mainnet config" ); }📝 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.if mainnet_config.is_none() && testnet_config.is_none() { return Err(ConfigError::NoValidConfigs); } if mainnet_config.is_none() { tracing::warn!("Failed to load mainnet configuration"); } if testnet_config.is_none() { tracing::warn!( "Failed to load testnet configuration, but successfully loaded mainnet config" ); }src/ui/network_chooser_screen.rs (2)
48-50:
⚠️ Potential issueAvoid unwrapping Option without handling
NonecasesIn the
context_for_networkmethod, usingunwrap()onself.mainnet_app_contextwithout handling theNonecase can lead to a panic ifmainnet_app_contextisNone. Even though there's a check withis_some(), it's safer to handle theNonecase explicitly to prevent potential panics.Apply this diff to handle the
Optionsafely:- Network::Dash if self.mainnet_app_context.is_some() => { - self.mainnet_app_context.as_ref().unwrap() - } + Network::Dash => { + match &self.mainnet_app_context { + Some(context) => context, + None => { + // Handle the None case appropriately, e.g., return an error or provide alternative behavior + } + } + }Committable suggestion was skipped due to low confidence.
145-145:
⚠️ Potential issueAvoid unwrapping Option without handling
NonecasesWhen adding a new wallet, unwrapping
self.mainnet_app_contextwithout checking if it isSomecan result in a panic ifmainnet_app_contextisNone. It's important to handle theNonecase to ensure the application remains stable.Apply this diff to safely handle the
Option:- &self.mainnet_app_context.as_ref().unwrap() + match &self.mainnet_app_context { + Some(context) => context, + None => { + // Handle the None case appropriately, e.g., display an error message or disable the action + } + }Committable suggestion was skipped due to low confidence.
src/app.rs (3)
111-115:
⚠️ Potential issuePrevent Panics When Both App Contexts Are
NoneIn the initialization logic,
mainnet_app_contextandtestnet_app_contextare unwrapped without checking if they containNone. If both contexts areNone, calling.unwrap()will cause a panic. Consider handling the scenario where neither context is available.Apply this diff to safely handle the absence of both contexts:
let app_context = if testnet_app_context.is_some() && mainnet_app_context.is_none() { - testnet_app_context.clone().unwrap() + testnet_app_context.clone().expect("Testnet app context is not available") } else { - mainnet_app_context.clone().unwrap() + mainnet_app_context.clone().expect("Mainnet app context is not available") };Alternatively, you can refactor the logic to handle the case when both contexts might be
None:let app_context = mainnet_app_context.clone() .or(testnet_app_context.clone()) .expect("No available app context (neither mainnet nor testnet)");
186-186:
⚠️ Potential issueHandle Potential
NoneValues incurrent_app_contextMethodThe method
current_app_contextuses.expect("expected mainnet")and.expect("expected testnet"), which can cause panics if the contexts areNone. It's advisable to handle theseOptionvalues more gracefully to prevent unintended crashes.Apply this diff to avoid unintentional panics:
match self.chosen_network { Network::Dash => { - self.mainnet_app_context.as_ref().expect("expected mainnet") + self.mainnet_app_context.as_ref().unwrap_or_else(|| { + panic!("Mainnet app context is not available") + }) }, Network::Testnet => { - self.testnet_app_context.as_ref().expect("expected testnet") + self.testnet_app_context.as_ref().unwrap_or_else(|| { + panic!("Testnet app context is not available") + }) }, Network::Devnet => todo!(), Network::Regtest => todo!(), _ => todo!(), }Consider changing the return type of
current_app_contexttoOption<&Arc<AppContext>>and handling theNonecase wherever this method is called to enhance robustness.Committable suggestion was skipped due to low confidence.
43-44: 💡 Codebase verification
⚠️ Potential issuePotential Panics Due to
.unwrap()and.expect()on OptionalAppContextFieldsInstances of
.unwrap()and.expect()found onmainnet_app_contextandtestnet_app_contextcan lead to runtime panics if these options areNone. It's recommended to handle these cases gracefully to ensure application stability.
- src/app.rs:43-44
- src/ui/network_chooser_screen.rs: [Multiple Lines]
🔗 Analysis chain
Ensure Proper Handling of Optional
AppContextThe fields
mainnet_app_contextandtestnet_app_contexthave been changed toOption<Arc<AppContext>>. It's crucial to handle these optional fields correctly throughout the code to prevent potentialNonedereferences that could cause panics.Run the following script to identify any instances where
.unwrap()or.expect()are called on these optional fields without prior checks:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of `.unwrap()` or `.expect()` on optional `AppContext` fields. rg 'self\.(mainnet|testnet)_app_context\.as_ref\(\)\.(unwrap|expect)\('Length of output: 650
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/app.rs (2)
Line range hint
169-173: Consider more graceful error handling in current_app_context().The current implementation uses
expect(), which will panic if the required context is missing. While this ensures that the application doesn't continue with an invalid state, it might lead to unexpected crashes.Consider a more graceful error handling approach:
pub fn current_app_context(&self) -> Result<&Arc<AppContext>, &'static str> { match self.chosen_network { Network::Dash => self.mainnet_app_context.as_ref().ok_or("Mainnet context not available"), Network::Testnet => self.testnet_app_context.as_ref().ok_or("Testnet context not available"), Network::Devnet => Err("Devnet not implemented"), Network::Regtest => Err("Regtest not implemented"), _ => Err("Unknown network"), } }This approach:
- Returns a
Result, allowing callers to handle missing contexts more flexibly.- Provides more informative error messages.
- Explicitly handles unimplemented network types.
You'll need to update the callers of this method to handle the
Result.
Line range hint
1-324: Summary of changes in src/app.rsThe modifications in this file successfully introduce the ability to load the app with only testnet configuration, aligning with the PR objective. Key changes include:
- Making
mainnet_app_contextoptional in theAppStatestruct.- Updating the initialization logic in
AppState::new()to handle optional contexts.- Adapting
NetworkChooserScreeninitialization for optional contexts.- Adjusting the
current_app_context()method to work with optional contexts.While these changes achieve the main goal, there are opportunities to improve error handling and robustness, particularly in the
AppState::new()andcurrent_app_context()methods. Implementing the suggested improvements will make the application more resilient to potential configuration issues.Consider implementing a more comprehensive error handling strategy throughout the application to gracefully manage scenarios where expected contexts are missing. This could involve creating custom error types and using the
Resulttype more extensively to propagate errors up the call stack, allowing for more informed decision-making at higher levels of the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/app.rs (4 hunks)
🧰 Additional context used
🔇 Additional comments (2)
src/app.rs (2)
118-121: LGTM: NetworkChooserScreen initialization adapted for optional contexts.The
NetworkChooserScreen::new()call has been correctly updated to pass optional references to contexts, aligning with the changes in theAppStatestructure. This change supports the possibility of missing contexts, which is consistent with the PR objective.
158-158: LGTM: Chosen network initialization aligned with available context.The
chosen_networkis now correctly initialized withapp_context.network. This change ensures consistency between the chosen network and the available context, which is important for the proper functioning of the application.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/config.rs (4)
Line range hint 63-67: Enhance error logging for .env file loading.
Consider using structured logging fields for better error tracking and analysis.
- tracing::error!(
- ?err,
- "Failed to load .env file. Continuing with environment variables."
- );
+ tracing::error!(
+ error = ?err,
+ file = ".env",
+ action = "load",
+ "Failed to load configuration file. Continuing with environment variables."
+ );Line range hint 70-94: Consider refactoring network config loading for better maintainability.
The current implementation duplicates the loading logic for mainnet and testnet configurations. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
+ fn load_network_config(prefix: &str, network: &str) -> Option<NetworkConfig> {
+ match envy::prefixed(prefix).from_env::<NetworkConfig>() {
+ Ok(config) => {
+ tracing::info!(network, "Network configuration loaded successfully");
+ Some(config)
+ }
+ Err(err) => {
+ tracing::error!(
+ error = ?err,
+ network,
+ "Failed to load network configuration"
+ );
+ None
+ }
+ }
+ }
+
pub fn load() -> Result<Self, ConfigError> {
if let Err(err) = dotenvy::from_path(".env") {
// ... existing .env loading code ...
}
- let mainnet_config = match envy::prefixed("MAINNET_").from_env::<NetworkConfig>() {
- Ok(config) => {
- tracing::info!("Mainnet configuration loaded successfully");
- Some(config)
- }
- Err(err) => {
- tracing::error!(?err, "Failed to load mainnet configuration");
- None
- }
- };
-
- let testnet_config = match envy::prefixed("TESTNET_").from_env::<NetworkConfig>() {
- Ok(config) => {
- tracing::info!("Testnet configuration loaded successfully");
- Some(config)
- }
- Err(err) => {
- tracing::error!(?err, "Failed to load testnet configuration");
- None
- }
- };
+ let mainnet_config = Self::load_network_config("MAINNET_", "mainnet");
+ let testnet_config = Self::load_network_config("TESTNET_", "testnet");Line range hint 96-104: Add configuration validation before returning.
Consider validating the loaded configurations using the existing is_valid() method before returning success.
if mainnet_config.is_none() && testnet_config.is_none() {
return Err(ConfigError::NoValidConfigs);
}
+ // Validate loaded configurations
+ if let Some(config) = &mainnet_config {
+ if !config.is_valid() {
+ tracing::error!(network = "mainnet", "Invalid configuration detected");
+ return Err(ConfigError::LoadError("Invalid mainnet configuration".into()));
+ }
+ }
+
+ if let Some(config) = &testnet_config {
+ if !config.is_valid() {
+ tracing::error!(network = "testnet", "Invalid configuration detected");
+ return Err(ConfigError::LoadError("Invalid testnet configuration".into()));
+ }
+ }
+
Ok(Config {
mainnet_config,
testnet_config,
})Line range hint 59-104: Document the partial configuration loading behavior.
The load method now supports partial configuration loading, but this behavior isn't documented. Consider adding documentation to explain this functionality.
+ /// Loads the configuration for all networks from environment variables and `.env` file.
+ ///
+ /// # Partial Loading
+ /// The method supports partial configuration loading, meaning it can succeed with only
+ /// mainnet or only testnet configuration present. This allows for testnet-only or
+ /// mainnet-only deployments.
+ ///
+ /// # Errors
+ /// Returns `ConfigError::NoValidConfigs` if neither mainnet nor testnet configurations
+ /// are successfully loaded.
+ ///
+ /// Returns `ConfigError::LoadError` if a loaded configuration is invalid.
pub fn load() -> Result<Self, ConfigError> {📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/app.rs (4 hunks)
- src/config.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app.rs
🔇 Additional comments (2)
src/config.rs (2)
Line range hint 22-27: Well-structured error handling implementation!
The ConfigError enum follows Rust best practices with clear error messages and appropriate variants for different failure scenarios.
Line range hint 59-60: Verify error handling in dependent code.
The load method now returns a Result instead of Self. Let's verify that all callers properly handle this change.
✅ Verification successful
All callers properly handle the Result return type
The verification shows that the only caller of Config::load() in src/context.rs already implements proper error handling through a match expression that:
- Handles the success case by using the config
- Handles the error case by printing the error and exiting the process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to Config::load to ensure proper error handling
rg -A 5 "Config::load\(\)"
Length of output: 329
Allow users to load the app with configs for testnet only (and no configs for mainnet)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor