-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this was made to be a quicker-than-quick fix. I'll make a PR with improvements
#1645
src/utils/miscellaneous.ts
Outdated
return new Promise((resolve, rejects) => | ||
setTimeout(() => { | ||
if (result) { | ||
resolve(result) | ||
} else { | ||
rejects(new Error(timeoutMsg)) | ||
} | ||
}, time), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have delay()
util to do that
src/utils/miscellaneous.ts
Outdated
timeoutErrorMsg?: string | ||
} | ||
|
||
export function timeout<T>(params: TimeoutParams<T>): Promise<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types are not exactly correct. You intentionally throw (return rejected Promise) for some T
. Typescript can detect that if you do an overload type, for example
src/hooks/useTokenBalances.ts
Outdated
fetchBalancesForToken(token, userAddress, contractAddress, networkId) | ||
.then((balance) => { | ||
const balancePromises: Promise<TokenBalanceDetails | null>[] = tokens.map((token) => { | ||
const timeoutPromise = timeout<TokenBalanceDetails | null>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this isn't Promise<TokenBalanceDetails | null>
, this is Promise<never>
as it will throw, always.
src/hooks/useTokenBalances.ts
Outdated
const balances = await Promise.all(balancePromises) | ||
|
||
// TODO: Would be better to show the errored tokens in error state | ||
return balances.filter(Boolean) as TokenBalanceDetails[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to cast if we use a type-guard predicate notEmpty
, which you made precisely for these cases, didn't you?
src/hooks/useTokenBalances.ts
Outdated
logDebug('Using cached value for', token, userAddress, contractAddress) | ||
return cachedValue | ||
} | ||
const cacheKey = constructCacheKey({ token, userAddress, contractAddress, networkId }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No real need to have constructCacheKey(...)
in two places
* simplify type without casting * default delay() return to void * simplify timeout function logic * improve timeout types * use new timeout with automatic rejected Promise detection * construct cacheKey once only * throw proper Error
We've observed in two occasions now that some Metamask fails to resolve some promise for contract calls. First with the issue of SNX/sUSD proxy deprecation, yesterday with Ledger MM (not it seems it doesn't do that anymore).
When this happens, we neither get a success or an error.
Currently, if for one token, the promise is not resolved, the overall "fetch all balances" promise don't resolve either (uses
Promise.all
)This PR introduces a timeout for fetching individual tokens for this extreme cases. It uses 10s, maybe it could be less. But theoretically promises should resolve one way or the other!