Qt: Make arbitrary funds private with Spark - #1891
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Summary by CodeRabbit
WalkthroughThe PR updates Spark privacy terminology, adds an interactive OverviewPage mint flow, centralizes wallet-operation threading in ChangesSpark privacy flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeAnt-AI review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:L
User descriptionPR intentionMake the Spark privacy action understandable without requiring users to know what Spark is, and let them choose how much of their transparent balance to make private. Code changes brief
Testing
CodeAnt-AI DescriptionLet users choose how much transparent FIRO to make private with Spark What Changed
Impact
💡 Usage GuideChecking Your Pull RequestEvery time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later. Talking to CodeAnt AIGot a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask: This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code. ExamplePreserve Org Learnings with CodeAntYou can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input: This helps CodeAnt AI learn and adapt to your team's coding style and standards. ExampleRetrigger reviewAsk CodeAnt AI to review the PR again, by typing: Check Your Repository HealthTo analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health. |
Sequence DiagramThis PR replaces the all-or-nothing privacy action with an amount-based flow that prepares a Spark transaction, reviews the exact fee, and commits the transfer without freezing the GUI. sequenceDiagram
participant User
participant Overview
participant Wallet
User->>Overview: Select Make Private
Overview-->>User: Show amount dialog
User->>Overview: Enter amount or choose Max
Overview->>Wallet: Prepare Spark transaction in background
Wallet-->>Overview: Return private amount and exact fee
Overview-->>User: Review transfer and fee
User->>Overview: Confirm Make Private
Overview->>Wallet: Commit Spark transaction in background
Wallet-->>User: Show funds moving to private balance
Generated by CodeAnt AI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/qt/guiutil.cpp (1)
110-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant join and assert the GUI-thread precondition.
Line 121 joins the worker, and the
Cleanupdestructor joins again through thejoinable()guard. The behavior is correct, but the duplicate join hides the RAII intent. LetCleanupown the join.
QApplication::setOverrideCursor,restoreOverrideCursor, and the nestedQEventLooprequire the GUI thread. The helper is now a public API, so a caller can invoke it from a worker thread. Add a precondition check. The assertion has no side effects.♻️ Proposed refactor
void runWalletOperation(const std::function<void()>& operation) { + assert(QThread::currentThread() == qApp->thread()); std::exception_ptr exception; QEventLoop waitLoop; @@ } cleanup{worker}; waitLoop.exec(QEventLoop::ExcludeUserInputEvents); - worker.join(); + worker.join(); // join before rethrowing so the operation is complete if (exception) std::rethrow_exception(exception); }Keep one join only. If you keep line 121, drop the join from
Cleanupand guard the destructor for the exceptional path instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/qt/guiutil.cpp` around lines 110 - 123, Remove the explicit worker.join() after waitLoop.exec() and let Cleanup’s destructor own the single join. Add a side-effect-free GUI-thread precondition assertion at the start of the helper, before QApplication cursor operations or nested event-loop execution, using the project’s established assertion mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/qt/forms/optionsdialog.ui`:
- Around line 188-191: Update the Spark privacy option labels in the options
dialog, including the label near “Enable automatic Spark privacy” and the other
Spark privacy label, so they use different keyboard mnemonics instead of both
defining Alt+P. Preserve the existing text and assign one label a unique
mnemonic.
In `@src/qt/overviewpage.cpp`:
- Around line 365-407: Wrap both GUIUtil::runWalletOperation calls in
on_anonymizeButton_clicked with exception handling so exceptions from
prepareMintSparkTransaction and mintSparkCoins do not escape the Qt slot. Catch
the wallet-operation exception, report it through the existing showError dialog
mechanism, and return while preserving normal status-based error handling and
wallet relocking during unwinding.
---
Nitpick comments:
In `@src/qt/guiutil.cpp`:
- Around line 110-123: Remove the explicit worker.join() after waitLoop.exec()
and let Cleanup’s destructor own the single join. Add a side-effect-free
GUI-thread precondition assertion at the start of the helper, before
QApplication cursor operations or nested event-loop execution, using the
project’s established assertion mechanism.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cebb4d6-49ec-4f2c-a371-c9ecae717752
📒 Files selected for processing (12)
src/qt/automintdialog.cppsrc/qt/automintmodel.cppsrc/qt/automintnotification.cppsrc/qt/forms/automintdialog.uisrc/qt/forms/automintnotification.uisrc/qt/forms/optionsdialog.uisrc/qt/forms/overviewpage.uisrc/qt/guiutil.cppsrc/qt/guiutil.hsrc/qt/optionsmodel.hsrc/qt/overviewpage.cppsrc/qt/sendcoinsdialog.cpp
| <string>When enabled, the wallet can prompt to make transparent funds private with Spark.</string> | ||
| </property> | ||
| <property name="text"> | ||
| <string>Enable &auto-anonymize features</string> | ||
| <string>Enable automatic Spark &privacy</string> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use unique keyboard mnemonics for the Spark privacy options.
Both labels define the same Alt+P mnemonic through &privacy. Keyboard users cannot reliably select one option. Assign a unique mnemonic to one label.
Proposed fix
- <string>Enable automatic Spark &privacy</string>
+ <string>Enable &automatic Spark privacy</string>Also applies to: 208-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/qt/forms/optionsdialog.ui` around lines 188 - 191, Update the Spark
privacy option labels in the options dialog, including the label near “Enable
automatic Spark privacy” and the other Spark privacy label, so they use
different keyboard mnemonics instead of both defining Alt+P. Preserve the
existing text and assign one label a unique mnemonic.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| confirmation.exec(); | ||
| if (confirmation.clickedButton() != confirmButton) { | ||
| return; |
There was a problem hiding this comment.
Suggestion: The transaction is prepared and its inputs and fee are calculated before this modal confirmation, but the confirmation releases the wallet locks while the user decides. Another wallet operation can spend or reserve the same transparent funds during that interval, leaving the prepared transaction stale and causing commit failure or a result that no longer matches the reviewed amount. Revalidate or reprepare the transaction immediately before committing. [race condition]
Severity Level: Major ⚠️
- ⚠️ Reviewed Spark transfers can fail after wallet changes.
- ⚠️ Confirmation amounts may no longer match committed transactions.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/qt/overviewpage.cpp
**Line:** 395:397
**Comment:**
*Race Condition: The transaction is prepared and its inputs and fee are calculated before this modal confirmation, but the confirmation releases the wallet locks while the user decides. Another wallet operation can spend or reserve the same transparent funds during that interval, leaving the prepared transaction stale and causing commit failure or a result that no longer matches the reviewed amount. Revalidate or reprepare the transaction immediately before committing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished running the review. |
CreateSparkMintTransactions can split a Make Private request into several transactions (the Split option defaults to on and groups inputs per transparent address), and mintSparkCoins commits them sequentially, returning on the first failure after earlier ones have already been broadcast. The commit failure dialog asserted total failure, misinforming the user about funds already in flight. Say "No funds were moved" only for single-transaction transfers; for split transfers, point the user at the Transactions tab. Keep the wallet's rejection reason under Details. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TV7dpUCcTQrSSxnXaR7Xuq
Two dead ends remained in the amount dialog. Entering the full balance (or clicking Max) silently switched to fee-subtraction mode, so the user only discovered at review that less than the typed amount would be made private. And typing an amount just under the balance failed preparation with only "change the amount and try again", leaving the user to guess how much smaller to go. Show a note under the amount field when the network fee will be deducted from the entered amount, and offer a Use Maximum button on amount/fee-exceeds-balance failures that refills the field with the highest possible amount and reopens the dialog, so the change stays explicit and reviewable before anything is sent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TV7dpUCcTQrSSxnXaR7Xuq
Column 2 of the overview balance grid holds the watch-only amounts, so placing the Make Private button there made it sit under the watch-only totals whenever watch-only display was on, implying it acts on funds it cannot spend. Move it to column 3 (the spacer column) left-aligned: with watch-only hidden the empty column collapses and the button still sits directly beside the eligible balance, and with watch-only shown it stays clear of that column. Also hide the button, rather than merely disabling it, when the wallet has nothing eligible to make private, so watch-only wallets do not show a control that can never act. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TV7dpUCcTQrSSxnXaR7Xuq
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 910e71577a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (useMaxButton && error.clickedButton() == useMaxButton) { | ||
| amountField->setValue(available); |
There was a problem hiding this comment.
If the wallet's transparent balance decreases while this modal is open—for example because an RPC transaction spends funds—the balanceChanged signal can update currentAnonymizableBalance, but available remains the snapshot captured when the dialog opened. Selecting “Use Maximum” therefore restores that stale amount, so preparation fails again with AmountExceedsBalance and the advertised recovery action loops until the user guesses a lower value or reopens the dialog. Refresh the available amount and its label before retrying.
AGENTS.md reference: AGENTS.md:L244-L245
Useful? React with 👍 / 👎.
|
|
||
| auto layout = new QVBoxLayout(&amountDialog); | ||
| auto description = new QLabel( | ||
| tr("Move FIRO from your transparent balance into Spark, Firo's private balance."), |
There was a problem hiding this comment.
This commit replaces and adds numerous user-facing tr() and .ui strings, including this dialog text, but leaves src/qt/locale/bitcoin_en.ts unchanged and none of the new strings occur there. The repository's translation procedure states at doc/translation_process.md:17 that this catalog is the source for all other translations and must be updated whenever source strings change, so these messages cannot enter the translation workflow and localized builds fall back to English. Regenerate the catalog with the translate target.
Useful? React with 👍 / 👎.
2 participants
PR intention
Make the Spark privacy action understandable without requiring users to know what Spark is, let them choose how much of their transparent balance to make private, and recover safely when the wallet cannot create the requested transaction.
Code changes brief
Anonymize Allaction withMake Private...beside the eligible transparent balance.Maxoption.Maxand an exact fee review before confirmation.Change AmountorCancel. No automatic splitting, silent amount changes, or fund movement is added. Encrypted wallets relock before amount editing.Details, while keeping the main message concise.Testing
git diff --check.uifiles as XML