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

refactor: quote for LIMIT and TWAP widgets #2544

Merged
merged 56 commits into from
May 29, 2023

Conversation

nenadV91
Copy link
Contributor

@nenadV91 nenadV91 commented May 24, 2023

Summary

Fixes #2416

Refactor quote for advanced orders

  • use TradeQuoteUpdater in TradeWidget
  • add QuoteObserverUpdater to Limit and Twap order widgets to observe changes in quote and do appropriate actions when the quote is changed.
Screenshot 2023-05-25 at 13 12 45

To test

  • Limit and Twap widgets should fetch quote and it should work normally

shoom3301 and others added 30 commits May 3, 2023 22:01
1. Raw state it's what stored into localStorage
2. Full state it's just enriched raw state: token address => Currency
* feat: refactor twap orders code

* feat: some small updates

* fix: pr comments 1

* fix: pr comments 2
…e-updates

feat(twap-orders): twap orders deadline updates (8)
…isplay

feat(twap-orders): add parts display to TWAP orders (7)
feat(twap-orders) add quote fetching loading indicator (6)
feat(twap-orders) Add quote fetching part 1 (5)
feat(twap-orders): update no. of parts error and responsivnes (4)
@nenadV91 nenadV91 changed the base branch from feature/twap-orders-deadline to develop May 25, 2023 11:44
@elena-zh
Copy link
Contributor

Hey @nenadV91 , I did not test the quotes on the TWAP form as I don't know how they should work 'normally'. I have run tests for the Limit orders page.
So:

  1. No error messages for Unsupported token
    unsupported

  2. Besides, the price=1 when an unsupported toke is selected (0 pn Prod)
    1

  3. No warning when no liquidity
    no liq connected

  4. No warning when sell amount is too small (until I refresh the page)
    sell amount is too small
    Once I refresh the page, the warning may appear, but it remains to be displayed when I change a token pair
    image

  5. No Price deviation percentage --> no warnings on the form
    Screenshot_4

  6. when there are 2 warnings on the form, there is no indent between them
    image

@nenadV91
Copy link
Contributor Author

Hey @elena-zh , thank you for review
I've created a separated PR and I think some of those issues should be fixed. Could you please take another look?

nenadV91 and others added 2 commits May 29, 2023 12:54
@nenadV91
Copy link
Contributor Author

@elena-zh
1-3 - should be fixed and confirmed by Elena
4. I think it should be fixed now
5. This also seems to be fixed now
6. I don't think this is related to this PR

Additional

  1. This is probably not merged yet
  2. That seems to goerli and I don't know what USDT is that

@shoom3301 shoom3301 force-pushed the feature/twap-refactor-quote-3 branch from f8dbe4a to 71bf32a Compare May 29, 2023 11:37
@shoom3301
Copy link
Collaborator

Reviewed code and tested the behavior. I see that all Elena's issues are fixed.

@nenadV91 nenadV91 changed the title feat: refactor quote refactor: quote for LIMIT and TWAP widgets May 29, 2023
@nenadV91 nenadV91 merged commit 894aa99 into develop May 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2023
@alfetopito alfetopito deleted the feature/twap-refactor-quote-3 branch May 29, 2023 12:22
@anxolin
Copy link
Contributor

anxolin commented May 29, 2023

Regarding the diagram, right now i think is great, but just i think long term the quote should be in its own module as I suggested in the overview diagram

image

import { useUpdateCurrencyAmount } from 'modules/trade/hooks/useUpdateCurrencyAmount'

export function QuoteObserverUpdater() {
const { state } = useDerivedTradeState()
Copy link
Contributor

Choose a reason for hiding this comment

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

im a bit confused about how this works.

  • I can see QuoteObserverUpdater is a twap updater (in the TWAP module)
  • depends on useDerivedTradeState to get the state --> the buy amount (in the TRADE module)
  • on changes, it will update it, but it will do it using useUpdateCurrencyAmount (in the TRADE module)

My thinking is, the trade module exports the state, and a method to update it, and it expects other modules to watch changes on it and call the update method 🧐
Seems a bit strange dependencies.

Copy link
Contributor Author

@nenadV91 nenadV91 May 30, 2023

Choose a reason for hiding this comment

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

@anxolin

So my thinking was this:

  • we have one quote updater in the whole app called TradeQuoteUpdater and it will be called inside of TradeWidget which is then being used for LIMIT, TWAP and later on SWAP. This quote updater will be responsible for updating the quote state, and this state will of course be used by all those 3 different order types.

  • now the second part is, those observers. Their responsibility is to perform some actions when quote update happens. But, different order types, need to perform different actions when quote is changed. For example, the LIMIT order quote observer will update the rate state, that exist only in LIMIT orders, and then based on the rate state changes we update the INPUT/OUTPUT field values. And TWAP order quote observer will just update output currency.

  • useUpdateCurrencyAmount is exported from trade module, since it will update the shared trade state, and we will use this in TWAP, LIMIT etc..

  • So to summarise, the idea is that we have these observers for each order type, since they perform different actions on quote update, and the quote state is shared. Not sure if I missed the point, but I hope this clarifies things a bit.

@elena-zh
Copy link
Contributor

elena-zh commented Jun 12, 2023

@nenadV91 , @shoom3301

@elena-zh 1-3 - should be fixed and confirmed by Elena 4. I think it should be fixed now 5. This also seems to be fixed now 6. I don't think this is related to this PR

Additional

  1. This is probably not merged yet
  2. That seems to goerli and I don't know what USDT is that

Additional cases reported in #2555 (comment) are not fixed:

  1. the app still run quotes for an unsupported token on the Limit orders page. And it was merged to Prod.
  2. reproducible for the Mainnet as well.
    image

A new issue is that an error message is not removed from the form when update an amount/token pair/etc.
image

And a price quote is displayed for an empty form when change a network:
price deviation

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

Successfully merging this pull request may close these issues.

Reusable trade quote service
5 participants