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: limit orders quote wiring #8095

Merged
merged 9 commits into from
Nov 12, 2024

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Nov 7, 2024

Description

Wires up the limit order quote query to the input and config UIs to display the result of the quote in the UI.

Includes:

  • Wiring up estimated affiliate fee and fee modal
  • Wiring up custom slippage
  • Wiring up quoted market price to UI
  • Resets limit orders config when assets change (i.e resets the rate, expiry and % input - not the input UI). Config is not reset when amounts or quote changes to avoid annoying users during polling (polling yet to be implemented)
  • Adds loading state to limit orders input, config, and receive summary

Excludes the following which are coming in follow-ups:

Issue (if applicable)

closes #8033

Risk

High Risk PRs Require 2 approvals

Majority of the change is behind a feature flag and is low risk, but as this refactors trade UI (ConfirmSummary and SlippagePopover) this technically has to be marked as high risk.

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

  • Trade quotes for all protocols
  • Trade slippage input for all protocols

Testing

Testing should be focused on regressions for trades:

  • Custom slippage input is correctly applied to trades
  • Summary of fees in ConfirmSummary is correct

Engineering

Operations

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

Screenshots (if applicable)

https://jam.dev/c/18e12936-6779-4a97-a2f7-181406ab0098

@woodenfurniture woodenfurniture changed the base branch from develop to limit-orders-receive-address November 7, 2024 01:08
@woodenfurniture woodenfurniture force-pushed the limit-orders-receive-address branch 2 times, most recently from f6a539f to 1a874c1 Compare November 7, 2024 04:07
Base automatically changed from limit-orders-receive-address to develop November 8, 2024 01:45
@woodenfurniture woodenfurniture marked this pull request as ready for review November 10, 2024 22:47
@woodenfurniture woodenfurniture requested a review from a team as a code owner November 10, 2024 22:47
@0xean
Copy link
Contributor

0xean commented Nov 11, 2024

blame woody if those go haywire - but only 1 review needed here :) 💀

@0xean 0xean removed the high risk label Nov 11, 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.

Code point of view looks very sane, big tree @woodenfurniture 🐐

Testing at runtime: https://jam.dev/c/85d588ef-05e2-43a3-a714-04134c0719ba

A few things:

  • 0$ is supposed to say Free as a green field as regular swapper does
    image

  • When inverting assets I have a super weird behavior:
    image

  • Same as inverting, if you look at https://jam.dev/c/85d588ef-05e2-43a3-a714-04134c0719ba , there is a moment where you have USDC > FOX, with When price reach 16, which should be 0.06 instead, I think it's due to the useEffect that is not rerunning on market price change and then if you click on the percent, it's populating the right ratio

Regular trade

image
image
image

Seems ok so far

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.

Summary seems like before

Slippage is rightly applied on a LI.FI swap
https://jam.dev/c/cb1cf9d0-4476-4953-9bd0-7fef0beda757

THORChain swap
https://jam.dev/c/e58929e6-6bb3-4e09-84b8-96bc6b284a30
memo is assuming 0.0006ETH as a receive amount vs 0.00067 estimated which is around 9.5 slippage, looking good

As long as it's behind a feature flag, limit order issues are not blocking and can be done in the next PR 👍

@woodenfurniture woodenfurniture enabled auto-merge (squash) November 12, 2024 23:41
@woodenfurniture woodenfurniture merged commit c988a20 into develop Nov 12, 2024
3 checks passed
@woodenfurniture
Copy link
Member Author

Appreciate the stamp, I've fixed both bugs in #8112:

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.

Wire up receive summary UI to limit order RTK response
3 participants