feat(08-wasm): querier plugins implemented #5345

srdtrk · 2023-12-08T10:18:33Z

Description

closes: #5325

Commit Message / Changelog Entry

feat(08-wasm): querier plugins implemented

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

This reverts commit 208e314.

This reverts commit b45b605.

damiannolan

Excellent work @srdtrk! This looks good to me, pending @DimitrisJim suggestions on error redactions. I left a couple of nits on godocs etc

LGTM!!

modules/light-clients/08-wasm/internal/ibcwasm/expected_interfaces.go

modules/light-clients/08-wasm/types/querier.go

modules/light-clients/08-wasm/keeper/options.go

modules/light-clients/08-wasm/types/querier.go

damiannolan · 2023-12-11T11:36:35Z

modules/light-clients/08-wasm/types/querier_test.go

@@ -107,3 +113,88 @@ func (suite *TypesTestSuite) TestCustomQuery() {
 		})
 	}
 }
+
+func (suite *TypesTestSuite) TestStargateQuery() {
+	typeURL := "/ibc.lightclients.wasm.v1.Query/Checksums"


wonder if we should add any more test queries for something basic, query from x/bank or x/staking for example, this is probably fine tho!

I will add more if others also want this

modules/light-clients/08-wasm/types/querier.go

crodriguezvega

Amazing, @srdtrk! Thanks for raising the issue and then opening the PR so quickly. Left a bunch of comments for now, but it looks good in my opinion. I will take a second look later today/tomorrow.

modules/light-clients/08-wasm/types/querier.go

crodriguezvega · 2023-12-11T09:17:04Z

modules/light-clients/08-wasm/types/querier.go

+// SetQueryPlugins sets the current query plugins
+func SetQueryPlugins(plugins *QueryPlugins) {
+	queryPlugins = plugins
+}
+
+// GetQueryPlugins returns the current query plugins
+func GetQueryPlugins() *QueryPlugins {
+	return queryPlugins
+}


Would it be possible to make these package internal? Should users use only WithQueryPlugins or do we expect them also to be able to use these 2 functions?

If possible it would be nice. There's the option of moving queryPlugins to internal/ibcwasm that I think @DimitrisJim suggested but unsure if that comes with some import cycle shenanigans.

But the keeper needs access to these functions and the user needs to be exposed to QueryPlugins

my main Q was w.r.t DefaultQuerier. As far as I understood, people will never override it, it just wraps things they override (namely, the queryPlugins) hence it could ideally be private (though still in types).

But the keeper needs access to these functions

The keeper can access these functions when it is internal? The 08-wasm keeper right?

After reviewing some more, I'd have a preference to move GetQueryPlugins/SetQueryPlugins to the internal package so that WithQueryPlugins is the only option to override the query plugins (ensuring no accidental nil values)

Fair enough @colin-axner. Do you think it'd be ok to keep queryHandler in types (after it is made private)? So that I don't need to move gas_registry to internal too?

yes, I am definitely in favour of keeping all implementation code in types. Make it private when possible. For Get/Set of QueryPlugins, I would add QueryPlugins as an expected interface, so you don't need to move over the implementation logic. Hopefully we remove all this global stuff before too long

If it is too much work, I'm fine as is. Mostly interested in limiting accidental misuse of query plugins by creating one api for setting/updating them

Hopefully we remove all this global stuff before too long

Agree! But from the looks of things we should be able to switch over quite nicely from the internal/ibcwasm stuff to the vm and such existing as fields on the Keeper. Looks like it will be clean enough to do it when the time comes :)

And fwiw I think working with the internal dir for the global types makes a big difference for the time being!

I moved getters and setters to internal now. And made queryHandler private

crodriguezvega · 2023-12-11T11:35:54Z

modules/light-clients/08-wasm/types/querier.go

+
+		isAccepted := slices.Contains(accepted, request.Path)
+		if !isAccepted {
+			return nil, wasmvmtypes.UnsupportedRequest{Kind: fmt.Sprintf("'%s' path is not allowed from the contract", request.Path)}


Should we use wasmvm errors or our own?

wasmvm in my opinion. Similar to what wasmd does. This also helps with the redactErr function that I just added.

modules/light-clients/08-wasm/types/querier.go

crodriguezvega · 2023-12-11T11:49:40Z

modules/light-clients/08-wasm/types/querier_test.go

+			clientState.Status(suite.chainA.GetContext(), clientStore, suite.chainA.App.AppCodec())
+
+			// reset query plugins after each test
+			types.SetQueryPlugins(types.NewDefaultQueryPlugins())


Why not do this after the call to SetupWasmWithMockVM as you do in the TestCustomQuery?

I think the end of test might be more reliable now. I should maybe change the other one too

modules/light-clients/08-wasm/keeper/options_test.go

colin-axner

LGTM, going to review the test files next

modules/light-clients/08-wasm/types/querier.go

colin-axner · 2023-12-11T15:01:03Z

modules/light-clients/08-wasm/types/querier.go

+// SetQueryPlugins sets the current query plugins
+func SetQueryPlugins(plugins *QueryPlugins) {
+	queryPlugins = plugins
+}
+
+// GetQueryPlugins returns the current query plugins
+func GetQueryPlugins() *QueryPlugins {
+	return queryPlugins
+}


But the keeper needs access to these functions

The keeper can access these functions when it is internal? The 08-wasm keeper right?

colin-axner · 2023-12-11T15:06:04Z

modules/light-clients/08-wasm/types/querier.go

+// SetQueryPlugins sets the current query plugins
+func SetQueryPlugins(plugins *QueryPlugins) {
+	queryPlugins = plugins
+}
+
+// GetQueryPlugins returns the current query plugins
+func GetQueryPlugins() *QueryPlugins {
+	return queryPlugins
+}


it just wraps things they override (namely, the queryPlugins) hence it could ideally be private (though still in types).

Yea, I think it could be nice to make queryHandler and newQueryHandler private as it should only be invoked by types/vm.go? But it is a nice to have

colin-axner

Thanks for the quick implementation @srdtrk! The feature looks good to me 🙂

crodriguezvega · 2023-12-11T15:53:21Z

@srdtrk Can you please, pretty please update the docs as well (in this PR or in a follow-up)? 🙏

crodriguezvega

Great work! Left just one more nit. Let's update the docs in a separate PR, so that the backport of this PR is clean to the release branches.

modules/light-clients/08-wasm/types/querier.go

srdtrk · 2023-12-12T09:14:17Z

@crodriguezvega, let's add the docs in another PR as this feature should not be blocked on documentation issues and nits

modules/light-clients/08-wasm/keeper/keeper.go

modules/light-clients/08-wasm/internal/ibcwasm/wasm.go

modules/light-clients/08-wasm/keeper/options.go

modules/light-clients/08-wasm/keeper/keeper.go

DimitrisJim

all aboard! ⛵ lgtm!

colin-axner

ACK

* imp: moved gas to internal * fix: fixed compiler complaints * feat: added 'QueryRouter' interface * imp: passing the queryRouter to keeper * Revert "fix: fixed compiler complaints" This reverts commit 208e314. * Revert "imp: moved gas to internal" This reverts commit b45b605. * fix(test): fixed keeper_test.go * imp: initial querier template * imp: moved querier to types * fix: compiler complaints * imp: removed querier from keeper * feat: including default querier * imp: added options * feat: querier implemented fully * docs: improved godocs * imp: improved the querier * style: improved styling of querier * fix: fixed options * fix: fixed options not being passed with config * style: renamed variables * imp: added review items * imp: review items * imp: set and get query plugins made private * docs: added more godocs * fix: default plugin not set * imp: review items * docs: added a godoc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit e2bcb77) # Conflicts: # modules/light-clients/08-wasm/internal/ibcwasm/wasm.go # modules/light-clients/08-wasm/keeper/keeper.go # modules/light-clients/08-wasm/keeper/keeper_test.go # modules/light-clients/08-wasm/keeper/migrations.go # modules/light-clients/08-wasm/testing/simapp/app.go # modules/light-clients/08-wasm/types/querier_test.go

* imp: moved gas to internal * fix: fixed compiler complaints * feat: added 'QueryRouter' interface * imp: passing the queryRouter to keeper * Revert "fix: fixed compiler complaints" This reverts commit 208e314. * Revert "imp: moved gas to internal" This reverts commit b45b605. * fix(test): fixed keeper_test.go * imp: initial querier template * imp: moved querier to types * fix: compiler complaints * imp: removed querier from keeper * feat: including default querier * imp: added options * feat: querier implemented fully * docs: improved godocs * imp: improved the querier * style: improved styling of querier * fix: fixed options * fix: fixed options not being passed with config * style: renamed variables * imp: added review items * imp: review items * imp: set and get query plugins made private * docs: added more godocs * fix: default plugin not set * imp: review items * docs: added a godoc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit e2bcb77)

* imp: moved gas to internal * fix: fixed compiler complaints * feat: added 'QueryRouter' interface * imp: passing the queryRouter to keeper * Revert "fix: fixed compiler complaints" This reverts commit 208e314. * Revert "imp: moved gas to internal" This reverts commit b45b605. * fix(test): fixed keeper_test.go * imp: initial querier template * imp: moved querier to types * fix: compiler complaints * imp: removed querier from keeper * feat: including default querier * imp: added options * feat: querier implemented fully * docs: improved godocs * imp: improved the querier * style: improved styling of querier * fix: fixed options * fix: fixed options not being passed with config * style: renamed variables * imp: added review items * imp: review items * imp: set and get query plugins made private * docs: added more godocs * fix: default plugin not set * imp: review items * docs: added a godoc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit e2bcb77) Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com>

* feat(08-wasm): querier plugins implemented (#5345) * imp: moved gas to internal * fix: fixed compiler complaints * feat: added 'QueryRouter' interface * imp: passing the queryRouter to keeper * Revert "fix: fixed compiler complaints" This reverts commit 208e314. * Revert "imp: moved gas to internal" This reverts commit b45b605. * fix(test): fixed keeper_test.go * imp: initial querier template * imp: moved querier to types * fix: compiler complaints * imp: removed querier from keeper * feat: including default querier * imp: added options * feat: querier implemented fully * docs: improved godocs * imp: improved the querier * style: improved styling of querier * fix: fixed options * fix: fixed options not being passed with config * style: renamed variables * imp: added review items * imp: review items * imp: set and get query plugins made private * docs: added more godocs * fix: default plugin not set * imp: review items * docs: added a godoc --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit e2bcb77) # Conflicts: # modules/light-clients/08-wasm/internal/ibcwasm/wasm.go # modules/light-clients/08-wasm/keeper/keeper.go # modules/light-clients/08-wasm/keeper/keeper_test.go # modules/light-clients/08-wasm/keeper/migrations.go # modules/light-clients/08-wasm/testing/simapp/app.go # modules/light-clients/08-wasm/types/querier_test.go * fix conflicts. * lint issues --------- Co-authored-by: srdtrk <59252793+srdtrk@users.noreply.github.com> Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>

srdtrk added 19 commits December 7, 2023 12:12

imp: moved gas to internal

b45b605

fix: fixed compiler complaints

208e314

feat: added 'QueryRouter' interface

fbb076e

imp: passing the queryRouter to keeper

e41b149

Revert "fix: fixed compiler complaints"

f9ddfdb

This reverts commit 208e314.

Revert "imp: moved gas to internal"

25f4cd3

This reverts commit b45b605.

fix(test): fixed keeper_test.go

52439b7

imp: initial querier template

b4707ef

imp: moved querier to types

d18be76

fix: compiler complaints

0dc9152

imp: removed querier from keeper

6e34d19

feat: including default querier

69513c1

imp: added options

350b80b

feat: querier implemented fully

28da657

docs: improved godocs

51bf845

imp: improved the querier

94649b9

style: improved styling of querier

6f116f9

fix: fixed options

e0bb1fb

Merge branch 'main' into serdar/issue#5325-replace-querier

1691bd8

srdtrk added the 08-wasm label Dec 8, 2023

fix: fixed options not being passed with config

ab155ce

srdtrk marked this pull request as ready for review December 8, 2023 10:29

srdtrk requested review from colin-axner, AdityaSripal, damiannolan, charleenfei, chatton, DimitrisJim and crodriguezvega as code owners December 8, 2023 10:29

crodriguezvega added the backport-wasm-v0.1.x+ibc-go-v7.3.x-wasmvm-v1.5.x label Dec 8, 2023

damiannolan approved these changes Dec 11, 2023

View reviewed changes

crodriguezvega reviewed Dec 11, 2023

View reviewed changes

imp: added review items

13cae40

colin-axner reviewed Dec 11, 2023

View reviewed changes

colin-axner approved these changes Dec 11, 2023

View reviewed changes

imp: review items

1cccd74

crodriguezvega approved these changes Dec 12, 2023

View reviewed changes

modules/light-clients/08-wasm/types/querier.go Show resolved Hide resolved

srdtrk added 3 commits December 12, 2023 11:29

imp: set and get query plugins made private

6d17d08

docs: added more godocs

c3ef405

Merge branch 'main' into serdar/issue#5325-replace-querier

a200d19

DimitrisJim reviewed Dec 12, 2023

View reviewed changes

modules/light-clients/08-wasm/keeper/keeper.go Show resolved Hide resolved

modules/light-clients/08-wasm/internal/ibcwasm/wasm.go Show resolved Hide resolved

modules/light-clients/08-wasm/keeper/options.go Outdated Show resolved Hide resolved

srdtrk and others added 3 commits December 12, 2023 12:59

fix: default plugin not set

f51bfa4

imp: review items

962ec46

Merge branch 'main' into serdar/issue#5325-replace-querier

d61a67f

colin-axner reviewed Dec 12, 2023

View reviewed changes

modules/light-clients/08-wasm/keeper/keeper.go Outdated Show resolved Hide resolved

docs: added a godoc

f40e387

DimitrisJim approved these changes Dec 12, 2023

View reviewed changes

colin-axner approved these changes Dec 12, 2023

View reviewed changes

srdtrk merged commit e2bcb77 into main Dec 12, 2023
74 checks passed

srdtrk deleted the serdar/issue#5325-replace-querier branch December 12, 2023 12:04

mergify bot mentioned this pull request Dec 12, 2023

feat(08-wasm): querier plugins implemented (backport #5345) #5381

Merged

mergify bot mentioned this pull request Dec 12, 2023

feat(08-wasm): querier plugins implemented (backport #5345) #5382

Merged

srdtrk mentioned this pull request Dec 12, 2023

Add documentation for query plugins and options api #5383

Closed

3 tasks

DimitrisJim mentioned this pull request Apr 10, 2024

Remove usage of globals from 08-wasm #6103

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(08-wasm): querier plugins implemented #5345

feat(08-wasm): querier plugins implemented #5345

srdtrk commented Dec 8, 2023

damiannolan left a comment

damiannolan Dec 11, 2023

srdtrk Dec 12, 2023

crodriguezvega left a comment

crodriguezvega Dec 11, 2023

damiannolan Dec 11, 2023

srdtrk Dec 11, 2023 •

edited

Loading

DimitrisJim Dec 11, 2023 •

edited

Loading

colin-axner Dec 11, 2023

colin-axner Dec 11, 2023

srdtrk Dec 11, 2023

colin-axner Dec 11, 2023 •

edited

Loading

damiannolan Dec 11, 2023

srdtrk Dec 12, 2023

crodriguezvega Dec 11, 2023

srdtrk Dec 11, 2023

crodriguezvega Dec 11, 2023

srdtrk Dec 11, 2023

colin-axner left a comment

colin-axner Dec 11, 2023

colin-axner Dec 11, 2023

colin-axner left a comment

crodriguezvega commented Dec 11, 2023

crodriguezvega left a comment

srdtrk commented Dec 12, 2023

DimitrisJim left a comment

colin-axner left a comment

feat(08-wasm): querier plugins implemented #5345

feat(08-wasm): querier plugins implemented #5345

Conversation

srdtrk commented Dec 8, 2023

Description

Commit Message / Changelog Entry

damiannolan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdtrk Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

DimitrisJim Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

crodriguezvega commented Dec 11, 2023

crodriguezvega left a comment

Choose a reason for hiding this comment

srdtrk commented Dec 12, 2023

DimitrisJim left a comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

srdtrk Dec 11, 2023 •

edited

Loading

DimitrisJim Dec 11, 2023 •

edited

Loading

colin-axner Dec 11, 2023 •

edited

Loading