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 AllDenomMetadata BankQuery #1426

Merged
merged 9 commits into from
Jul 6, 2023
Merged

Conversation

chipshort
Copy link
Collaborator

@chipshort chipshort commented May 31, 2023

Copy link
Member

@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.

Very nice work! Thanks for moving this forward. 💐
I have added some comments on testing but otherwise it looks great.
Unfortunately, I can not merge it without the wasmvm dependency resolved first.

go.mod Outdated Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Show resolved Hide resolved
@alpe alpe added the blocked label Jun 12, 2023
Copy link
Member

@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 adding the requested test cases! I appreciate the work a lot and added some detailed feedback on the test code in return. The tests are helping and there is no hard requirement to change them. Just sharing a bit of knowledge from working a lot in Go projects.

x/wasm/keeper/query_plugins_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Outdated Show resolved Hide resolved
x/wasm/keeper/query_plugins_test.go Show resolved Hide resolved
@alpe
Copy link
Member

alpe commented Jun 26, 2023

Thanks again for the updates. Good work!
We are waiting for the wasmvm release now to get this in

@alpe
Copy link
Member

alpe commented Jul 3, 2023

Linking #1296 to unblock this

@alpe alpe added this to the v0.41.0 milestone Jul 3, 2023
@chipshort chipshort mentioned this pull request Jul 5, 2023
5 tasks
@alpe alpe removed the blocked label Jul 6, 2023
@alpe alpe merged commit d2e9ace into CosmWasm:main Jul 6, 2023
3 checks passed
@chipshort chipshort deleted the denom-metadata-query branch July 6, 2023 12:57
@faddat faddat mentioned this pull request Jul 16, 2023
@alpe alpe added the backport/v0.3x Backport patches to sdk45 release branch label Jul 18, 2023
mergify bot pushed a commit that referenced this pull request Jul 18, 2023
* x/wasm: add AllDenomMetadata BankQuery

* x/wasm: fix AllDenomMetadata BankQuery to have pagination and add DenomMetadata BankQuery

* Use simplified pagination

* Fix request conversion

* Add unknown denom test cases

* Add test for pagination conversion

* Fix nits

* Use wasmvm 1.3.0-rc.0

* Fix test

---------

Co-authored-by: Nikhil Suri <[email protected]>
(cherry picked from commit d2e9ace)

# Conflicts:
#	x/wasm/keeper/query_plugins.go
alpe added a commit that referenced this pull request Jul 19, 2023
* Add AllDenomMetadata BankQuery (#1426)

* x/wasm: add AllDenomMetadata BankQuery

* x/wasm: fix AllDenomMetadata BankQuery to have pagination and add DenomMetadata BankQuery

* Use simplified pagination

* Fix request conversion

* Add unknown denom test cases

* Add test for pagination conversion

* Fix nits

* Use wasmvm 1.3.0-rc.0

* Fix test

---------

Co-authored-by: Nikhil Suri <[email protected]>
(cherry picked from commit d2e9ace)

# Conflicts:
#	x/wasm/keeper/query_plugins.go

* Resolve conflicts

---------

Co-authored-by: Christoph Otter <[email protected]>
Co-authored-by: Alex Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.3x Backport patches to sdk45 release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants