make request and response ID handling spec-compliant. #83
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The server part of this library only supports int64 IDs. This breaks the JSON-RPC 2.0 spec, which establishes that: > An identifier established by the Client that MUST contain a String, Number, or NULL value if included. > If it is not included it is assumed to be a notification. > The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]. This patch makes the implementation compliant by making the ID field a wildcard type, and validating and coercing the ID to the respective type when receiving a message.
magik6k
approved these changes
Nov 3, 2022
| return clientResponse{}, xerrors.Errorf("failed to response ID: %w", err) | ||
| } | ||
|
|
||
| if resp.ID != cr.req.ID { |
There was a problem hiding this comment.
I'm not 100% sure that comparing interface{} things this way is actually safe
There was a problem hiding this comment.
Should be ok. From the Go spec:
Interface values are comparable. Two interface values are equal if they have identical dynamic types and equal dynamic values or if both have value nil.
magik6k
approved these changes
Nov 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The server part of this library only supports int64 IDs. This breaks the JSON-RPC 2.0 spec, which establishes that:
This patch makes the implementation compliant by making the ID field a wildcard type, and validating and coercing the ID to the respective type when receiving a message (yikes, really missing Rust tagged enums here).
I'm not too happy about the
interface{}map key types as they are brittle, but I believe this logic deals only with values a never pointers, so we should be good.Note that the proxy client here will always generate int64 request IDs; unfortunately client logic does have to change because it depends on shared types.
This is breaking compatibility with some prominent Ethereum JSON-RPC API clients of Lotus, like MetaMask, which sends string-encoded UUID request IDs.
TODO
This patch is missing a server integration test validating the newly supported ID types.