-
Notifications
You must be signed in to change notification settings - Fork 18
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
Decode numbers using json.Decoder #148
Conversation
Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
581ccfd
to
9656714
Compare
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.
Looks good - just one comment on testing with FireFly Signer and understanding if not only the number is there in RLP but also the things around it like where the number starts
@@ -95,7 +96,9 @@ func (c *ethConnector) prepareDeployData(ctx context.Context, req *ffcapi.Contra | |||
ethParams := make([]interface{}, len(req.Params)) | |||
for i, p := range req.Params { | |||
if p != nil { | |||
err := json.Unmarshal([]byte(*p), ðParams[i]) | |||
decodedParam := json.NewDecoder(bytes.NewReader([]byte(*p))) | |||
decodedParam.UseNumber() |
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.
From what I can read this stores the number in the string representation, have you checked that FireFly Signer supports this?
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.
Just thinking of how the RLP encode code there would treat this
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.
So it feels like it was just going wrong before it hit FireFly Signer https://github.com/hyperledger/firefly-signer/blob/5256116539ed83a23a52103f6260f4887abe0e85/pkg/abi/abiencode.go#L181
decodedParam := json.NewDecoder(bytes.NewReader([]byte(*p))) | ||
decodedParam.UseNumber() | ||
err := decodedParam.Decode(ðParams[i]) |
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.
I think we need to fix the issue in https://github.com/hyperledger/firefly-common/blob/main/pkg/fftypes/jsonany.go#L51 if the issue is how data gets unmarshalled
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.
Agreed - and it probably needs a quick search of the code for other places where json.Unmarshal()
is used as well
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.
Going to prove out this fix with an end-to-end contract deploy first to check that the fix is good (beyond the UTs)
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.
Sorry @matthew1001 - I'm not sure I understand why there's a concern with json.Unmarshal()
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.
Ok - I do think I now understand, but the right fix (which I think would probably include json.Unmarshal
) would take time to investigate
Signed-off-by: Matthew Whitehead <[email protected]>
"outputs":[], | ||
"type":"constructor" | ||
}], | ||
"params": [ 10000000000000000000000001, "some-text" ] |
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 input is broken in JSON, it's attempting to use a number that does not fit into int64
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.
...
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.
ok - sorry, after some misunderstanding I now understand what this PR is trying to achieve.
This PR is an enhancement to support:
- JSON input in the
params
array is able to contain JSONnumber
elements larger than fit into anint64
.
One part of the fix, is that Golang
provides an option on json.Decoder
(not available on json.Unmarshal
) to decode number
s into json.Number
types (which are strings
).
If that is done in this one piece of the stack, then it has implications for other places that need to be reviewed:
firefly
firefly-signer
With this done, a user choosing to generate a JSON input containing JSON number
> that is possible to generate in Javascript/Java/Go serialization of numbers could be supported by the stack.
Thanks for the comments @peterbroadhurst, @Chengxuan, @EnriqueL8 - I might grab some in-person time with @EnriqueL8 to discuss options & other places in the stack that will be affected. I'll circle back round on this PR once I've done that. |
This PR has provoked some useful discussion around large number support. After some discussions I'm closing it in lieu of: |
If the
params
passed in for a contract deploy include very large numbers,json.Unmarshal
to aninterface{}
doesn't work and results in an incorrect number being passed in to the contract's constructor.To recreate the issue, if you checkout just the UT changes from this branch and run them it will fail.
This PR switches
json.Unmarshal
tojson.Decode
.