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: thorchain swap improvements #5748

Merged
merged 11 commits into from
Dec 4, 2023
Merged

Conversation

kaladinlight
Copy link
Contributor

@kaladinlight kaladinlight commented Dec 1, 2023

Description

  • Use 0 limit for thorchain streaming swaps which will use TC auto stream limits which should result in more completed trades and less refunds (see: https://discord.com/channels/838986635756044328/1166265575941619742/1166500062101250100)
    • Because we are using auto limits, don't show Min expected after slippage as we can't determine this accurately
    • TODO: disable custom slippage limit for streaming swaps in the UI (no being used, so want to make sure the user doesn't think they are using something they aren't) @reallybeard let me know if you have a good idea on how to handle this or if we even need to address specifically since we aren't showing the slippage details in the summary
  • Calculate trade slippage on expected_amount_out and don't account for affiliate fees in these calculations and display values as they are shown elsewhere (this very closely reflects TS UI as changed)

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)

closes #5733

Risk

Medium - this changes our limit logic on both regular and streaming swaps, we will want to do our best to verify completed trades and also monitor after this update is deployed to ensure we have reduced the occurrences of refunds for all thor trades

Testing

  • Ensure regular TC trades succeed and validate trade values against TS for sanity check (should see limit set in the memo)
  • Ensure streaming TC trades succeed and validate trade values against TS for sanity check (should see 0/10/0 streaming params in the memo, the first zero being limit = auto limit)

Engineering

☝️

Operations

☝️

Screenshots (if applicable)

No affiliate fee

SS TS
image image
image image
"=:ETH.DAI-0X6B175474E89094C44DA98B954EEDEAC495271D0F:0x7ae920bef29c849415635b1b97f1ceb33dca89b6:0/10/0:ss:30"
"=:ETH.DAI:0x7ae920bEf29c849415635B1b97f1CeB33dca89B6:0/3/6:t:30"
"=:ETH.DAI-0X6B175474E89094C44DA98B954EEDEAC495271D0F:0x7ae920bef29c849415635b1b97f1ceb33dca89b6:116914475388:ss:30"
"=:ETH.DAI:0x7ae920bEf29c849415635B1b97f1CeB33dca89B6:117015146861:t:30"

@kaladinlight kaladinlight requested a review from a team as a code owner December 1, 2023 18:52
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.

Conceptual stamp, haven't tested this at runtime yet but this is exactly what I thought the PR would look like 🏆 looks sane to me and on par with THORSwap for streaming swaps

Copy link
Member

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Functional test, memo looking as expected for streaming swaps:

0XC770EEFAD204B5180DF6A14EE197D99D808EE52D:0xB0E5C1DD13D78a314c3c0d82CE2C9f4c65C3e02c:0/10/0:ss:30.

@0xApotheosis
Copy link
Member

@kaladinlight I'll hold off on merging, as I'm not sure is you wanted to address the "TODO: disable custom slippage limit" in this PR, or a follow-up.

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.

Sanity tested with frame-injected vitalik.eth against THORSwap:

  • Receive amounts look sane against THORSwap
Screenshot 2023-12-04 at 09 56 59 Screenshot 2023-12-04 at 09 56 59
  • Memo looks sane against TS:
Screenshot 2023-12-04 at 09 57 55 Screenshot 2023-12-04 at 09 58 46
  • Limit is indeed present for streaming swaps
  • We use 0/10/0 as streaming parameters as mentioned in the testing steps. i.e:
    • 0 is on par with the explicit quantity from TS (0/explicit quantity from quote is the same), and
    • 10 is valid as per our current heuristics, possibly too high, but our 1/5/10 interval heuristics won't hurt, only make swaps possibly longer vs. the 3 used by TS for a similar a swap

@kaladinlight kaladinlight merged commit ef814e4 into develop Dec 4, 2023
3 checks passed
@kaladinlight kaladinlight deleted the tc-streaming-auto-limit branch December 4, 2023 18:20
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.

THORSwap trades refunded at "Emit asset is less than price limit"
4 participants