Add Create Unsigned button to SendConfirmationDialog

achow101 commented at 7:43 pm on September 30, 2021: member

Instead of having different buttons or changing button behavior for making a PSBT, just have SendConfirmationDialog return whether the user wants a PSBT or a broadcasted transaction. Since this dialog is used by both the bumpFeeAction and the SendCoinsDialog, changes to both to support the different behavior is needed. They will check the return value of the SendConfirmationDialog for whether a PSBT needs to be created instead of checking whether private keys are disabled.

Strings used in this dialog are being slightly modified to work with both private keys enabled and disabled wallets.

Moved from https://github.com/bitcoin/bitcoin/pull/18789

jarolrod added the label UX on Sep 30, 2021

in src/qt/forms/optionsdialog.ui:256 in 7aa0c618a9 outdated

251@@ -252,6 +252,16 @@
252             </property>
253            </widget>
254           </item>
255+          <item>
256+           <widget class="QCheckBox" name="m_enable_psbt_controls">

shaavan commented at 5:56 pm on October 1, 2021:

I think the name “m_enable_psbt_controls” differs in naming convention compared to other QWidgets. I think this should be renamed to follow the naming convention better.

achow101 commented at 7:24 pm on October 1, 2021:

This follows the new naming convention that the style guide specifies. There is generally an effort to migrate to a new style, so new code may not match the style of surrounding older code.

shaavan commented at 8:28 pm on October 1, 2021:

I was not aware of the change in writing style. That’s something new I learned today!

in src/qt/optionsmodel.cpp:88 in 7aa0c618a9 outdated

82@@ -83,6 +83,11 @@ void OptionsModel::Init(bool resetSettings)
83         settings.setValue("fCoinControlFeatures", false);
84     fCoinControlFeatures = settings.value("fCoinControlFeatures", false).toBool();
85 
86+    if (!settings.contains("fEnablePSBTControls")) {
87+        settings.setValue("fEnablePSBTControls", false);
88+    }

shaavan commented at 5:57 pm on October 1, 2021:

In one-liner if-else, the brackets are redundant and should be removed.

0    if (!settings.contains("fEnablePSBTControls"))
1        settings.setValue("fEnablePSBTControls", false);

achow101 commented at 7:23 pm on October 1, 2021:

While that may be technically correct, code written that way can be more confusing. Our style guide states that brackets must be included even for code like this.

shaavan commented at 8:33 pm on October 1, 2021:

I might be wrong, and it might be a new change in writing style. But the code above it doesn’t use brackets for one-liner if-else.

0    if (!settings.contains("strThirdPartyTxUrls"))
1        settings.setValue("strThirdPartyTxUrls", "");
2    strThirdPartyTxUrls = settings.value("strThirdPartyTxUrls", "").toString();
3
4    if (!settings.contains("fCoinControlFeatures"))
5        settings.setValue("fCoinControlFeatures", false);
6    fCoinControlFeatures = settings.value("fCoinControlFeatures", false).toBool();

achow101 commented at 8:36 pm on October 1, 2021:

We are trying to change code style gradually. Existing code may not adhere to the style guide, but modifications to it and newly added code must.

Also, personally, I hate the bracket-less style so I’m not going to do this even if the style guide allows it. There is no requirement that such code needs to be written this way.

shaavan commented at 8:48 pm on October 1, 2021:

We are trying to change code style gradually. Existing code may not adhere to the style guide, but modifications to it and newly added code must.

Understood.

Also, personally, I hate the bracket-less style so I’m not going to do this even if the style guide allows it. There is no requirement that such code needs to be written this way.

I get it. Bracket-less style have a chance of inducing unnecessary errors in case of adding some line inside the conditional statement.

in src/qt/sendcoinsdialog.cpp:329 in 7aa0c618a9 outdated

331-    }
332-
333     question_string.append("<br /><span style='font-size:10pt;'>");
334+    /*: Message displayed when attempting to create a transaction. Cautionary text to prompt the user to verify
335+        that the displayed transaction details represent the transaction the user intends to create. */
336+    question_string.append(tr("Do you want to create this transaction?"));

shaavan commented at 6:04 pm on October 1, 2021:

Though the line is correctly displayed in the GUI, there is a lack of space after it, which seems odd.

Screenshot (PR): Note: the slight white highlighting is done to emphasize and was not present while testing. Correction

Either a space should be added after the statement so that the spacing is corrected. Or it should be displayed as a header text as is done on the master branch:

Screenshot (Master): Master

achow101 commented at 7:37 pm on October 1, 2021:

Fixed

in src/qt/optionsmodel.h:73 in 7aa0c618a9 outdated

69@@ -70,6 +70,7 @@ class OptionsModel : public QAbstractListModel
70         SpendZeroConfChange,    // bool
71         Listen,                 // bool
72         Server,                 // bool
73+        EnablePSBTControls,     // bool

shaavan commented at 6:07 pm on October 1, 2021:

And Lastly. The Clang format check suggested the following corrections.

 0     enum OptionID {
 1-        StartAtStartup,         // bool
 2-        ShowTrayIcon,           // bool
 3-        MinimizeToTray,         // bool
 4-        MapPortUPnP,            // bool
 5-        MapPortNatpmp,          // bool
 6-        MinimizeOnClose,        // bool
 7-        ProxyUse,               // bool
 8-        ProxyIP,                // QString
 9-        ProxyPort,              // int
10-        ProxyUseTor,            // bool
11-        ProxyIPTor,             // QString
12-        ProxyPortTor,           // int
13-        DisplayUnit,            // BitcoinUnits::Unit
14-        ThirdPartyTxUrls,       // QString
15-        Language,               // QString
16+        StartAtStartup,            // bool
17+        ShowTrayIcon,              // bool
18+        MinimizeToTray,            // bool
19+        MapPortUPnP,               // bool
20+        MapPortNatpmp,             // bool
21+        MinimizeOnClose,           // bool
22+        ProxyUse,                  // bool
23+        ProxyIP,                   // QString
24+        ProxyPort,                 // int
25+        ProxyUseTor,               // bool
26+        ProxyIPTor,                // QString
27+        ProxyPortTor,              // int
28+        DisplayUnit,               // BitcoinUnits::Unit
29+        ThirdPartyTxUrls,          // QString
30+        Language,                  // QString
31         UseEmbeddedMonospacedFont, // bool
32-        CoinControlFeatures,    // bool
33-        SubFeeFromAmount,       // bool
34-        ThreadsScriptVerif,     // int
35-        Prune,                  // bool
36-        PruneSize,              // int
37-        DatabaseCache,          // int
38-        ExternalSignerPath,     // QString
39-        SpendZeroConfChange,    // bool
40-        Listen,                 // bool
41-        Server,                 // bool
42-        EnablePSBTControls,     // bool
43+        CoinControlFeatures,       // bool
44+        SubFeeFromAmount,          // bool
45+        ThreadsScriptVerif,        // int
46+        Prune,                     // bool
47+        PruneSize,                 // int
48+        DatabaseCache,             // int
49+        ExternalSignerPath,        // QString
50+        SpendZeroConfChange,       // bool
51+        Listen,                    // bool
52+        Server,                    // bool
53+        EnablePSBTControls,        // bool
54         OptionIDRowCount,
55     };

achow101 commented at 7:26 pm on October 1, 2021:

This are unrelated changes so I will not include them.

shaavan commented at 8:34 pm on October 1, 2021:

Makes Sense 👍

shaavan changes_requested

shaavan commented at 6:08 pm on October 1, 2021: contributor

Concept ACK

I agree with the idea behind this PR. Though I have not completed my test, I have found some corrections here and there in the code that will be great for starters. I shall post my thorough review after completing the testings later.

achow101 force-pushed on Oct 1, 2021

in src/qt/optionsmodel.h:121 in 790733e54c outdated

117@@ -116,6 +118,7 @@ class OptionsModel : public QAbstractListModel
118     bool m_use_embedded_monospaced_font;
119     bool fCoinControlFeatures;
120     bool m_sub_fee_from_amount;
121+    bool fEnablePSBTControls;

laanwj commented at 1:20 pm on October 7, 2021:

Any specific reason for using the antiquated fEnablePSBTControls convention here, or was this an oversight? (I see you tried to use m_ elsewhere)

achow101 commented at 7:23 pm on October 7, 2021:

Changed to m_enable_psbt_controls.

in src/qt/sendcoinsdialog.cpp:399 in fbe887f80f outdated

393@@ -397,21 +394,22 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
394     if (!PrepareSendText(question_string, informative_text, detailed_text)) return;
395     assert(m_current_transaction);
396 
397-    const QString confirmation = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Confirm transaction proposal") : tr("Confirm send coins");
398-    const QString confirmButtonText = model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner() ? tr("Create Unsigned") : tr("Sign and send");
399-    auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, this);
400+    const QString confirmation = tr("Confirm send coins");
401+    const QString confirmButtonText = tr("Send");
402+    const QString psbt_button_text = tr("Create Unsigned");

ryanofsky commented at 3:49 pm on October 7, 2021:

In commit “qt: Add Create Unsigned button to SendConfirmationDialog” (fbe887f80f97a9a948a3760da366ab274b887b35)

It would be nice to drop the confirmText the psbt_text arguments to the SendConfirmationDialog constructor so

SendConfirmationDialog has fewer constructor parameters and easier to create correctly
Strings are not repeated multiple places in the code and less likely to get out of sync in the future

achow101 commented at 7:24 pm on October 7, 2021:

Done

in src/qt/forms/optionsdialog.ui:258 in 790733e54c outdated

251@@ -252,6 +252,16 @@
252             </property>
253            </widget>
254           </item>
255+          <item>
256+           <widget class="QCheckBox" name="m_enable_psbt_controls">
257+            <property name="text">
258+             <string extracomment="An options window setting to enable advanced PSBT controls.">Enable advanced &amp;PSBT controls</string>

ryanofsky commented at 4:15 pm on October 7, 2021:

In commit “qt: hide Create Unsigned button behind an expert mode option” (790733e54c3daff547acbc90fb179b48b6d2fb3c)

This is just a personal opinion, but I don’t like the “advanced” weasel wording that makes the option more mysterious, implies that if you are an “advanced” user you should already know what this does without any explanation, and implies that if you don’t have some outside knowledge what this does, then maybe you are some kind of simpleton.

Easy to fix this by replacing: “Enable advanced PBST controls” with “Enable PSBT controls”. Or just literally “Enable create unsigned transaction feature when spending”.

achow101 commented at 7:24 pm on October 7, 2021:

Removed “advanced”

in src/qt/optionsmodel.cpp:380 in 790733e54c outdated

376@@ -372,6 +377,8 @@ QVariant OptionsModel::data(const QModelIndex & index, int role) const
377             return settings.value("fListen");
378         case Server:
379             return settings.value("server");
380+        case EnablePSBTControls:

ryanofsky commented at 4:22 pm on October 7, 2021:

In commit “qt: hide Create Unsigned button behind an expert mode option” (790733e54c3daff547acbc90fb179b48b6d2fb3c)

Suggest moving this case up near case CoinControlFeatures above, so this is grouped together with GUI options, not node options and it is easier ensure that all these options are handled consistently in the future. Same suggestion as well for OptionsModel::setData

achow101 commented at 7:24 pm on October 7, 2021:

Done

in src/qt/sendcoinsdialog.cpp:332 in 790733e54c outdated

328@@ -329,8 +329,17 @@ bool SendCoinsDialog::PrepareSendText(QString& question_string, QString& informa
329     question_string.append(tr("Do you want to create this transaction?"));
330     question_string.append("<br /><span style='font-size:10pt;'>");
331     if (model->wallet().privateKeysDisabled() && !model->wallet().hasExternalSigner()) {
332+        /*: Text to inform a user attempting to create a transaction of their current options. At this stage,

ryanofsky commented at 4:29 pm on October 7, 2021:

In commit “qt: hide Create Unsigned button behind an expert mode option” (790733e54c3daff547acbc90fb179b48b6d2fb3c)

Minor: This changes seem like they should be part of the previous commit. The previous commit is adding the “create unsigned” button unconditionally, and the new commit is adding the option to hide the button. It seems like the instructions for using the button should be added in the same commit that adds the button.

This is nitpicking though, so ignore unless there’s some need to change.

achow101 commented at 7:24 pm on October 7, 2021:

Done

in src/qt/sendcoinsdialog.cpp:411 in 790733e54c outdated

405@@ -397,7 +406,11 @@ void SendCoinsDialog::sendButtonClicked([[maybe_unused]] bool checked)
406     const QString confirmation = tr("Confirm send coins");
407     const QString confirmButtonText = tr("Send");
408     const QString psbt_button_text = tr("Create Unsigned");
409-    auto confirmationDialog = new SendConfirmationDialog(confirmation, question_string, informative_text, detailed_text, SEND_CONFIRM_DELAY, confirmButtonText, !model->wallet().privateKeysDisabled(), psbt_button_text, this);
410+
411+    // Show the Create Unsigned button if set in the options or if private keys are disabled.
412+    bool show_unsigned_button = model->getOptionsModel()->getEnablePSBTControls() || model->wallet().privateKeysDisabled();

ryanofsky commented at 4:46 pm on October 7, 2021:

In commit “qt: hide Create Unsigned button behind an expert mode option” (790733e54c3daff547acbc90fb179b48b6d2fb3c)

It seems clumsy for this show unsigned logic to have to be repeated two places (in spending and fee bumping) and for all these extra calls to privateKeysDisabled to be needed in the code. It would be clearer to replace the show_unsigned constructor argument with an always_show_unsigned constructor argument and pass getEnablePSBTControls directly instead of getEnablePSBTControls || privateKeysDisabled, since SendConfirmationDialog already knows the privateKeysDisabled value

Also it would make the PR clearer if instead of changing the SendConfirmationDialog gradually in two commits, you just implemented it fully in the first commit passing true for always_show_unsigned, and then simply replaced true with getEnablePSBTControls in the second commit.

achow101 commented at 7:24 pm on October 7, 2021:

Done

ryanofsky approved

ryanofsky commented at 4:56 pm on October 7, 2021: contributor

Code review ACK 790733e54c3daff547acbc90fb179b48b6d2fb3c. I guess I’m grumpy and left some complaints about the code and use of the word “advanced” in option description. But overall this change looks very good and makes the UI more consistent and understandable while adding a new feature. Please ignore any feedback below that is not helpful!

Would recommend adding release notes to this PR, and maybe including the nice screenshots from shaavan in the PR description.

qt: Add Create Unsigned button to SendConfirmationDialog

Instead of having different buttons or changing button behavior for
making a PSBT, just have SendConfirmationDialog return whether the user
wants a PSBT or a broadcasted transaction. Since this dialog is used
by both the bumpFeeAction and the SendCoinsDialog, changes to both
to support the different behavior is needed. They will check
the return value of the SendConfirmationDialog for whether a PSBT
needs to be created instead of checking whether private keys are
disabled.

Strings used in this dialog are being slightly modified to work with
both private keys enabled and disabled wallets.

5c3b800acd

achow101 force-pushed on Oct 7, 2021

in src/qt/optionsmodel.cpp:89 in 9d8cf228a3 outdated

82@@ -83,6 +83,11 @@ void OptionsModel::Init(bool resetSettings)
83         settings.setValue("fCoinControlFeatures", false);
84     fCoinControlFeatures = settings.value("fCoinControlFeatures", false).toBool();
85 
86+    if (!settings.contains("m_enable_psbt_controls")) {
87+        settings.setValue("m_enable_psbt_controls", false);
88+    }
89+    m_enable_psbt_controls = settings.value("m_enable_msbt_controls", false).toBool();

ryanofsky commented at 4:40 pm on October 8, 2021:

In commit “qt: hide Create Unsigned button behind an expert mode option” (9d8cf228a339101cc4956b32b39e377f5b5c21be)

Bug: spelling “msbt” should be “psbt”.

Separate, minor suggestion: I’d also suggest dropping the “m_” prefix in the setting name (not member name). Other setting names don’t have “m_” prefixes. Settings names are persisted externally and survive across releases, while member names are changeable implementation details. Also (this part is just my opinion), having redundant OptionsModel member variables like m_enable_psbt_controls and fCoinControlFeatures even exist is a mistake and a footgun to begin with. OptionsModel only needs member variables for specific settings with complicated external interactions, like the pruning setting, not for plain settings that can be get and set with data() and setData() calls. I’m not suggesting getting rid of the members in this PR, just saying it would be strange later if the members were simplified away and some settings still had m_ prefixes.

achow101 commented at 4:56 pm on October 8, 2021:

Fixed. Dropped the m_ from the setting name.

ryanofsky commented at 4:50 pm on October 8, 2021: contributor

There is a small bug second commit (msbt_controls spelling) but than that 9d8cf228a339101cc4956b32b39e377f5b5c21be looks very good, and thanks for the updates!

qt: hide Create Unsigned button behind an expert mode option 742918c8ef

achow101 force-pushed on Oct 8, 2021

ryanofsky approved

ryanofsky commented at 5:16 pm on October 8, 2021: contributor

Code review ACK 742918c8ef353993a07c060f476a160e8272a9ef. Just suggested changes since last review. Looks great!

hebasto commented at 7:44 pm on October 8, 2021: member

Concept ACK.

hebasto requested review from Sjors on Oct 8, 2021

in src/qt/sendcoinsdialog.h:126 in 742918c8ef

123-    void updateYesButton();
124+    void updateButtons();
125 
126 private:
127     QAbstractButton *yesButton;
128+    QAbstractButton *m_psbt_button;

hebasto commented at 9:39 am on October 9, 2021:

style nit:

0    QAbstractButton* m_psbt_button;

in src/qt/sendcoinsdialog.h:131 in 742918c8ef

129     QTimer countDownTimer;
130     int secDelay;
131-    QString confirmButtonText;
132+    QString confirmButtonText{tr("Send")};
133+    bool m_enable_send;
134+    QString m_psbt_button_text{tr("Create Unsigned")};

hebasto commented at 9:40 am on October 9, 2021:

nit:

0    const QString confirmButtonText{tr("Send")};
1    const bool m_enable_send;
2    const QString m_psbt_button_text{tr("Create Unsigned")};

in src/qt/walletmodel.cpp:525 in 742918c8ef

521@@ -523,7 +522,7 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
522     }
523 
524     // Short-circuit if we are returning a bumped transaction PSBT to clipboard
525-    if (create_psbt) {
526+    if (retval == QMessageBox::Save) {

hebasto commented at 9:53 am on October 9, 2021:

As m_psbt_button = button(QMessageBox::Save); is not near, maybe add a comment which describes that this code actually handles “Create Unsigned” button press?

Is // Short-circuit if we are returning a bumped transaction PSBT to clipboard comment above still relevant?

hebasto commented at 9:53 am on October 9, 2021: member

ACK 742918c8ef353993a07c060f476a160e8272a9ef, tested on Linux Mint 20.2 (Qt 5.12.8).

The following code looks dead now: https://github.com/bitcoin-core/gui/blob/742918c8ef353993a07c060f476a160e8272a9ef/src/qt/sendcoinsdialog.cpp#L1052-L1054

Maybe break very long lines for better readability?

jarolrod commented at 5:36 pm on October 28, 2021: member

ACK 742918c

This is looking good and works well. I would second @hebasto nits if you feel like retouching. 🥃

DrahtBot commented at 8:32 am on January 7, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#517 (refactor, qt: Use std::chrono for parameters of QTimer methods by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto merged this on Jan 9, 2022

hebasto closed this on Jan 9, 2022

sidhujag referenced this in commit 8d4951d824 on Jan 9, 2022

hebasto referenced this in commit aece566249 on Mar 17, 2022

bitcoin-core locked this on Jan 9, 2023

Add Create Unsigned button to SendConfirmationDialog #441

Conflicts