qt: Add Create Unsigned button to SendConfirmationDialog #18789

achow101 · 2020-04-27T21:03:22Z

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.

Supersedes #18656 and #18655

achow101 · 2020-04-27T21:08:42Z

@luke-jr suggested that such a button should be behind an expert mode thing but I'm not sure how to do that. It's not clear to me how that would look in the GUI. One thought I had was to have something like an "advanced options" box that could be hidden, but I don't know enough about Qt to implement such a thing.

DrahtBot · 2020-04-27T23:02:24Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21576 (rpc, gui: bumpfee signer support by Sjors)
#15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

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.

instagibbs · 2020-04-28T17:16:20Z

Definitely an improvement from prior PRs. Have you looked at Enable coin control features in the options menu? I tend to agree with Luke here in that for a basic user it's still too much clutter and could confuse them into thinking they've sent when they have not so that might be a place to put that option if you can figure it out. Maybe the button could also automagically show up if and only if it's a no privkey wallet(or instructs the user to set the option?).

Sjors · 2020-04-28T18:50:27Z

This is very scary:

I don't think this model is a good place for this. It's main goal is to allow the user to check their transaction, not to make new decisions.

I think the current behaviour of showing "the right" button is good enough for now. Under the hood it could be separate buttons of course, I don't care too deeply.

I would prefer to add an option like "Create PSBT instead of sending transaction" in the Expert settings:

This preference can later be stored in the wallet payload (so it becomes a per-wallet preference rather than global). See also "wallet config options" in #13044.

achow101 · 2020-04-28T19:25:29Z

I don't think this model is a good place for this. It's main goal is to allow the user to check their transaction, not to make new decisions.

I don't see how making new decisions is mutually exclusive with checking the transaction. Users are still checking the transaction.

I think the current behaviour of showing "the right" button is good enough for now. Under the hood it could be separate buttons of course, I don't care too deeply.

The whole point of this is to move away from having private keys disabled change what buttons do. I disagree that showing "the right button" is a good design decision.

achow101 · 2020-04-28T20:19:21Z

I've added a checkbox in the options to enable/disable showing the Create Unsigned button. The button will be shown for privateKeysDisabled too.

Sjors · 2020-04-29T07:58:01Z

Thanks for adding the setting.

For watch-only wallets, the send screen still says Create Unsigned:

That should be Send for consistency. But that could be confusing; a user might know they need a PSBT, but they only see a "Send" button.

So I think your original approach of adding the Create Unsigned button in the Send screen (and the extra menu option for bump fee) was better, at least when combined with the setting you added. That way:

nothing changes for new users with a default wallet with private keys
for watch-only wallets by default users will see a disabled Send button and a Create PSBT
with the setting enabled, users with a private key wallet will see both buttons

achow101 · 2020-04-29T16:41:42Z

That should be Send for consistency. But that could be confusing; a user might know they need a PSBT, but they only see a "Send" button.

That could be changed to say "Create". I think "Create" is broad enough to imply "make and send" and "make unsigned".

So I think your original approach of adding the Create Unsigned button in the Send screen (and the extra menu option for bump fee) was better, at least when combined with the setting you added. That way:

The whole point of this PR is because that may confuse users. I also like this approach as it is simpler.

src/qt/forms/optionsdialog.ui

src/qt/optionsmodel.cpp

src/qt/sendcoinsdialog.cpp

jarolrod

Testing 768ef5c, does not compile on Arch Linux (kernel 5.8.16) with Qt 5.15.2.
Failures are related to QPainterPath found in qt/trafficgraphwidget.cpp
Below are the failures:

qt/trafficgraphwidget.cpp:55:9: error: invalid use of incomplete type ‘class QPainterPath’
   55 |         path.moveTo(x, YMARGIN + h);
      |         ^~~~

qt/trafficgraphwidget.cpp:59:13: error: invalid use of incomplete type ‘class QPainterPath’
   59 |             path.lineTo(x, y);
      |             ^~~~
qt/trafficgraphwidget.cpp:61:9: error: invalid use of incomplete type ‘class QPainterPath’
   61 |         path.lineTo(x, YMARGIN + h);
      |         ^~~~
qt/trafficgraphwidget.cpp:109:22: error: aggregate ‘QPainterPath p’ has incomplete type and cannot be define
  109 |         QPainterPath p;
      |                      ^
qt/trafficgraphwidget.cpp:116:22: error: aggregate ‘QPainterPath p’ has incomplete type and cannot be define
  116 |         QPainterPath p;
      |                      ^

luke-jr · 2020-12-04T19:30:26Z

@jarolrod You probably need to merge with master... That's an old issue unrelated to this PR

jarolrod · 2020-12-04T19:53:57Z

@jarolrod You probably need to merge with master... That's an old issue unrelated to this PR

Thanks! Wasn't aware, now I know. False alarm, I merged and 768ef5c compiles.

jarolrod

Concept ACK

Will think about the changes over and give this another sweep through tomorrow. Some feedback for now. Mainly translator comments.

src/qt/sendcoinsdialog.cpp

src/qt/walletmodel.cpp

src/qt/sendcoinsdialog.cpp

src/qt/walletmodel.cpp

src/qt/sendcoinsdialog.cpp

src/qt/walletmodel.cpp

Sjors · 2021-06-16T09:23:36Z

tACK 72e0239

For the default wallet not much changes in the interface, unless the user opts into the advanced PSBT setting. So that's good.

Although "Increase fee" can correctly create a PSBT, if you actually broadcast it the transaction list tab is confusing, because it doesn't mark the original as replaced (until it's confirmed). But that's a separate bug.

src/qt/sendcoinsdialog.cpp

