refactor(CL): convert priceLimit API in swaps to BigDec #6368

p0mvn · 2023-09-11T16:18:07Z

Closes: #6355

What is the purpose of the change

State-compatible API change to the priceLimit swaps API needed to support the extended price range.

To be backported and validated on a v19.x node.

Testing and Verifying

This change is already covered by existing tests, such as (please describe 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

p0mvn · 2023-09-11T16:22:31Z

x/concentrated-liquidity/swaps_test.go

-		amountOneOut, _, _ := s.computeSwapAmountsInGivenOut(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTickV2, true, false)
+		amountOneOut, _, _ := s.computeSwapAmountsInGivenOut(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTick, true, false)

 		// estimate the amount in to fund
-		amountZeroIn, _, _ := s.computeSwapAmounts(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTickV2, true, false)
+		amountZeroIn, _, _ := s.computeSwapAmounts(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTick, true, false)


Note: swaps to v2 min tick are not enabled anymore.

Previously the estimation would stop the current min tick anyways, so this request to estimate till MinInitializedTickV2 is invalid at this time. To be re-added in a future PR.

The definition for min initialized tick will be the same across all pools at this time though right? So we shouldn't need to name it something different if I am understanding correctly.

Is this tracked in an issue?

Already addressed here:
https://github.com/osmosis-labs/osmosis/pull/6380/files#r1325756769

The definition for min initialized tick will be the same across all pools at this time though right? So we shouldn't need to name it something different if I am understanding correctly.

See: #6318

czarcas7ic

LGTM, just had question on comment vs actual code

czarcas7ic · 2023-09-13T14:16:42Z

x/concentrated-liquidity/swaps_test.go

-		amountOneOut, _, _ := s.computeSwapAmountsInGivenOut(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTickV2, true, false)
+		amountOneOut, _, _ := s.computeSwapAmountsInGivenOut(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTick, true, false)

 		// estimate the amount in to fund
-		amountZeroIn, _, _ := s.computeSwapAmounts(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTickV2, true, false)
+		amountZeroIn, _, _ := s.computeSwapAmounts(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTick, true, false)


The definition for min initialized tick will be the same across all pools at this time though right? So we shouldn't need to name it something different if I am understanding correctly.

czarcas7ic · 2023-09-13T14:19:02Z

x/concentrated-liquidity/swapstrategy/swap_strategy.go

 	if priceLimit.IsZero() {
 		if zeroForOne {
-			return osmomath.BigDecFromDec(types.MinSqrtPrice), nil
+			return types.MinSqrtPriceBigDec, nil
 		}
 		return osmomath.BigDecFromDec(types.MaxSqrtPrice), nil
 	}


Comment says if price limit is zero and strat is one for zero, max is returned. But I don't see that logic here.

Hmm don't the first few lines of the function do just that?

AlpinYukseloglu

Nice work, LGTM. Left a couple non blocking comments about capturing our offline discussion on monotonicity in comments and clarifying a point about MinSpotPriceV2

AlpinYukseloglu · 2023-09-14T05:39:55Z

x/concentrated-liquidity/swaps_test.go

-		amountOneOut, _, _ := s.computeSwapAmountsInGivenOut(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTickV2, true, false)
+		amountOneOut, _, _ := s.computeSwapAmountsInGivenOut(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTick, true, false)

 		// estimate the amount in to fund
-		amountZeroIn, _, _ := s.computeSwapAmounts(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTickV2, true, false)
+		amountZeroIn, _, _ := s.computeSwapAmounts(poolId, pool.GetCurrentSqrtPrice(), types.MinInitializedTick, true, false)


Is this tracked in an issue?

x/concentrated-liquidity/swapstrategy/swap_strategy.go

AlpinYukseloglu · 2023-09-14T05:43:29Z

x/concentrated-liquidity/swapstrategy/swap_strategy.go

 	if priceLimit.IsZero() {
 		if zeroForOne {
-			return osmomath.BigDecFromDec(types.MinSqrtPrice), nil
+			return types.MinSqrtPriceBigDec, nil
 		}
 		return osmomath.BigDecFromDec(types.MaxSqrtPrice), nil
 	}


Hmm don't the first few lines of the function do just that?

AlpinYukseloglu · 2023-09-14T05:46:50Z

x/concentrated-liquidity/swapstrategy/swap_strategy.go

 		}
 		return osmomath.BigDecFromDec(types.MaxSqrtPrice), nil
 	}

-	sqrtPriceLimit, err := osmomath.MonotonicSqrt(priceLimit)
+	if priceLimit.LT(types.MinSpotPriceV2) || priceLimit.GT(types.MaxSpotPriceBigDec) {


Trying to reconcile this with the "swaps to MinInitializedTickV2 are not enabled anymore" comment above in swap tests section – should this then be checking against regular min spot price (MinSpotPriceBigDec) or have I misunderstood the issue?

This was more of an added guard check for the future. It is guaranteed by the caller that priceLimit is above types.MinSpotPriceBigDec. As a result, state-compatibility is ensured

I made the check to adhere to v2 minimum right away to avoid refactoring the tests in the future PRs in the sequence.

Realize now that should have avoided doing that for clarity in the review. Will take a step by step approach in the future

AlpinYukseloglu · 2023-09-14T05:50:58Z

x/concentrated-liquidity/swapstrategy/swap_strategy_test.go

+		oneULPUnderThreshold = types.MinSpotPriceBigDec.Sub(oneULPBigDec)
+		atThreshold          = types.MinSpotPriceBigDec
+		oneULPAboveThreshold = types.MinSpotPriceBigDec.Add(oneULPBigDec)
+	)


Note to capture an offline discussion we had for posterity: this threshold does not break monotonicity because it is at a value that is equivalent in regular Dec and BigDec domains.

Specifically, calling the conversion of the threshold spot price from BigDec to Dec is a misnomer because nothing is actually being "truncated" (which would break monotonicity) – the padded 18 zeroes are simply being trimmed

It might be worthwhile to capture this in comments somewhere if it isn't already (sorry if it is and I missed it)

This is unrelated to testing the monotonicity of tick-to-sqrt price conversions that can be found in the rest of the test suite. It simply tests the execution flow of GetSqrtPriceLimit.

Added clarifying comment: e46ef8a

Co-authored-by: Alpo <[email protected]>

* refactor(CL): convert priceLimit API in swaps to BigDec * changelog * comment updates * remove unused constant * fix test * tests for GetSqrtPriceLimit * Update x/concentrated-liquidity/swapstrategy/swap_strategy.go Co-authored-by: Alpo <[email protected]> * clarifying comment --------- Co-authored-by: Alpo <[email protected]> (cherry picked from commit 3f15d3d) # Conflicts: # CHANGELOG.md # x/concentrated-liquidity/export_test.go # x/concentrated-liquidity/fuzz_test.go # x/concentrated-liquidity/keeper_test.go # x/concentrated-liquidity/position_test.go # x/concentrated-liquidity/range_test.go # x/concentrated-liquidity/spread_rewards_test.go # x/concentrated-liquidity/swaps.go # x/concentrated-liquidity/swaps_test.go # x/concentrated-liquidity/swaps_tick_cross_test.go # x/concentrated-liquidity/swapstrategy/swap_strategy.go # x/concentrated-liquidity/swapstrategy/swap_strategy_test.go # x/concentrated-liquidity/swapstrategy/zero_for_one.go # x/concentrated-liquidity/types/constants.go

…) (#6408) * refactor(CL): convert priceLimit API in swaps to BigDec (backport #6368) * lint --------- Co-authored-by: roman <[email protected]>

refactor(CL): convert priceLimit API in swaps to BigDec

450a51a

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

github-actions bot added the C:x/concentrated-liquidity label Sep 11, 2023

changelog

a6b868f

p0mvn force-pushed the roman/price-limit-api branch from bc88430 to a6b868f Compare September 11, 2023 16:19

p0mvn commented Sep 11, 2023

View reviewed changes

p0mvn added 4 commits September 11, 2023 12:23

comment updates

640bf20

remove unused constant

ae09d3a

fix test

8de14f0

tests for GetSqrtPriceLimit

aef2e81

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

p0mvn marked this pull request as ready for review September 11, 2023 17:31

p0mvn requested review from czarcas7ic and AlpinYukseloglu as code owners September 11, 2023 17:31

czarcas7ic approved these changes Sep 13, 2023

View reviewed changes

AlpinYukseloglu approved these changes Sep 14, 2023

View reviewed changes

p0mvn and others added 3 commits September 14, 2023 06:43

Update x/concentrated-liquidity/swapstrategy/swap_strategy.go

cdcf47b

Co-authored-by: Alpo <[email protected]>

clarifying comment

e46ef8a

Merge branch 'main' into roman/price-limit-api

d737fa2

p0mvn merged commit 3f15d3d into main Sep 14, 2023
1 check passed

p0mvn deleted the roman/price-limit-api branch September 14, 2023 11:17

mergify bot mentioned this pull request Sep 14, 2023

refactor(CL): convert priceLimit API in swaps to BigDec (backport #6368) #6408

Merged

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

refactor(CL): convert priceLimit API in swaps to BigDec (backport #6368)

d589e8d

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

Broken Links Detected #8183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(CL): convert priceLimit API in swaps to BigDec #6368

refactor(CL): convert priceLimit API in swaps to BigDec #6368

p0mvn commented Sep 11, 2023

p0mvn Sep 11, 2023

czarcas7ic Sep 13, 2023

AlpinYukseloglu Sep 14, 2023

p0mvn Sep 14, 2023

p0mvn Sep 14, 2023

czarcas7ic left a comment

czarcas7ic Sep 13, 2023

czarcas7ic Sep 13, 2023

AlpinYukseloglu Sep 14, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Sep 14, 2023

AlpinYukseloglu Sep 14, 2023

AlpinYukseloglu Sep 14, 2023

p0mvn Sep 14, 2023

AlpinYukseloglu Sep 14, 2023

AlpinYukseloglu Sep 14, 2023

p0mvn Sep 14, 2023

refactor(CL): convert priceLimit API in swaps to BigDec #6368

refactor(CL): convert priceLimit API in swaps to BigDec #6368

Conversation

p0mvn commented Sep 11, 2023

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

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

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

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