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

luke-jr commented at 1:00 pm on January 6, 2019: member

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.

fanquake added the label GUI on Jan 6, 2019

luke-jr commented at 3:02 pm on January 6, 2019: member

Apparently tests need updating. Concept ACK/NACK?

promag commented at 6:12 pm on January 6, 2019: member

Before:

After:

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

hebasto commented at 6:27 pm on January 6, 2019: member

@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 commented at 2:36 am on January 7, 2019: member

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

hebasto commented at 8:50 am on January 7, 2019: member

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

luke-jr commented at 8:58 am on January 7, 2019: member

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 commented at 8:59 am on January 7, 2019: member

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

kristapsk commented at 9:42 am on January 7, 2019: contributor

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

promag commented at 9:47 am on January 7, 2019: member

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

luke-jr commented at 10:48 am on January 7, 2019: member

Tests and sorting fixed. Ready for review.

DrahtBot commented at 10:49 pm on January 9, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19245 ([WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17) by kiminuo)

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 commented at 6:37 am on March 15, 2019: contributor

Concept ACK I guess this is much more understandable.

DrahtBot added the label Needs rebase on Oct 9, 2019

MarkLTZ referenced this in commit 7d803741ab on Nov 18, 2019

luke-jr force-pushed on Feb 15, 2020

luke-jr commented at 8:54 pm on February 15, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Feb 15, 2020

jonasschnelli commented at 7:19 am on May 29, 2020: contributor

Re-concept ACK. Can we have more eyes/reviews on that PR? @promag, @hebasto?

jonasschnelli commented at 7:20 am on May 29, 2020: contributor

Maybe also a QT test that produces a variate of transaction types including CoinJoins with an optional to manually verify the transaction table (or a produced screenshot) would be great.

hebasto commented at 12:49 pm on June 4, 2020: member

Tested 14bb8db698d406e59c173f428abbc8d1ea449fed on Linux Mint 19.3 (x86_64):

one “recipient”
- master
- this PR
two “recipients”
- master
- this PR

I’d prefer to keep the “Payment to yourself” type as descriptive enough.
Could a send-to-self tx has only two records: combined debit and combined credit?

luke-jr commented at 6:51 pm on June 4, 2020: member

I’d prefer to keep the “Payment to yourself” type as descriptive enough.

How would that even work? Why?

Could a send-to-self tx has only two records: combined debit and combined credit?

That breaks labels and defeats the point of having multiple recipients. If you want that as a user, you can just use one recipient?

hebasto commented at 8:20 am on June 5, 2020: member

@luke-jr

I’d prefer to keep the “Payment to yourself” type as descriptive enough.

How would that even work? Why?

On master, one could easily spot send-to-self txs in the tx list, and use sorting in “Type” column. I believe it is convenient. Without “Payment to yourself” type such UX is lost.

Could a send-to-self tx has only two records: combined debit and combined credit?

That breaks labels…

Agree.

… and defeats the point of having multiple recipients. If you want that as a user, you can just use one recipient?

It is up to a user how many txouts one wants to create in a send-to-self tx, no?

luke-jr commented at 4:21 pm on June 5, 2020: member

On master, one could easily spot send-to-self txs in the tx list, and use sorting in “Type” column. I believe it is convenient. Without “Payment to yourself” type such UX is lost.

What is the use case?

It is up to a user how many txouts one wants to create in a send-to-self tx, no?

But you want to make anything other than 1 (in effect) require multiple transactions…

hebasto commented at 4:37 pm on June 5, 2020: member

On master, one could easily spot send-to-self txs in the tx list, and use sorting in “Type” column. I believe it is convenient. Without “Payment to yourself” type such UX is lost.

What is the use case?

For example, a user want to review send-to-self txs made some time ago…

luke-jr commented at 5:41 pm on June 5, 2020: member

What is the use case for “send-to-self txs”?

The only time I’ve ever seen actual sends-to-self occur, are also cases in which deviations of behaviour are bad…

hebasto commented at 5:52 pm on June 5, 2020: member

The only time I’ve ever seen actual sends-to-self occur, are also cases in which deviations of behaviour are bad…

… tend to agree with you.

What is the use case for “send-to-self txs”?

The scope of this PR is to how to present the existing “send-to-self” txs to a user in the GUI, as I understand it.

The discussions about “should send-to-self txs exist” or “what is the use case for send-to-self txs” deserve their own issues/pulls :)

luke-jr commented at 6:12 pm on June 5, 2020: member

This replaces send-to-self. As in, they cease to exist conceptually.

(And they never had any existence beyond conceptually.)

DrahtBot added the label Needs rebase on Jun 17, 2020

GUI: TransactionRecord: Use "any from me" as the criteria for deciding 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.

11e896f456

GUI: TransactionRecord: Refactor to turn send-to-self into send+receive pairs 74e41d7a5f

GUI: Remove SendToSelf TransactionRecord type 7d408b13a0

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

6ac7f198f6

GUI: TransactionRecord: When time/index/etc match, sort send before receive 77a74aac44

luke-jr force-pushed on Aug 20, 2020

luke-jr commented at 1:36 pm on August 20, 2020: member

Rebased. This has quite a few Concept ACKs - how about some code review? :/

DrahtBot removed the label Needs rebase on Aug 20, 2020

fanquake commented at 12:11 pm on September 19, 2020: member

Rebased. This has quite a few Concept ACKs - how about some code review? :/

Lets try and make that happen over in the GUI repo.

fanquake closed this on Sep 19, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts