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

Fix: Support HTTP requests for FilteredPools #3737

Merged
merged 4 commits into from
Jan 2, 2023

Conversation

mattverse
Copy link
Member

cref: https://github.com/googleapis/googleapis/blob/master/google/api/http.proto

Current FilteredPools does not support http requests, as grpc gateway cannot do GET requests for non primitive types that are repeated.

This PR brings the solution using coins string, and then parsing the coins string manually with in the grp_query.go logic.

Brief Changelog

  • Change FilteredPools MinLiquidity field from sdk.Coins struct to string

Testing and Verifying

Passes existing test on the grpc method, also added new test cases for the helper method.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? yes
  • How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

@github-actions github-actions bot added C:CLI C:x/gamm Changes, features and bugs related to the gamm module. labels Dec 15, 2022
@mattverse mattverse added V:state/breaking State machine breaking PR A:backport/v13.x backport patches to v13.x branch labels Dec 15, 2022
}

min_liquidity = coins
min_liquidity = args[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make more sense to do:

			min_liquidity := args[0]
			var pool_type string
			if len(args) > 1 {
				pool_type = args[1]
			}

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! Nice catch!

@jonator
Copy link
Member

jonator commented Dec 15, 2022

Do you have an example of how the query should be serialized?

@mattverse
Copy link
Member Author

@jonator are you asking for what an example request would look like? If so, it would look something like this

{
"min_liquidity": "100uosmo, 300uatom",
"pool_type": "Balancer"
}

Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM but needs API break changelog update

@ValarDragon ValarDragon added V:state/compatible/backport State machine compatible PR, should be backported and removed V:state/breaking State machine breaking PR labels Dec 15, 2022
@ValarDragon
Copy link
Member

Changed labels, this is query breaking, but state compatible, as its not a query exposed to state machine. (If it was we couldn't backport)

@jonator
Copy link
Member

jonator commented Dec 15, 2022

@jonator are you asking for what an example request would look like? If so, it would look something like this

{
"min_liquidity": "100uosmo, 300uatom",
"pool_type": "Balancer"
}

No I meant something like:
https://lcd-osmosis.keplr.app/osmosis/gamm/v1beta1/filtered_pools?pagination.limit=100&pooltype=stableswap

We can't send a body in a GET request

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Dec 30, 2022
@mattverse mattverse removed the Stale label Jan 2, 2023
@mattverse
Copy link
Member Author

@mattverse mattverse merged commit 010d4cf into main Jan 2, 2023
@mattverse mattverse deleted the mattverse/query-pool-with-filters-grpc-fix branch January 2, 2023 07:17
mergify bot pushed a commit that referenced this pull request Jan 2, 2023
* Fix: Support GRPC For coins struct

* Add changelog entry

* use parse coin normalized

* Nicolas' review :)

(cherry picked from commit 010d4cf)
p0mvn pushed a commit that referenced this pull request Jan 2, 2023
* Fix: Support GRPC For coins struct

* Add changelog entry

* use parse coin normalized

* Nicolas' review :)

(cherry picked from commit 010d4cf)

Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v13.x backport patches to v13.x branch C:CLI C:x/gamm Changes, features and bugs related to the gamm module. V:state/compatible/backport State machine compatible PR, should be backported
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants