From 16ef254603dd149f3513f7eda1f822d6621e5cce Mon Sep 17 00:00:00 2001 From: David Terpay Date: Thu, 3 Aug 2023 10:34:30 -0400 Subject: [PATCH 1/8] testing --- x/protorev/keeper/keeper_test.go | 86 +++++++++++++++++++++++++ x/protorev/keeper/posthandler_test.go | 2 +- x/protorev/keeper/protorev.go | 2 +- x/protorev/keeper/routes.go | 49 +++++++++++++++ x/protorev/keeper/routes_test.go | 91 ++++++++++++++++++++++++++- 5 files changed, 226 insertions(+), 4 deletions(-) diff --git a/x/protorev/keeper/keeper_test.go b/x/protorev/keeper/keeper_test.go index 1c378d5a025..2d057878d75 100644 --- a/x/protorev/keeper/keeper_test.go +++ b/x/protorev/keeper/keeper_test.go @@ -142,6 +142,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() @@ -902,6 +903,91 @@ func (s *KeeperTestSuite) setUpPools() { // Pool 50 s.PrepareCosmWasmPool() + // Create a duplicate pool for testing + // Pool 51 + 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 52 + 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 53 + 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 54 + 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 55 + 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), + ) + // Set all of the pool info into the stores err := s.App.ProtoRevKeeper.UpdatePools(s.Ctx) s.Require().NoError(err) diff --git a/x/protorev/keeper/posthandler_test.go b/x/protorev/keeper/posthandler_test.go index 725652e7e66..6e2014e9174 100644 --- a/x/protorev/keeper/posthandler_test.go +++ b/x/protorev/keeper/posthandler_test.go @@ -340,7 +340,7 @@ func (s *KeeperTestSuite) TestAnteHandle() { params: param{ trades: []types.Trade{ { - Pool: 51, + Pool: 56, TokenOut: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", TokenIn: "uosmo", }, diff --git a/x/protorev/keeper/protorev.go b/x/protorev/keeper/protorev.go index fac66ce78fe..9766ca98cf2 100644 --- a/x/protorev/keeper/protorev.go +++ b/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) diff --git a/x/protorev/keeper/routes.go b/x/protorev/keeper/routes.go index 404b6576ee8..6ad33165198 100644 --- a/x/protorev/keeper/routes.go +++ b/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,51 @@ 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, tokenIn, tokenOut string, poolId uint64) (RouteMetaData, error) { + if baseDenom.Denom != tokenOut { + return RouteMetaData{}, fmt.Errorf("the token out denom must be the same as the base denom: got %s, expected %s", tokenOut, baseDenom.Denom) + } + + // Get the highest liquidity pool for the tokenIn and base denom + highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, tokenIn) + if err != nil { + return RouteMetaData{}, err + } + + // If the swapped on pool is already the pool with the highest liquidity, we cannot build a two pool route (it would be a two hop route with the same pool) + if highestLiquidityPool == poolId { + return RouteMetaData{}, fmt.Errorf("the pool id must be different from the highest liquidity pool: id %d", poolId) + } + + // Create the first swap for the MultiHopSwap Route + entryHop := poolmanagertypes.SwapAmountInRoute{ + PoolId: highestLiquidityPool, + TokenOutDenom: tokenIn, + } + + // Creating the second swap in the arb + exitHop := poolmanagertypes.SwapAmountInRoute{ + PoolId: poolId, + TokenOutDenom: baseDenom.Denom, + } + + newRoute := poolmanagertypes.SwapAmountInRoutes{entryHop, exitHop} + + // 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) { diff --git a/x/protorev/keeper/routes_test.go b/x/protorev/keeper/routes_test.go index 296b52d3a2f..77e8f86057c 100644 --- a/x/protorev/keeper/routes_test.go +++ b/x/protorev/keeper/routes_test.go @@ -44,13 +44,17 @@ func (s *KeeperTestSuite) TestBuildRoutes() { description: "Route exists for swap in Bitcoin and swap out Atom", inputDenom: "bitcoin", outputDenom: "Atom", - poolID: 4, + poolID: 55, expectedRoutes: [][]TestRoute{ { {PoolId: 25, InputDenom: types.OsmosisDenomination, OutputDenom: "Atom"}, - {PoolId: 4, InputDenom: "Atom", OutputDenom: "bitcoin"}, + {PoolId: 55, InputDenom: "Atom", OutputDenom: "bitcoin"}, {PoolId: 10, InputDenom: "bitcoin", OutputDenom: types.OsmosisDenomination}, }, + { + {PoolId: 4, InputDenom: "Atom", OutputDenom: "bitcoin"}, + {PoolId: 55, InputDenom: "bitcoin", OutputDenom: "Atom"}, + }, }, }, { @@ -91,6 +95,18 @@ func (s *KeeperTestSuite) TestBuildRoutes() { }, }, }, + { + description: "Two Pool Route exists for (osmo, atom)", + inputDenom: "Atom", + outputDenom: types.OsmosisDenomination, + poolID: 51, + expectedRoutes: [][]TestRoute{ + { + {PoolId: 25, InputDenom: types.OsmosisDenomination, OutputDenom: "Atom"}, + {PoolId: 51, InputDenom: "Atom", OutputDenom: types.OsmosisDenomination}, + }, + }, + }, } for _, tc := range cases { @@ -225,6 +241,77 @@ func (s *KeeperTestSuite) TestBuildHighestLiquidityRoute() { } } +// TestBuildTwoPoolRoute tests the BuildTwoPoolRoute function +func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { + cases := []struct { + description string + swapDenom types.BaseDenom + tokenIn string + tokenOut string + poolId uint64 + expectedRoute []TestRoute + hasRoute bool + }{ + { + description: "two pool route can be created", + swapDenom: types.BaseDenom{ + Denom: types.OsmosisDenomination, + StepSize: sdk.NewInt(1_000_000), + }, + tokenIn: "stake", + tokenOut: types.OsmosisDenomination, + poolId: 53, + expectedRoute: []TestRoute{ + {PoolId: 54, InputDenom: types.OsmosisDenomination, OutputDenom: "stake"}, + {PoolId: 53, InputDenom: "stake", OutputDenom: types.OsmosisDenomination}, + }, + hasRoute: true, + }, + { + description: "two pool route where swap is on the highest liquidity pool", + swapDenom: types.BaseDenom{ + Denom: types.OsmosisDenomination, + StepSize: sdk.NewInt(1_000_000), + }, + tokenIn: "stake", + tokenOut: types.OsmosisDenomination, + poolId: 54, + expectedRoute: []TestRoute{}, + hasRoute: false, + }, + { + description: "two pool route where swap in is the base denom", + swapDenom: types.BaseDenom{ + Denom: types.OsmosisDenomination, + StepSize: sdk.NewInt(1_000_000), + }, + tokenIn: types.OsmosisDenomination, + tokenOut: "stake", + poolId: 53, + expectedRoute: []TestRoute{}, + hasRoute: false, + }, + } + + for _, tc := range cases { + s.Run(tc.description, func() { + routeMetaData, err := s.App.ProtoRevKeeper.BuildTwoPoolRoute(s.Ctx, tc.swapDenom, tc.tokenIn, tc.tokenOut, tc.poolId) + + if tc.hasRoute { + s.Require().NoError(err) + s.Require().Equal(len(tc.expectedRoute), len(routeMetaData.Route.PoolIds())) + + for index, trade := range tc.expectedRoute { + s.Require().Equal(trade.PoolId, routeMetaData.Route.PoolIds()[index]) + } + } else { + s.Require().Equal(len(tc.expectedRoute), len(routeMetaData.Route.PoolIds())) + s.Require().Error(err) + } + }) + } +} + // TestBuildHotRoutes tests the BuildHotRoutes function func (s *KeeperTestSuite) TestBuildHotRoutes() { cases := []struct { From a65020ae01122921ff9e295f96f078a58de232fe Mon Sep 17 00:00:00 2001 From: David Terpay Date: Thu, 3 Aug 2023 10:40:13 -0400 Subject: [PATCH 2/8] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 034411997db..778031ac63c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * [#5870] (https://github.com/osmosis-labs/osmosis/pull/5870) Remove v14/ separator in protorev rest endpoints * [#5923] (https://github.com/osmosis-labs/osmosis/pull/5923) CL: Lower gas for initializing ticks * [#5927] (https://github.com/osmosis-labs/osmosis/pull/5927) Add gas metering to x/tokenfactory trackBeforeSend hook +* [#5953] (https://github.com/osmosis-labs/osmosis/pull/5953) Supporting two pool routes in ProtoRev ### Minor improvements & Bug Fixes From 0d8b0360c350062c4fbe9d6bdc61706d44605451 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Thu, 3 Aug 2023 19:54:14 -0400 Subject: [PATCH 3/8] trading both ways --- x/protorev/keeper/posthandler_test.go | 42 +++++------------ x/protorev/keeper/routes.go | 67 ++++++++++++++++++--------- x/protorev/keeper/routes_test.go | 49 +++++++++++++++----- 3 files changed, 93 insertions(+), 65 deletions(-) diff --git a/x/protorev/keeper/posthandler_test.go b/x/protorev/keeper/posthandler_test.go index 6e2014e9174..95a166d8d80 100644 --- a/x/protorev/keeper/posthandler_test.go +++ b/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, }, @@ -351,16 +335,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: 37, + expectedPoolPoints: 45, }, expectPass: true, }, diff --git a/x/protorev/keeper/routes.go b/x/protorev/keeper/routes.go index 6ad33165198..e2ec928bdbe 100644 --- a/x/protorev/keeper/routes.go +++ b/x/protorev/keeper/routes.go @@ -153,37 +153,58 @@ 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, tokenIn, tokenOut string, poolId uint64) (RouteMetaData, error) { - if baseDenom.Denom != tokenOut { - return RouteMetaData{}, fmt.Errorf("the token out denom must be the same as the base denom: got %s, expected %s", tokenOut, baseDenom.Denom) +// 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, + swapIn, swapOut string, + poolId uint64, +) (RouteMetaData, error) { + if baseDenom.Denom != swapIn && baseDenom.Denom != swapOut { + return RouteMetaData{}, fmt.Errorf("base denom (%s) must be either tokenIn (%s) or tokenOut (%s)", baseDenom.Denom, swapIn, swapOut) } - // Get the highest liquidity pool for the tokenIn and base denom - highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, tokenIn) - if err != nil { - return RouteMetaData{}, err - } + var ( + tokenOutDenom string + pool1, pool2 uint64 + ) - // If the swapped on pool is already the pool with the highest liquidity, we cannot build a two pool route (it would be a two hop route with the same pool) - if highestLiquidityPool == poolId { - return RouteMetaData{}, fmt.Errorf("the pool id must be different from the highest liquidity pool: id %d", poolId) - } + // 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 swapOut == baseDenom.Denom { + highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, swapIn) + if err != nil { + return RouteMetaData{}, err + } - // Create the first swap for the MultiHopSwap Route - entryHop := poolmanagertypes.SwapAmountInRoute{ - PoolId: highestLiquidityPool, - TokenOutDenom: tokenIn, + pool1, pool2 = poolId, highestLiquidityPool + tokenOutDenom = swapIn + } else { + highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, swapOut) + if err != nil { + return RouteMetaData{}, err + } + + pool1, pool2 = highestLiquidityPool, poolId + tokenOutDenom = swapOut } - // Creating the second swap in the arb - exitHop := poolmanagertypes.SwapAmountInRoute{ - PoolId: poolId, - TokenOutDenom: baseDenom.Denom, + if pool1 == pool2 { + return RouteMetaData{}, fmt.Errorf("cannot be trading on the same pool twice") } - newRoute := poolmanagertypes.SwapAmountInRoutes{entryHop, exitHop} + 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) diff --git a/x/protorev/keeper/routes_test.go b/x/protorev/keeper/routes_test.go index 77e8f86057c..704acb2f3a6 100644 --- a/x/protorev/keeper/routes_test.go +++ b/x/protorev/keeper/routes_test.go @@ -52,8 +52,8 @@ func (s *KeeperTestSuite) TestBuildRoutes() { {PoolId: 10, InputDenom: "bitcoin", OutputDenom: types.OsmosisDenomination}, }, { - {PoolId: 4, InputDenom: "Atom", OutputDenom: "bitcoin"}, - {PoolId: 55, InputDenom: "bitcoin", OutputDenom: "Atom"}, + {PoolId: 55, InputDenom: "Atom", OutputDenom: "bitcoin"}, + {PoolId: 4, InputDenom: "bitcoin", OutputDenom: "Atom"}, }, }, }, @@ -102,8 +102,12 @@ func (s *KeeperTestSuite) TestBuildRoutes() { poolID: 51, expectedRoutes: [][]TestRoute{ { - {PoolId: 25, InputDenom: types.OsmosisDenomination, OutputDenom: "Atom"}, - {PoolId: 51, InputDenom: "Atom", OutputDenom: types.OsmosisDenomination}, + {PoolId: 51, InputDenom: types.OsmosisDenomination, OutputDenom: "Atom"}, + {PoolId: 25, InputDenom: "Atom", OutputDenom: types.OsmosisDenomination}, + }, + { + {PoolId: 25, InputDenom: "Atom", OutputDenom: types.OsmosisDenomination}, + {PoolId: 51, InputDenom: types.OsmosisDenomination, OutputDenom: "Atom"}, }, }, }, @@ -117,6 +121,7 @@ func (s *KeeperTestSuite) TestBuildRoutes() { for routeIndex, route := range routes { for tradeIndex, poolID := range route.Route.PoolIds() { s.Require().Equal(tc.expectedRoutes[routeIndex][tradeIndex].PoolId, poolID) + s.Require().Equal(tc.expectedRoutes[routeIndex][tradeIndex].OutputDenom, route.Route[tradeIndex].TokenOutDenom) } } }) @@ -253,7 +258,7 @@ func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { hasRoute bool }{ { - description: "two pool route can be created", + description: "two pool route can be created with base as token out", swapDenom: types.BaseDenom{ Denom: types.OsmosisDenomination, StepSize: sdk.NewInt(1_000_000), @@ -261,6 +266,21 @@ func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { tokenIn: "stake", tokenOut: types.OsmosisDenomination, poolId: 53, + expectedRoute: []TestRoute{ + {PoolId: 53, InputDenom: types.OsmosisDenomination, OutputDenom: "stake"}, + {PoolId: 54, InputDenom: "stake", OutputDenom: types.OsmosisDenomination}, + }, + hasRoute: true, + }, + { + description: "two pool route can be created with base as token in", + swapDenom: types.BaseDenom{ + Denom: types.OsmosisDenomination, + StepSize: sdk.NewInt(1_000_000), + }, + tokenIn: types.OsmosisDenomination, + tokenOut: "stake", + poolId: 53, expectedRoute: []TestRoute{ {PoolId: 54, InputDenom: types.OsmosisDenomination, OutputDenom: "stake"}, {PoolId: 53, InputDenom: "stake", OutputDenom: types.OsmosisDenomination}, @@ -280,14 +300,14 @@ func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { hasRoute: false, }, { - description: "two pool route where swap in is the base denom", + description: "trade executes on pool not tracked by the module", swapDenom: types.BaseDenom{ Denom: types.OsmosisDenomination, StepSize: sdk.NewInt(1_000_000), }, - tokenIn: types.OsmosisDenomination, - tokenOut: "stake", - poolId: 53, + tokenIn: "stake", + tokenOut: types.OsmosisDenomination, + poolId: 1000000, expectedRoute: []TestRoute{}, hasRoute: false, }, @@ -295,14 +315,21 @@ func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { for _, tc := range cases { s.Run(tc.description, func() { - routeMetaData, err := s.App.ProtoRevKeeper.BuildTwoPoolRoute(s.Ctx, tc.swapDenom, tc.tokenIn, tc.tokenOut, tc.poolId) + routeMetaData, err := s.App.ProtoRevKeeper.BuildTwoPoolRoute( + s.Ctx, + tc.swapDenom, + tc.tokenIn, + tc.tokenOut, + tc.poolId, + ) if tc.hasRoute { s.Require().NoError(err) s.Require().Equal(len(tc.expectedRoute), len(routeMetaData.Route.PoolIds())) for index, trade := range tc.expectedRoute { - s.Require().Equal(trade.PoolId, routeMetaData.Route.PoolIds()[index]) + s.Require().Equal(trade.PoolId, routeMetaData.Route[index].PoolId) + s.Require().Equal(trade.OutputDenom, routeMetaData.Route[index].TokenOutDenom) } } else { s.Require().Equal(len(tc.expectedRoute), len(routeMetaData.Route.PoolIds())) From 5213b405ea9c54bc33b1278a5c4c3ad7d136a772 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Mon, 7 Aug 2023 11:12:27 -0400 Subject: [PATCH 4/8] updating test --- x/protorev/keeper/keeper_test.go | 15 ++++++++------- x/protorev/keeper/rebalance_test.go | 6 +++--- x/protorev/keeper/routes_test.go | 14 +++++++------- 3 files changed, 18 insertions(+), 17 deletions(-) diff --git a/x/protorev/keeper/keeper_test.go b/x/protorev/keeper/keeper_test.go index 0fe048f3bb6..bf9d341cdca 100644 --- a/x/protorev/keeper/keeper_test.go +++ b/x/protorev/keeper/keeper_test.go @@ -915,7 +915,7 @@ func (s *KeeperTestSuite) setUpPools() { s.PrepareCosmWasmPool() // Create a duplicate pool for testing - // Pool 51 + // Pool 52 s.createGAMMPool( []balancer.PoolAsset{ { @@ -932,7 +932,7 @@ func (s *KeeperTestSuite) setUpPools() { ) // Create a duplicate pool for testing - // Pool 52 + // Pool 53 s.createGAMMPool( []balancer.PoolAsset{ { @@ -949,7 +949,7 @@ func (s *KeeperTestSuite) setUpPools() { ) // Create a duplicate pool for testing - // Pool 53 + // Pool 54 s.createGAMMPool( []balancer.PoolAsset{ { @@ -966,7 +966,7 @@ func (s *KeeperTestSuite) setUpPools() { ) // Create a duplicate pool for testing - // Pool 54 + // Pool 55 s.createGAMMPool( []balancer.PoolAsset{ { @@ -983,7 +983,7 @@ func (s *KeeperTestSuite) setUpPools() { ) // Create a duplicate pool for testing - // Pool 55 + // Pool 56 s.createGAMMPool( []balancer.PoolAsset{ { @@ -998,8 +998,9 @@ func (s *KeeperTestSuite) setUpPools() { sdk.NewDecWithPrec(2, 3), sdk.NewDecWithPrec(0, 2), ) + // Create a concentrated liquidity pool for range testing - // Pool 57 + // 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))) @@ -1007,7 +1008,7 @@ func (s *KeeperTestSuite) setUpPools() { s.CreateFullRangePosition(clPool, fundCoins) // Create a concentrated liquidity pool for range testing - // Pool 58 + // 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))) diff --git a/x/protorev/keeper/rebalance_test.go b/x/protorev/keeper/rebalance_test.go index c6753381326..6a26714ea74 100644 --- a/x/protorev/keeper/rebalance_test.go +++ b/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", }, } diff --git a/x/protorev/keeper/routes_test.go b/x/protorev/keeper/routes_test.go index de7f64f2e10..e312b7b2a72 100644 --- a/x/protorev/keeper/routes_test.go +++ b/x/protorev/keeper/routes_test.go @@ -265,10 +265,10 @@ func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { }, tokenIn: "stake", tokenOut: types.OsmosisDenomination, - poolId: 53, + poolId: 54, expectedRoute: []TestRoute{ - {PoolId: 53, InputDenom: types.OsmosisDenomination, OutputDenom: "stake"}, - {PoolId: 54, InputDenom: "stake", OutputDenom: types.OsmosisDenomination}, + {PoolId: 54, InputDenom: types.OsmosisDenomination, OutputDenom: "stake"}, + {PoolId: 55, InputDenom: "stake", OutputDenom: types.OsmosisDenomination}, }, hasRoute: true, }, @@ -280,10 +280,10 @@ func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { }, tokenIn: types.OsmosisDenomination, tokenOut: "stake", - poolId: 53, + poolId: 54, expectedRoute: []TestRoute{ - {PoolId: 54, InputDenom: types.OsmosisDenomination, OutputDenom: "stake"}, - {PoolId: 53, InputDenom: "stake", OutputDenom: types.OsmosisDenomination}, + {PoolId: 55, InputDenom: types.OsmosisDenomination, OutputDenom: "stake"}, + {PoolId: 54, InputDenom: "stake", OutputDenom: types.OsmosisDenomination}, }, hasRoute: true, }, @@ -295,7 +295,7 @@ func (s *KeeperTestSuite) TestBuildTwoPoolRoute() { }, tokenIn: "stake", tokenOut: types.OsmosisDenomination, - poolId: 54, + poolId: 55, expectedRoute: []TestRoute{}, hasRoute: false, }, From 5e12273777667de29c07f5d9e49c9f4a4d9661d1 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Mon, 7 Aug 2023 11:19:59 -0400 Subject: [PATCH 5/8] nit --- x/protorev/keeper/routes.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/x/protorev/keeper/routes.go b/x/protorev/keeper/routes.go index e2ec928bdbe..478d690c39f 100644 --- a/x/protorev/keeper/routes.go +++ b/x/protorev/keeper/routes.go @@ -158,37 +158,35 @@ func (k Keeper) BuildHighestLiquidityRoute(ctx sdk.Context, swapDenom types.Base func (k Keeper) BuildTwoPoolRoute( ctx sdk.Context, baseDenom types.BaseDenom, - swapIn, swapOut string, + tokenInDenom, tokenOutDenom string, poolId uint64, ) (RouteMetaData, error) { - if baseDenom.Denom != swapIn && baseDenom.Denom != swapOut { - return RouteMetaData{}, fmt.Errorf("base denom (%s) must be either tokenIn (%s) or tokenOut (%s)", baseDenom.Denom, swapIn, swapOut) + 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 ( - tokenOutDenom string - pool1, pool2 uint64 + 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 swapOut == baseDenom.Denom { - highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, swapIn) + if tokenOutDenom == baseDenom.Denom { + highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, tokenInDenom) if err != nil { return RouteMetaData{}, err } pool1, pool2 = poolId, highestLiquidityPool - tokenOutDenom = swapIn + tokenOutDenom = tokenInDenom } else { - highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, swapOut) + highestLiquidityPool, err := k.GetPoolForDenomPair(ctx, baseDenom.Denom, tokenOutDenom) if err != nil { return RouteMetaData{}, err } pool1, pool2 = highestLiquidityPool, poolId - tokenOutDenom = swapOut } if pool1 == pool2 { From 92e297902f64cdcef35cd2413de89eb93e13c843 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Tue, 8 Aug 2023 13:16:09 -0400 Subject: [PATCH 6/8] test fix --- tests/e2e/e2e_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 79a2bca1e96..3c0e9292422 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -3,7 +3,6 @@ package e2e import ( "encoding/json" "fmt" - "github.com/cosmos/cosmos-sdk/types/address" "os" "path/filepath" "strconv" @@ -12,6 +11,8 @@ import ( "testing" "time" + "github.com/cosmos/cosmos-sdk/types/address" + transfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types" "github.com/iancoleman/orderedmap" @@ -34,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 ( @@ -325,6 +327,10 @@ func (s *IntegrationTestSuite) ConcentratedLiquidity() { err = chainABNode.ParamChangeProposal("concentratedliquidity", string(cltypes.KeyIsPermisionlessPoolCreationEnabled), []byte("true"), chainAB) s.Require().NoError(err) + // Change the parameter to disable protorev + err = chainABNode.ParamChangeProposal("protorev", string(protorevtypes.ParamStoreKeyEnableModule), []byte("false"), chainAB) + s.Require().NoError(err) + // Confirm that the parameter has been changed. isPermisionlessCreationEnabledStr = chainABNode.QueryParams(cltypes.ModuleName, string(cltypes.KeyIsPermisionlessPoolCreationEnabled)) if !strings.EqualFold(isPermisionlessCreationEnabledStr, "true") { From f8b018d657bf0277ab832d0440460cd9a28e5f21 Mon Sep 17 00:00:00 2001 From: David Terpay Date: Wed, 9 Aug 2023 11:14:37 -0400 Subject: [PATCH 7/8] protorev test update --- tests/e2e/configurer/chain/commands.go | 8 ++++++++ tests/e2e/e2e_test.go | 12 ++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/e2e/configurer/chain/commands.go b/tests/e2e/configurer/chain/commands.go index 1bc862fe8c5..9b72c730973 100644 --- a/tests/e2e/configurer/chain/commands.go +++ b/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"} @@ -770,6 +777,7 @@ func (n *NodeConfig) ParamChangeProposal(subspace, key string, value []byte, cha Deposit: "625000000uosmo", } proposalJson, err := json.Marshal(proposal) + fmt.Println("marshalling output", proposalJson, err) if err != nil { return err } diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 3c0e9292422..867f0214fc2 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -327,10 +327,15 @@ func (s *IntegrationTestSuite) ConcentratedLiquidity() { err = chainABNode.ParamChangeProposal("concentratedliquidity", string(cltypes.KeyIsPermisionlessPoolCreationEnabled), []byte("true"), chainAB) s.Require().NoError(err) - // Change the parameter to disable protorev - err = chainABNode.ParamChangeProposal("protorev", string(protorevtypes.ParamStoreKeyEnableModule), []byte("false"), chainAB) + // 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") { @@ -874,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() { From b2c792e82f892afe0a44308ffa41eea4d7637a6b Mon Sep 17 00:00:00 2001 From: Adam Tucker Date: Wed, 9 Aug 2023 21:42:59 -0500 Subject: [PATCH 8/8] Update tests/e2e/configurer/chain/commands.go --- tests/e2e/configurer/chain/commands.go | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/e2e/configurer/chain/commands.go b/tests/e2e/configurer/chain/commands.go index 9b72c730973..52385d7d43f 100644 --- a/tests/e2e/configurer/chain/commands.go +++ b/tests/e2e/configurer/chain/commands.go @@ -777,7 +777,6 @@ func (n *NodeConfig) ParamChangeProposal(subspace, key string, value []byte, cha Deposit: "625000000uosmo", } proposalJson, err := json.Marshal(proposal) - fmt.Println("marshalling output", proposalJson, err) if err != nil { return err }