Resolve JSON RPC 2.0 non-compliance (redo) #3213
Conversation
Currently, the JSON RPC requests and responses are not compliant with the spec as the server only accepts strings. This change is *non-breaking* as it simply adds the ability to understand integers as expected. Any old code continuing to only send string requests will continue to work as before. Unit tests are included to ensure (de)serialization works on both strings and integers.
There was a problem hiding this comment.
Thank you for the PR @WhoNeedszZz. Tested locally and looking good. LGTM.
|
Not to open a big can of worms but wondering if there is a more https://serde.rs/impl-deserialize.html#the-visitor-trait
I think the |
| @@ -501,7 +510,7 @@ impl Handler { | |||
| // Issue #1159 - use a serde_json Value type to avoid extra quoting | |||
| let job_template_value: Value = serde_json::from_str(&job_template_json).unwrap(); | |||
| let job_request = RpcRequest { | |||
| id: String::from("Stratum"), | |||
| id: JsonId::StrId(String::from("Stratum")), | |||
There was a problem hiding this comment.
To my point about an additional explicit layer - we need to remember to wrap values like this.
There was a problem hiding this comment.
One way would be to use the #[serde(with = "secp_ser::string_or_u64")] like grin-wallet does.
https://github.com/mimblewimble/grin-wallet/blob/master/libwallet/src/slate_versions/v3.rs#L60
Currently, the JSON RPC requests and responses are not compliant with the spec as the server only accepts strings.
This change is non-breaking as it simply adds the ability to understand integers as expected.
Any old code continuing to only send string requests will continue to work as before.
Unit tests are included to ensure (de)serialization works on both strings and integers.
Resolves #2149