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: When private key is disabled, only show watch-only balance #13966

Merged
merged 2 commits into from Dec 1, 2018

Conversation

@ken2812221
Copy link
Contributor

@ken2812221 ken2812221 commented Aug 14, 2018

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

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Aug 14, 2018

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

Conflicts

No conflicts as of last run.

@ken2812221 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
Copy link
Member

@promag promag left a comment

Concept ACK. Could add before and after shots.

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

@promag promag Aug 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());
});

@promag
Copy link
Member

@promag promag commented Aug 15, 2018

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

@ken2812221 ken2812221 force-pushed the ui-disable-privkey branch 2 times, most recently from a38df33 to b6d6d90 Aug 16, 2018
@ken2812221 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
@laanwj
Copy link
Member

@laanwj laanwj commented Aug 22, 2018

Concept ACK

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Aug 22, 2018

Nice!
Concept ACK

ui->labelUnconfirmed->setText(BitcoinUnits::formatWithUnit(unit, balances.unconfirmed_watch_only_balance, false, BitcoinUnits::separatorAlways));
ui->labelImmature->setText(BitcoinUnits::formatWithUnit(unit, balances.immature_watch_only_balance, false, BitcoinUnits::separatorAlways));
ui->labelTotal->setText(BitcoinUnits::formatWithUnit(unit, balances.watch_only_balance + balances.unconfirmed_watch_only_balance + balances.immature_watch_only_balance, false, BitcoinUnits::separatorAlways));
}
Copy link
Member

@laanwj laanwj Aug 27, 2018

code style nit:
} else {

Copy link
Contributor Author

@ken2812221 ken2812221 Aug 27, 2018

fixed

@ken2812221 ken2812221 force-pushed the ui-disable-privkey branch from f64b57c to a74cc54 Aug 27, 2018
@ken2812221 ken2812221 force-pushed the ui-disable-privkey branch from a74cc54 to fe1ff50 Aug 27, 2018
@fanquake
Copy link
Member

@fanquake fanquake commented Sep 2, 2018

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

regular-wallet

watch-only

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Sep 7, 2018

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

@Sjors
Copy link
Member

@Sjors Sjors commented Sep 7, 2018

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?

@Sjors Sjors mentioned this pull request Sep 13, 2018
@instagibbs
Copy link
Member

@instagibbs instagibbs commented Sep 14, 2018

@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
Copy link
Member

@Sjors Sjors commented Sep 15, 2018

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 ken2812221 force-pushed the ui-disable-privkey branch from a4f3084 to 82d6c5a Sep 18, 2018
@ken2812221
Copy link
Contributor Author

@ken2812221 ken2812221 commented Sep 18, 2018

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

Sjors
Sjors approved these changes Sep 20, 2018
Copy link
Member

@Sjors Sjors left a comment

tACK 82d6c5a on macOS 10.13.6

Before:
before

After:
after

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

@Sjors Sjors Sep 20, 2018

Nit: missing param comment

Copy link
Member

@meshcollider meshcollider left a comment

Tested ACK 82d6c5a

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

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>"));
Copy link
Member

@meshcollider meshcollider Nov 10, 2018

key -> keys?

@sipa
Copy link
Member

@sipa sipa commented Nov 11, 2018

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
Copy link
Member

@laanwj laanwj commented Dec 1, 2018

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.

@laanwj laanwj merged commit 82d6c5a into bitcoin:master Dec 1, 2018
2 checks passed
laanwj added a commit that referenced this issue Dec 1, 2018
… 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
@ken2812221 ken2812221 deleted the ui-disable-privkey branch Dec 1, 2018
@Sjors
Copy link
Member

@Sjors Sjors commented Feb 3, 2019

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

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Sep 2, 2020
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
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Sep 2, 2020
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
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

10 participants