refactor: break PoolI.SpotPrice API to return BigDec #6372

p0mvn · 2023-09-11T21:18:06Z

Progress towards: #6064

What is the purpose of the change

This PR is an incremental step towards implementing a 36 decimal spot price query.

The proposal and demo of what the end result would roughly look like can be seen here: #6371

The goal is to incrementally break APIs in a state-compatible manner while backporting them to v19.x

The assumptions made:

x/twap will continue to support only 18 decimal prices and not function correctly for pools with smaller spot price
x/gamm pools will continue to support 18 decimal spot price only. Fixing/changing them is outside of scope.

Testing and Verifying

Covered by existing tests

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 · 2023-09-11T21:18:21Z

Important Notice

This PR includes modifications to the tests/e2e/initialization module.
Please follow the instructions below:

Backport these changes to the previous Osmosis version's branch.
Run the script inside a Docker container to update genesis and configs for pre-upgrade Osmosis.
Merge the backported changes.
The image will be built and uploaded to Docker Hub here.
Grab the latest image and update it in the PR to the main branch replacing the previousVersionInitTag in the osmosis/tests/e2e/containers/config.go

Please let us know if you need any help.

p0mvn · 2023-09-11T21:18:37Z

x/concentrated-liquidity/model/pool.go

+	// The reason why we convert the result to Dec and then back to BigDec is to temporarily
+	// maintain backwards compatibility with the original implementation.
+	// TODO: remove before https://github.com/osmosis-labs/osmosis/issues/5726 is complete
 	if baseAssetDenom == p.Token0 {
-		return p.CurrentSqrtPrice.PowerInteger(2).Dec(), nil
+		return osmomath.BigDecFromDec(p.CurrentSqrtPrice.PowerInteger(2).Dec()), nil
 	}


Note: tracked in #5726

p0mvn · 2023-09-11T21:18:51Z

x/concentrated-liquidity/model/pool_test.go

+				// TODO: truncation is acceptable temporary
+				// remove before https://github.com/osmosis-labs/osmosis/issues/5726 is complete
+				s.Require().True(spotPriceFromMethod.Dec().Sub(tc.expectedSpotPrice).Abs().LT(elipson))


Note: tracked here: #5726

p0mvn · 2023-09-11T21:19:01Z

x/concentrated-liquidity/pool.go

+	// TODO: remove before https://github.com/osmosis-labs/osmosis/issues/5726 is complete.
+	// Acceptable for backwards compatibility with v19.x.
+	return price.Dec(), nil


Note: tracked here: #5726

p0mvn · 2023-09-11T21:20:31Z

x/cosmwasmpool/model/pool.go

-	return osmomath.MustNewDecFromStr(response.SpotPrice), nil
+	return osmomath.MustNewBigDecFromStr(response.SpotPrice), nil


Reviewer, please pay closer attention to this. I think it is acceptable to convert this to BigDec from string directly instead of string -> Dec -> BigDec but would like some more eyes.

This seems fine to me

This seems okay conceptually but would be great to have tests for it to be sure

Thanks.

On another look, will make an issue to error as opposed to panic in the conversion. Will try to see if I can test this without having to modify the transmuter contract as well

czarcas7ic

LGTM, question on one of your assumption though. You said twap will continue to only support 18 dec precision to be compatible with pools with smaller spot price. Did you mean that this is the behavior due to this PR or is this the desired end state? If the former, cant we support the full precision in the twap records? If the latter, then disregard.

czarcas7ic · 2023-09-13T18:00:41Z

x/cosmwasmpool/model/pool.go

-	return osmomath.MustNewDecFromStr(response.SpotPrice), nil
+	return osmomath.MustNewBigDecFromStr(response.SpotPrice), nil


This seems fine to me

AlpinYukseloglu

LGTM, seems in alignment with offline discussions. Had one light request for tests on the CW pool conversion but I think it's okay to do that in a separate PR to not block this one.

AlpinYukseloglu · 2023-09-14T05:35:53Z

x/cosmwasmpool/model/pool.go

-	return osmomath.MustNewDecFromStr(response.SpotPrice), nil
+	return osmomath.MustNewBigDecFromStr(response.SpotPrice), nil


This seems okay conceptually but would be great to have tests for it to be sure

p0mvn · 2023-09-14T10:33:48Z

