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

feat: query max amountIn based on max ticks willing to traverse #5889

Merged
merged 8 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from 4 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 @@ -55,6 +55,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

* [#5534](https://github.com/osmosis-labs/osmosis/pull/5534) fix: fix the account number of x/tokenfactory module account
* [#5750](https://github.com/osmosis-labs/osmosis/pull/5750) feat: add cli commmand for converting proto structs to proto marshalled bytes
* [#5889](https://github.com/osmosis-labs/osmosis/pull/5889) provides an API for protorev to determine max amountIn that can be swapped based on max ticks willing to be traversed

### Minor improvements & Bug Fixes

Expand Down
2 changes: 1 addition & 1 deletion tests/cl-go-client/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ go 1.20
require (
github.com/cosmos/cosmos-sdk v0.47.3
github.com/ignite/cli v0.23.0
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230623115558-38aaab07d343
github.com/osmosis-labs/osmosis/v16 v16.0.0-20230630175215-d5fcd089a71c
github.com/osmosis-labs/osmosis/x/epochs v0.0.0-20230328024000-175ec88e4304

Expand Down Expand Up @@ -90,7 +91,6 @@ require (
github.com/mtibben/percent v0.2.1 // indirect
github.com/nxadm/tail v1.4.8 // indirect
github.com/osmosis-labs/osmosis/osmomath v0.0.3-dev.0.20230621002052-afb82fbaa312 // indirect
github.com/osmosis-labs/osmosis/osmoutils v0.0.0-20230623115558-38aaab07d343 // indirect
github.com/pelletier/go-toml/v2 v2.0.8 // indirect
github.com/petermattis/goid v0.0.0-20180202154549-b0b1615b78e5 // indirect
github.com/pkg/errors v0.9.1 // indirect
Expand Down
4 changes: 4 additions & 0 deletions x/concentrated-liquidity/client/query_proto_wrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,10 @@ func (q Querier) UserUnbondingPositions(ctx sdk.Context, req clquery.UserUnbondi
}

cfmmPoolId, err := q.Keeper.GetUserUnbondingPositions(ctx, sdkAddr)
if err != nil {
return nil, err
}
Comment on lines +275 to +277
Copy link
Member Author

Choose a reason for hiding this comment

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

drive by lint (unchecked error)


return &clquery.UserUnbondingPositionsResponse{
PositionsWithPeriodLock: cfmmPoolId,
}, nil
Expand Down
119 changes: 119 additions & 0 deletions x/concentrated-liquidity/swaps.go
Original file line number Diff line number Diff line change
Expand Up @@ -808,3 +808,122 @@ func edgeCaseInequalityBasedOnSwapStrategy(isZeroForOne bool, nextInitializedTic
}
return nextInitializedTickSqrtPrice.LT(computedSqrtPrice)
}

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, ComputeMaxInAmtGivenMaxTicksCrossed follows nearly the same logic as calcIn/Out, but tracks the ticks traversed, does not track the spread or uptime accums, and does not save changes to state.

// ComputeMaxInAmtGivenMaxTicksCrossed calculates the maximum amount of the tokenInDenom that can be swapped
// into the pool to swap through all the liquidity from the current tick through the maxTicksCrossed tick,
// but not exceed it.
func (k Keeper) ComputeMaxInAmtGivenMaxTicksCrossed(
ctx sdk.Context,
poolId uint64,
tokenInDenom string,
maxTicksCrossed uint64,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: since tick liquidity is subject to change, how is protorev going to bound the max tick a swap is going to cross? Is it something like X swap can cross upto 100 ticks given the gas to cross that 100 ticks is <50M

I originally thought this function was going to be given X amt estimate how many ticks it will cross, but if this works then it's all g.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stackman27 I previously thought we wanted "Given X amount, how many ticks will it cross?" but since Protorev needs to bound itself from consuming too much gas and compute time, and it seems to me liquidity doesn't really impact that, but number of ticks crossed does, so it cares more about the ticks crossed.

If a method was made that given X amount how many ticks will it cross, and we put in $1k to swap in and it says it'll cross 1000 ticks, that would be too much and then the next action would be "okay how do I reduce the number of ticks crossed to something acceptable", so it feels like just querying directly for the acceptable number of ticks crossed and always using that amountIn bound is better than continuously trying in different amounts to identify what amount passes an acceptable amount of ticks.

Copy link
Contributor

@stackman27 stackman27 Jul 27, 2023

Choose a reason for hiding this comment

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

i see one last question, so in the current implementation how are you going to decide the maxTickCrossed value? are you going to predetermine that 50M or 100M gas can cross X amt of ticks and generate routes for swap?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the answer to this is crossing ticks should cost a fixed amt of gas so yes, your description should be accurate (but admittedly I just made this based of Skip's design requirements and treated the actual use of the API as a black box)

Copy link
Contributor

Choose a reason for hiding this comment

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

@stackman27 @czarcas7ic Yes exactly. The plan is to make X number of ticks be a changeable variable (similar to pool points), and change that depending on how many CL pools we want to be able to use in backrunning.

That is, if many new CL pools launch and almost all swaps are on CL pools, then we'd want to decrease the max number of ticks crossed per pool so that we can check 2-4 routes per swap without running out of the upperGasLimit, if there are still more balancer swaps than CL, and our routes mainly consist of balancer pool, then we can increase X to cross more ticks per CL pool since there are less of them, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, would love to review the PR when its ready too

) (maxTokenIn, resultingTokenOut sdk.Coin, err error) {
cacheCtx, _ := ctx.CacheContext()

p, err := k.getPoolForSwap(cacheCtx, poolId)
if err != nil {
return sdk.Coin{}, sdk.Coin{}, err
}

// Validate tokenInDenom exists in the pool
if tokenInDenom != p.GetToken0() && tokenInDenom != p.GetToken1() {
return sdk.Coin{}, sdk.Coin{}, types.TokenInDenomNotInPoolError{TokenInDenom: tokenInDenom}
}

// Determine the tokenOutDenom based on the tokenInDenom
var tokenOutDenom string
if tokenInDenom == p.GetToken0() {
tokenOutDenom = p.GetToken1()
} else {
tokenOutDenom = p.GetToken0()
}

// Setup the swap strategy
swapStrategy, _, err := k.setupSwapStrategy(p, p.GetSpreadFactor(cacheCtx), tokenInDenom, sdk.ZeroDec())
if err != nil {
return sdk.Coin{}, sdk.Coin{}, err
}

// Initialize swap state
swapState := newSwapState(types.MaxSpotPrice.RoundInt(), p, swapStrategy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use types.MaxSpotPrice? Do we decrement it until maxTicksCrossed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah we basically never want the reason we stop an estimation to be due to a lack of funds, it should only ever strictly be due to the limit imposed by amt of ticks willing to traverse.

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case would TotalPoolLiquidity not be a good value to use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think that makes sense too, I can change it to that


nextInitTickIter := swapStrategy.InitializeNextTickIterator(cacheCtx, poolId, swapState.tick)
defer nextInitTickIter.Close()

totalTokenOut := sdk.ZeroDec()

for i := uint64(0); i < maxTicksCrossed; i++ {
// Check if the iterator is valid
if !nextInitTickIter.Valid() {
break
}

nextInitializedTick, err := types.TickIndexFromBytes(nextInitTickIter.Key())
if err != nil {
return sdk.Coin{}, sdk.Coin{}, err
}

_, nextInitializedTickSqrtPrice, err := math.TickToSqrtPrice(nextInitializedTick)
if err != nil {
return sdk.Coin{}, sdk.Coin{}, types.TickToSqrtPriceConversionError{NextTick: nextInitializedTick}
}

sqrtPriceTarget := swapStrategy.GetSqrtTargetPrice(nextInitializedTickSqrtPrice)

// Compute the swap
computedSqrtPrice, amountOut, amountIn, spreadRewardChargeTotal := swapStrategy.ComputeSwapWithinBucketInGivenOut(
swapState.sqrtPrice,
sqrtPriceTarget,
swapState.liquidity,
swapState.amountSpecifiedRemaining,
)

swapState.sqrtPrice = computedSqrtPrice
swapState.amountSpecifiedRemaining.SubMut(amountOut)
swapState.amountCalculated.AddMut(amountIn.Add(spreadRewardChargeTotal))

totalTokenOut = totalTokenOut.Add(amountOut)

// Check if the tick needs to be updated
nextInitializedTickSqrtPriceBigDec := osmomath.BigDecFromSDKDec(nextInitializedTickSqrtPrice)

// We do not need to track spread rewards or uptime accums here since we are not actually swapping.
if nextInitializedTickSqrtPriceBigDec.Equal(computedSqrtPrice) {
nextInitializedTickInfo, err := ParseTickFromBz(nextInitTickIter.Value())
if err != nil {
return sdk.Coin{}, sdk.Coin{}, err
}
liquidityNet := nextInitializedTickInfo.LiquidityNet

nextInitTickIter.Next()

liquidityNet = swapState.swapStrategy.SetLiquidityDeltaSign(liquidityNet)
swapState.liquidity.AddMut(liquidityNet)

swapState.tick = swapStrategy.UpdateTickAfterCrossing(nextInitializedTick)
} else if edgeCaseInequalityBasedOnSwapStrategy(swapStrategy.ZeroForOne(), nextInitializedTickSqrtPriceBigDec, computedSqrtPrice) {
return sdk.Coin{}, sdk.Coin{}, types.ComputedSqrtPriceInequalityError{
IsZeroForOne: swapStrategy.ZeroForOne(),
ComputedSqrtPrice: computedSqrtPrice,
NextInitializedTickSqrtPrice: nextInitializedTickSqrtPriceBigDec,
}
} else if !swapState.sqrtPrice.Equal(computedSqrtPrice) {
newTick, err := math.CalculateSqrtPriceToTick(computedSqrtPrice)
if err != nil {
return sdk.Coin{}, sdk.Coin{}, err
}
swapState.tick = newTick
}

// Break the loop early if nothing was consumed from swapState.amountSpecifiedRemaining
if amountOut.IsZero() {
break
}
}

maxAmt := swapState.amountCalculated.Ceil().TruncateInt()
maxTokenIn = sdk.NewCoin(tokenInDenom, maxAmt)
resultingTokenOut = sdk.NewCoin(tokenOutDenom, totalTokenOut.TruncateInt())

return maxTokenIn, resultingTokenOut, nil
}
108 changes: 108 additions & 0 deletions x/concentrated-liquidity/swaps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3411,3 +3411,111 @@ func (s *KeeperTestSuite) TestInfiniteSwapLoop_OutGivenIn() {
_, _, _, err = s.clk.SwapOutAmtGivenIn(s.Ctx, swapAddress, pool, tokenOut, ETH, pool.GetSpreadFactor(s.Ctx), sdk.ZeroDec())
s.Require().NoError(err)
}

func (s *KeeperTestSuite) TestComputeMaxInAmtGivenMaxTicksCrossed() {
tests := []struct {
name string
tokenInDenom string
tokenOutDenom string
maxTicksCrossed uint64
expectedError error
}{
{
name: "happy path, ETH in, max ticks equal to number of initialized ticks in swap direction",
tokenInDenom: ETH,
tokenOutDenom: USDC,
maxTicksCrossed: 3,
},
{
name: "happy path, USDC in, max ticks equal to number of initialized ticks in swap direction",
tokenInDenom: USDC,
tokenOutDenom: ETH,
maxTicksCrossed: 3,
},
{
name: "ETH in, max ticks less than number of initialized ticks in swap direction",
tokenInDenom: ETH,
tokenOutDenom: USDC,
maxTicksCrossed: 2,
},
{
name: "USDC in, max ticks less than number of initialized ticks in swap direction",
tokenInDenom: USDC,
tokenOutDenom: ETH,
maxTicksCrossed: 2,
},
{
name: "ETH in, max ticks greater than number of initialized ticks in swap direction",
tokenInDenom: ETH,
tokenOutDenom: USDC,
maxTicksCrossed: 4,
},
{
name: "USDC in, max ticks greater than number of initialized ticks in swap direction",
tokenInDenom: USDC,
tokenOutDenom: ETH,
maxTicksCrossed: 4,
},
{
name: "error: tokenInDenom not in pool",
tokenInDenom: "BTC",
tokenOutDenom: ETH,
maxTicksCrossed: 4,
expectedError: types.TokenInDenomNotInPoolError{TokenInDenom: "BTC"},
},
}

for _, test := range tests {
s.Run(test.name, func() {
s.SetupTest()
clPool := s.PrepareConcentratedPool()
expectedResultingTokenOutAmount := sdk.ZeroInt()

// Create positions and calculate expected resulting tokens
positions := []struct {
lowerTick, upperTick int64
maxTicks uint64
}{
{DefaultLowerTick, DefaultUpperTick, 0}, // Surrounding the current price
{DefaultLowerTick - 10000, DefaultLowerTick, 1}, // Below the position surrounding the current price
{DefaultLowerTick - 20000, DefaultLowerTick - 10000, 2}, // Below the position below the position surrounding the current price
{DefaultUpperTick, DefaultUpperTick + 10000, 1}, // Above the position surrounding the current price
{DefaultUpperTick + 10000, DefaultUpperTick + 20000, 2}, // Above the position above the position surrounding the current price
}

// Create positions and determine how much token out we should expect given the maxTicksCrossed provided.
for _, pos := range positions {
amt0, amt1 := s.createPositionAndFundAcc(clPool, pos.lowerTick, pos.upperTick)
expectedResultingTokenOutAmount = s.calculateExpectedTokens(test.tokenInDenom, test.maxTicksCrossed, pos.maxTicks, amt0, amt1, expectedResultingTokenOutAmount)
}

// System Under Test
_, resultingTokenOut, err := s.App.ConcentratedLiquidityKeeper.ComputeMaxInAmtGivenMaxTicksCrossed(s.Ctx, clPool.GetId(), test.tokenInDenom, test.maxTicksCrossed)

if test.expectedError != nil {
s.Require().Error(err)
s.Require().ErrorContains(err, test.expectedError.Error())
} else {
s.Require().NoError(err)

errTolerance := osmomath.ErrTolerance{AdditiveTolerance: sdk.NewDec(int64(test.maxTicksCrossed))}
s.Require().Equal(0, errTolerance.Compare(expectedResultingTokenOutAmount, resultingTokenOut.Amount), "expected: %s, got: %s", expectedResultingTokenOutAmount, resultingTokenOut.Amount)
}
})
}
}

func (s *KeeperTestSuite) createPositionAndFundAcc(clPool types.ConcentratedPoolExtension, lowerTick, upperTick int64) (amt0, amt1 sdk.Int) {
s.FundAcc(s.TestAccs[0], DefaultCoins)
_, amt0, amt1, _, _, _, _ = s.App.ConcentratedLiquidityKeeper.CreatePosition(s.Ctx, clPool.GetId(), s.TestAccs[0], DefaultCoins, sdk.ZeroInt(), sdk.ZeroInt(), lowerTick, upperTick)
return
}

func (s *KeeperTestSuite) calculateExpectedTokens(tokenInDenom string, testMaxTicks, positionMaxTicks uint64, amt0, amt1, currentTotal sdk.Int) sdk.Int {
if tokenInDenom == ETH && testMaxTicks > positionMaxTicks {
return currentTotal.Add(amt1)
} else if tokenInDenom == USDC && testMaxTicks > positionMaxTicks {
return currentTotal.Add(amt0)
}
return currentTotal
}