WIP: transaction report rework #3316
Conversation
ryankurte
force-pushed
the
ryan/tx-report-rework
branch
from
357b316
to
2249e68
Compare
|
So here are my thoughts:
If we don't think it's interesting to display data about swap counterparties at all, my suggestion would be, lets remove anything that says swap in it before displaying. If we do think it's interesting, then let's record both credits and debits to the swap counterparties. In this change right now, we are still recording outputs destined for swap counterparties, but dropping inputs associated to them. I don't see the logic of this right now, but I'm willing to be convinced that this is better. I know we discussed this some weeks ago and I get that we definitely want to display every output, it just seems weird if we report credits to swap counterparties but not debits. But if you think that's exactly what we decided to do a few weeks ago, I'm okay with that, I think we should at least leave a comment saying that we are intentionally doing that, because this would be very confusing to me much later. |
|
Maybe we can write something in the README here about what is the structure of the report, and how it's intended to be used? https://github.com/mobilecoinfoundation/mobilecoin/tree/master/transaction/summary |
the motivation behind this is that the report shows the values and totals spent by our account to fulfill the transaction, and avoids the whole positive vs. negative balance change thing. maybe in the case of a swap we could show both the value sent to the counterparty and the exchanged value in the same report entry, so the information is there but without impacting the totals? |
ryankurte
force-pushed
the
ryan/tx-report-rework
branch
from
2249e68
to
31bdf3d
Compare
There was a problem hiding this comment.
I'm still wrapping my head around it, but I finished my first iteration of looking at it and made some comments/posted some questions. It overall makes sense to me but I haven't dived deep enough to notice the nuances @cbeck88 has mentioned in his comments, so I want to try and better understand that.
| *value = value.checked_sub(*v as i64).ok_or(Error::NumericOverflow)?; | ||
| } | ||
| // If it's coming from an SCI to us, add to total balance | ||
| TotalKind::Sci if e != &TransactionEntity::Swap => { |
There was a problem hiding this comment.
What about from an SCI to someone else? (TransactionEntity::OtherAddress?)
There was a problem hiding this comment.
there's a test for a three way SCI in transaction/extra which is okay, but how the SCI totals should be computed is a bit ambiguous atm, definitely appreciate if you have any thoughts on how to tidy this up?
There was a problem hiding this comment.
I think my comment is no longer relevant with the current implementation.
ryankurte
force-pushed
the
ryan/tx-report-rework
branch
from
dd6b139
to
c66eea3
Compare
splits output and totals, adds balance checks, support for SCIs, tests
ryankurte
force-pushed
the
ryan/tx-report-rework
branch
from
0d1380e
to
bc5bc3c
Compare
ryankurte
force-pushed
the
ryan/tx-report-rework
branch
from
bc5bc3c
to
3ab66d3
Compare
| *value = value.checked_sub(*v as i64).ok_or(Error::NumericOverflow)?; | ||
| } | ||
| // If it's coming from an SCI to us, add to total balance | ||
| TotalKind::Sci if e != &TransactionEntity::Swap => { |
There was a problem hiding this comment.
I think my comment is no longer relevant with the current implementation.
rework of transaction reports to split outputs and totals, allowing display that matches other wallets.
reporttrait to decouple report const generics(double-entry style
inputs - changeon one side,outputs + feeson the other)outputs to
changeaddresses are elided so SCI outputs MUST NOT be to the change address as these will not appear in the totals.