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): tick iterator bugs and tests #5526

Merged
merged 37 commits into from
Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
9abafa9
fix: zero for one tick iterator bug
p0mvn Jun 14, 2023
7913e5c
clean up tests
p0mvn Jun 14, 2023
c9567b4
test description
p0mvn Jun 14, 2023
e22f040
refactor and clean up
p0mvn Jun 14, 2023
834dd2a
fix position range concerns by subtracting next tick in zfo direction
p0mvn Jun 15, 2023
4b3e80d
updates
p0mvn Jun 15, 2023
0633c3a
updates
p0mvn Jun 15, 2023
aea44c6
tick crossing hack
p0mvn Jun 15, 2023
ef9549e
updates
p0mvn Jun 16, 2023
371753d
progress
p0mvn Jun 16, 2023
a5cedf7
clean up
p0mvn Jun 16, 2023
525a1e1
remove old tests
p0mvn Jun 16, 2023
8f3ed3e
fix
p0mvn Jun 16, 2023
54533ca
Merge branch 'main' into roman/tick-iterator-bug
p0mvn Jun 17, 2023
372968b
updates
p0mvn Jun 17, 2023
92e1ad9
updates
p0mvn Jun 17, 2023
2cb548b
Merge branch 'main' into roman/tick-iterator-bug
p0mvn Jun 17, 2023
c29225e
add docs and comments
p0mvn Jun 17, 2023
4aa41f8
refactor tests to map
p0mvn Jun 17, 2023
7009f3e
clean ups
p0mvn Jun 17, 2023
b59fdc0
updates
p0mvn Jun 17, 2023
48a2ef9
updates
p0mvn Jun 17, 2023
57a9166
updates
p0mvn Jun 17, 2023
b638e7d
update test docs
p0mvn Jun 17, 2023
fb20f0c
comments
p0mvn Jun 17, 2023
22080a4
updates
p0mvn Jun 17, 2023
83cde3a
updates
p0mvn Jun 17, 2023
859f199
typo
p0mvn Jun 17, 2023
50fb3de
fix zfo tests
p0mvn Jun 17, 2023
7e7f26b
clean up
p0mvn Jun 17, 2023
a433b1c
assert
p0mvn Jun 19, 2023
04fc8f0
dev's feedback
p0mvn Jun 19, 2023
dc40325
Merge branch 'main' into roman/tick-iterator-bug
p0mvn Jun 19, 2023
b49576e
conflict
p0mvn Jun 19, 2023
75483f6
updates
p0mvn Jun 19, 2023
bb881e1
swap test suite tick changes
p0mvn Jun 19, 2023
398380e
clean up
p0mvn Jun 19, 2023
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
7 changes: 7 additions & 0 deletions x/concentrated-liquidity/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"github.com/osmosis-labs/osmosis/osmoutils/accum"
"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/model"
"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/swapstrategy"
"github.com/osmosis-labs/osmosis/v16/x/concentrated-liquidity/types"
poolmanagertypes "github.com/osmosis-labs/osmosis/v16/x/poolmanager/types"
)
Expand Down Expand Up @@ -345,6 +346,12 @@ func (k Keeper) GetLargestSupportedUptimeDuration(ctx sdk.Context) time.Duration
return k.getLargestSupportedUptimeDuration(ctx)
}

func (k Keeper) SetupSwapStrategy(ctx sdk.Context, p types.ConcentratedPoolExtension,
spreadFactor sdk.Dec, tokenInDenom string,
priceLimit sdk.Dec) (strategy swapstrategy.SwapStrategy, sqrtPriceLimit sdk.Dec, err error) {
return k.setupSwapStrategy(ctx, p, spreadFactor, tokenInDenom, priceLimit)
}

