ProvableHQ / sdk

A Software Development Kit (SDK) for Zero-Knowledge Transactions
https://provable.tools
GNU General Public License v3.0
584 stars 468 forks source link

Ensure number precision on BigInts #901

Open WietzeSlagman opened 1 month ago

WietzeSlagman commented 1 month ago

Motivation

Solves #892 by utilizing json-bigint to parse incoming block data, allowing the integers that are too big in the web world to keep their precision and be workeable by developers.

Test Plan

Ran yarn test in sdk directory, all passed

Ran webapp using the updated SDK to verify block data and the following is observed fetching block 327674 where the target in a proposed solution is a known bigInt.

image

As shown in the above image several elements are now transformed into bigInts and have the correct precision when translated to forinstance a string. which looks like this on the solution: block.solutions.solutions.solutions[1].target.toString() = "2282730364275405" The following is returned from calling a node directly: "target": 2282730364275405 (as part of the solution). Which shows that the value now has the correct precision and the developer can use the bigInt to make calculations with.

Related PRs

N/A

WietzeSlagman commented 1 month ago

There is one point in this proposed solution that could be deemed not ideal: Several numbers are typed as for instance u64 in snarkVM which means they could become a too big number for the web to handle. However if the value is not yet too big to handle it is returned as a number, For example in the screenshot the target of solution[0] is parsed as a regular number (because it is not big enough to be a bigInt) however the target of solution[1] is. This leads to inconsistencies for devs and would lead to making extra checks to see which kind of operations they can do and how to parse values to display them correctly. To solve this each known bigInt should be parsed to a bigInt irregardles of its current size, allowing for the most consistency in terms of experience.

This issue i think should be solved in a seperate PR and get this in before, this ensures that developers are atleast working with the correct percision and the data is matching that which is present in the actual block.

Pauan commented 1 week ago

Thanks for the PR!

There's a few issues I see with using json-bigint:

Ideally we'd fix this on the SnarkOS side, so that the JSON uses strings for large numbers... but it's hard to change things in SnarkOS, so we might need a temporary stop-gap in the SDK...

It might be worthwhile to create a custom JSON parser that fixes the above issues.

Pauan commented 1 week ago

Note that because this is a breaking change, it will need to be done in the next major version bump, which should be soon! So we'll make sure to prioritize this issue to get it in for the next major version bump.

Pauan commented 1 week ago

I found this article about using native JSON.parse to parse BigInt:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON#using_json_numbers

JSON.parse(str, (key, value, context) => {
    if (Number.isInteger(value)) {
        return BigInt(context.source);

    } else {
        return value;
    }
})

This will cause all integers to be parsed as a bigint. We can also easily adjust it so that it only parses certain keys as bigint.

This seems preferable to using json-bigint, though as a downside it does parse the number twice.

WietzeSlagman commented 5 days ago

Thanks for the PR!

There's a few issues I see with using json-bigint:

  • Performance. JSON parsing is pretty slow, and I worry that json-bigint will make it even slower.
  • json-bigint includes a lot of extra code that we don't need, bloating up the code size.
  • As you mentioned, sometimes numbers are returned as bigint and sometimes they're not... that inconsistency needs to be fixed.
  • This is a breaking change, since existing SDK users will be expecting typeof "number", not typeof "bigint".

Ideally we'd fix this on the SnarkOS side, so that the JSON uses strings for large numbers... but it's hard to change things in SnarkOS, so we might need a temporary stop-gap in the SDK...

It might be worthwhile to create a custom JSON parser that fixes the above issues.

Thanks for the reply. i do agree it is some additional overhead but i wonder how big that impact actually is (in our applications i havent noticed a real impact yet).

This is indeed a breaking change that would require code updates for users so makes sense to put it in in a later date, however getting this fix to users quickly will help integrate them faster.

On your point of adding it to SnarkOS i am not sure if that is an ideal way to go, especially seeing that is an even bigger impact for users (as it not only affects JS/TS users but also Rust) as well as that several languages do handle this fine without needing the additional parse, this means you are making the experience and performance worse accross the board unnecesarily. The only way i would see this work in SnarkOS is if you would be able to provide a flag of how you want the data but that seems like a lat of engineering effort for little gain.

Pauan commented 5 days ago

On your point of adding it to SnarkOS i am not sure if that is an ideal way to go, especially seeing that is an even bigger impact for users (as it not only affects JS/TS users but also Rust) as well as that several languages do handle this fine without needing the additional parse

Do you know of any languages that parse JSON numbers as a bigint by default?

WietzeSlagman commented 5 days ago

On your point of adding it to SnarkOS i am not sure if that is an ideal way to go, especially seeing that is an even bigger impact for users (as it not only affects JS/TS users but also Rust) as well as that several languages do handle this fine without needing the additional parse

Do you know of any languages that parse JSON numbers as a bigint by default?

bigInt as a form is (almost) exclusively used within web and JS/TS frameworks, other languages would I assume parse it to either INT64 (or 32) or UINT if provided. It of course highly depends on the language implementation and packages used. My point with this is that most lower level languages have built in types for larger numbers and do not need something like a bigInt to be defined.

Pauan commented 5 days ago

bigInt as a form is (almost) exclusively used within web and JS/TS frameworks

Bigint originated outside of JavaScript, all the way back in the 1960s, it was only very recently that JavaScript received bigint support.

other languages would I assume parse it to either INT64 (or 32) or UINT if provided.

I would assume that most languages would parse it as 64-bit floating point, just like how JavaScript does.

I'm unaware of any languages which parse JSON numbers as integers, which is why I asked if you knew of any examples.

My point with this is that most lower level languages have built in types for larger numbers and do not need something like a bigInt to be defined.

Regardless of whether the language supports larger numbers or not, if the JSON parser for the language uses 64-bit floating points for JSON numbers, then it will have the exact same precision loss issues as JavaScript.

That's why it's common practice for APIs to return large numbers as strings, so that way it won't accidentally be parsed as a 64-bit floating point number.

WietzeSlagman commented 5 days ago

other languages would I assume parse it to either INT64 (or 32) or UINT if provided.

I would assume that most languages would parse it as 64-bit floating point, just like how JavaScript does.

I'm unaware of any languages which parse JSON numbers as integers, which is why I asked if you knew of any examples.

My point with this is that most lower level languages have built in types for larger numbers and do not need something like a bigInt to be defined.

Regardless of whether the language supports larger numbers or not, if the JSON parser for the language uses 64-bit floating points for JSON numbers, then it will have the exact same precision loss issues as JavaScript.

That's why it's common practice for APIs to return large numbers as strings, so that way it won't accidentally be parsed as a 64-bit floating point number.

You are correct that it al heavily depends on the JSON parser for any language implementation, I have no insights into the inter workings of these parsers and how they handles them.

Do note I am not opposed to the string change into SnarkOS, I just mentioned it as this could lead to bigger consequences that are effecting more than just the JS/TS SDK, and thus would lead to more change-management being needed as it is a bigger impactful change. From some parties that have used our API's I've noticed that they do have parsers implemented expecting actual number formated items and not string when it comes to those.

Pauan commented 5 days ago

I just mentioned it as this could lead to bigger consequences that are effecting more than just the JS/TS SDK, and thus would lead to more change-management being needed as it is a bigger impactful change.

Yes, of course, it is definitely a bigger change.