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.

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 Out Of Gas #5840

Closed
NotJeremyLiu opened this issue Jul 14, 2023 · 0 comments · Fixed by #5841
Closed

Protorev Out Of Gas #5840

NotJeremyLiu opened this issue Jul 14, 2023 · 0 comments · Fixed by #5841
Assignees
Labels
protorev All things related to x/protorev T:bug 🐛 Something isn't working

Comments

@NotJeremyLiu
Copy link
Contributor

NotJeremyLiu commented Jul 14, 2023

Background

  • With the new hook based approach, Protorev now stores swaps to backrun in state when triggered by GAMM and Concentrated Liquidity pool after swap hooks:
  • func (h Hooks) AfterCFMMSwap(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, input sdk.Coins, output sdk.Coins) {
  • func (h Hooks) AfterConcentratedPoolSwap(ctx sdk.Context, sender sdk.AccAddress, poolId uint64, input sdk.Coins, output sdk.Coins) {
  • Then, in the posthandler, ProtoRev deletes these stored swaps from state, regardless of the outcome of the rebalancing logic:
  • protoRevDec.ProtoRevKeeper.DeleteSwapsToBackrun(ctx.WithGasMeter(upperGasLimitMeter))
  • The issue is that the upperGasLimitMeter used to delete the swaps to backrun from store on the real ctx is the same upperGasLimitMeter used in the cachectx to perform the rebalancing logic. So, if within the rebalancing logic the upperGasLimitMeter is depleted, the call to DeleteSwapsToBackrun will fail with an out of gas error, resulting in a failed transaction for the user.

Suggested Design

  • Use an sdk.NewInfiniteGasMeter in the DeleteSwapsToBackrun call so that it never has the chance to run out of gas and fail the user's transaction. Using an infinite gas meter here should be safe as there is no looping logic within the DeleteSwapsToBackrun and the most number of swaps that is allowed to be stored in state in this kv store would be # of swaps the user took + # of swaps protorev performed to backrun.
  • Alternatively, instead of using an infinite gas meter, we could use a new large gas meter (if using an infinite gas meter is unacceptable)

Acceptance Criteria

  • Add and pass a test that depletes the gas meter used in the cachectx where protorev performs the rebalancing logic, while still successfully deleting the swaps from the kvstores and not failing the user's transaction.
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Osmosis Chain Development Jul 14, 2023
@NotJeremyLiu NotJeremyLiu added protorev All things related to x/protorev T:bug 🐛 Something isn't working labels Jul 14, 2023
@NotJeremyLiu NotJeremyLiu self-assigned this Jul 14, 2023
@stackman27 stackman27 self-assigned this Jul 14, 2023
@github-project-automation github-project-automation bot moved this from Needs Triage 🔍 to Done ✅ in Osmosis Chain Development Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protorev All things related to x/protorev T:bug 🐛 Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants