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

feat: lifi get final quote #8326

Merged
merged 3 commits into from
Dec 11, 2024
Merged

feat: lifi get final quote #8326

merged 3 commits into from
Dec 11, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Dec 10, 2024

Description

This PR removes the edge-case for Li.Fi, ensuring that like other swappers, gets a final trade quote with the most up-do-date data (rate/fees).
This improves things twofolds:

  • First, for tokens with approval required, this ensures that users get the correct gas fee after approval has been granted (which we weren't updating before, since we were never getting a final trade quote)
  • This also ensures that users are notified in case of rate updated

Issue (if applicable)

Risk

High Risk PRs Require 2 approvals

Medium - if we get this wrong, Li.Fi final trade quote time would show an empty broken state, see testing steps below

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

⚠️ Before beginning testing, you have to try and get a route which you notice is unstable between quotes i.e:

  • Try and get a quote for a given amount
  • Add $0.01, then back down to reforce quotes refetch
  • Notice a given Li.Fi quote will now be missing while the two others are present. That's your unstable quotes.

From my testing, 1 FOX.ARB -> ETH.ARB seems to be a good contender, as ZRX/Li.Fi and Enso Li.Fi routes will be present in some quotes list, but not consistently:

Screenshot 2024-12-10 at 17 06 37 Screenshot 2024-12-10 at 17 07 56
  • Once you found your unstable Li.Fi quote, go all the way to final trade quote and ensure you don't end up in this state:
Screenshot 2024-12-10 at 17 08 09

Note, you'll have to try to back out to input, then back to final quotes many many times (while having the quote you have spotted as unstable selected) over to ensure we consistently get said route and don't end up with the empty quote bug above

Engineering

  • Ensure we pass the allowed tool in Li.Fi final trade quote req as exchanges: { allow: [tool] }

Operations

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)
  • ☝🏽

Screenshots (if applicable)

  • Li.Fi swapper consistently gets final trade quote without early return null with no quote bug

https://jam.dev/c/fc162032-bbff-4c89-927c-259c79686071

  • Li.Fi network fees are accurate on final quote after allowance has been granted

https://jam.dev/c/1e1ea531-5961-49b3-b53f-4b8c54eefe09

@gomesalexandre gomesalexandre marked this pull request as ready for review December 10, 2024 10:07
@gomesalexandre gomesalexandre requested a review from a team as a code owner December 10, 2024 10:07
Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

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

Testing notes

✅ Unable to get into state described - working even with unstable quote

⚠️ Unable to actually do the trade due to quite severe rate limit ing from lifi. Its unclear whether this is related to the changes here, as if caching or something else is affected then possibly it is?

https://jam.dev/c/b529a925-920c-44c7-b50e-f6bbd4bf453f

Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

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

Ok, retested after not spamming the UI and trades do in fact go through fine

https://jam.dev/c/ad447ac2-8891-4969-9565-dba0f609b1db

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lifi gas estimates are incorrect at quote time
2 participants