Skip to content
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.

Sign up for GitHub

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

Protorev: Use new gas meter to delete swaps from state #5841

Merged
merged 8 commits into from
Jul 26, 2023
Merged

Protorev: Use new gas meter to delete swaps from state #5841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8b4a530
Use infinite gas meter to delete swaps from state
NotJeremyLiu Jul 14, 2023
9d27aaf
Add CL pool with many ticks test to run out of gas
NotJeremyLiu Jul 17, 2023
5ecbce7
Add changelog entry
NotJeremyLiu Jul 17, 2023
8edb147
fix comment
NotJeremyLiu Jul 17, 2023
e6e25c5
Use a new 50mil gas meter instead of infinite
NotJeremyLiu Jul 17, 2023
0543204
add clarifying comment
NotJeremyLiu Jul 18, 2023
246a4cb
make function name more specific
NotJeremyLiu Jul 18, 2023
c76b1a6
remove no longer needed export
NotJeremyLiu Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,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.

### Misc Improvements

Expand Down
61 changes: 54 additions & 7 deletions x/protorev/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (s *KeeperTestSuite) SetupTest() {
StepSize: sdk.NewInt(1_000_000),
},
{
Denom: "test/3",
Denom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
StepSize: sdk.NewInt(1_000_000),
},
}
Expand Down Expand Up @@ -130,7 +130,7 @@ func (s *KeeperTestSuite) SetupTest() {
sdk.NewCoin("ibc/A0CC0CF735BFB30E730C70019D4218A1244FF383503FF7579C9201AB93CA9293", sdk.NewInt(9000000000000000000)),
sdk.NewCoin("test/1", sdk.NewInt(9000000000000000000)),
sdk.NewCoin("test/2", sdk.NewInt(9000000000000000000)),
sdk.NewCoin("test/3", sdk.NewInt(9000000000000000000)),
sdk.NewCoin("ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", sdk.NewInt(9000000000000000000)),
sdk.NewCoin("usdx", sdk.NewInt(9000000000000000000)),
sdk.NewCoin("usdy", sdk.NewInt(9000000000000000000)),
sdk.NewCoin("epochOne", sdk.NewInt(9000000000000000000)),
Expand Down Expand Up @@ -755,7 +755,7 @@ func (s *KeeperTestSuite) setUpPools() {
Weight: sdk.NewInt(1),
},
{
Token: sdk.NewCoin("test/3", sdk.NewInt(4478366578)),
Token: sdk.NewCoin("ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", sdk.NewInt(4478366578)),
Weight: sdk.NewInt(1),
},
},
Expand All @@ -766,7 +766,7 @@ func (s *KeeperTestSuite) setUpPools() {
{ // Pool 39
PoolAssets: []balancer.PoolAsset{
{
Token: sdk.NewCoin("test/3", sdk.NewInt(18631000485558)),
Token: sdk.NewCoin("ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", sdk.NewInt(18631000485558)),
Weight: sdk.NewInt(1),
},
{
Expand Down Expand Up @@ -901,6 +901,53 @@ func (s *KeeperTestSuite) setUpPools() {
s.Require().NoError(err)
}

func (s *KeeperTestSuite) CreateCLPoolAndArbRouteWith_28000_Ticks() {
// Create the CL pool
clPool := s.PrepareCustomConcentratedPool(s.TestAccs[2], "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", "uosmo", 100, sdk.NewDecWithPrec(2, 3))
fundCoins := sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(1000000000000000000)), sdk.NewCoin("ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", sdk.NewInt(1000000000000000000)))
s.FundAcc(s.TestAccs[2], fundCoins)

// Create 28000 ticks in the CL pool, 14000 on each side
tokensProvided := sdk.NewCoins(sdk.NewCoin("uosmo", sdk.NewInt(100000)), sdk.NewCoin("ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", sdk.NewInt(100000)))
amount0Min := sdk.NewInt(0)
amount1Min := sdk.NewInt(0)
lowerTick := int64(0)
upperTick := int64(100)

for i := int64(0); i < 14000; i++ {
s.App.ConcentratedLiquidityKeeper.CreatePosition(s.Ctx, clPool.GetId(), s.TestAccs[2], tokensProvided, amount0Min, amount1Min, lowerTick-(100*i), upperTick-(100*i))
s.App.ConcentratedLiquidityKeeper.CreatePosition(s.Ctx, clPool.GetId(), s.TestAccs[2], tokensProvided, amount0Min, amount1Min, lowerTick+(100*i), upperTick+(100*i))
}

// Set 2-pool hot route between new CL pool and respective Balancer
s.App.ProtoRevKeeper.SetTokenPairArbRoutes(
s.Ctx,
"uosmo",
"ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
types.NewTokenPairArbRoutes(
[]types.Route{
{
Trades: []types.Trade{
{
Pool: 38,
TokenIn: "uosmo",
TokenOut: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
},
{
Pool: 0,
TokenIn: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
TokenOut: "uosmo",
},
},
StepSize: sdk.NewInt(100000),
},
},
"uosmo",
"ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
),
)
}

// createStableswapPool creates a stableswap pool with the given pool assets and params
func (s *KeeperTestSuite) createStableswapPool(initialLiquidity sdk.Coins, poolParams stableswap.PoolParams, scalingFactors []uint64) uint64 {
poolId, err := s.App.PoolManagerKeeper.CreatePool(
Expand Down Expand Up @@ -965,8 +1012,8 @@ func (s *KeeperTestSuite) setUpTokenPairRoutes() {
fourPool3 := types.NewTrade(0, "test/2", "Atom")

// Two-Pool Route
twoPool0 := types.NewTrade(0, "test/3", types.OsmosisDenomination)
twoPool1 := types.NewTrade(39, types.OsmosisDenomination, "test/3")
twoPool0 := types.NewTrade(0, "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", types.OsmosisDenomination)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's open up an issue to refactor the Posthandler logic slightly, so that we can directly test that the user's tx doesn't revert even when the gas meter runs out.

This test is basically assuming that's going to happen by initializing a pool with a ton of ticks and very low liquidity, such that Protorev tries to swap across it and runs out. But in theory, the gas optimization could get good enough or ProtoRev could be given more gas such that the test stops running out of gas, and we wouldn't know it's not doing the thing we think it is anymore.

I think this requires a slight refactor of the posthandler code that would allow initializing and passing in a zero gas meter, so we'd know for sure it would run out.

One way to do this would be to create a higher-order function that takes in a gas amount as a parameter returns a Posthandler that always initializes a gas meter with that amount.

A simpler way would be to create a helper method that the posthandler calls to wrap the logic for creating a gas meter and doing the simulations, then we could test that function directly.

( I'm not attached to any one approach at all. )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to this tracking issue: #5858

twoPool1 := types.NewTrade(39, types.OsmosisDenomination, "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7")

// Doomsday Route - Stableswap
doomsdayStable0 := types.NewTrade(29, types.OsmosisDenomination, "usdc")
Expand Down Expand Up @@ -1018,7 +1065,7 @@ func (s *KeeperTestSuite) setUpTokenPairRoutes() {
},
{
TokenIn: types.OsmosisDenomination,
TokenOut: "test/3",
TokenOut: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
ArbRoutes: []types.Route{
{
StepSize: standardStepSize,
Expand Down
5 changes: 3 additions & 2 deletions x/protorev/keeper/posthandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,9 @@ func (protoRevDec ProtoRevDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simu
}

// Delete swaps to backrun for next transaction without consuming gas
// from the current transaction's gas meter, but instead from a new gas meter
protoRevDec.ProtoRevKeeper.DeleteSwapsToBackrun(ctx.WithGasMeter(upperGasLimitMeter))
// from the current transaction's gas meter, but instead from a new gas meter with 50mil gas.
// 50 mil gas was chosen as an arbitrary large number to ensure deletion does not run out of gas.
protoRevDec.ProtoRevKeeper.DeleteSwapsToBackrun(ctx.WithGasMeter(sdk.NewGasMeter(sdk.Gas(50_000_000))))

return next(ctx, tx, simulate)
}
Expand Down
43 changes: 38 additions & 5 deletions x/protorev/keeper/posthandler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ func (s *KeeperTestSuite) TestAnteHandle() {
trades: []types.Trade{
{
Pool: 38,
TokenOut: "test/3",
TokenOut: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
TokenIn: types.OsmosisDenomination,
},
},
Expand All @@ -236,7 +236,7 @@ func (s *KeeperTestSuite) TestAnteHandle() {
Amount: sdk.NewInt(15_767_231),
},
{
Denom: "test/3",
Denom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
Amount: sdk.NewInt(218_149_058),
},
{
Expand Down Expand Up @@ -265,7 +265,7 @@ func (s *KeeperTestSuite) TestAnteHandle() {
Amount: sdk.NewInt(15_767_231),
},
{
Denom: "test/3",
Denom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
Amount: sdk.NewInt(218_149_058),
},
{
Expand Down Expand Up @@ -294,7 +294,7 @@ func (s *KeeperTestSuite) TestAnteHandle() {
Amount: sdk.NewInt(15_767_231),
},
{
Denom: "test/3",
Denom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
Amount: sdk.NewInt(218_149_058),
},
{
Expand Down Expand Up @@ -323,7 +323,7 @@ func (s *KeeperTestSuite) TestAnteHandle() {
Amount: sdk.NewInt(15_767_231),
},
{
Denom: "test/3",
Denom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
Amount: sdk.NewInt(218_149_058),
},
{
Expand All @@ -335,6 +335,35 @@ func (s *KeeperTestSuite) TestAnteHandle() {
},
expectPass: true,
},
{
name: "Concentrated Liquidity - Many Ticks Run Out of Gas While Rebalacing, Ensure Doesn't Fail User Tx",
params: param{
trades: []types.Trade{
{
Pool: 50,
TokenOut: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
TokenIn: "uosmo",
},
},
expectedNumOfTrades: sdk.NewInt(5),
expectedProfits: []sdk.Coin{
{
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),
},
},
expectedPoolPoints: 37,
},
expectPass: true,
},
}

// Ensure that the max points per tx is enough for the test suite
Expand Down Expand Up @@ -405,6 +434,10 @@ func (s *KeeperTestSuite) TestAnteHandle() {
tx = s.BuildTx(txBuilder, msgs, sigV2, "", txFee, gasLimit)
}

if strings.Contains(tc.name, "Concentrated Liquidity") {
s.CreateCLPoolAndArbRouteWith_28000_Ticks()
}

protoRevDecorator := keeper.NewProtoRevDecorator(*s.App.ProtoRevKeeper)
posthandlerProtoRev := sdk.ChainAnteDecorators(protoRevDecorator)

Expand Down
2 changes: 1 addition & 1 deletion x/protorev/keeper/protorev_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (s *KeeperTestSuite) TestGetAllBaseDenoms() {
s.Require().Equal(3, len(baseDenoms))
s.Require().Equal(baseDenoms[0].Denom, types.OsmosisDenomination)
s.Require().Equal(baseDenoms[1].Denom, "Atom")
s.Require().Equal(baseDenoms[2].Denom, "test/3")
s.Require().Equal(baseDenoms[2].Denom, "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7")

// Should be able to delete all base denoms
s.App.ProtoRevKeeper.DeleteBaseDenoms(s.Ctx)
Expand Down
10 changes: 5 additions & 5 deletions x/protorev/keeper/rebalance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ var twoPoolRoute = poolmanagertypes.SwapAmountInRoutes{
},
poolmanagertypes.SwapAmountInRoute{
PoolId: 39,
TokenOutDenom: "test/3",
TokenOutDenom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
},
}

Expand Down Expand Up @@ -388,10 +388,10 @@ func (s *KeeperTestSuite) TestExecuteTrade() {
name: "2-Pool Route Arb",
param: param{
route: twoPoolRoute,
inputCoin: sdk.NewCoin("test/3", sdk.NewInt(989_000_000)),
inputCoin: sdk.NewCoin("ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", sdk.NewInt(989_000_000)),
expectedProfit: sdk.NewInt(218_149_058),
},
arbDenom: "test/3",
arbDenom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
expectPass: true,
expectedNumOfTrades: sdk.NewInt(3),
},
Expand Down Expand Up @@ -517,9 +517,9 @@ func (s *KeeperTestSuite) TestIterateRoutes() {
params: paramm{
routes: []poolmanagertypes.SwapAmountInRoutes{twoPoolRoute},
expectedMaxProfitAmount: sdk.NewInt(198_653_535),
expectedMaxProfitInputCoin: sdk.NewCoin("test/3", sdk.NewInt(989_000_000)),
expectedMaxProfitInputCoin: sdk.NewCoin("ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7", sdk.NewInt(989_000_000)),
expectedOptimalRoute: twoPoolRoute,
arbDenom: "test/3",
arbDenom: "ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7",
},
expectPass: true,
},
Expand Down