indicate explicit to the user that the wallet balances shown is watch only. #37

Saibato · 2020-07-19T15:48:31Z

This little change got me back to earth when i realized that the
balance hint Your current spendable balance was just a label and
not the result of of some random pubkey addresses i imported with
importaddress warping into keys for those addresses,

At least for some short moment i felt mainnet btc rich, but this change
in hint wording should be done.

edit@saibato Todo: before merge run translation Makefile after we agree in wording.

Saibato · 2020-07-19T15:49:08Z

edit@saibato The prefix word "Your " implies that the user can spend this confirmed and not pending balance even though he has or had never the keys,
At multiple spaces in the ui we hint that some utxos are only watched.
So this PR changes this hint also at prominent space too, to:

Your current wallet balance`

We could sure even add more here and make a longer text that hints the user to the correct
interpretation of this balance shown, discussion welcome.

michaelfolkson · 2020-07-19T18:56:07Z

My understanding is that the language "spendable balance" is deliberate as it does not include unconfirmed transactions which are considered not yet "spendable". If my understanding is correct it is a Concept NACK from me.

Saibato · 2020-07-19T19:57:13Z

My understanding is that the language "spendable balance" is deliberate as it does not include unconfirmed transactions

@michaelfolkson hmm,,. this is not about unconfirmed transactions, they are confirmed and the balance is in sum correct.
The wallet has no private keys to this balance, do you think they are "spendable" by this wallet and me?

michaelfolkson · 2020-07-19T20:28:15Z

Spendable as in whoever has the keys can spend them. Not yet spendable would be funds in unconfirmed transactions. The language isn't referring to who has the keys, it is referring to whether the transactions are confirmed or not. In the example image you posted all your funds are spendable.

Saibato · 2020-07-19T20:31:18Z

Your ... is a synonym for whoever hmm...

Prob by this Gavin was tricked?

maflcko · 2020-07-20T06:05:33Z

Unrelated, but the wallet generally considers unconfirmed change as spendable as well

Saibato · 2020-07-20T07:37:28Z

Unrelated, but the wallet generally considers unconfirmed change as spendable as well

Yup, when you begin to see what we do with user eye;s, there is some work ahead.

And this is probably anyway an edge case and this wrong hint is somehow a minor bug, but since we allow watch only wallets, i still would like to have the ui state in any from that those balances are not spendable at all by the wallet that is currently visible to the user.

In general we clearly show that watch-only utxos are unspendable and have split row to diff , but not in this case. An unaware user might think since in this case the ui show's up almost like the normal full spendable wallet. And i must admit when testing back and forth with wallets, for a short moment, even i thought wow they are spendbable, only after inspecting deeper i saw that this was only watched utxos,

Saibato · 2020-07-20T09:40:41Z

@MarcoFalke Btw. since rona has gave me lots of free timeslots, if you know others thing to be addressed, that you have in your backlog and think, would be nice to have, pls ping.

maflcko · 2020-07-20T09:50:00Z

There'd be #1 for example

Saibato · 2020-07-20T10:10:51Z

There'd be #1 for example

thx, for hint,
I only have looked in the codebase for two weeks or so i am not so deep in bitcoin and c++ voodoo, aside from net, tor or ip things or a bit gui. But I have a quite remarkable learning by doing pace, if i must or have fun. But i also want not do push others, that can not keep up with such a pace hide away or think, why can she this in minutes? I am quite different from a healthy human.
So ping @sehyunc have you begun fix #1?

Saibato · 2020-07-23T08:40:24Z

fixup-Update;
Since we now have different wallet types and one ui can show different wallets, it;s still my view that we should indicate this to the user.

I considered @michaelfolkson argument and he is right that those confirmed balances are spendable, if you have the keys, what we know from the wallet, if the keys are there, but even
for a watch only wallet, the user might have the keys,

So this fixup changes in addition also the balancetext label
to indicated that the wallet displayed has no keys and that in general we could only say that those shown wallet balances are current confirmed.

This is the current way we display watch only wallets, they look the same as a normal spendable full key wallet, although the wallet has no keys and we might not had them also never.

After change;

promag

We already have the watch only icon on the status bar, maybe it should be more visible?

Saibato · 2020-07-27T07:57:36Z

We already have the watch only icon on the status bar, maybe it should be more visible?

@promag thx, whatever helps, my point is that up till now this design is not optimal and serve;s not perfect our high standards .

An unaware user could even have there keys stripped from there wallet by i.e. some rpc remote or local bytecode insert exploit or simple fileops filesytem wallet.dat exchange.
And be visual somewhat unaware of that his keys are gone, until he tries to spend in the other window,

And even long term experts could in my view, be somewhat astound, if they switch to newer node software. Where keyless and multiple wallets are possible.

luke-jr · 2020-07-31T21:42:54Z

I can't imagine a case where it would make sense for malware to turn a read/write wallet into watch-only. Typically, they would just spend the coins to a key they control exclusively. Even if they wanted to pull a trick like this, it would be comparable to just corrupt the private keys...

Also, aren't watch-only funds spendable with stuff like hardware wallets?

Saibato · 2020-08-01T06:17:45Z

@luke-jr I know, its not easy to follow or see the links of sense i would like to have changed in core and some of my PR are only hints and I never cared honestly, if any is merged, i done my footprint early on, but be assured, if you at some point will see the 3 stage picture that i see water clear, you will think omg, the heist of the century. imho

Saibato · 2020-08-01T08:45:19Z

Also, aren't watch-only funds spendable with stuff like hardware wallets?

Sure but i would say, that's technically an other wallet?

And btw, I will not push script hint pr's , but honestly why even doubt, that what i outlined
here makes obviously some sense, if you compare the overall watch-only wallet layout and the standard HD layout?

And have you hovered about the little disabled hd icon on the bottom, if you tell me
great choice of words, turing-test?

hebasto · 2020-08-01T11:06:32Z

@achow101 @meshcollider Mind looking into this PR?

meshcollider · 2020-08-10T11:03:37Z

I'm ~0 on this, as promag said, there is already an indicator. And spendable isn't the same as confirmed so I'm not convinced on that change either,

Happy if other people want this though.

michaelfolkson · 2020-08-10T12:08:03Z

Spendable takes into account confirmed/unconfirmed, whether wallet contains all the key(s) needed to spend, whether sufficient time has passed for coinbase output to be spent etc. So as @meshcollider says spendable meets more conditions than just confirmed. As a result it is a Concept NACK on this change. I do wonder though whether it should be made clearer to the GUI user how "spendable" is defined either in the UI or in documentation.

Saibato · 2020-08-21T14:15:51Z

Since the bug pun line confirmed did not resonated to action by review, here a modified version that address solely the poor hint and indication that a wallet is pure watch-only.

Also we can see here that blanked out HD symbol in status line is hardly to distinguish from a blanked eye.

With the description header and the hint, that distinction should be sufficient.

(change: add a headerline Watch-only in the watch only case and applied the same hint from watch-only address from the mixed cases )

Here for reference how the other cases will still look like :

(unchnaged/ by this PR same display as in old version )
Picture of a standard hd wallet with no imported address

(unchanged/ by this PR same display as in old version)
Picture of a non hd wallet with imported address

kristapsk

ACK 990c2cd. I had similar idea when switching between spendable and watch-only wallets on a testnet few days ago and got confused for a brief moment.

kristapsk · 2020-11-06T04:45:24Z

We already have the watch only icon on the status bar, maybe it should be more visible?

Yes, I didn't notice it before this comment actually. Also, with wallets having both spendable and watch-only addresses you have "Watch-only" / "Spendable" above amounts, so one might expect the same behaviour when having loaded wallet with only watch-only addresses.

Saibato · 2020-11-11T12:13:18Z

@kristapsk

Yes, I didn't notice it before this comment actually. Also, with wallets having both spendable and watch-only addresses you have "Watch-only" / "Spendable" above amounts, so one might expect the same behaviour when having loaded wallet with only watch-only addresses.

exactly, if I interpret correctly the case u describe?
That's the reason the change is in its last version only for the pure watch only wallets
Since as @michaelfolkson pointed out, to change the hint in general is a bit overdone.

Mixed stay untouched since they already display in two columns with a hinting header and this seams to be fine AFAICS.

hebasto · 2022-12-06T19:00:01Z

@achow101 Could you be interested in reviewing this PR?

DrahtBot · 2023-06-04T02:01:10Z

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

resolved

DrahtBot · 2023-06-04T12:55:40Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc
Concept NACK	michaelfolkson
Concept ACK	shaavan, achow101
Stale ACK	kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

achow101 · 2023-06-04T16:19:31Z

Concept ACK

I don't think this should be limited to just legacy wallets - descriptor wallets can have private keys disabled as well. This should probably apply to all wallets that have private keys disabled, as the the watch-only icon does.

hebasto · 2023-09-22T17:38:54Z

@Saibato

Are you still working on this? If so, mind addressing @achow101's #37 (comment) please?

pablomartin4btc

tACK 8f9fd44

Current change doesn't display the desirable goal:

I agree with @achow101, the change should be moved outside the legacy logic.

The commit title/ message could start with "gui: ", it shouldn't finish with a dot I think, and perhaps the commit body could be rephrased a bit to explain more direct the purpose of this change, eg (please check the commit guidelines):

gui: Show spendable balance for keyless watch-only

Make labelSpendable visible for watch only wallets
with their private keys disabled.
Also set the labelBalance accordingly for the same
condition.

pablomartin4btc · 2023-10-06T19:47:53Z

src/qt/overviewpage.cpp

@@ -201,7 +201,10 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
    m_balances = balances;
    if (walletModel->wallet().isLegacy()) {
        if (walletModel->wallet().privateKeysDisabled()) {
+            ui->labelSpendable->setText(tr("OverviewPage", "Watch-only:"));


Suggested change

ui->labelSpendable->setText(tr("OverviewPage", "Watch-only:"));

ui->labelSpendable->setText(tr("Watch-only:"));

pablomartin4btc · 2023-10-06T19:48:18Z

src/qt/overviewpage.cpp

            ui->labelBalance->setText(BitcoinUnits::formatWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy));
+            ui->labelBalance->setToolTip(tr("OverviewPage", "Your current balance in watch-only addresses"));


Suggested change

ui->labelBalance->setToolTip(tr("OverviewPage", "Your current balance in watch-only addresses"));

ui->labelBalance->setToolTip(tr("Your current balance in watch-only addresses"));

hebasto · 2024-02-07T16:53:27Z

Closing due to lack of support from the author.

hernanmarino · 2024-02-09T04:44:54Z

Hi, if no one disagrees, I am taking over the development of this functionality. Discussion can continue here: #792 Thanks @Saibato for the work

Saibato force-pushed the change-hint-text-balance branch from 6db25ea to 90ac7ff Compare July 23, 2020 08:11

Saibato closed this Jul 23, 2020

Saibato reopened this Jul 23, 2020

Saibato force-pushed the change-hint-text-balance branch from 90ac7ff to d0d83d2 Compare July 23, 2020 08:45

Saibato changed the title ~~change labelbalance hint to a more neutral language~~ indicate to the user that the wallet shown is watch only. Jul 23, 2020

promag reviewed Jul 26, 2020

View reviewed changes

Saibato force-pushed the change-hint-text-balance branch from d0d83d2 to 990c2cd Compare August 21, 2020 13:50

kristapsk approved these changes Nov 6, 2020

View reviewed changes

Saibato changed the title ~~indicate to the user that the wallet shown is watch only.~~ indicate explicit to the user that the wallet balances shown is watch only. Nov 11, 2020

maflcko mentioned this pull request Nov 9, 2022

Add Stale Bot configuration bitcoin/bitcoin#26300

Closed

hebasto self-requested a review December 6, 2022 18:53

hebasto requested a review from achow101 December 6, 2022 19:00

DrahtBot added CI failed and removed CI failed labels Sep 3, 2023

pablomartin4btc reviewed Oct 6, 2023

View reviewed changes

DrahtBot requested review from kristapsk and shaavan and removed request for kristapsk and shaavan October 6, 2023 20:17

DrahtBot added the CI failed label Oct 18, 2023

DrahtBot mentioned this pull request Oct 24, 2023

Remove the legacy wallet and BDB dependency bitcoin/bitcoin#28710

Draft

DrahtBot requested review from shaavan and kristapsk and removed request for kristapsk and shaavan February 7, 2024 16:53

hebasto closed this Feb 7, 2024

hebasto added the Up for grabs label Feb 7, 2024

hernanmarino mentioned this pull request Feb 9, 2024

Correct tooltip wording for watch-only wallets #792

Closed

hebasto removed the Up for grabs label Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

indicate explicit to the user that the wallet balances shown is watch only. #37

indicate explicit to the user that the wallet balances shown is watch only. #37

Saibato commented Jul 19, 2020 •

edited

Loading

Saibato commented Jul 19, 2020 •

edited

Loading

michaelfolkson commented Jul 19, 2020

Saibato commented Jul 19, 2020

michaelfolkson commented Jul 19, 2020

Saibato commented Jul 19, 2020 •

edited

Loading

maflcko commented Jul 20, 2020

Saibato commented Jul 20, 2020

Saibato commented Jul 20, 2020

maflcko commented Jul 20, 2020

Saibato commented Jul 20, 2020

Saibato commented Jul 23, 2020 •

edited

Loading

promag left a comment

Saibato commented Jul 27, 2020 •

edited

Loading

luke-jr commented Jul 31, 2020

Saibato commented Aug 1, 2020

Saibato commented Aug 1, 2020 •

edited

Loading

hebasto commented Aug 1, 2020

meshcollider commented Aug 10, 2020 •

edited

Loading

michaelfolkson commented Aug 10, 2020

Saibato commented Aug 21, 2020 •

edited

Loading

kristapsk left a comment

kristapsk commented Nov 6, 2020

Saibato commented Nov 11, 2020

hebasto commented Dec 6, 2022

DrahtBot commented Jun 4, 2023

DrahtBot commented Jun 4, 2023 •

edited

Loading

achow101 commented Jun 4, 2023

hebasto commented Sep 22, 2023

pablomartin4btc left a comment

pablomartin4btc Oct 6, 2023

pablomartin4btc Oct 6, 2023

hebasto commented Feb 7, 2024

hernanmarino commented Feb 9, 2024

	ui->labelSpendable->setText(tr("OverviewPage", "Watch-only:"));
	ui->labelSpendable->setText(tr("Watch-only:"));

		ui->labelBalance->setText(BitcoinUnits::formatWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy));
		ui->labelBalance->setToolTip(tr("OverviewPage", "Your current balance in watch-only addresses"));

	ui->labelBalance->setToolTip(tr("OverviewPage", "Your current balance in watch-only addresses"));
	ui->labelBalance->setToolTip(tr("Your current balance in watch-only addresses"));

indicate explicit to the user that the wallet balances shown is watch only. #37

indicate explicit to the user that the wallet balances shown is watch only. #37

Conversation

Saibato commented Jul 19, 2020 • edited Loading

Saibato commented Jul 19, 2020 • edited Loading

michaelfolkson commented Jul 19, 2020

Saibato commented Jul 19, 2020

michaelfolkson commented Jul 19, 2020

Saibato commented Jul 19, 2020 • edited Loading

maflcko commented Jul 20, 2020

Saibato commented Jul 20, 2020

Saibato commented Jul 20, 2020

maflcko commented Jul 20, 2020

Saibato commented Jul 20, 2020

Saibato commented Jul 23, 2020 • edited Loading

promag left a comment

Choose a reason for hiding this comment

Saibato commented Jul 27, 2020 • edited Loading

luke-jr commented Jul 31, 2020

Saibato commented Aug 1, 2020

Saibato commented Aug 1, 2020 • edited Loading

hebasto commented Aug 1, 2020

meshcollider commented Aug 10, 2020 • edited Loading

michaelfolkson commented Aug 10, 2020

Saibato commented Aug 21, 2020 • edited Loading

kristapsk left a comment

Choose a reason for hiding this comment

kristapsk commented Nov 6, 2020

Saibato commented Nov 11, 2020

hebasto commented Dec 6, 2022

DrahtBot commented Jun 4, 2023

DrahtBot commented Jun 4, 2023 • edited Loading

Reviews

Conflicts

achow101 commented Jun 4, 2023

hebasto commented Sep 22, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

pablomartin4btc Oct 6, 2023

Choose a reason for hiding this comment

pablomartin4btc Oct 6, 2023

Choose a reason for hiding this comment

hebasto commented Feb 7, 2024

hernanmarino commented Feb 9, 2024

Saibato commented Jul 19, 2020 •

edited

Loading

Saibato commented Jul 19, 2020 •

edited

Loading

Saibato commented Jul 19, 2020 •

edited

Loading

Saibato commented Jul 23, 2020 •

edited

Loading

Saibato commented Jul 27, 2020 •

edited

Loading

Saibato commented Aug 1, 2020 •

edited

Loading

meshcollider commented Aug 10, 2020 •

edited

Loading

Saibato commented Aug 21, 2020 •

edited

Loading

DrahtBot commented Jun 4, 2023 •

edited

Loading