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

Add support for 256 bit JSON numbers (vs. strings) #147

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Aug 27, 2024

This PR is related to firefly-signer PR hyperledger/firefly-signer#76 and it provides support for large JSON numbers being handled by FireFly.

Most usage of Ethereum (and this library) includes use of large integers greater than 64bits in size.
For example ERC-20 tokens are often deployed using 18 digits of precision so "1 token" is 1000000000000000000.

The FireFly APIs are designed for this, and support supplying this data as strings in JSON, using decimal or hex numeric form. This is the safest way to pass information through JSON as many code implementations in various languages (including Golang by default) do not support serialization/parsing of JSON numbers larger than the safe Javascript maximum (2^53-1) or the safe float64 maximum. They also might lose precision on serialization/de-serialization.

However, the JSON number notation is capable of representing large numbers without the loss of precision.

FireFly currently limits JSON number input params to 64 bits in size. For Ethereum data types such as uint256, this prevents you being able to invoke/deploy smart contracts with params up to max-size(uint256).

So this issue is very specific to the use of JSON number (not JSON string) parameters

This PR uses json.NewDecoder() with UseNumber() within the existing fftypes.Unmarhsal() function to treat large numbers as json.Number, not float64. This does not affect unmarshalling strings or smaller size integers.

…f a big int instead

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001 matthew1001 changed the title Large num test Unmarshal large JSON numbers as json.Number, not float64 Aug 27, 2024
@matthew1001 matthew1001 changed the title Unmarshal large JSON numbers as json.Number, not float64 Add support for 256 bit JSON numbers (vs. strings) Aug 27, 2024
@matthew1001 matthew1001 marked this pull request as ready for review August 27, 2024 15:41
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has a potentially wide big scope due to the generic nature of the JSONAny type, so I've been 🤔 hard about it.

The one case I believe that would be broken by this is:

var myVariable int64
myStruct.JSONAnyField.Unmarshal(ctx, &myVariable)

Before that would have worked, but I wonder now if Go will complain?

Either way, I'm convinced we can go forwards with this one.

@EnriqueL8
Copy link
Contributor

@peterbroadhurst Did you mean float64 instead of int64 given the UseNumber only applies to float64

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
@matthew1001
Copy link
Contributor Author

matthew1001 commented Aug 28, 2024

This change has a potentially wide big scope due to the generic nature of the JSONAny type, so I've been 🤔 hard about it.

The one case I believe that would be broken by this is:

var myVariable int64
myStruct.JSONAnyField.Unmarshal(ctx, &myVariable)

Before that would have worked, but I wonder now if Go will complain?

Either way, I'm convinced we can go forwards with this one.

I've added some extra checks to my new unit test (with both int64 and float64 variable types for completeness). Interestingly the decoded.Decode() behaviour with UseNumber() enabled appears to be to silently accept the precision loss and let you unmarshal to a float64. I'm not sure this is helpful so I've added an explicit check and a new error message if you attempt to unmarshal to a float64.

If you feel the better behaviour would be the previous behaviour of accepting the unmarshal request and potentially losing precision I'm happy to remove my new error.

@peterbroadhurst
Copy link
Contributor

This is great. No concerns from me.
I don't have any knowledge of a particular consumer of this library that will be affected, and it's good news that if they exist they'd be very unlikely to notice a very subtle change.

(They'd be better off using types like ethtypes.HexUint64 than a straight int64 or float64)

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