gui: Add transaction record type Fee #12578

pull promag wants to merge 3 commits into bitcoin:master from promag:2018-03-fee-transaction-record changing 6 files +59 −15
  1. promag commented at 6:31 pm on March 1, 2018: member

    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:

     0gettransaction 56d4c03f65f32acecd80ec6ec1b5cd12eddcd35d705f2d4c82a90c85b1cf872a
 1
 2{
 3  ...
 4  "details": [
 5    {
 6      "account": "",
 7      "address": "2N3AFooPuYvN6zWPpThPB8w8V48Ph31PuKH",
 8      "category": "send",
 9      "amount": -1,
10      "label": "",
11      "vout": 0,
12      "fee": -0.0000022,
13      "abandoned": false
14    },
15    {
16      "account": "",
17      "address": "2N42JYZHhwRXgrpuztod6CcEXfUDLTqbhY7",
18      "category": "send",
19      "amount": -1,
20      "label": "",
21      "vout": 1,
22      "fee": -0.0000022,
23      "abandoned": false
24    }
25  ],
26  ...
27}

    Before: After: The row background now alternates based on txid, so in the capture above, the first 3 rows refer to the same transaction.

  2. fanquake added the label GUI on Mar 1, 2018
  3. laanwj commented at 6:42 pm on March 5, 2018: member
    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.
  4. promag commented at 6:55 pm on March 5, 2018: member
    @laanwj that’s an existing problem isn’t?
  5. laanwj commented at 7:29 pm on March 5, 2018: member

    @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.

  6. promag commented at 8:22 pm on March 5, 2018: member

    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).

  7. laanwj commented at 8:45 pm on March 5, 2018: member

    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.

  8. luke-jr commented at 10:45 pm on March 5, 2018: member
    Concept ACK. It might (or might not) make sense to only do it for multi-debits…
  9. promag force-pushed on Mar 6, 2018
  10. promag commented at 2:02 pm on March 6, 2018: member
    @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.
  11. promag force-pushed on Mar 6, 2018
  12. promag commented at 8:07 pm on March 6, 2018: member
    Added release notes.
  13. jonasschnelli commented at 6:32 pm on April 10, 2018: contributor
    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)
  14. promag commented at 7:00 pm on April 12, 2018: member

    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?

  15. jonasschnelli commented at 7:40 pm on April 12, 2018: contributor

    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.

  16. promag force-pushed on Apr 13, 2018
  17. promag commented at 9:02 pm on April 13, 2018: member
    Rebased.
  18. promag force-pushed on Apr 13, 2018
  19. MarcoFalke renamed this:
    Add transaction record type Fee
    gui: Add transaction record type Fee
    on May 10, 2018
  20. laanwj commented at 8:20 pm on May 10, 2018: member
    Concept ACK
  21. jonasschnelli commented at 8:25 pm on May 10, 2018: contributor
    Build: https://bitcoin.jonasschnelli.ch/build/603
  22. jonasschnelli commented at 6:28 am on May 14, 2018: contributor

    Tested ACK ae7a25fafc08482ba8af2174c3e8002bbe5a3bac

    Before / After:

  23. promag force-pushed on May 24, 2018
  24. promag commented at 3:47 pm on May 24, 2018: member
    Rebased due to conflict in src/qt/test/wallettests.cpp.
  25. DrahtBot added the label Needs rebase on Jun 11, 2018
  26. promag force-pushed on Jun 24, 2018
  27. promag commented at 3:05 pm on June 24, 2018: member
    Rebased due to release notes conflict.
  28. DrahtBot removed the label Needs rebase on Jun 24, 2018
  29. DrahtBot added the label Needs rebase on Aug 13, 2018
  30. jb55 commented at 8:29 pm on August 24, 2018: member
    Concept ACK
  31. gui: Avoid TransactionRecord instance for IsMine outputs 82dd3e0ffa
  32. gui: Add transaction record type Fee 550f1f3d34
  33. gui: Alternate row background based on txid 264f9c4d4f
  34. promag force-pushed on Sep 9, 2018
  35. DrahtBot removed the label Needs rebase on Sep 9, 2018
  36. DrahtBot commented at 5:38 pm on December 29, 2018: 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:

    • #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.

  37. practicalswift commented at 0:03 am on December 30, 2018: contributor
    Please mark all single parameter ctors explicit :-)
  38. hebasto commented at 7:27 pm on January 4, 2019: member

    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.

  39. promag commented at 8:54 pm on January 4, 2019: member
    @hebasto the idea is to understand the breakdown of the transfer value. Do you agree with the current implementation?
  40. hebasto commented at 9:16 pm on January 4, 2019: member

    @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”).

  41. luke-jr commented at 2:18 am on January 5, 2019: member

    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.

  42. hebasto commented at 7:38 am on January 5, 2019: member

    @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?

  43. hebasto commented at 10:45 am on January 6, 2019: member

    @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.

  44. hebasto commented at 11:14 am on January 6, 2019: member

    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.

  45. hebasto commented at 8:49 am on January 7, 2019: member
    Does this approach work for special transactions (e.g., CoinJoin, Pay-to-Endpoint etc)?
  46. in src/qt/transactionview.cpp:70 in 264f9c4d4f 
    65+
