fix(ProtoRev): Updating Binary Search Range with CL Pools #5930

davidterpay · 2023-07-31T18:20:11Z

What is the purpose of the change

This PR introduces a fix to protorev to update the binary search range in the case where it is too gas intensive to search over the hard-coded range. This is particularly useful for Concentrated Liquidity pools where moving across several ticks is very expensive. By determining if a route has an CL pools in it, we can bound the upper input amount by figuring out the maximum input amount respecting a maximum number of ticks. We do so across all CL pools and use this as our maximum input amount on the entire route.

Note, MaxTicksMoved is currently hard coded, but we plan to update that in a follow up PR. This will be configurable by the DeveloperAccount.

Testing and Verifying

This change added tests and can be verified as follows:

Added unit tests for TestFindMaxProfitRoute that test liquid and illiquid CL pools
- One of the test cases reduces the search range respecting the number of ticks that can be moved.

Documentation and Release Note

This PR is related to these open issues (#5858) and (#5884)

NotJeremyLiu · 2023-07-31T18:52:37Z

CHANGELOG.md

@@ -53,6 +53,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#5831](https://github.com/osmosis-labs/osmosis/pull/5831) Fix superfluid_delegations query
 * [#5835](https://github.com/osmosis-labs/osmosis/pull/5835) Fix println's for "amountZeroInRemainingBigDec before fee" making it into production
 * [#5841] (https://github.com/osmosis-labs/osmosis/pull/5841) Fix protorev's out of gas erroring of the user's transcation.
+* [#5930] (https://github.com/osmosis-labs/osmosis/pull/5930) Updating Binary Search Range with CL Pools


Suggested change

* [#5930] (https://github.com/osmosis-labs/osmosis/pull/5930) Updating Binary Search Range with CL Pools

* [#5930] (https://github.com/osmosis-labs/osmosis/pull/5930) Updating Protorev Binary Search Range Logic with CL Pools

NotJeremyLiu · 2023-07-31T18:57:42Z

x/protorev/keeper/rebalance.go

-					continue
-				}
-				profit = uosmoProfit
+			profit, err := k.ConvertProfits(ctx, inputCoin, profit)


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.

adding a comment

Am I correct in saying we went for the former and didn't just add a comment right?

wait lol adding comment rn, misread the comment

added the comment

Per convo offline, we decided to not make the check. More likely than not we will not run into the issue of over-stepping MaxTicksCrossed in ConvertProfits. The upper bound check should be sufficient enough to cover most cases.

NotJeremyLiu · 2023-07-31T19:02:10Z

x/protorev/keeper/rebalance.go

-				profit = uosmoProfit
+			profit, err := k.ConvertProfits(ctx, inputCoin, profit)
+			if err != nil {
+				k.Logger(ctx).Error("Error converting profits: ", err)


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

ctx.Logger().Error("ProtoRevTrade failed with error: " + err.Error())

Realized I missed all places that use the format k.Logger(ctx).Error, I replaced all the ctx.Logger().Error instances, can we add this fix in this PR too for all k.Logger(ctx).Error

NotJeremyLiu · 2023-07-31T19:20:18Z

x/protorev/keeper/rebalance.go

 	}

 	// 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.LT(maxInPlusOne) {


I think updatedMax is not multiplied by stepsize since the check previously in UpdateSearchRangeIfNeeded is

if updatedMax.LT(curRight) { return curLeft, updatedMax, nil }

so this check smells wrong to me

NotJeremyLiu · 2023-07-31T19:23:42Z

x/protorev/keeper/rebalance.go

+
+// 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) {


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

NotJeremyLiu

left some comments and synced in person about using a reverse iteration approach to reduce the bounds

NotJeremyLiu · 2023-08-01T18:41:34Z

x/protorev/keeper/keeper_test.go

+	s.CreateFullRangePosition(clPool, fundCoins)
+
+	// Create a concentrated liquidity pool for range testing
+	// Pool 52


Suggested change

// Pool 52

// Pool 53

davidterpay · 2023-08-01T18:45:28Z

Will update the doomsday gas test case in a follow up PR (where you can configure max ticks moved).

NotJeremyLiu · 2023-08-01T18:45:41Z

x/protorev/keeper/posthandler_test.go

@@ -340,12 +340,12 @@ func (s *KeeperTestSuite) TestAnteHandle() {
 			params: param{
 				trades: []types.Trade{
 					{
-						Pool:     51,
+						Pool:     54,


Synced in person: So after the tick change, this test no longer tests what we want it to since this does not run out of gas. So in the follow up PR that allows maxTicks to be configurable, we'll make the maxTicks really large for this test so that the rebalance logic runs out of gas again

NotJeremyLiu · 2023-08-01T18:47:40Z

x/protorev/types/constants.go

@@ -27,6 +27,10 @@ const MaxPoolPointsPerTx uint64 = 50
 // to the maximum execution time (in ms) of protorev per block
 const MaxPoolPointsPerBlock uint64 = 200

+// Max number of ticks we can move in a concentrated pool swap. This will be parameterized in a
+// follow up PR.
+const MaxTicksMoved uint64 = 5


Let's call this MaxTicksCrossed to stay consistent with the CL method naming of ComputeMaxInAmtGivenMaxTicksCrossed

czarcas7ic

Nice work on this, was super clear/easy to review despite lacking context.

czarcas7ic · 2023-08-01T23:41:57Z

x/protorev/keeper/rebalance.go

-					continue
-				}
-				profit = uosmoProfit
+			profit, err := k.ConvertProfits(ctx, inputCoin, profit)


Am I correct in saying we went for the former and didn't just add a comment right?

czarcas7ic · 2023-08-01T23:54:36Z

x/protorev/keeper/rebalance.go

+	}
+
+	// Ensure that the normalized upper bound is not greater than the extended max input amount
+	upperBound := intermidiateCoin.Amount.Quo(route.StepSize)


Is it possible for a route's step size to be zero?

tldr no, we manually set these and enforce that the step size cannot be zero (when we run validate basic on a msg).

czarcas7ic · 2023-08-01T23:55:56Z

x/protorev/keeper/rebalance.go

+	// At most we can swap half of the liquidity in the pool
+	liquidTokenAmt := liquidity.AmountOf(outputCoin.Denom).Quo(sdk.NewInt(2))


Is this just a self imposed limit?

Yea, this would allow us to still capture some arb on illiquid pools while ensuring we never error out when we make the calculation on upper bound. Additionally, on larger/more liquid pools, the trade size will never be larger than ExtendedMaxInputAmount which most certainly will be less than half the liquidity in a liquid pool.

I will admit that this is a bit of a hacky work around atm.

NotJeremyLiu

lgtm

stackman27

this lgtm, can we please update the README.md with this changes

AlpinYukseloglu · 2023-08-03T20:52:42Z

x/protorev/keeper/rebalance.go

+		return sdk.NewCoin(tokenInDenom, sdk.ZeroInt()), err
+	}
+
+	// At most we can swap half of the liquidity in the pool


This is not merge-blocking, but due to the way we handle rounding (always in the pool's favor and can be meaningfully high in some scenarios), this assumption does not necessarily true.

Rounding error can be nontrivial here and accumulates over time, so while this is likely the simplest way to handle this specific case, it should be kept in mind for future logic leaning on assumptions related to absolute assets in the pool.

Yea, totally agree. We've been going back and forth on how to best estimate maximal swap in when CL/stable/balancer pools are involved and we more or less ran into the same issue with all of our approaches. I'll decrease this to 1/4 for now and will revisit this once we have a better idea of how frequently this is an actual issue.

Also related, we plan on doing some structural refactoring - decoupling the various components of protorev - sometime soon which will let us navigate this with more granularity.

@AlpinYukseloglu Based on your understanding, does the reduction to 1/4 improve the situation or are you thinking of a different approach here?

davidterpay · 2023-08-04T00:10:49Z

@stackman27 anything specific you want updated in the README.md?

stackman27 · 2023-08-04T02:37:28Z

@stackman27 anything specific you want updated in the README.md?

maybe something around how "UpdateSearchRangeIfNeeded" updates the search range for BS. And what we do when BS gets gas intensive with CL pools

davidterpay · 2023-08-04T14:17:47Z

Updated the readme. @stackman27

davidterpay added 2 commits July 28, 2023 18:15

init

063602c

testing update

e2b628e

davidterpay requested a review from p0mvn as a code owner July 31, 2023 18:20

github-actions bot added the C:app-wiring Changes to the app folder label Jul 31, 2023

changelog

c9b13cd

davidterpay added the protorev All things related to x/protorev label Jul 31, 2023

comments

a706848

davidterpay mentioned this pull request Jul 31, 2023

ProtoRev v17 Improvements Tracking #5858

Closed

10 tasks

davidterpay added the V:state/breaking State machine breaking PR label Jul 31, 2023

NotJeremyLiu reviewed Jul 31, 2023

View reviewed changes

nit

efa68d2

NotJeremyLiu reviewed Jul 31, 2023

View reviewed changes

NotJeremyLiu requested changes Jul 31, 2023

View reviewed changes

davidterpay marked this pull request as draft July 31, 2023 21:08

davidterpay added 4 commits August 1, 2023 14:06

refactor

d25d53e

clean up

bde5c73

nit

2f314fa

comment

1ede81d

davidterpay marked this pull request as ready for review August 1, 2023 18:39

NotJeremyLiu reviewed Aug 1, 2023

View reviewed changes

cr

b0c92f3

NotJeremyLiu reviewed Aug 1, 2023

View reviewed changes

davidterpay added 3 commits August 1, 2023 15:08

cl are not symetric

1c78485

smh more symetric changes

64a6891

simpler safe swap

b8e3f1f

more stuff

da72dd0

czarcas7ic approved these changes Aug 1, 2023

View reviewed changes

stackman27 self-requested a review August 2, 2023 08:26

davidterpay added 2 commits August 2, 2023 16:41

more testing

7ef7106

adding comments

aadbcc7

NotJeremyLiu approved these changes Aug 3, 2023

View reviewed changes

stackman27 approved these changes Aug 3, 2023

View reviewed changes

AlpinYukseloglu approved these changes Aug 3, 2023

View reviewed changes

updating max range wit pool liq

9f1e6e4

updating readme

b890a43

github-actions bot added the C:docs Improvements or additions to documentation label Aug 4, 2023

czarcas7ic merged commit edd1822 into main Aug 4, 2023

czarcas7ic deleted the terpay/search-range-update branch August 4, 2023 23:11

github-actions bot mentioned this pull request Apr 15, 2024

Broken Links Detected #8046

Closed

github-actions bot mentioned this pull request May 1, 2024

Broken Links Detected #8183

Closed

github-actions bot mentioned this pull request Aug 15, 2024

Broken Links Detected notional-labs/osmosis#71

Closed

github-actions bot mentioned this pull request Nov 1, 2024

Broken Links Detected notional-labs/osmosis#76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ProtoRev): Updating Binary Search Range with CL Pools #5930

fix(ProtoRev): Updating Binary Search Range with CL Pools #5930

davidterpay commented Jul 31, 2023 •

edited

Loading

NotJeremyLiu Jul 31, 2023

NotJeremyLiu Jul 31, 2023

davidterpay Jul 31, 2023

czarcas7ic Aug 1, 2023

davidterpay Aug 2, 2023

davidterpay Aug 2, 2023

davidterpay Aug 2, 2023

NotJeremyLiu Jul 31, 2023

davidterpay Jul 31, 2023

NotJeremyLiu Jul 31, 2023

NotJeremyLiu Jul 31, 2023

NotJeremyLiu left a comment

NotJeremyLiu Aug 1, 2023

davidterpay commented Aug 1, 2023

NotJeremyLiu Aug 1, 2023

NotJeremyLiu Aug 1, 2023

czarcas7ic left a comment

czarcas7ic Aug 1, 2023

czarcas7ic Aug 1, 2023

davidterpay Aug 2, 2023

czarcas7ic Aug 1, 2023

davidterpay Aug 2, 2023

davidterpay Aug 2, 2023

NotJeremyLiu left a comment

stackman27 left a comment

AlpinYukseloglu Aug 3, 2023

davidterpay Aug 3, 2023

NotJeremyLiu Aug 4, 2023

davidterpay commented Aug 4, 2023

stackman27 commented Aug 4, 2023

davidterpay commented Aug 4, 2023

	* [#5930] (https://github.com/osmosis-labs/osmosis/pull/5930) Updating Binary Search Range with CL Pools
	* [#5930] (https://github.com/osmosis-labs/osmosis/pull/5930) Updating Protorev Binary Search Range Logic with CL Pools

		// At most we can swap half of the liquidity in the pool
		liquidTokenAmt := liquidity.AmountOf(outputCoin.Denom).Quo(sdk.NewInt(2))

fix(ProtoRev): Updating Binary Search Range with CL Pools #5930

fix(ProtoRev): Updating Binary Search Range with CL Pools #5930

Conversation

davidterpay commented Jul 31, 2023 • edited Loading

What is the purpose of the change

Testing and Verifying

Documentation and Release Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NotJeremyLiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidterpay commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NotJeremyLiu left a comment

Choose a reason for hiding this comment

stackman27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidterpay commented Aug 4, 2023

stackman27 commented Aug 4, 2023

davidterpay commented Aug 4, 2023

davidterpay commented Jul 31, 2023 •

edited

Loading