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

Shelley genesis use serde json arbitrary precision #363

Merged

Conversation

ecioppettini
Copy link
Contributor

I can't really explain why, but I've been having some weird issues with the deserialize_string_from_number from serde-aux.

It seems to work when I run the tests in this repo, but it fails when I use it from carp (but it didn't at some point, which is the weird part).

The function itself uses this code inside

    #[derive(Deserialize)]
    #[serde(untagged)]
    enum StringOrNumber {
        String(String),
        Number(i64),
        Float(f64),
    }

Which I think shouldn't work in the first place (serde-rs/serde#2661 (comment))

So I think it's better to just use https://github.com/serde-rs/json/blob/1faf3a1db661c7fb1f3a9a141efa30fb67e9ab3a/Cargo.toml#L70

I also think the current code doesn't do what the comments imply. Numbers get deserialized as strings if you put quotes around them in the json, in which case it is lossless, but otherwise they are just parsed as floats (or i64), which introduces an error anyway before making it a fraction. This probably doesn't matter as much since no one is really using these fields, but probably best to avoid the issue.

The only problem with this is that the Fraction type doesn't support parsing scientific notation, so I did implement that manually here.

@ecioppettini ecioppettini merged commit c6a6d11 into develop Oct 1, 2024
1 check passed
@ecioppettini ecioppettini deleted the shelley-genesis-use-serde-json-arbitrary-precision branch October 1, 2024 17:43
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.

3 participants