-
Notifications
You must be signed in to change notification settings - Fork 607
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.
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
Conversation
x/concentrated-liquidity/position.go
Outdated
@@ -391,6 +391,10 @@ func (k Keeper) deletePosition(ctx sdk.Context, | |||
return nil | |||
} | |||
|
|||
func (k Keeper) CreatePosition(ctx sdk.Context, poolId uint64, owner sdk.AccAddress, tokensProvided sdk.Coins, amount0Min, amount1Min sdk.Int, lowerTick, upperTick int64) (positionId uint64, actualAmount0 sdk.Int, actualAmount1 sdk.Int, liquidityDelta sdk.Dec, lowerTickResult int64, upperTickResult int64, err error) { |
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.
@stackman27 @p0mvn For ProtoRev testing package to be able to create positions, I had to create a new exported CreatePosition
method, previously a CreatePosition
testing helper method was created in export_test.go but protorev doesn't have access to that method.
If there's a better way to get access to this ability I'm open to changing this to what is desired from y'alls end.
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.
If we are doing this I think it just makes sense to export the original createPosition. This will end up being confusing later on down the road imo
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.
@czarcas7ic Sounds good, I'll create a separate PR that just exports it and changes where it's being called from, then can rebase this PR on top of that once merged
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.
PR up: #5893, will ping in slack once all CI finishes and r4r
x/protorev/keeper/posthandler.go
Outdated
// 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 an infinite gas meter. | ||
protoRevDec.ProtoRevKeeper.DeleteSwapsToBackrun(ctx.WithGasMeter(sdk.NewInfiniteGasMeter())) |
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.
we should definitely bound this, maybe increase by x2 or x4 or anything else
i did some gas estimate check using our SwapLogic: https://github.com/osmosis-labs/osmosis/pull/5850/files#diff-1a552fcc0a3d36556402345e840b658a7daccb101d7f73bca9ee2e3057ade2e6R3451
and here's the output. While Protorev route building, more than 2000 ticks was crossed
tickSpacing = 1
swapOutGivenIn direction
TickCrossed = 1754 (from -20 to 1755)
positions = 200 tick wide positions, ranging from [-5000 to 5000]
NewGasCost = 22239116, OldGasCost = 22255556, DIFF = 16440
swapInGivenOut direction
TickCrossed = 998 (from 1755 to 736)
positions = 200 tick wide positions, ranging from [-5000 to 5000] tick
NewGasCost = 22358879, OldGasCost = 22383527, DIFF = 24648
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.
Using an infinite gas meter here does not mean we are having an unbounded gas meter for the rebalancing logic itself, that (at this point) is still 50 mil gas. This change is to use an infinite gas meter at the point of deleting swaps to backrun from the kvstore such that there will never be the ability for an out of gas error during deletion.
We should not use the same gasmeter for both rebalancing and deleting, we could use an arbitrarily high gasmeter here instead if that's desired.
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.
Put in another way, this change has no impact on the amount of ticks that can be crossed or the desired bounding of protorev computation itself for rebalancing.
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.
This commit changes it to use a new 50mil gas meter: 5789614
So now this can still fail a user's tx if for some reason deleting swaps from storage takes over 50mil gas (should never happen, but using any number that's not infinite gas meter runs into this issue)
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 see its the pointer reference error where "upperGasLimitMeter" turns into 0 while DeleteSwapsToBackrun
gets called? Therefore with current logic during computeSwap if Gas > 50M it will fail the swap not error, and same while DeletingSwapsToBackrun?
i think it will be very helpful if we add this comment too #5841 (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.
@stackman27 Correct, this PR is purely to fix the shared reference usage. With that understanding, would you still prefer this to be a bounded gas limit? If so, I will change it to something smaller (10mil gas) just so that 50mil isn't used twice
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.
yea i dont think deleting is too expensive, we can just set it to 50M and add inline comment on why we did that
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.
comment added: b06bdb4
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.
This LGTM! 🥐
@@ -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) |
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.
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. )
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 to this tracking issue: #5858
- Also renames test/3 to ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7 to match mainnet denoms for first cl pool
077a371
to
246a4cb
Compare
Spoke offline, going to merge now, but @NotJeremyLiu will test in localosmosis at some point today/tomorrow. @NotJeremyLiu if you could post on this merged PR once you have tested locally and tag me in it it would be greatly appreciated. |
@czarcas7ic Just verified that this change works as expected in LocalOsmosis, preventing the transaction from failing even when the cacheCtx gas meter is entirely consumed. To reproduce: The process to test was:
|
…5841) * Use infinite gas meter to delete swaps from state * Add CL pool with many ticks test to run out of gas - Also renames test/3 to ibc/0CD3A0285E1341859B5E86B6AB7682F023D03E97607CCC1DC95706411D866DF7 to match mainnet denoms for first cl pool * Add changelog entry * fix comment * Use a new 50mil gas meter instead of infinite * add clarifying comment * make function name more specific * remove no longer needed export
Closes: #5840
What is the purpose of the change
DeleteSwapsToBackrun
call to use a new 50 mil gas meter instead of a gas meter that is shared with the rest of the ProtoRev's rebalance logic.cacheCtx
.Testing and Verifying
cacheCtx
while rebalancing, but does not cause the user's transaction to run out of gas (whereas it error when using the previousupperGasMeterLimit
, you can substitute back inupperGasMeterLimit
to see the panic before the change).Documentation and Release Note
Where is the change documented?