Fix go private window behavior for exchange address #1426
Conversation
WalkthroughThe changes involve refining recipient handling in the Changes
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}
src/qt/sendcoinsdialog.cpp
Outdated
| bool openPageTag = true; | ||
| for(int i = 0; i < recipients.size(); ++i){ | ||
| std::string address = recipients[i].address.toStdString(); | ||
| CBitcoinAddress add(address); | ||
| CTxDestination dest = add.Get(); | ||
|
|
||
| if (boost::get<CExchangeKeyID>(&dest)) { | ||
| openPageTag = false; | ||
| } | ||
| } | ||
|
|
||
| if (openPageTag) { | ||
| SendGoPrivateDialog goPrivateDialog; | ||
| bool clickedButton = goPrivateDialog.getClickedButton(); | ||
| if (!clickedButton) { | ||
| setAnonymizeMode(true); | ||
| fNewRecipientAllowed = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
The logic to check for CExchangeKeyID type addresses and conditionally display the SendGoPrivateDialog is a crucial addition for enhancing the application's handling of private transactions. However, there are a few considerations and potential improvements:
-
Error Handling and User Feedback: When the
openPageTagis set tofalsedue to the presence of aCExchangeKeyIDtype address, the function returns without any user feedback. It might be beneficial to inform the user why the operation cannot proceed in this case. -
Code Duplication: The pattern of iterating over recipients to perform checks appears multiple times in this function. Consider refactoring this pattern into a separate method to improve code maintainability and reduce duplication.
-
Performance Consideration: The current implementation iterates over all recipients for each condition check. If the list of recipients is large, this could impact performance. Consider optimizing this by breaking early if possible or consolidating checks.
-
User Experience: The decision to block the transaction entirely if any recipient address is of the
CExchangeKeyIDtype might be too restrictive. It could be worth exploring alternatives that allow the user to proceed with the transaction for non-exchange addresses or provide a clear path to rectify the issue.
- setAnonymizeMode(true);
- fNewRecipientAllowed = true;
+ // Consider providing user feedback here to explain why the transaction cannot proceed.
+ QMessageBox::warning(this, tr("Incompatible Address"), tr("One or more recipient addresses are exchange addresses, which are not compatible with private transactions."));Consider refactoring the recipient address type checks into a separate method to improve code readability and maintainability.
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.
| bool openPageTag = true; | |
| for(int i = 0; i < recipients.size(); ++i){ | |
| std::string address = recipients[i].address.toStdString(); | |
| CBitcoinAddress add(address); | |
| CTxDestination dest = add.Get(); | |
| if (boost::get<CExchangeKeyID>(&dest)) { | |
| openPageTag = false; | |
| } | |
| } | |
| if (openPageTag) { | |
| SendGoPrivateDialog goPrivateDialog; | |
| bool clickedButton = goPrivateDialog.getClickedButton(); | |
| if (!clickedButton) { | |
| setAnonymizeMode(true); | |
| fNewRecipientAllowed = true; | |
| return; | |
| } | |
| bool openPageTag = true; | |
| for(int i = 0; i < recipients.size(); ++i){ | |
| std::string address = recipients[i].address.toStdString(); | |
| CBitcoinAddress add(address); | |
| CTxDestination dest = add.Get(); | |
| if (boost::get<CExchangeKeyID>(&dest)) { | |
| openPageTag = false; | |
| } | |
| } | |
| if (openPageTag) { | |
| SendGoPrivateDialog goPrivateDialog; | |
| bool clickedButton = goPrivateDialog.getClickedButton(); | |
| if (!clickedButton) { | |
| // Consider providing user feedback here to explain why the transaction cannot proceed. | |
| QMessageBox::warning(this, tr("Incompatible Address"), tr("One or more recipient addresses are exchange addresses, which are not compatible with private transactions.")); | |
| return; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- src/qt/forms/optionsdialog.ui (1 hunks)
- src/qt/optionsdialog.cpp (2 hunks)
- src/qt/optionsmodel.cpp (3 hunks)
- src/qt/optionsmodel.h (4 hunks)
- src/qt/sendcoinsdialog.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/qt/sendcoinsdialog.cpp
Additional comments not posted (7)
src/qt/optionsmodel.h (2)
49-49: The addition of theDisableGoToPrivatemember variable is correctly implemented and follows the naming conventions and structure of the existing code.
112-112: The addition of thedisableGoToPrivateChangedsignal is correctly implemented and follows Qt's best practices for notifying changes to properties.src/qt/optionsdialog.cpp (2)
168-168: The connection of thedisableGoToPrivateWindowcheckbox to theshowRestartWarningmethod is correctly implemented and follows the established pattern for settings that require a restart.
188-188: The mapping of thedisableGoToPrivateWindowcheckbox to theOptionsModel::DisableGoToPrivateis correctly implemented, ensuring the setting is properly saved and loaded.src/qt/optionsmodel.cpp (3)
84-87: The handling of theDisableGoToPrivatesetting in theInitmethod is correctly implemented, following the established pattern for initializing settings with default values.
433-437: The handling of theDisableGoToPrivatesetting in thesetDatamethod is correctly implemented, ensuring the setting is properly saved when changed.
436-436: The emission of thedisableGoToPrivateChangedsignal upon changing theDisableGoToPrivatesetting is correctly implemented, following best practices for Qt models.
src/qt/forms/optionsdialog.ui
Outdated
| <item> | ||
| <widget class="QCheckBox" name="disableGoToPrivateWindow"> | ||
| <property name="toolTip"> | ||
| <string>Disable go to private window.</string> | ||
| </property> | ||
| <property name="text"> | ||
| <string>&Disable go to private window</string> | ||
| </property> | ||
| </widget> |
There was a problem hiding this comment.
Consider enhancing the tooltip text for the disableGoToPrivateWindow checkbox for better clarity. For example, explain what the "go to private window" functionality is and why a user might want to disable it. This will improve user understanding and experience.
- <string>Disable go to private window.</string>
+ <string>Disable the 'Go Private' window feature, which is used for... [provide a brief explanation]. This option can be useful if... [provide a reason or scenario].</string>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.
| <item> | |
| <widget class="QCheckBox" name="disableGoToPrivateWindow"> | |
| <property name="toolTip"> | |
| <string>Disable go to private window.</string> | |
| </property> | |
| <property name="text"> | |
| <string>&Disable go to private window</string> | |
| </property> | |
| </widget> | |
| <item> | |
| <widget class="QCheckBox" name="disableGoToPrivateWindow"> | |
| <property name="toolTip"> | |
| <string>Disable the 'Go Private' window feature, which is used for... [provide a brief explanation]. This option can be useful if... [provide a reason or scenario].</string> | |
| </property> | |
| <property name="text"> | |
| <string>&Disable go to private window</string> | |
| </property> | |
| </widget> |
src/qt/optionsmodel.h
Outdated
| @@ -73,6 +74,7 @@ class OptionsModel : public QAbstractListModel | |||
| bool getRapAddresses() { return fenableRapAddresses; } | |||
| const QString& getOverriddenByCommandLine() { return strOverriddenByCommandLine; } | |||
| bool getAutoAnonymize() { return fAutoAnonymize; } | |||
| bool getGoPrivateWindow() {return fDisableGoToPrivate;} | |||
There was a problem hiding this comment.
Rename the getter method to accurately reflect the variable it accesses for clarity. Consider renaming it to getDisableGoToPrivate to match the variable name.
- bool getGoPrivateWindow() {return fDisableGoToPrivate;}
+ bool getDisableGoToPrivate() {return fDisableGoToPrivate;}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.
| bool getGoPrivateWindow() {return fDisableGoToPrivate;} | |
| bool getDisableGoToPrivate() {return fDisableGoToPrivate;} |
reubenyap
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/qt/optionsmodel.h (4 hunks)
- src/qt/sendcoinsdialog.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/qt/optionsmodel.h
- src/qt/sendcoinsdialog.cpp
|
We don't need this anymore, as we removed the popup window in #1434 . |
Summary by CodeRabbit
New Features
Bug Fixes