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.

Sign up for GitHub

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(eth-flow): update refund info for expired orders #3222

Merged
merged 4 commits into from
Oct 16, 2023
Merged

fix(eth-flow): update refund info for expired orders #3222

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61d0872
fix(eth-flow): update refund info for expired orders
shoom3301 Oct 13, 2023
c20dbf2
chore: update comment
shoom3301 Oct 13, 2023
1a06dd7
chore: fix ExpiredOrdersUpdater conditions
shoom3301 Oct 13, 2023
6b03ec1
chore: fix namings
shoom3301 Oct 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions apps/cowswap-frontend/src/common/updaters/orders/ExpiredOrdersUpdater.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { useEffect, useCallback, useRef } from 'react'

import { EXPIRED_ORDERS_PENDING_TIME } from '@cowprotocol/common-const'
import { SupportedChainId as ChainId } from '@cowprotocol/cow-sdk'
import { useWalletInfo } from '@cowprotocol/wallet'

Expand All @@ -24,7 +23,6 @@ export function ExpiredOrdersUpdater(): null {
const updateOrders = useCallback(
async (chainId: ChainId, account: string) => {
const lowerCaseAccount = account.toLowerCase()
const now = Date.now()

if (isUpdating.current) {
return
Expand All @@ -37,12 +35,8 @@ export function ExpiredOrdersUpdater(): null {
// - Owned by the current connected account
// - Created in the last 5 min, no further
// - Not yet refunded
const pending = expiredRef.current.filter(({ owner, creationTime: creationTimeString, refundHash }) => {
shoom3301 marked this conversation as resolved.
Show resolved Hide resolved
const creationTime = new Date(creationTimeString).getTime()

return (
owner.toLowerCase() === lowerCaseAccount && now - creationTime < EXPIRED_ORDERS_PENDING_TIME && !refundHash
)
const pending = expiredRef.current.filter(({ owner, refundHash }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we update for all orders: pending and expired+un-refunded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

6b03ec1
Fixed namings to avoid confusion

Copy link
Collaborator

Choose a reason for hiding this comment

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

The initial goal of this updater (pre-ethflow) was to check orders that matched after the UI already considered them expired

return owner.toLowerCase() === lowerCaseAccount && !refundHash
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch!
Added a check: 1a06dd7

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we will.
It's actually this case: #3222 (comment)

})

if (pending.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not from this PR, but i would just remove the ELSE. the IF already returns early, so there's no point

Copy link
Contributor

Choose a reason for hiding this comment

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

also delete the dead code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Expand Down
10 changes: 8 additions & 2 deletions apps/cowswap-frontend/src/modules/swap/pure/EthFlow/EthFlowStepper/index.cosmos.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import 'inter-ui' // TODO: We need to do a cosmos wrapper with the global styles! Will reiterate to remove this line

import { useSelect } from 'react-cosmos/client'
import styled from 'styled-components/macro'

import { EthFlowStepper, EthFlowStepperProps, SmartOrderStatus } from '.'

Expand Down Expand Up @@ -376,20 +377,25 @@ const STEPS_BY_DESCRIPTION = STEPS.reduce<{ [description: string]: EthFlowSteppe
return acc
}, {})

const Wrapper = styled.div`
width: 80%;
margin: 20px auto;
`

function Fixture() {
const [stepDescription] = useSelect('steps', {
options: STEPS.map((step) => step.description),
})
const props = STEPS_BY_DESCRIPTION[stepDescription]

return (
<>
<Wrapper>
<EthFlowStepper {...props} />
<h3>Params</h3>
<div>
<pre>{JSON.stringify(props, null, 2)}</pre>
</div>
</>
</Wrapper>
)
}

Expand Down
1 change: 0 additions & 1 deletion libs/common-const/src/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@ export const NATIVE_CURRENCY_BUY_TOKEN: { [chainId in ChainId | number]: Token }
export const INPUT_OUTPUT_EXPLANATION = 'Only executed swaps incur fees.'
export const PENDING_ORDERS_BUFFER = ms`60s` // 60s
export const CANCELLED_ORDERS_PENDING_TIME = ms`5min` // 5min
export const EXPIRED_ORDERS_PENDING_TIME = ms`15min` // 15min
export const PRICE_API_TIMEOUT_MS = ms`10s` // 10s
export const GP_ORDER_UPDATE_INTERVAL = ms`30s` // 30s
export const MINIMUM_ORDER_VALID_TO_TIME_SECONDS = 120
Expand Down
Loading