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]>
matthew1001
force-pushed
the
json-numbers
branch
from
581ccfd
to
9656714
Compare
| @@ -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.
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.
Just thinking of how the RLP encode code there would treat this
There was a problem hiding this comment.
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.
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.
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.
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.
Sorry @matthew1001 - I'm not sure I understand why there's a concern with json.Unmarshal()
There was a problem hiding this comment.
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.
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.
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
paramsarray is able to contain JSONnumberelements 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 numbers 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:
fireflyfirefly-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
paramspassed in for a contract deploy include very large numbers,json.Unmarshalto 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.Unmarshaltojson.Decode.