feat(ProtoRev): Supporting Two Pool Routes (#5953)

* testing

* changelog

* trading both ways

* updating test

* nit

* test fix

* protorev test update

* Update tests/e2e/configurer/chain/commands.go

---------

Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>

CHANGELOG.md

@@ -89,8 +89,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#5890](https://github.com/osmosis-labs/osmosis/pull/5890) feat: CreateCLPool & LinkCFMMtoCL pool into one gov-prop
 * [#5959](https://github.com/osmosis-labs/osmosis/pull/5959) allow testing with different chain-id's in E2E testing
 * [#5964](https://github.com/osmosis-labs/osmosis/pull/5964) fix e2e test concurrency bugs
-* [#5948](https://github.com/osmosis-labs/osmosis/pull/5948) Parameterizing Pool Type Information in Protorev
-* [#6001](https://github.com/osmosis-labs/osmosis/pull/6001) feat: improve set-env CLI cmd
+* [#5948] (https://github.com/osmosis-labs/osmosis/pull/5948) Parameterizing Pool Type Information in Protorev
+* [#6001](https://github.com/osmosis-labs/osmosis/pull/6001) feat: improve set-env CLI cmd\
+* [#5953] (https://github.com/osmosis-labs/osmosis/pull/5953) Supporting two pool routes in ProtoRev
 * [#6012](https://github.com/osmosis-labs/osmosis/pull/6012) chore: add autocomplete to makefile
 
 ### Minor improvements & Bug Fixes

tests/e2e/configurer/chain/commands.go

@@ -42,6 +42,13 @@ type params struct {
 	Value    string `json:"value"`
 }
 
+func (n *NodeConfig) SetMaxPoolPointsPerTx(maxPoolPoints int, from string) {
+	n.LogActionF("setting max pool points per tx for protorev at %d points", maxPoolPoints)
+	cmd := []string{"osmosisd", "tx", "protorev", "set-max-pool-points-per-tx", fmt.Sprintf("%d", maxPoolPoints), fmt.Sprintf("--from=%s", from), "--gas=700000", "--fees=5000uosmo"}
+	_, _, err := n.containerManager.ExecTxCmd(n.t, n.chainId, n.Name, cmd)
+	require.NoError(n.t, err)
+}
+
 func (n *NodeConfig) CreateBalancerPool(poolFile, from string) uint64 {
 	n.LogActionF("creating balancer pool from file %s", poolFile)
 	cmd := []string{"osmosisd", "tx", "gamm", "create-pool", fmt.Sprintf("--pool-file=/osmosis/%s", poolFile), fmt.Sprintf("--from=%s", from), "--gas=700000", "--fees=5000uosmo"}

tests/e2e/e2e_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/osmosis-labs/osmosis/v17/tests/e2e/initialization"
 	clmath "github.com/osmosis-labs/osmosis/v17/x/concentrated-liquidity/math"
 	cltypes "github.com/osmosis-labs/osmosis/v17/x/concentrated-liquidity/types"
+	protorevtypes "github.com/osmosis-labs/osmosis/v17/x/protorev/types"
 )
 
 var (
@@ -326,6 +327,15 @@ func (s *IntegrationTestSuite) ConcentratedLiquidity() {
 	err = chainABNode.ParamChangeProposal("concentratedliquidity", string(cltypes.KeyIsPermisionlessPoolCreationEnabled), []byte("true"), chainAB)
 	s.Require().NoError(err)
 
+	// Update the protorev admin address to a known wallet we can control
+
+	adminWalletAddr := chainABNode.CreateWalletAndFund("admin", []string{"4000000uosmo"}, chainAB)
+	err = chainABNode.ParamChangeProposal("protorev", string(protorevtypes.ParamStoreKeyAdminAccount), []byte(fmt.Sprintf(`"%s"`, adminWalletAddr)), chainAB)
+	s.Require().NoError(err)
+
+	// Update the weight of CL pools so that this test case is not back run by protorev.
+	chainABNode.SetMaxPoolPointsPerTx(7, adminWalletAddr)
+
 	// Confirm that the parameter has been changed.
 	isPermisionlessCreationEnabledStr = chainABNode.QueryParams(cltypes.ModuleName, string(cltypes.KeyIsPermisionlessPoolCreationEnabled))
 	if !strings.EqualFold(isPermisionlessCreationEnabledStr, "true") {
@@ -869,6 +879,9 @@ func (s *IntegrationTestSuite) ConcentratedLiquidity() {
 	// Check that the tick spacing was reduced to the expected new tick spacing
 	concentratedPool = s.updatedConcentratedPool(chainABNode, poolID)
 	s.Require().Equal(newTickSpacing, concentratedPool.GetTickSpacing())
+
+	// Reset the maximum number of pool points
+	chainABNode.SetMaxPoolPointsPerTx(int(protorevtypes.DefaultMaxPoolPointsPerTx), adminWalletAddr)
 }
 
 func (s *IntegrationTestSuite) StableSwapPostUpgrade() {

x/protorev/keeper/keeper_test.go

@@ -135,6 +135,7 @@ func (s *KeeperTestSuite) SetupTest() {
 		sdk.NewCoin("gamm/pool/1", sdk.NewInt(9000000000000000000)),
 		sdk.NewCoin(apptesting.DefaultTransmuterDenomA, sdk.NewInt(9000000000000000000)),
 		sdk.NewCoin(apptesting.DefaultTransmuterDenomB, sdk.NewInt(9000000000000000000)),
+		sdk.NewCoin("stake", sdk.NewInt(9000000000000000000)),
 	)
 	s.fundAllAccountsWith()
 	s.Commit()
@@ -916,16 +917,101 @@ func (s *KeeperTestSuite) setUpPools() {
 	}
 	s.App.ProtoRevKeeper.SetInfoByPoolType(s.Ctx, poolInfo)
 
-	// Create a concentrated liquidity pool for range testing
+	// Create a duplicate pool for testing
 	// Pool 52
+	s.createGAMMPool(
+		[]balancer.PoolAsset{
+			{
+				Token:  sdk.NewCoin("Atom", sdk.NewInt(10000)),
+				Weight: sdk.NewInt(1),
+			},
+			{
+				Token:  sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(10000)),
+				Weight: sdk.NewInt(1),
+			},
+		},
+		sdk.NewDecWithPrec(2, 3),
+		sdk.NewDecWithPrec(0, 2),
+	)
+
+	// Create a duplicate pool for testing
+	// Pool 53
+	s.createGAMMPool(
+		[]balancer.PoolAsset{
+			{
+				Token:  sdk.NewCoin("usdc", sdk.NewInt(10000)),
+				Weight: sdk.NewInt(1),
+			},
+			{
+				Token:  sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(10000)),
+				Weight: sdk.NewInt(1),
+			},
+		},
+		sdk.NewDecWithPrec(2, 3),
+		sdk.NewDecWithPrec(0, 2),
+	)
+
+	// Create a duplicate pool for testing
+	// Pool 54
+	s.createGAMMPool(
+		[]balancer.PoolAsset{
+			{
+				Token:  sdk.NewCoin("stake", sdk.NewInt(10000)),
+				Weight: sdk.NewInt(1),
+			},
+			{
+				Token:  sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(10000)),
+				Weight: sdk.NewInt(1),
+			},
+		},
+		sdk.NewDecWithPrec(2, 3),
+		sdk.NewDecWithPrec(0, 2),
+	)
+
+	// Create a duplicate pool for testing
+	// Pool 55
+	s.createGAMMPool(
+		[]balancer.PoolAsset{
+			{
+				Token:  sdk.NewCoin("stake", sdk.NewInt(100000000)),
+				Weight: sdk.NewInt(1),
+			},
+			{
+				Token:  sdk.NewCoin(types.OsmosisDenomination, sdk.NewInt(1000000000)),
+				Weight: sdk.NewInt(1),
+			},
+		},
+		sdk.NewDecWithPrec(2, 3),
+		sdk.NewDecWithPrec(0, 2),
+	)
+
+	// Create a duplicate pool for testing
+	// Pool 56
+	s.createGAMMPool(
+		[]balancer.PoolAsset{
+			{
+				Token:  sdk.NewCoin("bitcoin", sdk.NewInt(100)),
+				Weight: sdk.NewInt(1),
+			},
+			{
+				Token:  sdk.NewCoin("Atom", sdk.NewInt(100)),
+				Weight: sdk.NewInt(1),
+			},
+		},
+		sdk.NewDecWithPrec(2, 3),
+		sdk.NewDecWithPrec(0, 2),
+	)
+
+	// Create a concentrated liquidity pool for range testing
+	// Pool 58
 	// Create the CL pool
 	clPool := s.PrepareCustomConcentratedPool(s.TestAccs[0], "epochTwo", "uosmo", apptesting.DefaultTickSpacing, sdk.ZeroDec())
 	fundCoins := sdk.NewCoins(sdk.NewCoin("epochTwo", sdk.NewInt(10_000_000_000_000)), sdk.NewCoin("uosmo", sdk.NewInt(10_000_000_000_000)))
 	s.FundAcc(s.TestAccs[0], fundCoins)
 	s.CreateFullRangePosition(clPool, fundCoins)
 
 	// Create a concentrated liquidity pool for range testing
-	// Pool 53
+	// Pool 59
 	// Create the CL pool
 	clPool = s.PrepareCustomConcentratedPool(s.TestAccs[0], "epochTwo", "uosmo", apptesting.DefaultTickSpacing, sdk.ZeroDec())
 	fundCoins = sdk.NewCoins(sdk.NewCoin("epochTwo", sdk.NewInt(2_000_000_000)), sdk.NewCoin("uosmo", sdk.NewInt(1_000_000_000)))

x/protorev/keeper/posthandler_test.go

@@ -220,7 +220,7 @@ func (s *KeeperTestSuite) TestAnteHandle() {
 			expectPass: true,
 		},
 		{
-			name: "Two Pool Arb Route - Hot Route Build",
+			name: "Two Pool Arb Route",
 			params: param{
 				trades: []types.Trade{
 					{
@@ -235,16 +235,12 @@ func (s *KeeperTestSuite) TestAnteHandle() {
 						Denom:  "Atom",
 						Amount: sdk.NewInt(15_767_231),
 					},
-					{
-						Denom:  "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
-						Amount: sdk.NewInt(218_149_058),
-					},
 					{
 						Denom:  types.OsmosisDenomination,
-						Amount: sdk.NewInt(56_609_900),
+						Amount: sdk.NewInt(256_086_256),
 					},
 				},
-				expectedPoolPoints: 33,
+				expectedPoolPoints: 41,
 			},
 			expectPass: true,
 		},
@@ -264,16 +260,12 @@ func (s *KeeperTestSuite) TestAnteHandle() {
 						Denom:  "Atom",
 						Amount: sdk.NewInt(15_767_231),
 					},
-					{
-						Denom:  "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
-						Amount: sdk.NewInt(218_149_058),
-					},
 					{
 						Denom:  types.OsmosisDenomination,
-						Amount: sdk.NewInt(56_609_900),
+						Amount: sdk.NewInt(256_086_256),
 					},
 				},
-				expectedPoolPoints: 33,
+				expectedPoolPoints: 41,
 			},
 			expectPass: true,
 		},
@@ -293,16 +285,12 @@ func (s *KeeperTestSuite) TestAnteHandle() {
 						Denom:  "Atom",
 						Amount: sdk.NewInt(15_767_231),
 					},
-					{
-						Denom:  "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
-						Amount: sdk.NewInt(218_149_058),
-					},
 					{
 						Denom:  types.OsmosisDenomination,
-						Amount: sdk.NewInt(56_609_900),
+						Amount: sdk.NewInt(256_086_256),
 					},
 				},
-				expectedPoolPoints: 33,
+				expectedPoolPoints: 41,
 			},
 			expectPass: true,
 		},
@@ -322,16 +310,12 @@ func (s *KeeperTestSuite) TestAnteHandle() {
 						Denom:  "Atom",
 						Amount: sdk.NewInt(15_767_231),
 					},
-					{
-						Denom:  "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
-						Amount: sdk.NewInt(218_149_058),
-					},
 					{
 						Denom:  types.OsmosisDenomination,
-						Amount: sdk.NewInt(56_609_900),
+						Amount: sdk.NewInt(256_086_256),
 					},
 				},
-				expectedPoolPoints: 33,
+				expectedPoolPoints: 41,
 			},
 			expectPass: true,
 		},

x/protorev/keeper/protorev.go

@@ -118,7 +118,7 @@ func (k Keeper) DeleteBaseDenoms(ctx sdk.Context) {
 	k.DeleteAllEntriesForKeyPrefix(ctx, types.KeyPrefixBaseDenoms)
 }
 
-// GetPoolForDenomPair returns the id of the highest liquidty pool between the base denom and the denom to match
+// GetPoolForDenomPair returns the id of the highest liquidity pool between the base denom and the denom to match
 func (k Keeper) GetPoolForDenomPair(ctx sdk.Context, baseDenom, denomToMatch string) (uint64, error) {
 	store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixDenomPairToPool)
 	key := types.GetKeyPrefixDenomPairToPool(baseDenom, denomToMatch)

x/protorev/keeper/rebalance_test.go

@@ -182,11 +182,11 @@ var clPoolRouteExtended = poolmanagertypes.SwapAmountInRoutes{
 // Tests multiple CL pools in the same route
 var clPoolRouteMulti = poolmanagertypes.SwapAmountInRoutes{
 	poolmanagertypes.SwapAmountInRoute{
-		PoolId:        52,
+		PoolId:        57,
 		TokenOutDenom: "uosmo",
 	},
 	poolmanagertypes.SwapAmountInRoute{
-		PoolId:        53,
+		PoolId:        58,
 		TokenOutDenom: "epochTwo",
 	},
 }
@@ -198,7 +198,7 @@ var clPoolRoute = poolmanagertypes.SwapAmountInRoutes{
 		TokenOutDenom: "uosmo",
 	},
 	poolmanagertypes.SwapAmountInRoute{
-		PoolId:        52,
+		PoolId:        57,
 		TokenOutDenom: "epochTwo",
 	},
 }

x/protorev/keeper/routes.go

@@ -102,6 +102,10 @@ func (k Keeper) BuildHighestLiquidityRoutes(ctx sdk.Context, tokenIn, tokenOut s
 		if newRoute, err := k.BuildHighestLiquidityRoute(ctx, baseDenom, tokenIn, tokenOut, poolId); err == nil {
 			routes = append(routes, newRoute)
 		}
+
+		if newRoute, err := k.BuildTwoPoolRoute(ctx, baseDenom, tokenIn, tokenOut, poolId); err == nil {
+			routes = append(routes, newRoute)
+		}
 	}
 
 	return routes, nil
@@ -149,6 +153,70 @@ func (k Keeper) BuildHighestLiquidityRoute(ctx sdk.Context, swapDenom types.Base
 	}, nil
 }
 
+// BuildTwoPoolRoute will attempt to create a two pool route that will rebalance pools that are paired
+// with the base denom. This is useful for pools that contain the same assets but are imbalanced.
+func (k Keeper) BuildTwoPoolRoute(
+	ctx sdk.Context,
+	baseDenom types.BaseDenom,
+	tokenInDenom, tokenOutDenom string,
+	poolId uint64,
+) (RouteMetaData, error) {
+	if baseDenom.Denom != tokenInDenom && baseDenom.Denom != tokenOutDenom {
+		return RouteMetaData{}, fmt.Errorf("base denom (%s) must be either tokenIn (%s) or tokenOut (%s)", baseDenom.Denom, tokenInDenom, tokenOutDenom)
+	}
+
+	var (
+		pool1, pool2 uint64
+	)
+
+	// In the case where the base denom is the swap out, the base denom becomes more ~expensive~ on the current pool id
+	// and potentially cheaper on the highest liquidity pool. So we swap first on the current pool id and then on the
+	// highest liquidity pool.
+	if tokenOutDenom == baseDenom.Denom {
+		highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, tokenInDenom)
+		if err != nil {
+			return RouteMetaData{}, err
+		}
+
+		pool1, pool2 = poolId, highestLiquidityPool
+		tokenOutDenom = tokenInDenom
+	} else {
+		highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, tokenOutDenom)
+		if err != nil {
+			return RouteMetaData{}, err
+		}
+
+		pool1, pool2 = highestLiquidityPool, poolId
+	}
+
+	if pool1 == pool2 {
+		return RouteMetaData{}, fmt.Errorf("cannot be trading on the same pool twice")
+	}
+
+	newRoute := poolmanagertypes.SwapAmountInRoutes{
+		poolmanagertypes.SwapAmountInRoute{
+			TokenOutDenom: tokenOutDenom,
+			PoolId:        pool1,
+		},
+		poolmanagertypes.SwapAmountInRoute{
+			TokenOutDenom: baseDenom.Denom,
+			PoolId:        pool2,
+		},
+	}
+
+	// Check that the route is valid and update the number of pool points that this route will consume when simulating and executing trades
+	routePoolPoints, err := k.CalculateRoutePoolPoints(ctx, newRoute)
+	if err != nil {
+		return RouteMetaData{}, err
+	}
+
+	return RouteMetaData{
+		Route:      newRoute,
+		PoolPoints: routePoolPoints,
+		StepSize:   baseDenom.StepSize,
+	}, nil
+}
+
 // CalculateRoutePoolPoints calculates the number of pool points that will be consumed by a route when simulating and executing trades. This
 // is only added to the global pool point counter if the route simulated is minimally profitable i.e. it will make a profit.
 func (k Keeper) CalculateRoutePoolPoints(ctx sdk.Context, route poolmanagertypes.SwapAmountInRoutes) (uint64, error) {