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] Spend cold-stake delegations #1391

Merged

Conversation

random-zebra
Copy link

Problem:
A few users have reported of having accidentally spent cold-stake delegations, after interacting with the GUI, in the following cases

  1. Sending a transaction
  2. Creating a MN controller with the masternode wizard
  3. Creating another delegation

Cause:
Cold stakes and delegations are picked up (owner-side) by default in AvailableCoins (in order to get the correct balances).
While in the RPC functions it is possible to use a boolean parameter fIncludeDelegations to decide whether or not to let the automatic selection algorithm include them (if needed), the GUI has no such option, thus, by default, doesn't skip them.

The user is presented with a warning before spending a cold-stake delegation, but some user doesn't pay attention, and, in any case, dismissing and retrying would result again in the same warning.

The only way for the user to be sure not to include any delegation is using coin control.
But this is tricky as well, since P2CS outputs are not clearly distinguished there, the user can only identify them by checking the owner address (this particular aspect will be addressed in a future PR).

Further, coin control is not available in the MN Wizard, so this doesn't solve 2.

Proposed solution:

  1. A new checkbox in the send widget lets the user decide whether to include delegated coins in the selection algorithm. It is un-checked by default.

Once checked, the "total available balance" and "remaining balance" get updated (including the delegated balance):
include_delegated_1


If coin control is used, the checkbox gets hidden (delegated coins are included only if directly selected in coin control).
The checkbox is also hidden when interacting with zPIV
include_delegated_2


  1. Delegations are now excluded by default in the MN Wizard

  2. Delegations are now excluded by default when creating another delegation.

A user must first cancel a cold-stake delegation (sending it to himself), before being able to use those coins for a MN collateral via Wizard.
To use delegated coins for another delegation (quite unlikely use case), the only possibility now is by using coin control.

@random-zebra random-zebra added this to the 4.1.0 milestone Mar 8, 2020
@random-zebra random-zebra self-assigned this Mar 8, 2020
@random-zebra random-zebra added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Mar 8, 2020
@Fuzzbawls
Copy link
Collaborator

Hmm, I wonder if this checkbox wouldn't be better suited as a QSettings option, set in the Settings area to apply it's effects to the UI as a whole rather than on a per-transaction basis

@random-zebra
Copy link
Author

My main concern with that would be about leaving it unchecked by default.
Regular user wouldn't know it's there and would freak out with "send page is reporting an available balance lower than overview page".
I also think it could make sense to want to change it on a per-transaction basis (spending delegations is a one-off operation).

@Fuzzbawls
Copy link
Collaborator

Would be good to get some user feedback. This is similar enough to the "Spend unconfirmed change" option (which resides in the settings area), at least in functionality, that the exact placement of such an option is not blatantly straight forward.

We obviously need to address the situations where delegations are spent in a way that is unexpected, so the overall concept here is good.

@random-zebra random-zebra force-pushed the 2020_spend_delegations_check branch from dd86489 to d633b04 Compare March 14, 2020 10:05
@random-zebra
Copy link
Author

Rebased.

@random-zebra random-zebra force-pushed the 2020_spend_delegations_check branch from d633b04 to a127c76 Compare March 19, 2020 15:28
@random-zebra
Copy link
Author

Rebased and added release notes. Ready for testing / review.

@random-zebra random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Mar 19, 2020
@furszy
Copy link

furszy commented Mar 20, 2020

I agree with @Fuzzbawls here, this is more suited for a settings configuration than a per-transaction basis. Plus needs a better description there, the "include delegated" text only will not be enough for regular users (In settings we have more space to explain this kind of things).

What we could do to solve the "send page reporting an available balance lower than overview page" is two things:

  1. Add delegations to the "locked included" string show in the topbar ([GUI] Show topbar "Locked Included" label on delegations. #1441).
  2. Notify the user about the delegated locked coins once the first spend fail and ask them if they want to use the delegations (Explaining what happen if they spend them) or point them to the settings configurations, there they will read more about this option and be aware of what they are doing.

I think that the goal here should be explain it properly and not add a easy checkBox that they can touch blindly without really know the consequences. It's the same scenario as spend unconfirmed or the collateral spending.

@random-zebra
Copy link
Author

random-zebra commented Mar 20, 2020

I disagree. I think this is not a setting option, and could be very well used on a per-transaction basis (much like a "shortcut to coincontrol").
Also, having it so close to the available amount and automatically updating it, makes it (in my opinion) much more intuitive than hiding it inside an (already overcrowded) setting page and adopting the confusing flow described in (2) with failure messages.

@furszy
Copy link

furszy commented Mar 20, 2020

It's the same situation as the locked collateral utxo or unconfirmed utxo. Add more controls to the send screen with no proper description will not end up well.
That is how our 3.0 wallet got full of stuff, most of them not used (or only used by few people who knows what the single word control description really means) and with a really bad user experience.

Flood the users with buttons/actions without a good description is not good.

With #1441, the user will know that not all of the balance that he/she has is available. So, the overview vs send screen balance difference is well described now.

@furszy
Copy link

furszy commented Mar 25, 2020

As we spoke in the meeting:

Remember to hide the checkbox when the user has no delegated balance and add a tooltip over the check box explaining what the "include delegated" action means.

@random-zebra
Copy link
Author

As we spoke in the meeting:

Remember to hide the checkbox when the user has no delegated balance and add a tooltip over the check box explaining what the "include delegated" action means.

Yes 👍 will add the automatic hide in a successive commit.
As for the tooltip, currently I've set

    /* CheckBox */
    ui->checkBoxDelegations->setToolTip(tr("Possibly spend coins delegated for cold-staking, if available"));

Feel free to suggest better wording, if need be.

@random-zebra random-zebra force-pushed the 2020_spend_delegations_check branch from 3df5797 to ef1da3a Compare March 26, 2020 20:51
@random-zebra
Copy link
Author

Added automatic hiding of the checkbox when the delegated balance is zero.
Ready for review.

Fuzzbawls
Fuzzbawls previously approved these changes Mar 27, 2020
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ef1da3a

@random-zebra
Copy link
Author

Rebased.

@random-zebra random-zebra force-pushed the 2020_spend_delegations_check branch from 1f7c903 to 35f5c9c Compare April 7, 2020 10:40
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 35f5c9c

furszy added a commit that referenced this pull request Apr 7, 2020
1ef737b [GUI] coin control: add icon and tooltip for delegated UTXOs (random-zebra)

Pull request description:

  Implemented on top of:
  - [x] #1469

  (only last commit is new)

  **Problem**: As outlined in the description of #1391, delegated outputs are not clearly distinguishable in the coin control dialog.

  **Proposed Solution**: Add a "cold staking icon" (purple snow flake) at the end of delegated utxo rows. Hovering on this icon shows a tooltip with the staker address which has received the delegation.

  ![ccontrol1](https://user-images.githubusercontent.com/18186894/77853074-650b2c00-71e2-11ea-8908-926b39f9e230.png)

  <br>

  ![ccontrol2](https://user-images.githubusercontent.com/18186894/77853078-69374980-71e2-11ea-839a-53a2b0a83d20.png)

ACKs for top commit:
  Fuzzbawls:
    ACK 1ef737b
  furszy:
    utACK 1ef737b .

Tree-SHA512: 4da7c478ef8b2463bc80e002e25f6da4b2e18e47afbb024fa344945dbbe5dfc3b0fb75c6e821d67d19b373cf123aad1c827dfcf37b412d0fda5d8cd279aa5fff
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working fine.

The only point is the tooltip text of the checkbox. I'm not sure about the text correctness but can be tackled in another PR.

ACK 35f5c9c .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants