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: Add transaction record type Fee #12578

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@promag
Copy link
Member

promag commented Mar 1, 2018

Currently, in the transactions table, the fee is added as debit to the first output. It's a different behaviour from gettransaction RPC where the values are seen separately:

gettransaction 56d4c03f65f32acecd80ec6ec1b5cd12eddcd35d705f2d4c82a90c85b1cf872a

{
  ...
  "details": [
    {
      "account": "",
      "address": "2N3AFooPuYvN6zWPpThPB8w8V48Ph31PuKH",
      "category": "send",
      "amount": -1,
      "label": "",
      "vout": 0,
      "fee": -0.0000022,
      "abandoned": false
    },
    {
      "account": "",
      "address": "2N42JYZHhwRXgrpuztod6CcEXfUDLTqbhY7",
      "category": "send",
      "amount": -1,
      "label": "",
      "vout": 1,
      "fee": -0.0000022,
      "abandoned": false
    }
  ],
  ...
}

Before:
screen shot 2018-03-01 at 18 44 20
After:
screen shot 2018-03-06 at 13 55 08
The row background now alternates based on txid, so in the capture above, the first 3 rows refer to the same transaction.

@fanquake fanquake added the GUI label Mar 1, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 5, 2018

Concept ACK, I think it is clearer to list the fee separately. On the other hand, this compounds the issue that it's not possible to see in the list what transaction output records group to one transaction.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Mar 5, 2018

@laanwj that's an existing problem isn't?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 5, 2018

@laanwj that's an existing problem isn't?

Yes, that's what I said. However this compounds it, because it means every transaction will get multiple records, whereas it used to be that this only happens for (rarer) multi-sends. Not an argument against doing this, but something to be aware of, as people will ask "what does this fee belong to". They can find out by looking at the transaction details.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Mar 5, 2018

Yeah, in the typical listing it will show an alternating "Fee" row.

I was thinking in adding a checkbox to show/hide individual fee rows.

Another thing that could help the grouping is to change from alternate rows into "alternate txids", but that is something more complicated to do (as it depends on the proxy model parameters).

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 5, 2018

Another thing that could help the grouping is to change from alternate rows into "alternate txids", but that is something more complicated to do (as it depends on the proxy model parameters).

Yeah that can be done in a future PR. Sounds like a good idea.
I wonder how that will/should interact with sorting on alternative columns, though.

I was thinking in adding a checkbox to show/hide individual fee rows.

Might be overkill, just adds an extra configuration that needs to be supported. But no strong opinion.

Do add it to the release notes, this is something that users are bound to notice.

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Mar 5, 2018

Concept ACK. It might (or might not) make sense to only do it for multi-debits...

@promag promag force-pushed the promag:2018-03-fee-transaction-record branch Mar 6, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Mar 6, 2018

@laanwj I've implemented alternating rows based on transaction id (see after image above). The implementation is not efficient but it's enough to evaluate the concept.

@luke-jr even for single-debit it's weird to have the fee associated to that address, sounds like you sent that value there.

@promag promag force-pushed the promag:2018-03-fee-transaction-record branch Mar 6, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Mar 6, 2018

Added release notes.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 10, 2018

Concept ACK especially with the grouping (odd/even background alternative). IMO the grouping could be more visible (group entries from a transaction with an outline border line or similar)

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Apr 12, 2018

group entries from a transaction with an outline border line or similar

I'll try something along that, but it's not something usual right?

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Apr 12, 2018

I'll try something along that, but it's not something usual right?

I guess that is something that could be improved in a follow up PR.

@promag promag force-pushed the promag:2018-03-fee-transaction-record branch Apr 13, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Apr 13, 2018

Rebased.

@promag promag force-pushed the promag:2018-03-fee-transaction-record branch Apr 13, 2018

@MarcoFalke MarcoFalke changed the title Add transaction record type Fee gui: Add transaction record type Fee May 10, 2018

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 10, 2018

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 10, 2018

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 14, 2018

Tested ACK ae7a25fafc08482ba8af2174c3e8002bbe5a3bac

Before / After:

bildschirmfoto 2018-05-14 um 08 26 42

bildschirmfoto 2018-05-14 um 08 27 56

@promag promag force-pushed the promag:2018-03-fee-transaction-record branch May 24, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented May 24, 2018

Rebased due to conflict in src/qt/test/wallettests.cpp.

@promag promag force-pushed the promag:2018-03-fee-transaction-record branch Jun 24, 2018

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jun 24, 2018

Rebased due to release notes conflict.

@jb55

This comment has been minimized.

Copy link
Contributor

jb55 commented Aug 24, 2018

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Dec 29, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15452 (Replace CScriptID and CKeyID in CTxDestination with dedicated types by instagibbs)
  • #15115 (GUI: Replace send-to-self with dual send+receive entries by luke-jr)

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.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 30, 2018

Please mark all single parameter ctors explicit :-)

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Jan 4, 2019

https://en.bitcoin.it/wiki/Transaction:

A transaction is a transfer of Bitcoin value...

So, the "Transaction" table should present the value transfer history and not be littered with "Fee" type output rows.

Moreover, there is no such thing as "Fee" transaction.

If one wants display fees in the "Transaction" table this could be done with a separate "Fee" column.

@promag

... the first 3 rows refer to the same transaction.

I'm strongly disagree with such approach.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 4, 2019

@hebasto the idea is to understand the breakdown of the transfer value. Do you agree with the current implementation?

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Jan 4, 2019

@promag

the idea is to understand the breakdown of the transfer value.

There is a "Details ..." window to find out and understand the breakdown of the transfer value.

Do you agree with the current implementation?

No. I believe we should stick to "one transaction - one row" rule in the "Transactions" table.
"Fee" rows will produce a mess while user tries sorting within columns (e.g. by "Amount").

@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jan 5, 2019

https://en.bitcoin.it/wiki/Transaction

If the wiki is confusing, it would be good to improve it.

I believe we should stick to "one transaction - one row" rule in the "Transactions" table.

@promag's point is that we have NEVER had such a "rule"...

It would also be very user unfriendly.

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Jan 5, 2019

@luke-jr

https://en.bitcoin.it/wiki/Transaction

If the wiki is confusing, it would be good to improve it.

There is nothing confusing in the wiki, IMO.
A bitcoin transaction is a well-defined technical notion since 2008 :)

@promag's point is that we have NEVER had such a "rule"...

A "rule" is a bad wording from my side; I mean "de facto".

It would also be very user unfriendly.

You consider "one transaction - one row" presentation of the "Transaction" table very user unfriendly. Did I understand you correctly?

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Jan 6, 2019

@promag @luke-jr on second thought I've realized the "one transaction - one row" presentation is overkill. Moreover, multiple recipients transactions (multi-sends) currently are presented with appropriate number of rows.

Concept ACK.

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Jan 6, 2019

Tested on Linux:

screenshot from 2019-01-06 12-54-30

Note "Fee" rows are listed at the end of transaction groups. When they somehow change rows order and revert it back, "Fee" rows can be listed at the beginning of transaction groups:
screenshot from 2019-01-06 13-07-04

Although, this behavior can be fixed in another PR.

@promag

I was thinking in adding a checkbox to show/hide individual fee rows.

It will significantly improve UX, IMO.

Also, a "Fee" row does not appear for "Payment to yourself" transactions.

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Jan 7, 2019

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

}

public:
TransactionRecordDelegate(QSortFilterProxyModel* proxy) : m_proxy(proxy) {}

This comment has been minimized.

@practicalswift

practicalswift Jan 9, 2019

Member

This should be explicit :-)

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.