Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Add waiting time to reconnection #2526

Merged
merged 4 commits into from
Mar 9, 2022
Merged

Add waiting time to reconnection #2526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
28126c9
Add waiting time to reconnection
henrypalacios Mar 4, 2022
6265d81
Remove label countdown
henrypalacios Mar 4, 2022
282a2be
Avoid modifying the original constants file
henrypalacios Mar 8, 2022
f2b3104
Improve comments
henrypalacios Mar 9, 2022
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
8 changes: 8 additions & 0 deletions src/constants/wallet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,15 @@ export const SUPPORTED_WALLETS: { [key: string]: WalletInfo } = {
description: 'Use Coinbase Wallet app on mobile device',
href: null,
color: '#315CF5',
},
anxolin marked this conversation as resolved.
Show resolved Hide resolved
COINBASE_LINK: {
name: 'Open in Coinbase Wallet',
iconURL: COINBASE_ICON_URL,
description: 'Open in Coinbase Wallet app.',
href: 'https://go.cb-w.com/mtUDhEZPy1',
color: '#315CF5',
mobile: true,
mobileOnly: true,
},
FORTMATIC: {
connector: fortmatic,
Expand Down
9 changes: 8 additions & 1 deletion src/custom/constants/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,13 @@ export const APP_DATA_HASH = getAppDataHash()
export const PRODUCTION_URL = 'cowswap.exchange'
export const BARN_URL = `barn.${PRODUCTION_URL}`

const DISABLED_WALLETS = /^(?:Portis)$/i
// Allow WALLET_LINK to be activated on mobile
// since COINBASE_LINK is limited to use only 1 deeplink on mobile
SUPPORTED_WALLETS_UNISWAP.WALLET_LINK = {
...SUPPORTED_WALLETS_UNISWAP.WALLET_LINK,
mobile: true,
}
const DISABLED_WALLETS = /^(?:Portis|COINBASE_LINK)$/i

// Re-export only the supported wallets
export const SUPPORTED_WALLETS = Object.keys(SUPPORTED_WALLETS_UNISWAP).reduce((acc, key) => {
Expand Down Expand Up @@ -115,6 +121,7 @@ export const AMOUNT_OF_ORDERS_TO_FETCH = 100

// last wallet provider key used in local storage
export const STORAGE_KEY_LAST_PROVIDER = 'lastProvider'
export const WAITING_TIME_RECONNECT_LAST_PROVIDER = 15000 // 15s

// Default price strategy to use for getting app prices
// COWSWAP = new quote endpoint
Expand Down
13 changes: 11 additions & 2 deletions src/custom/hooks/web3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { AbstractConnector } from '@web3-react/abstract-connector'
import { useEffect, useState, useCallback } from 'react'
import { isMobile } from 'react-device-detect'
import { injected, walletconnect, getProviderType, WalletProvider, fortmatic, walletlink } from 'connectors'
import { STORAGE_KEY_LAST_PROVIDER } from 'constants/index'
import { STORAGE_KEY_LAST_PROVIDER, WAITING_TIME_RECONNECT_LAST_PROVIDER } from 'constants/index'

// exports from the original file
export { useActiveWeb3React, useInactiveListener } from '@src/hooks/web3'
Expand Down Expand Up @@ -58,7 +58,7 @@ export function useEagerConnect() {
setTried(true)
})
},
[activate, setTried]
[activate]
)

useEffect(() => {
Expand All @@ -85,9 +85,18 @@ export function useEagerConnect() {

// if the connection worked, wait until we get confirmation of that to flip the flag
useEffect(() => {
let timeout: NodeJS.Timeout | undefined

if (active) {
setTried(true)
} else {
timeout = setTimeout(() => {
localStorage.removeItem(STORAGE_KEY_LAST_PROVIDER)
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 get it. Why do we need to add this? maybe a comment would help to understand.

Copy link
Contributor Author

@henrypalacios henrypalacios Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The last provider is stored before the unload component.

After a page reload when trying to reconnect a wallet from an external provider such as Fortmatic or CoinbaseWallet we have no way of knowing if the session was cut off.

The timeout would be the time we wait while the external provider reconnects the session, otherwise we assume that it has been tried, there is no lastProvider and the user can open walletModals.

Copy link
Contributor Author

@henrypalacios henrypalacios Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thinking about it, maybe it makes more sense that this timeout logic is inside the reconnectProvider and this way it is only executed when there is a lastProvider.

I can do this together with #2515 but let me know if you have any suggestion on this.

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 fully get it TBH. In principle seems like a fragile logic. Also, i was referring to adding a comment in the code too

Maybe i would understand describing the sequence of events, and why we really need this timeout logic

setTried(true)
}, WAITING_TIME_RECONNECT_LAST_PROVIDER)
}

return () => timeout && clearTimeout(timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we clear the timeout? I understand we don't want to do the setTried but, if you clear it you won't do the removeItem

Copy link
Contributor Author

@henrypalacios henrypalacios Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The clearTimeout is because if during the waiting time the provider changes its active sate, it would no longer need to remove the lastProvider.

And ultimately removeItem will be executed by handleBeforeUnload

}, [active])

useEffect(() => {
Expand Down