interledgerjs / btp-packet

Packet parser for Bilateral Transfer Protocol
1 stars 3 forks source link

feat: match asn.1 names, en/decode errors #3

Closed michielbdejong closed 7 years ago

michielbdejong commented 7 years ago

One thing that this PR leaves suboptimal is that how it reads the IlpError inside an Error or Reject:

I added a TODO to clean up that inefficient situation.

Another thing is I wasn't sure whether conditions and fulfillments should be passed in and out of the clp-packet module as buffers or as base64 strings. I would say buffers, because there's no need to use base64 unless including them in log statements, and it will be more efficient to not constantly convert them from and to base64. But when changing that I would also need to change the representation for amount and transferId/requestId which are all strings now, and then for dates you could argue it's more efficient to use millisecond-since-epoch integers instead of date-strings, so I left that as it is for now.

sharafian commented 7 years ago

Most of these fields that don't match the names of the ASN.1 instead match the names used in the ilp-packet module and LPI. Given that this is the JS implementation, it would be easier to use if the naming matches other JS modules. On the other hand, it could make the ASN.1 harder to read

sharafian commented 7 years ago

for dates you could argue it's more efficient to use millisecond-since-epoch integers instead of date-strings, so I left that as it is for now.

We can't use UTC timestamps because they're not strictly increasing (leap-seconds). Using a GeneralizedTime timestamp lets us represent leap-second times like 23:59:60