-
Notifications
You must be signed in to change notification settings - Fork 173
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
types: make jsonrpc field optional #1385
Conversation
types/src/request.rs
Outdated
// Without params and ID. | ||
(r#"{"jsonrpc":"2.0","id":null,"method":"subtract"}"#, None, None, method), | ||
(r#"{"jsonrpc":"2.0","id":null,"method":"subtract"}"#, None, None, method, Some(TwoPointZero)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the idea of giving the version too so that you'd add a test which didn't have the "jsonrpc"
field? :)
Yeah; I wasn't 100% convinced (I mean, without the version, how do you know how to interpret a given request/response?). The spec does also say this:
Emphasis on "always" is mine |
The only way around that is that is doesn't specify "REQUIRED" for jsonrpc stuff but yeah I agree |
Yeah, I was initially agreeing with this PR on account of that, but maybe the spec just isn't quite being clear, or it's allowing it to be optional for implementations to handle 1.0 too (which doesn't have that field at all)? |
Close #1142
Previously, we made the interpretation that jsonrpc field must exist but recently #1045 explained to me that it could be interpreted as optional.
So, let's change it to optional all over jsonrpsee.
I'm not 100% convinced by the interpretation of request object but it doesn't bother me to support it for stuff without the jsonrpc header...