Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(CL): error blow up in CalcAmount0Delta causes OverChargeSwapOutGivenInError when swapping zero for one #6352

Merged
merged 9 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Bug Fixes

* [#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.

### API Breaks

Expand Down
11 changes: 9 additions & 2 deletions osmoutils/osmoassert/assertions.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,13 @@ func messageFromMsgAndArgs(msgAndArgs ...interface{}) string {
return ""
}

type Stringer interface {
String() string
}

// Equal compares A with B and asserts that they are equal within tolerance error tolerance
func Equal[T any](t *testing.T, tolerance osmomath.ErrTolerance, A, B T) {
errMsg := fmt.Sprintf("expected %T, actual %T", A, B)
func Equal[T Stringer](t *testing.T, tolerance osmomath.ErrTolerance, A, B T) {
errMsg := fmt.Sprintf("expected %s, actual %s", A.String(), B.String())
switch a := any(A).(type) {
case osmomath.Int:
b, ok := any(B).(osmomath.Int)
Expand All @@ -81,5 +85,8 @@ func Equal[T any](t *testing.T, tolerance osmomath.ErrTolerance, A, B T) {
failNowIfNot(t, ok)

require.True(t, tolerance.CompareDec(a, b) == 0, errMsg)
default:
require.FailNow(t, "unsupported types")
}

}
3 changes: 1 addition & 2 deletions x/concentrated-liquidity/math/math.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ 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.
denom := sqrtPriceA.MulTruncate(sqrtPriceB)
return liq.Mul(diff).QuoMut(denom).Ceil()
return liq.MulRoundUp(diff).QuoTruncate(sqrtPriceA).QuoTruncate(sqrtPriceB).Ceil()
p0mvn marked this conversation as resolved.
Show resolved Hide resolved
}
// These are truncated at precision end to round in favor of the pool when:
// - calculating amount out during swap
Expand Down
16 changes: 16 additions & 0 deletions x/concentrated-liquidity/math/math_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,22 @@ func TestCalcAmount0Delta(t *testing.T) {
amount0Expected: osmomath.MustNewBigDecFromStr("6098022989717817431593106314408.88812810159039320984467945943").Ceil(), // rounded up at precision end.
isWithTolerance: true,
},
// See: https://github.com/osmosis-labs/osmosis/issues/6351 for details.
// The values were taken from the failing swap on the development branch that reproduced the issue.
"edge case: low sqrt prices may cause error amplofication if incorrect order of operations": {
// from decimal import *
// from math import *
// getcontext().prec = 100
// max_sqrt_p = Decimal("0.00000099994999874993749609347654199")
// min_sqrt_p = Decimal("0.000000000000001409841835100661211756")
// liq = Decimal("5000252259822539816806336.971796256914465071095518135400579243")
// liq * (max_sqrt_p - min_sqrt_p) / (max_sqrt_p * min_sqrt_p)
liquidity: osmomath.MustNewBigDecFromStr("5000252259822539816806336.971796256914465071095518135400579243"),
sqrtPA: osmomath.MustNewBigDecFromStr("0.00000099994999874993749609347654199"),
sqrtPB: osmomath.MustNewBigDecFromStr("0.000000000000001409841835100661211756"),
roundUp: true,
amount0Expected: osmomath.MustNewBigDecFromStr("3546676037185128488234786333758360815266.999539026068480181194797910898392880").Ceil(),
},
}

for name, tc := range testCases {
Expand Down