fix(ProtoRev): Updating Binary Search Range with CL Pools #5930
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| package keeper | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
|
||
| poolmanagertypes "github.com/osmosis-labs/osmosis/v17/x/poolmanager/types" | ||
|
|
@@ -30,13 +32,10 @@ func (k Keeper) IterateRoutes(ctx sdk.Context, routes []RouteMetaData, remaining | |
|
|
||
| // If the profit is greater than zero, then we convert the profits to uosmo and compare profits in terms of uosmo | ||
| if profit.GT(sdk.ZeroInt()) { | ||
| profit, err := k.ConvertProfits(ctx, inputCoin, profit) | ||
| if err != nil { | ||
| k.Logger(ctx).Error("Error converting profits: ", err) | ||
|
There was a problem hiding this comment. This error won't propagate properly because it accepts kv pairs, let's follow this format: osmosis/x/protorev/keeper/posthandler.go Line 61 in a706848 Realized I missed all places that use the format |
||
| continue | ||
| } | ||
|
|
||
| // Select the optimal route King of the Hill style (route with the highest profit will be executed) | ||
|
|
@@ -53,6 +52,10 @@ func (k Keeper) IterateRoutes(ctx sdk.Context, routes []RouteMetaData, remaining | |
|
|
||
| // ConvertProfits converts the profit denom to uosmo to allow for a fair comparison of profits | ||
| func (k Keeper) ConvertProfits(ctx sdk.Context, inputCoin sdk.Coin, profit sdk.Int) (sdk.Int, error) { | ||
| if inputCoin.Denom == types.OsmosisDenomination { | ||
| return profit, nil | ||
| } | ||
|
|
||
| // Get highest liquidity pool ID for the input coin and uosmo | ||
| conversionPoolID, err := k.GetPoolForDenomPair(ctx, types.OsmosisDenomination, inputCoin.Denom) | ||
| if err != nil { | ||
|
|
@@ -132,8 +135,11 @@ func (k Keeper) FindMaxProfitForRoute(ctx sdk.Context, route RouteMetaData, rema | |
| return sdk.Coin{}, sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| // Update the search range if the max input amount is too small/large | ||
| curLeft, curRight, err = k.UpdateSearchRangeIfNeeded(ctx, route, inputDenom, curLeft, curRight) | ||
| if err != nil { | ||
| return sdk.Coin{}, sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| // Binary search to find the max profit | ||
| for iteration := 0; curLeft.LT(curRight) && iteration < types.MaxIterations; iteration++ { | ||
|
|
@@ -167,30 +173,151 @@ func (k Keeper) FindMaxProfitForRoute(ctx sdk.Context, route RouteMetaData, rema | |
| return tokenIn, profit, nil | ||
| } | ||
|
|
||
| // UpdateSearchRangeIfNeeded updates the search range for the binary search. First, we check if there are any | ||
| // concentrated liquidity pools in the route. If there are, then we may need to reduce the upper bound of the | ||
| // binary search since it is gas intensive to move across several ticks. Next, we determine if the current bound | ||
| // includes the optimal amount in. If it does not, then we can extend the search range to capture more profits. | ||
| func (k Keeper) UpdateSearchRangeIfNeeded( | ||
| ctx sdk.Context, | ||
| route RouteMetaData, | ||
| inputDenom string, | ||
| curLeft, curRight sdk.Int, | ||
| ) (sdk.Int, sdk.Int, error) { | ||
| // Retrieve all concentrated liquidity pools in the route | ||
| clPools := make([]uint64, 0) | ||
| inputMap := make(map[uint64]string) | ||
| prevInput := inputDenom | ||
|
|
||
| for _, route := range route.Route { | ||
| pool, err := k.poolmanagerKeeper.GetPool(ctx, route.PoolId) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| if pool.GetType() == poolmanagertypes.Concentrated { | ||
| clPools = append(clPools, route.PoolId) | ||
| inputMap[route.PoolId] = prevInput | ||
| } | ||
|
|
||
| prevInput = route.TokenOutDenom | ||
| } | ||
|
|
||
| // If there are concentrated liquidity pools in the route, then we may need to reduce the upper bound of the binary search. | ||
| updatedMax, err := k.ReduceSearchRangeIfNeeded(ctx, clPools, inputDenom, inputMap, curLeft, curRight) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| if updatedMax.LT(curRight) { | ||
| return curLeft, updatedMax, nil | ||
| } | ||
|
|
||
| return k.ExtendSearchRangeIfNeeded(ctx, route, inputDenom, curLeft, curRight, updatedMax) | ||
| } | ||
|
|
||
| // ReduceSearchRangeIfNeeded returns the max amount in that can be used for the binary search | ||
| // respecting the max ticks moved across all concentrated liquidity pools in the route. | ||
| func (k Keeper) ReduceSearchRangeIfNeeded( | ||
| ctx sdk.Context, | ||
| clPools []uint64, | ||
| inputDenom string, | ||
| inputMap map[uint64]string, | ||
| curLeft, curRight sdk.Int, | ||
| ) (sdk.Int, error) { | ||
| // Find the max amount in that can be used for the binary search (in terms of uosmo) | ||
| maxInputAmount, stepSize, err := k.MaxInputAmount(ctx) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| // Iterate through all CL pools and determine the maximal amount of input that can be used | ||
| // respecting the max ticks moved. | ||
| for _, poolId := range clPools { | ||
| maxInForPool, _, err := k.concentratedLiquidityKeeper.ComputeMaxInAmtGivenMaxTicksCrossed( | ||
| ctx, | ||
| poolId, | ||
| inputMap[poolId], | ||
| types.MaxTicksMoved, | ||
| ) | ||
|
|
||
| // In the case where we cannot calculate the max in amount, we short circuit the search | ||
| // and return an upper bound of 0. This will cause the binary search to not run. | ||
| if err != nil { | ||
| return sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| maxInForPoolInOsmo, err := k.ConvertProfits(ctx, maxInForPool, maxInForPool.Amount) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| if maxInForPoolInOsmo.LT(maxInputAmount) { | ||
| maxInputAmount = maxInForPoolInOsmo | ||
| } | ||
| } | ||
|
|
||
| return maxInputAmount.Quo(stepSize), nil | ||
| } | ||
|
|
||
| // Determine if the binary search range needs to be extended | ||
| func (k Keeper) ExtendSearchRangeIfNeeded( | ||
| ctx sdk.Context, | ||
| route RouteMetaData, | ||
| inputDenom string, | ||
| curLeft, curRight, updatedMax sdk.Int, | ||
| ) (sdk.Int, sdk.Int, error) { | ||
| // Get the profit for the maximum amount in | ||
| _, maxInProfit, err := k.EstimateMultihopProfit(ctx, inputDenom, curRight.Mul(route.StepSize), route.Route) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| // If the profit for the maximum amount in is still increasing, then we can increase the range of the binary search | ||
| if maxInProfit.GTE(sdk.ZeroInt()) { | ||
| maxInPlusOne := curRight.Add(sdk.OneInt()).Mul(route.StepSize) | ||
| if updatedMax.Mul(route.StepSize).LT(maxInPlusOne) { | ||
| return curLeft, curRight, nil | ||
| } | ||
|
|
||
| // Get the profit for the maximum amount in + 1 | ||
| _, maxInProfitPlusOne, err := k.EstimateMultihopProfit(ctx, inputDenom, maxInPlusOne, route.Route) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| // Change the range of the binary search if the profit is still increasing | ||
| if maxInProfitPlusOne.GT(maxInProfit) { | ||
| curLeft = curRight | ||
| curRight = updatedMax | ||
| } | ||
| } | ||
|
|
||
| return curLeft, curRight, nil | ||
| } | ||
|
|
||
| // MaxInputAmount returns the max input amount that can be used in any route. We use uosmo as the base | ||
| // unit of account for the max input amount. | ||
| func (k Keeper) MaxInputAmount(ctx sdk.Context) (sdk.Int, sdk.Int, error) { | ||
|
There was a problem hiding this comment. Re: our in-person sync, if we go the reverse iteration method I don't think we need to choose any base unit of account for this so should be able to remove |
||
| baseDenom, err := k.GetAllBaseDenoms(ctx) | ||
| if err != nil { | ||
| return sdk.ZeroInt(), sdk.ZeroInt(), err | ||
| } | ||
|
|
||
| uosmoStepSize := sdk.ZeroInt() | ||
| foundUosmo := false | ||
| for _, denom := range baseDenom { | ||
| if denom.Denom == types.OsmosisDenomination { | ||
| uosmoStepSize = denom.StepSize | ||
| foundUosmo = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !foundUosmo { | ||
| return sdk.ZeroInt(), sdk.ZeroInt(), fmt.Errorf("uosmo not found in base denoms") | ||
| } | ||
|
|
||
| return types.ExtendedMaxInputAmount.Mul(uosmoStepSize), uosmoStepSize, nil | ||
| } | ||
|
|
||
| // ExecuteTrade inputs a route, amount in, and rebalances the pool | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think we may also need to add bounding in ConvertProfits. If a CL pool is the highest liquidity pool then we will swap the entire amount without any tick bounds.
If a CL pool is the highest liquidity than I imagine this won't run into any issues, but maybe good to note in as a comment if we don't want to add the tick checking here that we are not doing so.
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.
adding a comment
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.
Am I correct in saying we went for the former and didn't just add a comment right?
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.
wait lol adding comment rn, misread the comment
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.
added the comment
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.
Per convo offline, we decided to not make the check. More likely than not we will not run into the issue of over-stepping
MaxTicksCrossedinConvertProfits. The upper bound check should be sufficient enough to cover most cases.