-
Notifications
You must be signed in to change notification settings - Fork 602
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: cl spotprice query #5863
fix: cl spotprice query #5863
Conversation
upon futher investigation, this affects and reverse the twap for cl pools also, making this state breaking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have it along with the twap fix we need to make (by twap fix, I'm referring to the twap record that this change will cause break. We would either need migration or removal of the existing twap record)
@@ -91,7 +91,7 @@ func GetCmdPool() (*osmocli.QueryDescriptor, *queryproto.PoolRequest) { | |||
|
|||
func GetCmdSpotPrice() (*osmocli.QueryDescriptor, *queryproto.SpotPriceRequest) { | |||
return &osmocli.QueryDescriptor{ | |||
Use: "spot-price <pool-ID> [quote-asset-denom] [base-asset-denom]", | |||
Use: "spot-price <pool-ID> [base-asset-denom] [quote-asset-denom]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to other reviewers: This changed due to order of proto query client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once changelog entry is added. We should also create a follow-up issue for migrating/deleting existing records as discussed internally (converged on it being okay to do in follow-up PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, appreciate the PR!
Closes: #XXX
What is the purpose of the change
fixed inconsistency in spot price query for cl pools - the base and quote denoms are being reversed due to the spot price querier providing them in reverse order:
osmosis/x/poolmanager/client/query_proto_wrap.go
Line 151 in c26756d
and edited spot price cli command help text to reflect actual behavior of "denom in, denom out"
note - I also noticed that the path for this query doesn't include the v1beta1 version name
osmosis/proto/osmosis/poolmanager/v1beta1/query.proto
Lines 70 to 72 in c26756d
Testing and Verifying
This change modified the spotprice test to swap the args and can be verified as follows:
<lcd>/osmosis/poolmanager/pools/1066/prices?base_asset_denom=uosmo"e_asset_denom=ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7
Documentation and Release Note
Unreleased
section ofCHANGELOG.md
?Where is the change documented?
x/{module}/README.md
)