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

backend: update coin control outputs sorting #2605

Merged
merged 1 commit into from
May 20, 2024

Conversation

strmci
Copy link
Collaborator

@strmci strmci commented Mar 5, 2024

Currently, the list of all UTXOs in the coin control selection is ordered by amount. Users sometimes re-use addresses and are not aware that there might be other UTXOs they didn't want or want to use on the same address as their selected output. Grouping the UTXOs by address helps the user to detect if there are other UTXOs.

This commit updates the sorting of outputs in the coin control dialog and is now grouped by address from the biggest to the lowest amount.

@strmci strmci requested a review from Beerosagos March 5, 2024 11:54
@strmci strmci force-pushed the coincontrol_utxo_sort branch from b188d2c to 3a19b45 Compare March 8, 2024 10:31
@strmci
Copy link
Collaborator Author

strmci commented Mar 8, 2024

@thisconnect thanks, I updated the commit message, PTAL

@thisconnect
Copy link
Collaborator

Before this change the sorting was outputs by amounts. Now with this change it is outputs grouped by address then sorted by amounts.

For users that didn't re-use addresses nothing should change.

For users that did reuse address the list should change so that all UTXO's of the same address are shown together.

I think it would be great if there is a banner on top that shows "Warning: some addresses were re-used" or similar (banner of shame).

Copy link
Collaborator

@Beerosagos Beerosagos left a comment

Choose a reason for hiding this comment

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

LGTM apart for a small nit. I agree that we definitely need a proper warning banner in case of address reuse, and maybe also highlighting somehow the reused addresses (e.g. bold font, or smth else).

for _, output := range outputs {
sum += output.TxOut.Value
}
sums = append(sums, struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I don't like it much defining the struct twice. Maybe it is better to define it once with a proper private name outside the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a new type addressOutputsSum and used it in the function.

@strmci strmci force-pushed the coincontrol_utxo_sort branch from 3a19b45 to fbd0f40 Compare May 17, 2024 07:19
Currently, the list of all UTXOs in the coin control
selection is ordered by amount. Users sometimes re-use
addresses and are not aware that there might be other
UTXOs they didn't want or want to use on the same address
as their selected output. Grouping the UTXOs by address
helps the user to detect if there are other UTXOs.

This commit updates the sorting of outputs in the coin
control dialog and is now grouped by address from the
biggest to the lowest amount.
@strmci strmci force-pushed the coincontrol_utxo_sort branch from fbd0f40 to 6e60ac7 Compare May 17, 2024 07:42
@strmci
Copy link
Collaborator Author

strmci commented May 17, 2024

I agree that we definitely need a proper warning banner in case of address reuse, and maybe also highlighting somehow the reused addresses (e.g. bold font, or smth else).

Would you like to add it here, or should we do it in a separate PR?

@thisconnect
Copy link
Collaborator

I think separate PR makes sense or at least separate commit. For example in the case we need to undo one of the 2 changes.

@strmci
Copy link
Collaborator Author

strmci commented May 20, 2024

PR to add address reuse warning: #2724

@strmci strmci merged commit ab47bcf into BitBoxSwiss:master May 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants