-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
…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>
json.Number
, not float64
json.Number
, not float64
There was a problem hiding this 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.
@peterbroadhurst Did you mean |
Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
I've added some extra checks to my new unit test (with both 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. |
This is great. No concerns from me. (They'd be better off using types like |
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()
withUseNumber()
within the existingfftypes.Unmarhsal()
function to treat large numbers asjson.Number
, notfloat64
. This does not affect unmarshalling strings or smaller size integers.