ui: make send a wizard

Sjors commented at 6:43 pm on September 25, 2019: member

Implements #16954 for the current functionality + #16944 (gui: create PSBT with watch-only wallet). This PR splits the send screen into three tabs, like a wizard.

This frees up UI real estate where we can add support for PSBT, hardware wallets and education.

I renamed SendCoinsDialog to SendCompose and SendConfirmationDialog to SendSignfor clarity, which ended up (trivially) touching lots of src/qt/locale/bitcoin_##.ts files. This is contained in two move-only commits.

Tab 1: Draft

Same as the current send screen: enter destination, do coin selection, set fee etc. This can be split further in future PRs for a less cluttered experience, e.g. one tab for coin selection (if enabled), one for destination(s) and one for fees. Having a separate tab for fees also provides an entry point for RBF, which currently doesn’t let the user pick an amount.

Tab 2: Sign

This asks to unlock the wallet if needed.

Display transaction details like the current popup does.

Edit jumps back to Draft. Send jumps to Finish, unless something goes wrong.

Bump fee jumps straight to this tab:

For watch-only wallets it displays the same text as #16944.

Tab 3: Finish

This is where the actual broadcast takes place, or where the PSBT is copied to the clipboard. In a followup we can add support for saving the PSBT to disk, or for copying a signed transaction hex to clipboard if the user wants to broadcast that elsewhere.

Bump fee shows both the previous and new transaction index:

The manual “Show” button has the nice side-effect of fixing #16875 / #16876 in two out of three places.

Todo:

clean up WalletModelTransaction and CoinControl object passing mess
restore test

Followups:

add PSBT export to Sign tab (extract from gwillen’s branch https://github.com/gwillen/bitcoin/tree/feature-offline-v2, i.e. redo #16944)
add Load PSBT menu option, jump to Broadcast tab if complete, otherwise to Sign tab
list connected hardware wallets in Sign tab (redo #16549
split fee selection into its own tab
allow custom RBF amounts: bump fee should jump to fee selection tab

promag commented at 8:09 pm on September 25, 2019: member

Concept ACK. Not sure if the tab view is the best container for the send wizard. Have you considered back/next buttons?

gwillen commented at 8:17 pm on September 25, 2019: contributor

I went back and for myself on the view style and ultimately did settle on a tab view. Ultimately there’s never actually a need to press the Back and Next buttons unless you’re doing something wrong (or a trivial demo) – each step of the process will happen on a different machine.

I like the idea of a “load PSBT” menu item taking you to the right tab. In my implementation, there is an “offline” menu from which you have to select “sign” or “broadcast” to open the dialog to the right tab; then you load from there.

There’s a lot of stuff that came up in the process of implementing this which I don’t know if you’ve run into yet. For example, the signing wallet may not be able to put up the usual form of the confirmation display, as shown here, unless it has some trustworthy way to know which output is change, which it doesn’t except maybe if you’re using descriptors to tell it so.

Jumping back to Draft, much like having back/next buttons, is probably mostly not going to get used in real life, where the signing is happening on a different machine from the drafting. Honestly I think the main reason to have the steps of this flow be one dialog are to help the user have a sense of their place in the process, even though I expect probably they will be mostly used separately and on separate machines.

DrahtBot added the label Build system on Sep 25, 2019

DrahtBot added the label GUI on Sep 25, 2019

DrahtBot added the label Scripts and tools on Sep 25, 2019

fanquake removed the label Scripts and tools on Sep 25, 2019

DrahtBot commented at 11:18 pm on September 25, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17219 (wallet: allow transaction without change if keypool is empty by Sjors)
#17165 (Remove BIP70 support by fanquake)
#17154 (wallet: Remove return value from CommitTransaction by jnewbery)
#16944 (gui: create PSBT with watch-only wallet by Sjors)
#16876 (gui: Drop calls to QCoreApplication::processEvents by promag)
#16710 (build: Enable -Wsuggest-override if available by hebasto)
#16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)

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.

Sjors commented at 8:45 am on September 26, 2019: member

the signing wallet may not be able to put up the usual form of the confirmation display, as shown here, unless it has some trustworthy way to know which output is change, which it doesn’t except maybe if you’re using descriptors to tell it so.

I have a hardware wallet GUI PR #16549 which is built on top of native descriptor wallets #16528.

I’m also thinking of replacing WalletModelTransction, which is a QT-only thing, with PSBT. But perhaps that’s better for a followup.

Re tab view: I briefly looked at QWizard but that doesn’t give the user an overview of where they are in the flow. The good news is that QTabWidget can easily be swapped with QStackedWidget which can be completely customized, e.g. into something like this:

I’ll probably disable the other tabs, so that all “navigation” happens through the buttons at the bottom of the screen.

I expect probably they will be mostly used separately and on separate machines

I’m thinking more of the hardware wallet use case. In the simplest configuration of 1 hardware wallet that’s physically connected, the Sign tab is a nice place to show it. In case of something like ColdCard or the new BitBox this is where you would save the unsigned PSBT to SD drive.

Sjors force-pushed on Sep 26, 2019

Sjors commented at 6:10 pm on September 26, 2019: member

Added RBF. If a followup PR splits fee selection out of Tab 1 then bump fee could also support a custom amount, rather the current take it or leave it fee.

I’m considering squashing this into a single commit; current commit history is probably more confusing than useful.

hebasto commented at 1:55 pm on September 30, 2019: member

Concept ACK

Sjors force-pushed on Sep 30, 2019

MarcoFalke referenced this in commit 19ba43ae2d on Oct 2, 2019

[wallet] ListCoins: include watch-only for wallets without private keys

This makes them available in GUI coin selection.

a16d2eb0d5

[gui] send: include watch-only

For wallets with WALLET_FLAG_DISABLE_PRIVATE_KEYS.

eef2b63506

[wallet] add fillPSBT to interface 2bc1012287

Sjors force-pushed on Oct 2, 2019

sidhujag referenced this in commit da4ce75ee1 on Oct 2, 2019

Sjors force-pushed on Oct 11, 2019

Sjors commented at 3:04 pm on October 11, 2019: member

qt/wallettests.cpp breaks because the transactionTableModel doesn’t update after the transactions are created. @promag could this be related to #16876 (comment)?

Before this change, a healthy test log contains an updateWallet entry which is missing in the failing test:

0PASS   : WalletTests::initTestCase()
1QDEBUG : WalletTests::walletTests() TransactionTablePriv::refreshWallet
2QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: df30faf7e56ab6b7221ca3464427b4a18bf55cb8dd1ea0f2a7f05e29db5fa3fb status= 0"
3QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: ff71c01681d9c02363746313b6e58cd174e0afde19a40e0597e4240a84209668 status= 1"
4QDEBUG : WalletTests::walletTests() "NotifyAddressBookChanged: mfWxJ45yp2SFn7UciZyNpvDKrzbhyfKrY8  isMine=0 purpose=send status=0"
5QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: df30faf7e56ab6b7221ca3464427b4a18bf55cb8dd1ea0f2a7f05e29db5fa3fb 0"
6QDEBUG : WalletTests::walletTests() "    inModel=0 Index=103-103 showTransaction=1 derivedStatus=0"

Sjors force-pushed on Oct 11, 2019

Sjors commented at 5:07 pm on October 11, 2019: member

NotifyTransactionChanged in WalletModel calls updateTransaction using a Qt::QueuedConnection. That caused the test to fail.

Sjors force-pushed on Oct 11, 2019

Sjors marked this as ready for review on Oct 11, 2019

promag commented at 9:25 am on October 12, 2019: member

@Sjors try like this https://github.com/bitcoin/bitcoin/pull/16876/files#diff-6fcb1829ce66ad65046d6896dbcc910bR73

Sjors commented at 10:11 am on October 12, 2019: member

@promag added.

For some reason the Travis nowallet build is failing, but I can’t reproduce:

 0D__STDC_FORMAT_MACROS  -fstack-reuse=none -Wstack-protector -fstack-protector-all      -fPIE -pipe -O2  -fvisibility=hidden -c -o qt/qt_libbitcoinqt_a-moc_sendcompose.o `test -f 'qt/moc_sendcompose.cpp' || echo './'`qt/moc_sendcompose.cpp
 12790In file included from ./wallet/walletdb.h:12:0,
 22791                 from ./wallet/wallet.h:23,
 32792                 from ./wallet/coincontrol.h:11,
 42793                 from ./qt/sendcompose.h:14,
 52794                 from qt/moc_sendcompose.cpp:8:
 62795./wallet/db.h:24:10: fatal error: db_cxx.h: No such file or directory
 72796 #include <db_cxx.h>
 82797          ^~~~~~~~~~
 92798compilation terminated.
102799make[2]: *** [qt/qt_libbitcoinqt_a-moc_sendcompose.o] Error 1

Update: figured out on IRC, problem is in using CCoinControl in a Q_SIGNAL. Looking at some different approaches to passing the transaction and coin control around.

Sjors force-pushed on Oct 12, 2019

Sjors force-pushed on Oct 13, 2019

Sjors commented at 8:51 am on October 13, 2019: member

Travis is happy. TIL that build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj isn’t auto generated, so I had to update that as well. Now AppVeyor is happy too.

I’m probably going to add a SendWidget class to share common code between the tabs. I might also replace unique_ptr with shared_ptr as the current passing the baton approach seems a bit brittle.

[gui] watch-only wallet: copy PSBT to clipboard 9808f7867d

DrahtBot commented at 9:36 am on October 24, 2019: member

DrahtBot added the label Needs rebase on Oct 24, 2019

[move-only] rename SendCoinsdialog to SendCompose 7fcae96253

[move-only] rename SendConfirmationDialog to SendSign bf019245a0

[gui] temporarily remove RBF 64b280d388

[wallet] CCoinControl: add copy constructor de0ed928ad

[gui] make send screen a wizard

Review hint:
git show -w --color-moved=dimmed-zebra

Co-authored-by: Glenn Willen <gwillen@nerdnet.org>

4887649b1b

[gui] use send wizard for RBF bdc0f5e810

gui: Drop calls to QCoreApplication::processEvents fa1cd32873

Sjors force-pushed on Oct 27, 2019

luke-jr commented at 1:38 pm on November 4, 2019: member

Concept NACK. Sorry, this looks ugly, much worse than the current flow.

(Also, normal users probably shouldn’t see txids…)

Sjors commented at 2:40 pm on November 4, 2019: member

That’s subjective. I think it looks better, because it’s less cluttered and gets rid of a popup. Happy to hide the transaction id.

The current sign screen (which is a popup) doesn’t have space to list hardware wallets. There’s also not much room for PSBT creation and/or signing, other than just magically putting it on a clipboard (see #16944). If you have a better design I’d love to see it.

luke-jr commented at 3:06 pm on November 4, 2019: member

If you have a better design I’d love to see it.

To be clear, I think the current design with popup is better…

michaelfolkson commented at 3:25 pm on November 4, 2019: contributor

This frees up UI real estate where we can add support for PSBT, hardware wallets and education.

Concept ACK on motivation for this. I don’t have a view on its ugliness. For these UI redesigns it seems totally dependent on the regular users of the GUI. Personally I support the goal of the GUI being an educational stepping stone to using the CLI. There will be always be better GUIs for the non technically curious which aren’t subject to the constraints of Core.

luke-jr commented at 5:23 pm on November 4, 2019: member

My view is almost entirely the opposite: The JSON-RPC server isn’t meant for end users, but for software developers to interface with. The GUI is the primary method for end users to use Bitcoin Core, not some mere “stepping stone” to a half-baked JSON-RPC client…

P.S. In addition to being a developer, I am a regular user of the GUI.

Sjors commented at 5:33 pm on November 4, 2019: member

I think @michaelfolkson was referring to having independently developed wallet applications that use the RPC (somewhat limited atm) or IPC (#10102). I use the GUI as well.

michaelfolkson commented at 6:00 pm on November 4, 2019: contributor

I’m not entirely sure who this GUI is being designed for. Long time contributors like Sjors, Luke? Complete Bitcoin beginners? Someone like me who is technically curious and would ideally want to transition from any reliance on the GUI? All of us? (fiendishly difficult as you are making design compromises all over the place).

If it is complete Bitcoin beginners then none of us are the target audience. But this is a meta point and this discussion has probably been rehashed multiple times over the years so I won’t clog up this PR with it.

I’ve opened an issue #17395. I don’t want to derail review of this particular PR.

Rspigler commented at 0:56 am on November 8, 2019: contributor

Concept NACK. Sorry, this looks ugly, much worse than the current flow To be clear, I think the current design with popup is better…

What current design allows you to generate PSBT’s from the GUI of offline versions of Bitcoin Core for secure multisig? AFAIK, #16944, #16954, and this PR are the only discussions/PRs around this (https://github.com/bitcoin/bitcoin/pull/16546, #16549, and #16895 for HWWs).

The UI isn’t beautiful (neither is the rest of Core), but it’s not ugly. And I don’t believe that’s the point, especially from reading #17395, that seems to be the consensus. The point is to be secure, power-user friendly, and mistake-minimizing.

michaelfolkson commented at 1:10 pm on November 8, 2019: contributor

And I don’t believe that’s the point, especially from reading #17395, that seems to be the consensus. The point is to be secure, power-user friendly, and mistake-minimizing.

Do you agree with that @luke-jr? Feel free to comment on #17395 if you do/don’t.

Sjors commented at 10:54 am on November 19, 2019: member

Closing this in favor of more incremental changes, e.g. #17509.

Sjors closed this on Nov 19, 2019

fanquake removed the label Needs rebase on Aug 20, 2020

PastaPastaPasta referenced this in commit a5a13e9f99 on Jun 27, 2021

PastaPastaPasta referenced this in commit c850fec079 on Jun 28, 2021

PastaPastaPasta referenced this in commit 83359cdac6 on Jun 29, 2021

PastaPastaPasta referenced this in commit 4aca3fa00b on Jul 1, 2021

PastaPastaPasta referenced this in commit 1c21116cc2 on Jul 1, 2021

PastaPastaPasta referenced this in commit 7ee36b407a on Jul 12, 2021

PastaPastaPasta referenced this in commit fb3f3747a4 on Jul 13, 2021

PastaPastaPasta referenced this in commit f46f16772d on Jul 13, 2021

DrahtBot locked this on Feb 15, 2022

ui: make send a wizard #16966

Conflicts