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 QueryRequest::Grpc #1966

Closed
chipshort opened this issue Nov 27, 2023 · 9 comments · Fixed by #1973
Closed

Add QueryRequest::Grpc #1966

chipshort opened this issue Nov 27, 2023 · 9 comments · Fixed by #1973
Milestone

Comments

@chipshort
Copy link
Collaborator

We want this to be an improved version of the QueryRequest::Stargate query.

Some of the pain points of the current Stargate query:

  • As far as I can see, responses seem to sometimes be json, sometimes protobuf, even though the docs clearly specify protobuf. I wonder if there is some way to enforce this, since the responses are defined by the chains
  • The use of protobufs (as opposed to json like everywhere else) seems to regularly confuse contract devs who try to pass json in there. It would be good to make this easier somehow.

We could add something like a ProtobufBytes wrapper type that does some basic validation.
With that we could solve both points. Alternatively, for the first point, that validation could be done in wasmd to not burden the contract code with it.
However, the tricky part here is: How much can even be validated without knowing the concrete protobuf type? Does it become easier if we require all of them to by Any types?

@chipshort chipshort added this to the 2.0.0 milestone Nov 27, 2023
@webmaster128
Copy link
Member

As far as I can see, responses seem to sometimes be json, sometimes protobuf, even though the docs clearly specify protobuf. I wonder if there is some way to enforce this, since the responses are defined by the chains

There is basic protobuf bytes validation you can do in let's say wasmvm or wasmd. It won't tell you much about the content but should be good enough to differentiate protobuf from JSON.

The use of protobufs (as opposed to json like everywhere else) seems to regularly confuse contract devs who try to pass json in there. It would be good to make this easier somehow.

This sense of "JSON everywhere" is changing and needs to change. A lot of advanced developers can't work without the Any message and need to parse protobuf message responses. Tooling-wise there are options:

  • Anybuf/Bufany
  • prost code generator
  • prost annotated types
    There is an ecosystem developing around the core functionalits. E.g. Osmosis has a standard library that allows you to have typed interfaces for the messages and queries relevant to their chain. It does the proto ser/de in Wasm wrapped in their lib.
    Step 1 is to have a correct interface. Making this easy to use will follow.

Another thing I was thinking about is the interface name and fields. This is not really an Any. It's a gRPC request which consists of a path to the service and a request body. This looks similar, but I'd use the "grpc" naming instead.

@chipshort
Copy link
Collaborator Author

This sense of "JSON everywhere" is changing and needs to change

Agreed. What I meant to say is that it would be nice to make the use of protobuf more obvious through types.
That way users would notice incorrect usage early on.

Osmosis has a standard library that allows you to have typed interfaces for the messages and queries relevant to their chain

That's exactly what I would hope to see! If we can get most chains to do this, then my point above will become less important. Maybe we can think about ways to make this easier for more chains in the future. Like code generation or something like that.

QueryRequest::Grpc sounds fine to me.

@webmaster128
Copy link
Member

That's exactly what I would hope to see!

This is somewhere in https://crates.io/crates/osmosis-std. Examples:

@Unique-Divine
Copy link

"As far as I can see, responses seem to sometimes be json, sometimes protobuf, even though the docs clearly specify protobuf"

What determines this? When are the responses from QueryRequest::Stargate JSON versus protobuf?

@webmaster128
Copy link
Member

What determines this? When are the responses from QueryRequest::Stargate JSON versus protobuf?

ConvertProtoToJSONMarshal introduced in CosmWasm/wasmd#1069 for chains using AcceptListStargateQuerier. If a custom replacement of AcceptListStargateQuerier, responses might be proto bytes depending on the implementation.

To quote the original author: Note that this PR does not simply revert CosmWasm/wasmd#812, this PR includes converting the state machine response from a proto response to a json marsahlled response so that clients can use it directly by json unmarshalling. (This is also why we now feed codec as a parameter in StargateQuerier in this PR).
This change was a mistake but it slipped through.

@webmaster128 webmaster128 changed the title Add QueryRequest::Any Add QueryRequest::Grpc Dec 4, 2023
@chipshort
Copy link
Collaborator Author

After looking into this, I am wondering if it would make more sense to keep this query separate from the other QueryRequests.
If we make this a QueryRequest variant, it should stay compatible with QuerierWrapper::query, which expects a json response. That would force us to convert the protobuf to json Binary (so, basically base64 encoding and decoding it), which seems unnecessary.

Instead, we could provide a separate function like QuerierWrapper::query_grpc that just returns the protobuf binary directly.
WDYT?

@webmaster128
Copy link
Member

All query responses are wrapped in two layers of result types. For the contract-contract query this made sense, but in many cases the double result is unnecessary overhead. I was expecting we just keep the structure for the sake of not opening up a can of worms. But maybe there is a way to route this through a separate channel. I'm open to ideas here.

@chipshort
Copy link
Collaborator Author

I didn't mean touching the result types originally, but I get what you mean now.
It's a bit unfortunate for passing binary around that the ExternalQuerier always expects json data.
To change that, we'd have to change the Querier trait in both cosmwasm-std and cosmwasm-vm.
We probably still want at least one level of error, so if we want to avoid json here completely, we'd have to have an alternative version of the query_chain import with different encoding or a separate error message.
Overall, that seem like a bit too much. I'm not sure it's worth that much effort.

@webmaster128
Copy link
Member

One way to look at this is: we can always add a new route later on to increase encoding efficiency if needed. Adding it in the current system (response binary embedded in 2 levels of JSON result structures) is straight forward.

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 a pull request may close this issue.

3 participants