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.
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:
0Your 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.
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?
Your … is a synonym for whoever hmm…
Prob by this Gavin was tricked?
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,
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?
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;
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.
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?
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?
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.
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
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.
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.
179@@ -180,7 +180,10 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
180 m_balances = balances;
181 if (walletModel->wallet().isLegacy()) {
182 if (walletModel->wallet().privateKeysDisabled()) {
183+ ui->labelSpendable->setText(QApplication::translate("OverviewPage", "Watch-only:", Q_NULLPTR));
0 ui->labelSpendable->setText(tr("Watch-only:"));
179@@ -180,7 +180,10 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
180 m_balances = balances;
181 if (walletModel->wallet().isLegacy()) {
182 if (walletModel->wallet().privateKeysDisabled()) {
183+ ui->labelSpendable->setText(QApplication::translate("OverviewPage", "Watch-only:", Q_NULLPTR));
184+ ui->labelSpendable->setVisible(true);
185 ui->labelBalance->setText(BitcoinUnits::formatWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy));
186+ ui->labelBalance->setToolTip(QApplication::translate("OverviewPage", "Your current balance in watch-only addresses", Q_NULLPTR));
0 ui->labelBalance->setToolTip(tr("Your current balance in watch-only addresses"));
tACK 990c2cd6b8d1f0f39fb8804c065ed1c7243318be
This PR is trying to update the interface around a “Watch-Only” wallet to make it easier for a user to identify the type of wallet, by clearly displaying the wallet type over the balance of that wallet, and also by mentioning it in the ToolTip text for the balance. It works exactly as the op mentioned in the PR.
I think this PR is an improvement, and I agree with adding a label of “Watch-Only” over the balance, as it would suggest to the user that the balance is Watch-Only. But I think editing the ToolTip message is quite over-the-top and is not needed, because the message is properly conveyed to the user that the wallet is “Watch-Only” by the wallet heading. And I think the tooltip message should remain unchanged, to maintain uniformity over all types of wallets.
Testing on PR Branch:
I have tested the original PR over the testnet network. It works correctly and as intended. I am attaching a screenshot here of a “Watch-Only” Wallet and the message displayed when the cursor hovers on the balance of the wallet.
Testing Rebased on Master:
I have rebased this branch over the latest state of the master branch of bitcoin-GUI. Here is the link to the rebased branch. The reason that I tested it rebased is that the PR branch is very old and I wanted to ensure that functionality remains the same if it is merged over the master. Additionally, rebasing on master lets me test the PR on the signet network. The PR seems to work fine on the signet network on the rebased branch. Here is a screenshot of it.
thx, for testing this.
I merged the suggestion from @hebasto to use the Qt r() function here for auto translate, since we have tr in the object and so we have shorter code. ;-)
The effect is the same, and you might want to revisit and confirm that this has still the desired effect
While revisiting this PR I found a minor bug when displaying icons if the wallets are reloaded, #407 you might want to test that too?
With descriptor wallets, users aren’t going to have mixed wallets anymore. So I don’t know if this is really necessary. The balance should either be all watch only, or all single key, or all multi-signature, etc.
There probably could be some GUI improvements, but I don’t think this is complete/or going in the right direction.
With descriptor wallets, users aren’t going to have mixed wallets anymore. So I don’t know if this is really necessary. The balance should either be all watch only, or all single key, or all multi-signature, etc.
There probably could be some GUI improvements, but I don’t think this is complete/or going in the right direction.
this is for sure an edge case of bare keyless and not mixed wallets. afaics mixed wallets are handled already correct. I would guess, core will for ever have wallets without keys, and some bold obvious distinction in UI to the user that those spendable coins are in a watch-only wallets ( maybe ancient ) without keys, is in my view good UX. Afaics that is what this PR intends and archives.
m privkeys are on the device? It’s neither “watch only” or “spendable”?
How would it label a multisignature wallet where less than
mprivkeys are on the device? It’s neither “watch only” or “spendable”?
one could argue not your keys, not your coins, so if you need m and have only m-1 that bare wallet at the moment you watch the balance is still “watch-only” like the other ones of whom we do not call his name?
After further thought, I like the current change (the “Watch-Only” label). I just don’t know what the solution is for multisignature wallets.
There’s lots of different ways for wallets to be setup and I think that complicates this. Don’t know if @Bosch-0 has thought about this. For example:
You could have a cold storage situation with a full node being in watch only mode. Then n wallets each with a privkey. The GUI for the full node vs for the wallets should probably represent the balance in different ways? (The wallets have one “piece of the puzzle” to spend, the node has none).
I think Specter and Nunchuk both go about solving this issue quite eloquently. I think these type of products are what the GUI should strive to be, hopefully a lot easier with the work being done with QML GUI.
You can actually go through this UX flow using n local core wallets in Specter.
Changes since my last review:
QApplication::translate has been replaced with tr().I am adding new screenshots to emphasize no change in perceived behavior.
PR:
PR Rebased on latest Master branch:
Just squash the commits, and it would be ready for an ACK
Just squash the commits, and it would be ready for an ACK
Sorry, for the lag, covid takes it tolls, i had a lot of trouble, so many relatives died and that leaves you alone with lot of things one has to manage, … rebased it now.
How would it label a multisignature wallet where less than
mprivkeys are on the device? It’s neither “watch only” or “spendable”?
Maybe solution is to just figure out another label for such wallets and display it instead of “watch only”?
Changes since my last review:
tr() back to QApplication::translate.I think the second change was unintentional because the above discussion concluded tr() to be a better choice.
Sorry, for the lag, covid takes it tolls, i had a lot of trouble, so many relatives died and that leaves you alone with lot of things one has to manage,
I know it’s hard. Covid has taken away many of our loved ones. I am sorry for your loss. Take your time, and take care.
Since we have now different wallet types, there was no
indication what kind of wallet is visible. And the hint was
such that a user might have thought even balances in watch only
wallets where spendable by the wallet shown in overview.
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 pubkeys addresses i imported with
importaddress warping into keys for those addresses,
At least for some short moment i felt mainnet btc rich :-)
I think the second change was unintentional because the above discussion concluded tr() to be a better choice.
yup hoops, i sure prefer that too, lost @hebasto ’s suggestion i merged sadly only in the upstream fixed that, now that it’s forced sqashed by me from downstream, i guess his commit credits are lost, i hope he can forgive me?
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.
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
No conflicts as of last run.
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.
Are you still working on this? If so, mind addressing @achow101’s #37 (comment) please?
200@@ -201,7 +201,10 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
201 m_balances = balances;
202 if (walletModel->wallet().isLegacy()) {
203 if (walletModel->wallet().privateKeysDisabled()) {
204+ ui->labelSpendable->setText(tr("OverviewPage", "Watch-only:"));
0 ui->labelSpendable->setText(tr("Watch-only:"));
200@@ -201,7 +201,10 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
201 m_balances = balances;
202 if (walletModel->wallet().isLegacy()) {
203 if (walletModel->wallet().privateKeysDisabled()) {
204+ ui->labelSpendable->setText(tr("OverviewPage", "Watch-only:"));
205+ ui->labelSpendable->setVisible(true);
206 ui->labelBalance->setText(BitcoinUnits::formatWithPrivacy(unit, balances.watch_only_balance, BitcoinUnits::SeparatorStyle::ALWAYS, m_privacy));
207+ ui->labelBalance->setToolTip(tr("OverviewPage", "Your current balance in watch-only addresses"));
0 ui->labelBalance->setToolTip(tr("Your current balance in watch-only addresses"));
tACK 8f9fd44146c3a0512dd9da657aeedb4339e050d1
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):
0gui: Show spendable balance for keyless watch-only
1
2Make labelSpendable visible for watch only wallets
3with their private keys disabled.
4Also set the labelBalance accordingly for the same
5condition.
