[CL]: Increase min tick past canonical tick range #5551
Conversation
github-actions
bot
added
C:x/gamm
| defaultErrorTolerance := osmomath.ErrTolerance{ | ||
| AdditiveTolerance: sdk.NewDec(100), | ||
| AdditiveTolerance: sdk.NewDec(100000), |
There was a problem hiding this comment.
Note to reviewer: sorry for the large comment here, but wanted to make sure our tolerances aren't arbitrary.
A good sanity check for this change is that increasing min price by 10^6 increases min sqrt price by 10^3, so it follows that liquidity calculations (most of which are done in sqrt price domain should shift by 10^3
| @@ -162,7 +162,7 @@ func (s *KeeperTestSuite) TestUpdateHighestLiquidityPools() { | |||
| }, | |||
| expectedBaseDenomPools: map[string]map[string]keeper.LiquidityPoolStruct{ | |||
| "epochTwo": { | |||
| "uosmo": {Liquidity: sdk.Int(sdk.NewUintFromString("999999999000000001000000000000000000")), PoolId: 49}, | |||
| "uosmo": {Liquidity: sdk.Int(sdk.NewUintFromString("999999000000000001000000000000000000")), PoolId: 49}, | |||
There was a problem hiding this comment.
Note to reviewer: this number seemed right based on the migration calculations (above) and superfluid calculations (below), but admittedly I did slightly eyeball this as the underlying test components are quite obscure
(there's 0 documentation on where this number comes from, although I can infer it's the asset1 amount for a set of full range positions)
AlpinYukseloglu
requested review from
mattverse,
stackman27,
p0mvn and
czarcas7ic
as code owners
| // Note that these numbers were calculated using `GetLiquidityFromAmounts` and `TickToSqrtPrice` and thus assume correctness of those functions. | ||
| // https://www.wolframalpha.com/input?i=212041526.154556192317664016+*+%2870.728769315114743566+-+0.000001000000000000%29 | ||
| amount1Expected: sdk.NewInt(14997435977), |
There was a problem hiding this comment.
Note that these numbers were calculated using
GetLiquidityFromAmountsandTickToSqrtPriceand thus assume correctness of those functions.
Are you explaining being off by one compared to the Wolfram calculation?
There was a problem hiding this comment.
Ah yes sorry – the expected output is bankers rounded instead of truncated due to GetLiquidityFromAmounts being bankers rounded. This does not pose any security risks as the balance is still deducted from the caller.
| @@ -1405,89 +1405,3 @@ func (s *KeeperTestSuite) TestValidateTickRangeIsValid() { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func (s *KeeperTestSuite) TestRoundTickToCanonicalPriceTick() { | |||
There was a problem hiding this comment.
Why is this removed? Is there a replacement for these test vectors?
There was a problem hiding this comment.
The notion of canonical price ticks no longer exist, so these vectors no longer make sense. All the test vectors added relating to the behavior in ticks surrounding the new MinTick ensure the appropriate properties hold.
| "smallest + min price * increment 10^12": { | ||
| price: sdk.MustNewDecFromStr("1.000000000000000000"), | ||
| tickExpected: 0, | ||
| }, |
There was a problem hiding this comment.
Does this title reflect this test case? I think the previous one might have been incorrect as well right?
There was a problem hiding this comment.
Thanks for flagging this, fixed
…abs/osmosis into alpo/increase-min-tick
|
Addressed all comments, will merge once CI passes |
Closes: #5550
What is the purpose of the change
This PR moves the min tick & min spot price for CL up to past the point where canonical ticks start to matter.
Due to a majority of test vectors relating to min tick or full range being hard coded and/or manually computed, this change broke many more tests than we originally expected. Most of them have been amended and modularized to not break for future changes, and the remaining ones were recomputed with the math documented in comments
Testing and Verifying
All relevant tests were amended to refer to the new min tick/min price. Where applicable,
types.MinTickwas used in place of hard coded values to make future changes break fewer tests.Documentation and Release Note
Unreleasedsection ofCHANGELOG.md?Where is the change documented?
x/{module}/README.md)