feat(keeper): handle JSON arguments in MsgCall

[gfanton]

depends on:

• [GnoVM] Implement JSON marshalling on gnoland.TypedValue #2029

This is an initial PR to initiate a discussion on a potential approach for managing JSON arguments in MsgCall.

I achieve this using Go2GnoValue and Gno2GoValue methods, along with UnmarshalJSON and MarshalJSON (need to find a way to make this work) methods from amino package.

This PR still WIP but I have set it to Ready For Review to discuss some points:

• Should we mark this as experimental, using feature flags for instance?
• Should we use named arguments in a single JSON string request or an array of (JSON) strings or handle both.
  For example, for a method like: func(argsA []int, argsB string), we would have:
  • single string: Args: string(`{"ArgsA": ["2", "3"], "ArgsB": "hello"}`)
    vs
  • array of strings: Args: []string{["2", "3"], "hello"}`?
    EDIT: using array of args is best suited, named argument can be done on the client side
• How should we handle unexported fields? for now, returning a struct can cause Gno2GoValue to panic because reflect cannot set unexported fields. Ignoring these fields should be the logical approach, but we will need to find a way to avoid reflection on unexported fields in Gno2GoValue. I've updated gno2GoValue to skip unexported fields. Not sure if that's the best approach.
• DDoS and Perfomance concern ?

UPDATE:

• I have to fallback on golang std json instead of amino.
  • The main reason is that I'm not able to use amino.Marshal on unregistered struct. if anybody have a workaround, I can to put it back
• Stack overflow can occur with a recursive struct. However, the VM crashes before reaching the Unmarshal method if a method returns one.
• I have updated runMsg so that when multiple messages are sent, the response data is now split by a new line (\n) for each message reply.
• I updated the method Call to return only its value without including CPU cycles. This change aligns with expectations, and CPU cycles should be returned in the upper response if really needed.

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

[codecov]

Codecov Report

Attention: Patch coverage is 53.00000% with 47 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gno.land/pkg/sdk/vm/convert.go	45.09%	24 Missing and 4 partials ⚠️
gno.land/pkg/sdk/vm/keeper.go	68.75%	5 Missing and 5 partials ⚠️
gno.land/pkg/sdk/vm/convert_json.go	47.05%	8 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[notJoon]

• Should we use named arguments in a single JSON string request or an array of strings (like we currently have with args) or handle both:

I prefer to use a JSON string instead of an array of strings for input. Because when types overlap, the sequence in the array might unintentionally change the assignment of keys, leading to an incorrect order of values (I'm not sure if this can actually happen).

However, the main issue with using JSON strings is that they are more complex and need to be validated to ensure proper format.

Nonetheless, the current method is seems quite effective for encoding and decoding. I attempted to implement marshaling and unmarshaling in p/demo, but found it less efficient than using encoding/json. It also led to excessive and unnecessary unsafe boilerplates just to interpret the data structure correctly.

[deelawn]

I agree with @notJoon -- one string per argument, with that string being JSON for non-primitive types. Here are two other things I noticed while playing around with this:

1. the recent change to unquote txtar test args means that this feature won't work for them because the quotes will be removed
2. amino requires that numeric types also be in quotes but the resulting numeric fields are not quoted -- an inconsistent behavior from a user's point of view

[gfanton]

@deelawn

the recent change to unquote txtar test args means that this feature won't work for them because the quotes will be removed

Hmm nice catch, but I think it should be possible if you use a single quote or manually escape double quotes within the double quotes, but that can be a bit tedious.
I will make some tests.

amino requires that numeric types also be in quotes but the resulting numeric fields are not quoted -- an inconsistent behavior from a user's point of view

As I mentioned in the body of this PR, I wasn't able to use Amino for the reply because I have to register the Reply 'Type' dynamically to Amino, and I didn’t find a way to do that (yet). If anybody knows how to do that, it would be awesome.

[deelawn]

@gfanton I was just referring to how it is odd that if I have a struct type arg:

type S struct {
    A int
}

I can't pass in a string like {"A": 5}. Due to amino's restrictions, the value must be quoted -- {"A": "5"}. It might be possible to add another step to alter all field tags to include json:",string" so that all values can be transformed before unmarshaling using amino, but it might be better to require everything to be quoted and make a note of that somewhere.

[ajnavarro]

LGTM. The only thing I wonder is: How are we serializing byte arrays? how do we know the real number field type (int8 or int64)?

[gfanton]

After the review meeting, the team decided that need to implement dedicated amino (Un)Marshaling before merging this. This PR will be on hold until the feature is implemented.

[moul]

By the way, having this amino marshaller will be helpful for std.Emit as well (#1653 or a newer version).

[ltzmaxwell]

potentially related to #473

[jefft0]

Depends on #2113

[github-actions]

This PR is stale because it has been open 3 months with no activity. Remove stale label or comment or this will be closed in 3 months.