Mint refactor #1515
Mint refactor #1515
Conversation
levonpetrosyan93
commented
Dec 26, 2024
levonpetrosyan93
commented
- Added support sendFrom for mintSpark rpc as requested in issue [Request][Suggestion] New CLI command 'mintsparkfrom' #1500
- now mintSpark is not splitting, and added option in Options dialog in Qt.
WalkthroughThe pull request introduces a new configuration option for splitting coins during the minting process in the Bitcoin wallet. This feature is implemented across multiple files in the Qt interface, options model, wallet model, and core wallet functionality. The changes add a new checkbox in the options dialog, update the options model to persist the setting, and modify various wallet methods to support the new Changes
Sequence DiagramsequenceDiagram
participant User
participant OptionsDialog
participant OptionsModel
participant WalletModel
participant SparkWallet
User->>OptionsDialog: Toggle fSplit checkbox
OptionsDialog->>OptionsModel: Save fSplit setting
OptionsModel-->>User: Persist setting
User->>WalletModel: Initiate Spark Mint
WalletModel->>SparkWallet: CreateSparkMintTransactions(fSplit)
SparkWallet-->>WalletModel: Process transactions
WalletModel-->>User: Complete minting
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/wallet/rpcwallet.cpp (2)
Line range hint
3673-3681: Handle 'from' addresses carefully.This new block selectively reads the third parameter (
request.params[2]) for specifying the coin control addresses inmintspark. Consider adding explicit error checks or fallback for cases of malformed or empty arrays, preventing silent failures if the user inputs invalid addresses.
3755-3755: Check for error propagation.The returned
strErrorshould be logged or thrown to ensure that silent failures don't occur. If it’s critical for the user to know, consider a JSONRPCError for more direct feedback.src/qt/optionsmodel.h (1)
77-77: Getter method forfSplit.Exposing
getfSplit()is consistent with other getters. Consider adding a short comment explaining what 'splitting' does in the minting context.src/wallet/test/spark_tests.cpp (3)
210-210: Reusable pattern for minted coins.The second mint reuses the same mintedCoins vector. Confirm that resetting or re-initializing mintedCoins between tests is consistent, preventing hidden state issues.
213-213: Multiple mints in quick succession.Calling
MintAndStoreSparktwice checks multiple transaction creation. Ensure test also inspects final wallet balance or mint states to catch concurrency or stateful race conditions.
322-322: Large-value mint coverage.600 FIRO is a bigger test. It's good for boundary checks (e.g., large transactions). Ensure all minted outputs are recognized, and the test checks for memory usage or splitting across multiple transactions.
src/qt/forms/optionsdialog.ui (1)
192-197: UI addition for enabling splitting
The new checkbox and label for splitting enhances user functionality. Ensure that help text, accessibility labels, and translations are properly updated so users understand the purpose of enabling splitting when minting.src/wallet/wallet.h (1)
Line range hint
1050-1097: Add documentation for the newfSplitparameter.
It's recommended to include a brief explanation in the function comments (e.g., doxygen) describing how thefSplitparameter affects Spark mint transactions. This will help future maintainers and developers understand the purpose of the parameter and any implications for callers.src/qt/optionsmodel.cpp (2)
286-287: Expose and test the newSplitcase.
ReturningfSplitprovides the correct integration for the UI. Make sure there is enough test coverage to confirm that the UI properly reads and applies this setting in all relevant flows.
438-441: Emit signals or notify changes whenfSplitis updated.
Currently, there is no separate signal emission for thefSplitstate change. If the code needs real-time updates in other components, consider adding a custom signal (e.g.,fSplitChanged) to notify interested parts of the UI.src/spark/sparkwallet.cpp (2)
765-765: Provide a high-level comment forCreateSparkMintTransactionsusage.
Since this function grew to incorporate thefSplitlogic, it would be helpful to add a short comment describing what “split” does at a functional level, especially given how the function’s branching logic changes the UTXO selection strategy.
794-804: Extract or refactor to improve maintainability.
The if-else branching forfSplitvs. non-split paths is readable but fairly large. Consider splitting each approach into its own helper method or reorganizing for clarity, especially if more conditions accumulate in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/qt/forms/optionsdialog.ui(1 hunks)src/qt/optionsdialog.cpp(1 hunks)src/qt/optionsmodel.cpp(3 hunks)src/qt/optionsmodel.h(3 hunks)src/qt/walletmodel.cpp(1 hunks)src/rpc/client.cpp(1 hunks)src/spark/sparkwallet.cpp(2 hunks)src/spark/sparkwallet.h(1 hunks)src/test/evospork_tests.cpp(1 hunks)src/test/fixtures.cpp(1 hunks)src/wallet/rpcwallet.cpp(6 hunks)src/wallet/test/spark_tests.cpp(7 hunks)src/wallet/wallet.cpp(3 hunks)src/wallet/wallet.h(2 hunks)
🔇 Additional comments (23)
src/wallet/rpcwallet.cpp (5)
34-34: Header inclusion check.
Including "wallet/coincontrol.h" is proper for accessing coin control functionality.
3657-3657: Validate request parameter counts in mintspark.
Good to see the function checks for valid parameter counts. Consider explicitly handling cases where the user might pass fewer/more parameters with a clear error message suggesting the expected usage.
3731-3732: Clarify usage of wtxAndFee structure.
A new vector of <CWalletTx, CAmount> is introduced. This approach nicely bundles each minted transaction with its fee. Ensure consistent usage and correct fee aggregation across all call sites.
3733-3753: Refine parameter flow and error handling.
Within this block, the logic for calling MintAndStoreSpark with various arguments is introduced, including coin control. Verify that any errors from the method are properly logged/relayed back (e.g., strError usage) and that partial failures (if any) are handled gracefully.
5804-5804: Registering the mintspark command.
Adding the new RPC command to CRPCCommand is correct. Double-check that the function pointer and argument count align with the changes (additional parameters) introduced in mintspark.
src/qt/optionsmodel.h (2)
52-52: Introducing Split option.
The new Split enum entry clarifies the feature for future readers. Ensure it’s documented in the associated settings help text.
94-94: New private member fSplit.
Storing the split setting in OptionsModel is consistent with the approach for other wallet options. Ensure it has a default initialization that matches user expectations.
src/spark/sparkwallet.h (1)
125-125: fSplit parameter in CreateSparkMintTransactions.
Adding the bool fSplit parameter clarifies that splitting behavior is controlled externally. Be mindful to document its effects on mint transaction creation, especially for testers and maintainers.
src/rpc/client.cpp (1)
201-202: JSON conversion for additional mintspark parameters.
Enabling JSON parsing for the second and third arguments simplifies usage of arrays/objects. Confirm that the server-side code robustly handles this parsed data (e.g., for coin control addresses).
src/wallet/test/spark_tests.cpp (5)
79-79: Extended MintAndStoreSpark call in mint_and_store_spark.
The added true parameter indicates the splitting behavior is tested here. Ensure the test covers scenarios where no splitting is expected vs. splitting is active.
122-122: Test for subtract-fee with splitting.
Passing true for both subtractFeeFromAmount and fSplit is valuable for coverage. Confirm that fees are deducted correctly in combined scenarios.
311-311: Testing 10 FIRO mint.
Passing false, true ensures that the test scenario with no fee subtraction but splitting is validated. Confirm that the resulting minted sum matches expectations.
333-333: Automint check.
Empty mintedCoins triggers auto mint. Setting false, true, true clarifies the test for automatically minting all available funds while splitting. Ensure that leftover UTXOs are accounted for if partial failures happen.
347-347: Unique change script test.
Verifying that each minted transaction uses a unique change script is a good approach to detect collisions or unexpected merges. Confirm no accidental script reuse.
src/test/fixtures.cpp (1)
372-372: Ensure proper handling of the newly added split parameter
Including the fSplit parameter (true in this call) changes the transaction behavior. Confirm that relevant code paths handle this scenario by verifying the returned error string and ensuring all usage sites of MintAndStoreSpark are updated accordingly.
src/qt/optionsdialog.cpp (1)
191-191: Mapping fSplit to the model is consistent
This addition correctly reflects the new fSplit option in the user interface, allowing the setting to be saved and restored by the OptionsModel.
src/test/evospork_tests.cpp (1)
607-607: Propagate and check the fSplit parameter usage
Similarly to other calls, this addition includes fSplit=true. Make sure all test scenarios and downstream code properly handle this parameter, especially related to splitting logic in Spark transactions.
src/qt/optionsmodel.cpp (1)
88-91: Confirm the default for fSplit.
Right now, the default is set to true. If the user might expect otherwise (e.g., keep the existing behavior of not splitting), consider double-checking or adding a tooltip or label clarifying the default.
src/qt/walletmodel.cpp (1)
1410-1410: Verify fSplit usage in prepareMintSparkTransaction.
You're directly passing optionsModel->getfSplit() to CreateSparkMintTransactions. Confirm that all callers and UI paths that rely on non-splitting behavior now explicitly handle the scenario when fSplit is false.
src/wallet/wallet.cpp (4)
5660-5660: New parameter added to CreateSparkMintTransactions signature
The addition of the fSplit boolean parameter appears to be for controlling transaction splitting behavior in Spark mint transactions.
5687-5687: Parameter properly passed through to sparkWallet implementation
The fSplit parameter is correctly forwarded to the sparkWallet's CreateSparkMintTransactions method.
8224-8228: Parameter added to method declaration and passed through
The fSplit parameter is properly added to the method declaration and passed through to the underlying implementation.
Line range hint 5660-8228: Verify consistent usage of new fSplit parameter
The changes look good but we should verify the parameter is used consistently throughout the codebase.
✅ Verification successful
Let me check the implementation in sparkwallet.cpp to verify the parameter is used correctly.
Let me check how the fSplit parameter is used in the implementation.
Let me check the header file to verify the parameter is documented correctly.
Let me check one final thing - the usage in the Qt wallet code.
The fSplit parameter is used consistently across the codebase
Based on the verification:
- The parameter is correctly passed through in wallet.cpp to sparkWallet->CreateSparkMintTransactions()
- The implementation in sparkwallet.cpp properly uses fSplit to control coin selection behavior:
- When fSplit is true, it uses AvailableCoinsForLMint() and shuffles the outputs
- When false, it uses regular AvailableCoins()
- The Qt wallet correctly passes the user's split preference via optionsModel->getfSplit()
- The parameter is properly declared in all relevant header files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other usages of CreateSparkMintTransactions to ensure they're updated
rg "CreateSparkMintTransactions" --type cpp
Length of output: 948
Script:
#!/bin/bash
# Check implementation in sparkwallet.cpp
rg -A 20 "bool CSparkWallet::CreateSparkMintTransactions" src/spark/sparkwallet.cpp
Length of output: 734
Script:
#!/bin/bash
# Search for fSplit usage in sparkwallet.cpp
rg -A 50 "fSplit" src/spark/sparkwallet.cpp
Length of output: 3093
Script:
#!/bin/bash
# Check header file documentation
rg -A 10 "CreateSparkMintTransactions" src/spark/sparkwallet.h
Length of output: 508
Script:
#!/bin/bash
# Check Qt wallet implementation context
rg -B 5 -A 10 "CreateSparkMintTransactions.*optionsModel" src/qt/walletmodel.cpp
Length of output: 787
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wallet/rpcwallet.cpp (1)
Line range hint
3657-3673: Help text needs to be updatedThe help text should be updated to reflect the new
sendFromparameter and its usage.- if (request.fHelp || request.params.size() == 0 || request.params.size() > 3) + if (request.fHelp || request.params.size() == 0 || request.params.size() > 4) throw std::runtime_error( - "mintspark {\"address\":{amount,memo...}}\n" + "mintspark {\"address\":{amount,memo...}} ( subtractfeefromamount [\"fromaddress\",...] )\n" + HelpRequiringPassphrase(pwallet) + "\n" "\nArguments:\n" " {\n" " \"address\":amount (numeric or string) The Spark address is the key, the numeric amount (can be string) in " + CURRENCY_UNIT + " is the value\n" " ,...\n" " }\n" + "2. subtractfeefromamount (boolean, optional) Subtract fee from amount\n" + "3. fromaddresses (array, optional) A json array with addresses to spend from\n" + " [\"address\",...] (string) The addresses to spend from\n" "\nResult:\n"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/wallet/rpcwallet.cpp(6 hunks)
🔇 Additional comments (4)
src/wallet/rpcwallet.cpp (4)
34-34: LGTM!
Added include for coin control functionality which is needed for the new sendFrom parameter.
3731-3753: LGTM!
The implementation of the new sendFrom functionality using CCoinControl looks correct:
- Creates coin control instance
- Gets available coins
- Filters coins by provided addresses
- Selects matching outputs for spending
3755-3755: LGTM!
Updated MintAndStoreSpark call to pass the new coin control parameter.
5804-5804: LGTM!
Added registration for the new mintspark RPC command with correct permissions.