func MoveRewardsToNewPositionAndDeleteOldAcc(ctx sdk.Context, accum accum.AccumulatorObject, oldPositionName, newPositionName string, growthOutside sdk.DecCoins) error {
return moveRewardsToNewPositionAndDeleteOldAcc(ctx, accum, oldPositionName, newPositionName, growthOutside)
}
24 changes: 18 additions & 6 deletions x/concentrated-liquidity/swaps.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func newSwapState(specifiedAmount sdk.Int, p types.ConcentratedPoolExtension, st
amountSpecifiedRemaining: specifiedAmount.ToDec(),
amountCalculated: sdk.ZeroDec(),
sqrtPrice: p.GetCurrentSqrtPrice(),
tick: strategy.InitializeTickValue(p.GetCurrentTick()),
tick: p.GetCurrentTick(),
liquidity: p.GetLiquidity(),
globalSpreadRewardGrowthPerUnitLiquidity: sdk.ZeroDec(),
globalSpreadRewardGrowth: sdk.ZeroDec(),
Expand Down Expand Up @@ -362,13 +362,27 @@ func (k Keeper) computeOutAmtGivenIn(
if err != nil {
return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, err
}
// Update the swapState's tick with the tick we retrieved liquidity from
swapState.tick = swapStrategy.UpdateTickAfterCrossing(nextTick)
Comment on lines +365 to +366
Copy link
Member Author

@p0mvn p0mvn Jun 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: this is the key realization and fix to the original bug as opposed to tick initialization as originally thought

Once the general change is accepted, I will improve the abstraction by moving it inside the swapCrossTickLogic method. Currently, I cannot put there as swapStrategy is not given as an argument to swapCrossTickLogic

I put it here to avoid large code diff and be able to point at the core fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense per discussions!

} else if !sqrtPriceStart.Equal(computedSqrtPrice) {
// Otherwise if the sqrtPrice calculated from ComputeSwapWithinBucketOutGivenIn(...) does not equal the sqrtPriceStart we started with at the
// beginning of this iteration, we set the swapState tick to the corresponding tick of the computedSqrtPrice calculated from ComputeSwapWithinBucketOutGivenIn(...)
price := computedSqrtPrice.Mul(computedSqrtPrice)
swapState.tick, err = math.PriceToTickRoundDown(price, p.GetTickSpacing())
if err != nil {
return sdk.Coin{}, sdk.Coin{}, 0, sdk.Dec{}, sdk.Dec{}, sdk.Dec{}, err
newTick := math.CalculatePriceToTick(price)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: see "CHANGE 3" section in the PR description for details about this change


// TEMPORARY HACK: this is to fix tick rounding error where
// the tick is off by 1 due to banker's rounding error in CalculatePriceToTick
// TODO: if this is to remain in the codebase, consider abstracting this into a
// method of swap strategy.
isZeroForOne := getZeroForOne(tokenInMin.Denom, p.GetToken0())
if isZeroForOne {
if newTick <= swapState.tick {
swapState.tick = newTick
}
} else {
if newTick > swapState.tick {
swapState.tick = newTick
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any cases where none of these branches would be hit?

Copy link
Member Author

@p0mvn p0mvn Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

			if isZeroForOne {
				if newTick <= swapState.tick {
					swapState.tick = newTick
				} else {
					fmt.Println("11111")
				}
			} else {
				if newTick > swapState.tick {
					swapState.tick = newTick
				} else {
					fmt.Println("22222")
				}
			}

Prior to merging #5541 all branches were hit. Now, only "11111" is not being hit.

I'm going to make an issue to investigate this separately. Merging for now

}
}
Copy link
Member Author

@p0mvn p0mvn Jun 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: see "BUG 2" section in the PR description for details

I expect this to be temporary until the following PRs are in:

Would like to intentionally leave this in a visible place and poorly abstracted so that we track this and converge on the right solution

}
}
Expand Down Expand Up @@ -554,8 +568,6 @@ func (k Keeper) swapCrossTickLogic(ctx sdk.Context,
newLiquidity := swapState.liquidity.AddMut(liquidityNet)
swapState.liquidity = newLiquidity

// Update the swapState's tick with the tick we retrieved liquidity from
swapState.tick = nextInitializedTick
return swapState, nil
}

Expand Down
35 changes: 22 additions & 13 deletions x/concentrated-liquidity/swaps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ type SwapTest struct {
expectErr bool
}

// positionMeta defines the metadata of a position
// after its creation.
type positionMeta struct {
positionId uint64
lowerTick int64
upperTick int64
liquidity sdk.Dec
}

var (
swapFundCoins = sdk.NewCoins(sdk.NewCoin("eth", sdk.NewInt(10000000000000)), sdk.NewCoin("usdc", sdk.NewInt(1000000000000)))

Expand Down Expand Up @@ -84,7 +93,7 @@ var (
// expectedTokenOut: 8396.71424216 rounded down https://www.wolframalpha.com/input?i=%281517882343.751510418088349649+*+%2870.738348247484497717+-+70.710678118654752440+%29%29+%2F+%2870.710678118654752440+*+70.738348247484497717%29
expectedTokenIn: sdk.NewCoin("usdc", sdk.NewInt(42000000)),
expectedTokenOut: sdk.NewCoin("eth", sdk.NewInt(8396)),
expectedTick: 31003900,
expectedTick: 31003914,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I only validated that the tick is in the desired bucket. I did not manually calculate and verify the translation between "current tick" and "current sqrt price", as I don't think the benefits of doing that outweigh the time costs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This SGTM

expectedSqrtPrice: sdk.MustNewDecFromStr("70.738348247484497717"), // https://www.wolframalpha.com/input?i=70.710678118654752440+%2B+42000000+%2F+1517882343.751510418088349649
// tick's accum coins stay same since crossing tick does not occur in this case
expectedLowerTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
Expand All @@ -103,7 +112,7 @@ var (
// expectedTokenOut: 8396.71424216 rounded down https://www.wolframalpha.com/input?i=%281517882343.751510418088349649+*+%2870.738348247484497717+-+70.710678118654752440+%29%29+%2F+%2870.710678118654752440+*+70.738348247484497717%29
expectedTokenIn: sdk.NewCoin("usdc", sdk.NewInt(42000000)),
expectedTokenOut: sdk.NewCoin("eth", sdk.NewInt(8396)),
expectedTick: 31003900,
expectedTick: 31003914,
expectedSqrtPrice: sdk.MustNewDecFromStr("70.738348247484497717"), // https://www.wolframalpha.com/input?i=70.710678118654752440+%2B+42000000+%2F+1517882343.751510418088349649
// tick's accum coins stay same since crossing tick does not occur in this case
expectedLowerTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
Expand All @@ -122,7 +131,7 @@ var (
// expectedTokenOut: 66808388.8901 rounded down https://www.wolframalpha.com/input?i=1517882343.751510418088349649+*+%2870.710678118654752440+-+70.6666639108571443311%29
expectedTokenIn: sdk.NewCoin("eth", sdk.NewInt(13370)),
expectedTokenOut: sdk.NewCoin("usdc", sdk.NewInt(66808388)),
expectedTick: 30993700,
expectedTick: 30993777,
expectedSqrtPrice: sdk.MustNewDecFromStr("70.666663910857144332"), // https://www.wolframalpha.com/input?i=%28%281517882343.751510418088349649%29%29+%2F+%28%28%281517882343.751510418088349649%29+%2F+%2870.710678118654752440%29%29+%2B+%2813370%29%29
expectedLowerTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
expectedUpperTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
Expand All @@ -140,7 +149,7 @@ var (
// expectedTokenOut: 66808388.8901 rounded down https://www.wolframalpha.com/input?i=1517882343.751510418088349649+*+%2870.710678118654752440+-+70.6666639108571443311%29
expectedTokenIn: sdk.NewCoin("eth", sdk.NewInt(13370)),
expectedTokenOut: sdk.NewCoin("usdc", sdk.NewInt(66808388)),
expectedTick: 30993700,
expectedTick: 30993777,
expectedSqrtPrice: sdk.MustNewDecFromStr("70.666663910857144332"), // https://www.wolframalpha.com/input?i=%28%281517882343.751510418088349649%29%29+%2F+%28%28%281517882343.751510418088349649%29+%2F+%2870.710678118654752440%29%29+%2B+%2813370%29%29
expectedLowerTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
expectedUpperTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
Expand All @@ -165,7 +174,7 @@ var (
// expectedTokenOut: 8398.3567 rounded down https://www.wolframalpha.com/input?i=%283035764687.503020836176699298+*+%2870.724513183069625078+-+70.710678118654752440+%29%29+%2F+%2870.710678118654752440+*+70.724513183069625078%29
expectedTokenIn: sdk.NewCoin("usdc", sdk.NewInt(42000000)),
expectedTokenOut: sdk.NewCoin("eth", sdk.NewInt(8398)),
expectedTick: 31001900,
expectedTick: 31001957,
expectedSqrtPrice: sdk.MustNewDecFromStr("70.724513183069625078"), // https://www.wolframalpha.com/input?i=70.710678118654752440+%2B++++%2842000000++%2F+3035764687.503020836176699298%29
// two positions with same liquidity entered
poolLiqAmount0: sdk.NewInt(1000000).MulRaw(2),
Expand All @@ -188,7 +197,7 @@ var (
// expectedTokenOut: 66829187.9678 rounded down https://www.wolframalpha.com/input?i=3035764687.503020836176699298+*+%2870.710678118654752440+-+70.688664163408836319%29
expectedTokenIn: sdk.NewCoin("eth", sdk.NewInt(13370)),
expectedTokenOut: sdk.NewCoin("usdc", sdk.NewInt(66829187)),
expectedTick: 30996800,
expectedTick: 30996887,
expectedSqrtPrice: sdk.MustNewDecFromStr("70.688664163408836320"), // https://www.wolframalpha.com/input?i=%28%283035764687.503020836176699298%29%29+%2F+%28%28%283035764687.503020836176699298%29+%2F+%2870.710678118654752440%29%29+%2B+%2813370.0000%29%29
// two positions with same liquidity entered
poolLiqAmount0: sdk.NewInt(1000000).MulRaw(2),
Expand Down Expand Up @@ -225,7 +234,7 @@ var (
// expectedTokenOut: 998976.6183474263883566299269 + 821653.4522259 = 1820630.070 round down = 1.820630 eth
expectedTokenIn: sdk.NewCoin("usdc", sdk.NewInt(10000000000)),
expectedTokenOut: sdk.NewCoin("eth", sdk.NewInt(1820630)),
expectedTick: 32105400,
expectedTick: 32105414,
expectedSqrtPrice: sdk.MustNewDecFromStr("78.137149196095607129"), // https://www.wolframalpha.com/input?i=74.16198487095662948711397441+%2B+4761322417+%2F+1197767444.955508123222985080
expectedLowerTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
expectedUpperTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
Expand Down Expand Up @@ -258,7 +267,7 @@ var (
expectedTokenIn: sdk.NewCoin("eth", sdk.NewInt(2000000)),
expectedTokenOut: sdk.NewCoin("usdc", sdk.NewInt(9103422788)),
// crosses one tick with spread reward growth outside
expectedTick: 30095100,
expectedTick: 30095167,
expectedSqrtPrice: sdk.MustNewDecFromStr("63.993489023323078693"), // https://www.wolframalpha.com/input?i=%28%281198735489.597250295669959397%29%29+%2F+%28%28%281198735489.597250295669959397%29+%2F+%28+67.41661516273269559379442134%29%29+%2B+%28951138.000000000000000000%29%29
// crossing tick happens single time for each upper tick and lower tick.
// Thus the tick's spread reward growth is DefaultSpreadRewardAccumCoins * 3 - DefaultSpreadRewardAccumCoins
Expand Down Expand Up @@ -298,7 +307,7 @@ var (
// expectedTokenOut: 998976.6183474263883566299269692777 + 865185.2591363751404579873403641 = 1864161.877 round down = 1.864161 eth
expectedTokenIn: sdk.NewCoin("usdc", sdk.NewInt(10000000000)),
expectedTokenOut: sdk.NewCoin("eth", sdk.NewInt(1864161)),
expectedTick: 32055900,
expectedTick: 32055920,
expectedSqrtPrice: sdk.MustNewDecFromStr("77.819789636800169393"), // https://www.wolframalpha.com/input?i=74.16198487095662948711397441+%2B++++%282452251164.000000000000000000+%2F+670416088.605668727039240782%29
expectedLowerTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
expectedUpperTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
Expand Down Expand Up @@ -333,7 +342,7 @@ var (
expectedUpperTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
expectedSecondLowerTickSpreadRewardGrowth: secondPosition{tickIndex: 310010, expectedSpreadRewardGrowth: cl.EmptyCoins},
expectedSecondUpperTickSpreadRewardGrowth: secondPosition{tickIndex: 322500, expectedSpreadRewardGrowth: cl.EmptyCoins},
expectedTick: 31712600,
expectedTick: 31712695,
expectedSqrtPrice: sdk.MustNewDecFromStr("75.582373164412551491"), // https://www.wolframalpha.com/input?i=74.16198487095662948711397441++%2B+%28+952251164.000000000000000000++%2F+670416088.605668727039240782%29
newLowerPrice: sdk.NewDec(5001),
newUpperPrice: sdk.NewDec(6250),
Expand All @@ -360,7 +369,7 @@ var (
secondPositionUpperPrice: sdk.NewDec(4999),
expectedTokenIn: sdk.NewCoin("eth", sdk.NewInt(2000000)),
expectedTokenOut: sdk.NewCoin("usdc", sdk.NewInt(9321276930)),
expectedTick: 30129000,
expectedTick: 30129083,
expectedSqrtPrice: sdk.MustNewDecFromStr("64.257943794993248955"), // https://www.wolframalpha.com/input?i=%28%28670416215.71882744366040059300%29%29+%2F+%28%28%28670416215.71882744366040059300%29+%2F+%2867.41661516273269559379442134%29%29+%2B+%28488827.000000000000000000%29%29
// Started from DefaultSpreadRewardAccumCoins * 3, crossed tick once, thus becoming
// DefaultSpreadRewardAccumCoins * 3 - DefaultSpreadRewardAccumCoins = DefaultSpreadRewardAccumCoins * 2
Expand Down Expand Up @@ -390,7 +399,7 @@ var (
secondPositionUpperPrice: sdk.NewDec(4999),
expectedTokenIn: sdk.NewCoin("eth", sdk.NewInt(1800000)),
expectedTokenOut: sdk.NewCoin("usdc", sdk.NewInt(8479320318)),
expectedTick: 30292000,
expectedTick: 30292060,
expectedSqrtPrice: sdk.MustNewDecFromStr("65.513815285481060960"), // https://www.wolframalpha.com/input?i=%28%28670416215.718827443660400593000%29%29+%2F+%28%28%28670416215.718827443660400593000%29+%2F+%2867.41661516273269559379442134%29%29+%2B+%28288827.000000000000000000%29%29
// Started from DefaultSpreadRewardAccumCoins * 3, crossed tick once, thus becoming
// DefaultSpreadRewardAccumCoins * 3 - DefaultSpreadRewardAccumCoins = DefaultSpreadRewardAccumCoins * 2
Expand Down Expand Up @@ -429,7 +438,7 @@ var (
// expectedTokenOut: 998976.61834742638835 + 821569.240826953837970 = 1820545.85917438022632 round down = 1.820545 eth
expectedTokenIn: sdk.NewCoin("usdc", sdk.NewInt(10000000000)),
expectedTokenOut: sdk.NewCoin("eth", sdk.NewInt(1820545)),
expectedTick: 32105500,
expectedTick: 32105556,
expectedSqrtPrice: sdk.MustNewDecFromStr("78.138055169663761658"), // https://www.wolframalpha.com/input?i=74.16872656315463530313879691++%2B+%28+4761322417.000000000000000000++%2F+1199528406.187413669220037261%29
expectedLowerTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
expectedUpperTickSpreadRewardGrowth: DefaultSpreadRewardAccumCoins,
Expand Down
Loading