-
Notifications
You must be signed in to change notification settings - Fork 608
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
Changes from 4 commits
1d233f5
16ca275
b2f0a37
01fd5bd
50cfbec
f719b04
3287b44
e78d4a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -808,3 +808,122 @@ func edgeCaseInequalityBasedOnSwapStrategy(isZeroForOne bool, nextInitializedTic | |
} | ||
return nextInitializedTickSqrtPrice.LT(computedSqrtPrice) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to reviewer, |
||
// 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we use types.MaxSpotPrice? Do we decrement it until maxTicksCrossed ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in that case would TotalPoolLiquidity not be a good value to use? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
There was a problem hiding this comment.
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)