refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way #6256

p0mvn · 2023-08-31T19:16:23Z

Closes: #6246

What is the purpose of the change

Refactors CalculatePriceToTick function to operate on BigDec and support prices in the newly added range [10^-30, 10^-12);

State-compatibility is guaranteed by the following:

The caller guarantees that the provided price is within the current mainnet bounds of `[10^-12, 10^38]
For the currently supported range, we truncate the price to 18 decimals before performing any price to tick conversions
There is a test added showing that for all supported prices given our exponent at price one, CalcPriceToTickV1 (18 decimal) and CalcPriceToTick (36 decimal) output the same values.

The test is currently skipped as it is only meant for display purposes

The CalcPriceToTickV1 and test showing compatibility are to be removed in #6247.

Testing and Verifying

Added test vectors for CalculatePriceToTick in the newly supported range
Added test showing compatibility with the current mainnet function

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

…nversions in a state-compatible way

p0mvn · 2023-08-31T19:17:17Z

x/concentrated-liquidity/math/tick.go

@@ -152,19 +152,21 @@ func powTenBigDec(exponent int64) osmomath.BigDec {
 	return bigNegPowersOfTen[-exponent]
 }

-func CalculatePriceToTickDec(priceBigDec osmomath.BigDec) (tickIndex sdk.Dec, err error) {
+// CalculatePriceToTickV1 computes tick from price using 18 decimal math under the hood.
+// TODO: remove


Note: TBD in #6247

p0mvn · 2023-08-31T19:19:03Z

x/concentrated-liquidity/math/tick.go

+		// TODO: implement efficient big decimal truncation.
+		// It is acceptable to truncate price as the minimum we support is
+		// 10**-12 which is above the smallest value of sdk.Dec.
+		price = osmomath.BigDecFromSDKDec(price.SDKDec())


Note: TBD #6257

p0mvn · 2023-08-31T19:19:19Z

x/concentrated-liquidity/math/tick.go

+	// There is one geometric spacing for every power of ten.
+	// If price > 1, we search for the first geometric spacing w/ a max price greater than our price.
+	// If price < 1, we search for the first geometric spacing w/ a min price smaller than our price.
+	// TODO: We can optimize by using smarter search algorithms


Note: this is copied from the original function

p0mvn · 2023-08-31T19:54:49Z

e2e is flaky. If failure, it is unrelated

AlpinYukseloglu

Did a brief first pass and had a couple minor questions. Will re-review more rigorously tonight or tomorrow

AlpinYukseloglu · 2023-08-31T23:03:34Z

x/concentrated-liquidity/pool.go

@@ -164,10 +165,10 @@ func (k Keeper) CalculateSpotPrice(
 	}

 	if price.IsZero() {
-		return sdk.Dec{}, types.PriceBoundError{ProvidedPrice: price, MinSpotPrice: types.MinSpotPrice, MaxSpotPrice: types.MaxSpotPrice}
+		return sdk.Dec{}, types.PriceBoundError{ProvidedPrice: osmomath.BigDecFromSDKDec(price), MinSpotPrice: types.MinSpotPriceV2, MaxSpotPrice: types.MaxSpotPrice}
 	}
 	if price.GT(types.MaxSpotPrice) || price.LT(types.MinSpotPrice) {


Is spot price still on 18 precision? How do we expect assets with spot price below the previous MinSpotPrice to be represented here?

Yes, it is.

This query will not work correctly for the pools with spot price below the old MinSpotPrice.

I might have erroneously set MinSpotPriceV2 in the error. Will fix.

The new query that will work universally for all pools is TBD: #6064

I see. I ask because we do lean on SpotPrice in volume splitting incentives so there might be an implication here that it wouldn't be possible to retrieve spot price for pools with too small of a spot price

Yeah - I thought the conclusion was that this is acceptable for the majority of the pools that we care about for volume splitting. Do you still have any concerns?

AlpinYukseloglu · 2023-08-31T23:04:55Z

x/concentrated-liquidity/types/constants.go

@@ -11,6 +11,8 @@ import (
 const (
 	// Precomputed values for min and max tick
 	MinInitializedTick, MaxTick int64 = -108000000, 342000000
+	MinInitializedTickV2        int64 = -270000000
+	MinCurrentTickV2            int64 = MinInitializedTickV2 - 1


Is this V2 distinction mainly to keep tests compatible?

Yes, "V2" notation is temporary. By the end of this refactor, anything "V2" will become the canonical.

We will still need to keep track of V1 threshold for deciding on what tick-to-price and price-to-tick conversation strategy to use. Note that for all pools, anything above the old threshold will use 18 decimal strategies and anything below the threshold 36 decimal strategies.

It will probably make sense to rename V1 MinInitializedTick to TickConversionThreshold. Similarly with V1 MinSpotPrice

AlpinYukseloglu

Acknowledging that this is an intermediate step, LGTM! Nice work. Had one remaining non-blocking question about how the new min spot price was derived.

AlpinYukseloglu · 2023-09-01T17:40:42Z

x/concentrated-liquidity/pool.go

@@ -164,10 +165,10 @@ func (k Keeper) CalculateSpotPrice(
 	}

 	if price.IsZero() {
-		return sdk.Dec{}, types.PriceBoundError{ProvidedPrice: price, MinSpotPrice: types.MinSpotPrice, MaxSpotPrice: types.MaxSpotPrice}
+		return sdk.Dec{}, types.PriceBoundError{ProvidedPrice: osmomath.BigDecFromSDKDec(price), MinSpotPrice: types.MinSpotPriceV2, MaxSpotPrice: types.MaxSpotPrice}
 	}
 	if price.GT(types.MaxSpotPrice) || price.LT(types.MinSpotPrice) {


I see. I ask because we do lean on SpotPrice in volume splitting incentives so there might be an implication here that it wouldn't be possible to retrieve spot price for pools with too small of a spot price

AlpinYukseloglu · 2023-09-01T17:47:11Z

x/concentrated-liquidity/types/constants.go

 	MinSpotPrice       = sdk.MustNewDecFromStr("0.000000000001") // 10^-12
+	MinSpotPriceBigDec = osmomath.BigDecFromSDKDec(MinSpotPrice)
+	MinSpotPriceV2     = osmomath.NewDecWithPrec(1, 30)


How was this min spot price picked? Also, it looks like MinSqrtPriceBigDec below still derives from the regular min spot price – is this intentional?

Yes, MinSqrtPriceBigDec refers to V1 (old min). It is temporary and will be removed on completion.

10^-30 is the minimum we can support given our BigDec precision of 36 and exponent at price one of -6.

So 30 + 6 = 36. If interested, happy to breakdown the reasons in more detail but this stems from how we do price-to-tick conversion. Workaround could be found but was deemed unnecessary since the current min should be enough.

…nversions in a state-compatible way (#6256) * refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way * updates * changelog * refactor(CL): remove CalculatePriceToTickDecV1 function (#6258) * lint (cherry picked from commit 04655ba) # Conflicts: # CHANGELOG.md # x/concentrated-liquidity/lp.go # x/concentrated-liquidity/math/precompute.go # x/concentrated-liquidity/math/tick.go # x/concentrated-liquidity/math/tick_test.go # x/concentrated-liquidity/model/pool.go # x/concentrated-liquidity/pool.go # x/concentrated-liquidity/types/constants.go # x/concentrated-liquidity/types/errors.go

…nversions in a state-compatible way (backport #6256)

…nversions in a state-compatible way (backport #6256) (#6273) * refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way (backport #6256) * fix * conflict * lint --------- Co-authored-by: roman <[email protected]>

…nversions in a state-compatible way (#6256) * refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way * updates * changelog * refactor(CL): remove CalculatePriceToTickDecV1 function (#6258) * lint (cherry picked from commit 04655ba) # Conflicts: # x/concentrated-liquidity/lp.go # x/concentrated-liquidity/math/precompute.go # x/concentrated-liquidity/math/tick.go # x/concentrated-liquidity/math/tick_test.go # x/concentrated-liquidity/model/pool.go # x/concentrated-liquidity/pool.go # x/concentrated-liquidity/types/constants.go # x/concentrated-liquidity/types/errors.go

…nversions in a state-compatible way (backport #6256)

…nversions in a state-compatible way (backport #6256) (#6291) Co-authored-by: Roman <[email protected]>

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec co…

2008b4f

…nversions in a state-compatible way

p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v18.x backport patches to v18.x branch labels Aug 31, 2023

github-actions bot added the C:x/concentrated-liquidity label Aug 31, 2023

p0mvn commented Aug 31, 2023

View reviewed changes

updates

c050569

p0mvn commented Aug 31, 2023

View reviewed changes

changelog

81bda15

p0mvn mentioned this pull request Aug 31, 2023

feat(osmomath): mutative and efficient BigDec truncations with arbitrary decimals #6257

Closed

p0mvn marked this pull request as ready for review August 31, 2023 19:54

p0mvn requested review from czarcas7ic and AlpinYukseloglu as code owners August 31, 2023 19:54

AlpinYukseloglu reviewed Aug 31, 2023

View reviewed changes

refactor(CL): remove CalculatePriceToTickDecV1 function (#6258)

cb9fb12

AlpinYukseloglu approved these changes Sep 1, 2023

View reviewed changes

p0mvn added 2 commits September 1, 2023 20:41

Merge branch 'main' into roman/ref-pricetotick

20f430c

lint

3f82d1b

p0mvn merged commit 04655ba into main Sep 1, 2023
1 check passed

p0mvn deleted the roman/ref-pricetotick branch September 1, 2023 21:12

mergify bot mentioned this pull request Sep 1, 2023

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way (backport #6256) #6273

Merged

p0mvn added a commit that referenced this pull request Sep 1, 2023

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec co…

d1ff02f

…nversions in a state-compatible way (backport #6256)

p0mvn added the A:backport/v19.x backport patches to v19.x branch label Sep 3, 2023

mergify bot mentioned this pull request Sep 3, 2023

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way (backport #6256) #6291

Merged

p0mvn mentioned this pull request Sep 5, 2023

osmomath: mutative and efficient BigDec truncations with arbitrary decimals #6261

Merged

p0mvn added a commit that referenced this pull request Sep 7, 2023

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec co…

4bf702d

…nversions in a state-compatible way (backport #6256)

p0mvn added a commit that referenced this pull request Sep 7, 2023

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec co…

24c4100

…nversions in a state-compatible way (backport #6256) (#6291) Co-authored-by: Roman <[email protected]>

github-actions bot mentioned this pull request May 1, 2024

Broken Links Detected #8183

Closed

github-actions bot mentioned this pull request Oct 15, 2024

Broken Links Detected notional-labs/osmosis#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way #6256

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way #6256

p0mvn commented Aug 31, 2023

p0mvn Aug 31, 2023 •

edited

Loading

p0mvn Aug 31, 2023 •

edited

Loading

p0mvn Aug 31, 2023

p0mvn commented Aug 31, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Aug 31, 2023

p0mvn Aug 31, 2023

AlpinYukseloglu Sep 1, 2023

p0mvn Sep 1, 2023

AlpinYukseloglu Aug 31, 2023

p0mvn Aug 31, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Sep 1, 2023

AlpinYukseloglu Sep 1, 2023

p0mvn Sep 1, 2023

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way #6256

refactor(CL): convert CalculatePriceToTickDec to operate on BigDec conversions in a state-compatible way #6256

Conversation

p0mvn commented Aug 31, 2023

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

p0mvn Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

p0mvn Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn commented Aug 31, 2023

AlpinYukseloglu left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0mvn Aug 31, 2023 •

edited

Loading

p0mvn Aug 31, 2023 •

edited

Loading