Fixed fee calculation for the second transaction when spending spark funds to EX-address #1465
Conversation
WalkthroughThe primary change involves the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SendCoinsDialog
participant CWallet
User ->> SendCoinsDialog: Click send button
SendCoinsDialog ->> CWallet: GetMinimumFee(nTxConfirmTarget)
CWallet -->> SendCoinsDialog: returns calculated fee
SendCoinsDialog ->> User: Displays confirmation dialog
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/qt/sendcoinsdialog.cpp (1 hunks)
Additional comments not posted (2)
src/qt/sendcoinsdialog.cpp (2)
363-363: EnsurenTxConfirmTargetis correctly initialized before use.The variable
nTxConfirmTargetis used in theGetMinimumFeemethod call, but it's not clear from the provided code snippet where it is initialized and whether it is set appropriately based on the transaction context. This could potentially lead to incorrect fee calculations.Verification successful
nTxConfirmTargetis correctly initialized before use.The variable
nTxConfirmTargetis declared as anexternvariable in the header filesrc/wallet/wallet.hand is initialized with a default value insrc/wallet/wallet.cpp. This ensures that it is properly set before being used in theGetMinimumFeemethod call.
- Declaration:
src/wallet/wallet.h- Initialization:
src/wallet/wallet.cppScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of `nTxConfirmTarget` in the function. # Test: Search for the initialization of `nTxConfirmTarget`. Expect: Initialization before usage. rg --type cpp -C 3 'int nTxConfirmTarget =|nTxConfirmTarget =' src/qt/sendcoinsdialog.cppLength of output: 91
Script:
#!/bin/bash # Description: Search for all occurrences of `nTxConfirmTarget` in the codebase to trace its initialization and usage. # Test: Find all occurrences of `nTxConfirmTarget` in the repository. rg 'nTxConfirmTarget' --type cpp --type hppLength of output: 71
Script:
#!/bin/bash # Description: Search for all occurrences of `nTxConfirmTarget` in the codebase to trace its initialization and usage. # Test: Find all occurrences of `nTxConfirmTarget` in the repository. rg 'nTxConfirmTarget' --type cppLength of output: 2782
363-363: Validate handling ofextraFeein transaction amount calculations.The calculated
extraFeeis added directly to the transaction amount for the new recipient. This approach assumes that the extra fee is always required and correctly calculated, which might not be the case. It’s crucial to ensure that this does not lead to overcharging the user or underpaying the transaction fee.
| @@ -360,7 +360,7 @@ void SendCoinsDialog::on_sendButton_clicked() | |||
| pwalletMain->SetAddressBook(newKey.GetID(), "", "receive"); | |||
| intermediateAddressScript = GetScriptForDestination(newKey.GetID()); | |||
|
|
|||
| extraFee = CWallet::GetMinimumFee(secondTxSize, 0, mempool); | |||
| extraFee = CWallet::GetMinimumFee(secondTxSize, nTxConfirmTarget, mempool); | |||
There was a problem hiding this comment.
Adjust the fee calculation logic for better clarity and accuracy.
The addition of extraFee calculation using CWallet::GetMinimumFee is crucial for handling the fees correctly when transactions involve EX-addresses. However, the logic here is complex and could benefit from further explanation or simplification to ensure it's maintainable and clear to other developers.
// Refactor to encapsulate fee calculation in a separate function for clarity
CAmount calculateExtraFee(uint32_t txSize, int confirmTarget) {
return CWallet::GetMinimumFee(txSize, confirmTarget, mempool);
}
// Usage
extraFee = calculateExtraFee(secondTxSize, nTxConfirmTarget);
PR intention
Calculate the fee for the second tx correctly