fix: update identity after registering name instead of reinserting#41
Conversation
WalkthroughThe pull request encompasses significant updates across multiple files, primarily enhancing the "Release Dash Evo Tool" workflow, restructuring dependencies, and introducing new functionalities for identity and wallet management. Key changes include the addition of permissions in workflow configurations, modifications to dependency management in 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.
🛑 Comments failed to post (87)
dash_core_configs/mainnet.conf (2)
3-4:
⚠️ Potential issueConsider externalizing credentials.
Having hardcoded credentials in configuration files is a security risk. Consider:
- Using environment variables
- Using a secure credential store
- At minimum, changing from the default password
6-7: 💡 Codebase verification
⚠️ Potential issueSecurity concern confirmed: ZMQ endpoints exposed on all network interfaces
The verification confirms that both mainnet and testnet configurations bind ZMQ endpoints to 0.0.0.0, exposing them to all network interfaces. This is particularly concerning because:
- Mainnet uses port 23708 for both txlocksig and chainlock endpoints
- Testnet uses port 23709 for both txlocksig and chainlock endpoints
- No other configuration files show more restrictive ZMQ bindings
- While RPC is properly restricted to 127.0.0.1/32, ZMQ endpoints remain openly accessible
Recommendations remain valid:
- Bind to 127.0.0.1 for local-only access
- Use specific IP for controlled remote access
- Implement network-level security controls
🔗 Analysis chain
Review ZMQ endpoint security configuration.
Binding to 0.0.0.0 exposes the ZMQ endpoints to all network interfaces, which could pose a security risk. Consider:
- Binding to localhost (127.0.0.1) if only local connections are needed
- Using a specific IP address if remote access is required
- Implementing proper firewall rules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any other ZMQ configurations that might be more restrictive rg "zmqpub.*=tcp://0\.0\.0\.0:" --type confLength of output: 74
Script:
#!/bin/bash # Search for ZMQ configurations in any file rg "zmqpub.*=tcp://(0\.0\.0\.0|127\.0\.0\.1|localhost)" # Also search for any config files that might contain ZMQ settings fd -e conf -e config -e cfg -x cat {} \;Length of output: 723
src/backend_task/identity/transfer.rs (2)
19-31: 🛠️ Refactor suggestion
Improve error handling and remove unnecessary clone.
- The error message could be more descriptive
- The
clone()at line 21 seems unnecessary as we're only reading the identityApply these improvements:
let remaining_balance = qualified_identity - .identity - .clone() + .identity .transfer_credits( &self.sdk, to_identifier, credits, id.and_then(|key_id| qualified_identity.identity.get_public_key_by_id(key_id)), qualified_identity.clone(), None, ) .await - .map_err(|e| format!("Withdrawal error: {}", e))?; + .map_err(|e| format!("Failed to transfer {} credits to identity {}: {}", + credits, to_identifier, e))?;📝 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.let remaining_balance = qualified_identity .identity .transfer_credits( &self.sdk, to_identifier, credits, id.and_then(|key_id| qualified_identity.identity.get_public_key_by_id(key_id)), qualified_identity.clone(), None, ) .await .map_err(|e| format!("Failed to transfer {} credits to identity {}: {}", credits, to_identifier, e))?;
32-38: 💡 Codebase verification
Based on the search results, I can now provide a final response:
Race condition concern is valid - synchronization is needed for concurrent transfers
The code analysis reveals:
- The transfer operation is async (
pub(super) async fn transfer_to_identity)- The identity state is shared across multiple async tasks (withdrawals, transfers, etc.)
- The wallet is protected by
Arc<RwLock<Wallet>>but the identity balance updates lack similar synchronization- Multiple concurrent tasks can modify the same identity's balance through
set_balance()Key locations requiring attention:
src/backend_task/identity/transfer.rs: Add synchronization around balance updatessrc/model/qualified_identity.rs: Consider adding a mutex for balance state🔗 Analysis chain
Consider potential race conditions in state updates.
The local state update happens after the transfer is complete. There could be a race condition if multiple transfers are happening concurrently.
Let's check if there are any concurrent access patterns to this method:
Consider implementing optimistic locking or using a mutex to protect the identity state if concurrent transfers are possible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for concurrent calls to transfer_to_identity # Expected: No direct concurrent access patterns # Check for async calls to this method rg -A 5 "transfer_to_identity.*await" # Check for any mutex or lock patterns around identity updates ast-grep --pattern 'Mutex<$_>'Length of output: 72
Script:
#!/bin/bash # Let's try a more comprehensive analysis: # 1. Find all references to transfer_to_identity rg "transfer_to_identity" -B 2 -A 2 # 2. Look for async/concurrent patterns in the identity module rg "async|tokio|concurrent|parallel" src/backend_task/identity/ # 3. Check how QualifiedIdentity is used in concurrent contexts rg "QualifiedIdentity" -B 2 -A 2 # 4. Look for synchronization primitives rg "Mutex|RwLock|sync::" src/backend_task/identity/ # 5. Check the struct definition containing transfer_to_identity ast-grep --pattern 'impl $_ { $$$ fn transfer_to_identity($$$) { $$$ } $$$ }'Length of output: 29099
src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs (2)
43-45:
⚠️ Potential issueAdd proper error handling for amount parsing.
The current implementation silently returns on parse error without providing feedback to the user.
Consider applying this improvement:
- let Ok(amount_dash) = self.funding_amount.parse::<f64>() else { - return action; - }; + let amount_dash = match self.funding_amount.parse::<f64>() { + Ok(amount) if amount <= 0.0 => { + self.error_message = Some("Amount must be greater than 0".to_string()); + return action; + } + Ok(amount) => amount, + Err(_) => { + self.error_message = Some("Invalid amount format".to_string()); + return action; + } + };Committable suggestion skipped: line range outside the PR's diff.
8-22: 🛠️ Refactor suggestion
Consider improving error handling and number formatting.
The current implementation has several areas that could be made more robust:
- The
unwrap()on RwLock could panic if the lock is poisoned- The balance formatting might truncate significant digits
- No error handling for
max_balance()callConsider applying these improvements:
fn show_wallet_balance(&self, ui: &mut egui::Ui) { if let Some(selected_wallet) = &self.selected_wallet { - let wallet = selected_wallet.read().unwrap(); // Read lock on the wallet + let wallet = match selected_wallet.read() { + Ok(wallet) => wallet, + Err(e) => { + ui.label(format!("Error accessing wallet: {}", e)); + return; + } + }; - let total_balance: u64 = wallet.max_balance(); // Sum up all the balances + let total_balance: u64 = match wallet.max_balance() { + Ok(balance) => balance, + Err(e) => { + ui.label(format!("Error getting balance: {}", e)); + return; + } + }; let dash_balance = total_balance as f64 * 1e-8; // Convert to DASH units ui.horizontal(|ui| { - ui.label(format!("Wallet Balance: {:.8} DASH", dash_balance)); + // Use scientific notation for very small amounts + if dash_balance < 0.00000001 { + ui.label(format!("Wallet Balance: {:e} DASH", dash_balance)); + } else { + ui.label(format!("Wallet Balance: {:.8} DASH", dash_balance)); + } }); } else { ui.label("No wallet selected"); } }Committable suggestion skipped: line range outside the PR's diff.
src/backend_task/contested_names/query_ending_times.rs (1)
19-20: 💡 Codebase verification
Based on the search results, I can now provide a final response. The codebase shows a clear pattern of error handling through the TaskResult sender channel:
The sender channel should be used to report errors and progress
Looking at similar implementations in the codebase:
- Other methods use the sender to report errors:
sender.send(TaskResult::Error(e))- They also use it to report progress:
sender.send(TaskResult::Refresh)- The method should follow this pattern instead of returning errors directly
Example locations showing this pattern:
query_dpns_contested_resources.rs: Uses sender for both errors and refresh notificationsrefresh_identity.rs: Sends refresh notifications through the channelregister_identity.rs: Reports both success and errors through the channel🔗 Analysis chain
Consider implementing error reporting through the sender channel.
The
senderparameter is marked as unused, but given it's aTaskResultchannel, it might be intended for error reporting. Consider using it to communicate errors and status updates to the caller, preventing silent failures.Let's check how other similar methods handle task results:
Consider these improvements:
- Use the sender to report errors instead of just returning them
- Send progress updates for long-running operations
- Report successful completion through the channel
Example usage:
sender.send(TaskResult::Error(format!("error querying vote poll end times: {}", e))).await?; // or sender.send(TaskResult::Success(BackendTaskSuccessResult::ContestEndTimeUpdated)).await?;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find similar backend task methods to understand the error handling pattern # Look for methods that take TaskResult sender as parameter ast-grep --pattern 'fn $NAME($$$, sender: mpsc::Sender<TaskResult>$$$) $$$'Length of output: 77
Script:
#!/bin/bash # Let's try a broader search to find TaskResult usage patterns rg "TaskResult" -A 5 -B 5 # Also search for Sender usage patterns rg "mpsc::Sender" -A 5 -B 5 # And look for any error handling patterns in backend tasks fd "task" --type f --exec rg "Error|send\(" {} -A 3 -B 3Length of output: 35205
src/config.rs (2)
63-64: 🛠️ Refactor suggestion
Consider improving error handling for path creation.
While using
app_user_data_file_pathis a good improvement, the current implementation has two concerns:
- Using
.expect()could cause runtime panics if path creation fails- The error message "should create .env file path" is not very descriptive
Consider handling the error case explicitly:
- let env_file_path = app_user_data_file_path(".env").expect("should create .env file path"); - if let Err(err) = dotenvy::from_path(env_file_path) { + let env_file_path = match app_user_data_file_path(".env") { + Ok(path) => path, + Err(err) => { + tracing::error!(?err, "Failed to create .env file path"); + return Err(ConfigError::LoadError("Failed to create .env file path".into())); + } + }; + if let Err(err) = dotenvy::from_path(&env_file_path) {Committable suggestion skipped: line range outside the PR's diff.
63-64: 💡 Codebase verification
File permissions for .env need to be explicitly managed
The codebase currently lacks explicit file permission management when creating or copying the .env file. While the directory permissions are verified, the sensitive configuration file itself doesn't have controlled permissions:
- In
app_dir.rs,copy_env_file_if_not_exists()usesfs::copywithout setting restrictive permissions- No file permission controls are implemented after copying the .env file
- The .env file may contain sensitive data (RPC credentials, wallet keys) but remains with default permissions
Recommendation:
- Add explicit file permission setting (e.g., 600) after creating or copying the .env file to ensure it's only readable by the owner
🔗 Analysis chain
Verify .env file permissions.
Since the configuration file contains sensitive data (RPC credentials, wallet private keys), ensure that the .env file has appropriate file permissions when created.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the .env file has secure permissions # Expected: File should be readable only by the owner # Find .env files in the repository fd -H "\.env$" -x stat -c "%a %n" {} \;Length of output: 44
Script:
#!/bin/bash # Let's check how the .env file permissions are handled in the code # Search for file permission related code around .env file creation/handling # First, let's find the app_user_data_file_path function implementation ast-grep --pattern 'fn app_user_data_file_path' # Also search for any file permission related code rg -A 3 "set_mode|chmod|permissions|fs::create|fs::write"Length of output: 393
Script:
#!/bin/bash # Let's examine the app_dir.rs file more thoroughly to understand the file handling cat src/app_dir.rs # Also check if there are any other files that handle .env file operations rg -l "\.env"Length of output: 2291
src/ui/components/left_panel.rs (1)
40-40: 💡 Codebase verification
Remove unused
_app_contextparameterThe
app_contextparameter inadd_left_panelfunction is unused and can be safely removed. The search results show that whileAppContextis widely used across other UI components, this particular function doesn't utilize the parameter at all. There are no references toapp_contextwithin the function body, and the underscore prefix confirms it's intentionally marked as unused.
- Remove the
_app_context: &Arc<AppContext>parameter from theadd_left_panelfunction signature insrc/ui/components/left_panel.rs🔗 Analysis chain
Verify if
_app_contextparameter can be removed.The parameter is marked as unused with an underscore prefix. Consider removing it if it's no longer needed, or document why it's retained for future use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if app_context is used in similar UI components # This helps determine if removing it would be consistent with the codebase # Search for other UI components using AppContext rg "fn.*&Arc<AppContext>" "src/ui/components/"Length of output: 134
Script:
#!/bin/bash # Let's try a more comprehensive search for AppContext usage patterns # 1. Search for struct definitions containing AppContext rg "struct.*AppContext" --type rust # 2. Search for function parameters containing AppContext with better pattern rg "fn.*AppContext" --type rust # 3. Look specifically for the LeftPanel struct definition and impl rg "struct LeftPanel|impl LeftPanel" --type rust -A 5Length of output: 2672
Script:
#!/bin/bash # Let's look for the LeftPanel implementation and its usage of AppContext # 1. Search for the file content to understand the context better rg -l "LeftPanel" --type rust | xargs cat # 2. Search for any references to app_context within the same file rg "app_context" "src/ui/components/left_panel.rs" -C 2Length of output: 227
src/ui/components/left_wallet_panel.rs (1)
14-36: 🛠️ Refactor suggestion
Improve error handling and add documentation.
The function needs proper documentation and should use a proper logging system instead of
eprintln!.+/// Loads an icon from embedded assets and converts it to a texture handle. +/// +/// # Arguments +/// * `ctx` - The egui context for texture loading +/// * `path` - The path to the icon file in embedded assets +/// +/// # Returns +/// * `Some(TextureHandle)` if the icon was successfully loaded +/// * `None` if the icon couldn't be found or loaded fn load_icon(ctx: &Context, path: &str) -> Option<TextureHandle> { // Attempt to retrieve the embedded file if let Some(content) = Assets::get(path) { // Load the image from the embedded bytes if let Ok(image) = image::load_from_memory(&content.data) { let size = [image.width() as usize, image.height() as usize]; let rgba_image = image.into_rgba8(); let pixels = rgba_image.into_raw(); Some(ctx.load_texture( path, egui::ColorImage::from_rgba_unmultiplied(size, &pixels), Default::default(), )) } else { - eprintln!("Failed to load image from embedded data at path: {}", path); + log::error!("Failed to load image from embedded data at path: {}", path); None } } else { - eprintln!("Image not found in embedded assets at path: {}", path); + log::error!("Image not found in embedded assets at path: {}", path); None } }📝 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./// Loads an icon from embedded assets and converts it to a texture handle. /// /// # Arguments /// * `ctx` - The egui context for texture loading /// * `path` - The path to the icon file in embedded assets /// /// # Returns /// * `Some(TextureHandle)` if the icon was successfully loaded /// * `None` if the icon couldn't be found or loaded fn load_icon(ctx: &Context, path: &str) -> Option<TextureHandle> { // Attempt to retrieve the embedded file if let Some(content) = Assets::get(path) { // Load the image from the embedded bytes if let Ok(image) = image::load_from_memory(&content.data) { let size = [image.width() as usize, image.height() as usize]; let rgba_image = image.into_rgba8(); let pixels = rgba_image.into_raw(); Some(ctx.load_texture( path, egui::ColorImage::from_rgba_unmultiplied(size, &pixels), Default::default(), )) } else { log::error!("Failed to load image from embedded data at path: {}", path); None } } else { log::error!("Image not found in embedded assets at path: {}", path); None } }src/ui/identities/register_dpns_name_screen.rs (1)
142-146: 💡 Codebase verification
Based on the gathered information, I can now provide a final response:
Error handling pattern needs correlation across multiple UI components
The investigation reveals this is a systemic pattern across multiple UI components (
add_key_screen.rs,add_existing_identity_screen.rs, andregister_dpns_name_screen.rs), all sharing the same potential race condition issue with error messages. Each component:
- Uses a similar status enum pattern with
ErrorMessagevariant- Has identical comments warning about message attribution issues
- Lacks message correlation with specific operations
Recommendations:
- Introduce a correlation ID or operation context in the
MessageTypeenum- Update all affected components:
- src/ui/add_key_screen.rs
- src/ui/identities/add_existing_identity_screen.rs
- src/ui/identities/register_dpns_name_screen.rs
🔗 Analysis chain
Address potential race condition in error handling.
The comment indicates that error messages could be misattributed if multiple processes are running. Consider:
- Adding message correlation IDs to track which operation generated the error
- Scoping error handlers to specific operations
Let's check if there are similar patterns in other UI components:
These scripts will help us understand:
- How errors are handled across UI components
- The structure of the status enum to see available states
- The broader message handling context in this specific file
This will help us understand:
- The structure of error handling states
- Where and how messages are triggered to better assess the race condition concern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar error handling patterns in UI components rg -A 5 "MessageType::Error.*ErrorMessage" "src/ui/"Length of output: 52
Script:
#!/bin/bash # Let's search for error handling patterns more broadly rg -A 5 "MessageType::Error =>" src/ui/Length of output: 2643
Script:
#!/bin/bash # Let's also check how errors are handled in the status enum ast-grep --pattern 'enum RegisterDpnsNameStatus { $$$ }'Length of output: 60
Script:
#!/bin/bash # Search for any message handling in the file rg -A 10 "message" src/ui/identities/register_dpns_name_screen.rsLength of output: 982
Script:
#!/bin/bash # Let's check how the status enums are defined in these files rg -B 5 "ErrorMessage" src/ui/identities/register_dpns_name_screen.rs # Let's also check where the messages originate from rg -B 5 "display_message.*MessageType" src/ui/identities/register_dpns_name_screen.rsLength of output: 1055
src/ui/withdrawals/mod.rs (1)
79-85: 🛠️ Refactor suggestion
Improve max amount calculation implementation.
The current implementation has several concerns:
- The magic number
30000should be defined as a named constant with documentation explaining its purpose- The floating-point conversion factor
1e-8should be documented- The reason for flooring to 4 decimal places should be explained
- There's no validation for negative amounts after subtraction
- Floating-point operations might lead to precision loss
Consider applying these improvements:
+ // Reserve 30000 satoshis for transaction fees + const TRANSACTION_FEE_RESERVE: u64 = 30000; + // Convert satoshis to Dash (1 Dash = 100000000 satoshis) + const SATOSHI_TO_DASH: f64 = 1e-8; + // Maximum decimal places for Dash amounts + const MAX_DECIMAL_PLACES: u32 = 4; - let expected_max_amount = self.max_amount.saturating_sub(30000) as f64 * 1e-8; + // Calculate maximum withdrawal amount by reserving transaction fees + let reserved_amount = self.max_amount.saturating_sub(TRANSACTION_FEE_RESERVE); + if reserved_amount == 0 { + self.withdrawal_amount = "0".to_string(); + return; + } + let expected_max_amount = reserved_amount as f64 * SATOSHI_TO_DASH; - // Use flooring and format the result with 4 decimal places - let floored_amount = (expected_max_amount * 10_000.0).floor() / 10_000.0; + // Floor to 4 decimal places to ensure valid Dash amount + let scale = 10f64.powi(MAX_DECIMAL_PLACES as i32); + let floored_amount = (expected_max_amount * scale).floor() / scale; - // Set the withdrawal amount to the floored value formatted as a string - self.withdrawal_amount = format!("{:.4}", floored_amount); + self.withdrawal_amount = format!("{:.4}", floored_amount);📝 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.// Reserve 30000 satoshis for transaction fees const TRANSACTION_FEE_RESERVE: u64 = 30000; // Convert satoshis to Dash (1 Dash = 100000000 satoshis) const SATOSHI_TO_DASH: f64 = 1e-8; // Maximum decimal places for Dash amounts const MAX_DECIMAL_PLACES: u32 = 4; // Calculate maximum withdrawal amount by reserving transaction fees let reserved_amount = self.max_amount.saturating_sub(TRANSACTION_FEE_RESERVE); if reserved_amount == 0 { self.withdrawal_amount = "0".to_string(); return; } let expected_max_amount = reserved_amount as f64 * SATOSHI_TO_DASH; // Floor to 4 decimal places to ensure valid Dash amount let scale = 10f64.powi(MAX_DECIMAL_PLACES as i32); let floored_amount = (expected_max_amount * scale).floor() / scale; self.withdrawal_amount = format!("{:.4}", floored_amount);src/backend_task/core/mod.rs (1)
13-13: 🛠️ Refactor suggestion
Avoid passing
Arc<RwLock<Wallet>>by valuePassing
Arc<RwLock<Wallet>>by value can lead to unnecessary cloning of theArc. Consider passing it by reference to avoid the overhead.Apply this diff to pass the wallet reference:
pub(crate) enum CoreTask { GetBestChainLock, - RefreshWalletInfo(Arc<RwLock<Wallet>>), + RefreshWalletInfo(&'a Arc<RwLock<Wallet>>), }Note: You may need to introduce a lifetime parameter
'ato accommodate the reference.Committable suggestion skipped: line range outside the PR's diff.
src/app_dir.rs (2)
34-39:
⚠️ Potential issueFix incorrect usage of
std::path::is_separatorin filename validationThe condition
filename.contains(std::path::is_separator)is incorrect becausestd::path::is_separatoris a function, not a pattern. This leads to a type mismatch error. To check if the filename contains any path separator characters, iterate over the characters using.any().Apply this diff to fix the issue:
-if filename.is_empty() || filename.contains(std::path::is_separator) { +if filename.is_empty() || filename.chars().any(std::path::is_separator) {📝 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 filename.is_empty() || filename.chars().any(std::path::is_separator) { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, "Invalid filename", )); }
44-60: 🛠️ Refactor suggestion
Refactor to handle errors without panicking and improve readability
The function
copy_env_file_if_not_existsusesexpect, which will cause the program to panic on errors. It's better practice to return aResultand let the caller handle any errors. Additionally, the emptyifblock at lines 48-49 can be refactored for clarity.Apply the following changes:
Change the function signature to return a
Result:-pub fn copy_env_file_if_not_exists() { +pub fn copy_env_file_if_not_exists() -> Result<(), std::io::Error> {Replace
expectwith the?operator to propagate errors:-let app_data_dir = - app_user_data_dir_path().expect("Failed to determine application data directory"); +let app_data_dir = app_user_data_dir_path()?; ... -fs::copy(&env_example_file_in_exe_dir, env_file_in_app_dir) - .expect("Failed to copy main net env file"); +fs::copy(&env_example_file_in_exe_dir, &env_file_in_app_dir)?; ... -fs::copy(&env_file_in_exe_dir, env_file_in_app_dir) - .expect("Failed to copy main net env file"); +fs::copy(&env_file_in_exe_dir, &env_file_in_app_dir)?;Refactor the
ifcondition to avoid the empty block and improve readability:-if env_file_in_app_dir.exists() && env_file_in_app_dir.is_file() { -} else { +if !env_file_in_app_dir.exists() || !env_file_in_app_dir.is_file() { // Proceed with copying the fileEnsure the function returns
Ok(())at the end:+} +Ok(())The refactored function will handle errors gracefully and enhance code clarity.
📝 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 copy_env_file_if_not_exists() -> Result<(), std::io::Error> { let app_data_dir = app_user_data_dir_path()?; let env_file_in_app_dir = app_data_dir.join(".env".to_string()); if !env_file_in_app_dir.exists() || !env_file_in_app_dir.is_file() { let env_example_file_in_exe_dir = PathBuf::from(".env.example"); if env_example_file_in_exe_dir.exists() && env_example_file_in_exe_dir.is_file() { fs::copy(&env_example_file_in_exe_dir, &env_file_in_app_dir)?; } else { let env_file_in_exe_dir = PathBuf::from(".env"); fs::copy(&env_file_in_exe_dir, &env_file_in_app_dir)?; } } Ok(()) }src/backend_task/core/refresh_wallet_info.rs (4)
10-13: 🛠️ Refactor suggestion
Convert
refresh_wallet_infoto an asynchronous functionThe
refresh_wallet_infofunction performs network I/O operations, such as fetching balances from Dash Core and reloading UTXOs, which are potentially blocking. To prevent blocking the thread and improve performance, consider converting this function toasyncand updating its calls accordingly.
37-38: 🛠️ Refactor suggestion
Avoid holding write locks during potentially blocking operations
Currently, the write lock on the wallet is held while updating the address balance. If
update_address_balanceinvolves I/O or other blocking operations, holding the lock can lead to performance issues or deadlocks. Consider minimizing the duration of the lock by preparing data beforehand and acquiring the lock only when necessary for the update.
49-56: 🛠️ Refactor suggestion
Minimize lock duration during UTXO reloading
The write lock on the wallet is held during the call to
reload_utxos, which may perform network I/O or intensive computations. Holding the lock for the entire duration can hinder concurrent access. Refactor to release the lock before performing the reload and acquire it only when updating the wallet state with the new UTXOs.
16-16: 🛠️ Refactor suggestion
Avoid converting errors to
String; use structured error handlingConverting errors to
Stringwith.to_string()and returningResult<_, String>loses valuable context and makes error handling less precise. Implement a custom error type or use an existing error handling crate likethiserrororanyhowto provide richer error information and maintain the error chain.Also applies to: 37-37, 49-49, 63-63, 73-73
src/ui/identities/add_new_identity_screen/by_using_unused_asset_lock.rs (4)
60-60: 🛠️ Refactor suggestion
Avoid potential panic when accessing
proofUsing
.expect("Asset lock proof is required")can cause a panic ifproofisNone. Even if you expectproofto always beSome, it's safer to handle theOptionexplicitly to prevent unexpected panics.Apply this diff to handle the
Nonecase:- proof.clone().expect("Asset lock proof is required"), + match proof.clone() { + Some(proof) => proof, + None => { + ui.label("Asset lock proof is missing."); + return; + } + },Committable suggestion skipped: line range outside the PR's diff.
65-65: 🛠️ Refactor suggestion
Handle potential poisoning of
self.steplockUsing
.unwrap()onself.step.write()may lead to a panic if the lock is poisoned. Consider handling thePoisonErrorto improve the program's resilience.Apply this diff to handle potential poisoning:
- let mut step = self.step.write().unwrap(); + let mut step = match self.step.write() { + Ok(step) => step, + Err(poisoned) => { + ui.label("Failed to update step due to lock poisoning."); + return; + } + };Committable suggestion skipped: line range outside the PR's diff.
16-16: 🛠️ Refactor suggestion
Handle potential poisoning of
selected_walletlockUsing
.unwrap()onselected_wallet.read()can lead to a panic if the lock is poisoned. To enhance robustness, consider handling thePoisonErrorto gracefully manage such situations.Apply this diff to handle potential poisoning:
- let wallet = selected_wallet.read().unwrap(); + let wallet = match selected_wallet.read() { + Ok(wallet) => wallet, + Err(poisoned) => { + ui.label("Failed to access wallet due to lock poisoning."); + return; + } + };Committable suggestion skipped: line range outside the PR's diff.
83-83: 🛠️ Refactor suggestion
Handle potential poisoning when reading
self.stepUsing
.unwrap()onself.step.read()can cause a panic if the lock is poisoned. To ensure robustness, handle thePoisonErrorappropriately.Apply this diff:
- let step = self.step.read().unwrap().clone(); + let step = match self.step.read() { + Ok(step) => step.clone(), + Err(poisoned) => { + ui.label("Failed to read current step due to lock poisoning."); + return AppAction::None; + } + };Committable suggestion skipped: line range outside the PR's diff.
src/model/wallet/encryption.rs (1)
29-35: 🛠️ Refactor suggestion
Specify Argon2 parameters explicitly for enhanced security
Relying on
Argon2::default()uses default parameters that may not meet your security requirements. It's recommended to specify Argon2 parameters explicitly to control the computation cost and strengthen the key derivation process.Consider applying this diff to set explicit Argon2 parameters:
use argon2::{self, Argon2}; +use argon2::{Algorithm, Params, Version}; fn derive_key(password: &str, salt: &[u8]) -> Result<Vec<u8>, String> { let key_length = 32; // For AES-256, we use a 256-bit key (32 bytes) let mut key = vec![0u8; key_length]; - // Using Argon2 with default parameters - let argon2 = Argon2::default(); + // Using Argon2 with explicit parameters + let params = Params::new( + 65536, // Memory size in KiB (e.g., 64 MiB) + 3, // Number of iterations + 1, // Parallelism + None, // Optional length of the output key + ).map_err(|e| e.to_string())?; + let argon2 = Argon2::new(Algorithm::Argon2id, Version::V0x13, params); // Deriving the key argon2 .hash_password_into(password.as_bytes(), salt, &mut key)📝 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.use argon2::{Algorithm, Params, Version}; // Using Argon2 with explicit parameters let params = Params::new( 65536, // Memory size in KiB (e.g., 64 MiB) 3, // Number of iterations 1, // Parallelism None, // Optional length of the output key ).map_err(|e| e.to_string())?; let argon2 = Argon2::new(Algorithm::Argon2id, Version::V0x13, params); // Deriving the key argon2 .hash_password_into(password.as_bytes(), salt, &mut key) .map_err(|e| e.to_string())?;src/database/initialization.rs (3)
12-13:
⚠️ Potential issueEnsure 'start_root_screen' and 'database_version' columns are properly initialized
Adding
NOT NULLcolumnsstart_root_screenanddatabase_versionwithout default values may cause issues if thesettingstable already exists. Confirm that when inserting into thesettingstable, values for these columns are always provided to prevent runtime errors.
21-28:
⚠️ Potential issueChanging primary key to 'seed_hash' requires data migration
The
wallettable's primary key has been changed fromseedtoseed_hash, and new columns have been added. Ensure that data migration strategies are in place to handle existing data and update primary keys without data loss.
38-45:
⚠️ Potential issueUpdate foreign key references and migrate data for 'wallet_addresses' table
Changing the primary key and foreign key references from
seedtoseed_hashin thewallet_addressestable necessitates careful data migration. Verify that all references are updated accordingly and that existing data maintains integrity.src/model/wallet/utxos.rs (2)
108-109:
⚠️ Potential issueCorrect error handling for
Address::from_scriptreturningOption<Address>.The method
Address::from_scriptreturns anOption<Address>, not aResult. Usingmap_erron anOptionis incorrect. Instead, useok_or_elseto convert theOptionto aResultwith an error message.Apply this diff to fix the error handling:
-let address = Address::from_script(&tx_out.script_pubkey, network) - .map_err(|e| e.to_string())?; +let address = Address::from_script(&tx_out.script_pubkey, network) + .ok_or_else(|| "Failed to derive address from script_pubkey".to_string())?;📝 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.let address = Address::from_script(&tx_out.script_pubkey, network) .ok_or_else(|| "Failed to derive address from script_pubkey".to_string())?;
130-131:
⚠️ Potential issueCorrect error handling for
Address::from_scriptreturningOption<Address>.The same issue exists here when deriving the address from
tx_out.script_pubkey. Useok_or_elseto handle theOptionproperly.Apply this diff to fix the error handling:
-let address = Address::from_script(&tx_out.script_pubkey, network) - .map_err(|e| e.to_string())?; +let address = Address::from_script(&tx_out.script_pubkey, network) + .ok_or_else(|| "Failed to derive address from script_pubkey".to_string())?;📝 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.let address = Address::from_script(&tx_out.script_pubkey, network) .ok_or_else(|| "Failed to derive address from script_pubkey".to_string())?;src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs (2)
16-17:
⚠️ Potential issueAvoid using floating-point types (
f64) for currency amounts to prevent rounding errorsUsing
f64to represent currency amounts can introduce precision and rounding errors, which is critical in financial applications. It's recommended to use integer types representing the smallest currency unit (e.g., duffs for Dash) or a fixed-point decimal type to ensure accuracy.Apply this diff to change
amountto an integer type representing duffs:- fn render_qr_code(&mut self, ui: &mut egui::Ui, amount: f64) -> Result<(), String> { + fn render_qr_code(&mut self, ui: &mut egui::Ui, amount_duffs: u64) -> Result<(), String> { ... - let Ok(amount_dash) = self.funding_amount.parse::<f64>() else { + let Ok(amount_duffs) = self.funding_amount.parse::<u64>() else { ... - let pay_uri = format!("{}?amount={:.4}", address.to_qr_uri(), amount); + let pay_uri = format!("{}?amount={}", address.to_qr_uri(), amount_duffs);Also applies to: 78-79, 122-124
23-23: 🛠️ Refactor suggestion
Handle potential panics from
unwrap()onRwLockUsing
unwrap()onRwLockcan cause a panic if the lock is poisoned due to a previous thread panic. Consider handling thePoisonErrorto prevent unexpected panics in your application.Apply this diff to handle the error:
- let mut wallet = wallet_guard.write().unwrap(); + let mut wallet = wallet_guard.write().map_err(|e| e.to_string())?; ... - let step = self.step.read().unwrap().clone(); + let step = self.step.read().map_err(|e| e.to_string())?.clone(); ... - let mut step = self.step.write().unwrap(); + let mut step = self.step.write().map_err(|e| e.to_string())?;Also applies to: 120-120, 170-170
src/backend_task/identity/register_dpns_name.rs (2)
163-163: 🛠️ Refactor suggestion
Review the hardcoded limit in DPNS names query
The query sets a hardcoded
limitof 100. If an identity owns more than 100 DPNS names, some names may not be retrieved. Consider implementing pagination or increasing the limit to ensure all names are fetched.
176-176:
⚠️ Potential issueHandle potential errors in string conversion explicitly
Using
unwrap_or_default()inlabel.to_str().unwrap_or_default()may silently ignore errors during string conversion, potentially leading to empty strings without indication of failure. It's better to handle possible errors explicitly.Apply this diff to handle the error explicitly:
- .map(|label| label.to_str().unwrap_or_default()); + .and_then(|label| label.to_str().ok());This change ensures that if
to_str()fails, it returnsNone, which will be properly handled in the subsequent logic.📝 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..and_then(|label| label.to_str().ok());src/database/identities.rs (1)
83-86:
⚠️ Potential issuePrevent cross-network updates by including
networkin theWHEREclause and avoid updating immutablenetworkfieldThe
UPDATEstatement inupdate_local_qualified_identitycurrently modifies thenetworkfield and filters only byid. Identities may exist with the sameidacross different networks. To prevent unintended updates to identities in other networks, includenetworkin theWHEREclause. Additionally, thenetworkfield is typically immutable and should not be updated.Apply this diff to correct the SQL statement:
self.execute( - "UPDATE identity - SET data = ?, alias = ?, identity_type = ?, network = ?, is_local = 1 - WHERE id = ?", - params![data, alias, identity_type, network, id], + "UPDATE identity + SET data = ?, alias = ?, identity_type = ?, is_local = 1 + WHERE id = ? AND network = ?", + params![data, alias, identity_type, id, network], )?;📝 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."UPDATE identity SET data = ?, alias = ?, identity_type = ?, is_local = 1 WHERE id = ? AND network = ?", params![data, alias, identity_type, id, network],src/database/asset_lock_transaction.rs (3)
48-48: 🛠️ Refactor suggestion
Ensure consistent
txidparameter types across methodsThe
txidparameter varies between&strand&[u8]in different methods:
&stringet_asset_lock_transaction(line 48),update_asset_lock_chain_locked_height(line 85), anddelete_asset_lock_transaction(line 134)&[u8]inset_asset_lock_identity_id(line 101) andset_asset_lock_identity_id_before_confirmation_by_network(line 119)For consistency and to reduce potential errors, consider standardizing the
txidparameter type across all methods.Also applies to: 85-85, 101-101, 119-119, 134-134
65-66: 🛠️ Refactor suggestion
Improve error handling during deserialization
When deserializing transaction data and InstantLock data, errors are currently mapped to
rusqlite::Error::InvalidQuery, which may not accurately represent the cause of the error (e.g., data corruption or invalid format). Consider preserving the original error or creating a more descriptive error type to provide better context for debugging.Also applies to: 175-176, 222-223
26-26:
⚠️ Potential issueHandle potential
PoisonErrorwhen acquiring database connectionAcross several methods, the database connection is acquired using
self.conn.lock().unwrap(). Usingunwrap()can cause a panic if the mutex is poisoned. To improve robustness, consider handling thePoisonErrorexplicitly or usingexpect()with a descriptive error message to prevent unexpected panics.Also applies to: 50-50, 88-88, 104-104, 122-122, 134-134, 156-156, 204-204
src/ui/transfers/mod.rs (1)
142-148:
⚠️ Potential issueHandle Potential Overflow When Casting Credits
Casting
creditstoCredits(which might be a smaller type likeu64) without checking could result in an overflow for large amounts.Consider adding a check to ensure
creditsdoes not exceedu64::MAXbefore casting, and inform the user if the amount is too large.src/model/wallet/asset_lock_transaction.rs (2)
159-160:
⚠️ Potential issueEnsure
output_amountis not negative or zeroCalculating
output_amountasprevious_tx_output.value - feemay result in a negative or zero value ifprevious_tx_output.valueis less than or equal tofee. This would create an invalid transaction.Add a check to validate the
previous_tx_output.value:+if previous_tx_output.value <= fee { + return Err("Previous transaction output value is insufficient to cover the fee.".to_string()); +} let output_amount = previous_tx_output.value - fee;📝 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 previous_tx_output.value <= fee { return Err("Previous transaction output value is insufficient to cover the fee.".to_string()); } let output_amount = previous_tx_output.value - fee;
140-240: 🛠️ Refactor suggestion
Refactor duplicated code between asset lock transaction methods
The new method
asset_lock_transaction_for_utxoshares significant code with the existingasset_lock_transactionmethod. To improve maintainability and reduce redundancy, consider extracting the common logic into a shared helper function.Apply this refactor to extract common functionality:
+// Extract common logic into a helper function +fn build_asset_lock_transaction( + &self, + network: Network, + inputs: Vec<TxIn>, + previous_tx_outputs: Vec<TxOut>, + asset_lock_public_key: &PublicKey, +) -> Result<Transaction, String> { + let fee = 3_000; + let payload_output = TxOut { + value: /* calculate value based on inputs and fee */, + script_pubkey: ScriptBuf::new_p2pkh(&asset_lock_public_key.pubkey_hash()), + }; + let burn_output = TxOut { + value: payload_output.value, + script_pubkey: ScriptBuf::new_op_return(&[]), + }; + let payload = AssetLockPayload { + version: 1, + credit_outputs: vec![payload_output.clone()], + }; + + let tx = Transaction { + version: 3, + lock_time: 0, + input: inputs.clone(), + output: vec![burn_output], + special_transaction_payload: Some(TransactionPayload::AssetLockPayloadType(payload)), + }; + + // Signing logic + // ... + + Ok(tx) +} + // In `asset_lock_transaction` -// Existing code to prepare inputs and outputs +let tx = self.build_asset_lock_transaction( + network, + inputs, + vec![/* previous_tx_outputs */], + &asset_lock_public_key, +)?; // In `asset_lock_transaction_for_utxo` -// Existing code to prepare input and outputs +let tx = self.build_asset_lock_transaction( + network, + vec![tx_in], + vec![previous_tx_output], + &asset_lock_public_key, +)?;Committable suggestion skipped: line range outside the PR's diff.
src/components/core_zmq_listener.rs (1)
67-68: 🛠️ Refactor suggestion
Handle invalid UTF-8 in topic messages explicitly.
The code uses
topic_message.as_str().unwrap_or(""), which silently defaults to an empty string if the topic contains invalid UTF-8. This could lead to missed or miscategorized messages.Modify the code to handle invalid UTF-8 explicitly:
- let topic = topic_message.as_str().unwrap_or(""); + let topic = match topic_message.as_str() { + Some(s) => s, + None => { + eprintln!("Received topic with invalid UTF-8"); + continue; + } + };This change logs an error and skips processing for messages with invalid topics.
📝 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.let topic = match topic_message.as_str() { Some(s) => s, None => { eprintln!("Received topic with invalid UTF-8"); continue; } }; let has_more = socket.get_rcvmore().unwrap_or(false);src/ui/wallet/import_wallet_screen.rs (2)
254-254:
⚠️ Potential issueCritical: 'Save Wallet' functionality is disabled
The
save_walletaction is commented out, which prevents the wallet from being saved when the user clicks the "Save Wallet" button. This effectively disables the wallet creation process and significantly impacts the user experience.If this is intentional and the saving mechanism has changed, please implement the new saving functionality before merging. Otherwise, consider uncommenting the line or updating it to reflect the new method for saving wallets.
15-15: 🛠️ Refactor suggestion
Inconsistent naming:
ImportWalletScreenused for wallet creationThe struct
ImportWalletScreenis used for creating a new wallet, including generating a new seed phrase, which suggests it is intended for wallet creation rather than importing an existing wallet. This discrepancy may cause confusion.Consider renaming the struct to
CreateWalletScreento accurately represent its functionality, or adjust the UI and flow to align with wallet import procedures if that is the intended purpose.Also applies to: 184-184
src/backend_task/withdrawals/mod.rs (2)
217-223:
⚠️ Potential issueHandle unexpected
Elementvariants in sum tree value retrieval.The pattern matching assumes
Element::SumTreevariant. If a different variant is encountered, the error message is generic. Improve error handling by accounting for all possible variants and providing specific error messages.Apply this diff to handle unexpected variants:
let value = if let Element::SumTree(_, value, _) = sum_tree_element { value } else { - return Err( - "could not get sum tree value for current withdrawal maximum".to_string(), - ); + return Err(format!( + "unexpected element type: expected SumTree, found {:?}", + sum_tree_element + )); };📝 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.let value = if let Element::SumTree(_, value, _) = sum_tree_element { value } else { return Err(format!( "unexpected element type: expected SumTree, found {:?}", sum_tree_element )); };
293-297:
⚠️ Potential issueCheck for possible timestamp overflows when converting
created_at.Casting
index(au64) toi64can cause overflow if the value exceedsi64::MAX. Ensure safe conversion to prevent potential runtime errors.Apply this fix to safely convert the timestamp:
-let local_datetime: DateTime<Local> = DateTime::<Utc>::from_timestamp_millis(index as i64) +let index_i64 = i64::try_from(index).map_err(|_| "timestamp overflow")?; +let local_datetime: DateTime<Local> = DateTime::<Utc>::from_timestamp_millis(index_i64)📝 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..ok_or_else(|| "expected created at".to_string())?; let index_i64 = i64::try_from(index).map_err(|_| "timestamp overflow")?; let local_datetime: DateTime<Local> = DateTime::<Utc>::from_timestamp_millis(index_i64) .ok_or_else(|| "expected date time".to_string())? .into();src/ui/add_key_screen.rs (2)
111-111: 🛠️ Refactor suggestion
Use
OsRnginstead ofStdRngfor cryptographic key generationFor generating cryptographic keys, it's recommended to use
OsRng, which provides secure randomness from the operating system, ensuring higher security.Apply this diff to switch to
OsRng:- let mut rng = StdRng::from_entropy(); + let mut rng = rand::rngs::OsRng;Don't forget to import
OsRngat the top of the file:+ use rand::rngs::OsRng;Committable suggestion skipped: line range outside the PR's diff.
54-54: 🛠️ Refactor suggestion
Handle potential errors when converting to array to avoid panics
Using
unwrap()can cause the application to panic if an unexpected error occurs. Even with the length check, it's safer to handle the conversion using pattern matching to ensure robustness.Apply this diff to handle the conversion safely:
- let private_key_bytes = private_key_bytes_vec.try_into().unwrap(); + let private_key_bytes = match private_key_bytes_vec.try_into() { + Ok(bytes) => bytes, + Err(_) => { + self.add_key_status = AddKeyStatus::ErrorMessage("Failed to parse private key bytes.".to_string()); + return app_action; + } + };Ensure that subsequent code that uses
private_key_bytesis within theOkbranch.📝 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.let private_key_bytes = match private_key_bytes_vec.try_into() { Ok(bytes) => bytes, Err(_) => { self.add_key_status = AddKeyStatus::ErrorMessage("Failed to parse private key bytes.".to_string()); return app_action; } };src/context.rs (3)
241-243:
⚠️ Potential issuePotential Deadlock Due to Nested Locks
Acquiring a read lock on
walletsand then a write lock on eachwallet(lines 241-243) can lead to deadlocks if not managed carefully. To mitigate risks:
- Avoid holding locks longer than necessary.
- Consider restructuring to minimize nested locking.
- Use lock ordering consistently throughout the code.
38-38: 🛠️ Refactor suggestion
Variable Naming Inconsistency:
withdraws_contractvswithdrawal_contractThere's an inconsistency in the naming of the
withdraws_contractfield (line 38) and the variablewithdrawal_contractused during initialization (lines 68-71 and 100). To maintain clarity and avoid potential errors, consider standardizing the name.Apply this diff to align the naming:
- pub(crate) withdraws_contract: Arc<DataContract>, + pub(crate) withdrawal_contract: Arc<DataContract>,And update all occurrences accordingly.
📝 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(crate) withdrawal_contract: Arc<DataContract>,
364-365: 🛠️ Refactor suggestion
Avoid Unwrapping Without Error Handling
Using
expectonAddress::from_script(line 365) can cause a panic if the address cannot be parsed. Instead, handle the possible error to improve robustness.Apply this diff to handle the error gracefully:
- let address = Address::from_script(&first.script_pubkey, self.network) - .expect("expected an address"); + let address = match Address::from_script(&first.script_pubkey, self.network) { + Ok(addr) => addr, + Err(e) => { + eprintln!("Failed to parse address: {:?}", e); + return Err(rusqlite::Error::Other(Box::new(e))); + } + };📝 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.let address = match Address::from_script(&first.script_pubkey, self.network) { Ok(addr) => addr, Err(e) => { eprintln!("Failed to parse address: {:?}", e); return Err(rusqlite::Error::Other(Box::new(e))); } };src/backend_task/identity/mod.rs (3)
112-121: 🛠️ Refactor suggestion
Replace
panic!with proper error handling for unsupportedKeyTypeUsing
panic!may crash the application if an unsupportedKeyTypeis encountered. It's better to handle this scenario gracefully by returning an error.Apply this diff to return an error instead of panicking:
let data = match master_private_key_type { KeyType::ECDSA_SECP256K1 => master_private_key.public_key(&secp).to_bytes().into(), KeyType::ECDSA_HASH160 => master_private_key .public_key(&secp) .pubkey_hash() .to_byte_array() .to_vec() .into(), - _ => panic!("need a ECDSA Key for now"), + _ => return Err("Unsupported KeyType for master private key".to_string()), };Note: You may need to adjust the function signature to return
Result<..., String>.Committable suggestion skipped: line range outside the PR's diff.
138-147: 🛠️ Refactor suggestion
Replace
panic!with proper error handling for unsupportedKeyTypeUsing
panic!may crash the application if an unsupportedKeyTypeis encountered. It's better to handle this scenario gracefully by returning an error.Apply this diff to return an error instead of panicking:
let data = match key_type { KeyType::ECDSA_SECP256K1 => private_key.public_key(&secp).to_bytes().into(), KeyType::ECDSA_HASH160 => private_key .public_key(&secp) .pubkey_hash() .to_byte_array() .to_vec() .into(), - _ => panic!("need a ECDSA Key for now"), + _ => return Err("Unsupported KeyType".to_string()), };Note: You may need to adjust the function signature to return
Result<..., String>.Committable suggestion skipped: line range outside the PR's diff.
6-6: 💡 Codebase verification
Add unit tests for the
transfer_to_identityfunctionalityThe
transfer.rsmodule contains critical functionality for transferring credits between identities but lacks unit tests. Tests should be added to cover:
- Successful credit transfer scenarios
- Error handling for withdrawal failures
- Balance updates after transfer
- Local identity update verification
🔗 Analysis chain
Ensure unit tests cover the new
transfermodulePlease verify that the new
transfermodule is adequately tested to ensure reliability and catch potential issues early.Run the following script to check for unit tests related to the
transfermodule:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unit tests in the transfer module. # Test: Find test files related to transfer module. Expect: Test files found. fd --type f --name '*transfer*_test.rs' --exec echo "Found test: {}"Length of output: 321
Script:
#!/bin/bash # Let's try a different approach to find test files # First, let's find the transfer module location fd transfer.rs # Then let's look for test modules or test functions within the transfer module rg -l "mod test|#\[test\]" --type rust | grep -i transferLength of output: 114
Script:
#!/bin/bash # Let's examine the content of the transfer module to see if it contains tests # or if tests need to be added cat src/backend_task/identity/transfer.rsLength of output: 1476
src/database/wallet.rs (4)
366-367:
⚠️ Potential issueEnsure
networkis accessible within the closure.In the closure processing
asset_lock_rows, the variablenetworkis used when creating an address from a script. To prevent potential compilation errors, ensure thatnetworkis captured or accessible within the closure's scope.If
networkis not accessible, consider capturing it explicitly:let asset_lock_rows = asset_lock_stmt.query_map([], |row| { let network = *network; // Capture network // Rest of the code...
338-393: 🛠️ Refactor suggestion
Handle potential deserialization errors gracefully in asset lock processing.
Deserialization of transactions and instant locks may fail, which currently results in a panic. Consider handling these errors gracefully to prevent crashing the application.
Modify the deserialization to handle errors:
let tx: Transaction = deserialize(&tx_data) .map_err(|e| rusqlite::Error::FromSqlConversionFailure(0, rusqlite::types::Type::Blob, Box::new(e)))?; if let Some(islock_bytes) = islock_data { let is_lock: InstantLock = deserialize(&islock_bytes) .map_err(|e| rusqlite::Error::FromSqlConversionFailure(0, rusqlite::types::Type::Blob, Box::new(e)))?; // Rest of the code... }
69-75:
⚠️ Potential issueFix inconsistency between parameter and SQL query in
update_wallet_alias_and_main.In the
update_wallet_alias_and_mainmethod, the SQL query usesseedin theWHEREclause, but the method parameter isseed_hash. This mismatch may result in the query not affecting any rows.Apply this diff to correct the SQL query:
- "UPDATE wallet SET alias = ?, is_main = ? WHERE seed = ?", + "UPDATE wallet SET alias = ?, is_main = ? WHERE seed_hash = ?",📝 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.seed_hash: &[u8; 32], new_alias: Option<String>, is_main: bool, ) -> rusqlite::Result<()> { self.execute( "UPDATE wallet SET alias = ?, is_main = ? WHERE seed_hash = ?", params![new_alias, is_main as i32, seed_hash],
348-349:
⚠️ Potential issueUpdate error message to match expected seed hash length.
The error message in the
expectcall suggests that the seed should be 64 bytes, butwallet_seed_hash_arrayis of type[u8; 32], indicating it should be 32 bytes.Apply this diff to correct the error message:
- wallet_seed.try_into().expect("Seed should be 64 bytes"); + wallet_seed.try_into().expect("Seed hash should be 32 bytes");📝 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.let wallet_seed_hash_array: [u8; 32] = wallet_seed.try_into().expect("Seed hash should be 32 bytes");src/backend_task/identity/register_identity.rs (5)
212-221:
⚠️ Potential issuePotential infinite loop when waiting for asset lock proof
The loop at lines 212 to 221 waits indefinitely for the asset lock proof by polling every 200 milliseconds. If the proof never becomes available, this could result in an infinite loop. Consider implementing a timeout mechanism to prevent the loop from running indefinitely and handle the timeout appropriately.
You can modify the code to include a timeout:
use std::time::Instant; let start_time = Instant::now(); let timeout_duration = Duration::from_secs(60); // Set an appropriate timeout duration loop { { let proofs = self.transactions_waiting_for_finality.lock().unwrap(); if let Some(Some(proof)) = proofs.get(&tx_id) { asset_lock_proof = proof.clone(); break; } } if start_time.elapsed() > timeout_duration { return Err("Timed out waiting for asset lock proof".to_string()); } tokio::time::sleep(Duration::from_millis(200)).await; }
264-273:
⚠️ Potential issuePotential infinite loop when waiting for asset lock proof
Similar to the previous issue, the loop at lines 264 to 273 could result in an infinite loop if the asset lock proof never becomes available. Implementing a timeout mechanism here as well would prevent potential hangs in the application.
Apply the same timeout strategy:
use std::time::Instant; let start_time = Instant::now(); let timeout_duration = Duration::from_secs(60); // Set an appropriate timeout duration loop { { let proofs = self.transactions_waiting_for_finality.lock().unwrap(); if let Some(Some(proof)) = proofs.get(&tx_id) { asset_lock_proof = proof.clone(); break; } } if start_time.elapsed() > timeout_duration { return Err("Timed out waiting for asset lock proof".to_string()); } tokio::time::sleep(Duration::from_millis(200)).await; }
281-281:
⚠️ Potential issueAvoid using
expectto prevent unintended panicsAt line 281, the code uses
.expect("expected to create an identifier"). Usingexpectcan cause the application to panic if the function fails. It's safer to handle the error gracefully using?and propagate it upwards or handle it appropriately.Modify the code to handle the error:
let identity_id = asset_lock_proof .create_identifier() .map_err(|e| format!("Failed to create identifier: {}", e))?;
292-292:
⚠️ Potential issueAvoid using
expectto prevent unintended panicsAt line 292, using
.expect("expected to make identity")can lead to a panic ifnew_with_id_and_keysfails. It's better to handle the potential error.Update the code to handle the error:
let identity = Identity::new_with_id_and_keys(identity_id, public_keys, sdk.version()) .map_err(|e| format!("Failed to create identity: {}", e))?;
341-341:
⚠️ Potential issueAvoid using
expectto prevent unintended panicsAt line 341, the use of
.expect("expected to make transition")may cause a panic if the function fails. It's advisable to handle this error to maintain application stability.Adjust the code to handle the error:
let identity_create_transition = IdentityCreateTransition::try_from_identity_with_signer( &identity, asset_lock_proof, asset_lock_proof_private_key.inner.as_ref(), &qualified_identity, &NativeBlsModule, 0, PlatformVersion::latest(), ).map_err(|e| format!("Failed to create identity transition: {}", e))?;src/app.rs (1)
382-384:
⚠️ Potential issueFix undefined variable
messageinWithdrawalStatusmatch armIn the match arm
BackendTaskSuccessResult::WithdrawalStatus(_), the variablemessageis not defined, leading to a compilation error. You should bind themessagevariable in the match arm and pass it todisplay_task_result.Apply this diff to fix the issue:
match message { // Other match arms... - BackendTaskSuccessResult::WithdrawalStatus(_) => { + BackendTaskSuccessResult::WithdrawalStatus(message) => { self.visible_screen_mut().display_task_result(message); } // ... }Committable suggestion skipped: line range outside the PR's diff.
src/ui/wallet/add_new_wallet_screen.rs (8)
51-51: 🛠️ Refactor suggestion
Secure Password Handling
Storing the password directly as a
Stringmay leave it in memory longer than necessary, posing a security risk. Consider using a more secure data structure, such as thesecrecycrate'sSecretString, to handle sensitive data like passwords.
149-154: 🛠️ Refactor suggestion
Improve Error Handling When Acquiring Write Lock
If acquiring the write lock on
walletsfails, the code currently prints an error message to stderr without informing the user or attempting recovery. Consider handling this error more gracefully by notifying the user or implementing a retry mechanism.Apply this diff to enhance error handling:
if let Ok(mut wallets) = self.app_context.wallets.write() { wallets.push(Arc::new(RwLock::new(wallet))); self.app_context.has_wallet.store(true, Ordering::Relaxed); } else { - eprintln!("Failed to acquire write lock on wallets"); + self.error = Some("Failed to save wallet. Please try again.".to_string()); }📝 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 let Ok(mut wallets) = self.app_context.wallets.write() { wallets.push(Arc::new(RwLock::new(wallet))); self.app_context.has_wallet.store(true, Ordering::Relaxed); } else { self.error = Some("Failed to save wallet. Please try again.".to_string()); }
90-91:
⚠️ Potential issueConsider Using Password in Seed Generation
The seed is generated using
mnemonic.to_seed(""), which means the user's password is not utilized in the seed derivation process. If the intention is to strengthen the seed with the user's password (as per the BIP39 standard), consider passingself.passwordtoto_seed.Apply this diff if including the password in seed generation is desired:
-let seed = mnemonic.to_seed(""); +let seed = mnemonic.to_seed(&self.password);If omitting the password intentionally, please ensure this aligns with your security requirements.
Committable suggestion skipped: line range outside the PR's diff.
80-85:
⚠️ Potential issueHandle Errors Gracefully in Seed Phrase Generation
The
generate_seed_phrasemethod uses.expect("Failed to generate mnemonic"), which will panic and potentially crash the application if mnemonic generation fails. Instead, consider handling the error gracefully and providing user feedback.Apply this diff to handle the error:
fn generate_seed_phrase(&mut self) { let mnemonic_result = Mnemonic::from_entropy_in( self.selected_language, &self.entropy_grid.random_number_with_user_input(), ); - .expect("Failed to generate mnemonic"); + match mnemonic_result { + Ok(mnemonic) => { + self.seed_phrase = Some(mnemonic); + } + Err(e) => { + self.error = Some(format!("Failed to generate mnemonic: {}", e)); + } + } }Committable suggestion skipped: line range outside the PR's diff.
415-419:
⚠️ Potential issueDisable "Save Wallet" Button When Not Ready
Using
egui::Sense::hover()makes the "Save Wallet" button appear interactive even when it cannot be clicked, which may confuse users. Instead, disable the button to clearly indicate it's inactive until the required conditions are met.Apply this diff to disable the button appropriately:
let save_button = egui::Button::new( RichText::new("Save Wallet").strong().size(30.0), ) .min_size(Vec2::new(300.0, 60.0)) .rounding(10.0) .stroke(Stroke::new(1.5, Color32::WHITE)); -if ui.add(save_button).clicked() { +if ui.add_enabled(self.wrote_it_down, save_button).clicked() { match self.save_wallet() { Ok(save_wallet_action) => { action = save_wallet_action; } Err(e) => { self.error = Some(e) } } }Committable suggestion skipped: line range outside the PR's diff.
102-116: 🛠️ Refactor suggestion
Handle Potential Errors in Key Derivation
The code assumes that deriving the master ECDSA extended private key and public key will always succeed. However, these operations can fail, and the errors should be handled to prevent panics.
Modify the code to handle errors:
// Generate master ECDSA extended private key -let master_ecdsa_extended_private_key = - ExtendedPrivKey::new_master(self.app_context.network, &seed) - .expect("Failed to create master ECDSA extended private key"); +let master_ecdsa_extended_private_key = ExtendedPrivKey::new_master(self.app_context.network, &seed) + .map_err(|e| format!("Failed to create master ECDSA extended private key: {}", e))?; // Derive the BIP44 root derivation path let bip44_root_derivation_path: DerivationPath = match self.app_context.network { Network::Dash => DerivationPath::from(DASH_BIP44_ACCOUNT_0_PATH_MAINNET.as_slice()), _ => DerivationPath::from(DASH_BIP44_ACCOUNT_0_PATH_TESTNET.as_slice()), }; let secp = Secp256k1::new(); -let master_bip44_ecdsa_extended_public_key = master_ecdsa_extended_private_key - .derive_priv(&secp, &bip44_root_derivation_path) - .map_err(|e| e.to_string())?; +let derived_private_key = master_ecdsa_extended_private_key + .derive_priv(&secp, &bip44_root_derivation_path) + .map_err(|e| format!("Failed to derive private key: {}", e))?; let master_bip44_ecdsa_extended_public_key = ExtendedPubKey::from_priv(&secp, &derived_private_key);📝 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.let master_ecdsa_extended_private_key = ExtendedPrivKey::new_master(self.app_context.network, &seed) .map_err(|e| format!("Failed to create master ECDSA extended private key: {}", e))?; let bip44_root_derivation_path: DerivationPath = match self.app_context.network { Network::Dash => DerivationPath::from(DASH_BIP44_ACCOUNT_0_PATH_MAINNET.as_slice()), _ => DerivationPath::from(DASH_BIP44_ACCOUNT_0_PATH_TESTNET.as_slice()), }; let secp = Secp256k1::new(); let derived_private_key = master_ecdsa_extended_private_key .derive_priv(&secp, &bip44_root_derivation_path) .map_err(|e| format!("Failed to derive private key: {}", e))?; let master_bip44_ecdsa_extended_public_key = ExtendedPubKey::from_priv(&secp, &derived_private_key);
348-365:
⚠️ Potential issueHandle Potential Errors from Password Strength Estimation
The
zxcvbnfunction can return errors, but the current implementation doesn't handle them. To prevent potential crashes, handle theResultreturned byzxcvbn.Update the code to handle possible errors:
if !self.password.is_empty() { - let estimate = zxcvbn(&self.password, &[]); + match zxcvbn(&self.password, &[]) { + Ok(estimate) => { + // Existing logic for processing the estimate + let score_u8 = u8::from(estimate.score()); + self.password_strength = score_u8 as f64 * 25.0; + let estimated_seconds = estimate.crack_times().offline_slow_hashing_1e4_per_second(); + self.estimated_time_to_crack = estimated_seconds.to_string(); + } + Err(e) => { + self.error = Some(format!("Password strength estimation failed: {}", e)); + self.password_strength = 0.0; + self.estimated_time_to_crack = String::new(); + } + } } else { self.password_strength = 0.0; self.estimated_time_to_crack = String::new(); }Committable suggestion skipped: line range outside the PR's diff.
374-391:
⚠️ Potential issueAlign Password Strength Calculation with Display Logic
The password strength percentage and the matching ranges may not align correctly. Since
self.password_strengthis calculated asscore_u8 * 25.0, resulting in values 0, 25, 50, 75, or 100, the match arms should correspond to these exact values.Adjust the match statements to reflect the actual strength values:
let color = match self.password_strength as i32 { - 0..=25 => Color32::RED, - 26..=50 => Color32::YELLOW, - 51..=75 => Color32::LIGHT_GREEN, - _ => Color32::GREEN, + 0 => Color32::RED, + 25 => Color32::RED, + 50 => Color32::YELLOW, + 75 => Color32::LIGHT_GREEN, + 100 => Color32::GREEN, + _ => Color32::GRAY, }; ui.add( egui::ProgressBar::new(strength_percentage as f32) .desired_width(200.0) .show_percentage() .text(match self.password_strength as i32 { - 0 => "None".to_string(), - 1..=25 => "Very Weak".to_string(), - 26..=50 => "Weak".to_string(), - 51..=75 => "Strong".to_string(), - _ => "Very Strong".to_string(), + 0 => "None".to_string(), + 25 => "Very Weak".to_string(), + 50 => "Weak".to_string(), + 75 => "Strong".to_string(), + 100 => "Very Strong".to_string(), + _ => "Unknown".to_string(), }) .fill(color), );📝 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.let strength_percentage = (self.password_strength / 100.0).min(1.0); let color = match self.password_strength as i32 { 0 => Color32::RED, 25 => Color32::RED, 50 => Color32::YELLOW, 75 => Color32::LIGHT_GREEN, 100 => Color32::GREEN, _ => Color32::GRAY, }; ui.add( egui::ProgressBar::new(strength_percentage as f32) .desired_width(200.0) .show_percentage() .text(match self.password_strength as i32 { 0 => "None".to_string(), 25 => "Very Weak".to_string(), 50 => "Weak".to_string(), 75 => "Strong".to_string(), 100 => "Very Strong".to_string(), _ => "Unknown".to_string(), })src/model/wallet/mod.rs (2)
170-182:
⚠️ Potential issueSecurity Issue: Insecure handling of encrypted seed in
open_no_passwordThe
open_no_passwordmethod directly assigns theencrypted_seedto theseedwithout decryption. This bypasses the encryption and may lead to incorrect or insecure usage of the seed data.Consider modifying the method to handle the scenario appropriately, ensuring that the seed is correctly managed when no password is provided. If the seed is not meant to be accessible without a password, this method should return an error or handle it securely.
439-448: 🛠️ Refactor suggestion
Error Handling Inconsistency: Using
expectinidentity_registration_ecdsa_private_keyThe use of
expect("derivation should not be able to fail")can cause the application to panic if an error occurs. This is inconsistent with the rest of the method, which returns aResult.Apply this diff to propagate the error instead of panicking:
-let extended_private_key = derivation_path - .derive_priv_ecdsa_for_master_seed(self.seed_bytes()?, network) - .expect("derivation should not be able to fail"); +let extended_private_key = derivation_path + .derive_priv_ecdsa_for_master_seed(self.seed_bytes()?, network) + .map_err(|e| e.to_string())?;📝 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.&mut self, network: Network, index: u32, register_addresses: Option<&AppContext>, ) -> Result<PrivateKey, String> { let derivation_path = DerivationPath::identity_registration_path(network, index); let extended_private_key = derivation_path .derive_priv_ecdsa_for_master_seed(self.seed_bytes()?, network) .map_err(|e| e.to_string())?; let private_key = extended_private_key.to_priv();src/ui/withdraws_status_screen.rs (3)
208-303: 🛠️ Refactor suggestion
Refactor filter checkbox handling to reduce code duplication
The code handling filter checkboxes between lines 208 and 303 is repetitive. Refactoring can improve maintainability and readability.
Consider refactoring the filter checkboxes into a loop:
let filters = [ (&mut self.filter_status_queued, "Queued", WithdrawalStatus::QUEUED), (&mut self.filter_status_pooled, "Pooled", WithdrawalStatus::POOLED), (&mut self.filter_status_broadcasted, "Broadcasted", WithdrawalStatus::BROADCASTED), (&mut self.filter_status_complete, "Complete", WithdrawalStatus::COMPLETE), (&mut self.filter_status_expired, "Expired", WithdrawalStatus::EXPIRED), ]; for (filter_status, label, status_enum) in filters.iter_mut() { if ui.checkbox(*filter_status, *label).changed() { self.util_build_combined_filter_status_mix(); self.requested_data = true; if let Ok(mut lock_data) = self.data.write() { *lock_data = None; } app_action |= AppAction::BackendTask(BackendTask::WithdrawalTask( WithdrawalsTask::QueryWithdrawals( self.filter_status_mix.clone(), self.pagination_items_per_page.into(), None, true, true, ), )); } ui.add_space(8.0); }This refactoring reduces code duplication and simplifies the addition of new filters in the future.
399-399:
⚠️ Potential issueAdd missing semicolon in decrementing
pagination_current_pageIn line 399, the semicolon is missing after
self.pagination_current_page = current_page - 1, which can cause a syntax error.Apply this diff to fix the issue:
- self.pagination_current_page = current_page - 1 + self.pagination_current_page = current_page - 1;📝 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.self.pagination_current_page = current_page - 1;
403-418:
⚠️ Potential issueCorrect pagination logic for the "Next" button
There is a potential off-by-one error in the pagination logic for the "Next" button. After incrementing
self.pagination_current_page, the checkif current_page == total_pages - 1does not account for the updated page number.Modify the code as follows to ensure correct pagination:
if ui.button("Next").clicked() && self.pagination_current_page + 1 < total_pages { self.pagination_current_page = current_page + 1; if self.pagination_current_page == total_pages - 1 { app_action = AppAction::BackendTask(BackendTask::WithdrawalTask( WithdrawalsTask::QueryWithdrawals( self.filter_status_mix.clone(), self.pagination_items_per_page.into(), data.withdrawals .last() .map(|withdrawal_record| withdrawal_record.document_id), false, false, ), )); } }This change ensures that
self.pagination_current_pagedoes not exceed the total number of pages and that additional data is fetched correctly when reaching the last page.📝 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 ui.button("Next").clicked() && self.pagination_current_page + 1 < total_pages { self.pagination_current_page = current_page + 1; if self.pagination_current_page == total_pages - 1 { app_action = AppAction::BackendTask(BackendTask::WithdrawalTask( WithdrawalsTask::QueryWithdrawals( self.filter_status_mix.clone(), self.pagination_items_per_page.into(), data.withdrawals .last() .map(|withdrawal_record| withdrawal_record.document_id), false, false, ), )); } }src/ui/wallet/wallets_screen.rs (4)
437-444:
⚠️ Potential issueHandle read lock poisoning when accessing wallet asset locks
In
render_wallet_asset_locks, unwrapping the read lock without error handling can lead to panics if the lock is poisoned. Implement proper error handling to enhance robustness.Modify the code:
if let Some(wallet) = &self.selected_wallet { - let wallet = wallet.read().unwrap(); + let wallet = match wallet.read() { + Ok(guard) => guard, + Err(poisoned) => { + self.display_message("Failed to acquire wallet read lock", MessageType::Error); + return; + } + }; if wallet.unused_asset_locks.is_empty() { ui.label("No asset locks available."); return; }This approach ensures that the UI does not panic and provides feedback to the user in case of errors.
📝 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.fn render_wallet_asset_locks(&mut self, ui: &mut Ui) { if let Some(wallet) = &self.selected_wallet { let wallet = match wallet.read() { Ok(guard) => guard, Err(poisoned) => { self.display_message("Failed to acquire wallet read lock", MessageType::Error); return; } }; if wallet.unused_asset_locks.is_empty() { ui.label("No asset locks available."); return; }
110-112:
⚠️ Potential issueHandle potential panics when unwrapping locks
Using
.unwrap()on lock acquisitions can cause panics if the lock is poisoned due to a previous thread's panic. To make the application more robust, consider handlingPoisonErrorby providing a fallback or informing the user.Modify the code to handle potential errors:
let result = { - let mut wallet = wallet.write().unwrap(); + let mut wallet = match wallet.write() { + Ok(guard) => guard, + Err(poisoned) => { + self.display_message("Failed to acquire wallet write lock", MessageType::Error); + return; + } + }; wallet.receive_address(self.app_context.network, true, Some(&self.app_context)) };By handling the
PoisonError, you prevent the application from panicking and provide meaningful feedback to the user.Committable suggestion skipped: line range outside the PR's diff.
199-224:
⚠️ Potential issueGracefully handle errors when writing to wallet
When updating the wallet alias, unwrapping the write lock can cause a panic if the lock is poisoned. Handle this scenario to maintain application stability.
Modify the code to manage potential errors:
{ - let mut wallet = selected_wallet.write().unwrap(); + let mut wallet = match selected_wallet.write() { + Ok(guard) => guard, + Err(poisoned) => { + self.display_message("Failed to acquire wallet write lock", MessageType::Error); + return; + } + }; let mut alias = wallet.alias.clone().unwrap_or_default(); // Rest of the code... }By handling the error, you prevent unexpected panics and improve user experience.
Committable suggestion skipped: line range outside the PR's diff.
247-296:
⚠️ Potential issueHandle potential lock poisoning when reading wallet data
In
render_address_table, unwrapping the read lock without error handling can lead to panics. To enhance reliability, handlePoisonErrorwhen acquiring the read lock.Modify the code as follows:
let wallet = match self.selected_wallet.as_ref() { Some(selected_wallet) => match selected_wallet.read() { Ok(guard) => guard, Err(poisoned) => { self.display_message("Failed to acquire wallet read lock", MessageType::Error); return action; } }, None => { self.display_message("No wallet selected", MessageType::Error); return action; } };This change ensures that your application gracefully handles lock poisoning scenarios.
Committable suggestion skipped: line range outside the PR's diff.
src/ui/identities/identities_screen.rs (1)
78-82: 🛠️ Refactor suggestion
Avoid comparing user input to placeholder text
Comparing the user's alias input to the placeholder text can cause unintended behavior if a user sets their alias to the placeholder text (e.g., "An Identity"). This would result in the alias being unset (
None). Consider checking only for empty strings to determine if the alias should be left unset.Apply this diff to adjust the condition:
- if alias == placeholder_text || alias == "" { + if alias.trim().is_empty() {📝 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 alias.trim().is_empty() { identity_to_update.alias = None; } else { identity_to_update.alias = Some(alias); }src/ui/dpns_contested_names_screen.rs (1)
624-630:
⚠️ Potential issueFix incorrect timestamp conversion method
The method
DateTime::from_timestampdoes not exist in thechronocrate. Instead, you should useUtc.timestamp_optto convert a timestamp to aDateTime<Utc>. This will correctly handle the timestamp and provide anOption<DateTime<Utc>>.Apply this diff to fix the issue:
-let datetime = DateTime::from_timestamp( +let datetime = Utc.timestamp_opt( dpns_info.acquired_at as i64 / 1000, ((dpns_info.acquired_at % 1000) * 1_000_000) as u32, -) -.map(|dt| dt.to_string()) -.unwrap_or_else(|| "Invalid timestamp".to_string()); +).single() + .map(|dt| dt.to_string()) + .unwrap_or_else(|| "Invalid timestamp".to_string());📝 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.let datetime = Utc.timestamp_opt( dpns_info.acquired_at as i64 / 1000, ((dpns_info.acquired_at % 1000) * 1_000_000) as u32, ).single() .map(|dt| dt.to_string()) .unwrap_or_else(|| "Invalid timestamp".to_string());src/ui/identities/add_new_identity_screen/mod.rs (4)
180-217: 🛠️ Refactor suggestion
Improve Error Handling in Balance Check Loop
In the
start_balance_checkfunction (lines 175-220), the error handling within the balance checking loop (lines 210-217) logs errors but continues looping indefinitely. This could lead to infinite loops in case of persistent errors. Consider implementing a retry mechanism with a maximum number of retries or exiting the loop upon critical errors to prevent potential resource exhaustion.
832-833:
⚠️ Potential issueReview Unreachable Code After Return Statement
At lines 832-833, the code returns early if
self.selected_wallet.is_none():if self.selected_wallet.is_none() { return; };Any code following this return statement will not execute when
selected_walletisNone. Ensure this is the intended behavior. If not, consider restructuring the logic to prevent skipping essential UI rendering.
146-147: 🛠️ Refactor suggestion
Securely Zeroize Sensitive Data
In lines 146-147, the
master_private_keyis stored asOption<PrivateKey>:master_private_key: Option<PrivateKey>,Ensure that sensitive data like private keys are securely zeroized when they're no longer needed to prevent memory exposure. Implement the
Droptrait or use types that automatically zeroize on drop.[security_issue]
711-716: 🛠️ Refactor suggestion
Handle Potential Errors in Key Generation
In the
update_identity_keyfunction (lines 708-716), the use of.expect("expected to have decrypted wallet")may cause the application to panic if the wallet is not decrypted. To improve robustness, handle theResultusing proper error handling and provide user feedback if key generation fails.Apply this diff to handle errors gracefully:
self.identity_keys.master_private_key = Some( wallet .identity_authentication_ecdsa_private_key( self.app_context.network, identity_index, 0, ) - .expect("expected to have decrypted wallet"), + .unwrap_or_else(|e| { + self.error_message = Some(format!("Failed to generate master private key: {}", e)); + PrivateKey::default() // Return a default key or handle accordingly + }), );Committable suggestion skipped: line range outside the PR's diff.
Bot issues fixed: - #35: WalletNotFound stores only the identifier, not full error sentence - #36: MCP log differentiates unset vs invalid MCP_API_KEY - #37: Release workflow splits GUI and CLI builds (GUI without --features cli, CLI with --features headless --bin det-cli) - #38: SendCoreFunds validates empty network string before require_network - #39: Remove unnecessary ensure_spv_synced() from QueryWithdrawals - #40: Remove input_schema() override from NetworkTool Human issues addressed: - #41: Tool table includes det-cli command column - #42: CLI section added to MCP.md - #43: Headless mode performance characteristics documented - #45: Authentication section with setup, behavior, and security guidance - #46: ListWalletsTool moved from network.rs to wallet.rs - #47: TODO added for withdrawal pagination (requires backend changes) - #48: TODO added for structured JSON output (requires new backend variant) - #49: Verified correct BackendTask dispatch for withdrawals Issue #44 (CLAUDE.md duplication): no actual duplication found — the MCP section in CLAUDE.md covers architecture guidance while docs/MCP.md covers usage. Both serve distinct purposes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add MCP HTTP server for programmatic access
Embed a streamable-HTTP MCP server (rmcp + axum) that exposes backend
tasks as tools for AI agents. Feature-gated behind `mcp` feature flag
and opt-in via MCP_API_KEY environment variable.
Tools: list_wallets, generate_receive_address, list_core_wallets.
Auth: Bearer token with constant-time comparison (subtle crate).
Supports network switching -- MCP context follows GUI network changes
via ArcSwap.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: support dual MCP transport (stdio + HTTP) with lazy init
Refactor MCP module to support two feature-gated transports:
- mcp-stdio: standalone binary with lazy AppContext initialization
- mcp-http: embedded HTTP server sharing GUI app's context
New binary: dash-evo-tool-mcp (requires mcp-stdio feature).
Neither transport is enabled by default.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add det-cli MCP client with tool caching and completion
Dynamic CLI client that discovers tools from the MCP server at runtime.
Supports HTTP transport (default) and standalone mode (--standalone).
Caches tool schemas locally for shell completion.
New binary: det-cli (requires `cli` feature).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: add MCP and CLI quick start guides
Add docs/MCP.md covering the HTTP and stdio transport modes, available tools,
environment variables, and security notes. Add docs/CLI.md covering det-cli
connection modes, subcommands, and shell completion setup. Update README.md
with a Programmatic Access section linking to both guides.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(cli): read MCP config from app .env file
- Load app's .env on startup so MCP_API_KEY/MCP_LISTEN are available
without separate DET_CLI_BEARER setup
- Replace DET_CLI_BEARER with MCP_API_KEY (matches server config)
- Derive server URL from MCP_LISTEN env var; fall back to 127.0.0.1:9527
- Auto-select stdio mode when no API key is configured; HTTP when set
- Make subcommand optional: bare `det-cli` lists tools live
- Inject cached tool list into --help output via print_cached_tools_help()
- Update docs/CLI.md to reflect new config and connection behavior
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(cli): implicit versioned cache, remove Cache subcommand
- Remove explicit `det-cli cache` subcommand; tool list is cached
automatically after every `tools` invocation
- Add ToolCache struct with version field; cache includes server version
from MCP peer info (falls back to PKG_VERSION)
- Cache staleness: --help only shows cached tools when version matches
the binary; stale cache prompts user to run `det-cli` to refresh
- Add `#[command(version)]` so `det-cli --version` works
- Fix server info access: use `client.peer().peer_info()` (correct rmcp
API) instead of non-existent `peer_server_info()`
- Update bash completion jq path to `.tools[].name` for new cache format
- Update docs/CLI.md to reflect automatic caching behavior
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* ci(release): build and package det-cli binary alongside dash-evo-tool
Add --features cli to all release build commands and include the det-cli
binary in release packages (zip for Linux/Windows, .app bundle for macOS).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: consolidate MCP features and embed stdio server in det-cli
Simplify from 3 features (mcp-stdio, mcp-http, cli) to 2:
- `mcp`: HTTP server embedded in GUI app
- `cli`: det-cli binary with in-process MCP and `serve` subcommand
Remove standalone `dash-evo-tool-mcp` binary — `det-cli serve` replaces
it as the MCP stdio server for Claude Desktop and AI agents.
Update docs (MCP.md, CLI.md, CONTRIBUTING.md) to reflect new layout.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: expand feature flags documentation in CONTRIBUTING.md
Add binary output mapping, dependency details, CI usage table,
and combination examples for all cargo features.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: remove redundant build examples from feature flags section
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: remove dependencies section from feature flags
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: remove CI feature usage section
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(cli): auto-install bash completion on cache refresh
Write bash completion script to ~/.local/share/bash-completion/completions/det-cli
whenever the tool cache is rebuilt. bash-completion 2.x auto-loads from this
directory — no .bashrc editing or manual setup needed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(cli): direct tool subcommands with custom help
- Remove `call` subcommand — tools are now first-class subcommands
(e.g. `det-cli list-wallets` instead of `det-cli call list_wallets`)
- Use hyphens in command and parameter names (CLI convention)
- Custom help output shows all available tools as primary content,
with management commands and options below
- Hyphen-to-underscore conversion for MCP tool/param names
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: update CLI.md for direct subcommand UX
Rewrite examples and usage to reflect hyphenated commands
(det-cli list-wallets), remove call subcommand references,
document auto-installed bash completion.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(cli): unify help output for bare invocation and --help
Both `det-cli` (no args) and `det-cli --help` now display the same
format. Bare invocation queries the server for fresh tools first;
--help uses the disk cache for instant response.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(cli): replace MCP_NETWORK with --network flag and database default
Network selection now follows the same pattern as the GUI:
read from the database (last selected network), with --network
flag as override. Removes the MCP_NETWORK env var.
Also adds short flags: -n (network), -s (standalone), -a (addr),
-b (bearer).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(mcp): remove list_core_wallets tool — MCP is SPV-only
Core wallet operations require a local Dash Core node, which conflicts
with the MCP server's SPV-only design. Remove the tool and all
references from docs.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(cli): show meaningful errors when AppContext init fails
- Add tracing subscriber (stderr, warn level) for non-serve CLI paths
so tracing::error!() from AppContext::new() is visible to users
- Pre-validate network config exists before attempting AppContext init,
giving a clear error like "no configuration found for network 'Dash'.
Check your .env file has MAINNET_dapi_addresses set."
- Fall through message now says "check logs for details" instead of
the opaque "failed to create AppContext"
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(mcp): add network tool, remove --network CLI flag
Replace the --network CLI flag with a proper `network` MCP tool that
returns the active network and available configured networks as JSON.
This keeps det-cli as a thin client with no local logic — network
info comes from the server, same as all other operations.
Changes:
- Add `network` tool to DashMcpService (active + available networks)
- Remove --network flag from det-cli
- Remove network_override threading (ContextProvider::Lazy, new_lazy,
init_app_context, start_stdio)
- Simplify init_app_context to use DB default only
- Add available_network_names() and network_display_name() helpers
- Make AppContext.network and data_dir pub (needed by MCP tools)
- Update docs/CLI.md and docs/MCP.md
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: silently skip unconfigured networks in Config loading
Missing network configs are normal — not every user configures all four
networks. Remove tracing::error! spam and the hard mainnet requirement.
Only fail when no networks are configured at all. The actual network
availability check happens in init_app_context where it matters.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(cli): clean error messages and network tool fixes
- Handle rmcp ServiceError with typed matching instead of string parsing.
McpError variant extracts message + code directly from ErrorData fields.
- Suppress rmcp service-level tracing warnings (duplicates handled errors)
- Revert network tool to use AppContext (correct for embedded mode —
needs working config to be useful)
- Make AppContext.network and data_dir pub (needed by MCP tools)
- Add collect_available() helper for listing configured networks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(mcp): start SPV and wait for wallets in standalone mode
In CLI/stdio mode, init_app_context now starts the SPV client after
creating AppContext so wallet operations work. The init returns
immediately — tools like list-wallets and network are not blocked.
The generate-receive-address tool polls ConnectionStatus via
trigger_refresh() until SPV reaches Synced state (60s timeout),
then verifies the specific wallet is loaded into SPV's det_wallets
map before dispatching the backend task.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(cli): add per-tool --help support
Intercept --help/-h in tool subcommand args and print tool-specific
help from the disk cache instead of forwarding to the MCP server.
Uses cached tool metadata to show description, usage, and parameters
with required/optional markers.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore(cli): bump SPV timeout to 600s, disable logs by default
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(mcp): add wallet_balances and send_core_funds tools
Add two new MCP tools:
- wallet_balances: read total/confirmed/unconfirmed balances in duffs
- send_core_funds: send DASH from a wallet to an address via CoreTask
Extract SPV wait loop from generate_receive_address into a reusable
wait_for_spv_sync() helper shared by both SPV-dependent tools.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(cli): use wrapper pattern for generate_completion bash output
The generate_completion() function was emitting a standalone _det_cli_tools
function with its own `complete -F` registration, overriding clap's built-in
completions. Now uses the same _det_cli_wrapper pattern as install_bash_completion()
— calls clap's _det-cli first, then adds dynamic tool/parameter names.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(mcp): wait for SPV sync before reading wallet balances
wallet_balances was returning stale data because it read in-memory state
without waiting for SPV to finish syncing. Now waits for green state like
the other wallet-dependent tools.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(cli): split det_cli into modules and add headless feature
Split the 546-line single-file binary into a multi-file directory:
main.rs, connect.rs, cache.rs, completion.rs, help.rs.
Add `headless` feature flag (= ["cli", "mcp"]) and `det-cli headless`
command that runs as a standalone HTTP MCP server daemon with bearer
auth, ctrl+c graceful shutdown, and eager AppContext initialization.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(mcp): migrate tools from proc-macro to trait-based ToolBase/AsyncTool pattern
Replace #[tool_router]/#[tool] proc-macro approach with rmcp's
ToolBase + AsyncTool traits. Each tool is now a standalone struct
in its own domain file, enabling auto-generated schemas, typed
output with structuredContent, and per-tool annotations.
New files:
- error.rs: McpToolError enum with From<McpToolError> for ErrorData
- resolve.rs: wallet resolution and SPV sync helpers (extracted)
- tools/mod.rs: shared param types (WalletIdParams, EmptyParams, etc.)
- tools/network.rs: NetworkTool, ListWalletsTool
- tools/wallet.rs: GenerateReceiveAddress, WalletBalancesQuery,
SendCoreFunds, FetchPlatformBalances (new)
- tools/meta.rs: DescribeTool
Also cleans up dispatch.rs (removes resolve_wallet/task_error_to_mcp)
and simplifies server.rs (manual ServerHandler impl).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(mcp): add query_withdrawals tool for Platform withdrawal queries
Wraps existing PlatformInfoTask backend (CurrentWithdrawalsInQueue,
RecentlyCompletedWithdrawals) as a new MCP tool. Accepts status
parameter: "queued" (default) or "completed".
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(mcp): wait for SPV sync in query_withdrawals tool
Platform queries require proof verification which needs SPV to be
initialized. Without the sync wait, standalone CLI mode fails with
"SPV client not initialized" after exhausting retries.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix(mcp): override bare `true` in describe_tool outputSchema
schemars v1 derives bare `true` for serde_json::Value fields, which
Claude Code's MCP client rejects during tools/list schema validation.
Use `#[schemars(transform)]` to emit `{"type": "object"}` instead.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(mcp): add network verification guard to tools
Add an optional `network` parameter to wallet, platform, and listing
tools so MCP clients can assert the expected network before executing.
If the caller provides a network name that does not match the server's
active network, the request is rejected with an InvalidParams error.
Tools guarded: generate_receive_address, wallet_balances,
fetch_platform_address_balances, send_core_funds, query_withdrawals,
list_wallets.
Exempt (by design): network (discovery tool), describe_tool (meta).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: update MCP, CLI, and CONTRIBUTING for current tool set
- MCP.md: add all 8 tools to table (fetch_platform_address_balances,
query_withdrawals, describe_tool were missing); add network verification
section; add headless mode section; add Claude Code configuration example
- CLI.md: add examples for query-withdrawals, fetch-platform-address-balances,
describe-tool, and network guard usage; document headless subcommand
- CONTRIBUTING.md: add headless feature to feature flags table
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor(mcp): rename tools to domain_object_action convention
Adopt consistent naming: core_* for wallet/address operations,
platform_* for Platform queries, tool_* and network_* for meta/infra.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* docs: add MCP/CLI architecture decisions to CLAUDE.md
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: resolve merge artifacts after v1.0-dev merge
Network::Dash was renamed to Network::Mainnet in dashcore; update
two remaining references in src/mcp/server.rs that were missed during
the v1.0-dev merge.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Update src/mcp/server.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* fix(mcp): typed McpToolError, input validation, mandatory network on send
Replace String-based McpToolError with typed thiserror variants
(WalletNotFound, InvalidParam, NetworkMismatch, SpvSyncFailed,
TaskFailed, Internal) each mapped to a distinct MCP error code.
Add input validation at the MCP layer for SendCoreFunds: reject
zero amounts, validate Dash address prefix format, and require the
'network' parameter on destructive operations to prevent accidental
cross-network transfers.
Add INTENTIONAL(PROJ-003) annotation for tool_describe naming.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(mcp): auth hardening, API key minimum length, dispatch comment
- Enforce minimum 16-character MCP_API_KEY at server startup; refuse
to start and log error if too short.
- Return JSON body {"error": "unauthorized"} on 401 instead of bare
status code.
- Log failed auth attempts at warn level with client address (never
log the attempted key).
- Add ConnectInfo<SocketAddr> extraction for auth logging.
- Document why the dispatch channel receiver is intentionally dropped
in headless/CLI mode.
- Fix collapsible_if clippy lint in server.rs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* refactor: pub(crate) AppContext fields, deduplicate bash completion
- Change AppContext.data_dir and .network from pub to pub(crate) and
add data_dir()/network() accessor methods for external consumers.
- Extract the bash completion wrapper script to a shared constant in
completion.rs, eliminating duplication between generate and install.
- Document TOCTOU-safe pattern in DashMcpService::ctx() docstring
(each tool calls ctx() exactly once and passes the Arc snapshot).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix(config): log parse failures instead of silently swallowing them
Add tracing::debug! before .ok() when parsing per-network configs
from environment variables, so parse errors are visible in logs
instead of silently falling through to "no config found".
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* test(mcp): add unit tests for MCP validation layer
15 tests covering:
- API key length validation (too short rejected, 16+ accepted, empty)
- Amount validation (zero rejected, positive accepted)
- Address format validation (empty, valid mainnet/testnet, invalid prefix)
- McpToolError Display output for all variants
- MCP error code distinctness for custom codes
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs: headless build note, feature flag deps, MCP user stories
- Add note in MCP.md that headless mode requires building from source.
- Fix CONTRIBUTING.md feature flag table: clarify headless depends on
cli + mcp rather than claiming all features are independent.
- Add MCP-001 and MCP-002 user stories for CLI wallet management and
AI agent MCP server access.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* docs: add guide for exposing backend tasks via MCP/CLI
Add docs/EXPOSING_BACKEND_TASKS.md — a concise reference with architecture
rules, step-by-step checklist, and don'ts for adding new MCP tools that
wrap BackendTask variants. Cross-reference from CLAUDE.md and CONTRIBUTING.md.
https://claude.ai/code/session_01D4TGENSgE6fFCrXn5iPMnD
* fix: address review findings from CodeRabbit and Copilot
- Fix jq query in bash completion: use `input_schema` (snake_case)
instead of `inputSchema` (camelCase) to match rmcp serialization
- Validate hex seed hash exists in wallet map before accepting it,
preventing misleading errors for non-existent wallets
- Version-check cache in print_tool_help() to reject stale entries
- Add headless subcommand to custom help output (cfg-gated)
- Return non-zero exit for unknown tool help lookups
- Add Accept: application/json header to MCP curl examples
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address 7 PR review findings for MCP server
- docs/MCP.md: mark `network` as required for `core_funds_send`
- docs/CLI.md: add `network=testnet` to send example, remove misleading
"optional" example
- src/app.rs: initialize MCP HTTP server with context matching
`chosen_network` instead of hardcoded mainnet
- src/mcp/tools/wallet.rs: make `SendFundsParams.network` a non-optional
`String` to match runtime requirement
- docs/EXPOSING_BACKEND_TASKS.md: fix typo `InvalidParams` → `InvalidParam`
- CLAUDE.md: fix same `InvalidParams` → `InvalidParam` typo
- src/mcp/tests.rs: remove env var tests that race with parallel tests
due to unsynchronized `set_var`/`remove_var`
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* fix: address PR #773 review comments across MCP server
Bot issues fixed:
- #35: WalletNotFound stores only the identifier, not full error sentence
- #36: MCP log differentiates unset vs invalid MCP_API_KEY
- #37: Release workflow splits GUI and CLI builds (GUI without --features cli,
CLI with --features headless --bin det-cli)
- #38: SendCoreFunds validates empty network string before require_network
- #39: Remove unnecessary ensure_spv_synced() from QueryWithdrawals
- #40: Remove input_schema() override from NetworkTool
Human issues addressed:
- #41: Tool table includes det-cli command column
- #42: CLI section added to MCP.md
- #43: Headless mode performance characteristics documented
- #45: Authentication section with setup, behavior, and security guidance
- #46: ListWalletsTool moved from network.rs to wallet.rs
- #47: TODO added for withdrawal pagination (requires backend changes)
- #48: TODO added for structured JSON output (requires new backend variant)
- #49: Verified correct BackendTask dispatch for withdrawals
Issue #44 (CLAUDE.md duplication): no actual duplication found — the MCP
section in CLAUDE.md covers architecture guidance while docs/MCP.md covers
usage. Both serve distinct purposes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation