v0.8.4-dev to master#190
Conversation
WalkthroughThis pull request introduces several new functions to modularize and generalize directory and cookie path handling for application data. It refactors user data directory resolution by adding dedicated functions for generic and DashCore-specific paths. Additionally, the PR refines configuration loading and authentication flows by streamlining error handling and unifying chain lock retrieval logic in the backend task module and context initialization. The changes also update provider initialization to support cookie-based authentication with a fallback to user/password methods. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Provider as Provider
participant CookieManager as CookieManager
participant Client as Client
App->>Provider: Initialize with (db, network, config)
Provider->>CookieManager: Retrieve cookie path (via core_cookie_path)
CookieManager-->>Provider: Return cookie path or error
alt Cookie exists
Provider->>Client: Attempt cookie-based authentication
Client-->>Provider: Authentication successful
else Cookie missing/invalid
Provider->>Client: Fallback to user/password authentication
Client-->>Provider: Authentication successful/failure
end
sequenceDiagram
participant AppContext as AppContext
participant Config as NetworkConfig
participant Client as Client
participant ErrorHandler as ErrorHandler
AppContext->>Config: Retrieve configuration
AppContext->>Client: Call get_best_chain_lock(config, network)
Client-->>AppContext: Return ChainLock or error
alt Successful lock retrieval
AppContext-->>Caller: Return ChainLock
else Error encountered
AppContext->>ErrorHandler: Handle error and report
ErrorHandler-->>AppContext: Error details
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 3
🧹 Nitpick comments (3)
src/context_provider.rs (1)
43-44: Preserve error context when mapping the core client creation error.
Currently,map_err(|e| e.to_string())?discards the original error type, which can lose valuable error context. Consider adopting an error type or more descriptive error messages.src/context.rs (1)
113-116: Consider more robust error handling instead of.expect().
Using.expect()here will panic on failure to create the client, which may not be ideal for production. Consider handling and returning the error gracefully or using a logging approach.src/backend_task/core/mod.rs (1)
97-139: Retain original client creation error for better debugging.
The code uses.map_err(|_| format!("Failed to create {} client", network.to_string()))?, which hides the underlying error detail. Preserving the original error message can greatly help debugging.- .map_err(|_| format!("Failed to create {} client", network.to_string()))?; + .map_err(|e| format!("Failed to create {} client: {}", network, e))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/app_dir.rs(1 hunks)src/backend_task/core/mod.rs(3 hunks)src/context.rs(2 hunks)src/context_provider.rs(3 hunks)src/app_dir.rs(2 hunks)src/backend_task/core/mod.rs(4 hunks)src/context.rs(4 hunks)src/context_provider.rs(2 hunks)src/backend_task/core/mod.rs(1 hunks)src/context.rs(2 hunks)src/app_dir.rs(1 hunks)src/backend_task/core/mod.rs(3 hunks)src/context.rs(3 hunks)src/context_provider.rs(2 hunks)
🔇 Additional comments (9)
src/context_provider.rs (1)
1-11: No major concerns.
The new imports look straightforward and are properly used in subsequent code.src/app_dir.rs (2)
1-4: No issues found.
Imports fromdirectoriesanddashcore::Networkare straightforward and correct.
26-44: Good approach for cross-platform user data directory detection.
The conditional code for Linux vs. other platforms is clear. The usage ofUserDirsis correct and will preserve the.dashcorefolder approach for Linux.src/context.rs (3)
1-1: No issues found.
The additional import is aligned with the usage ofcore_cookie_path.
74-74: Signature change to includenetworkis consistent with usage.
This looks good, ensuring the provider is aware of the active network.
90-90: Cookie path retrieval.
Usingcore_cookie_path(network, &network_config.devnet_name)centralizes path logic and is consistent with approach insrc/context_provider.rs.src/backend_task/core/mod.rs (3)
4-6: No major concerns.
The added imports forcore_cookie_path,core_user_data_dir_pathandNetworkConfigare consistent with the introduced logic.
57-58: Clean error handling for configuration loading.
Mapping the error into a concise message is appropriate here.
60-65: Integration for retrieving mainnet and testnet chain locks is succinct.
The approach to handle partial failures separately is clear and maintains continuity even if one network fails.
| pub fn new(db: Arc<Database>, network: Network, config: &NetworkConfig) -> Result<Self, String> { | ||
| let cookie_path = core_cookie_path(network, &config.devnet_name) | ||
| .expect("Failed to get core cookie path"); | ||
|
|
||
| // Read the cookie from disk | ||
| let cookie = std::fs::read_to_string(cookie_path); | ||
| let (user, pass) = if let Ok(cookie) = cookie { | ||
| // split the cookie at ":", first part is user (__cookie__), second part is password | ||
| let cookie_parts: Vec<&str> = cookie.split(':').collect(); | ||
| let user = cookie_parts[0]; | ||
| let password = cookie_parts[1]; | ||
| (user.to_string(), password.to_string()) | ||
| } else { | ||
| // Fall back to the pre-set user / pass if needed | ||
| ( | ||
| config.core_rpc_user.clone(), | ||
| config.core_rpc_password.clone(), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate cookie file content and handle potential errors.
The code inline-panics with .unwrap() for the user data directory, which may cause a crash if retrieving the home directory fails. Also, splitting on : might panic if there's no delimiter or the file is empty. Consider gracefully handling these errors:
Here is an example diff making error handling more robust:
- let cookie_path = core_user_data_dir_path().unwrap().join(".cookie");
+ let cookie_path = match core_user_data_dir_path() {
+ Ok(dir) => dir.join(".cookie"),
+ Err(e) => return Err(format!("Failed to get user data directory: {}", e)),
+ };
...
- let user = cookie_parts[0];
- let password = cookie_parts[1];
- (user.to_string(), password.to_string())
+ if cookie_parts.len() < 2 {
+ return Err("Invalid cookie format in .cookie file; expected 'user:password'".into());
+ }
+ (cookie_parts[0].to_string(), cookie_parts[1].to_string())Committable suggestion skipped: line range outside the PR's diff.
| pub fn core_cookie_path(network: Network, devnet_name: &Option<String>) -> Result<PathBuf, std::io::Error> { | ||
| core_user_data_dir_path().map(|path| { | ||
| let network_dir = match network { | ||
| Network::Dash => "", | ||
| Network::Testnet => "testnet3", | ||
| Network::Devnet => devnet_name.as_deref().unwrap_or(""), | ||
| Network::Regtest => "regtest", | ||
| _ => unimplemented!(), | ||
| }; | ||
| path.join(network_dir).join(".cookie") | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling unsupported networks gracefully.
Using unimplemented!() for unknown networks causes a runtime panic. Returning a descriptive error or fallback path would enhance stability.
- _ => unimplemented!(),
+ other => return Err(std::io::Error::new(
+ std::io::ErrorKind::Unsupported,
+ format!("Unsupported network variant: {:?}", other)
+ ))📝 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 core_cookie_path(network: Network, devnet_name: &Option<String>) -> Result<PathBuf, std::io::Error> { | |
| core_user_data_dir_path().map(|path| { | |
| let network_dir = match network { | |
| Network::Dash => "", | |
| Network::Testnet => "testnet3", | |
| Network::Devnet => devnet_name.as_deref().unwrap_or(""), | |
| Network::Regtest => "regtest", | |
| _ => unimplemented!(), | |
| }; | |
| path.join(network_dir).join(".cookie") | |
| }) | |
| pub fn core_cookie_path(network: Network, devnet_name: &Option<String>) -> Result<PathBuf, std::io::Error> { | |
| core_user_data_dir_path().map(|path| { | |
| let network_dir = match network { | |
| Network::Dash => "", | |
| Network::Testnet => "testnet3", | |
| Network::Devnet => devnet_name.as_deref().unwrap_or(""), | |
| Network::Regtest => "regtest", | |
| other => return Err(std::io::Error::new( | |
| std::io::ErrorKind::Unsupported, | |
| format!("Unsupported network variant: {:?}", other) | |
| )) | |
| }; | |
| path.join(network_dir).join(".cookie") | |
| }) | |
| } |
| "Failed to authenticate using .cookie file at {:?}, falling back to user/pass", | ||
| cookie_path, | ||
| ); | ||
| Client::new( |
There was a problem hiding this comment.
Hide sensitive credentials in logs.
Printing network_config.core_rpc_user or core_rpc_password can expose credentials in logs, representing a security risk. Mask or remove these values to avoid accidental leakage.
- eprintln!(
- "Failed to authenticate using .cookie file at {:?}, falling back to user/pass1, {:?}, {:?}",
- cookie_path,
- network_config.core_rpc_user.to_string(),
- network_config.core_rpc_password.to_string(),
- );
+ eprintln!(
+ "Failed to authenticate using .cookie file at {:?}, falling back to user/pass credentials",
+ cookie_path
+ );📝 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.
| "Failed to authenticate using .cookie file at {:?}, falling back to user/pass", | |
| cookie_path, | |
| ); | |
| Client::new( | |
| eprintln!( | |
| "Failed to authenticate using .cookie file at {:?}, falling back to user/pass credentials", | |
| cookie_path | |
| ); | |
| Client::new( |
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores