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

Enable whitelisted Stargate Queries #971

Closed

Conversation

mattverse
Copy link
Contributor

@mattverse mattverse commented Sep 1, 2022

Resolves #904

We have a history of disabling stargate queries, as some of the queries were non-deterministic to run on the state machine. (cref: #812).

This PR introduces re-enabling stargate queries whilst ensuring determinism using proto marshalling, json marshalling and whitelists.

Note that this PR does not simply revert #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)

@mattverse mattverse requested a review from alpe as a code owner September 1, 2022 09:11
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #971 (af42da8) into main (a575bce) will decrease coverage by 0.09%.
The diff coverage is 46.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #971      +/-   ##
==========================================
- Coverage   59.01%   58.92%   -0.10%     
==========================================
  Files          52       53       +1     
  Lines        6183     6210      +27     
==========================================
+ Hits         3649     3659      +10     
- Misses       2266     2281      +15     
- Partials      268      270       +2     
Impacted Files Coverage Δ
x/wasm/keeper/query_plugins.go 76.92% <43.33%> (-3.68%) ⬇️
x/wasm/keeper/keeper.go 87.93% <100.00%> (ø)
x/wasm/keeper/stargate_whitelist.go 100.00% <100.00%> (ø)

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution and driving this forward!
I still feel not very confident with enabling Stargate queries due to their history. I have commented a bit on the code but would ask @webmaster128 and @ethanfrey to comment on the status with the SDK and steps needed to enabled them again.

x/wasm/keeper/query_plugins.go Outdated Show resolved Hide resolved
x/wasm/keeper/query_plugins.go Outdated Show resolved Hide resolved
x/wasm/keeper/query_plugins.go Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Outdated Show resolved Hide resolved
// set up whitelist manually for testing
protoResponse, ok := tc.responseProtoStruct.(proto.Message)
require.True(t, ok)
keeper.StargateWhitelist.Store(tc.queryPath, protoResponse)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the whitelist adds much value to this test. You may want to call StargateQuerier to cover it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of a separate case for StargateQuerier, this is just a test case to ensure we have the same marshalled value and response upon proto struct change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, if you think this testing does not have much value in testing, we can simply replace this with a test for StargateQuerier.

https://github.com/osmosis-labs/osmosis/pull/2353/files#diff-b1c91ccd0545c457c9470ecc7dd0fb906193e8986db10316c9f14a1d7d3d4f26R43-R164 This is a test I'm trying to get on this PR(we can also do it on a separate PR, but having hard time in doing so due to different testing settings)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattverse I think it would be nice to have a test where you setup AcceptList and then you call StargateQuerier to verify that whithelisted queries are executed and the others return an error.

//
// The query can be multi-thread, so we have to use
// thread safe sync.Map.
var StargateWhitelist sync.Map
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this an AcceptList to be sensible with terms

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with you on moving towards being more sensible with terms here, while I still feel like having Stargate as a part of the variable name is an important part. What do you think about StargateAcceptList?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps StargateQueryAllowList ?


// Define whitelists here as maps using 'StargateWhitelist'
// e.x) StargateWhitelist.Store("/cosmos.auth.v1beta1.Query/Account", &authtypes.QueryAccountResponse{})
func init() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions what should go in here? I would prefer to not use the init() method but set/ call this from app.go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll also prefer to be something to be set by app.go or similar, since this will go unnoticed for projects that may not want to enable some of the items in this potential list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we simply remove this init method here? And then projects can simply call AcceptList.Store() or AcceptList.Load() for setter and getter for the whitelists

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattverse I agree you can simply remove the init method

@ethanfrey
Copy link
Member

@alpe

I feel strongly against enabling any StargateQueries by default in x/wasm or wasmd.

I have talked with the sdk team and they will be working to provide stability guarantees in the future, so we will be able to safely whitelist some queries in the future.

Some chains like Osmosis want to whitelist both their custom queries (from modules they control, can guarantee) and some standard sdk modules they feel comfortable handling.

Since there is a strong push for this, I think providing a standard way for a chain to safely enable a subset of queries makes sense. As long as the implementation seems solid, and the default behavior is same as now (no StargateQueries) unless the chain explicitly enables them, I think this is safe and happy to roll this out.

Each chain can decide how much risk they want to take and knowingly take it.

@ethanfrey
Copy link
Member

Once the above code comments are addressed, I would be happy to see this merged (and know a few chains are also excited by that)

@ethanfrey ethanfrey added this to the v0.30.0 milestone Sep 6, 2022
@alpe
Copy link
Contributor

alpe commented Nov 1, 2022

I have opened a new PR based on this to complete the feature integration. @mattverse please 👀 #1069

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 this pull request may close these issues.

Use an Allow list for StargateQuery instead of completely disabling it
6 participants