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

GUI: Replace send-to-self with dual send+receive entries #15115

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@luke-jr
Copy link
Member

commented Jan 6, 2019

Makes the GUI transaction list more like the RPC, and IMO clearer in general.

As a side effect, this also fixes the GUI entries when a transaction is a net profit to us, but some inputs were also from us.

luke-jr added some commits Dec 15, 2018

GUI: TransactionRecord: Use "any from me" as the criteria for decidin…
…g whether a transaction is a send or receive

This changes behaviour (IMO for the better) in the case where some but not all inputs are from us, and the net amount is positive.

@fanquake fanquake added the GUI label Jan 6, 2019

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2019

Apparently tests need updating. Concept ACK/NACK?

@promag

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Before:
screenshot 2019-01-06 at 18 06 22

After:
screenshot 2019-01-06 at 18 06 41

In #12578 the "Sent to" row would only have 0.1 and the fee would be in a separate row.

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

@promag

In #12578 the "Sent to" row would only have 0.1 and the fee would be in a separate row.

I cannot reproduce that.

@Empact

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Concept ACK - seems like a nice simplification, and IMO the table is more clear

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Does this approach work for special transactions (e.g., CoinJoin, Pay-to-Endpoint etc)?

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Does this approach work for special transactions (e.g., CoinJoin, Pay-to-Endpoint etc)?

It can't. If you have a tx from A & B to C & D, there's no logical way to know the correct logical transaction(s) that comprise it, without some kind of metadata.

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

(It might display it nicer, though, as a single "net change" entry)

Bugfix: Ignore ischange flag when we're not the sender
If we didn't send it, it can't possibly be change, even if that's the key's purpose
@kristapsk

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Haven't tested, but in the screenshot above "Received with" is before "Sent to". Shouldn't it be other way around?

@promag

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

@kristapsk yap, it makes more sense if you consider the order important.

@luke-jr

This comment has been minimized.

Copy link
Member Author

commented Jan 7, 2019

Tests and sorting fixed. Ready for review.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15098 (qt: Show addresses for "SendToSelf" transactions by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

Concept ACK
I guess this is much more understandable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.