refactor(CL): change tick API from sdk.Dec to osmomath.BigDec #6033

p0mvn · 2023-08-13T19:21:35Z

Closes: #6032

What is the purpose of the change

Please see #6032 for a detailed explanation of the motivations behind this refactor.

The primary reason for doing this is to have an osmomath.BigDec API in tick conversions to later allow us to support pools with sdk.Dec osmomath.BigDec sqrt price ticks. This will allow us to support pools with min spot price of 10^-12 and 10^-30 without migrations.

A secondary benefit of this refactor is minimizing conversions between osmomath.BigDec and sdk.Dec due to poor choice of API when doing #5612 pre-launch.

This change is expected to be fully state-compatible. To confirm, a node should be synched starting from v16.0.0

TDD: https://app.clickup.com/37420681/v/dc/13nzm9-22453

Testing and Verifying

Covered by existing tests. All higher-level of abstraction tests continue to pass (such as swap_test.go and lp_test.go), proving that there should be no changes to the logic, only the APIs.

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

mattverse

LGTM,

The only question / concern I have is regarding state consistency, is the claim that since we are changing to minimum of 12 precision to 30 and cause we're going to a more precise precision, there will be no state break, only have spot price precision increase for future pools?

mattverse · 2023-08-14T05:31:31Z

x/concentrated-liquidity/range_test.go

@@ -439,7 +439,7 @@ func (s *KeeperTestSuite) getInitialPositionAssets(pool types.ConcentratedPoolEx

 	// Calculate asset amounts that would be required to get the required spot price (rounding up on asset1 to ensure we stay in the intended tick)
 	asset0Amount := sdk.NewInt(100000000000000)
-	asset1Amount := sdk.NewDecFromInt(asset0Amount).Mul(requiredPrice).Ceil().TruncateInt()
+	asset1Amount := sdk.NewDecFromInt(asset0Amount).Mul(requiredPrice.SDKDec()).Ceil().TruncateInt()


Asking to learn: for these places where we turn bigdec into sdk dec, how do we ensure it is safe to do truncations?

Our underlying sqrt function operates on the sdk.Dec.

As a result, all tick sqrt prices have 18 decimals despite being osmomath.BigDec with precision of 36.

That is, they get computed to something like 1.123456789123456789000000000000000000` where the last 18 decimals are just zeroes.

However, our "current sqrt price" can fluctuate beyond 18th decimal.

This PR makes the API for sqrt prices to be consistently 36 decimals. However, this change is state-compatible because we simply truncate tick data from 36 to 18 decimals in different places while ensuring 18 decimals by the sqrt function despite having a 36 decimal API.

In the future, this change will allow us to have two strategies for computing sqrt prices from ticks and vice versa. One with 18 decimals, and the other one with 36. As a result, we will be able to lower our min spot price for the strategy with 36 decimals. 18 decimals will continue to exist as is, thus, not requiring a state migration

p0mvn · 2023-08-14T08:30:44Z

LGTM,

The only question / concern I have is regarding state consistency, is the claim that since we are changing to minimum of 12 precision to 30 and cause we're going to a more precise precision, there will be no state break, only have spot price precision increase for future pools?

Tried to cover the state-break concerns in this reply. This PR will be validated against state breaks by synching a v16 node upon approval.

Additionally, note that we are not changing precision from 10^-12 to 10^-30. This change will allow us to do the following in the future:

Implement MonotonicSqrt with osmomath.BigDec (36 decimals)
Support tick conversions with both 18 and 36 decimal sqrt function
- For 18 decimals, keep having a minimum spot price of 10^-12 as it is today
- For 36 decimals, lower the minimum spot price to 10^-36 to resolve our current restrictions stemming from varying base and quote asset denom precisions

Supporting two kinds of tick conversions will let us avoid having to do a state migration for currently existing CL pools

Please let me know if further questions.

mattverse

Ok, I think the change makes sense to me, also please make sure you test this on a node and have it running for a day or so to test this is not state breaking!

p0mvn · 2023-08-21T10:18:23Z

x/concentrated-liquidity/swapstrategy/one_for_zero.go

-func (s oneForZeroStrategy) ComputeSwapWithinBucketOutGivenIn(sqrtPriceCurrent osmomath.BigDec, sqrtPriceTarget, liquidity, amountOneInRemaining sdk.Dec) (osmomath.BigDec, sdk.Dec, sdk.Dec, sdk.Dec) {
-	sqrtPriceTargetBigDec := osmomath.BigDecFromSDKDec(sqrtPriceTarget)
+func (s oneForZeroStrategy) ComputeSwapWithinBucketOutGivenIn(sqrtPriceCurrent, sqrtPriceTarget osmomath.BigDec, liquidity, amountOneInRemaining sdk.Dec) (osmomath.BigDec, sdk.Dec, sdk.Dec, sdk.Dec) {
 	liquidityBigDec := osmomath.BigDecFromSDKDec(liquidity)
 	amountOneInRemainingBigDec := osmomath.BigDecFromSDKDec(amountOneInRemaining)


Example link

AlpinYukseloglu

LGTM, nice work, especially with the cleanup of all the Dec -> BigDec conversions

AlpinYukseloglu · 2023-08-22T15:09:05Z

x/concentrated-liquidity/incentives.go

@@ -200,7 +201,7 @@ func (k Keeper) prepareBalancerPoolAsFullRange(ctx sdk.Context, clPoolId uint64,
 	// Calculate the amount of liquidity the Balancer amounts qualify in the CL pool. Note that since we use the CL spot price, this is
 	// safe against prices drifting apart between the two pools (we take the lower bound on the qualifying liquidity in this case).
 	// The `sqrtPriceLowerTick` and `sqrtPriceUpperTick` fields are set to the appropriate values for a full range position.
-	qualifyingFullRangeSharesPreDiscount := math.GetLiquidityFromAmounts(clPool.GetCurrentSqrtPrice(), types.MinSqrtPrice, types.MaxSqrtPrice, asset0Amount, asset1Amount)
+	qualifyingFullRangeSharesPreDiscount := math.GetLiquidityFromAmounts(clPool.GetCurrentSqrtPrice(), osmomath.BigDecFromSDKDec(types.MinSqrtPrice), osmomath.BigDecFromSDKDec(types.MaxSqrtPrice), asset0Amount, asset1Amount)


Out of curiosity, is there a reason we cast here instead of just directly changing types.MinSqrtPrice and types.MaxSqrtPrice? Is this mainly to keep v1 pools on old precision?

I'm planning to change that incrementally in a future refactor.

Many of the tests / other logic still rely on min and max being `sdk.Dec so I've been trying to limit the scope of changes in this PR

AlpinYukseloglu · 2023-08-22T15:33:54Z

x/concentrated-liquidity/math/tick_test.go

 			tickSpacing:  1,
 			tickExpected: types.MinInitializedTick + 2,
 		},
 		"sqrt price corresponds exactly to min tick + 2 minus 1 ULP (tick spacing 1)": {
 			// Calculated using TickToSqrtPrice(types.MinInitializedTick + 2) - 1 ULP
-			sqrtPrice:    sqpMinTickPlusTwo.Sub(sdk.SmallestDec()),
+			sqrtPrice:    sqpMinTickPlusTwo.Sub(sdkULP),


Not sure if it's worth the time to convert these to be actual 1 ULP diffs, but just flagging that using sdkULP tests a qualitatively very different thing (the original tests were essentially very granular boundary checks)

Not sure what you mean here. It should still be equivalent to the previous logic sdkULP is 10^-18 which is the same as sdk.SmallestDec().

Followed up in DM

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201) # Conflicts: # CHANGELOG.md # x/concentrated-liquidity/math/tick.go # x/concentrated-liquidity/math/tick_test.go # x/concentrated-liquidity/position_test.go # x/concentrated-liquidity/swaps.go # x/concentrated-liquidity/swaps_tick_cross_test.go

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201)

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201)

…#6262) * refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201) Co-authored-by: Roman <[email protected]>

…#6245) * refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201) Co-authored-by: Roman <[email protected]>

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

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

p0mvn marked this pull request as ready for review August 13, 2023 19:38

p0mvn requested review from czarcas7ic and AlpinYukseloglu as code owners August 13, 2023 19:38

mattverse reviewed Aug 14, 2023

View reviewed changes

p0mvn added 2 commits August 20, 2023 10:46

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec

01958a4

changelog

d7c6129

p0mvn force-pushed the roman/tick-api-big-dec branch from 99349ff to d7c6129 Compare August 20, 2023 08:47

mattverse approved these changes Aug 21, 2023

View reviewed changes

p0mvn commented Aug 21, 2023

View reviewed changes

AlpinYukseloglu approved these changes Aug 22, 2023

View reviewed changes

p0mvn added the A:backport/v17.x backport patches to v17.x branch label Aug 23, 2023

p0mvn merged commit 36e7201 into main Aug 23, 2023

p0mvn deleted the roman/tick-api-big-dec branch August 23, 2023 09:15

This was referenced Aug 23, 2023

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (backport #6033) #6135

Closed

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (backport #6033) #6136

Closed

mergify bot pushed a commit that referenced this pull request Aug 23, 2023

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (#6033)

d000417

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201)

p0mvn added the A:backport/v18.x backport patches to v18.x branch label Aug 30, 2023

mergify bot pushed a commit that referenced this pull request Aug 30, 2023

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (#6033)

c3bf42c

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201)

mergify bot mentioned this pull request Aug 30, 2023

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (backport #6033) #6245

Merged

nicolaslara pushed a commit that referenced this pull request Aug 31, 2023

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (#6033)

6713d08

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog

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

mergify bot pushed a commit that referenced this pull request Aug 31, 2023

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (#6033)

211b0a1

* refactor(CL): change tick API from sdk.Dec to osmomath.BigDec * changelog (cherry picked from commit 36e7201)

mergify bot mentioned this pull request Aug 31, 2023

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec (backport #6033) #6262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec #6033

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec #6033

p0mvn commented Aug 13, 2023 •

edited

Loading

mattverse left a comment

mattverse Aug 14, 2023

p0mvn Aug 14, 2023

p0mvn commented Aug 14, 2023 •

edited

Loading

mattverse left a comment

p0mvn Aug 21, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Aug 22, 2023

p0mvn Aug 23, 2023

AlpinYukseloglu Aug 22, 2023

p0mvn Aug 23, 2023 •

edited

Loading

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec #6033

refactor(CL): change tick API from sdk.Dec to osmomath.BigDec #6033

Conversation

p0mvn commented Aug 13, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

mattverse left a comment

Choose a reason for hiding this comment

mattverse Aug 14, 2023

Choose a reason for hiding this comment

p0mvn Aug 14, 2023

Choose a reason for hiding this comment

p0mvn commented Aug 14, 2023 • edited Loading

mattverse left a comment

Choose a reason for hiding this comment

p0mvn Aug 21, 2023

Choose a reason for hiding this comment

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

AlpinYukseloglu Aug 22, 2023

Choose a reason for hiding this comment

p0mvn Aug 23, 2023

Choose a reason for hiding this comment

AlpinYukseloglu Aug 22, 2023

Choose a reason for hiding this comment

p0mvn Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

p0mvn commented Aug 13, 2023 •

edited

Loading

p0mvn commented Aug 14, 2023 •

edited

Loading

p0mvn Aug 23, 2023 •

edited

Loading