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

serde_json arbitrary_precision flag causes conflicts #2206

Closed
grigull opened this issue Feb 27, 2023 · 6 comments · Fixed by #2617
Closed

serde_json arbitrary_precision flag causes conflicts #2206

grigull opened this issue Feb 27, 2023 · 6 comments · Fixed by #2617
Labels
bug Something isn't working

Comments

@grigull
Copy link
Contributor

grigull commented Feb 27, 2023

Version
current master branch, this is NOT an issue with 1.0.2.

Description
In the following commit there was an update to use the serde_json arbitrary_precision feature. This feature has been known to cause problems with the serde_json macros as documented here, with the currently tracking issue here. What will happen is that anyone that uses the ethers package will see problems when attempting to use the tag untagged and flatten macros. The open issues mentioned show multiple examples of this. Below is an example that I have.

I tried this code:
[dependencies]
serde_json = { version = "1.0.64", default-features = false, features = ["arbitrary_precision"] }

    #[test]
    fn temp() {
        struct M {
            value: f32,
        }

        impl<'de> Deserialize<'de> for M {
            fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
            where
                D: serde::Deserializer<'de>,
            {
                #[derive(Debug, Deserialize)]
                #[serde(untagged)]
                enum TempM {
                    Single(f32),
                    Sum { one: f32, two: f32 },
                }

                match TempM::deserialize(deserializer)? {
                    TempM::Single(value) => Ok(M { value }),
                    TempM::Sum { one, two } => Ok(M { value: one + two }),
                }
            }
        }

        let input = r#"10.1"#;
        let result: M = serde_json::from_str(input).unwrap();
    }

I expect in the example above for the result to resolve to M { value: 10.1 }

Instead i get the following error:
panicked at 'called `Result::unwrap()` on an `Err` value: Error("data did not match any variant of untagged enum TempM", line: 0, column: 0)'

When disabling the arbitrary_precision flag this correctly resolves.

@grigull grigull added the bug Something isn't working label Feb 27, 2023
@allanbrondumkr
Copy link

@gakonst Is it possible to remove the feature arbitrary_precision from the serde_json dependency? I've looked a bit at StringifiedNumeric and deserialize_stringified_numeric_opt in serde_helpers.rs where it is used, and it seems more natural to rely on U256::from_dec_str directly than first deserializing to serde_json::Number and then calling U256::from_dec_str on to_string of that value.

@allanbrondumkr
Copy link

If serde_json::Number is not used there should be no need to use the feature arbitrary_precision.

@allanbrondumkr
Copy link

That did not make sense of course. For JSON number values U256::from_dec_str cannot be called directly. But maybe u128 would be sufficient as intermediate value instead of serde_json::Number?

@vdods
Copy link

vdods commented Aug 16, 2023

I'm hitting this as well. In particular, it's causing the canonicalization of JSON data for cryptographic signing to fail.

Is the serde_json/arbitrary_precision feature actually needed? In researching the problem, I've found a lot of Github issues describing de/serialization headaches in general dealing with use of the serde_json/arbitrary_precision feature.

@humb1t
Copy link

humb1t commented Sep 28, 2023

It also breaks usage of async-graphql async-graphql/async-graphql#1198

@mattsse
Copy link
Collaborator

mattsse commented Sep 28, 2023

yeah, this feature is kinda fucked and we should not force anyone into using it,

afaik this is only used for deserialization of u256 numbers, which should be fine to not support since most of the time they're either hex or fit in u64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants