Skip to content

Coin control dialog utxo processing/update flow large cleanup and improvements#1977

Merged
furszy merged 20 commits into
PIVX-Project:masterfrom
furszy:2020_improve_coin_control_update
Nov 20, 2020
Merged

Coin control dialog utxo processing/update flow large cleanup and improvements#1977
furszy merged 20 commits into
PIVX-Project:masterfrom
furszy:2020_improve_coin_control_update

Conversation

@furszy
Copy link
Copy Markdown

@furszy furszy commented Nov 17, 2020

Base initial work in order to be able to generalize CoinControlDialog up to the point of introducing shielded notes selection (being developed in the Sapling GUI PR #1963).

Found several redundancies and not so good code over the overall CoinControlDialog class. So.. have re-structured it in a large percentage, aiming to improve the performance and clean all of the inefficiencies.

@furszy
Copy link
Copy Markdown
Author

furszy commented Nov 17, 2020

Added more cleanups. This got bigger than what expected.

@furszy furszy force-pushed the 2020_improve_coin_control_update branch from d6f06ea to 4df5872 Compare November 18, 2020 09:34
@furszy furszy added this to the 5.0.0 milestone Nov 18, 2020
@furszy
Copy link
Copy Markdown
Author

furszy commented Nov 18, 2020

pushed one more time, ready for review.

@Fuzzbawls
Copy link
Copy Markdown
Collaborator

Fuzzbawls commented Nov 19, 2020

Commit 561de4d7dea630d996815a1ffb08822189ecd621 makes it so that "change" outputs (when not specifying a custom change address) are no longer properly designated as such in either view mode. They instead show with a (no label). I'd like to retain the "change" designation.

Copy link
Copy Markdown

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Code review ACK up till 750858b3b8ab806bac87df29fc45876b34badf35
Really nice improvements, left just some minor notes.

This removes the tooltip for change address, showing where the change is coming from.
But I think that the performance gain beats the usefulness of such info (can't think of any possible use case, now that the account system is removed)

Comment thread src/qt/walletmodel.cpp Outdated
Comment thread src/qt/walletmodel.cpp Outdated
Comment thread src/qt/walletmodel.h Outdated
Comment thread src/qt/walletmodel.h Outdated
furszy and others added 16 commits November 19, 2020 16:58
…dress == sWalletAddress` has been always true.
…lculation. Our reference client is not creating them by default. Most likely never did it.
… be able to return unspent notes in the future as well.
@furszy
Copy link
Copy Markdown
Author

furszy commented Nov 19, 2020

yep agree. Going to add the change empty label in a much straightforward manner and done.
great feedback 👍

@furszy furszy force-pushed the 2020_improve_coin_control_update branch from 7f0328c to 71e1f98 Compare November 19, 2020 20:27
@furszy
Copy link
Copy Markdown
Author

furszy commented Nov 19, 2020

Updated per feedback. Added the change label.

Copy link
Copy Markdown

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Nice stuff 🥃 ACK 71e1f98

The change addresses still report "(no label)" in tree mode (as that is set before calling loadAvailableCoin) but no big deal.

Copy link
Copy Markdown
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.

Nice cleanup! We can address the change label in tree mode later if need be

ACK 71e1f98

@furszy furszy merged commit bc2ec85 into PIVX-Project:master Nov 20, 2020
@furszy furszy deleted the 2020_improve_coin_control_update branch November 29, 2022 14:22
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.

4 participants