[PAUZED] feat: (de-)serialize whole packet #4

michielbdejong · 2017-08-28T09:00:05Z

No description provided.

michielbdejong · 2017-08-28T12:17:58Z

Rebased on #5. This PR questions why you would ever want to serialize, or deserialize, only the 'data' part of a clp packet, and simplifies it all by collapsing the many serialize* functions into one serialize function with a switch statement, and the same for deserialize.

To indicate what type of clp packet you are serializing, just set the .type field to one of the constants exposed by the module. The single argument for serialize is the same object as what deserialize return, and the simplification this offers can be seen nicely in the unit tests.

dappelt

Removing the serialize* functions breaks a lot of existing code. I prefer to have an expressive function name than passing in the packet type as an argument. Also, ilp-packet names the functions in the scheme serialize_PacketType_. For a consistent developer experience, we should keep the serialize_PacketType_ functions.

dappelt · 2017-08-28T13:22:16Z

This PR questions why you would ever want to serialize, or deserialize, only the 'data' part of a clp packet, and simplifies it all by collapsing the many serialize* functions into one serialize function with a switch statement, and the same for deserialize.

serializing and deserializing are different in this aspect. When a peer is sending a packet we typically don't know its type, so a deserialize function that takes any packet and returns the type and packet data is handy. On the other hand, when you serialize a packet you need to know which type of packet you want to create. Therefore, having several serialize functions is nice.

Anyway, if you want to add a serialize function that takes the packet type as an argument, add this function as an alias (i.e. without removing the existing serialize* functions).

dappelt · 2017-08-28T13:29:59Z

index.js

+  } break
+
+  case TYPE_PREPARE: {
+    const transferIdBuffer = Buffer.from(obj.data.transferId.replace(/\-/g, ''), 'hex')


That's a zombie bug. I removed the backslash in the previous commit and now its back ;-) (that backslash is unnecessary)

dappelt · 2017-08-28T13:30:35Z

index.js

-      packet: deserializeMessage(buffer) }
-    default:
-      throw new Error('Packet has invalid type')
+function deserialize (buffer) {


I am not a fan of collapsing everything in one method. It reduces readability.

Also, you removed typeString.

OK, I'll reinstate the separate methods.

About the typeString, what is it needed for? It just duplicates the type constant, right?
Adding that as a field just breaks it away from the asn.1 format, so that's quite ugly. What is the use case, just to make the logs more readable? Would it be ok if I just add a static function typeToString to the module exports, that converts from e.g. ClpPacket.TYPE_ACK to the string 'ack'?

Yes, typeString is handy for logging. Replacing it with a function is fine.

However, the current return type (i.e. containing a typeString) is consistent with ilp-packet. Although I agree that a static function would be cleaner, we might want to keep typeString for consistency. (or change ilp-packet also to a static function if that does not break too many dependencies.)

ok. i'll open an issue on ilp-packet because i do like it a lot better as a separate function. This way, the object returned by deserialize is the same as the object you need to give to serialize. Adding typeString inside the return object would mean obj === deserialize(serialize(obj)) would no longer be true.

michielbdejong · 2017-08-28T20:19:56Z

@dappelt all comments addressed, and rebased on master.

dappelt

The serialize* functions are still missing.

michielbdejong · 2017-08-29T10:57:02Z

sorry, hadn't seen your remark about restoring those functions. done now. also added more tests.

comments addressed

dappelt · 2017-08-29T12:21:58Z

index.js

+  deserializeAck (buffer) {
+    const obj = deserialize(buffer)
+    return {
+      requestId: obj.requestId,


There is an inconsistency between deserialize and the deserialize_Type_() functions. The former returns requestId as a field in the envelope, the latter returns requestId as a field in data.

So if you compare the objects returned from clp.deserialize(packet).data to clp.deserializePrepare(packet) the objects will not be the same because of requestId.

How about putting requestId always into the data field?

Yes, clp.deserialize follows the OER format, and clp.deserializePrepare etc follow the slightly different format which Ben introduced. The reason these legacy functions are still there is that they are used in the payment channel framework. I implemented them because you said 'Removing the serialize* functions breaks a lot of existing code' in your previous review. Changing them would break that existing code just as much as removing them.

Michiel and I settled over slack to remove the type specific deserialization functions (clp.deserializePrepare(..) etc.).

removed deserialize* as discussed. kept that in a separate commit in case we ever want to revert it. please re-review.

dappelt · 2017-08-29T15:08:08Z

index.js

+    case TYPE_ACK:
+    case TYPE_RESPONSE:
+    case TYPE_MESSAGE:
+      data = readProtocolData(reader) // see https://github.com/interledger/rfcs/issues/284


That is a breaking change. Can we make this for now non-breaking by changing this line to data = {protocolData: readProtocolData(reader).

Also see this related PR to make the specs match the implementation: interledger/rfcs#285

A bug fix is by definition breaking the behavior for people who relied on that bug being present :)

This wasn't a bug in the first place, but intentional to avoid the problems outlined in interledger/rfcs#284.

Lets merge interledger/rfcs#285 an be done with it.

dappelt · 2017-08-29T15:17:13Z

index.js

+    case TYPE_ACK:
+    case TYPE_RESPONSE:
+    case TYPE_MESSAGE:
+      data = readProtocolData(reader) // see https://github.com/interledger/rfcs/issues/284


This wasn't a bug in the first place, but intentional to avoid the problems outlined in interledger/rfcs#284.

Lets merge interledger/rfcs#285 an be done with it.

michielbdejong · 2017-08-29T15:37:05Z

clp-packet currently has "bugs" (deviations from the spec) which correspond to open PRs on the RFCs repo. (e.g. https://github.com/interledger/rfcs/pull/285/files)

I tried to fix those, but some of Dennis's code relies on these bugs, and since the behavior may become the new correct behavior, it's a waste of work to merge these fixes now as breaking changes, and then revert them next week when the spec changes.

@dappelt and I have both put multiple hours into trying to get to a single clp-packet code base that both he and I are happy with, and we failed.

But luckily we agreed there will be a spec freeze starting 18 september, so parking this PR until then, hopefully it will be easier once we at least know what the correct behavior is on the wire.

dappelt · 2017-08-29T15:43:42Z

It's the spec that has a bug not the code ;-) In fact, we both agree that the spec has a bug: interledger/rfcs#284

I think we should fix the spec, change the single line that is affected by the spec change and merge your PR.

michielbdejong · 2017-08-29T19:28:30Z

It's about the speed at which spec versions succeed each other. Yesterday, I wrote down that day's version of CLP in https://github.com/interledger/interledger/wiki/Interledger-over-CLP#clp, and I pushed this branch with a working implementation of CLP as defined in that wiki page.

I don't care about any other spec version than the version I'm using for the testnet. I had hoped that it would remain "the latest version" for at least a week or so, but that didn't happen. So now I guess the master branch of this repo will track the spec changes as they happen, and I'll just keep using my forked branch for the testnet.

It's a win-win for everybody: you can propose, discuss, and implement the changes you want to, and I can work on the testnet without being distracted by daily or weekly spec changes. That's why I pauzed this PR.

michielbdejong · 2017-08-30T12:26:09Z

I implemented #8 in 5a45886

michielbdejong · 2017-09-15T13:53:25Z

Not sure what happened, but this PR was for 'collapsing the many serialize* functions into one serialize function with a switch statement, and the same for deserialize' and these methods exist in master now, so this PR can be closed. :)

michielbdejong · 2017-09-15T13:56:18Z

Ah right, looks like this PR's commits were mostly included in #11. 👍

michielbdejong requested a review from dappelt August 28, 2017 09:00

michielbdejong mentioned this pull request Aug 28, 2017

docs: introduce CommonLedgerProtocol interledger/rfcs#271

Merged

michielbdejong force-pushed the mj-serialize-deserialize branch from 3377da0 to 53b1b36 Compare August 28, 2017 12:15

michielbdejong changed the base branch from master to da-deserialize-packet August 28, 2017 12:15

dappelt suggested changes Aug 28, 2017

View reviewed changes

dappelt reviewed Aug 28, 2017

View reviewed changes

michielbdejong mentioned this pull request Aug 28, 2017

consider moving typeString to a separate static function interledgerjs/ilp-packet#12

Open

michielbdejong force-pushed the mj-serialize-deserialize branch from 53b1b36 to 376f192 Compare August 28, 2017 20:18

michielbdejong requested a review from dappelt August 28, 2017 20:19

dappelt previously requested changes Aug 29, 2017

View reviewed changes

michielbdejong force-pushed the mj-serialize-deserialize branch from 87d9cf5 to 8015539 Compare August 29, 2017 10:56

michielbdejong requested a review from dappelt August 29, 2017 10:57

feat: symmetric serialize and deserialize

19cbb42

michielbdejong force-pushed the mj-serialize-deserialize branch from 8015539 to 19cbb42 Compare August 29, 2017 11:40

dappelt previously requested changes Aug 29, 2017

View reviewed changes

remove deserialize* as discussed with @dappelt

05b21f4

michielbdejong requested a review from dappelt August 29, 2017 14:26

michielbdejong changed the base branch from da-deserialize-packet to master August 29, 2017 14:26

dappelt reviewed Aug 29, 2017

View reviewed changes

dappelt suggested changes Aug 29, 2017

View reviewed changes

michielbdejong changed the title ~~feat: (de-)serialize whole packet~~ [PAUZED] feat: (de-)serialize whole packet Aug 29, 2017

Don't base64 executionCondition and fulfillment

5a45886

michielbdejong closed this Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAUZED] feat: (de-)serialize whole packet #4

[PAUZED] feat: (de-)serialize whole packet #4

michielbdejong commented Aug 28, 2017

michielbdejong commented Aug 28, 2017

dappelt left a comment •

edited

dappelt commented Aug 28, 2017

dappelt Aug 28, 2017

dappelt Aug 28, 2017

michielbdejong Aug 28, 2017

dappelt Aug 28, 2017

dappelt Aug 28, 2017

michielbdejong Aug 28, 2017

michielbdejong commented Aug 28, 2017

dappelt left a comment

michielbdejong commented Aug 29, 2017

dappelt Aug 29, 2017 •

edited

dappelt Aug 29, 2017

michielbdejong Aug 29, 2017

dappelt Aug 29, 2017

dappelt Aug 29, 2017

michielbdejong Aug 29, 2017

dappelt Aug 29, 2017

dappelt Aug 29, 2017

michielbdejong commented Aug 29, 2017

dappelt commented Aug 29, 2017

michielbdejong commented Aug 29, 2017

michielbdejong commented Aug 30, 2017

michielbdejong commented Sep 15, 2017

michielbdejong commented Sep 15, 2017

[PAUZED] feat: (de-)serialize whole packet #4

[PAUZED] feat: (de-)serialize whole packet #4

Conversation

michielbdejong commented Aug 28, 2017

michielbdejong commented Aug 28, 2017

dappelt left a comment • edited

Choose a reason for hiding this comment

dappelt commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michielbdejong commented Aug 28, 2017

dappelt left a comment

Choose a reason for hiding this comment

michielbdejong commented Aug 29, 2017

dappelt Aug 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michielbdejong commented Aug 29, 2017

dappelt commented Aug 29, 2017

michielbdejong commented Aug 29, 2017

michielbdejong commented Aug 30, 2017

michielbdejong commented Sep 15, 2017

michielbdejong commented Sep 15, 2017

dappelt left a comment •

edited

dappelt Aug 29, 2017 •

edited