Add Create Unsigned button to SendConfirmationDialog #441

achow101 · 2021-09-30T19:43:54Z

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 bitcoin/bitcoin#18789

shaavan

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.

src/qt/forms/optionsdialog.ui

src/qt/optionsmodel.cpp

src/qt/sendcoinsdialog.cpp

src/qt/optionsmodel.h

ryanofsky

Code review ACK 790733e. 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.

src/qt/sendcoinsdialog.cpp

src/qt/forms/optionsdialog.ui

src/qt/optionsmodel.cpp

src/qt/sendcoinsdialog.cpp

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.

ryanofsky

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

src/qt/optionsmodel.cpp

ryanofsky

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

hebasto · 2021-10-08T19:44:33Z

Concept ACK.

hebasto

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

The following code looks dead now:

gui/src/qt/sendcoinsdialog.cpp

Lines 1052 to 1054 in 742918c

    
           if (confirmButtonText.isEmpty()) { 
        
               confirmButtonText = yesButton->text(); 
        
           }

Maybe break very long lines for better readability?

hebasto · 2021-10-09T09:39:38Z

src/qt/sendcoinsdialog.h


 private:
    QAbstractButton *yesButton;
+    QAbstractButton *m_psbt_button;


style nit:

Suggested change

QAbstractButton *m_psbt_button;

QAbstractButton* m_psbt_button;

hebasto · 2021-10-09T09:40:04Z

src/qt/sendcoinsdialog.h

+    QString confirmButtonText{tr("Send")};
+    bool m_enable_send;
+    QString m_psbt_button_text{tr("Create Unsigned")};


nit:

Suggested change

QString confirmButtonText{tr("Send")};

bool m_enable_send;

QString m_psbt_button_text{tr("Create Unsigned")};

const QString confirmButtonText{tr("Send")};

const bool m_enable_send;

const QString m_psbt_button_text{tr("Create Unsigned")};

hebasto · 2021-10-09T09:53:20Z

src/qt/walletmodel.cpp

@@ -523,7 +522,7 @@ bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
    }

    // Short-circuit if we are returning a bumped transaction PSBT to clipboard
-    if (create_psbt) {
+    if (retval == QMessageBox::Save) {


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?

jarolrod · 2021-10-28T17:36:14Z

ACK 742918c

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

DrahtBot · 2022-01-07T08:32:08Z

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.

2efdfb8 gui: restore Send for external signer (Sjors Provoost) 4b5a6cd refactor: helper function signWithExternalSigner() (Sjors Provoost) 026b5b4 move-only: helper function to present PSBT (Sjors Provoost) Pull request description: Fixes #551 For the simplest use case of a wallet with one external signer and "PSBT Controls" disabled in settings (the default), the send dialog will behave the same as when using a wallet with private keys. I.e. there's no "Create Unsigned" button. When PSBT controls are turned on, you can now actually make a PSBT with signing it; before this PR that button would trigger a sign event and would broadcast the transaction. In case a multisig, the Send button will sign on the device, and then fall back to presenting a PSBT (same behavior as before #441). This PR starts with two refactoring commits to move some stuff into a helper function for improved readability in general, and to make the main commit easier to review. ACKs for top commit: jonatack: utACK 2efdfb8 diff review since my last review, code re-review, rebased to current master, verified clean debug build of each commit luke-jr: utACK 2efdfb8 Tree-SHA512: e8731a0ef9e87564b2676c7b022b742d9621bba964c19dba9fd9f6961eb608737a9e1a22c0a3c8b2f2f6d583bba067606ee8392422e82082deefb20ea7b88c7c

achow101 mentioned this pull request Sep 30, 2021

qt: Add Create Unsigned button to SendConfirmationDialog bitcoin/bitcoin#18789

Closed

jarolrod added the UX All about "how to get things done" label Sep 30, 2021

shaavan suggested changes Oct 1, 2021

View reviewed changes

src/qt/forms/optionsdialog.ui Show resolved Hide resolved

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

src/qt/sendcoinsdialog.cpp Show resolved Hide resolved

src/qt/optionsmodel.h Show resolved Hide resolved

achow101 force-pushed the create-unsigned-sendconfdialog branch 2 times, most recently from fe11f65 to 790733e Compare October 1, 2021 22:45

laanwj reviewed Oct 7, 2021

View reviewed changes

src/qt/optionsmodel.h Outdated Show resolved Hide resolved

ryanofsky approved these changes Oct 7, 2021

View reviewed changes

achow101 force-pushed the create-unsigned-sendconfdialog branch from 790733e to 9d8cf22 Compare October 7, 2021 19:23

ryanofsky reviewed Oct 8, 2021

View reviewed changes

src/qt/optionsmodel.cpp Outdated Show resolved Hide resolved

qt: hide Create Unsigned button behind an expert mode option

742918c

achow101 force-pushed the create-unsigned-sendconfdialog branch from 9d8cf22 to 742918c Compare October 8, 2021 16:56

ryanofsky approved these changes Oct 8, 2021

View reviewed changes

hebasto requested a review from Sjors October 8, 2021 19:45

hebasto reviewed Oct 9, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 7, 2022

refactor, qt: Use std::chrono for parameters of QTimer methods #517

Merged

hebasto merged commit 2e01b69 into bitcoin-core:master Jan 9, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 9, 2022

Merge bitcoin-core/gui#441: Add Create Unsigned button to SendConfirm…

8d4951d

ryanofsky mentioned this pull request Jan 10, 2022

interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936

Merged

Sjors mentioned this pull request Jan 12, 2022

rpc, gui: bumpfee signer support bitcoin/bitcoin#21576

Merged

This was referenced Feb 18, 2022

External signer Send button disabled #551

Closed

Restore Send button when using external signer #555

Merged

bitcoin-core locked and limited conversation to collaborators Jan 9, 2023

	if (confirmButtonText.isEmpty()) {
	confirmButtonText = yesButton->text();
	}

	QAbstractButton *m_psbt_button;
	QAbstractButton* m_psbt_button;

Add Create Unsigned button to SendConfirmationDialog #441

Add Create Unsigned button to SendConfirmationDialog #441

Uh oh!

Conversation

achow101 commented Sep 30, 2021

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 8, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 9, 2021

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Oct 28, 2021

Uh oh!

DrahtBot commented Jan 7, 2022

Conflicts

Uh oh!

Uh oh!