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

Mark interaction as internalizable instead of dropping it #931

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

MartinquaXD
Copy link
Contributor

The optimization pipeline which should improve the objective value of a settlement by applying a few gas optimizations can cause solutions that are not viable to pass the simulation by internalizing AMM interactions that are overly optimistic.

Check out this trade selling USDC for AMPL. Paraswap currently gives too optimistic prices for AMPL. That means the settlement provided by paraswap would likely have reverted. Because the order was selling USDC (a trusted token) the optimization pipeline was allowed to internalize the erroneous AMM interaction which resulted in a working settlement with an unbeatable price (because we pay with our buffers).

I saw 2 ways to tackle this issue:

  1. Don't apply optimizations if the original settlement would already revert
  2. Instead of right out dropping the interaction we keep the original interaction but mark it as internalizable

I decided to go with 2 because together with #843 it should prevent the same problems while offering better debuggability than 1.

Test Plan

?

@MartinquaXD MartinquaXD requested a review from a team as a code owner December 8, 2022 15:16
@MartinquaXD MartinquaXD enabled auto-merge (squash) December 9, 2022 06:51
@MartinquaXD MartinquaXD merged commit 019a1d1 into main Dec 9, 2022
@MartinquaXD MartinquaXD deleted the prevent-optimistic-internalization branch December 9, 2022 06:52
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants