Make felt_from_number report errors properly

[omerfirmak]

Make felt_from_number report errors properly

Description

fixed not reporting the parse error properly

Checklist

• Linked to Github Issue
• Unit tests added
• Integration tests added.
• This change requires new documentation.
  • Documentation has been added/updated.
  • CHANGELOG has been updated.

[Oppen]

Hi @omerfirmak, thanks for the PR. I'm not quite sure I understand it tho.
Is there a real world program that failed to parse due to lack of support for scientific notation?
Why is it converting the parsed input into a float? Do note this loses data. In fact, most of the prime field values lose a lot of information when doing this (everything above 2^64 loses low bits).

[omerfirmak]

Hi @omerfirmak, thanks for the PR. I'm not quite sure I understand it tho. Is there a real world program that failed to parse due to lack of support for scientific notation? Why is it converting the parsed input into a float? Do note this loses data. In fact, most of the prime field values lose a lot of information when doing this (everything above 2^64 loses low bits).

for example

https://alpha-mainnet.starknet.io/feeder_gateway/get_class_by_hash?classHash=0x3297a93c52357144b7da71296d7e8231c3e0959f0a1d37222204f2f7712010e

search for starkware.starknet.common.syscalls.CALL_CONTRACT_SELECTOR

Do note this loses data. In fact, most of the prime field values lose a lot of information when doing this (everything above 2^64 loses low bits).

I wasnt aware, any alternatives that won't be causing any data loss?

[Oppen]

search for starkware.starknet.common.syscalls.CALL_CONTRACT_SELECTOR

This is the only match I found, it's not in scientific notation here:

"starkware.starknet.common.syscalls.CALL_CONTRACT_SELECTOR": {"value": 20853273475220472486191784820, "type": "const"}

I wasnt aware, any alternatives that won't be causing any data loss?

I can see if I find some arbitrary precision decimal crates that we can use. Alternatively, I guess there's also the option of storing the mantissa as a separate number (would usize be enough?) and the digits as a BigUint or some scheme like that.

[omerfirmak]

This is the only match I found, it's not in scientific notation here:

Oh damn, it must be my browser messing up the value somehow when pretty printing. If I dont pretty print it is a plain integer.

Sorry for the confusion.

[omerfirmak]

Dropping the scientific notation part of this PR and keeping the proper error reporting.

[Oppen]

We would need to fix the changelog. The Hyperfine workflow failing is just a matter of permissions, it can't post the comment when the PR comes from outside this repo (it's a limitation of the action we use), so it doesn't matter. I checked manually that was the issue.

[codecov]

Codecov Report

Merging #1012 (fbac71d) into main (92a81f7) will decrease coverage by 0.01%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main    #1012      +/-   ##
==========================================
- Coverage   97.71%   97.71%   -0.01%     
==========================================
  Files          89       89              
  Lines       35725    35737      +12     
==========================================
+ Hits        34908    34919      +11     
- Misses        817      818       +1     

Impacted Files	Coverage Δ
src/serde/deserialize_program.rs	97.41% <92.30%> (-0.07%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[Oppen]

It also seems like we're missing a test case hitting the error.

[Oppen]

BTW @omerfirmak let me know if you need help with the CHANGELOG. The format is a bit ad-hoc.

[Oppen]

Hi @omerfirmak. We'll need you to run cargo fmt on the PR and solve conflicts with the changelog. After that it's ready to go.

[omerfirmak]

Hi @omerfirmak. We'll need you to run cargo fmt on the PR and solve conflicts with the changelog. After that it's ready to go.

Done

[omerfirmak]

This in-repo CHANGELOG so annoying, it makes PRs race against each other to get merged first. Otherwise you get conflicts.

Commits and their messages are changelogs anyways, I suggest you to try enforcing better commit messages instead of an in-repo changelog.

[Oppen]

This in-repo CHANGELOG so annoying, it makes PRs race against each other to get merged first. Otherwise you get conflicts.

Commits and their messages are changelogs anyways, I suggest you to try enforcing better commit messages instead of an in-repo changelog.

Yes. I personally want to go with that approach, but we try to keep things democratic in the team so I need to make my point more convincing before doing that, as there may be some drawbacks (say, whatever we use to generate the file afterwards should give some freedom to edits, otherwise it may not be great to read). This input actually helps for that :)

Do note even then the changelog would be in-repo, just not manually updated on each PR. Relying on commit messages for end users would not be great UX/DX.