refactor: state-compatible big decimal tick to sqrt price conversions #6317

p0mvn · 2023-09-05T19:34:58Z

Closes: #6046

What is the purpose of the change

This PR finalizes BigDec tick to sqrt price conversion infrastructure, making all relevant CL math functions to be ready to support the new extended [10^-30, 10^-12) price range at the lower end.

It maintains backwards compatibility with v19.x and earlier release lines by returning an error here:

osmosis/x/concentrated-liquidity/math/tick.go

Lines 257 to 267 in b335df5

    
           // TODO: remove this check. It is present to maintain backwards state-compatibility with 
        
           // v19.x and earlier major releases of Osmosis. 
        
           // Once https://github.com/osmosis-labs/osmosis/issues/5726 is fully complete, 
        
           // this should be removed. 
        
           // 
        
           // Backwards state-compatibility is maintained by having the swap and LP logic error 
        
           // here in case the price/tick falls below the origina minimum tick bounds that are 
        
           // consistent with v19.x and earlier release lines. 
        
           if tick < types.MinCurrentTick { 
        
           	return 0, types.TickIndexMinimumError{MinTick: types.MinCurrentTick} 
        
           }

This would make swap or LP logic error consistently with v19.x and earlier releases if the current tick / spot price falls under the original minimum.

The linked check is to be removed in a subsequent PR.

Testing and Verifying

Covered by existing tests
For each changed function, added unit tests at the new price range and the edges.

Drive By Changes

Removed obsolete price return from TickToSqrtPrice that was unused anywhere

Notes

"V2" notation for min tick and min spot price is temporary and is to be replaced to be the canonical upon epic completion

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

AlpinYukseloglu

Nice work. Had a question about monotonicity across the MinInitialized tick boundary (see comment)

x/concentrated-liquidity/math/tick.go

mattverse

LGTM

mattverse · 2023-09-07T07:23:46Z

x/concentrated-liquidity/types/constants.go

+	// Tick corresponding to the at launch min spot price of 10^-12.
 	MinInitializedTick, MaxTick int64 = -108000000, 342000000


Any thoughts on calling these LegacyMinInitializedTick and LegacyMaxTick?

This naming is temporary and will be changed on completion. Ref: #6318

I like the suggestion and will track in #6318

For now, would prefer to keep it as is to avoid causing conflicts for the PR sequence

x/concentrated-liquidity/types/constants.go

mattverse · 2023-09-07T10:31:57Z

x/concentrated-liquidity/math/tick_test.go

+			tickIndex:     types.MinCurrentTickV2,
+			expectedPrice: types.MinSpotPriceV2,
+		},
+		"tickIndex is MinInitializedTick + 1 ULP": {


just out of curiosity, what does ULP here stand for?

Unit in the last place https://en.wikipedia.org/wiki/Unit_in_the_last_place

x/concentrated-liquidity/math/tick_test.go

mattverse · 2023-09-07T10:34:04Z

Also, as discussed in previous PRs, please share results of testing state compatibility after all big dec conversion PRs have been merged

AlpinYukseloglu

LGTM, nice work. Had a minor concern around spot price similarity between min initialized and min current ticks, but I believe it's safe so approving and leaving that comment as a clarifying question.

x/concentrated-liquidity/math/tick.go

AlpinYukseloglu · 2023-09-07T22:32:13Z

x/concentrated-liquidity/math/tick.go

+	// Given BigDec's precision of 36, that cannot be supported.
+	// The fact that MinInitializedTickV2 and MinCurrentTickV2 translate to the same
+	// price is acceptable since MinCurrentTickV2 cannot be initialized.
+	if tickIndex == types.MinInitializedTickV2 || tickIndex == types.MinCurrentTickV2 {


Hmm this feels like a generally risky thing to do, but just to confirm that it's safe as is, the claim is that one would never be able to create a position on MinCurrentTickV2 and so it's okay if this returns the same price as MinInitializedTickV2?

Yes, MinCurrentTickV2 can only be reached if all liquidity is drained when swapping zero for one.

However, it does not contain any liquidity itself. Before the swap can begin, we must first cross MinInitializedTickV2 and kick in liquidity into the bucket.

Therefore, the price translation of MinCurrentTickV2 does not play any significant role here

…#6317) * refactor: state-compatible big decimal tick to sqrt price conversions * updates and clean ups * lint (cherry picked from commit 8ac3efb) # Conflicts: # tests/e2e/e2e_cl_test.go # x/concentrated-liquidity/math/math_test.go # x/concentrated-liquidity/math/tick.go # x/concentrated-liquidity/math/tick_test.go # x/concentrated-liquidity/query.go # x/concentrated-liquidity/types/constants.go

… (backport #6317) (#6346) * updates * lint * update fuzz * go mod for osmomath and go work sync --------- Co-authored-by: Roman <[email protected]>

refactor: state-compatible big decimal tick to sqrt price conversions

b335df5

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

p0mvn added V:state/compatible/backport State machine compatible PR, should be backported A:backport/v18.x backport patches to v18.x branch A:backport/v19.x backport patches to v19.x branch and removed C:x/concentrated-liquidity labels Sep 5, 2023

p0mvn added 2 commits September 5, 2023 15:42

updates and clean ups

c2af17f

lint

3e51683

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

p0mvn mentioned this pull request Sep 5, 2023

refactor: breaking CL tick to sqrt price math min spot price relaxation #6319

Merged

6 tasks

p0mvn marked this pull request as ready for review September 5, 2023 20:37

p0mvn requested review from czarcas7ic and AlpinYukseloglu as code owners September 5, 2023 20:37

AlpinYukseloglu reviewed Sep 5, 2023

View reviewed changes

x/concentrated-liquidity/math/tick.go Show resolved Hide resolved

mattverse approved these changes Sep 7, 2023

View reviewed changes

p0mvn requested a review from AlpinYukseloglu September 7, 2023 14:09

AlpinYukseloglu approved these changes Sep 8, 2023

View reviewed changes

p0mvn removed the A:backport/v18.x backport patches to v18.x branch label Sep 8, 2023

p0mvn merged commit 8ac3efb into main Sep 8, 2023

p0mvn deleted the roman/state-compat-tick-to-price branch September 8, 2023 15:43

mergify bot mentioned this pull request Sep 8, 2023

refactor: state-compatible big decimal tick to sqrt price conversions (backport #6317) #6346

Merged

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

Broken Links Detected #8046

Closed

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: state-compatible big decimal tick to sqrt price conversions #6317

refactor: state-compatible big decimal tick to sqrt price conversions #6317

p0mvn commented Sep 5, 2023 •

edited

Loading

AlpinYukseloglu left a comment

mattverse left a comment

mattverse Sep 7, 2023

p0mvn Sep 7, 2023

mattverse Sep 7, 2023

p0mvn Sep 7, 2023

mattverse commented Sep 7, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Sep 7, 2023

p0mvn Sep 8, 2023

	// TODO: remove this check. It is present to maintain backwards state-compatibility with
	// v19.x and earlier major releases of Osmosis.
	// Once https://github.com/osmosis-labs/osmosis/issues/5726 is fully complete,
	// this should be removed.
	//
	// Backwards state-compatibility is maintained by having the swap and LP logic error
	// here in case the price/tick falls below the origina minimum tick bounds that are
	// consistent with v19.x and earlier release lines.
	if tick < types.MinCurrentTick {
	return 0, types.TickIndexMinimumError{MinTick: types.MinCurrentTick}
	}

		// Tick corresponding to the at launch min spot price of 10^-12.
		MinInitializedTick, MaxTick int64 = -108000000, 342000000

refactor: state-compatible big decimal tick to sqrt price conversions #6317

refactor: state-compatible big decimal tick to sqrt price conversions #6317

Conversation

p0mvn commented Sep 5, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Drive By Changes

Notes

Documentation and Release Note

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

mattverse left a comment

Choose a reason for hiding this comment

mattverse Sep 7, 2023

Choose a reason for hiding this comment

p0mvn Sep 7, 2023

Choose a reason for hiding this comment

mattverse Sep 7, 2023

Choose a reason for hiding this comment

p0mvn Sep 7, 2023

Choose a reason for hiding this comment

mattverse commented Sep 7, 2023

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

AlpinYukseloglu Sep 7, 2023

Choose a reason for hiding this comment

p0mvn Sep 8, 2023

Choose a reason for hiding this comment

p0mvn commented Sep 5, 2023 •

edited

Loading