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

pull ken2812221 wants to merge 2 commits into bitcoin:master from ken2812221:ui-disable-privkey changing 3 files +25 −17
  1. 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.

    image

  2. DrahtBot commented at 2:34 PM on August 14, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. laanwj added the label GUI on Aug 14, 2018
  4. laanwj added the label Wallet on Aug 14, 2018
  5. 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
  6. 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

    connect(model, &WalletModel::notifyWatchonlyChanged, [this](bool showWatchOnly) {
    updateWalletLabels(showWatchOnly, !walletModel->privateKeysDisabled());
});
  7. promag commented at 11:02 PM on August 15, 2018: member

    Concept ACK. Could add before and after shots.

  8. 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?

  9. ken2812221 force-pushed on Aug 16, 2018
  10. ken2812221 force-pushed on Aug 16, 2018
  11. 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
  12. DrahtBot added the label Needs rebase on Aug 21, 2018
  13. laanwj commented at 7:48 AM on August 22, 2018: member

    Concept ACK

  14. jonasschnelli commented at 7:50 AM on August 22, 2018: contributor

    Nice! Concept ACK

  15. ken2812221 force-pushed on Aug 22, 2018
  16. DrahtBot removed the label Needs rebase on Aug 22, 2018
  17. 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

  18. ken2812221 force-pushed on Aug 27, 2018
  19. Hide spendable label if priveate key is disabled fe1ff5026b
  20. ken2812221 force-pushed on Aug 27, 2018
  21. 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.

    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

    <img width="872" alt="regular-wallet" src="https://user-images.githubusercontent.com/863730/44956157-0391b700-aef2-11e8-9b57-8ceacb0d60d1.png">

    <img width="874" alt="watch-only" src="https://user-images.githubusercontent.com/863730/44956159-068ca780-aef2-11e8-85ac-e04045c26744.png">

  22. jonasschnelli commented at 7:32 PM on September 7, 2018: contributor

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

  23. 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?

  24. 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.

  25. 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.

  26. gui: Show watch-only eye instead of HD disabled 82d6c5aad3
  27. ken2812221 force-pushed on Sep 18, 2018
  28. 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.

  29. 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

  30. Sjors approved
  31. Sjors commented at 3:21 PM on September 20, 2018: member

    tACK 82d6c5a on macOS 10.13.6

    Before: <img width="841" alt="before" src="https://user-images.githubusercontent.com/10217/45827242-ecbed300-bced-11e8-9c30-29e32e478f02.png">

    After: <img width="841" alt="after" src="https://user-images.githubusercontent.com/10217/45827789-1f1d0000-bcef-11e8-9bc7-72359e07bb2f.png">

  32. 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?

  33. 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?

    image

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

  34. 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.

  35. 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.

  36. laanwj merged this on Dec 1, 2018
  37. laanwj closed this on Dec 1, 2018
  38. laanwj referenced this in commit ed12fd83ca on Dec 1, 2018
  39. ken2812221 deleted the branch on Dec 1, 2018
  40. 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

  41. jasonbcox referenced this in commit b69646f089 on Sep 2, 2020
  42. jasonbcox referenced this in commit fb16bf2638 on Sep 2, 2020
  43. MarcoFalke locked this on Dec 16, 2021
Contributors
ken2812221DrahtBotpromaglaanwjjonasschnellifanquakeSjorsinstagibbsmeshcollidersipa
Labels
GUIWallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 21:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me