feat: balance event timestamps#78
Conversation
benjlevesque
force-pushed
the
balance-events-timestamp
branch
2 times, most recently
from
d69f56b to
e123cf7
Compare
vrolland
left a comment
vrolland
left a comment
There was a problem hiding this comment.
Good move. Nice to see improvements of this part of code.
I have a concern about the amount outside event.parameters, see comments.
| (a: Types.IPaymentNetworkEvent, b: Types.IPaymentNetworkEvent) => | ||
| a.parameters.timestamp - b.parameters.timestamp, | ||
| const events: Types.BTCPaymentNetworkEvent[] = [...payments.events, ...refunds.events].sort( | ||
| (a, b) => (a.timestamp || 0) - (b.timestamp || 0), |
There was a problem hiding this comment.
less typing but more clarity... @romaric-juniet's brain may explode! 😄
| .map( | ||
| (tx: any): Types.IPaymentNetworkEvent => ({ | ||
| (tx: any): Types.BTCPaymentNetworkEvent => ({ | ||
| amount: tx.value.toString(), |
There was a problem hiding this comment.
imo, amount should stay in event.parameters because all the events won't have necessary an amount. (e.g "deny a payment")
There was a problem hiding this comment.
This may need to be discussed with the team
There was a problem hiding this comment.
discussed with Vince, we agreed that it's OK to have an amount of 0 for specific events.
There was a problem hiding this comment.
I would like to be included in this discussion. I'm not sure this makes a lot of sense to me.
| timestamp: output.timestamp, | ||
| txHash: output.txHash, | ||
| }, | ||
| timestamp: output.timestamp, |
There was a problem hiding this comment.
timestamp outside parameters make more sense! 👍
benjlevesque
requested review from
lumtis,
rittme,
romaric-juniet and
vrolland
| .map( | ||
| (tx: any): Types.IPaymentNetworkEvent => ({ | ||
| (tx: any): Types.BTCPaymentNetworkEvent => ({ | ||
| amount: tx.value.toString(), |
There was a problem hiding this comment.
I would like to be included in this discussion. I'm not sure this makes a lot of sense to me.
| '0x627306090abaB3A6e1400e9345bC60c78a8BEf57', | ||
| ); | ||
| expect(balance.events[0].parameters.value).to.be.equal('10'); | ||
| // TODO: add to & from to parameters? |
| }); | ||
| // Clean up the Transfer logs data | ||
| const events = await Bluebird.map(logs, async log => { | ||
| if (!log.blockNumber) { |
There was a problem hiding this comment.
If this can actually fail, it may mean we need some other considerations on the code. For example, to wait block confirmations.
with refactoring of payment detection: - unification of variable names - strong typings
benjlevesque
force-pushed
the
balance-events-timestamp
branch
from
255bbd7 to
dff55fd
Compare
5 participants
Description of the changes
Link to Jira
PROT-1042
PROT-1043