FABN-1530: Timestamp added to full and private block transaction events #490
Conversation
sapthasurendran
force-pushed
the
event
branch
2 times, most recently
from
96eee59 to
fcd4ce2
Compare
There was a problem hiding this comment.
A few comments that I think would make it more user friendly and make the testing more robust. See what you think.
It might also be worth a unit test for the filtered blocks case, where we expect timestamp to be undefined since we don't get the timestamp in the raw event.
fabric-network/src/events.ts
Outdated
| @@ -24,6 +24,7 @@ export interface TransactionEvent { | |||
| readonly transactionData: fabproto6.protos.ITransaction | fabproto6.protos.IFilteredTransaction; | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| readonly privateData?: any; | |||
| readonly timestamp?:string; | |||
There was a problem hiding this comment.
It might be more helpful for us to expose a JavaScript Date object instead of a string
| const timestamp = new fabproto6.google.protobuf.Timestamp({seconds:1630075029083}); | ||
| channelHeader.timestamp = new Date(timestamp.seconds as number).toISOString(); |
There was a problem hiding this comment.
The block decoder looks to decode the protobuf timestamp to a date string like you've done here, so that's good. So we know the value to expect in the timestamp field, I think we'd be better to declare a constant in the describe block, something like const timestamp = new Date(), and then populate the timestamp with timestamp.toISOString()
| await network.addBlockListener(listener, listenerOptions); | ||
| eventService.sendEvent(event); | ||
| const actual = await listener.completePromise; | ||
| expect(actual[0].getTransactionEvents()[0].timestamp).not.null; |
There was a problem hiding this comment.
Should check the value is correct, not just that it exists. So if we've defined a timestamp Date value for these tests, we could test that the timestamp we get from the event matches our expected timestamp value
| await network.addBlockListener(listener, listenerOptions); | ||
| eventService.sendEvent(event); | ||
| const actual = await listener.completePromise; | ||
| expect(actual[0].getTransactionEvents()[0].timestamp).not.null; |
There was a problem hiding this comment.
Should check the value is correct
| @@ -41,6 +41,7 @@ export function newFullTransactionEvent(blockEvent: BlockEvent, txEnvelopeIndex: | |||
|
|
|||
| const envelope: any = block.data.data[txEnvelopeIndex]; | |||
| const transactionId = envelope.payload.header.channel_header.tx_id; | |||
| const timestamp = envelope.payload.header.channel_header.timestamp; | |||
There was a problem hiding this comment.
We should be able to fluff up a Date object from the string with new Date(envelope.payload.header.channel_header.timestamp)
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
| function newEnvelope(transaction: any, transactionId?: string, timestamp?:Date): any { | ||
| const channelHeader :any = {}; | ||
| channelHeader.type = fabproto6.common.HeaderType.ENDORSER_TRANSACTION; | ||
| channelHeader.tx_id = transactionId; | ||
| channelHeader.timestamp = timestamp.toISOString(); |
There was a problem hiding this comment.
Unsafe use of optional (Date | undefined) timestamp variable here, which would fail at runtime if not set
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Signed-off-by: sapthasurendran <saptha.surendran@ibm.com>
Release includes: - Add support for getting CA server information (hyperledger#510) - Close Client in Gateway disconnect (hyperledger#287) - Include timestamp in transaction events (hyperledger#490) Signed-off-by: Mark S. Lewis <mark_lewis@uk.ibm.com>
No description provided.