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

feat: add custom queries to wasm module #5261

Merged
merged 11 commits into from
Dec 1, 2023

Conversation

aeryz
Copy link
Contributor

@aeryz aeryz commented Nov 30, 2023

Description

Adds custom query support to the wasm client module, see the issue for more details.

closes: #5192

Commit Message / Changelog Entry

type: commit message

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.

Signed-off-by: aeryz <abdullaheryz@protonmail.com>
crodriguezvega and others added 3 commits November 30, 2023 12:37
Signed-off-by: aeryz <abdullaheryz@protonmail.com>
Signed-off-by: aeryz <abdullaheryz@protonmail.com>
Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Looks great @aeryz, thank you so much! 🙏🏻

Once there's consensus on the interface usage (which looks like there is) we can remove the duplicated interface! Will drop an approval then 🚀

edit: also happy to collaborate on this branch to help push things over the line

@aeryz
Copy link
Contributor Author

aeryz commented Nov 30, 2023

Looks great @aeryz, thank you so much! 🙏🏻

Once there's consensus on the interface usage (which looks like there is) we can remove the duplicated interface! Will drop an approval then 🚀

edit: also happy to collaborate on this branch to help push things over the line

Hey, thanks! Please go ahead :)

@@ -32,6 +34,16 @@ func GetVM() WasmEngine {
return vm
}

// SetQuerier sets the custom wasm query handle for the 08-wasm module.
func SetQuerier(wasmQuerier WasmQuerier) {
querier = wasmQuerier
Copy link
Contributor

Choose a reason for hiding this comment

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

Simon from Confio has warned us against nil queriers and they will not be allowed in the future, so it would be good to add a check here that wasmQuerier is not nil and also in NewKeeperWithVM, just as we do now for the vm.

Copy link
Contributor Author

@aeryz aeryz Nov 30, 2023

Choose a reason for hiding this comment

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

That makes sense but also, isn't it a better API for end users to pass nil to NewKeeperWithVM and in the setter function, do:

if wasmQuerier == nil {
    querier = defaultQuerier // or noop or error, etc.
} 

This way we could hide the default type and users won't have to give that to the NewKeeperWithVM function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, basically this comment. Sorry lol: #5261 (comment)

Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

dropping my approval here. I think it might be best to do #5281 afterwards, considering additional testing that should be added. Feel free to override if anyone wants to push it in on this branch.

@github-actions github-actions bot added the docs Improvements or additions to documentation label Nov 30, 2023
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

Ack changes, beautiful @crodriguezvega!

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks everyone for their input and @aeryz for opening the initial issue and PR ❤️

@DimitrisJim DimitrisJim merged commit 7016a94 into cosmos:main Dec 1, 2023
63 checks passed
mergify bot pushed a commit that referenced this pull request Dec 1, 2023
* feat: add custom queries to wasm module

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* linter stuff as per usual.

* review comments

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* lint

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* Use wasmvm.Querier.

* Lint this bad boy

* Small test nits.

* add default querier and update documentation

---------

Signed-off-by: aeryz <abdullaheryz@protonmail.com>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 7016a94)

# Conflicts:
#	docs/docs/03-light-clients/04-wasm/03-integration.md
#	docs/docs/03-light-clients/04-wasm/05-governance.md
#	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/testing/simapp/app.go
mergify bot pushed a commit that referenced this pull request Dec 1, 2023
* feat: add custom queries to wasm module

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* linter stuff as per usual.

* review comments

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* lint

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* Use wasmvm.Querier.

* Lint this bad boy

* Small test nits.

* add default querier and update documentation

---------

Signed-off-by: aeryz <abdullaheryz@protonmail.com>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 7016a94)
crodriguezvega added a commit that referenced this pull request Dec 1, 2023
* feat: add custom queries to wasm module (#5261)

* feat: add custom queries to wasm module

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* linter stuff as per usual.

* review comments

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* lint

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* Use wasmvm.Querier.

* Lint this bad boy

* Small test nits.

* add default querier and update documentation

---------

Signed-off-by: aeryz <abdullaheryz@protonmail.com>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 7016a94)

# Conflicts:
#	docs/docs/03-light-clients/04-wasm/03-integration.md
#	docs/docs/03-light-clients/04-wasm/05-governance.md
#	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/testing/simapp/app.go

* rm docs

* Fix conflicts.

* Update wasm.go

* gofumpt

---------

Co-authored-by: Abdullah Eryuzlu <24809834+aeryz@users.noreply.github.com>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
crodriguezvega pushed a commit that referenced this pull request Dec 1, 2023
* feat: add custom queries to wasm module

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* linter stuff as per usual.

* review comments

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* lint

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* Use wasmvm.Querier.

* Lint this bad boy

* Small test nits.

* add default querier and update documentation

---------

Signed-off-by: aeryz <abdullaheryz@protonmail.com>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
(cherry picked from commit 7016a94)

Co-authored-by: Abdullah Eryuzlu <24809834+aeryz@users.noreply.github.com>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
damiannolan pushed a commit that referenced this pull request Dec 5, 2023
* feat: add custom queries to wasm module

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* linter stuff as per usual.

* review comments

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* lint

Signed-off-by: aeryz <abdullaheryz@protonmail.com>

* Use wasmvm.Querier.

* Lint this bad boy

* Small test nits.

* add default querier and update documentation

---------

Signed-off-by: aeryz <abdullaheryz@protonmail.com>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
08-wasm docs Improvements or additions to documentation priority PRs that need prompt reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add custom query support to wasm module
5 participants