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

workaround serde deserializing incorrectly when arbitrary_precision enabled #521

Merged
merged 6 commits into from
Nov 22, 2019

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Nov 21, 2019

serde deserializes incorrectly when arbitrary precision is enabled.

fixes #520

The workaround is to deserialize into Value before ClientResponse. This is gated under the feature arbitrary_precision in core-client and transports.

serde deserializes incorrectly when arbitrary precision is enabled.
Fix by first deserializing into `Value` before `ClientResponse`
@parity-cla-bot
Copy link

It looks like @insipx signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@insipx insipx requested a review from tomusdrw November 21, 2019 12:07
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm! Would be cool to get an integration test for this.

@insipx
Copy link
Contributor Author

insipx commented Nov 21, 2019

there is now a generic fn serde_from_str which checks for cfg!(feature = "arbitrary_precision").

Also made sure that cargo test --feature arbitrary_precision works

@insipx insipx requested a review from tomusdrw November 21, 2019 19:03
@@ -81,7 +81,7 @@ impl RequestBuilder {
pub fn parse_response(
response: &str,
) -> Result<(Id, Result<Value, RpcError>, Option<String>, Option<SubscriptionId>), RpcError> {
serde_json::from_str::<ClientResponse>(&response)
jsonrpc_core::serde_from_str::<ClientResponse>(response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we now get rid of serde_json dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

serde_json is still used for all to_string serializations

core/src/lib.rs Outdated Show resolved Hide resolved
Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
@insipx insipx merged commit 591e916 into paritytech:master Nov 22, 2019
koushiro added a commit to chainx-org/ChainX that referenced this pull request Sep 17, 2020
JSON serialization/deserialization of `i128`/`u128` can only be supported when the `arbitrary_precision` feature enabled.

But `arbitrary_precision` numbers don't work with `serde(tag = ..)` and `serde(flatten)` syntax (serde-rs/json#505), as a result, the deserialization of structures in `jsonrpc-core` will be error.

Fortunately, the crate `jsonrpc-core` provides a workround(paritytech/jsonrpc#521).
Although this workround will cause more extra serialization/deserialization work, but I think it's worth it.

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
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.

[jsonrpc-client-transports] jsonrpc does not deserialize if user has "arbitrary_precision" enabled in serde
3 participants