66+        QItemDelegate::paint(painter, option, index);
67+    }
68+
69+public:
70+    TransactionRecordDelegate(QSortFilterProxyModel* proxy) : m_proxy(proxy) {}
    practicalswift commented at 8:41 pm on January 9, 2019:
    This should be explicit :-)
  47. DrahtBot closed this on May 7, 2019
  48. DrahtBot commented at 5:07 pm on May 7, 2019: member
  49. DrahtBot reopened this on May 7, 2019
  50. DrahtBot added the label Needs rebase on May 9, 2019
  51. DrahtBot commented at 5:49 pm on May 9, 2019: member
  52. promag commented at 9:02 am on June 30, 2019: member
    Closing for now. The implementation is not ideal anyway.
  53. promag closed this on Jun 30, 2019
  54. kallewoof referenced this in commit 41c8d4c89c on Oct 4, 2019
  55. kallewoof referenced this in commit 3edcdf8f0b on Oct 4, 2019
  56. kallewoof referenced this in commit 1655947c15 on Oct 4, 2019
  57. laanwj removed the label Needs rebase on Oct 24, 2019
  58. apoelstra referenced this in commit b314a222b9 on Dec 12, 2020
  59. apoelstra referenced this in commit e56825b3bb on Dec 13, 2020
  60. apoelstra referenced this in commit 2351324302 on Dec 13, 2020
  61. apoelstra referenced this in commit 9e75d2b63d on Dec 13, 2020
  62. apoelstra referenced this in commit c2041a4364 on Dec 13, 2020
  63. apoelstra referenced this in commit 51ec128897 on Dec 13, 2020
  64. apoelstra referenced this in commit a8a23f6205 on Dec 13, 2020
  65. apoelstra referenced this in commit 9d6b8c75e4 on Dec 13, 2020
  66. apoelstra referenced this in commit 246aa9d59e on Dec 14, 2020
  67. apoelstra referenced this in commit 578377a9af on Dec 14, 2020
  68. apoelstra referenced this in commit 11e80537f8 on Dec 14, 2020
  69. apoelstra referenced this in commit 2e46cecf35 on Dec 14, 2020
  70. apoelstra referenced this in commit 1af4e96ce0 on Dec 16, 2020
  71. apoelstra referenced this in commit fe4ca2ee58 on Dec 16, 2020
  72. apoelstra referenced this in commit 832990c146 on Dec 16, 2020
  73. apoelstra referenced this in commit f912cb8e2e on Mar 26, 2021
  74. DrahtBot locked this on Dec 16, 2021


promag laanwj luke-jr jonasschnelli jb55 DrahtBot practicalswift hebasto

Labels
GUI

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me