Enable whitelisted Stargate Query with determinism #7
Conversation
mattverse
requested review from
ValarDragon and
p0mvn
and removed request for
ValarDragon
|
@mattverse will this PR enables queries for osmosis's module as well? |
|
@iboss-ptk We can enable it via bindings. Rn I'm trying to move these proto layers into Osmosis, while keeping these inits in the wasmd layer empty! |
|
Update: Leaving the initialization whitelisted stargate queries empty so that we can also get this upstremed |
x/wasm/keeper/query_plugins.go
Outdated
| return func(ctx sdk.Context, msg *wasmvmtypes.StargateQuery) ([]byte, error) { | ||
| return nil, wasmvmtypes.UnsupportedRequest{Kind: "Stargate queries are disabled."} | ||
| return func(ctx sdk.Context, request *wasmvmtypes.StargateQuery) ([]byte, error) { | ||
| binding, whitelisted := StargateLayerBindings.Load(request.Path) |
There was a problem hiding this comment.
Can we call this binding? I feel like the semantics are different
In my eyes, bindings are API abstractions that we implement on top of the existing message server and querier.
On the other hand, this seems like a query response to me.
Please correct me if I'm wrong
| binding, whitelisted := StargateLayerBindings.Load(request.Path) | |
| response, whitelisted := StargateLayerBindings.Load(request.Path) |
There was a problem hiding this comment.
I named it binding because imo it is the layer of API abstraction, as it returns an interface to marshal into, not the response itself. Lmk what you think
|
Note that this PR is different from osmosis-labs/osmosis#2353 in the sense that we don't json marshal the responses here |
Why do we need another |
This is a very good question, this PR serves its main focus for upstream, I wasnt so sure if we want to enforce using the response by json unmarshalling as we did in our osmosis repo. On the second thought, I do think that providing json marshalled responses is how it should be supported in the upstream as well, changed this PR to sync with the logic in Osmosis |
|
@mattverse Do you mind making this PR against https://github.com/osmosis-labs/wasmd/tree/v0.28.0-osmo-v12? We will use that branch for the v12 release. It is made off of https://github.com/CosmWasm/wasmd/releases/tag/v0.28.0. In addition, it changes the SDK and iavl dependencies to our forks. This is necessary to avoid dependency conflicts when updating Once upstream releases the whitelist change that we need, we can switch to using that instead |
|
@p0mvn actually, since we have osmosis-labs/osmosis#2353 on Osmosis, I don't think we need to maintain a separate commit for wasmd fork for Osmosis. + we have a separate PR for this on upstream. Thius I think this PR should be good to close. wdyt about this idea? |
Sounds good. One concern is I wonder if they have time to review, approve and release it in the upstream before we need it. Maybe let's avoid deleting this branch for now so that we can fallback to it in the worst case |
|
Same PR going upstream, closing here |
What is the purpose of the change
We have a history of disabling stargate queries, as some of the queries were non-deterministic to run on the state machine. (cref: CosmWasm#812).
This PR introduces re-enabling stargate queries whilst ensuring determinism using protos and bindings. (Orignial idea was from terra-money/classic-core#738, but most are modified as they have a different structure of wasmd from current wasmd)
Testing
Further plans to test this is TBD(upon concept ACK of this approach), although I do not have exact plans to test this in a local machine as of rn.