gui: show watch-only balance in send screen #17587

pull Sjors wants to merge 2 commits into bitcoin:master from Sjors:2019/11/send_balance changing 3 files +17 −2

Sjors commented at 10:39 AM on November 25, 2019: member

Now that we can create a PSBT from a watch-only wallet (#16944), we should also display the watch-only balance on the send screen.

Before: <img width="1008" alt="before" src="https://user-images.githubusercontent.com/10217/69533384-030e9180-0f78-11ea-9748-c32c957e822e.png">

After: <img width="1009" alt="Schermafbeelding 2019-11-26 om 11 44 17" src="https://user-images.githubusercontent.com/10217/69622879-19811f80-1042-11ea-8279-091012f39b38.png">

I added a test to check the balance on the send screen, but it only covers regular wallets. A better would add a watch-only only wallet.
DrahtBot added the label GUI on Nov 25, 2019
DrahtBot added the label Tests on Nov 25, 2019
promag commented at 2:31 PM on November 25, 2019: member

ACK 0a0f3676d0fe755c91693ecfdb1cb48608fb1fdc.

A better would add a watch-only only wallet.

Indeed, and it would be even better if added in this PR. I can write it for you.
Sjors commented at 2:47 PM on November 25, 2019: member

@promag I created #17588 as a reminder, but if you feel like it...
promag commented at 2:50 PM on November 25, 2019: member

Ok, considering that issue I think this is ready to merge. The only nit I can think of is to change the bottom label to "watch only balance" or something like that.
Sjors commented at 3:03 PM on November 25, 2019: member

Maybe. But there's already a watch-only icon (the eye) right below the balance, and this feature is exclusively for watch-only wallets.
jonasschnelli commented at 2:20 AM on November 26, 2019: contributor

What if the wallet has watch only and hot balances? Maybe display both in a such case and if the wallet only has a watch-only balance, label it correctly?
gwillen commented at 9:38 AM on November 26, 2019: contributor

Whoops, I went to go make this change and discovered this PR open! :-) @Sjors, I am hoping to start breaking my old giant offline branch into manageable pieces and start PRing it on top of your work -- we should coordinate so we aren't both trying to do it.

I do think the label should change; I think displaying both balances (which is what I had done in my old branch) is less compatible with the way this was handled in Sjors' other PR, since all the new logic only triggers if the wallet has private keys disabled, which forbids having a hot balance.
[test] qt: add send screen balance test 2689c8fd71
Sjors commented at 10:43 AM on November 26, 2019: member

I renamed the balance label. @jonasschnelli there's no PSBT functionality in wallets with private keys, hence if (model->privateKeysDisabled()). Those wallets can't spend from their watch-only address in the GUI, and so it makes sense to not display the watch-only balance on the send screen (it's shown on the Overview screen).

In general I think the plan is to encourage creating separate wallets. @gwillen I've been taking bits and pieces from your offline project, see also #17509.
[gui] send: show watch-only balance in send screen 4a96e459d7
Sjors force-pushed on Nov 26, 2019
laanwj commented at 12:07 PM on November 26, 2019: member

Concept ACK.

there's no PSBT functionality in wallets with private keys, hence if (model->privateKeysDisabled())

I really like @Sjors keeping this simple and displaying only one balance (the most relevant one for the purpose). For a full list of balances there's the overview page.
gwillen commented at 6:24 PM on November 26, 2019: contributor

@Sjors maybe we could chat on IRC about it, I am back from my sabbatical and hoping to resume this work myself. Sorry for leaving it hanging for so long!
meshcollider commented at 8:30 AM on November 27, 2019: contributor

utACK 4a96e459d733f1b6427221aaa1874ea00f79988a

in src/qt/test/wallettests.cpp:174 in 2689c8fd71 outdated

 169 | @@ -170,6 +170,16 @@ void TestGUI(interfaces::Node& node)
 170 |      sendCoinsDialog.setModel(&walletModel);
 171 |      transactionView.setModel(&walletModel);
 172 |  
 173 | +    {
 174 | +        // Check balance in send dialog

instagibbs commented at 3:03 PM on November 27, 2019:

just noting this test won't catch the current behavior, that's ok for now

promag commented at 10:11 PM on November 28, 2019:

Yup #17588.

instagibbs approved
instagibbs commented at 3:05 PM on November 27, 2019: member

code review and light test ACK https://github.com/bitcoin/bitcoin/pull/17587/commits/4a96e459d733f1b6427221aaa1874ea00f79988a

single, most-relevant balance is the best way to go. I'm doing it for all my other related PRs.
jb55 commented at 4:35 PM on November 27, 2019: member

utACK 4a96e459d733f1b6427221aaa1874ea00f79988a
promag commented at 10:13 PM on November 28, 2019: member

reACK 4a96e45, rebased and label change since last review.
meshcollider referenced this in commit abb30de63f on Nov 29, 2019
meshcollider merged this on Nov 29, 2019
meshcollider closed this on Nov 29, 2019
Sjors deleted the branch on Nov 29, 2019
MarkLTZ referenced this in commit a53a2cb9a1 on Nov 29, 2019
sidhujag referenced this in commit a289b8cba8 on Nov 30, 2019
luke-jr referenced this in commit fd158a64b9 on Jan 5, 2020
jasonbcox referenced this in commit a690e66ad2 on Oct 23, 2020
sidhujag referenced this in commit b9a1d85c52 on Nov 10, 2020
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#16944 gui: create PSBT with watch-only wallet #17588 Test PSBT creation in GUI