swap: fetch quotes on input changes#3915
Conversation
thisconnect
left a comment
There was a problem hiding this comment.
for backend review cc @Beerosagos
| </Label> | ||
| <Button transparent className={style.maxButton}> | ||
| 45678 ETH | ||
| </Button> |
There was a problem hiding this comment.
Could you leave the max-receive button so we can add functionality later and dont forget about it?
2f5b422 to
7a23225
Compare
|
@thisconnect thanks for the review, I shoulkd have addressed all the comments |
thisconnect
left a comment
There was a problem hiding this comment.
frontend untested LGTM with small nit
| ); | ||
| }; | ||
|
|
||
| // shown in dropdown |
There was a problem hiding this comment.
nit: keep this 2 comments here and a few lines before
7a23225 to
84d8e8b
Compare
| }, QUOTE_DEBOUNCE_MS); | ||
|
|
||
| return () => { | ||
| isCancelled = true; |
There was a problem hiding this comment.
nit: use useMountedRef instead of isCancelled
There was a problem hiding this comment.
I didn't forget about this one btw :) going to change it
Edit, since my frontend knowledge is limited, this is what codex said, can you help me understand if it's reasonable:
useMountedRef() only protects against updates after unmount. The local isCancelled
flag here is doing more than that: it also cancels stale async work when the effect
reruns because sellAmount, sellAccountCode, or buyAccountCode changed.
That distinction matters here:
- user types
- effect A starts
- user types again
- effect B starts
- effect A resolves late
With isCancelled, effect A’s result is ignored after cleanup.
With only useMountedRef(), the component is still mounted, so effect A could still
update state and overwrite fresher quote data.
So my view is:
- useMountedRef() is not a drop-in replacement here
- the current cancellation pattern is justified
- if you want to improve it, use a request-id / sequence guard or AbortController, not just useMountedRef()
A concrete race looks like this:
1. You enter amount 1.
2. Request A starts for sellAmount=1.
3. Before A returns, you quickly change the amount to 10.
4. Request B starts for sellAmount=10.
5. B returns first and updates routes for 10.
6. A returns later
It seems like an unlikely scenario, but maybe worth considering?
a36569e to
b50dd7f
Compare
b50dd7f to
eadf2b1
Compare
Beerosagos
left a comment
There was a problem hiding this comment.
backend untested LGTM
This PR adds calls to the quote endpoint in the backiend whenever the input (buy/sell accounts/amounts) changes. @thisconnect can you please take a look at the frontend part? I tried to make the diff as limited as possible and some stuff is still missing (for example we do not show the fee here, and we can probably get rid of the route selector since we are only using one provider and we will have only one route). But when the frontends looks good to you, I can add someone from backend for review on that part. Thanks!
Before asking for reviews, here is a check list of the most common things you might need to consider: