feat: fund with wallet balance #92
Conversation
WalkthroughThe pull request introduces multiple changes across several files, primarily focusing on enhancing wallet functionality, including balance retrieval, identity registration, and user interface improvements. Key modifications include adjustments to how balances are fetched, the introduction of new parameters for transaction handling, and refinements in UI rendering for better user experience. The changes also involve updates to error handling and logic for managing wallet identities and transactions, ensuring a more robust and user-friendly application. 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 (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (14)
src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs (1)
Line range hint
42-75: Consider enhancing user feedbackWhile the implementation is solid, consider these improvements for better user experience:
- Add immediate validation feedback for the funding amount input
- Show loading indicators during state transitions
- Display the minimum/maximum allowed funding amounts
Example implementation for validation feedback:
self.render_funding_amount_input(ui); // Extract the step from the RwLock to minimize borrow scope let step = self.step.read().unwrap().clone(); let Ok(amount) = self.funding_amount.parse::<f64>() else { + ui.label(egui::RichText::new("Please enter a valid number").color(egui::Color32::RED)); return action; }; + +// Add minimum/maximum amount validation +if let Some(selected_wallet) = &self.selected_wallet { + let wallet = selected_wallet.read().unwrap(); + let max_balance = wallet.max_balance() as f64 * 1e-8; + if amount > max_balance { + ui.label(egui::RichText::new( + format!("Amount exceeds wallet balance of {:.8} DASH", max_balance) + ).color(egui::Color32::RED)); + return action; + } +}src/backend_task/core/refresh_wallet_info.rs (2)
Line range hint
1-89: Consider performance optimizations for large-scale operationsThe current implementation processes addresses and UTXOs sequentially, with individual database operations for each UTXO. For wallets with many addresses or UTXOs, this could impact performance.
Consider these optimizations:
- Batch database operations instead of individual inserts
- Process address balances in parallel using async/await
- Add progress tracking for long-running operations
Example batch implementation:
// Collect UTXO data first let utxo_data: Vec<(Vec<u8>, i64, String, i64, Vec<u8>)> = utxo_map .iter() .filter_map(|(outpoint, tx_out)| { Address::from_script(&tx_out.script_pubkey, self.network) .ok() .map(|address| { ( outpoint.txid.as_ref().to_vec(), outpoint.vout, address.to_string(), tx_out.value, tx_out.script_pubkey.to_bytes(), ) }) }) .collect(); // Batch insert into database self.db.batch_insert_utxos(&utxo_data, self.network) .map_err(|e| e.to_string())?;
Line range hint
40-43: Improve error handling for balance fetch failuresThe current implementation silently continues when balance fetching fails for an address, which could lead to incorrect total balances.
Consider these improvements:
- Err(e) => { - eprintln!("Error fetching balance for address {}: {:?}", address, e); - } + Err(e) => { + let err_msg = format!("Error fetching balance for address {}: {:?}", address, e); + eprintln!("{}", err_msg); + // Either accumulate errors or notify the user + wallet.write() + .map_err(|e| e.to_string())? + .add_balance_fetch_error(address.clone(), err_msg)?; + }src/ui/identities/withdraw_from_identity_screen.rs (2)
Line range hint
190-207: LGTM with suggestions for improved error handlingThe Credits calculation logic correctly handles the conversion using the 1e11 factor. However, there are opportunities to improve error handling and input validation.
Consider these improvements:
let parts: Vec<&str> = self.withdrawal_amount.split('.').collect(); let mut credits: u128 = 0; // Process the whole number part if it exists. if let Some(whole) = parts.first() { - if let Ok(whole_number) = whole.parse::<u128>() { + match whole.parse::<u128>() { + Ok(whole_number) => { credits += whole_number * 100_000_000_000; // Whole Dash amount to credits + } + Err(e) => { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + format!("Invalid whole number part: {}", e) + ); + return; + } } } // Process the fractional part if it exists. if let Some(fraction) = parts.get(1) { let fraction_length = fraction.len(); - let fraction_number = fraction.parse::<u128>().unwrap_or(0); + let fraction_number = match fraction.parse::<u128>() { + Ok(num) => num, + Err(e) => { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + format!("Invalid fraction part: {}", e) + ); + return; + } + }; + if fraction_length > 11 { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + "Too many decimal places. Maximum is 11.".to_string() + ); + return; + } // Calculate the multiplier based on the number of digits in the fraction. let multiplier = 10u128.pow(11 - fraction_length as u32); credits += fraction_number * multiplier; // Fractional Dash to credits }
Line range hint
271-275: Consider adding pre-confirmation validationsWhile the confirmation popup includes validations, moving these checks earlier in the flow would provide faster feedback to users.
Consider adding these validations before showing the confirmation popup:
// Withdraw button let button = egui::Button::new(RichText::new("Withdraw").color(Color32::WHITE)) .fill(Color32::from_rgb(0, 128, 255)) .frame(true) .rounding(3.0) .min_size(egui::vec2(60.0, 30.0)); if ui.add(button).clicked() { + // Validate withdrawal amount + if let Err(e) = self.validate_withdrawal_amount() { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage(e); + return; + } + + // Validate address if provided + if !self.withdrawal_address.is_empty() { + if let Err(e) = Address::from_str(&self.withdrawal_address) { + self.withdraw_from_identity_status = WithdrawFromIdentityStatus::ErrorMessage( + format!("Invalid withdrawal address: {}", e) + ); + return; + } + } + self.confirmation_popup = true; }src/backend_task/identity/register_identity.rs (2)
414-414: Consider adding error handling for identity insertionWhile the identity insertion into the wallet's map is correctly placed after successful registration, consider adding error handling for potential map insertion failures. This would make the code more robust.
- wallet.identities.insert(wallet_identity_index, identity); + wallet.identities + .insert(wallet_identity_index, identity) + .map_err(|e| format!("Failed to insert identity into wallet: {}", e))?;
Line range hint
241-290: Consider refactoring duplicated asset lock proof waiting logicThe asset lock proof waiting logic is duplicated in both
FundWithWalletandFundWithUtxobranches. Consider extracting this into a separate method to improve maintainability and reduce code duplication.+ impl AppContext { + async fn wait_for_asset_lock_proof(&self, tx_id: Hash) -> Result<AssetLockProof, String> { + loop { + { + let proofs = self.transactions_waiting_for_finality.lock().unwrap(); + if let Some(Some(proof)) = proofs.get(&tx_id) { + return Ok(proof.clone()); + } + } + tokio::time::sleep(Duration::from_millis(200)).await; + } + } + }Then use it in both branches:
- let asset_lock_proof; - 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; - } - } - tokio::time::sleep(Duration::from_millis(200)).await; - } + let asset_lock_proof = self.wait_for_asset_lock_proof(tx_id).await?;Also applies to: 306-355
src/model/wallet/mod.rs (2)
245-249: Consider improving readability with intermediate steps.The implementation is correct but could be more readable. Consider breaking down the nested operations:
- self.utxos - .values() - .map(|outpoints_to_tx_out| outpoints_to_tx_out.values().map(|tx_out| tx_out.value)) - .flatten() - .sum::<Duffs>() + self.utxos.values().flat_map(|outpoints_to_tx_out| { + // Get all transaction outputs for this address + outpoints_to_tx_out.values().map(|tx_out| tx_out.value) + }).sum::<Duffs>()
Line range hint
4-4: Add tests for wallet functionality.The TODO comment indicates missing tests. Given the critical nature of wallet operations, comprehensive tests are essential.
Would you like me to help create test cases covering:
- Balance calculation
- Key derivation
- Address management
- Error handling scenarios
src/ui/wallet/wallets_screen/mod.rs (1)
Line range hint
1-24: Consider architectural improvementsThe current implementation could benefit from several architectural improvements:
Separate business logic from UI:
- Move balance conversion and validation logic to a dedicated module
- Create a view model layer to handle data preparation
Improve error handling:
- Add proper error types instead of using strings
- Implement systematic error handling for all operations
Consider splitting the file into smaller modules:
- Extract table-related code into a separate module
- Move filter logic to a dedicated component
Would you like assistance in creating a modular architecture proposal or implementing any of these improvements?
src/ui/identities/add_new_identity_screen/mod.rs (1)
Line range hint
1-1183: Consider architectural improvements for better maintainability.The code structure could benefit from the following improvements:
- The extensive use of nested RwLocks could lead to potential deadlocks. Consider implementing a more structured locking strategy.
- There are several commented-out code blocks that should either be removed or documented with clear TODOs explaining their future purpose.
- Error handling could be more descriptive and consistent throughout the code.
Consider:
- Implementing a dedicated lock manager to prevent deadlocks
- Using structured error types instead of Option for error messages
- Adding comprehensive documentation for public methods
- Removing or properly documenting commented-out code sections
Would you like me to provide specific examples for any of these improvements?
src/model/wallet/utxos.rs (1)
49-58: Refactor duplicated code into a helper function to improve maintainabilityThe code for removing UTXOs from the wallet is duplicated in two places (lines 49-58 and 69-77). Extracting this logic into a helper function would reduce duplication and enhance readability.
Consider creating a new function, such as:
fn remove_utxos_from_wallet( utxos: &mut BTreeMap<Address, HashMap<OutPoint, TxOut>>, utxos_to_remove: &Vec<(Address, OutPoint)> ) { for (address, outpoint) in utxos_to_remove { if let Some(outpoints) = utxos.get_mut(address) { outpoints.remove(outpoint); if outpoints.is_empty() { utxos.remove(address); } } } }And replace the duplicated code with calls to this function:
// Before - for (address, outpoint) in utxos_to_remove { - if let Some(outpoints) = utxos.get_mut(&address) { - outpoints.remove(&outpoint); - if outpoints.is_empty() { - utxos.remove(&address); - } - } - } // After + remove_utxos_from_wallet(utxos, &utxos_to_remove);Also applies to: 69-77
src/model/wallet/asset_lock_transaction.rs (2)
19-19: Consider documenting the new parameterallow_take_fee_from_amount.Adding a doc comment explaining the purpose and expected behavior of this parameter would improve the function's readability and maintainability.
42-43: Consider providing a more descriptive error message.The error message
"take_unspent_utxos_for() returned None"may not give sufficient context to the caller. Providing additional details about why UTXOs could not be retrieved (e.g., insufficient funds) could improve error handling and debuggability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
src/backend_task/core/refresh_wallet_info.rs(1 hunks)src/backend_task/identity/register_identity.rs(3 hunks)src/cpu_compatibility.rs(2 hunks)src/model/wallet/asset_lock_transaction.rs(2 hunks)src/model/wallet/mod.rs(1 hunks)src/model/wallet/utxos.rs(2 hunks)src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs(1 hunks)src/ui/identities/add_new_identity_screen/mod.rs(1 hunks)src/ui/identities/withdraw_from_identity_screen.rs(1 hunks)src/ui/wallet/wallets_screen/mod.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/cpu_compatibility.rs
🔇 Additional comments (13)
src/ui/identities/add_new_identity_screen/by_using_unused_balance.rs (2)
33-33: LGTM: UI spacing improvement
The added spacing provides better visual separation between the wallet balance display and the funding amount prompt.
40-41: Verify step numbering sequence
The step increment has been moved after the UI heading. While this is logically sound, please verify that this doesn't affect the step numbering sequence in the broader identity registration flow.
✅ Verification successful
Let me analyze the step number increments in the identity registration flow to verify if the change affects the sequence.
Step number increment placement is correct and consistent
The step number increment in by_using_unused_balance.rs follows the same pattern as in mod.rs where the increment happens after displaying the step heading and before rendering the corresponding UI component. The change maintains consistency with the broader identity registration flow where each step follows this sequence:
- Display heading with current step number
- Increment step number
- Render UI component for that step
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other step number increments in the identity registration flow
rg -A 3 "step_number.*\+=" src/ui/identities/
Length of output: 1563
Script:
#!/bin/bash
# Get more context around the step number increments to understand the UI flow
rg -B 3 -A 5 "step_number.*\+=" src/ui/identities/add_new_identity_screen/
Length of output: 3637
src/backend_task/core/refresh_wallet_info.rs (1)
33-33: Verify the impact of removing minimum confirmations parameter
The change from Some(0) to None in get_received_by_address could affect balance calculations. This might change which transactions are included in the balance based on their confirmation status.
Consider the following:
- Document the expected behavior with
None - Verify if this aligns with other balance-related features in the codebase
- Consider edge cases like handling of unconfirmed transactions
Run this script to check for related balance calculation logic:
src/ui/identities/withdraw_from_identity_screen.rs (1)
Line range hint 108-113: Verify the conversion factor change from 1e-8 to 1e-11
The change in conversion factor from 1e-8 to 1e-11 appears to be related to the Credits system used in the confirmation popup. However, this change needs verification to ensure it aligns with the Dash protocol's requirements.
- Add a comment explaining the conversion factor:
+ // Convert from Credits (1e-11 DASH) to DASH. We subtract 5000000 credits to account for transaction fees
let expected_max_amount = self.max_amount.saturating_sub(5000000) as f64 * 1e-11;- Let's verify the conversion factor usage across the codebase:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The conversion factor change from 1e-8 to 1e-11 is correct and consistent with Credits usage
The codebase shows a clear pattern:
1e-8is used for regular DASH conversions (100,000,000 satoshis = 1 DASH)1e-11is specifically used for Credits-based operations, confirmed by:identities_screen.rsusing1e-11for identity balance displaywithdraw_from_identity_screen.rsusing100_000_000_000(1e11) for Credits conversion- Proper imports of
dash_sdk::dpp::fee::Creditstype in relevant files
The conversion change aligns with the Credits system implementation across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of DASH/Credits conversion factors
# Search for conversion factors
echo "Searching for conversion factors..."
rg -A 2 '1e-8|1e-11|100_000_000|100_000_000_000'
# Search for Credits type usage
echo "Searching for Credits type usage..."
ast-grep --pattern 'Credits'
Length of output: 4233
src/backend_task/identity/register_identity.rs (2)
192-194: LGTM! Consistent parameter usage
The new parameter is consistently applied in both the initial attempt and error recovery path, ensuring uniform fee handling behavior.
180-182: Verify documentation for the new fee parameter
A new boolean parameter has been added to the asset_lock_transaction call. Please ensure this parameter's purpose and behavior are properly documented in the wallet implementation.
✅ Verification successful
Let me gather more specific information about the asset_lock_transaction method implementation to verify the new parameter.
Let me get the complete implementation to understand how this parameter is used.
The new boolean parameter is correctly implemented and used
The allow_take_fee_from_amount parameter is properly implemented in the wallet's asset_lock_transaction method. It controls whether the transaction fee (3000 satoshis) can be deducted from the specified amount when there aren't enough funds to cover both amount and fee. The implementation correctly adjusts the actual transaction amount when this parameter is true and there's no change output.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new parameter is documented in the wallet implementation
# Search for the asset_lock_transaction method definition and documentation
ast-grep --pattern 'fn asset_lock_transaction($$$)'
# Search for any related documentation or comments
rg -B 3 'asset_lock_transaction'
Length of output: 12355
Script:
#!/bin/bash
# Get the implementation of asset_lock_transaction method
ast-grep --pattern 'pub fn asset_lock_transaction(
$$$
) -> $$ {
$$$
}'
# Search for any test cases or usage examples
rg -B 2 -A 5 'asset_lock_transaction\(' src/model/wallet/
Length of output: 672
Script:
#!/bin/bash
# Get the complete implementation of the asset_lock_transaction method
rg -B 5 -A 50 'pub fn asset_lock_transaction\(' src/model/wallet/asset_lock_transaction.rs
# Also check for any documentation comments above the method
rg -B 10 -A 2 'pub fn asset_lock_transaction\(' src/model/wallet/asset_lock_transaction.rs
Length of output: 2707
src/model/wallet/mod.rs (2)
Line range hint 251-256: LGTM! Clear and actionable error message.
The error message clearly indicates what action the user needs to take.
245-249: Verify balance calculation consistency.
The change from using address_balances to utxos for balance calculation is significant. Ensure that:
- The balance is correctly updated when UTXOs change
- The
address_balancesfield stays in sync with UTXO-based calculations
Also applies to: 19-19
✅ Verification successful
Let me gather more information about the UTXO handling to verify the balance calculation consistency.
Based on the gathered information, I can now provide the final response:
The balance calculation is correctly synchronized between UTXOs and address_balances
The codebase shows that:
- The
max_balance()method correctly calculates total balance from UTXOs - The
update_address_balance()method maintains synchronization by:- Updating the in-memory
address_balancesmap - Persisting changes to the database
- Updating the in-memory
- UTXO modifications (insertions/removals) are properly tracked and the corresponding address balances are updated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential inconsistencies between address_balances and utxos usage
rg -A 5 'address_balances'
rg -A 5 'update_address_balance'
# Check if there are any direct balance calculations not using the max_balance method
ast-grep --pattern 'balance = $$$'
Length of output: 6387
Script:
#!/bin/bash
# Check UTXO-related operations and balance calculations
rg -A 10 'fn.*balance'
rg -A 10 'utxos.*insert'
rg -A 10 'utxos.*remove'
# Check for UTXO updates and their relationship with address balances
ast-grep --pattern 'self.utxos.$$$($$$)'
Length of output: 13079
src/model/wallet/asset_lock_transaction.rs (5)
26-26: Updating return type to Option<Address> is appropriate.
The change to return Option<Address> seems correct to handle cases where there might be no change address. Ensure that any calling code properly handles the None case.
64-74: Properly handling change output and address
The logic to create a change output when change_option is Some and to handle the None case appropriately is correct.
85-87: Constructing transaction inputs from UTXOs
Inputs are correctly constructed from the collected UTXOs.
105-106: Initializing sighash_u32 correctly
The initialization of sighash_u32 appears correct and is appropriately used in signature hashing.
56-60:
Verify that assigning actual_amount to both outputs is correct
Assigning actual_amount to both payload_output and burn_output results in total outputs summing to 2 * actual_amount. Ensure that this does not exceed the total input value minus fees. There is a risk that the transaction outputs may exceed the available inputs.
Please confirm that this is intentional and that the transaction remains valid. If not, adjust the output values accordingly.
| row.col(|ui| { | ||
| let dash_balance = data.balance as f64 * 1e-8; | ||
| ui.label(format!("{:.8}", dash_balance)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve balance conversion implementation
The current implementation has several potential issues:
- Uses magic number
1e-8without explanation - Direct floating-point multiplication could lead to precision issues
- No validation for potential overflow scenarios
Consider implementing a dedicated conversion function:
+ const DUFFS_PER_DASH: f64 = 1e8;
+
+ /// Converts an amount from Duffs to DASH
+ /// Returns None if the conversion would overflow
+ fn duffs_to_dash(duffs: u64) -> Option<f64> {
+ let dash = (duffs as f64) / DUFFS_PER_DASH;
+ if dash.is_finite() {
+ Some(dash)
+ } else {
+ None
+ }
+ }
+
row.col(|ui| {
- let dash_balance = data.balance as f64 * 1e-8;
- ui.label(format!("{:.8}", dash_balance));
+ match duffs_to_dash(data.balance) {
+ Some(dash_balance) => ui.label(format!("{:.8}", dash_balance)),
+ None => ui.label("Error: Invalid balance"),
+ }
});Committable suggestion skipped: line range outside the PR's diff.
| header.col(|ui| { | ||
| let label = if self.sort_column == SortColumn::Balance { | ||
| match self.sort_order { | ||
| SortOrder::Ascending => "Total Received (DASH) ^", | ||
| SortOrder::Descending => "Total Received (DASH) v", | ||
| } | ||
| } else { | ||
| "Total Received (DASH)" | ||
| }; | ||
| if ui.button(label).clicked() { | ||
| self.toggle_sort(SortColumn::Balance); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Fix inconsistencies in column headers and sorting logic
There are several issues in the column header implementation:
- The sorting enum
SortColumn::Balanceis used for "Total Received" column, which is inconsistent. - Both Balance and TotalReceived columns show the same header "Total Received (DASH)".
- The term "Total Received" might be misleading for a balance column.
Consider applying these changes:
- let label = if self.sort_column == SortColumn::Balance {
+ let label = if self.sort_column == SortColumn::Balance {
match self.sort_order {
- SortOrder::Ascending => "Total Received (DASH) ^",
- SortOrder::Descending => "Total Received (DASH) v",
+ SortOrder::Ascending => "Balance (DASH) ^",
+ SortOrder::Descending => "Balance (DASH) v",
}
} else {
- "Total Received (DASH)"
+ "Balance (DASH)"
};Also, consider updating the SortColumn enum to better reflect the column's purpose:
enum SortColumn {
Address,
- Balance,
+ CurrentBalance,
UTXOs,
TotalReceived,
// ... rest of the enum
}Committable suggestion skipped: line range outside the PR's diff.
| if has_balance { | ||
| if ui | ||
| .selectable_value( | ||
| &mut *funding_method, | ||
| FundingMethod::UseWalletBalance, | ||
| "Use Wallet Balance", | ||
| ) | ||
| .changed() | ||
| { | ||
| if let Some(wallet) = &self.selected_wallet { | ||
| let wallet = wallet.read().unwrap(); // Read lock on the wallet | ||
| let max_amount = wallet.max_balance(); | ||
| self.funding_amount = format!("{:.4}", max_amount as f64 * 1e-8); | ||
| } | ||
| let mut step = self.step.write().unwrap(); // Write lock on step | ||
| *step = AddNewIdentityWalletFundedScreenStep::ReadyToCreate; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving wallet balance handling and validation.
The wallet balance funding implementation could be enhanced in several ways:
- The funding amount formatting uses fixed precision which might truncate significant digits.
- There's no validation to ensure the wallet has sufficient balance for transaction fees.
- The satoshi-to-DASH conversion factor is hardcoded.
Consider applying these improvements:
if has_balance {
if ui
.selectable_value(
&mut *funding_method,
FundingMethod::UseWalletBalance,
"Use Wallet Balance",
)
.changed()
{
if let Some(wallet) = &self.selected_wallet {
let wallet = wallet.read().unwrap();
let max_amount = wallet.max_balance();
- self.funding_amount = format!("{:.4}", max_amount as f64 * 1e-8);
+ // Reserve some balance for transaction fees
+ let reserved_for_fees = 1_000_000; // 0.01 DASH
+ let available_amount = max_amount.saturating_sub(reserved_for_fees);
+ const SATS_PER_DASH: f64 = 100_000_000.0;
+ self.funding_amount = format!("{}", available_amount as f64 / SATS_PER_DASH);
}
let mut step = self.step.write().unwrap();
*step = AddNewIdentityWalletFundedScreenStep::ReadyToCreate;
}
}Committable suggestion skipped: line range outside the PR's diff.
| let utxos = &mut self.utxos; | ||
|
|
||
| let mut required: i64 = amount as i64; | ||
| let mut required: i64 = (amount + fee) as i64; |
There was a problem hiding this comment.
Avoid casting from u64 to i64 to prevent potential integer overflows
Casting u64 values to i64 can lead to integer overflows when dealing with large amounts, potentially causing incorrect calculations or negative values. It's safer to use u128 or consistently use u64 to handle large values without loss of precision.
Apply the following changes to use u128 instead of i64:
- let mut required: i64 = (amount + fee) as i64;
+ let mut required: u128 = amount + fee as u128;
...
- required -= tx_out.value as i64;
+ required = required.saturating_sub(tx_out.value as u64);
...
- if required > 0 {
+ if required > 0 {
...
- let total_collected = (amount + fee) as i64 - required;
+ let total_collected = (amount + fee) as u128 - required;
...
- let adjusted_amount = amount as i64 - missing_fee;
+ let adjusted_amount = amount as u128 - missing_fee;
...
- let total_input = (amount + fee) as i64 - required;
+ let total_input = (amount + fee) as u128 - required;
...
// Adjust all subsequent calculations and conditionals to use `u128`.Also applies to: 28-28, 39-39, 44-44, 79-79
| output: { | ||
| let mut outputs = vec![burn_output]; | ||
| if let Some(change_output) = change_output { | ||
| outputs.push(change_output); | ||
| } | ||
| outputs | ||
| }, |
There was a problem hiding this comment.
payload_output is not included in the transaction outputs
The transaction outputs currently include burn_output and optionally change_output, but payload_output is not added to the outputs. This might be unintended, as payload_output should likely be included in the transaction outputs.
Apply this diff to include payload_output in the outputs:
let mut outputs = vec![burn_output];
+ outputs.push(payload_output);
if let Some(change_output) = change_output {
outputs.push(change_output);
}Committable suggestion skipped: line range outside the PR's diff.
| let actual_amount = if change_option.is_none() && allow_take_fee_from_amount { | ||
| // The amount has been adjusted by taking the fee from the amount | ||
| // Calculate the adjusted amount based on the total value of the UTXOs minus the fee | ||
| let total_input_value: u64 = utxos.iter().map(|(_, (tx_out, _))| tx_out.value).sum(); | ||
| total_input_value - fee | ||
| } else { | ||
| amount | ||
| }; |
There was a problem hiding this comment.
Ensure total_input_value is greater than or equal to fee to prevent underflow
When calculating actual_amount, if total_input_value is less than fee, subtracting fee could cause an underflow. Consider adding a check to ensure total_input_value >= fee before performing the subtraction.
Apply this diff to add the check:
let actual_amount = if change_option.is_none() && allow_take_fee_from_amount {
// The amount has been adjusted by taking the fee from the amount
// Calculate the adjusted amount based on the total value of the UTXOs minus the fee
let total_input_value: u64 = utxos.iter().map(|(_, (tx_out, _))| tx_out.value).sum();
+ if total_input_value < fee {
+ return Err("Total input value is less than the fee.".to_string());
+ }
total_input_value - fee
} else {
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.
| let actual_amount = if change_option.is_none() && allow_take_fee_from_amount { | |
| // The amount has been adjusted by taking the fee from the amount | |
| // Calculate the adjusted amount based on the total value of the UTXOs minus the fee | |
| let total_input_value: u64 = utxos.iter().map(|(_, (tx_out, _))| tx_out.value).sum(); | |
| total_input_value - fee | |
| } else { | |
| amount | |
| }; | |
| let actual_amount = if change_option.is_none() && allow_take_fee_from_amount { | |
| // The amount has been adjusted by taking the fee from the amount | |
| // Calculate the adjusted amount based on the total value of the UTXOs minus the fee | |
| let total_input_value: u64 = utxos.iter().map(|(_, (tx_out, _))| tx_out.value).sum(); | |
| if total_input_value < fee { | |
| return Err("Total input value is less than the fee.".to_string()); | |
| } | |
| total_input_value - fee | |
| } else { | |
| amount | |
| }; |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
UI/UX Changes