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

feat(poolmanager): v2 SpotPrice query with 36 decimals #6488

Merged
merged 13 commits into from
Sep 27, 2023
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Sep 21, 2023

Progress towards: #6064

What is the purpose of the change

This PR introduces a GRPC and CLI wrapper around the new BigDec API. Contrary to our existing queries, it does not truncate the final result and returns the full 36 decimals.

It includes some modifications to our query generation script to be able to properly generate v2 queries without conflicts.

Localosmosis Test

osmosisd q poolmanager spot-price-v2 1 "uosmo" "uusdc"
spot_price: "1.000000000000000000000000000000000000"

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

@github-actions github-actions bot added C:CLI C:app-wiring Changes to the app folder C:x/twap Changes to the twap module C:x/concentrated-liquidity C:x/poolmanager labels Sep 21, 2023
@p0mvn p0mvn changed the title feat(poolmaanger): v2 SpotPrice query with 36 decimals feat(poolmanager): v2 SpotPrice query with 36 decimals Sep 21, 2023
@p0mvn p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v19.x backport patches to v19.x branch labels Sep 21, 2023
@p0mvn p0mvn marked this pull request as ready for review September 21, 2023 21:58
@p0mvn p0mvn marked this pull request as draft September 22, 2023 01:37
@p0mvn p0mvn marked this pull request as ready for review September 22, 2023 12:09
Base automatically changed from roman/spot-price-big-dec to main September 22, 2023 12:10
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.

LGTM!


// SpotPriceRequest defines the gRPC request structure for a SpotPrice
// query.
message SpotPriceRequest {
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this also have a V2 suffix? to avoid confusing it to v1's version:

message SpotPriceRequest {
.

same goes for SpotPriceResponse

Copy link
Member

Choose a reason for hiding this comment

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

on the second thought, I guess it is fine, since we call it by specifying a package name, which has v2 in it. still going to leave it here just in case

Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

ACK with minor request

cmd/querygen/main.go Outdated Show resolved Hide resolved
@@ -1,4 +1,5 @@
package grpc

Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't matter but the package should be the first line, are we able to modify the template to do this for all of the below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline and concluded that this is fine as long as no errors in the editor.

This stems from this line in the template:
#6488 (comment)

Didn't find a trivial way to fix and concluded that it is fine to keep as is since the change is only cosmetic

Comment on lines -1 to +2
package grpc
{{ $version := .VersionSuffix }}
package grpc{{$version}}
Copy link
Member Author

Choose a reason for hiding this comment

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

Link

@p0mvn p0mvn merged commit b1bfa20 into main Sep 27, 2023
1 check passed
@p0mvn p0mvn deleted the roman/v2-spot-price branch September 27, 2023 20:56
mergify bot pushed a commit that referenced this pull request Sep 27, 2023
* feat: make PoolModuleI CalculateSpotPrice API  return BigDec

* updates

* clean up

* updates

* updates

* feat(poolmaanger): v2 SpotPrice query with 36 decimals

* changelog

* clean up

* Generated protofile changes

* querygen updates

* Update cmd/querygen/main.go

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
(cherry picked from commit b1bfa20)

# Conflicts:
#	CHANGELOG.md
p0mvn added a commit that referenced this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v19.x backport patches to v19.x branch C:app-wiring Changes to the app folder C:CLI C:x/concentrated-liquidity C:x/poolmanager C:x/twap Changes to the twap module 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