gui: When private key is disabled, only show watch-only balance by ken2812221 · Pull Request #13966 · bitcoin/bitcoin

ken2812221 · 2018-08-14T13:37:28Z

If a wallet is in private key disabled mode, the spendable balance is always zero, it does not have to show on GUI. Show the watch-only balance at normal balance column if a wallet is in that mode.

DrahtBot · 2018-08-14T14:34:49Z

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

Conflicts

No conflicts as of last run.

promag

Concept ACK. Could add before and after shots.

promag · 2018-08-15T23:00:22Z

src/qt/overviewpage.cpp

nit, since there is one caller only, could remove it and below do

connect(model, &WalletModel::notifyWatchonlyChanged, [this](bool showWatchOnly) { updateWalletLabels(showWatchOnly, !walletModel->privateKeysDisabled()); });

promag · 2018-08-15T23:04:14Z

With multiple wallets loaded, it may be weird to have the toggling, maybe disable instead of hiding?

laanwj · 2018-08-22T07:48:52Z

Concept ACK

jonasschnelli · 2018-08-22T07:50:02Z

Nice!
Concept ACK

laanwj · 2018-08-27T11:01:35Z

src/qt/overviewpage.cpp

code style nit:
} else {

fanquake · 2018-09-02T13:28:44Z

Testing fe1ff50 using the following commands (in the Debug window given that we're looking at a GUI change). Screenshots below, will review further shortly.

src/qt/bitcoin-qt --testnet
createwallet "regular-wallet" false
createwallet "watch-only-wallet" true

// import a random testnet address
importaddress "some-address" "random-addr" false false

jonasschnelli · 2018-09-07T19:32:05Z

Tested ACK fe1ff50
Works as expected (also tested with multiwallet)

Sjors · 2018-09-07T20:10:33Z

Concept ACK, the distinction between"spendable" and "watch-only" is confusing anyway, and a big source of scams (even if that particular scam is less likely here, though something to be aware of once #13100 is merged).

The concept of watch-only might become less clear in general moving forward. @sipa does this get in the way of the direction you have in mind for the wallet refactor?

instagibbs · 2018-09-14T19:52:34Z

@fanquake based on your image, I think some indications that private keys are disabled is probably desired? Otherwise it looks to a long-time user that indeed you own the funds and have the keys locally.

Sjors · 2018-09-15T11:58:41Z

I believe (didn't test) that you still see the eye icon next to transactions to indicate watch-only, but I think a regular icon in the bottom right would be better. We could get rid of the HD icon to keep visual clutter constant.

ken2812221 · 2018-09-18T03:24:01Z

Update: Now it shows watch-only eye instead of HD/non-HD wallet icon.

Sjors

tACK 82d6c5a on macOS 10.13.6

Before:

After:

Sjors · 2018-09-20T15:18:25Z

src/qt/bitcoingui.h

@@ -215,7 +215,7 @@ public Q_SLOTS:
     @param[in] status            current hd enabled status


Nit: missing param comment

meshcollider

Tested ACK 82d6c5a

As an alternative which I think might be clearer, what about including the eye next to the Balances title like this?

micro-nit: typo in first commit message priveate -> private

meshcollider · 2018-11-10T05:02:15Z

src/qt/bitcoingui.cpp

-    labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(hdEnabled ? ":/icons/hd_enabled" : ":/icons/hd_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
-    labelWalletHDStatusIcon->setToolTip(hdEnabled ? tr("HD key generation is <b>enabled</b>") : tr("HD key generation is <b>disabled</b>"));
+    labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(privkeyDisabled ? ":/icons/eye" : hdEnabled ? ":/icons/hd_enabled" : ":/icons/hd_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
+    labelWalletHDStatusIcon->setToolTip(privkeyDisabled ? tr("Private key <b>disabled</b>") : hdEnabled ? tr("HD key generation is <b>enabled</b>") : tr("HD key generation is <b>disabled</b>"));


key -> keys?

sipa · 2018-11-11T03:23:57Z

Only vaguely related, @gmaxwell suggested that perhaps we want a wallet-wide flag to treat all watchonly stuff as normal ismine. That goes further than this PR (also affects all RPC commands), but since the existence of multiwallet there is little reason for anyone to mix different "levels" of ismine in the same wallet.

laanwj · 2018-12-01T11:20:38Z

utACK 82d6c5a

I agree with @sipa, but a change like that will take some planning on how to avoid disruption for people currently using watch-only in existing wallets.

… balance 82d6c5a gui: Show watch-only eye instead of HD disabled (Chun Kuan Lee) fe1ff50 Hide spendable label if priveate key is disabled (Chun Kuan Lee) Pull request description: If a wallet is in private key disabled mode, the spendable balance is always zero, it does not have to show on GUI. Show the watch-only balance at normal balance column if a wallet is in that mode. ![image](https://user-images.githubusercontent.com/11154118/45662527-dfaab400-bb34-11e8-98c8-c06ac5c0b08a.png) Tree-SHA512: 8b535427d26d3f8e61081f50e4773bd25656be042d378fd34cf647e9a0065cb4dfb67a8ab9fb4fbf5f196390df8cb983ebf2f0fa8a6503b7c046c56bec87ba72

Sjors · 2019-02-03T10:28:43Z

I'll try this again when I've cleanly rebased #14912, but in my current branch I'm not seeing this. cc @achow101

Summary: bitcoin/bitcoin@fe1ff50 --- Partial backport of Core [[bitcoin/bitcoin#13966 | PR13966]] Test Plan: ninja check ./src/qt/bitcoind -testnet create a watch-only wallet, import some random address, see that there is no spendable label Reviewers: #bitcoin_abc, jasonbcox, deadalnix Reviewed By: #bitcoin_abc, jasonbcox, deadalnix Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7300

Summary: bitcoin/bitcoin@82d6c5a --- Depends on D7300 Concludes backport of Core [[bitcoin/bitcoin#13966 | PR13966]] Test Plan: ninja check ./src/qt/bitcoin-qt -testnet open a watch-only wallet. see that the HD icon has been replaced by The Eye Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7301

DrahtBot mentioned this pull request Aug 14, 2018

Use new Qt5 connect syntax #13529

Merged

laanwj added GUI Wallet labels Aug 14, 2018

ken2812221 changed the title ~~gui: Hide spendable label if priveate key is disabled~~ gui: Hide spendable label if private key is disabled Aug 15, 2018

promag reviewed Aug 15, 2018

View reviewed changes

ken2812221 changed the title ~~gui: Hide spendable label if private key is disabled~~ gui: When private key is disabled, only show watch-only balance Aug 16, 2018

DrahtBot added the Needs rebase label Aug 21, 2018

DrahtBot removed the Needs rebase label Aug 22, 2018

laanwj reviewed Aug 27, 2018

View reviewed changes

Hide spendable label if priveate key is disabled

fe1ff50

Sjors mentioned this pull request Sep 13, 2018

Hardware wallet support #14145

Closed

gui: Show watch-only eye instead of HD disabled

82d6c5a

Sjors approved these changes Sep 20, 2018

View reviewed changes

src/qt/bitcoingui.h

@@ -215,7 +215,7 @@ public Q_SLOTS:

@param[in] status current hd enabled status

Copy link

Member

Sjors Sep 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: missing param comment

meshcollider reviewed Nov 10, 2018

View reviewed changes

laanwj merged commit 82d6c5a into bitcoin:master Dec 1, 2018

ken2812221 deleted the ui-disable-privkey branch December 1, 2018 11:37

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

		@@ -215,7 +215,7 @@ public Q_SLOTS:
		@param[in] status current hd enabled status

Conversation

ken2812221 commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 15, 2018

Uh oh!

laanwj commented Aug 22, 2018

Uh oh!

jonasschnelli commented Aug 22, 2018

Uh oh!

laanwj Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

ken2812221 Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 2, 2018

Uh oh!

jonasschnelli commented Sep 7, 2018

Uh oh!

Sjors commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Sep 14, 2018

Uh oh!

Sjors commented Sep 15, 2018

Uh oh!

ken2812221 commented Sep 18, 2018

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Sep 20, 2018

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider Nov 10, 2018

Choose a reason for hiding this comment

Uh oh!

sipa commented Nov 11, 2018

Uh oh!

laanwj commented Dec 1, 2018

Uh oh!

Sjors commented Feb 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ken2812221 commented Aug 14, 2018 •

edited

Loading

DrahtBot commented Aug 14, 2018 •

edited

Loading

promag Aug 15, 2018 •

edited

Loading

Sjors commented Sep 7, 2018 •

edited

Loading