Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Nov 29, 2019

Conversation

@Sjors
Copy link
Member

Sjors commented Nov 25, 2019

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:
before

After:
Schermafbeelding 2019-11-26 om 11 44 17

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.

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 25, 2019

ACK 0a0f367.

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

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 25, 2019

@promag I created #17588 as a reminder, but if you feel like it...

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 25, 2019

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

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 25, 2019

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Contributor

gwillen commented Nov 26, 2019

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.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Nov 26, 2019

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.

@Sjors Sjors force-pushed the Sjors:2019/11/send_balance branch from 0a0f367 to 4a96e45 Nov 26, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 26, 2019

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

This comment has been minimized.

Copy link
Contributor

gwillen commented Nov 26, 2019

@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

This comment has been minimized.

Copy link
Member

meshcollider commented Nov 27, 2019

utACK 4a96e45

Copy link
Member

instagibbs left a comment

code review and light test ACK 4a96e45

single, most-relevant balance is the best way to go. I'm doing it for all my other related PRs.

@@ -170,6 +170,16 @@ void TestGUI(interfaces::Node& node)
sendCoinsDialog.setModel(&walletModel);
transactionView.setModel(&walletModel);

{
// Check balance in send dialog

This comment has been minimized.

Copy link
@instagibbs

instagibbs Nov 27, 2019

Member

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

This comment has been minimized.

Copy link
@promag
@jb55

This comment has been minimized.

Copy link
Contributor

jb55 commented Nov 27, 2019

utACK 4a96e45

@promag

This comment has been minimized.

Copy link
Member

promag commented Nov 28, 2019

reACK 4a96e45, rebased and label change since last review.

meshcollider added a commit that referenced this pull request Nov 29, 2019
4a96e45 [gui] send: show watch-only balance in send screen (Sjors Provoost)
2689c8f [test] qt: add send screen balance test (Sjors Provoost)

Pull request description:

  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.

ACKs for top commit:
  meshcollider:
    utACK 4a96e45
  jb55:
    utACK 4a96e45
  promag:
    reACK 4a96e45, rebased and label change since last review.
  instagibbs:
    code review and light test ACK 4a96e45

Tree-SHA512: 4213549888bd309f72bdbba1453218f4a2b07e809100d786a3791897c75468f9092b06fe4b971942b1c228aa75ee7c04971f262ca9a478b42756e056eb534620
@meshcollider meshcollider merged commit 4a96e45 into bitcoin:master Nov 29, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Sjors Sjors deleted the Sjors:2019/11/send_balance branch Nov 29, 2019
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 29, 2019
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 30, 2019
4a96e45 [gui] send: show watch-only balance in send screen (Sjors Provoost)
2689c8f [test] qt: add send screen balance test (Sjors Provoost)

Pull request description:

  Now that we can create a PSBT from a watch-only wallet (bitcoin#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.

ACKs for top commit:
  meshcollider:
    utACK 4a96e45
  jb55:
    utACK 4a96e45
  promag:
    reACK 4a96e45, rebased and label change since last review.
  instagibbs:
    code review and light test ACK bitcoin@4a96e45

Tree-SHA512: 4213549888bd309f72bdbba1453218f4a2b07e809100d786a3791897c75468f9092b06fe4b971942b1c228aa75ee7c04971f262ca9a478b42756e056eb534620
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.