fix(eth-flow): update refund info for expired orders #3222
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
apps/cowswap-frontend/src/common/updaters/orders/ExpiredOrdersUpdater.ts
Outdated
Show resolved
Hide resolved
| const pending = expiredRef.current.filter(({ owner, refundHash }) => { | ||
| return owner.toLowerCase() === lowerCaseAccount && !refundHash |
There was a problem hiding this comment.
Actually, this might be a problem, because we'll forever try to update expired orders that are not ETH FLOW (don't have refundHash), and that's why this check was there in the first place.
Maybe it should only NOT have the limit when it's an ETH FLOW order.
There was a problem hiding this comment.
Nice catch!
Added a check: 1a06dd7
There was a problem hiding this comment.
But what if an order is expired, and we don't know if its traded or not trade?
we created the order, closed the browswe and came back 1h later
will we update the order and get the right state?
There was a problem hiding this comment.
Yes, we will.
It's actually this case: #3222 (comment)
|
The PR healed my state automatically |
| owner.toLowerCase() === lowerCaseAccount && now - creationTime < EXPIRED_ORDERS_PENDING_TIME && !refundHash | ||
| ) | ||
| const pending = expiredRef.current.filter(({ owner, refundHash }) => { | ||
| return owner.toLowerCase() === lowerCaseAccount && !refundHash | ||
| }) | ||
|
|
||
| if (pending.length === 0) { |
There was a problem hiding this comment.
not from this PR, but i would just remove the ELSE. the IF already returns early, so there's no point
There was a problem hiding this comment.
It actually makes sense.
If we don't have any pending orders, we should not move further.
I see only one exit before, when isUpdating.current is truth, but it's different
| return ( | ||
| owner.toLowerCase() === lowerCaseAccount && now - creationTime < EXPIRED_ORDERS_PENDING_TIME && !refundHash | ||
| ) | ||
| const pending = expiredRef.current.filter(({ owner, refundHash }) => { |
There was a problem hiding this comment.
I don't understand this. We only check the new state on expired orders?
the method is called updateOrders, but it looks like is just for handling these type of orders
There was a problem hiding this comment.
shouldn't we update for all orders: pending and expired+un-refunded?
There was a problem hiding this comment.
So, as far as I understand, the updater does only one thing:
It takes expired eth-flow orders without refundHash value and set it if it exists in order-book
There was a problem hiding this comment.
6b03ec1
Fixed namings to avoid confusion
There was a problem hiding this comment.
The initial goal of this updater (pre-ethflow) was to check orders that matched after the UI already considered them expired
apps/cowswap-frontend/src/common/updaters/orders/ExpiredOrdersUpdater.ts
Outdated
Show resolved
Hide resolved
apps/cowswap-frontend/src/common/updaters/orders/ExpiredOrdersUpdater.ts
Show resolved
Hide resolved
| return ( | ||
| owner.toLowerCase() === lowerCaseAccount && now - creationTime < EXPIRED_ORDERS_PENDING_TIME && !refundHash | ||
| ) | ||
| return isEthFlowOrder && owner.toLowerCase() === lowerCaseAccount && !refundHash |
There was a problem hiding this comment.
What about regular orders?
Now they are not checked at all.
I meant to keep the former behaviour for regular orders, and check ethflow orders without a refund hash indefinitely.
There was a problem hiding this comment.
But only eth-flow order might have refundHash, isn't it?
There was a problem hiding this comment.
Yes, you are 100% correct.
I was confusing this updated with the CancelledOrdersUpdater
Summary
Fixes #3218
The problem was in
ExpiredOrdersUpdater, it was updating only orders created within the last 15 minutes, obviously, no one expired eth-flow order doesn't match this condition.To Test