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

[VDG] Coin Control #9089

Conversation

SuperJMN
Copy link
Collaborator

@SuperJMN SuperJMN commented Sep 10, 2022

Closes #7721
This adds a generic coin selection dialog that can be reused. It's already integrated in the Send flow when the Manual coin control toggle is selected.

Alternate version of #8975

I decided to give the view a better look and this is the result. I personally think this is way better than the original design.
Please, give me your feedback.

Screenshots

image

image

Selection dropdown
image

Plain list (non-grouped)
image

Warning message
image

Light theme
image

image

Thanks to @lontivero for the great ideas!

Copy link
Collaborator

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

tested 6dc4eb3

the enter button selects a coin, it does not continue

@SuperJMN
Copy link
Collaborator Author

tested 6dc4eb3

the enter button selects a coin, it does not continue

Fixed.

As I mentioned before, to fix this there's a workaround that breaks keyboard navigation.
We will need to revisit this in the future.
Avalonia needs changes for better keyboard usability since default button isn't favored when enter is hit.

Copy link
Collaborator

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

tested e91b530

when I click the drop down menu to select all/none, and then click enter, it again opens that dropdown, and does not continue.

Also now it seems that I cannot continue at all, even if sufficient coins are selected, the continue button doesn't work.

@SuperJMN SuperJMN changed the title [VDG] Coin Selection [VDG] Coin Control Oct 25, 2022
MaxHillebrand
MaxHillebrand previously approved these changes Oct 25, 2022
Copy link
Collaborator

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

tACK 3b03e90

Continue button works now.

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

NACK, this PR should not be merged, not before the release.

  • This PR was being tested (the UI part) since more than 2 months (counting [VDG] Coin Selection #8975) but it should be now retested thoroughly again given that the amount of changes that was made in less than 2 days is already huge, and that might have broken things that we are not aware of.

  • There are a lot of previous suggestions/comments that are not taken into consideration.

  • In the last 2 months the code was barely properly reviewed by anyone I guess. @soosr started doing that but just less than 24 hours ago.
    Is that enough to merge this huge PR that adds 2000 LoC that was merely reviewed only by one person?

  • Small examples of the code not being carefully reviewed:
    1- Unnecessary change in Button.axaml.
    2- Duplicate line in SelectableCoin.cs: _amount = Money.Zero;

  • We should not rush things just because there is a release coming.
    If this (or any other) PR is not ready it should simply not be merged.

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

Here are some issues while testing this quickly:

  1. Having the back button and the search label on the same line looks so weird and strange.
    All other dialogs have a title, why not this one?
  2. The coins under the same pocket are neither ordered by amount nor by anonscore which looks confusing.
  3. The coins selected are enough by the progress bar is not blue.

1

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

Here is an even more interesting thing to consider:

While trying to investigate the issue 3 in my previous comment I realized that the automatic coin selection ("smart" option) is selecting more coins than needed (I am trying to send 0.003):

This is the smart option (default):

2

Deselected 3 coins:

3

Deselected one coin:

4

This most likely is not related to this PR directly but it is worth investigating thoroughly.

It is for sure related to the recent changes to take into account the fee, which were introduced in: #9289 #9319 #9335, and also to how InsufficientBalanceException works I guess.

@yahiheb
Copy link
Collaborator

yahiheb commented Oct 26, 2022

I noticed that the number of prebuilds is different when using the automatic or the manual coin control. Is this normal?

BTW the number of prebuilds is a lot but this is also not related to this PR but worth mentioning.

@SuperJMN
Copy link
Collaborator Author

SuperJMN commented Oct 26, 2022

  1. Having the back button and the search label on the same line looks so weird and strange.
    All other dialogs have a title, why not this one?

@zkSNACKs/visual-design-group should discuss this and come to an agreement. Changing this is trivial, but we should be aligned.

  1. The coins under the same pocket are neither ordered by amount nor by anonscore which looks confusing.

Fixed. Ordering by amount now.

  1. The coins selected are enough by the progress bar is not blue.

Can you give me the steps to reproduce it? I haver never had that behavior.

@SuperJMN
Copy link
Collaborator Author

tested e91b530

when I click the drop down menu to select all/none, and then click enter, it again opens that dropdown, and does not continue.

Also now it seems that I cannot continue at all, even if sufficient coins are selected, the continue button doesn't work.

Can't be fixed without hacks. Avalonia needs to handle this differently. The button is not focusable, but it still gets focused. That's why it steals the Enter key.

Copy link
Collaborator

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

tACK eecc19a

sorted by amount now.

Also now it seems that I cannot continue at all, even if sufficient coins are selected, the continue button doesn't work.

Can't be fixed without hacks

Ok, don't let's wait for avalonia update

automatic coin selection ("smart" option) is selecting more coins than needed

yes, there are unnecessary inputs selected, and it's not even caught in the transaction preview.
I guess this is unrelated to this PR, and should be fixed properly elsewhere.
https://mempool.space/testnet/tx/e70e83a59cc0f09d9f757417f8c165f68487236b11c58ae91da0efecc02d1314

Having the back button and the search label on the same line looks so weird and strange.
All other dialogs have a title, why not this one?

I think it is good to have a "compact" dialog, so that the table is larger and more coins are visible.

@soosr
Copy link
Collaborator

soosr commented Oct 26, 2022

After a long discussion today we (@danwalmsley, @SuperJMN and I) decided not to continue this PR and start it over and implement it in smaller parts in separate PRs and also decrease the complexity of the code significantly.

@soosr soosr closed this Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fallback coinlist with payment coin control