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

Coin control follow-up #6222

Merged
merged 5 commits into from
Sep 9, 2022
Merged

Coin control follow-up #6222

merged 5 commits into from
Sep 9, 2022

Conversation

komret
Copy link
Contributor

@komret komret commented Sep 7, 2022

Coin control improvements

Description

  • add comments and tiny refactoring e01e212
  • hide "missing to your input" message when the amount is too big 06a62c5
  • [x]hide "missing to your input" message when there is an error in the amount field other than "Not enough funds selected" 910ca36
  • fix behaviour of the check-all checkbox when dust is involved 4a3db2c
  • replace the "change address" message with an icon+tooltip

Related Issue

#2770

QA: So far, this should only fix bugs reported by @bosomt and @STew790 on Slack. Plus there is a small UI change in how change address is displayed.

@komret komret added send Send page bitcoin Bitcoin related labels Sep 7, 2022
@STew790
Copy link
Contributor

STew790 commented Sep 9, 2022

QA OK
Missing to your input message now hides correctly. Check all checkbox now behaves correctly with dust involved.
Checked on the coin-control-followup branch.

Info:

  • Suite version: web 22.9.0 (4a3db2c)
  • Browser: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/104.0.5112.79 Safari/537.36
  • OS: Linux x86_64
  • Screen: 1920x1080
  • Device: model T 2.5.2 Universal (revision 0d87b55ba4fed7eecc72bf2a94ee473830b095e9)
  • Transport: bridge 2.0.32

@komret komret added the release Will be included in the upcoming release. Needs to be backported to the release branch. label Sep 9, 2022
@komret
Copy link
Contributor Author

komret commented Sep 9, 2022

I added the "change address" icon. Let's review & merge.

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

👍

@matejkriz matejkriz marked this pull request as ready for review September 9, 2022 09:57
@matejkriz matejkriz merged commit 0ee1fa7 into develop Sep 9, 2022
@matejkriz matejkriz deleted the feat/coin-control-follow-up branch September 9, 2022 10:01
@komret
Copy link
Contributor Author

komret commented Sep 9, 2022

@STew790 Regarding field validation with multiple recipients:
This is not directly related to Coin control. The reason validation behaves differently here is that the error comes from utxo-lib which composes the transaction. Every time any of the amounts change, all errors are cleared and utxo-lib attempts to compose a new transaction. If it results to an error, it is only linked to the last edited field. This behaviour can be observed even without Coin control enabled:
Screenshot 2022-09-09 at 14 05 45

This is how it has been working for some time now, I suggest keeping it as is. There are some cases when the error should be linked to multiple fields, but I think it is not a big deal because it is visible anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcoin Bitcoin related coin control release Will be included in the upcoming release. Needs to be backported to the release branch. Roadmap: Product send Send page
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants