Serde_json arbitrary precision

[717a56e1]

jsonrpc had a feature flag to allow arbitrary_precision, this library currently breaks if it is built with it. Could be we open to adding a similar serde_from_str override?

[dvdplm]

Yes, I think we need to add that. It's not pretty but I don't see a better way at the moment.

For other readers, here's some context:

• https://github.com/serde-rs/json/issues/505 describes the problem and suggests the work-around that is used in jsonrpc.
• the likely underlying cause in serde: https://github.com/serde-rs/serde/issues/1183
• at the deep end of the rabbit hole we have the "Associated type defaults" RFC and all the fall-out from that

@717a56e1 Are you open to contributing a PR?

[niklasad1]

I agree but IIRC this affects mainly u128/i128 and floats but comes with extra costs.

[717a56e1]

Yup, got it running with a patched from_slice, should get up PR up. Any place we'd like to put the function? I've got it in types/lib.rs.

[dvdplm]

Probably better off in jsonrpsee-utils I think. @niklasad1 wdyt?

[niklasad1]

I don't know really because we re-export serde stuff from types so a utility function in types probably makes most sense but I need to see to code first before saying yes or no :)

Either should be fine I think...

[niklasad1]

Hey, I have investigated arbitrary precision feature in serde. It defeats our zero-alloc parsing of RpcParams and it's really inefficient in practice one would have to do json_str -> serde_json::Value -> T

Further, as the Request/Response types in jsonrpsee doesn't use #[serde(untagged/flatten)] those will work anyway but there are two cases where arbitrary precision are needed at to moment:

1. custom types as "JSON-RPC params" or "JSON-RPC result" that have #[serde(untagged/flatten)] with u128/u128 as field (no way to mitigate those)
2. ParamsSer that is currently only used by the client but could be fixed.

Thus, the server supports plain u128/u128 but not in combination with #[serde(untagged/flatten)]. I regard this as a serde issue and I prefer not support it because it's so much slower but if more people show interest of this I'm willing to add it behind a feature flag.

[niklasad1]

@717a56e1 you can explain your use-case it would help us to understand the motivation behind this?

[717a56e1]

I've been using custom types with u128 and serde untagged, performance isn't too critical in my use case. I've been running a branch with the custom headers and the double deserialization hack (behind a feature gate).

It has been working fine on websockets (haven't really used http side of the crate but assume the deserialization logic is common).

I'm still fine opening a pull request for this, we had run into some strange behavior with serde untagged (specifically with u128) and was hoping to sort that out before opening the PR.

Realistically at this point I see my use case moving away from untagged towards custom Deserialize impls until untagged is stabilized upstream.

[717a56e1]

Perhaps a bit more context, I've rebased and applied the changes more consistently. jsonrpsee tests break quite broadly because of the induced allocations, i.e. thread 'v2::request::test::deserialize_valid_notif_works' panicked at 'called Result::unwrap() on an Err value: Error("invalid type: string \"say_hello\", expected a borrowed string", line: 0, column: 0)', types/src/v2/request.rs:149:71

Also, ID deserialization fails with arbitrary_precision enabled (since it's an untagged enum) without the deserializing to value first, even though none of the types are u128/i128.

[niklasad1]

ah, I see

thread 'v2::request::test::deserialize_valid_notif_works' panicked at 'called Result::unwrap() on an Err value: Error("invalid type: string \"say_hello\", expected a borrowed string", line: 0, column: 0)', types/src/v2/request.rs:149:71

I think we need put the method names in Cow<'a, str> instead of &'a str for that to work, similar to what we did in https://github.com/paritytech/jsonrpsee/pull/578. I can fix that for in master.

Also, ID deserialization fails with arbitrary_precision enabled (since it's an untagged enum) without the deserializing to value first, even though none of the types are u128/i128.

can you share the error? I think it might a similar reason.

[717a56e1]

Sure thing. Let me park this on a branch as well: https://github.com/717a56e1/jsonrpsee

thread 'v2::params::test::id_deserialization' panicked at 'called `Result::unwrap()` on an `Err` value: Error("data did not match any variant of untagged enum Id", line: 0, column: 0)', types/src/v2/params.rs:376:56

[niklasad1]

alright that is expected I realize now with arbitrary_precision serde can't deduce that Number variant is u64.

We might consider to a handwritten deserialize impl for Id and SubscriptionId instead then.

[niklasad1]

Hey @717a56e1 again,

We have been discussing this internally and came to the conclusion not support this because it's really a "serde-json bug" and defeats the zero-alloc serialization/deserialization with serde_json::RawValue, "we" (parity) tried to get https://github.com/serde-rs/json/pull/586 merged but got rejected so please work with serde to get this fixed properly.

In addition it can break things because it's a feature flag so it will be enabled for anything that depends on serde (note if anyone enabled --feature arbritary-precision on serde-json this might end up with weird scenarios but nothing we can do in this crate to protect against that)

We might revisit this in the future but it's not really anything we will work on to support any time soon so closing this.

[717a56e1]

@niklasad1 Any interest in getting arbitrary-precision working without serde flatten/untagged? I see you've gotten rid of most of the uses of untagged besides ParamsSer, and for my use-case I've given up on those macros until they're fixed upstream.

If so I can open another issue/PR.

[niklasad1]

Go ahead and open a PR so I understand what you mean, so it depends how clean it will be :)