refactor(p2p): JSON encoding to use protobuf instead #2061
Conversation
| @@ -215,6 +214,7 @@ | |||
| "grpc-tools": "^1.10.0", | |||
| "grpc_tools_node_protoc_ts": "^5.1.0", | |||
| "jest": "^26.6.3", | |||
| "json-stable-stringify": "^1.0.1", | |||
There was a problem hiding this comment.
It looks like this is only being used for the Parser.spec.ts suite, but it doesn't make much sense to me to use it for tests when it's not used in the actual functioning of xud. I think we can just change the line that expects the stringify outputs to match to instead check that the packet checksums match, and then get rid of this package entirely, right? If you'd like though I could do this in a follow-up PR instead.
There was a problem hiding this comment.
I was thinking of this as well, but there’s a small problem - protobuf scheme (in which checksum now relies on) is fixed (unlike JSON), and so it will only compare between the define fields, while in some of the negative test cases these are just a subset, because an additional (undefined in protobuf) field is added (e.g., reqId).
Also, there’s a small benefit in comparing between the constructed POJO, and not the binary representation, which is an intermediate stage. I tried to use the built-inJSON.stringify, but the output isn’t deterministic and so many tests fail.
So the usage of stringify here isn’t related to its previous usage in xud. It’s just a tool for comparing between nested objects reliably.
Let me know what you agree. If not, I can just change some of the negative tests.
There was a problem hiding this comment.
Thanks for the explanation. That makes sense and I think it's OK to leave it for now, it's not a big deal to have as a dev dependency. I might take another look at this test suite some time to see if there's a sensible way to run it without this stringify function but no need to hold this PR up for that.
Closing opendexnetwork/opendex.network#34.
Changes:
SessionInitp2p packet fields needed for signing to be encoded via the same protobuf's scheme of the entire packet, instead of JSON. Using a subset of the fields isn't a problem with protobuf - the used fields will be encoded while the remaining ones won't exist on the binary representation.BREAKING CHANGE: JSON encoding to use protobuf instead