LGTM, question on one of your assumption though. You said twap will continue to only support 18 dec precision to be compatible with pools with smaller spot price. Did you mean that this is the behavior due to this PR or is this the desired end state? If the former, cant we support the full precision in the twap records? If the latter, then disregard.

This is the end state. We can but this is outside of scope of this work. This can be done in the future if there is a need for it

p0mvn · 2023-09-14T10:34:58Z

LGTM, question on one of your assumption though. You said twap will continue to only support 18 dec precision to be compatible with pools with smaller spot price. Did you mean that this is the behavior due to this PR or is this the desired end state? If the former, cant we support the full precision in the twap records? If the latter, then disregard.

This is the end state. We can but this is outside of scope of this work. This can be done in the future if there is a need for it

Issue made: #6406

(cherry picked from commit 39341fa) # Conflicts: # app/apptesting/gamm.go # tests/e2e/initialization/config.go # x/concentrated-liquidity/model/pool.go # x/concentrated-liquidity/model/pool_test.go # x/concentrated-liquidity/pool.go # x/concentrated-liquidity/swaps_test.go # x/cosmwasmpool/model/pool.go # x/cosmwasmpool/model/pool_test.go # x/cosmwasmpool/model/store_model.go # x/gamm/keeper/pool_service_test.go # x/gamm/pool-models/balancer/pool.go # x/gamm/pool-models/stableswap/pool.go # x/poolmanager/types/pool.go # x/superfluid/keeper/migrate_test.go

…#6407) * updates * fix conflicts in tests * lint * Update CHANGELOG.md * Update CHANGELOG.md --------- Co-authored-by: roman <[email protected]>

refactor: break PoolI.SpotPrice API to return BigDec

6223616

p0mvn added the V:state/compatible/backport State machine compatible PR, should be backported label Sep 11, 2023

github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid C:app-wiring Changes to the app folder C:x/concentrated-liquidity C:x/poolmanager labels Sep 11, 2023

p0mvn removed C:x/gamm Changes, features and bugs related to the gamm module. C:x/superfluid labels Sep 11, 2023

p0mvn added A:backport/v19.x backport patches to v19.x branch and removed C:app-wiring Changes to the app folder C:x/concentrated-liquidity C:x/poolmanager labels Sep 11, 2023

p0mvn commented Sep 11, 2023

View reviewed changes

p0mvn marked this pull request as ready for review September 11, 2023 21:44

p0mvn requested review from mattverse, AlpinYukseloglu and czarcas7ic as code owners September 11, 2023 21:44

czarcas7ic approved these changes Sep 13, 2023

View reviewed changes

AlpinYukseloglu approved these changes Sep 14, 2023

View reviewed changes

p0mvn mentioned this pull request Sep 14, 2023

refactor: use NewBigDec instead of MustBigDec in cwpool spot price #6406

Open

p0mvn merged commit 39341fa into main Sep 14, 2023

p0mvn deleted the roman/spot-price-pooli-bigdec branch September 14, 2023 10:35

mergify bot mentioned this pull request Sep 14, 2023

refactor: break PoolI.SpotPrice API to return BigDec (backport #6372) #6407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: break PoolI.SpotPrice API to return BigDec #6372

refactor: break PoolI.SpotPrice API to return BigDec #6372

p0mvn commented Sep 11, 2023 •

edited

Loading

github-actions bot commented Sep 11, 2023

p0mvn Sep 11, 2023

p0mvn Sep 11, 2023

p0mvn Sep 11, 2023

p0mvn Sep 11, 2023

czarcas7ic Sep 13, 2023

AlpinYukseloglu Sep 14, 2023

p0mvn Sep 14, 2023

czarcas7ic left a comment

czarcas7ic Sep 13, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Sep 14, 2023

p0mvn commented Sep 14, 2023 •

edited

Loading

p0mvn commented Sep 14, 2023

		return osmomath.MustNewDecFromStr(response.SpotPrice), nil
		return osmomath.MustNewBigDecFromStr(response.SpotPrice), nil

refactor: break PoolI.SpotPrice API to return BigDec #6372

refactor: break PoolI.SpotPrice API to return BigDec #6372

Conversation

p0mvn commented Sep 11, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

github-actions bot commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Sep 14, 2023 • edited Loading

p0mvn commented Sep 14, 2023

p0mvn commented Sep 11, 2023 •

edited

Loading

p0mvn commented Sep 14, 2023 •

edited

Loading