Add Create Unsigned button to SendConfirmationDialog #441

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.

    if (!settings.contains("fEnablePSBTControls"))
        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.

    if (!settings.contains("strThirdPartyTxUrls"))
        settings.setValue("strThirdPartyTxUrls", "");
    strThirdPartyTxUrls = settings.value("strThirdPartyTxUrls", "").toString();

    if (!settings.contains("fCoinControlFeatures"))
        settings.setValue("fCoinControlFeatures", false);
    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.

     enum OptionID {
-        StartAtStartup,         // bool
-        ShowTrayIcon,           // bool
-        MinimizeToTray,         // bool
-        MapPortUPnP,            // bool
-        MapPortNatpmp,          // bool
-        MinimizeOnClose,        // bool
-        ProxyUse,               // bool
-        ProxyIP,                // QString
-        ProxyPort,              // int
-        ProxyUseTor,            // bool
-        ProxyIPTor,             // QString
-        ProxyPortTor,           // int
-        DisplayUnit,            // BitcoinUnits::Unit
-        ThirdPartyTxUrls,       // QString
-        Language,               // QString
+        StartAtStartup,            // bool
+        ShowTrayIcon,              // bool
+        MinimizeToTray,            // bool
+        MapPortUPnP,               // bool
+        MapPortNatpmp,             // bool
+        MinimizeOnClose,           // bool
+        ProxyUse,                  // bool
+        ProxyIP,                   // QString
+        ProxyPort,                 // int
+        ProxyUseTor,               // bool
+        ProxyIPTor,                // QString
+        ProxyPortTor,              // int
+        DisplayUnit,               // BitcoinUnits::Unit
+        ThirdPartyTxUrls,          // QString
+        Language,                  // QString
         UseEmbeddedMonospacedFont, // bool
-        CoinControlFeatures,    // bool
-        SubFeeFromAmount,       // bool
-        ThreadsScriptVerif,     // int
-        Prune,                  // bool
-        PruneSize,              // int
-        DatabaseCache,          // int
-        ExternalSignerPath,     // QString
-        SpendZeroConfChange,    // bool
-        Listen,                 // bool
-        Server,                 // bool
-        EnablePSBTControls,     // bool
+        CoinControlFeatures,       // bool
+        SubFeeFromAmount,          // bool
+        ThreadsScriptVerif,        // int
+        Prune,                     // bool
+        PruneSize,                 // int
+        DatabaseCache,             // int
+        ExternalSignerPath,        // QString
+        SpendZeroConfChange,       // bool
+        Listen,                    // bool
+        Server,                    // bool
+        EnablePSBTControls,        // bool
         OptionIDRowCount,
     };

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:

    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:

    const QString confirmButtonText{tr("Send")};
    const bool m_enable_send;
    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