From ad759f8d8d3ed161300dbd88d8155717ac18be71 Mon Sep 17 00:00:00 2001 From: Roman Date: Mon, 11 Sep 2023 14:59:03 -0400 Subject: [PATCH 1/4] refactor/test(CL): Stricter rounding behavior in CL math methods; unit tests at low price level --- CHANGELOG.md | 1 + x/concentrated-liquidity/math/math.go | 18 +++- x/concentrated-liquidity/math/math_test.go | 98 ++++++++++++++++++++++ x/concentrated-liquidity/python/clmath.py | 6 +- 4 files changed, 116 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 56ab25d6d15..8ce59efdbf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#6334](https://github.com/osmosis-labs/osmosis/pull/6334) fix: enable taker fee cli * [#6352](https://github.com/osmosis-labs/osmosis/pull/6352) Reduce error blow-up in CalcAmount0Delta by changing the order of math operations. +* [#6368](https://github.com/osmosis-labs/osmosis/pull/6368) Stricter rounding behavior in CL math's CalcAmount0Delta and GetNextSqrtPriceFromAmount0InRoundingUp ### API Breaks diff --git a/x/concentrated-liquidity/math/math.go b/x/concentrated-liquidity/math/math.go index d24ab58b110..c3d3a0b4b2b 100644 --- a/x/concentrated-liquidity/math/math.go +++ b/x/concentrated-liquidity/math/math.go @@ -18,6 +18,7 @@ func Liquidity0(amount osmomath.Int, sqrtPriceA, sqrtPriceB osmomath.BigDec) osm // We convert to BigDec to avoid precision loss when calculating liquidity. Without doing this, // our liquidity calculations will be off from our theoretical calculations within our tests. + // TODO (perf): consider better conversion helpers to minimize reallocations. amountBigDec := osmomath.BigDecFromDec(amount.ToLegacyDec()) product := sqrtPriceA.Mul(sqrtPriceB) @@ -26,6 +27,7 @@ func Liquidity0(amount osmomath.Int, sqrtPriceA, sqrtPriceB osmomath.BigDec) osm panic(fmt.Sprintf("liquidity0 diff is zero: sqrtPriceA %s sqrtPriceB %s", sqrtPriceA, sqrtPriceB)) } + // TODO (perf): consider Dec() function that does not reallocate return amountBigDec.MulMut(product).QuoMut(diff).Dec() } @@ -40,12 +42,14 @@ func Liquidity1(amount osmomath.Int, sqrtPriceA, sqrtPriceB osmomath.BigDec) osm // We convert to BigDec to avoid precision loss when calculating liquidity. Without doing this, // our liquidity calculations will be off from our theoretical calculations within our tests. + // TODO (perf): consider better conversion helpers to minimize reallocations. amountBigDec := osmomath.BigDecFromDec(amount.ToLegacyDec()) diff := sqrtPriceB.Sub(sqrtPriceA) if diff.IsZero() { panic(fmt.Sprintf("liquidity1 diff is zero: sqrtPriceA %s sqrtPriceB %s", sqrtPriceA, sqrtPriceB)) } + // TODO (perf): consider Dec() function that does not reallocate return amountBigDec.QuoMut(diff).Dec() } @@ -73,13 +77,19 @@ func CalcAmount0Delta(liq, sqrtPriceA, sqrtPriceB osmomath.BigDec, roundUp bool) // - calculating amountIn during swap // - adding liquidity (request user to provide more tokens in in favor of the pool) // The denominator is truncated to get a higher final amount. - return liq.MulRoundUp(diff).QuoRoundUp(sqrtPriceA).QuoRoundUp(sqrtPriceB).Ceil() + // Note that the order of divisions is important here. First, we divide by a larger number (sqrtPriceB) and then by a smaller number (sqrtPriceA). + // This leads to a smaller error amplification. + // TODO (perf): QuoRoundUpMut with no reallocation. + return liq.MulRoundUp(diff).QuoRoundUp(sqrtPriceB).QuoRoundUp(sqrtPriceA).Ceil() } // These are truncated at precision end to round in favor of the pool when: // - calculating amount out during swap // - withdrawing liquidity // Each intermediary step is truncated at precision end to get a smaller final amount. - return liq.MulTruncate(diff).QuoTruncate(sqrtPriceA).QuoTruncate(sqrtPriceB) + // Note that the order of divisions is important here. First, we divide by a larger number (sqrtPriceB) and then by a smaller number (sqrtPriceA). + // This leads to a smaller error amplification. + // TODO (perf): QuoTruncate with no reallocation. + return liq.MulTruncate(diff).QuoTruncate(sqrtPriceB).QuoTruncate(sqrtPriceA) } // CalcAmount1Delta takes the asset with the smaller liquidity in the pool as well as the sqrtpCur and the nextPrice and calculates the amount of asset 1 @@ -124,11 +134,11 @@ func GetNextSqrtPriceFromAmount0InRoundingUp(sqrtPriceCurrent, liquidity, amount return sqrtPriceCurrent } - product := amountZeroRemainingIn.Mul(sqrtPriceCurrent) + product := amountZeroRemainingIn.MulTruncate(sqrtPriceCurrent) // denominator = product + liquidity denominator := product denominator.AddMut(liquidity) - return liquidity.Mul(sqrtPriceCurrent).QuoRoundUp(denominator) + return liquidity.MulRoundUp(sqrtPriceCurrent).QuoRoundUp(denominator) } // GetNextSqrtPriceFromAmount0OutRoundingUp utilizes sqrtPriceCurrent, liquidity, and amount of denom0 that still needs diff --git a/x/concentrated-liquidity/math/math_test.go b/x/concentrated-liquidity/math/math_test.go index 0c2c943bcfa..6877a9fc9a7 100644 --- a/x/concentrated-liquidity/math/math_test.go +++ b/x/concentrated-liquidity/math/math_test.go @@ -19,6 +19,17 @@ var ( sqrt4545BigDec = osmomath.BigDecFromDec(sqrt4545) sqrt5000BigDec = osmomath.BigDecFromDec(sqrt5000) sqrt5500BigDec = osmomath.BigDecFromDec(sqrt5500) + + // sqrt(10 ^-36 * 567) 36 decimals + // chosen arbitrarily for testing the extended price range + sqrtANearMin = osmomath.MustNewBigDecFromStr("0.000000000000000023811761799581315315") + // sqrt(10 ^-36 * 123567) 36 decimals + // chosen arbitrarily for testing the extended price range + sqrtBNearMin = osmomath.MustNewBigDecFromStr("0.000000000000000351520980881653776714") + // This value is estimated using liquidity1 function in clmath.py between sqrtANearMin and sqrtBNearMin. + smallLiquidity = osmomath.MustNewBigDecFromStr("0.000000000000316705045072312223884779") + // Arbitrary small value, exists to test small movement over the low price range. + smallValue = osmomath.MustNewBigDecFromStr("10.12345678912345671234567891234567") ) // liquidity1 takes an amount of asset1 in the pool as well as the sqrtpCur and the nextPrice @@ -39,6 +50,15 @@ func TestLiquidity1(t *testing.T) { expectedLiquidity: "1517882343.751510418088349649", // https://www.wolframalpha.com/input?i=5000000000+%2F+%2870.710678118654752440+-+67.416615162732695594%29 }, + "low price range": { + currentSqrtP: sqrtANearMin, + sqrtPLow: sqrtBNearMin, + amount1Desired: osmomath.NewInt(5000000000), + // from math import * + // from decimal import * + // amount1 / (sqrtPriceB - sqrtPriceA) + expectedLiquidity: "15257428564277849269508363.222206252646611708", + }, } for name, tc := range testCases { @@ -76,6 +96,18 @@ func TestLiquidity0(t *testing.T) { expectedLiquidity: "1519437308.014768571720923239", // https://www.wolframalpha.com/input?i=1000000+*+%2870.710678118654752440*+74.161984870956629487%29+%2F+%2874.161984870956629487+-+70.710678118654752440%29 }, + "low price range": { + currentSqrtP: sqrtANearMin, + sqrtPHigh: sqrtBNearMin, + amount0Desired: osmomath.NewInt(123999), + // from clmath import * + // from math import * + // product1 = round_osmo_prec_down(sqrtPriceA * sqrtPriceB) + // product2 = round_osmo_prec_down(amount0 * product1) + // diff = round_osmo_prec_down(sqrtPriceB - sqrtPriceA) + // round_sdk_prec_down(product2 / diff) + expectedLiquidity: "0.000000000003167050", + }, } for name, tc := range testCases { @@ -191,6 +223,16 @@ func TestCalcAmount0Delta(t *testing.T) { isWithTolerance: true, amount0Expected: osmomath.MustNewBigDecFromStr("3546676037185128488234786333758360815266.999539026068480181194797910898392880"), }, + "low price range": { + liquidity: smallLiquidity, + sqrtPA: sqrtANearMin, + sqrtPB: sqrtBNearMin, + roundUp: false, + // from clmath decimal import * + // from math import * + // calc_amount_zero_delta(liq, sqrtPriceA, sqrtPriceB, False) + amount0Expected: osmomath.MustNewBigDecFromStr("12399.405290456300691064448232516066947340"), + }, } for name, tc := range testCases { @@ -277,6 +319,27 @@ func TestCalcAmount1Delta(t *testing.T) { roundUp: true, amount1Expected: osmomath.MustNewBigDecFromStr("28742157707995443393876876754535992.801567623738751734").Ceil(), // round up at precision end. }, + "low price range (no round up)": { + liquidity: smallLiquidity, + sqrtPA: sqrtANearMin, + sqrtPB: sqrtBNearMin, + roundUp: false, + // from clmath decimal import * + // from math import * + // calc_amount_one_delta(liq, sqrtPriceA, sqrtPriceB, False) + amount1Expected: osmomath.MustNewBigDecFromStr("0.000000000000000000000000000103787162"), + }, + "low price range (with round up)": { + liquidity: smallLiquidity, + sqrtPA: sqrtANearMin, + sqrtPB: sqrtBNearMin, + roundUp: true, + // from clmath decimal import * + // calc_amount_one_delta(liq, sqrtPriceA, sqrtPriceB, False) + // Actual result: 0.000000000000000000000000000103787163 + // Gets rounded up to 1. Is this acceptable when the multiplicative difference is so large? + amount1Expected: osmomath.MustNewBigDecFromStr("0.000000000000000000000000000103787163").Ceil(), + }, } for name, tc := range testCases { @@ -449,6 +512,14 @@ func TestGetNextSqrtPriceFromAmount0InRoundingUp(t *testing.T) { // round_osmo_prec_up(liquidity / (round_osmo_prec_down(liquidity / sqrtPriceCurrent) + amountRemaining)) expected: osmomath.MustNewBigDecFromStr("70.666663910857144331148691821263626767"), }, + "low price range": { + liquidity: smallLiquidity, + sqrtPriceCurrent: sqrtANearMin, + amountRemaining: smallValue, + // from clmath decimal import * + // get_next_sqrt_price_from_amount0_in_round_up(liq, sqrtPriceA, amountRemaining) + expected: osmomath.MustNewBigDecFromStr("0.000000000000000023793654323441728435"), + }, } runSqrtRoundingTestCase(t, "TestGetNextSqrtPriceFromAmount0InRoundingUp", math.GetNextSqrtPriceFromAmount0InRoundingUp, tests) } @@ -470,6 +541,14 @@ func TestGetNextSqrtPriceFromAmount0OutRoundingUp(t *testing.T) { // liq * sqrt_cur / (liq + token_out * sqrt_cur) = 2.5 expected: osmomath.MustNewBigDecFromStr("2.5"), }, + "low price range": { + liquidity: smallLiquidity, + sqrtPriceCurrent: sqrtANearMin, + amountRemaining: smallValue, + // from clmath decimal import * + // get_next_sqrt_price_from_amount0_out_round_up(liq, sqrtPriceA, amountRemaining) + expected: osmomath.MustNewBigDecFromStr("0.000000000000000023829902587267894423"), + }, } runSqrtRoundingTestCase(t, "TestGetNextSqrtPriceFromAmount0OutRoundingUp", math.GetNextSqrtPriceFromAmount0OutRoundingUp, tests) } @@ -499,6 +578,14 @@ func TestGetNextSqrtPriceFromAmount1InRoundingDown(t *testing.T) { // calculated with x/concentrated-liquidity/python/clmath.py round_decimal(sqrt_next, 36, ROUND_FLOOR) expected: osmomath.MustNewBigDecFromStr("70.738319930382329008049494613660784220"), }, + "low price range": { + liquidity: smallLiquidity, + sqrtPriceCurrent: sqrtANearMin, + amountRemaining: smallValue, + // from clmath decimal import * + // get_next_sqrt_price_from_amount1_in_round_down(liq, sqrtPriceA, amountRemaining) + expected: osmomath.MustNewBigDecFromStr("31964936923603.477920799226065501544948016880497639"), + }, } runSqrtRoundingTestCase(t, "TestGetNextSqrtPriceFromAmount1InRoundingDown", math.GetNextSqrtPriceFromAmount1InRoundingDown, tests) } @@ -519,6 +606,17 @@ func TestGetNextSqrtPriceFromAmount1OutRoundingDown(t *testing.T) { // round_osmo_prec_down(sqrtPriceCurrent - round_osmo_prec_up(tokenOut / liquidity)) expected: osmomath.MustNewBigDecFromStr("2.5"), }, + "low price range": { + liquidity: smallLiquidity, + sqrtPriceCurrent: sqrtANearMin, + amountRemaining: smallValue, + // from clmath decimal import * + // get_next_sqrt_price_from_amount1_out_round_down(liq, sqrtPriceA, amountRemaining) + // While a negative sqrt price value is invalid and should be caught by the caller, + // we mostly focus on testing rounding behavior and math correctness at low spot prices. + // For the purposes of our test, this result is acceptable. + expected: osmomath.MustNewBigDecFromStr("-31964936923603.477920799226065453921424417717867010"), + }, } runSqrtRoundingTestCase(t, "TestGetNextSqrtPriceFromAmount1OutRoundingDown", math.GetNextSqrtPriceFromAmount1OutRoundingDown, tests) } diff --git a/x/concentrated-liquidity/python/clmath.py b/x/concentrated-liquidity/python/clmath.py index 849500d6082..313efa8a8cb 100644 --- a/x/concentrated-liquidity/python/clmath.py +++ b/x/concentrated-liquidity/python/clmath.py @@ -1,4 +1,5 @@ from decimal import Decimal, ROUND_FLOOR, ROUND_CEILING, getcontext +from math import * class Coin: # Define this class based on what fields sdk.Coin has. @@ -114,12 +115,11 @@ def calc_amount_zero_delta(liquidity, sqrtPriceA, sqrtPriceB, roundUp): diff = sqrtPriceA - sqrtPriceB product_num = liquidity * diff - product_denom = sqrtPriceA * sqrtPriceB if roundUp: - return round_osmo_prec_up(round_osmo_prec_up(product_num) / round_osmo_prec_down(product_denom)) + return round_osmo_prec_up(round_osmo_prec_up(round_osmo_prec_up(product_num) / sqrtPriceA) / sqrtPriceB) - return round_osmo_prec_down(round_osmo_prec_down(product_num) / round_osmo_prec_up(product_denom)) + return round_osmo_prec_down(round_osmo_prec_down(round_osmo_prec_down(product_num) / sqrtPriceA)/ sqrtPriceB) def calc_amount_one_delta(liquidity, sqrtPriceA, sqrtPriceB, roundUp): if sqrtPriceB > sqrtPriceA: From 3cc21a8528a9c0363f163a8cbdfdfdabb05aa724 Mon Sep 17 00:00:00 2001 From: Roman Date: Mon, 11 Sep 2023 15:37:55 -0400 Subject: [PATCH 2/4] comment updates --- x/concentrated-liquidity/math/math.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x/concentrated-liquidity/math/math.go b/x/concentrated-liquidity/math/math.go index c3d3a0b4b2b..6429182bb41 100644 --- a/x/concentrated-liquidity/math/math.go +++ b/x/concentrated-liquidity/math/math.go @@ -134,10 +134,12 @@ func GetNextSqrtPriceFromAmount0InRoundingUp(sqrtPriceCurrent, liquidity, amount return sqrtPriceCurrent } + // Truncate at precision end to make denominator smaller so that the final result is larger. product := amountZeroRemainingIn.MulTruncate(sqrtPriceCurrent) // denominator = product + liquidity denominator := product denominator.AddMut(liquidity) + // MulRoundUp and QuoRoundUp to make the final result larger by rounding up at precision end. return liquidity.MulRoundUp(sqrtPriceCurrent).QuoRoundUp(denominator) } From 41e436ce8db2082ed359967ab2ddd46b9557e5ab Mon Sep 17 00:00:00 2001 From: Roman Date: Thu, 14 Sep 2023 07:03:24 -0400 Subject: [PATCH 3/4] clarifying comment --- x/concentrated-liquidity/math/math.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/concentrated-liquidity/math/math.go b/x/concentrated-liquidity/math/math.go index 6429182bb41..210f43d430d 100644 --- a/x/concentrated-liquidity/math/math.go +++ b/x/concentrated-liquidity/math/math.go @@ -78,7 +78,7 @@ func CalcAmount0Delta(liq, sqrtPriceA, sqrtPriceB osmomath.BigDec, roundUp bool) // - adding liquidity (request user to provide more tokens in in favor of the pool) // The denominator is truncated to get a higher final amount. // Note that the order of divisions is important here. First, we divide by a larger number (sqrtPriceB) and then by a smaller number (sqrtPriceA). - // This leads to a smaller error amplification. + // This leads to a smaller error amplification. This only matters in cases where at least one of the sqrt prices is below 1. // TODO (perf): QuoRoundUpMut with no reallocation. return liq.MulRoundUp(diff).QuoRoundUp(sqrtPriceB).QuoRoundUp(sqrtPriceA).Ceil() } From 1562a84fecc4498a3feeafd1eb1c6eed69626fb6 Mon Sep 17 00:00:00 2001 From: Roman Date: Thu, 14 Sep 2023 07:05:24 -0400 Subject: [PATCH 4/4] remove comment --- x/concentrated-liquidity/math/math_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/x/concentrated-liquidity/math/math_test.go b/x/concentrated-liquidity/math/math_test.go index 6877a9fc9a7..affd657841e 100644 --- a/x/concentrated-liquidity/math/math_test.go +++ b/x/concentrated-liquidity/math/math_test.go @@ -337,7 +337,6 @@ func TestCalcAmount1Delta(t *testing.T) { // from clmath decimal import * // calc_amount_one_delta(liq, sqrtPriceA, sqrtPriceB, False) // Actual result: 0.000000000000000000000000000103787163 - // Gets rounded up to 1. Is this acceptable when the multiplicative difference is so large? amount1Expected: osmomath.MustNewBigDecFromStr("0.000000000000000000000000000103787163").Ceil(), }, }