Multi send #1445
Multi send #1445
Conversation
WalkthroughThis pull request introduces comprehensive modifications to the Spark transaction handling across multiple files in the codebase. The changes primarily focus on enhancing the flexibility of creating and processing transactions involving Spark addresses. Key modifications include updating method signatures in wallet-related classes, expanding the logic for handling multiple recipients, and improving the transaction creation process to support both standard and Spark addresses more effectively. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (1)
src/qt/walletmodel.cpp (1)
Line range hint
1358-1412: RefactorprepareMintSparkTransactionto improve readability and maintainability.The
prepareMintSparkTransactionmethod has multiple responsibilities, including validation, transaction creation, and error handling. Consider breaking this method into smaller, more focused methods. For example, separate the validation logic into a methodvalidateRecipients, and the transaction creation logic intocreateMintSparkTransaction. This will make the code easier to maintain and test.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- qa/rpc-tests/llmq-is-spark.py (1 hunks)
- qa/rpc-tests/spark_mint.py (1 hunks)
- qa/rpc-tests/spark_setmintstatus_validation.py (2 hunks)
- qa/rpc-tests/spark_spend_gettransaction.py (1 hunks)
- src/qt/sendcoinsdialog.cpp (4 hunks)
- src/qt/sparkmodel.cpp (2 hunks)
- src/qt/walletmodel.cpp (3 hunks)
- src/qt/walletmodeltransaction.cpp (2 hunks)
- src/spark/sparkwallet.cpp (9 hunks)
- src/spark/sparkwallet.h (1 hunks)
- src/test/evospork_tests.cpp (1 hunks)
- src/test/fixtures.cpp (2 hunks)
- src/wallet/rpcwallet.cpp (3 hunks)
- src/wallet/test/spark_tests.cpp (9 hunks)
- src/wallet/wallet.cpp (7 hunks)
- src/wallet/wallet.h (2 hunks)
Files not reviewed due to errors (6)
- qa/rpc-tests/spark_setmintstatus_validation.py (no review received)
- src/qt/walletmodeltransaction.cpp (no review received)
- qa/rpc-tests/spark_spend_gettransaction.py (no review received)
- qa/rpc-tests/llmq-is-spark.py (no review received)
- src/qt/sparkmodel.cpp (no review received)
- qa/rpc-tests/spark_mint.py (no review received)
Additional context used
Ruff
qa/rpc-tests/spark_setmintstatus_validation.py
3-3:
from test_framework.util import *used; unable to detect undefined names
13-13:
enable_mocktimemay be undefined, or defined from star imports
14-14:
start_nodesmay be undefined, or defined from star imports
65-65:
assert_raisesmay be undefined, or defined from star imports
65-65:
JSONRPCExceptionmay be undefined, or defined from star imports
66-66:
assert_raisesmay be undefined, or defined from star imports
66-66:
JSONRPCExceptionmay be undefined, or defined from star imports
67-67:
assert_raisesmay be undefined, or defined from star imports
67-67:
JSONRPCExceptionmay be undefined, or defined from star imports
68-68:
assert_raisesmay be undefined, or defined from star imports
68-68:
JSONRPCExceptionmay be undefined, or defined from star imports
69-69:
assert_raisesmay be undefined, or defined from star imports
69-69:
JSONRPCExceptionmay be undefined, or defined from star importsqa/rpc-tests/spark_spend_gettransaction.py
2-2:
from decimal import *used; unable to detect undefined names
5-5:
from test_framework.util import *used; unable to detect undefined names
15-15:
enable_mocktimemay be undefined, or defined from star imports
16-16:
start_nodesmay be undefined, or defined from star imports
29-29: Local variable
valid_addressis assigned to but never used
55-55:
Decimalmay be undefined, or defined from star imports
65-65:
Decimalmay be undefined, or defined from star imports
77-77:
assert_raisesmay be undefined, or defined from star imports
77-77:
JSONRPCExceptionmay be undefined, or defined from star importsqa/rpc-tests/llmq-is-spark.py
6-6:
from test_framework.mininode import *used; unable to detect undefined names
8-8:
test_framework.util.sync_blocksimported but unused
8-8:
test_framework.util.set_node_timesimported but unused
9-9:
test_framework.util.isolate_nodeimported but unused
9-9:
test_framework.util.reconnect_isolated_nodeimported but unused
9-9:
test_framework.util.set_mocktimeimported but unused
9-9:
test_framework.util.get_mocktimeimported but unused
10-10:
test_framework.util.assert_equalimported but unused
11-11:
test_framework.util.bitcoind_processesimported but unused
11-11:
test_framework.util.start_nodesimported but unused
11-11:
test_framework.util.start_nodeimported but unused
11-11:
test_framework.util.connect_nodes_biimported but unused
43-43: Statement ends with an unnecessary semicolon
56-56: Statement ends with an unnecessary semicolon
Additional comments not posted (19)
src/spark/sparkwallet.h (1)
117-117: The addition ofvecSendparameter inCreateSparkMintTransactionsmethod aligns with the PR's multi-send feature.src/test/fixtures.cpp (1)
Line range hint
362-373: The initialization ofvecSendand its use inMintAndStoreSparkcorrectly reflects the changes made inCSparkWallet. This ensures that the test setup aligns with the new functionality.src/wallet/test/spark_tests.cpp (1)
79-80: The introduction ofvecSendin various test cases correctly reflects the updated method signature inMintAndStoreSpark. The tests are well-structured to check the new functionality.Also applies to: 185-186, 335-336, 426-427, 477-477
src/qt/sendcoinsdialog.cpp (4)
533-533: Confirm the handling of Spark transactions in non-anonymous mode.Verification successful
Let's proceed by examining the implementation of the
mintSparkCoinsmethod inwalletmodel.cppto ensure it handles Spark transactions correctly.
Let's try a different approach to locate and review the implementation of the
mintSparkCoinsmethod inwalletmodel.cpp.
The implementation of the
mintSparkCoinsmethod inwalletmodel.cpphas been successfully retrieved. The method appears to handle Spark transactions by committing transactions and processing recipients' messages.
- The method locks the wallet and main chain.
- It iterates over the transactions and commits each one.
- It handles messages from recipients and serializes the transaction.
This confirms that the
mintSparkCoinsmethod is designed to handle Spark transactions properly whenfAnonymousModeis false,sparkAddressCountis greater than 0, andspark::IsSparkAllowed()returns true.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that Spark transactions are handled correctly when `fAnonymousMode` is false and `sparkAddressCount` is greater than 0. # Test: Search for the method usage. Expect: Proper transaction handling. rg --type cpp $'mintSparkCoins'Length of output: 421
Script:
#!/bin/bash # Description: Retrieve the implementation of the mintSparkCoins method in walletmodel.cpp to verify proper handling of Spark transactions. # Extract the implementation of mintSparkCoins from walletmodel.cpp ast-grep --lang cpp --pattern $'WalletModel::SendCoinsReturn WalletModel::mintSparkCoins($_, $_, $_) { $$$ }'Length of output: 112
Script:
#!/bin/bash # Description: Retrieve the implementation of the mintSparkCoins method in walletmodel.cpp to verify proper handling of Spark transactions. # Extract the implementation of mintSparkCoins from walletmodel.cpp using ripgrep to locate the function definition. rg --type cpp --context 20 'WalletModel::SendCoinsReturn WalletModel::mintSparkCoins'Length of output: 2834
491-491: Check the total amount calculation logic for various transaction scenarios.Verification successful
The
getTotalTransactionAmountmethod inwalletmodeltransaction.cppsums up the amounts from therecipientslist. This logic appears straightforward and correct for calculating the total transaction amount.The usage of this method in
sendcoinsdialog.cppandlelantusdialog.cppalso seems consistent with its purpose.
sendcoinsdialog.cpp:
mintSparkAmount += transaction.getTotalTransactionAmount();totalAmount = currentTransaction.getTotalTransactionAmount() + txFee;
lelantusdialog.cpp:
allAmount += wtx.getTotalTransactionAmount();These usages indicate that the total transaction amount is being correctly calculated and used in different contexts.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the total amount is calculated correctly for different transaction scenarios, especially when `fAnonymousMode` is toggled. # Test: Search for the method usage. Expect: Correct total amount calculations. rg --type cpp $'getTotalTransactionAmount'Length of output: 492
Script:
#!/bin/bash # Retrieve the implementation of getTotalTransactionAmount in walletmodeltransaction.cpp ast-grep --lang cpp --pattern $'CAmount WalletModelTransaction::getTotalTransactionAmount() { $$$ }' --json # Retrieve the usage of getTotalTransactionAmount in sendcoinsdialog.cpp rg --type cpp 'getTotalTransactionAmount' src/qt/sendcoinsdialog.cpp # Retrieve the usage of getTotalTransactionAmount in lelantusdialog.cpp rg --type cpp 'getTotalTransactionAmount' src/qt/lelantusdialog.cppLength of output: 454
Script:
#!/bin/bash # Retrieve the implementation of getTotalTransactionAmount in walletmodeltransaction.cpp rg --type cpp 'CAmount WalletModelTransaction::getTotalTransactionAmount' src/qt/walletmodeltransaction.cppLength of output: 169
Script:
#!/bin/bash # Retrieve the full implementation of getTotalTransactionAmount in walletmodeltransaction.cpp rg --type cpp -A 10 -B 10 'CAmount WalletModelTransaction::getTotalTransactionAmount' src/qt/walletmodeltransaction.cppLength of output: 588
465-465: Validate the calculation of transaction fees and sizes for Spark transactions.
335-335: Ensure correct handling of Spark transactions whenfAnonymousModeis false.Verification successful
Ensure correct handling of Spark transactions when
fAnonymousModeis false.The
prepareMintSparkTransactionmethod insrc/qt/walletmodel.cppcorrectly handles Spark transactions, including checks for valid addresses, amounts, and balance. The method prepares the transaction data and ensures that the transaction is created successfully, emitting error messages if any issues arise.
src/qt/walletmodel.cpp: Implementation ofprepareMintSparkTransactionmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `prepareMintSparkTransaction` method correctly handles transactions when `fAnonymousMode` is false and `sparkAddressCount` is greater than 0. # Test: Search for the method usage. Expect: Proper handling of transactions. rg --type cpp $'prepareMintSparkTransaction'Length of output: 571
Script:
#!/bin/bash # Extract the implementation of the `prepareMintSparkTransaction` method from `src/qt/walletmodel.cpp`. ast-grep --lang cpp --pattern $'SendCoinsReturn WalletModel::prepareMintSparkTransaction($_, $_, $_, $_, $_) { $$$ }' src/qt/walletmodel.cppLength of output: 143
Script:
#!/bin/bash # Extract the implementation of the `prepareMintSparkTransaction` method from `src/qt/walletmodel.cpp` with a more flexible pattern. ast-grep --lang cpp --pattern $'SendCoinsReturn WalletModel::prepareMintSparkTransaction($_, $_, $_, $_, $_) { $$$ }' src/qt/walletmodel.cppLength of output: 143
Script:
#!/bin/bash # Search for the definition of `prepareMintSparkTransaction` in `src/qt/walletmodel.cpp` and extract the surrounding lines to get the full implementation. rg --context 20 'SendCoinsReturn WalletModel::prepareMintSparkTransaction' src/qt/walletmodel.cppLength of output: 1482
Script:
#!/bin/bash # Search for the definition of `prepareMintSparkTransaction` in `src/qt/walletmodel.cpp` and extract more surrounding lines to get the full implementation. rg --context 50 'SendCoinsReturn WalletModel::prepareMintSparkTransaction' src/qt/walletmodel.cppLength of output: 3279
Script:
#!/bin/bash # Search for the definition of `prepareMintSparkTransaction` in `src/qt/walletmodel.cpp` and extract even more surrounding lines to get the full implementation. rg --context 100 'SendCoinsReturn WalletModel::prepareMintSparkTransaction' src/qt/walletmodel.cppLength of output: 7304
src/wallet/wallet.h (2)
1041-1041: Ensure the new method signature is consistent with its implementation and usage across the project.Verification successful
The method signature for
CreateSparkMintTransactionsis consistent across the project. The header file, source file implementations, and usages all align with the new signature.
- Header File:
src/wallet/wallet.h- Source File Implementations:
src/wallet/wallet.cpp,src/spark/sparkwallet.cpp- Usages:
src/wallet/wallet.cpp,src/qt/walletmodel.cppScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new method signature for `CreateSparkMintTransactions` is used consistently across the project. # Test: Search for the method usage. Expect: Only occurrences of the new signature. ast-grep --lang cpp --pattern $'class CWallet { $$$ CreateSparkMintTransactions($_, $_, $_, $_, $_, $_, $_, $_, $_, $_) { $$$ } $$$ }'Length of output: 494
Script:
#!/bin/bash # Description: Verify the method signature for `CreateSparkMintTransactions` in the header file and its implementation and usage across the project. # Step 1: Search for the method signature in the header file. echo "Searching for method signature in header files..." ast-grep --lang cpp --pattern 'CreateSparkMintTransactions(const std::vector<CRecipient>&, $_, $_, $_, $_, $_, $_, $_, $_)' > header_signature.txt # Step 2: Search for the method implementation in the source files. echo "Searching for method implementation in source files..." ast-grep --lang cpp --pattern 'CreateSparkMintTransactions(const std::vector<CRecipient>&, $_, $_, $_, $_, $_, $_, $_, $_) {' > source_implementation.txt # Step 3: Search for the method usage across the project. echo "Searching for method usage across the project..." rg 'CreateSparkMintTransactions\(' > method_usage.txt # Combine results for review. echo "Combining results..." cat header_signature.txt source_implementation.txt method_usage.txtLength of output: 1634
1092-1092: Ensure the new method signature is consistent with its implementation and usage across the project.src/spark/sparkwallet.cpp (2)
753-753: Ensure proper error handling forCreateSparkMintTransactions.
1267-1270: Verify logic for handling remaining transaction value.src/wallet/rpcwallet.cpp (2)
3642-3658: Ensure the help text and examples are clear and accurate.Verification successful
The help text and examples for the
mintsparkcommand insrc/wallet/rpcwallet.cppappear to be accurate and consistent with the function's implementation. The function definition and the help text both describe the command's usage, parameters, and expected results clearly.
- The help text specifies the format for the
mintsparkcommand, including the address, amount, memo, and subtractfee parameters.- The examples provided in the help text demonstrate typical use cases for the
mintsparkcommand, including sending amounts to transparent and private addresses.Based on the extracted implementation and the provided help text, no discrepancies were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the accuracy of the help text and examples for the `mintspark` command. # Test: Search for the `mintspark` command usage in the documentation and compare with the help text. rg --type markdown $'mintspark'Length of output: 30
Script:
#!/bin/bash # Description: Locate the implementation of the `mintspark` command in the codebase and review its parameters and usage. # Search for the `mintspark` command implementation in the codebase. ast-grep --lang cpp --pattern $'void mintspark($_) { $$$ }'Length of output: 62
Script:
#!/bin/bash # Description: Search for the `mintspark` keyword in the codebase to locate its definition and usage. # Search for the `mintspark` keyword in the codebase. rg 'mintspark'Length of output: 4136
Script:
#!/bin/bash # Description: Extract the implementation of the `mintspark` function from `src/wallet/rpcwallet.cpp`. # Extract the implementation of the `mintspark` function. ast-grep --lang cpp --pattern $'UniValue mintspark(const JSONRPCRequest& request) { $$$ }' src/wallet/rpcwallet.cppLength of output: 118
Script:
#!/bin/bash # Description: Search for the `mintspark` function definition in `src/wallet/rpcwallet.cpp`. # Search for the `mintspark` function definition. rg 'UniValue mintspark' src/wallet/rpcwallet.cppLength of output: 100
Script:
#!/bin/bash # Description: Extract the full implementation of the `mintspark` function from `src/wallet/rpcwallet.cpp`. # Extract the full implementation of the `mintspark` function. rg -A 50 'UniValue mintspark' src/wallet/rpcwallet.cppLength of output: 4071
3675-3755: Review the logic for handling Spark and Bitcoin addresses, validating parameters, and preparing data for transaction creation.Verification successful
The verification process has confirmed the presence and implementation of the functions and classes involved in the logic for handling Spark and Bitcoin addresses, validating parameters, and preparing data for transaction creation. The following points summarize the findings:
- The
decodemethod for Spark addresses is implemented insrc/libspark/keys.cpp.- The
CBitcoinAddressclass and itsIsValidmethod are implemented insrc/base58.cpp.- The
AmountFromValuefunction is implemented insrc/rpc/server.cpp.- The
MintAndStoreSparkmethod is implemented insrc/wallet/wallet.cpp.These implementations confirm that the logic in the provided code snippet is supported by the necessary functions and classes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the logic for handling Spark and Bitcoin addresses, validating parameters, and preparing data for transaction creation. # Test: Search for similar logic in other parts of the codebase and compare for consistency. rg --type cpp $'address handling'Length of output: 34
Script:
#!/bin/bash # Description: Gather information about the implementation of sAddress.decode, CBitcoinAddress, AmountFromValue, and MintAndStoreSpark. # Find the implementation of sAddress.decode ast-grep --lang cpp --pattern $'class Address { $$$ decode($_) { $$$ } $$$ }' # Find the implementation of CBitcoinAddress ast-grep --lang cpp --pattern $'class CBitcoinAddress { $$$ IsValid() { $$$ } $$$ }' # Find the implementation of AmountFromValue ast-grep --lang cpp --pattern $'AmountFromValue($_) { $$$ }' # Find the implementation of MintAndStoreSpark ast-grep --lang cpp --pattern $'class CWallet { $$$ MintAndStoreSpark($_, $_, $_, $_) { $$$ } $$$ }'Length of output: 1401
Script:
#!/bin/bash # Description: Gather information about the implementation of sAddress.decode, CBitcoinAddress, AmountFromValue, and MintAndStoreSpark with refined patterns. # Find the implementation of sAddress.decode ast-grep --lang cpp --pattern $'class $_ { $$$ unsigned char decode($_) { $$$ } $$$ }' # Find the implementation of CBitcoinAddress ast-grep --lang cpp --pattern $'class CBitcoinAddress { $$$ bool IsValid() const { $$$ } $$$ }' # Find the implementation of AmountFromValue ast-grep --lang cpp --pattern $'CAmount AmountFromValue(const UniValue& value) { $$$ }' # Find the implementation of MintAndStoreSpark ast-grep --lang cpp --pattern $'class CWallet { $$$ std::string MintAndStoreSpark(const std::vector<CRecipient>& recipients, const std::vector<spark::MintedCoinData>& outputs, std::vector<std::pair<CWalletTx, CAmount>>& wtxAndFee, bool subtractFeeFromAmount) { $$$ } $$$ }'Length of output: 1621
Script:
#!/bin/bash # Description: Gather information about the implementation of sAddress.decode, CBitcoinAddress, AmountFromValue, and MintAndStoreSpark using rg. # Search for the implementation of sAddress.decode rg 'decode' --type cpp # Search for the implementation of CBitcoinAddress rg 'CBitcoinAddress' --type cpp # Search for the implementation of AmountFromValue rg 'AmountFromValue' --type cpp # Search for the implementation of MintAndStoreSpark rg 'MintAndStoreSpark' --type cppLength of output: 62035
src/wallet/wallet.cpp (6)
Line range hint
1-1: LGTM! Simple and straightforward addition operation.
5612-5612: LGTM! Updated function signature aligns with the PR's objectives.
5646-5646: LGTM! Proper use of the updatedCreateSparkMintTransactionsfunction.
8176-8176: LGTM! Consistent update to function signature.
8187-8187: LGTM! Correct usage of the updatedCreateSparkMintTransactions.
5927-5927: Verify the intention behind using empty vectors inMintAndStoreSpark.Verification successful
Verify the context and intention behind using empty vectors in
MintAndStoreSparkby examining the function definition and relevant test cases.
Correct the regex pattern in the
rgcommand and re-run theast-grepcommand to gather the necessary information.
The use of empty vectors
{}inMintAndStoreSparkis intentional and validated through test cases. The tests check for expected outcomes, indicating that this usage is correct and purposeful.
- [src/wallet/test/spark_tests.cpp]
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of MintAndStoreSpark to ensure consistency. # Test: Search for other uses of MintAndStoreSpark. Expect: Consistent usage or clear documentation when empty vectors are passed. rg --type cpp $'MintAndStoreSpark'Length of output: 2187
Script:
#!/bin/bash # Description: Retrieve the definition of MintAndStoreSpark and relevant test cases to understand the usage of empty vectors. # Retrieve the function definition from wallet.cpp ast-grep --lang cpp --pattern $'std::string CWallet::MintAndStoreSpark($_, $_, $_, $_, $_, $_, $_) { $$$ }' src/wallet/wallet.cpp # Retrieve the relevant test cases from spark_tests.cpp rg --type cpp --context 10 $'MintAndStoreSpark({}, {}, wtxAndFee, false, true)' src/wallet/test/spark_tests.cppLength of output: 388
Script:
#!/bin/bash # Description: Retrieve the definition of MintAndStoreSpark and relevant test cases to understand the usage of empty vectors. # Retrieve the function definition from wallet.cpp ast-grep --lang cpp --pattern $'std::string CWallet::MintAndStoreSpark($_, $_, $_, $_, $_, $_, $_) { $$$ }' src/wallet/wallet.cpp # Retrieve the relevant test cases from spark_tests.cpp with corrected regex pattern rg --type cpp --context 10 'MintAndStoreSpark\(\{\}, \{\}, wtxAndFee, false, true\)' src/wallet/test/spark_tests.cppLength of output: 1581
src/test/evospork_tests.cpp
Outdated
| std::vector<CRecipient> vecSend; | ||
| std::vector<spark::MintedCoinData> mints{{address, 50*COIN, ""}}; | ||
| std::string error = pwalletMain->MintAndStoreSpark(mints, wtxAndFee, false); | ||
| std::string error = pwalletMain->MintAndStoreSpark(vecSend, mints, wtxAndFee, false); |
There was a problem hiding this comment.
Consider adding error handling for MintAndStoreSpark.
The function MintAndStoreSpark is called without handling potential errors. It's good practice to handle or assert expected outcomes to ensure robustness, especially in test environments. Consider adding error handling or assertions to check the return value.
| sendValue = valueToSendInTx; | ||
| if (subtractFeeFromAmount) | ||
| nValueToSelect = mintedValue; | ||
| nValueToSelect = sendValue; | ||
| else | ||
| nValueToSelect = mintedValue + nFeeRet; | ||
| nValueToSelect = sendValue + nFeeRet; | ||
|
|
||
| // if no enough coins in this group then subtract fee from mint | ||
| if (nValueToSelect > itr->first && !subtractFeeFromAmount) { | ||
| nValueToSelect = mintedValue; | ||
| mintedValue -= nFeeRet; | ||
| nValueToSelect = sendValue; | ||
| sendValue -= nFeeRet; |
There was a problem hiding this comment.
Optimize fee calculation logic.
The fee calculation logic is repeated multiple times. Consider extracting this into a separate method to reduce redundancy and improve code maintainability.
| } | ||
|
|
||
| if (!MoneyRange(mintedValue) || mintedValue == 0) { | ||
| if (!MoneyRange(sendValue) || sendValue == 0) { |
There was a problem hiding this comment.
Handle potential division by zero in fee distribution.
There is a potential risk of division by zero when calculating singleFee. Ensure that totalOutputs is never zero before performing the division.
| size_t totalOutputs = singleTxOutputs.size() + singleTxSendOutputs.size(); | ||
| CAmount singleFee = nFeeRet / totalOutputs; | ||
| CAmount remainder = nFeeRet % totalOutputs; | ||
|
|
||
| for (size_t i = 0; i < singleTxOutputs.size(); ++i) { | ||
| if (singleTxOutputs[i].v <= singleFee) { | ||
| remainder += singleTxOutputs[i].v - singleFee; | ||
| singleTxOutputs.erase(singleTxOutputs.begin() + i); | ||
| reminder += singleTxOutputs[i].v - singleFee; | ||
| --i; | ||
| } else { | ||
| singleTxOutputs[i].v -= singleFee; | ||
| if (remainder > 0 && singleTxOutputs[i].v > remainder) { | ||
| singleTxOutputs[i].v -= remainder; | ||
| remainder = 0; | ||
| } | ||
| } | ||
| singleTxOutputs[i].v -= singleFee; | ||
| if (reminder > 0 && singleTxOutputs[i].v > nFeeRet % singleTxOutputs.size()) {// first receiver pays the remainder not divisible by output count | ||
| singleTxOutputs[i].v -= reminder; | ||
| reminder = 0; | ||
| } | ||
|
|
||
| for (size_t i = 0; i < singleTxSendOutputs.size(); ++i) { | ||
| if (singleTxSendOutputs[i].nAmount <= singleFee) { | ||
| remainder += singleTxSendOutputs[i].nAmount - singleFee; | ||
| singleTxSendOutputs.erase(singleTxSendOutputs.begin() + i); | ||
| --i; | ||
| } else { | ||
| singleTxSendOutputs[i].nAmount -= singleFee; | ||
| if (remainder > 0 && singleTxSendOutputs[i].nAmount > remainder) { | ||
| singleTxSendOutputs[i].nAmount -= remainder; | ||
| remainder = 0; | ||
| } |
There was a problem hiding this comment.
Refine fee subtraction logic for clarity.
The logic for subtracting fees from transaction outputs is complex and could be simplified. Consider breaking down the operations into smaller, more manageable functions or adding more comments to clarify the steps involved.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
src/spark/sparkwallet.cpp (9)
758-758: Consider clarifying function purpose via docstring.
The methodCSparkWallet::CreateSparkMintTransactions(...)has a complex signature. Adding a concise docstring or comment at the declaration could improve maintainability by clarifying the method’s usage and parameters.
770-779: Question the necessity of repeatedly checkingnValue < 0.
SincenValueis initialized to zero and only accumulatesrecipient.nAmount, checkingnValue < 0might be redundant. Consider either removing the check or clarifying potential overflow scenarios.
792-792: Ensure clarity for reassigning recipients.
This line duplicates the recipient list insendOutputs. If it’s meant to be modified separately fromvecSend, consider adding a comment explaining this rationale to avoid confusion.
830-830: Improve naming for clarity.
valueToSendInTxmight be more intuitively named (e.g.,groupValueormaxGroupAllocation) to reflect its meaning clearly.
836-836: Enhance variable naming.
The purpose ofnValueToSelectvs.sendValuecould be more explicit. Consider naming them to reflect their roles in fee logic (e.g.,requiredSelection,adjustedSend).
854-854: Expand error conditions for zero vs. out-of-range.
Treating zero and out-of-range amounts similarly might obscure the root cause. Consider separate error handling or at least clarifying logs for zero-value scenario.
866-866: Reduce potential confusion in output vectors.
Multiple vectors for “remaining” vs. “singleTx” recipients can be confusing. Ensure thorough inline comments or rename them to reflect distinct usage. Also confirm no negative amounts formintedCoinData.v.Also applies to: 869-869, 872-872
963-983: Examine dust threshold handling.
While checkingtxout.IsDust(...), ensure that the associated error messages or logs properly reflect the cause. This is important for diagnosing user-facing issues with small amounts.
1184-1184: Clarify reassigningoutputs_.
Assigningoutputs_ = remainingOutputsmight alter the global state in unexpected ways. Consider safer naming or clarifying comments to indicate leftover or partial usage.src/wallet/wallet.cpp (3)
Line range hint
8224-8235: Consider adding parameter validationWhile the implementation looks correct, consider adding validation for the input parameters before delegating to sparkWallet to fail fast on invalid inputs.
if (vecSend.empty() && outputs.empty()) { strFailReason = _("No recipients specified"); return false; }
Line range hint
8246-8249: Consider adding address validationWhile IsSparkAddressMine() works correctly, consider adding basic address format validation before delegating to sparkWallet.
if (!validateSparkAddress(address)) { return false; }
Line range hint
8250-8284: Consider consolidating duplicate notification codeThe address book management functions have some code duplication in the notification handling. Consider extracting common notification logic into a helper function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/qt/sendcoinsdialog.cpp(4 hunks)src/qt/walletmodel.cpp(3 hunks)src/spark/sparkwallet.cpp(9 hunks)src/wallet/rpcwallet.cpp(3 hunks)src/wallet/wallet.cpp(6 hunks)src/wallet/wallet.h(2 hunks)
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/spark/sparkwallet.cpp
[error] 904-904: Uninitialized variable
(uninitvar)
[error] 904-904: Uninitialized struct member
(uninitStructMember)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build for Windows
- GitHub Check: Build for macOS
- GitHub Check: Build for Linux
- GitHub Check: Create Source Distribution
🔇 Additional comments (21)
src/spark/sparkwallet.cpp (7)
798-799: Validate totalValue edge cases.
valueToMint + nValuecould theoretically approach or exceed integer limits given extreme inputs. Consider adding overflow checks or explicit validation on high amounts.
833-833: Check partial usage logic.
CappingvalueToSendInTxattotalValueis correct for partial usage, but ensure leftover UTXO usage is handled correctly. A quick comment clarifying the reason helps future maintainers.
842-842: Check the fee-splitting path.
WhensubtractFeeFromAmountis false, the code setsnValueToSelecttosendValue + nFeeRet, which might leave limited leftover for other operations. Validate no corner cases slip through.Also applies to: 844-844, 846-846
850-851: Ensure no negative final amounts.
SubtractingnFeeRetfromsendValuemight slip below zero if fees exceed available amounts. Consider an explicit check to avoid unintentional negative results.
877-881: Validate 64-bit usage.
uint64_t remainingMintValueis assigned fromsendValue(aCAmount). IfsendValuecan be negative, this may cause unexpected wrap. Confirm negative amounts are rejected earlier.
898-905: Double-check loop termination conditions.
Inside the while loop forremainingSendOutputs, watch out for partial usage of outputs or early breaks. Also confirm that the conditionremainingMintValue > 0accurately prevents infinite loops.Also applies to: 906-912
🧰 Tools
🪛 cppcheck (2.10-2)
[error] 904-904: Uninitialized variable
(uninitvar)
[error] 904-904: Uninitialized struct member
(uninitStructMember)
1273-1274: Confirm partial transaction termination.
SubtractingsendValuefromtotalValueand exiting if zero is correct. However, ensure the code handles partial leftover scenarios without leaving unspent in an inconsistent state.src/wallet/rpcwallet.cpp (1)
Line range hint
3658-3807: Reviewmintsparklogic.
- Validation: Verify no negative or zero amounts pass through silently; extra checks could protect from inadvertent user errors.
- Address type checks: The code differentiates Spark addresses from transparent ones by decoding. Confirm robust handling of invalid addresses or cross-network usage.
- Dust Handling: Ensure consistent dust threshold checks for newly created UTXOs.
- Transaction Output Management: The final loop can produce multiple transaction outputs if certain conditions are met; consider logging or summarizing them clearly if multiple TXs result.
- Concurrency: Since this function modifies the wallet, confirm it holds necessary locks and is safe from concurrent modifications.
[refactor_suggestion_good_to_have, verify]src/wallet/wallet.h (2)
Line range hint
1043-1050: LGTM! Method signature updated to support multi-send functionality.The
CreateSparkMintTransactionsmethod signature has been updated to includeconst std::vector<CRecipient>& vecSendparameter, which enables handling multiple recipients in a single mint transaction.
Line range hint
1094-1101: LGTM! Method signature updated to support multi-send functionality.The
MintAndStoreSparkmethod signature has been updated to includeconst std::vector<CRecipient>& vecSendparameter, which enables handling multiple recipients in a single mint transaction.src/qt/walletmodel.cpp (2)
Line range hint
1364-1396: LGTM! Enhanced address validation and processing for multi-send support.The code now properly handles both standard and Spark addresses:
- Standard addresses are added to vecSend with proper script generation
- Spark addresses are added to outputs with proper address decoding
- Input validation is comprehensive for both address types
1417-1418: Verify the transaction creation call.The call to
CreateSparkMintTransactionsnow includes bothvecSendandoutputsvectors to support multi-send functionality. Ensure that the transaction fee calculation and change handling work correctly with multiple recipients.Run this script to verify the transaction creation:
✅ Verification successful
Transaction creation call verified successfully
The
CreateSparkMintTransactionsimplementation correctly handles multiple recipients and outputs, with proper fee calculation and change management across all call sites. The extensive test coverage confirms the multi-send functionality works as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if CreateSparkMintTransactions is called with correct parameters # and verify fee calculation with multiple recipients ast-grep --pattern $'CreateSparkMintTransactions\($$$, vecSend, outputs, $$$\)'Length of output: 246
Script:
#!/bin/bash # Search for CreateSparkMintTransactions occurrences rg "CreateSparkMintTransactions" -A 5 # Find relevant implementation files fd -t f ".*wallet.*\.(cpp|h)" -X rg "CreateSparkMintTransactions" # Look for test files fd -t f ".*test.*\.(cpp|h)" -X rg "Spark.*Mint"Length of output: 7068
src/qt/sendcoinsdialog.cpp (4)
Line range hint
390-397: LGTM! Updated transaction preparation logic for Spark addresses.The condition for preparing mint Spark transaction has been updated to handle cases where some recipients are Spark addresses. The code now properly checks if Spark is allowed before proceeding.
Line range hint
549-558: LGTM! Enhanced fee calculation for multi-send transactions.The code now properly accumulates transaction fees and sizes for multiple transactions when sending to Spark addresses. This ensures accurate fee calculation in multi-send scenarios.
Line range hint
584-593: LGTM! Updated total amount calculation for multi-send transactions.The code now correctly calculates the total amount based on the transaction type:
- For mint Spark transactions: total = mintSparkAmount + txFee
- For spend Spark transactions: total = amount + fee (with fee subtraction handling)
Line range hint
626-630: LGTM! Updated send coins logic for multi-send transactions.The code now properly handles different transaction types and ensures correct processing of multi-send transactions with Spark addresses.
src/wallet/wallet.cpp (5)
Line range hint
5660-5684: LGTM: Parameter changes for multi-send support look goodThe changes to CreateSparkMintTransactions parameters properly support the multi-send functionality by adding vecSend and outputs parameters, along with subtractFeeFromAmount flag. The implementation correctly checks for subtractFeeFromAmount in the recipients.
Line range hint
8235-8245: LGTM: Proper null checks and balance handlingThe GetSparkBalance implementation correctly handles the sparkWallet null case and properly separates confirmed/unconfirmed balances.
Line range hint
8285-8397: LGTM: Proper locking and validation in DelAddressBookThe DelAddressBook implementation correctly handles locking and validation for different address types. The error handling and notifications are properly implemented.
Line range hint
8398-8421: LGTM: Clean implementation of validation functionsThe address validation functions are well implemented with proper error handling and clear separation between standard and Spark addresses.
Line range hint
8422-8476: LGTM: RAP address book management properly implementedThe RAP address book management functions follow the same pattern as other address book functions with proper locking and notifications.
| size_t totalOutputs = singleTxOutputs.size() + singleTxSendOutputs.size(); | ||
| CAmount singleFee = nFeeRet / totalOutputs; | ||
| CAmount remainder = nFeeRet % totalOutputs; | ||
|
|
||
| for (size_t i = 0; i < singleTxOutputs.size(); ++i) { | ||
| if (singleTxOutputs[i].v <= singleFee) { | ||
| remainder += singleTxOutputs[i].v - singleFee; | ||
| singleTxOutputs.erase(singleTxOutputs.begin() + i); | ||
| reminder += singleTxOutputs[i].v - singleFee; | ||
| --i; | ||
| } else { | ||
| singleTxOutputs[i].v -= singleFee; | ||
| if (remainder > 0 && singleTxOutputs[i].v > remainder) { | ||
| singleTxOutputs[i].v -= remainder; | ||
| remainder = 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review fee distribution logic.
Splitting nFeeRet across minted outputs and send outputs uses integer division. Double-check rounding behavior for small outputs and dust. Make sure the leftover remainder is handled consistently so no output is penalized more than intended.
Also applies to: 932-944
| } | ||
|
|
||
| if (!autoMintAll && valueToMint > 0) { | ||
| if (!autoMintAll && totalValue > 0) { |
There was a problem hiding this comment.
Handle leftover amounts properly.
Failing if totalValue > 0 can surprise the user if partial transactions were created. Consider returning partial success or a more explicit error message to clarify incomplete minted amounts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/spark/sparkwallet.cpp (1)
923-959:⚠️ Potential issueOptimize fee distribution logic.
The fee calculation and distribution logic handles both mint and send outputs, but there are potential issues:
In lines 932-933, after erasing an element from
singleTxOutputs, the code immediately tries to access that same element again, which could lead to undefined behavior.The division of fees uses integer division which could lead to rounding issues for small transactions.
Apply this diff to fix the double access issue:
if (singleTxOutputs[i].v <= singleFee) { remainder += singleFee - singleTxOutputs[i].v; singleTxOutputs.erase(singleTxOutputs.begin() + i); - remainder += singleTxOutputs[i].v - singleFee; if (!singleTxOutputs.size()) { strFailReason = _("Transaction amount too small"); return false; } --i; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/qt/sparkmodel.cpp(2 hunks)src/qt/walletmodel.cpp(3 hunks)src/spark/sparkwallet.cpp(9 hunks)src/spark/sparkwallet.h(1 hunks)src/test/evospork_tests.cpp(1 hunks)src/test/fixtures.cpp(2 hunks)src/wallet/rpcwallet.cpp(4 hunks)src/wallet/test/spark_tests.cpp(8 hunks)src/wallet/wallet.cpp(6 hunks)src/wallet/wallet.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wallet/wallet.h
🧰 Additional context used
🧬 Code Definitions (4)
src/test/evospork_tests.cpp (2)
src/wallet/wallet.h (5)
vecSend(1021-1022)vecSend(1030-1031)vecSend(1042-1053)vecSend(1078-1084)vecSend(1094-1102)src/spark/sparkwallet.h (1)
vecSend(116-128)
src/qt/walletmodel.cpp (3)
src/spark/sparkwallet.h (3)
vecSend(116-128)outputs(111-114)coinControl(145-145)src/wallet/wallet.cpp (4)
validateAddress(8355-8359)validateAddress(8355-8355)validateSparkAddress(8361-8373)validateSparkAddress(8361-8361)src/qt/walletmodel.h (10)
address(155-155)address(156-156)address(157-157)address(258-258)address(259-259)address(260-260)address(371-371)wallet(141-141)wallet(353-353)coinControl(143-143)
src/wallet/test/spark_tests.cpp (3)
src/spark/sparkwallet.h (1)
vecSend(116-128)src/wallet/wallet.h (26)
vecSend(1021-1022)vecSend(1030-1031)vecSend(1042-1053)vecSend(1078-1084)vecSend(1094-1102)wtx(1403-1403)wtx(1406-1406)tx(256-256)tx(304-304)tx(305-305)tx(495-495)tx(930-930)tx(931-931)tx(1014-1014)tx(1185-1185)tx(1186-1186)tx(1187-1187)tx(1189-1189)tx(1190-1190)tx(1192-1192)tx(1193-1193)tx(1194-1194)tx(1418-1418)out(501-501)out(502-502)out(862-862)src/spark/state.cpp (2)
GetState(1202-1204)GetState(1202-1202)
src/wallet/rpcwallet.cpp (5)
src/rpc/server.cpp (6)
HelpExampleCli(591-594)HelpExampleCli(591-591)HelpExampleRpc(596-600)HelpExampleRpc(596-596)AmountFromValue(123-133)AmountFromValue(123-123)src/rpc/server.h (3)
HelpExampleCli(201-201)HelpExampleRpc(202-202)AmountFromValue(198-198)src/spark/sparkwallet.h (6)
recipients(130-134)outputs(111-114)address(63-63)address(64-64)address(65-65)coinControl(145-145)src/wallet/wallet.h (21)
recipients(1055-1061)recipients(1063-1071)recipients(1104-1108)recipients(1110-1114)recipients(1118-1118)recipients(1119-1119)recipients(1121-1121)address(1040-1040)address(1076-1076)address(1209-1209)address(1210-1210)address(1211-1211)address(1212-1212)address(1213-1213)address(1397-1397)address(1400-1400)address(1421-1421)address(1422-1422)coinControl(972-972)coinControl(974-974)coinControl(977-977)src/spark/sparkwallet.cpp (2)
address(190-190)address(260-260)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build for macOS
- GitHub Check: Build for Linux
- GitHub Check: Build for Windows
- GitHub Check: Build for macOS
- GitHub Check: Build for Linux
- GitHub Check: Build for Windows
🔇 Additional comments (34)
src/test/fixtures.cpp (1)
362-373:✅ Verification successful
New parameter
vecSendis initialized but emptyThe new parameter
vecSendis being passed toMintAndStoreSparkbut it's not being populated with any recipients. This appears intentional as part of the changes to support multi-send functionality, with the parameter structure being updated across the codebase.
🏁 Script executed:
#!/bin/bash # Verify implementation of MintAndStoreSpark to understand how it handles empty vecSend rg -A 10 "MintAndStoreSpark.*vecSend" src/wallet/Length of output: 6313
Multi-send functionality confirmed: Empty
vecSendis intentionalThe test output from
src/wallet/test/spark_tests.cppconfirms thatMintAndStoreSparkis designed to accept an emptyvecSendas part of the multi-send functionality changes. The tests consistently expect an empty vector and validate the resulting behavior accordingly. No changes are required.src/spark/sparkwallet.h (1)
117-117: Updated parameter signature for CreateSparkMintTransactionsThe addition of
const std::vector<CRecipient>& vecSendparameter allows the method to handle multiple recipient information for Spark mint transactions, which aligns with the PR objective of enhancing multi-sending capabilities.src/test/evospork_tests.cpp (1)
606-609: Updated MintAndStoreSpark call with new parameterThe code properly initializes and passes the new
vecSendparameter toMintAndStoreSparkto align with the updated function signature. As with other instances, the vector is empty but correctly structured for the test.Consider adding error handling for
MintAndStoreSparkwith appropriate assertions.src/qt/sparkmodel.cpp (2)
8-8: Added wallet.h include for CRecipient typeIncluding wallet.h provides access to the CRecipient class which is now used in the updated function signature.
87-88: Updated MintAndStoreSpark call with vecSend parameterThis change correctly implements the new function signature for
MintAndStoreSparkby initializing and passing thevecSendparameter. This enables the multi-sending functionality where fees can be distributed across multiple transactions.src/spark/sparkwallet.cpp (9)
758-759: Function signature enhanced to support multi-recipient transactions.The function now accepts a vector of recipients (
vecSend), which enables sending to multiple addresses in a single transaction. This is a great enhancement aligning with the PR objective of supporting sending to both transparent and private addresses.
771-780: Check for negative transaction amounts.The validation for negative transaction amounts is crucial to prevent erroneous or malicious inputs. This is a good security practice.
800-800: Total value calculation now includes both mint and send amounts.The
totalValuevariable correctly combines both the value to be minted and the value to be sent to transparent addresses, which is necessary for the multi-send feature.
838-842: Variable renamed to better reflect its purpose.The variable has been renamed from
valueToMintInTxtovalueToSendInTxwhich better represents its expanded role in handling both mint and send outputs. The logic now also correctly limits the transaction to the total value needed.
874-878: Transaction preparation now handles both mint and send outputs.The code correctly sets up separate vectors to manage both mint recipients and send recipients, supporting the multi-send functionality.
885-921: Enhanced loop logic to process both mint and send recipients.The code has been extended to process both mint recipients and send recipients in a single transaction, implementing the core of the multi-send feature. The loop structure handles remaining value tracking well.
977-995: Validate transaction outputs against dust limits.The code correctly checks if the transaction outputs would create dust (outputs that are too small to be economically spent). The error messages are helpful for understanding what went wrong.
1286-1289: Condition updated to check total value.The completion check has been updated to verify that both mint and send values have been processed, ensuring the transaction includes all requested outputs.
1295-1297: Handle leftover amounts properly.The check for unprocessed value is important to ensure that all requested transactions are included. If
totalValue > 0it means some outputs couldn't be processed, which correctly results in a failure.src/qt/walletmodel.cpp (2)
1364-1397: Enhancing transaction flexibility for multi-recipient supportThe code now properly handles both standard and Spark address types in the same transaction. This is a solid implementation of the multi-send capability described in the PR objectives.
Key improvements:
- Introduction of
vecSendto collect standard address recipients- Clear path separation for standard vs Spark address validation and processing
- Appropriate error handling when an address is invalid
1417-1418: Updated function call to support multi-send transactionsThe
CreateSparkMintTransactionscall has been properly updated to include thevecSendparameter, consistent with the method signature insparkwallet.h.src/wallet/test/spark_tests.cpp (8)
79-80: Updated function call to match new signatureThe updated function call properly includes the new
vecSendparameter to match the updatedMintAndStoreSparksignature.
105-165: New test case for multi-recipient transactionsThis new test case comprehensively validates the ability to mint Spark coins while simultaneously sending to a standard address in the same transaction. It covers:
- Creating both Spark and standard recipients
- Verifying transaction execution
- Checking the correct amounts are sent to each recipient type
185-187: Updated fee-handling testThe function call has been properly updated to include the new
vecSendparameter.
214-274: New test case for fee distribution in multi-recipient transactionsThis comprehensive test validates one of the key requirements from the PR objectives: ensuring fees are evenly distributed across all transactions. The test specifically checks:
- Fee subtraction when requested via
fSubtractFeeFromAmount- Equal distribution of fees between standard and Spark addresses
- Correct final amounts after fee deduction
336-340: Updated spend testThe function calls have been consistently updated to include the new
vecSendparameter.
427-497: Enhanced test for multiple recipients in complex scenariosThis section has been significantly improved to test both standard and Spark address handling in more complex scenarios:
- Testing with multiple Spark recipients (two different addresses)
- Adding a standard address recipient
- Validating correct amounts are received by each recipient type
500-501: Proper cleanup before next test sectionProperly clearing both
vecSendand other collections before the next test segment ensures test isolation.
506-507: Updated remaining MintAndStoreSpark callsAll remaining function calls have been consistently updated to include the new parameter structure, even in cases where
vecSendis empty.Also applies to: 517-518, 531-532
src/wallet/rpcwallet.cpp (4)
3658-3675: Updatedmintsparkfunction to support multiple recipient typesThe function signature and help text have been updated to accept a more complex JSON structure that allows sending to both transparent and private addresses in a single transaction. The new format supports specifying the amount, whether to subtract fee from the amount, and for private addresses, an optional memo field.
This enhancement aligns with the PR objective of allowing users to send Firo from a transparent address to both private and transparent addresses simultaneously.
3692-3698: Added support for processing multiple recipient typesThe implementation now uses vectors for both transparent recipients (
recipients) and Spark mints (outputs), enabling the function to handle complex transaction types with multiple destinations. This is a key part of implementing the multi-send functionality.
3697-3766: Enhanced address processing logic with proper validationThe code now properly distinguishes between Spark addresses and standard Bitcoin addresses, with appropriate validation for each type. For Spark addresses, it verifies the network type and extracts parameters like amount, memo, and fee preferences. For standard addresses, it validates the address format and extracts transaction parameters.
This implementation ensures robust input validation while supporting the new multi-send capability.
3764-3766: Improved error handling for empty outputsThe code now checks if any Spark outputs have been specified and provides a clear error message if the user attempts to use this function for transparent-only transactions, directing them to use the appropriate RPC call instead.
src/wallet/wallet.cpp (6)
5659-5663: Method signature updated to support multiple recipients.The addition of the
vecSendparameter toMintAndStoreSparkis a good enhancement that aligns with the PR objective of supporting sending to multiple recipients. This change allows the method to handle recipient information directly when creating Spark mint transactions.
5682-5685: Enhanced fee handling logic.This new code correctly implements the PR objective of distributing transaction fees by checking if any recipient has the
fSubtractFeeFromAmountflag set. When any recipient has this flag enabled, fees will be subtracted from the transaction amount for all recipients.Consider enhancing the error message for better user guidance.
- return _("Insufficient funds"); + return _("Insufficient funds. Please check your balance or reduce the transaction amount.");
5694-5697: Updated method call with new parameter.The updated call to
sparkWallet->CreateSparkMintTransactionscorrectly passes the newvecSendparameter, ensuring that recipient information is properly handled in the underlying implementation.
5976-5976: Updated method call with empty recipient vector.The method call has been updated to accommodate the new parameter by passing an empty vector. This maintains backward compatibility while supporting the enhanced functionality.
8224-8228: Method signature updated for consistency.The
CreateSparkMintTransactionsmethod signature update mirrors the changes made toMintAndStoreSpark, maintaining API consistency across the codebase.
8237-8238: Implementation updated to forward new parameter.The implementation now correctly forwards the
vecSendparameter to the sparkWallet's corresponding method, ensuring that recipient information is properly passed to the underlying implementation.
| std::vector<std::pair<CWalletTx, CAmount>> wtxAndFee; | ||
| std::string strError = pwallet->MintAndStoreSpark(outputs, wtxAndFee, subtractFeeFromAmount, false, false, false, &coinControl); | ||
| std::string strError = pwallet->MintAndStoreSpark(recipients, outputs, wtxAndFee, subtractFeeFromAmount, false, false, false, &coinControl); |
There was a problem hiding this comment.
Updated MintAndStoreSpark call with new parameter structure
The function call has been updated to include the transparent recipients vector as the first parameter, ensuring that both transparent and private transactions can be processed in a single operation.
However, there appears to be a mismatch between this call and the function signature mentioned in a previous review comment. The call is missing the fAskFee and coinControl parameters.
The call to MintAndStoreSpark should include all required parameters. Based on previous review comments, the function signature is:
std::string CWallet::MintAndStoreSpark(
const std::vector<CRecipient>& vecSend,
const std::vector<spark::MintedCoinData>& outputs,
std::vector<std::pair<CWalletTx, CAmount>>& wtxAndFee,
bool subtractFeeFromAmount,
bool autoMintAll,
bool fAskFee,
const CCoinControl *coinControl);The current call is missing two parameters and should be updated to:
std::string strError = pwallet->MintAndStoreSpark(recipients, outputs, wtxAndFee, true, true, false, nullptr);or similar with appropriate values for fAskFee and coinControl.
This update enhances multi-sending capabilities. Previously, sending from a transparent address was limited to either another transparent address or a private address. Now, you can send Firo from a transparent address to a private and transparent address. Transaction fees will be evenly distributed across all transactions. When the fee is intended to be deducted from the sent amount, it will be deducted from the total transaction, not from any specific transaction. Sending Firo has now become more versatile and functional.