gui: When private key is disabled, only show watch-only balance

ken2812221 commented at 1:37 pm on August 14, 2018: contributor

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 commented at 2:34 pm on August 14, 2018: member

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

Conflicts

No conflicts as of last run.

laanwj added the label GUI on Aug 14, 2018

laanwj added the label Wallet on Aug 14, 2018

ken2812221 renamed this:
~~gui: Hide spendable label if priveate key is disabled~~
gui: Hide spendable label if private key is disabled
on Aug 15, 2018

in src/qt/overviewpage.cpp:191 in 0d37e47f3f outdated

183@@ -184,13 +184,24 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
184 // show/hide watch-only labels
185 void OverviewPage::updateWatchOnlyLabels(bool showWatchOnly)

promag commented at 11:00 pm on August 15, 2018:

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

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

promag commented at 11:02 pm on August 15, 2018: member

Concept ACK. Could add before and after shots.

promag commented at 11:04 pm on August 15, 2018: member

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

ken2812221 force-pushed on Aug 16, 2018

ken2812221 renamed this:
~~gui: Hide spendable label if private key is disabled~~
gui: When private key is disabled, only show watch-only balance
on Aug 16, 2018

DrahtBot added the label Needs rebase on Aug 21, 2018

laanwj commented at 7:48 am on August 22, 2018: member

Concept ACK

jonasschnelli commented at 7:50 am on August 22, 2018: contributor

Nice! Concept ACK

ken2812221 force-pushed on Aug 22, 2018

DrahtBot removed the label Needs rebase on Aug 22, 2018

in src/qt/overviewpage.cpp:169 in f64b57cf9f outdated

173+    if (walletModel->privateKeysDisabled()) {
174+        ui->labelBalance->setText(BitcoinUnits::formatWithUnit(unit, balances.watch_only_balance, false, BitcoinUnits::separatorAlways));
175+        ui->labelUnconfirmed->setText(BitcoinUnits::formatWithUnit(unit, balances.unconfirmed_watch_only_balance, false, BitcoinUnits::separatorAlways));
176+        ui->labelImmature->setText(BitcoinUnits::formatWithUnit(unit, balances.immature_watch_only_balance, false, BitcoinUnits::separatorAlways));
177+        ui->labelTotal->setText(BitcoinUnits::formatWithUnit(unit, balances.watch_only_balance + balances.unconfirmed_watch_only_balance + balances.immature_watch_only_balance, false, BitcoinUnits::separatorAlways));
178+    }

laanwj commented at 11:01 am on August 27, 2018:

code style nit: } else {

ken2812221 commented at 4:57 pm on August 27, 2018:

fixed

ken2812221 force-pushed on Aug 27, 2018

Hide spendable label if priveate key is disabled fe1ff5026b

ken2812221 force-pushed on Aug 27, 2018

fanquake commented at 1:28 pm on September 2, 2018: member

Testing https://github.com/bitcoin/bitcoin/pull/13966/commits/fe1ff5026bace9b43ac01c14a26ed0dd75ccc6f7 using the following commands (in the Debug window given that we’re looking at a GUI change). Screenshots below, will review further shortly.

0src/qt/bitcoin-qt --testnet
1createwallet "regular-wallet" false
2createwallet "watch-only-wallet" true
3
4// import a random testnet address
5importaddress "some-address" "random-addr" false false

jonasschnelli commented at 7:32 pm on September 7, 2018: contributor

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

Sjors commented at 8:10 pm on September 7, 2018: member

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 commented at 7:52 pm on September 14, 2018: member

@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 commented at 11:58 am on September 15, 2018: member

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.

gui: Show watch-only eye instead of HD disabled 82d6c5aad3

ken2812221 force-pushed on Sep 18, 2018

ken2812221 commented at 3:24 am on September 18, 2018: contributor

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

in src/qt/bitcoingui.h:215 in 82d6c5aad3

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

Sjors commented at 3:18 pm on September 20, 2018:

Nit: missing param comment

Sjors approved

Sjors commented at 3:21 pm on September 20, 2018: member

tACK 82d6c5a on macOS 10.13.6

Before:

After:

in src/qt/bitcoingui.cpp:1072 in 82d6c5aad3

1070+void BitcoinGUI::setHDStatus(bool privkeyDisabled, int hdEnabled)
1071 {
1072-    labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(hdEnabled ? ":/icons/hd_enabled" : ":/icons/hd_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
1073-    labelWalletHDStatusIcon->setToolTip(hdEnabled ? tr("HD key generation is <b>enabled</b>") : tr("HD key generation is <b>disabled</b>"));
1074+    labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(privkeyDisabled ? ":/icons/eye" : hdEnabled ? ":/icons/hd_enabled" : ":/icons/hd_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
1075+    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>"));

meshcollider commented at 5:02 am on November 10, 2018:

key -> keys?

meshcollider commented at 11:25 am on November 10, 2018: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/13966/commits/82d6c5aad330bb06d0c918b4e9304c25ff2bdcc8

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

sipa commented at 3:23 am on November 11, 2018: member

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 commented at 11:20 am on December 1, 2018: member

utACK 82d6c5aad330bb06d0c918b4e9304c25ff2bdcc8

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.

laanwj merged this on Dec 1, 2018

laanwj closed this on Dec 1, 2018

laanwj referenced this in commit ed12fd83ca on Dec 1, 2018

ken2812221 deleted the branch on Dec 1, 2018

Sjors commented at 10:28 am on February 3, 2019: member

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

jasonbcox referenced this in commit b69646f089 on Sep 2, 2020

jasonbcox referenced this in commit fb16bf2638 on Sep 2, 2020

MarcoFalke locked this on Dec 16, 2021

gui: When private key is disabled, only show watch-only balance #13966

Conflicts