Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use nim-json-serialization for RPCs #172

Merged
merged 7 commits into from
Dec 14, 2023

Conversation

coffeepots
Copy link
Contributor

This is a PR to use nim-json-serialization for encoding and decoding.

Notes about testrpcmacro.nim:

  1. Instances of check r == %value were changed to check r == Eth1JsonRpc.encode value. This is because serialization handles optionals slightly differently to the native json library.
  2. Floating point isn't the same after the round trip. For now I just set the test value to 1.0 rather than the 1.23 it was before - otherwise we get 1.2300000014 or similar. I assume this is because the internal floating point output is in a different precision. Please advise if there's a better way to test this.

@arnetheduck
Copy link
Member

arnetheduck commented Oct 24, 2023

before we move in this direction we need:

  • comprehensive tests in the eth/web3 packages that no serialization changes as a result of changing to this version - that means input/output tests for each serialised type vs how it's serialised now
  • support for distinct serializers that don't provide any default JSON serialization for random types passed to it, ie fully explicit serialization only - this is to ensure that lack of or reordering of imports, generic sandwitches and simple oversight doesn't accidentally introduce regressions

@coffeepots
Copy link
Contributor Author

@arnetheduck I've added some serialization wrappers that will hopefully satisfy the second requirement. This commit fixes types to specific json_serialization types, which should avoid generic sandwiches and import order dependencies.

Let me know if you'd prefer me to completely avoid all pass-through to json_serialization and implement them as lower level token processing (e.g., like the byte implementation in the above commit).

@coffeepots
Copy link
Contributor Author

coffeepots commented Oct 30, 2023

Regarding f314957, I hope a converter is an acceptable solution here. I usually avoid them, but the helpers module is only used for testing and this way avoids the verbosity of:

template `==`*(a, b: distinct (string|StringOfJson)): bool =
  {.push hint[ConvFromXtoItselfNotNeeded]:off.}
  let r =string(a) == string(b)
  {.pop.}
  r

@arnetheduck
Copy link
Member

I've added some serialization wrappers

this is something that needs fixing in nim-json-serialization - ie it's JsonFlavor that is fundamentally not fit for purpose and json-rpc happens to be just one victim of this gap - a new feature needs to be written there in such a way that it doesn't expose default serializers unless asked. When people want the default serializers, they should not have to write lexers, macros and other hacks/workarounds in their projects or we will keep seeing this problem wherever nim-json-serialization is used - ie that new feature could expose functionality to make it easy to map some "common" ways to interpret the json standard types as nim types but we need to address the root cause at this stage.

nim-json-rpc is just a thin wrapper - the only thing it should do is to implement the json-rpc standard on top of a few transports - ie the framing etc - it should not contain any json lexing code - that belongs in nim-json-serialization. It should also not contain any ethereum-specific encoding quirks - that belong in nim-web3 and friends - this project is agnostic to ethereum and it should be possible to customize the "standard" used to interpret / parse the json data.

Taking a step back, there are two standards at play here: one is the JSON-RPC protocol which defines a "frame" for an RPC call - the other is the standard that the library user controls - ie how to parse the result field of the https://www.jsonrpc.org/specification#response_object and any error data.

The key insight from the standards text is that this serialization is distinct from the parsing of the frame.

As a user of the json-rpc library, I need to be able to define a parser used for for the user-controlled parts which needs to be precise and without cross-implementation leaks in general while JSON-RPC itself needs to use its own parser to ensure that it correctly parses the data in the standard - these two parsers may coincide but they don't have to. Going even deeper, parsing JSON is made up of two "stages": interpreting text as "json types" and translating "json types" to "nim types" - the first stage is shared between all json parsers while the second stage needs to be customizable with fine-grained control.

As such, looking at the state of this PR we need to go back to the basics:

  • Users of nim-json-ser need to be able to create "second-stage" mappings for json-based standards that are precise and don't have leaks - that includes leaks for "core types" like int etc because there exist many ways to do these mappings generally and nim-json-ser is not the right place to decide how to do this - providing default serializers for int etc only has utility where adherence to standards doesn't matter, ie loose pretty printing for human consumption, in-process / ephemeral communication and so on - in most actual uses of json for the purpose of serialization there exists a common standard that is being implemented whose job it is to define this mapping (to enable cross-language/component communication). In part, this means that Json of nim-json-serialization is not an appropriate starting point, neither is JsonFlavor. This needs a distinct approach.
  • Users of json-rpc need to be able to inject this parser - however, it should only be used for the fields that in the json-rpc spec are described as "The value of this member is determined by the method invoked on the Server" or similar. This is where the "ethereum" part comes in, ie in nim-web3 and/or nimbus-eth1/eth2 etc we would have a clean serialization declaration of every ethereum type and how it's serialized - other projects would declare their own serializers etc. The outcome of that approach is that we can unit-test the encoding/decoding of ethereum types without having to run an RPC server and do "live" communication - we can write test cases that take take as input "json" text and verify that the resulting objects are correct - to avoid regressions, this is the test suite that needs to be written and it can be written only once nim-json-serialization itself has been upgraded, and it would live in those projects specifically.
  • Once nim-json-serialization is updated and we have tested serializers in the eth projects that are accurate, the change in nim-json-rpc can be done - it will at that point be a small change. I believe at that stage it might be good to do it in an additive way, ie create new modules that can be used side-by-side with the existing std/json implementation, for ease of migration


export json, options, json_serialization

Json.createFlavor Eth1JsonRpc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change Eth1JsonRpc to JsonRpc.

@jangko
Copy link
Contributor

jangko commented Dec 13, 2023

And please set the target branch to v0.2.0 because we still need to refactor nim-web3 and nim-json-serialization before merging to master

@jangko jangko changed the base branch from master to v0.2.0 December 14, 2023 00:07
@jangko jangko merged commit 819b6fe into status-im:v0.2.0 Dec 14, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants