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: li.fi unstable quotes use original rate #8350

Merged
merged 34 commits into from
Dec 13, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Dec 12, 2024

Description

#8326 introduced support for final trade quotes. This brought in two issues, one which was a blatant bug for Li.Fi bridge swaps introduced by 8326 (fixed in #8348), the second one which is inherent to how Li.Fi works:

It is not unusual (though, relatively rare all things considered) to yield what we call an unstable route as part of their quotes list. This is a route which is almost guaranteed to never be gotten the second time around when firing a /routes call. I've seen this happen very rarely for main Li.Fi tools (1inch, ZRX...), but if you're unlucky to select one which
is an almost guaranteed fail (e.g Enso), then you're out of luck and the swapper will blank out, since we are unable to fetch the same route the second time around.

This is fixed by returning the original rate as originalRate as part of TradeRate, and passing it as TradeQuoteInput at quote time, so we can fallback to it in case we hit this unlucky scenario.

Since I was having some lil' issues with types while working on that, did a little cleanup of types whilst in the house to better reflect the new TradeQuote/TradeRate flow following #8309

Issue (if applicable)

closes #8347

Risk

High Risk PRs Require 2 approvals

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

Low - do a little sanity check of Li.Fi, and other swappers while at it. This change should be constrained to Li.Fi, though there are some small notable runtime changes to accommodate for the improved types, which will be commented out inline in this PR.

Testing

  • NOTE: You'll need to use a VPN to circumvent rate limits as you will absolutely hit it many times during testing, and will need to select a new server many times. e.g when devv'ing and testing this, I was out 7 servers with rate-limit exhausted.
  • Refer to Blank screen attempting to execute unstable tool on Li.Fi #8347 for context to get an understanding of what constitutes an unstable quote and how to get there
  • Try to go from trade input to final trade quote for Li.Fi, over and over, selecting different tools, possibly identifying which is the unstable one as the delta of previous -> next quotes (not required, this is quite hard to actually test and is extremely non-deterministic)
  • Ensure that Li.Fi never blanks out at final quote time

Engineering

  • Revert the revert of the monkey-patch i.e ada0243. This is crucial if you want to test this while not losing your sanity.
  • The other way (not recommanded, the monkey-patch simulates the exact same) is to test Li.Fi rate to final quote flow over and over while getting new quotes everytime, identifying which is the unstable one as the delta of previous -> next quotes, or to try over and over until you get an Enso quote.

Operations

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

  • ☝️

Screenshots (if applicable)

  • Before (monkey-patch diff on top of develop)

https://jam.dev/c/77584de8-9c5e-4153-9965-91a385d33577

  • After (monkey-patch commit reverted on top of this diff)

https://jam.dev/c/84d024e5-d439-4803-8021-7fe38ad642de

@gomesalexandre gomesalexandre requested a review from a team as a code owner December 12, 2024 09:18
@gomesalexandre gomesalexandre changed the title fix: li.fi unstable quotes use stale rate fix: li.fi unstable quotes use original rate Dec 12, 2024
Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

https://jam.dev/c/1bd57a91-3a1f-49eb-9735-007c8d263cc3

By having the monkey patch set up, I'm constantly having the blank screen even with your changes, I couldn't make it work with the monkey patch applied

@gomesalexandre
Copy link
Contributor Author

gomesalexandre commented Dec 12, 2024

jam.dev/c/1bd57a91-3a1f-49eb-9735-007c8d263cc3

By having the monkey patch set up, I'm constantly having the blank screen even with your changes, I couldn't make it work with the monkey patch applied

@NeOMakinG this is a bridge and is expected to be borked for the time being until release is merged back into develop - as mentioned in the description, this is only one out of two fixes, #8348 (merged) fixes bridges at quote time, but is not in develop, yet, since 8348 was targeted to release.

Can you retest with regular Li.Fi swaps? 🙏🏽 just retested now with the monkey patch applied FYI https://jam.dev/c/da942d46-6958-4f7d-ab6c-121a20a59f2a

@NeOMakinG
Copy link
Collaborator

#8348

Ah damn, I was sure this branch was aiming the bridge fix, will give it another try asap

Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

Li.FI

https://jam.dev/c/edeaa67f-e022-451d-9d66-8784723fb63c

Other swappers quotes

https://jam.dev/c/4dc76ac5-8b3e-48ff-bd79-0882a16a17cd

Thorchain routes are not testable right now because of the upgrade, but looks good overall!

Base automatically changed from feat_quoteOrRate_discriminator to develop December 13, 2024 03:05
@gomesalexandre gomesalexandre merged commit 804c821 into develop Dec 13, 2024
3 checks passed
@gomesalexandre gomesalexandre deleted the fix_lifi_final_quotes branch December 13, 2024 03:41
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.

Blank screen attempting to execute unstable tool on Li.Fi
2 participants