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

feat(suite): Coin Control #6019

Merged
merged 13 commits into from
Sep 7, 2022
Merged

feat(suite): Coin Control #6019

merged 13 commits into from
Sep 7, 2022

Conversation

komret
Copy link
Contributor

@komret komret commented Aug 16, 2022

Enable coin control in Suite.

Description

  1. Coin selection is enable for Bitcoin (including its test and regtest) in send form. There is a new option Coin control:

Screenshot 2022-08-31 at 16 16 57

  1. When coin selection is used (but hidden manually), it looks like this:

Screenshot 2022-08-31 at 16 18 26

  1. When the Coin control button is clicked, but there are no UTXOs, this message is displayed (copy needed):

Screenshot 2022-08-31 at 16 18 49

  1. If there are any UTXOs and the button is clicked, UTXOs selected by the UTXO lib are checked with grey background:

Screenshot 2022-08-31 at 16 32 32

  1. To turn manual coin selection on, click on the switch or any of the checkboxes:

Screenshot 2022-08-31 at 16 32 44

  1. In case the user has dust in their account, it is shown at the end of the list of UTXOs as a separate category (copy needed):

Screenshot 2022-08-31 at 16 32 51

  1. Pagination is used when there is more than 25 UTXOs:

Screenshot 2022-08-31 at 16 33 12

  1. When hovering over a UTXO, buttons to see transaction details and add label are shown:

Screenshot 2022-08-31 at 16 33 28

  1. When not enough funds are selected, there is a message. At this point, we do not know the final fee, so it is excluded (copy needed):

Screenshot 2022-08-31 at 16 34 11

  1. When the selected funds are enough to cover the outputs, but there is not enough for a fee, a different message is shown (copy needed):

Screenshot 2022-08-31 at 17 30 56

  1. UTXOs on change addresses are marked (users won't have them labeled from transaction history):

Screenshot 2022-08-31 at 16 34 45

Related Issue

Resolve #2770

@b-irving, please check the copy in points 3, 6, 9 and 10.

QA: The above points correspond to what to test, please look for any edge cases as well. There is also fiat amount shown for every UTXO when not on a testnet.

@komret komret force-pushed the feat/utxo-selection-new branch 5 times, most recently from 3f9fd61 to d5be092 Compare August 18, 2022 11:28
@komret komret force-pushed the feat/utxo-selection-new branch 10 times, most recently from 1826f1a to 1915783 Compare August 30, 2022 18:22
@komret komret added bitcoin Bitcoin related send Send page labels Aug 30, 2022
@komret komret added this to the CoinJoin 🥣 milestone Aug 30, 2022
@komret komret force-pushed the feat/utxo-selection-new branch 4 times, most recently from 4e7b3d4 to 959eff8 Compare August 31, 2022 16:26
@dahaca dahaca changed the title Feat/utxo selection new feat(suite): Coin Control Aug 31, 2022
@dahaca dahaca self-assigned this Aug 31, 2022
@dahaca dahaca marked this pull request as ready for review August 31, 2022 17:37
Copy link
Contributor

@dahaca dahaca left a comment

Choose a reason for hiding this comment

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

Great job! 🤓

suite-common/wallet-constants/src/sendForm.ts Outdated Show resolved Hide resolved
<>
<Dot />
<ChangeAddress>
<Translation id="TR_CHANGE_ADDRESS" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps would make sense to add a tooltip and a guide article? @hynek-jina

Copy link
Contributor

Choose a reason for hiding this comment

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

@dahaca Where would be the tooltip? I can't imagine it from these few lines..

Copy link
Member

Choose a reason for hiding this comment

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

change_address

it may sound like call to action 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, i think the tooltip would be nice..

Copy link
Contributor

@andrewkozlik andrewkozlik Sep 2, 2022

Choose a reason for hiding this comment

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

it may sound like call to action

I like to use a dash to emphasize that it's a single term, so "change-address" or "change-output", but I don't think it's widely used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would stick to change address or only change

With reference to Bitcoin Design
https://bitcoin.design/guide/glossary/address/#change-address

Copy link
Contributor

Choose a reason for hiding this comment

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

@b-irving please decide 🙂

Copy link
Member

Choose a reason for hiding this comment

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

let's solve it as a followup

packages/suite/src/components/wallet/UtxoSelection.tsx Outdated Show resolved Hide resolved
packages/suite/src/components/wallet/UtxoSelectionList.tsx Outdated Show resolved Hide resolved
packages/suite/src/hooks/wallet/form/useUtxoSelection.ts Outdated Show resolved Hide resolved
packages/suite/src/hooks/wallet/form/useUtxoSelection.ts Outdated Show resolved Hide resolved
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.

It looks great and the PR is well structured in commits - easy to follow 👍

setValue(
'selectedUtxos',
!selectedUtxos.length && !isCoinControl
? [...new Set([...preselectedUtxos, utxo])]
Copy link
Member

Choose a reason for hiding this comment

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

Does this work as expected here with objects? This prevents to add utxo object with the reference already existing in preselectedUtxos array, but doesn't prevent to add the same utxo with different reference - wouldn't it be safer to check it by txid and vout?

Copy link
Member

@matejkriz matejkriz Sep 2, 2022

Choose a reason for hiding this comment

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

@dahaca please check 3719247 [EDIT: I put the wrong commit before.]

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, I am not following what this code does, could you explain?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are selectedUtxosOld the pre-selected ones?

Copy link
Member

Choose a reason for hiding this comment

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

selectedUtxosOld are the pre-selected ones only if Coin Control is not enabled and user turning the Coin Control on by selecting some UTXO. If Coin Control is already enabled, selectedUtxosOld are those already selected. I know it's a bit confusing, not sure how to name it better. Maybe it would be useful to add a comment?

image

Copy link
Contributor

@dahaca dahaca Sep 6, 2022

Choose a reason for hiding this comment

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

Yes, a commend would be great. Also what about preselectedUtxos?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the comments and made a refactored it a little bit in 206f141. I did not change the variable names because preselectedUtxos is already used.

<>
<Dot />
<ChangeAddress>
<Translation id="TR_CHANGE_ADDRESS" />
Copy link
Member

Choose a reason for hiding this comment

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

change_address

it may sound like call to action 😄

@dahaca
Copy link
Contributor

dahaca commented Sep 2, 2022

it may sound like call to action 😄

Also figuring that out with Ondra

@matejkriz matejkriz assigned dahaca and unassigned matejkriz Sep 2, 2022
@komret komret merged commit 23e369d into develop Sep 7, 2022
@komret komret deleted the feat/utxo-selection-new branch September 7, 2022 11:39
@komret komret mentioned this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitcoin Bitcoin related send Send page
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Coin control 🎯
7 participants