interledgerjs / btp-packet

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

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

Closed michielbdejong closed 7 years ago

michielbdejong commented 7 years ago

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 commented 7 years ago

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).

michielbdejong commented 7 years ago

@dappelt all comments addressed, and rebased on master.

michielbdejong commented 7 years ago

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

michielbdejong commented 7 years ago

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 commented 7 years ago

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

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

michielbdejong commented 7 years ago

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 commented 7 years ago

I implemented #8 in https://github.com/interledgerjs/clp-packet/pull/4/commits/5a45886172427add3c9cb766caaa8d8691739579

michielbdejong commented 7 years ago

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 commented 7 years ago

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