src/qt/forms/optionsdialog.ui

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.

DrahtBot · 2021-09-30T09:57:40Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

achow101 · 2021-09-30T19:44:03Z

Moved to the GUI repo: bitcoin-core/gui#441

742918c qt: hide Create Unsigned button behind an expert mode option (Andrew Chow) 5c3b800 qt: Add Create Unsigned button to SendConfirmationDialog (Andrew Chow) Pull request description: 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 ACKs for top commit: jarolrod: ACK 742918c ryanofsky: Code review ACK 742918c. Just suggested changes since last review. Looks great! hebasto: ACK 742918c, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: dd29f4364c7b4f15befe8fe63257b26187918786b005e0f8336183270b1a162680b93f6ced60f0285c6e607c084cc0d24950fc68a8f9c056521ede614041be66

This was referenced Apr 27, 2020

gui: Add a Make unsigned button next to Send #18656

Closed

gui: Add bumpFeePSBT action instead of changing normal bumpfee behavior #18655

Closed

DrahtBot added the GUI label Apr 27, 2020

This was referenced Apr 28, 2020

build: Enable -Wsuggest-override if available #16710

Merged

UI external signer support (e.g. hardware wallet) #16549

Closed

DrahtBot mentioned this pull request May 8, 2020

refactor: Apply override specifier consistently #18914

Merged

DrahtBot added the Needs rebase label May 11, 2020

achow101 force-pushed the create-unsigned-sendconfdialog branch from 47829d1 to dcf43d6 Compare May 12, 2020 18:01

DrahtBot removed the Needs rebase label May 12, 2020

luke-jr suggested changes May 18, 2020

View reviewed changes

achow101 force-pushed the create-unsigned-sendconfdialog branch 2 times, most recently from 284d0aa to fc2d0a9 Compare May 19, 2020 20:12

luke-jr reviewed May 30, 2020

View reviewed changes

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

luke-jr reviewed May 30, 2020

View reviewed changes

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

This was referenced Jun 3, 2020

Updated text on send confirmation dialog #18351

Closed

interfaces: Expose settings.json methods to GUI #15936

Merged

achow101 force-pushed the create-unsigned-sendconfdialog branch from fc2d0a9 to 768ef5c Compare June 8, 2020 15:51

DrahtBot mentioned this pull request Aug 21, 2020

Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" #17463

Closed

jarolrod reviewed Dec 4, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jan 13, 2021

achow101 force-pushed the create-unsigned-sendconfdialog branch from 768ef5c to 1601a71 Compare January 13, 2021 18:45

DrahtBot removed the Needs rebase label Jan 13, 2021

DrahtBot added the Needs rebase label Jan 26, 2021

achow101 force-pushed the create-unsigned-sendconfdialog branch from 1601a71 to 954ed37 Compare January 26, 2021 18:30

DrahtBot removed the Needs rebase label Jan 26, 2021

DrahtBot mentioned this pull request May 13, 2021

Enable external signer support by default, reduce #ifdef #21935

Merged

DrahtBot added the Needs rebase label Jun 9, 2021

achow101 force-pushed the create-unsigned-sendconfdialog branch from 954ed37 to 72e0239 Compare June 9, 2021 18:17

DrahtBot removed the Needs rebase label Jun 9, 2021

jarolrod reviewed Jun 14, 2021

View reviewed changes

DrahtBot added the Needs rebase label Aug 12, 2021

achow101 force-pushed the create-unsigned-sendconfdialog branch from 72e0239 to 6e74c01 Compare August 17, 2021 23:15

DrahtBot removed the Needs rebase label Aug 18, 2021

jarolrod reviewed Aug 18, 2021

View reviewed changes

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

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

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

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

achow101 force-pushed the create-unsigned-sendconfdialog branch from 6e74c01 to 84077db Compare August 18, 2021 17:58

DrahtBot mentioned this pull request Sep 2, 2021

rpc, gui: bumpfee signer support #21576

Merged

DrahtBot added the Needs rebase label Sep 29, 2021

achow101 added 2 commits September 29, 2021 14:23

qt: hide Create Unsigned button behind an expert mode option

55c19f9

achow101 force-pushed the create-unsigned-sendconfdialog branch from 84077db to 55c19f9 Compare September 29, 2021 18:32

DrahtBot removed the Needs rebase label Sep 29, 2021

DrahtBot added the Needs rebase label Sep 30, 2021

achow101 mentioned this pull request Sep 30, 2021

Add Create Unsigned button to SendConfirmationDialog bitcoin-core/gui#441

Merged

achow101 closed this Sep 30, 2021

bitcoin locked and limited conversation to collaborators Oct 30, 2022

qt: Add Create Unsigned button to SendConfirmationDialog #18789

qt: Add Create Unsigned button to SendConfirmationDialog #18789

Uh oh!

Conversation

achow101 commented Apr 27, 2020

Uh oh!

achow101 commented Apr 27, 2020

Uh oh!

DrahtBot commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

instagibbs commented Apr 28, 2020

Uh oh!

Sjors commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Apr 28, 2020

Uh oh!

achow101 commented Apr 28, 2020

Uh oh!

Sjors commented Apr 29, 2020

Uh oh!

achow101 commented Apr 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Dec 4, 2020

Uh oh!

jarolrod commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Sep 30, 2021

Uh oh!

achow101 commented Sep 30, 2021

Uh oh!

Uh oh!

DrahtBot commented Apr 27, 2020 •

edited

Loading

Sjors commented Apr 28, 2020 •

edited

Loading

jarolrod commented Dec 4, 2020 •

edited

Loading

Sjors commented Jun 16, 2021 •

edited

Loading