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

fix: add amountOutMin to longtail #6052

Merged
merged 10 commits into from
Jan 23, 2024
Merged

fix: add amountOutMin to longtail #6052

merged 10 commits into from
Jan 23, 2024

Conversation

0xApotheosis
Copy link
Contributor

@0xApotheosis 0xApotheosis commented Jan 22, 2024

Description

Adds a amountOutMin value to the first "step" of a longtail to L1 swap (the call to the swapIn function of the UniswapV3 contract on Ethererum).

This PR also re-enables longtail to L1 swaps. - keeping this off for now. There is an unrelated issue that seems to be showing an incorrect value in the trade confirm step. I'm not yet sure if this is a longtail thing, or a multi-hop wiring thing. I'll investigate and fix in a follow-up PR.

Note, as a follow-up we'll need to align with @shapeshift/product on how to handle slippage on trades with multiple steps.
A longtail to L1 swap has two steps, each with the slippage value encoded. This means that if a user has selected 1% slippage, it's possible both steps could incur that entire slippage amount, resulting in a total slippage of 1.98% for the trade.

An example TX performed on this branch:

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

Contributes to #6017

Risk

High Risk PRs Require 2 approvals

High - an incorrect implementation could leave users open to sandwich attacks.

Testing

Perform a longtail to L1 swap (e.g. CRV on Ethereum to DOGE). Ensure that the specified slippage value is correctly encoded in both the amountOutMin of the Ethereum TX:

Screenshot 2024-01-22 at 3 30 33 pm

and on the associated THORChain TX (in the memo):

Screenshot 2024-01-22 at 3 31 37 pm

Engineering

☝️

Operations

☝️

Screenshots (if applicable)

Screenshot 2024-01-22 at 1 18 23 pm Screenshot 2024-01-22 at 1 18 48 pm Screenshot 2024-01-22 at 1 40 45 pm Screenshot 2024-01-22 at 1 40 54 pm Screenshot 2024-01-22 at 1 40 59 pm Screenshot 2024-01-22 at 1 41 12 pm Screenshot 2024-01-22 at 1 43 09 pm

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Stamping as this does what it says from a functional perspective, a few comments that are not related to the functionality of this.

Tested locally:

Quote

Screenshot 2024-01-22 at 10 03 33

Debugged expectedAmountOut and amountOutMin looks sane

Screenshot 2024-01-22 at 10 05 10

On-chain amountOutMin is present and sane

Screenshot 2024-01-22 at 10 06 54

Tx Success detection

Screenshot 2024-01-22 at 10 06 54

@0xApotheosis while this was (most likely not?) introduced by this PR, I noticed a discrepancy in the final amount received that's shown here - a discrepancy from the quote, which itself is very close (although not 100% accurate) to the actual on-chain received amount

Screenshot 2024-01-22 at 10 10 47 Screenshot 2024-01-22 at 10 11 26

i.e:

  • ~0.527 ATOM on our success status step vs. ~0.422 on quote 🚫
  • ~0.422 ATOM on our quote step vs. ~0.421 ATOM on-chain ✅

@0xApotheosis
Copy link
Contributor Author

Thank @gomesalexandre. I think something related to the multi-hop UI is broken here, as that success status step is off (it seems to show the amount before fees).

I'm not sure if it only affects longtail swaps, or all swaps, but I'll update this PR to not turn on the longtail feature flag in production until I work out what's up and PR a fix.

@0xApotheosis 0xApotheosis merged commit 3cd639f into develop Jan 23, 2024
3 checks passed
@0xApotheosis 0xApotheosis deleted the amountOutMin branch January 23, 2024 00:53
@0xApotheosis 0xApotheosis mentioned this pull request Jan 24, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants