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

pull Saibato wants to merge 1 commits into bitcoin-core:master from Saibato:change-hint-text-balance changing 1 files +3 βˆ’0
  1. Saibato commented at 3:48 pm on July 19, 2020: contributor

    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.

  2. Saibato commented at 3:49 pm on July 19, 2020: contributor

    Untitled

    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.

  3. michaelfolkson commented at 6:56 pm on July 19, 2020: contributor
    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.
  4. Saibato commented at 7:57 pm on July 19, 2020: contributor

    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?

  5. michaelfolkson commented at 8:28 pm on July 19, 2020: contributor
    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.
  6. Saibato commented at 8:31 pm on July 19, 2020: contributor

    Your … is a synonym for whoever hmm…

    Prob by this Gavin was tricked?

  7. maflcko commented at 6:05 am on July 20, 2020: contributor
    Unrelated, but the wallet generally considers unconfirmed change as spendable as well
  8. Saibato commented at 7:37 am on July 20, 2020: contributor

    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,

  9. Saibato commented at 9:40 am on July 20, 2020: contributor
    @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.
  10. maflcko commented at 9:50 am on July 20, 2020: contributor
    There’d be #1 for example
  11. Saibato commented at 10:10 am on July 20, 2020: contributor

    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?

  12. Saibato force-pushed on Jul 23, 2020
  13. Saibato commented at 8:40 am on July 23, 2020: contributor

    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.

    old_balance

    After change;

    new_balance

  14. Saibato closed this on Jul 23, 2020

  15. Saibato reopened this on Jul 23, 2020

  16. Saibato force-pushed on Jul 23, 2020
  17. Saibato renamed this:
    change labelbalance hint to a more neutral language
    indicate to the user that the wallet shown is watch only.
    on Jul 23, 2020
  18. promag commented at 11:03 am on July 26, 2020: contributor
    We already have the watch only icon on the status bar, maybe it should be more visible?
  19. Saibato commented at 7:57 am on July 27, 2020: contributor

    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.

  20. luke-jr commented at 9:42 pm on July 31, 2020: member

    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?

  21. Saibato commented at 6:17 am on August 1, 2020: contributor
    @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
  22. Saibato commented at 8:45 am on August 1, 2020: contributor

    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?

  23. hebasto commented at 11:06 am on August 1, 2020: member
    @achow101 @meshcollider Mind looking into this PR?
  24. meshcollider commented at 11:03 am on August 10, 2020: contributor

    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.

  25. michaelfolkson commented at 12:08 pm on August 10, 2020: contributor
    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.
  26. Saibato force-pushed on Aug 21, 2020
  27. Saibato commented at 2:15 pm on August 21, 2020: contributor

    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 )

    GUI-wallet-1

    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

    GUI-wallet-2

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

    GUI-wallet-3

  28. kristapsk approved
  29. kristapsk commented at 4:43 am on November 6, 2020: contributor
    ACK 990c2cd6b8d1f0f39fb8804c065ed1c7243318be. 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.
  30. kristapsk commented at 4:45 am on November 6, 2020: contributor

    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.

  31. Saibato commented at 12:13 pm on November 11, 2020: contributor

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

  32. Saibato renamed this:
    indicate to the user that the wallet shown is watch only.
    indicate explicit to the user that the wallet balances shown is watch only.
    on Nov 11, 2020
  33. in src/qt/overviewpage.cpp:204 in 990c2cd6b8 outdated
    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));
    


    hebasto commented at 0:20 am on March 24, 2021:
    0            ui->labelSpendable->setText(tr("Watch-only:"));
    
  34. in src/qt/overviewpage.cpp:207 in 990c2cd6b8 outdated
    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));
    


    hebasto commented at 0:20 am on March 24, 2021:
    0            ui->labelBalance->setToolTip(tr("Your current balance in watch-only addresses"));
    

    Saibato commented at 1:37 pm on August 23, 2021:
    effect is the the same, but to translate her by tr makes sense since we have that in the object, and fever code is better code ;-)
  35. hebasto dismissed
  36. hebasto added the label Design on Mar 24, 2021
  37. hebasto added the label Wallet on Apr 19, 2021
  38. jarolrod commented at 1:45 am on July 6, 2021: member
    @Saibato are you going to revisit this?
  39. shaavan commented at 6:42 pm on August 2, 2021: contributor

    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. Screenshot from 2021-08-02 18-29-08

    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. Screenshot from 2021-08-02 18-24-38

  40. unknown approved
  41. Saibato commented at 1:47 pm on August 23, 2021: contributor

    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?

  42. Rspigler commented at 0:27 am on August 24, 2021: contributor

    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.

  43. Saibato commented at 10:16 am on August 24, 2021: contributor

    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.

  44. Rspigler commented at 7:12 am on August 25, 2021: contributor
    How would it label a multisignature wallet where less than m privkeys are on the device? It’s neither “watch only” or “spendable”?
  45. Saibato commented at 8:32 am on August 27, 2021: contributor

    How would it label a multisignature wallet where less than m privkeys 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?

  46. Rspigler commented at 7:37 am on August 31, 2021: contributor

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

  47. Bosch-0 commented at 6:35 am on September 1, 2021: 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.

  48. hebasto commented at 1:14 pm on September 14, 2021: member

    @Saibato

    Please squash your commits, and rebase the branch to avoid CI failures.

  49. hebasto removed the label Design on Sep 14, 2021
  50. hebasto added the label UX on Sep 14, 2021
  51. shaavan commented at 2:43 pm on September 17, 2021: contributor

    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

    PR Rebased on latest Master branch: rebased

    Just squash the commits, and it would be ready for an ACK

  52. hebasto added the label Waiting for author on Sep 29, 2021
  53. Saibato force-pushed on Oct 25, 2021
  54. Saibato commented at 5:58 am on October 25, 2021: contributor

    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.

  55. kristapsk commented at 9:21 am on October 25, 2021: contributor

    How would it label a multisignature wallet where less than m privkeys 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”?

  56. shaavan commented at 12:46 pm on October 25, 2021: contributor

    Changes since my last review:

    1. Comments have now been squashed together.
    2. Reverted 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.

  57. Indicate the balance in keyless wallets is watch-only.
    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 :-)
    8f9fd44146
  58. Saibato force-pushed on Oct 25, 2021
  59. Saibato commented at 6:47 pm on October 25, 2021: contributor

    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?

  60. hebasto removed the label Waiting for author on Oct 27, 2021
  61. DrahtBot commented at 10:52 am on November 9, 2022: contributor

    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.

  62. maflcko cross-referenced this on Nov 9, 2022 from issue Add Stale Bot configuration by aureleoules
  63. hebasto requested review from hebasto on Dec 6, 2022
  64. hebasto commented at 7:00 pm on December 6, 2022: member
    @achow101 Could you be interested in reviewing this PR?
  65. hebasto requested review from achow101 on Dec 6, 2022
  66. DrahtBot commented at 2:01 am on June 4, 2023: contributor

    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.

  67. DrahtBot commented at 12:55 pm on June 4, 2023: contributor

    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.

  68. achow101 commented at 4:19 pm on June 4, 2023: member

    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.

  69. DrahtBot added the label CI failed on Sep 3, 2023
  70. DrahtBot removed the label CI failed on Sep 4, 2023
  71. hebasto commented at 5:38 pm on September 22, 2023: member

    @Saibato

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

  72. in src/qt/overviewpage.cpp:204 in 8f9fd44146
    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:"));
    


    pablomartin4btc commented at 7:47 pm on October 6, 2023:
    0            ui->labelSpendable->setText(tr("Watch-only:"));
    
  73. in src/qt/overviewpage.cpp:207 in 8f9fd44146
    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"));
    


    pablomartin4btc commented at 7:48 pm on October 6, 2023:
    0            ui->labelBalance->setToolTip(tr("Your current balance in watch-only addresses"));
    
  74. pablomartin4btc commented at 8:17 pm on October 6, 2023: contributor

    tACK 8f9fd44146c3a0512dd9da657aeedb4339e050d1

    Current change doesn’t display the desirable goal:

    image

    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.
    
  75. DrahtBot requested review from kristapsk on Oct 6, 2023
  76. DrahtBot requested review from shaavan on Oct 6, 2023
  77. DrahtBot removed review request from kristapsk on Oct 6, 2023
  78. DrahtBot removed review request from shaavan on Oct 6, 2023
  79. DrahtBot requested review from kristapsk on Oct 6, 2023
  80. DrahtBot requested review from shaavan on Oct 6, 2023
  81. DrahtBot added the label CI failed on Oct 18, 2023
  82. DrahtBot cross-referenced this on Oct 24, 2023 from issue Remove the legacy wallet and BDB dependency by achow101
  83. hebasto commented at 4:53 pm on February 7, 2024: member
    Closing due to lack of support from the author.
  84. DrahtBot removed review request from kristapsk on Feb 7, 2024
  85. DrahtBot removed review request from shaavan on Feb 7, 2024
  86. DrahtBot requested review from shaavan on Feb 7, 2024
  87. DrahtBot requested review from kristapsk on Feb 7, 2024
  88. hebasto closed this on Feb 7, 2024

  89. hebasto added the label Up for grabs on Feb 7, 2024
  90. hernanmarino cross-referenced this on Feb 9, 2024 from issue Correct tooltip wording for watch-only wallets by hernanmarino
  91. hernanmarino commented at 4:44 am on February 9, 2024: contributor
    Hi, if no one disagrees, I am taking over the development of this functionality. Discussion can continue here: #792 Thanks @Saibato for the work
  92. hebasto removed the label Up for grabs on Feb 9, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 20:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me