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

Fix u64 deserialization #705

Open
litchipi opened this issue Mar 21, 2024 · 6 comments
Open

Fix u64 deserialization #705

litchipi opened this issue Mar 21, 2024 · 6 comments
Labels
A-parse Area: Parsing TOML A-serde Area: Serde integration C-bug Category: Things not working as expected

Comments

@litchipi
Copy link

Past issue: toml-rs/toml-rs#256

u64 cannot be deserialized as it's backed by a i64 format

This causes this to fail:

let data = format!("id = {}", u64::MAX);
let val : toml::Value = toml::from_str(&data).unwrap();
println!("{:?}", val);

And any number > i64::MAX will also fail.

@epage epage added C-bug Category: Things not working as expected A-parse Area: Parsing TOML A-serde Area: Serde integration labels Mar 21, 2024
@epage
Copy link
Member

epage commented Mar 21, 2024

This deals with problems similar to #540

@litchipi
Copy link
Author

Indeed, I hacked a bit around, and it just makes sense to have a Number(i64) variant to cover the everyday usage.
We could imagine some kind of variants for larger numbers, but the toml::Value enum would be much more of a burden (many variants to take care of).

I guess the best way would be to have:

enum Value {
   // other variants ...
   Number(NumberValue),
}

enum NumberValue {
  Unsigned(u64),
  Signed(i64),
  BigUnsigned(u128),
  BigSigned(i128),
  Float(f64),
}

impl TryInto<u64> for NumberValue {
  // If the variant matches, get the value, else raise an error
}

It forces an extra step to get a usable number from the toml::Value, but it covers all kind of numeric types we could want.
Seams to me like a good middle ground

@jaskij
Copy link

jaskij commented Jul 25, 2024

I'd say, a feature flag, and then simply use something like num_bigint internally.

This allows for flexibility, while not penalizing users who don't care things larger than i64. Could even be relatively simple to implement, as that crate already has serde support.

@litchipi
Copy link
Author

Feature flag absolutely makes sense here indeed.
I may start to work on it later next week

@epage
Copy link
Member

epage commented Jul 25, 2024

Feature flag was covered as one of the potential solutions in #540 but it isn't "free"

Use a feature flag for controlling this, requiring wrapping the numbers in a newtype to avoid this being a breaking change (one dependency enabling a feature shouldn't break other dependencies) which both adds implementation complexity and complexity on the users side

There are two layers we are talking about, toml which provides higher api stability serde support and toml_edit which directly exposes the parsed results. toml_edit would need to stop exposing the numeric type directly and instead provide a newtype for it.

@jaskij
Copy link

jaskij commented Jul 26, 2024

Ah, I was unaware there would be so much change. This does sound more difficult then. It would also mean adding the feature flag in toml_edit.

I still feel like a feature flag is the best solution here, but it will be a decently big undertaking, involving breaking changes in both crates.


After some recent changes on my end, it has become a non-issue for me.

In case somebody was curious about my use case: I used toml to directly transform the TOML into Protobuf messages using prost and prost-serde. One place in our protocol sends the first eight bytes of git SHAs using fixed64 (u64).

The use case changed, and I need to use an intermediate type for reading TOML. Since I need to add a conversion, I may as well add u64::parse in there.

github-merge-queue bot pushed a commit to foundry-rs/starknet-foundry that referenced this issue Nov 4, 2024
<!-- Reference any GitHub issues resolved by this PR -->

Closes #2245

## Introduced changes

<!-- A brief description of the changes -->

- Added new enum type `Input` with variants `String` and `Number(i64)`
to support both string and numeric inputs in TOML files
- Updated `DeployCall` and `InvokeCall` structs to use `Vec<Input>`
instead of `Vec<String>` for their `inputs` field
- Modified `parse_inputs` function to handle both string and numeric
input variants
- Added `#[serde(untagged)]` attribute to enable seamless
deserialization of both input types from TOML

## Checklist

<!-- Make sure all of these are complete -->

- [x] Linked relevant issue
- [x] Updated relevant documentation
- [x] Added relevant tests
- [x] Performed self-review of the code
- [x] Added changes to `CHANGELOG.md`

## Other notes
- Numbers larger than 2^64 are not supported by TOML format:
https://toml.io/en/v1.0.0#integer
- However, numbers outside i64 range cannot be supported currently due
to an [issue](toml-rs/toml#705) in the toml
parser

---------

Co-authored-by: Franciszek Job <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parse Area: Parsing TOML A-serde Area: Serde integration C-bug Category: Things not working as expected
Projects
None yet
Development

No branches or pull requests

3 participants