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

CLI: Query pools by coin denom #6766

Merged
merged 31 commits into from
Nov 15, 2023

Conversation

DongLieu
Copy link
Contributor

Closes: #6712

What is the purpose of the change

Add a description of the overall background and high level changes that this PR introduces

(E.g.: This pull request improves documentation of area A by adding ....

Testing and Verifying

(Please pick one of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added unit test that validates ...
  • Added integration tests for end-to-end deployment with ...
  • Extended integration test for ...
  • Manually verified the change by ...

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@DongLieu
Copy link
Contributor Author

image

@DongLieu
Copy link
Contributor Author

image

@DongLieu
Copy link
Contributor Author

  • I have 6 pools in which:
    + 1. stake-uosmo
    + 2. atom-stake
    + 3.uosmo-atom
    + 4. stake-uosmo
    + 5. atom-stake
    + 6.uosmo-atom

  • Query by denom "stake":
    + 1. stake-uosmo
    + 2. atom-stake
    + 4. stake-uosmo
    + 5. atom-stake

pysel
pysel previously requested changes Oct 26, 2023
Copy link
Member

@pysel pysel left a comment

Choose a reason for hiding this comment

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

hey @DongLieu! Thank you for tackling this issue. And thanks for attaching logs to the PR.

Could you please also include a unit test for this query?

x/poolmanager/client/query_proto_wrap.go Outdated Show resolved Hide resolved
x/poolmanager/client/query_proto_wrap.go Outdated Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

Nice! Left some comments, please take a look!

proto/osmosis/poolmanager/v1beta1/query.proto Outdated Show resolved Hide resolved
proto/osmosis/poolmanager/v1beta1/query.proto Outdated Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
@mattverse mattverse added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Oct 27, 2023
@mattverse
Copy link
Member

@DongLieu Can we also add changelog for this?

@DongLieu
Copy link
Contributor Author

Thanks @mattverse and @pysel for your review.
Ok @mattverse. I add changelog now

@DongLieu
Copy link
Contributor Author

DongLieu commented Nov 1, 2023

Hey, to make it easier for the following issues, can you add me to the repo?

@DongLieu
Copy link
Contributor Author

DongLieu commented Nov 1, 2023

I mean I can't push my branch to osmosis

@pysel
Copy link
Member

pysel commented Nov 1, 2023

Hey, to make it easier for the following issues, can you add me to the repo?

I cannot do that unfortunately. For the sake of this PR, could you please run make proto-all and push changes to your local branch? thanks!

@mattverse
Copy link
Member

If it continues failing, happy to merge it and then run make proto-all after wards, no worries

@DongLieu
Copy link
Contributor Author

DongLieu commented Nov 2, 2023

I tried pulling main from osmosis and merging, then running 'make proto-all' but no change. So I thought it was my laptop's fault, but thanks to @hieuvubk help, it's still the same.

@DongLieu
Copy link
Contributor Author

DongLieu commented Nov 5, 2023

I added test for func @mattverse

Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

LGTM

@DongLieu Would you be able to add a case that includes CL pools? After that I think we should be good to merge

x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
expectedNumPools: 0,
},
"Many pools": {
poolType: []types.PoolType{types.Balancer, types.Balancer, types.Balancer, types.Balancer},
Copy link
Member

Choose a reason for hiding this comment

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

Can we also test with CL?

@DongLieu
Copy link
Contributor Author

DongLieu commented Nov 7, 2023

I added. Is it exactly what you wanted? @mattverse

@mattverse
Copy link
Member

@DongLieu awesome thanks!

Will leave this PR open incase anyone wants to give it extra review, will merge this by EOW o.w

@mattverse mattverse requested a review from pysel November 15, 2023 10:30
@mattverse mattverse dismissed pysel’s stale review November 15, 2023 10:31

Has been addresed

@mattverse mattverse merged commit 6e876a6 into osmosis-labs:main Nov 15, 2023
1 check passed
doggystylez pushed a commit to doggystylez/osmosis that referenced this pull request Feb 14, 2024
* Add Cli query pools by denom

* revert

* minor

* minor

* Update x/poolmanager/client/query_proto_wrap.go

Co-authored-by: Ruslan Akhtariev <[email protected]>

* Update x/poolmanager/client/query_proto_wrap.go

Co-authored-by: Ruslan Akhtariev <[email protected]>

* Update x/poolmanager/router.go

Co-authored-by: Ruslan Akhtariev <[email protected]>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <[email protected]>

* Update x/poolmanager/router.go

Co-authored-by: Matt, Park <[email protected]>

* Update proto/osmosis/poolmanager/v1beta1/query.proto

Co-authored-by: Matt, Park <[email protected]>

* Update x/poolmanager/router.go

Co-authored-by: Matt, Park <[email protected]>

* Update x/poolmanager/router.go

Co-authored-by: Matt, Park <[email protected]>

* minor and lint

* Use dynamic slice instead of Allocate the slice

* changelog

* make proto-gen

* lint proto

* add test

* Update x/poolmanager/router_test.go

Co-authored-by: Matt, Park <[email protected]>

* Update x/poolmanager/router_test.go

Co-authored-by: Matt, Park <[email protected]>

* make proto-all

* use a few poolType Concentrated

---------

Co-authored-by: Ruslan Akhtariev <[email protected]>
Co-authored-by: Matt, Park <[email protected]>
Co-authored-by: Hieu Vu <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
@github-actions github-actions bot mentioned this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:x/poolmanager T:build V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: osmosisd: Query pool by coin denom
4 participants