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

Protorev query highest liquidity pools #4829

Merged
merged 11 commits into from
Apr 18, 2023
Merged

Conversation

NotJeremyLiu
Copy link
Contributor

@NotJeremyLiu NotJeremyLiu commented Apr 4, 2023

What is the purpose of the change

  • Add visibility into protorev for the highest liquidity pool stored given a denom pair via a query endpoint

Brief Changelog

  • Adds rpc / cli queries that return a pool id given a denom pair to look up in protorev's highest liquidity pool method

Testing and Verifying

  • All tests still pass
  • Added test to verify new query
  • Verified CLI works via localosmosis

Documentation and Release Note

  • Added new queries to documentation

@NotJeremyLiu NotJeremyLiu added the protorev All things related to x/protorev label Apr 4, 2023
@github-actions github-actions bot added the C:CLI label Apr 4, 2023
@NotJeremyLiu NotJeremyLiu added the V:state/compatible/backport State machine compatible PR, should be backported label Apr 5, 2023
@NotJeremyLiu
Copy link
Contributor Author

Added label for non-state breaking, don't know if it should be backport or no-backport

@NotJeremyLiu NotJeremyLiu requested a review from davidterpay April 5, 2023 22:47
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Apr 5, 2023
@NotJeremyLiu NotJeremyLiu marked this pull request as ready for review April 5, 2023 22:48
@p0mvn p0mvn mentioned this pull request Apr 10, 2023
func NewQueryPoolCmd() (*osmocli.QueryDescriptor, *types.QueryGetProtoRevPoolRequest) {
return &osmocli.QueryDescriptor{
Use: "pool [base_denom] [other_denom]",
Short: "Query the pool id for a given denom pair stored in ProtoRev",
Copy link
Contributor

Choose a reason for hiding this comment

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

Update this based on the comment above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, just want highest liquidity pools. Specified here: 821b247

Copy link
Contributor

@davidterpay davidterpay left a comment

Choose a reason for hiding this comment

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

LGTM with small nits.

Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM

x/protorev/keeper/grpc_query_test.go Outdated Show resolved Hide resolved
@p0mvn p0mvn added the A:backport/v15.x backport patches to v15.x branch label Apr 17, 2023
@p0mvn
Copy link
Member

p0mvn commented Apr 17, 2023

Let's add a changelog entry for this and merge

@NotJeremyLiu NotJeremyLiu force-pushed the jl/protorev-query-highest-liq-pools branch from 8838a65 to 7049f01 Compare April 18, 2023 00:01
@NotJeremyLiu
Copy link
Contributor Author

Let's add a changelog entry for this and merge

Added in: 7049f01 and rebased on main, so should be ready to merge.

@p0mvn p0mvn merged commit 5f4beb7 into main Apr 18, 2023
@p0mvn p0mvn deleted the jl/protorev-query-highest-liq-pools branch April 18, 2023 00:26
mergify bot pushed a commit that referenced this pull request Apr 18, 2023
* Update proto files for protorev pool query

- protorev pool query will return pool id given a denom pair for the highest liquidity method

* add grpc query handler logic for GetProtoRevPool query

* Add cli query support

* Add test for GetProtoRevPool query

* Change endpoint to pool and re-gen proto code

* add pool query in documentation

* Specify the query is for highest liquidity method pools only

* Change comment to represent query

Co-authored-by: David Terpay <[email protected]>

* change comments in proto file and generated pb

* fix test comment

* Add query to changelog

---------

Co-authored-by: David Terpay <[email protected]>
(cherry picked from commit 5f4beb7)

# Conflicts:
#	CHANGELOG.md
#	x/protorev/client/cli/query.go
#	x/protorev/protorev.md
p0mvn pushed a commit that referenced this pull request Apr 19, 2023
* Protorev query highest liquidity pools (#4829)

* Update proto files for protorev pool query

- protorev pool query will return pool id given a denom pair for the highest liquidity method

* add grpc query handler logic for GetProtoRevPool query

* Add cli query support

* Add test for GetProtoRevPool query

* Change endpoint to pool and re-gen proto code

* add pool query in documentation

* Specify the query is for highest liquidity method pools only

* Change comment to represent query

Co-authored-by: David Terpay <[email protected]>

* change comments in proto file and generated pb

* fix test comment

* Add query to changelog

---------

Co-authored-by: David Terpay <[email protected]>
(cherry picked from commit 5f4beb7)

# Conflicts:
#	CHANGELOG.md
#	x/protorev/client/cli/query.go
#	x/protorev/protorev.md

* Resolve merge conflicts

- Main issue I think is I rebased on main when I should've rebased on v15 since this was to be backported
- I only added this entry into the changelog since the other changes may not be reflected in the backport

---------

Co-authored-by: Jeremy Liu <[email protected]>
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v15.x backport patches to v15.x branch C:CLI C:docs Improvements or additions to documentation protorev All things related to x/protorev V:state/compatible/backport State machine compatible PR, should be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants