-
Notifications
You must be signed in to change notification settings - Fork 176
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.
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
feat: blazingly fast initial accounts load #7822
base: develop
Are you sure you want to change the base?
Changes from 19 commits
87a2daf
44630f9
ce1172b
06ca68f
57a04c8
190de45
3507515
eee8fd7
10189e6
fb7f97d
da7d405
b6dc216
2b4defc
2746284
ed84af5
847369c
84303c3
6df58ff
150ae0f
64e8c63
5a3b1ae
ac172d0
5c094cc
81b6e9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||
import { usePrevious, useToast } from '@chakra-ui/react' | ||||||||||||||||||||||||||||
import type { AccountId, ChainId } from '@shapeshiftoss/caip' | ||||||||||||||||||||||||||||
import { fromAccountId } from '@shapeshiftoss/caip' | ||||||||||||||||||||||||||||
import type { LedgerOpenAppEventArgs } from '@shapeshiftoss/chain-adapters' | ||||||||||||||||||||||||||||
import { emitter } from '@shapeshiftoss/chain-adapters' | ||||||||||||||||||||||||||||
|
@@ -33,11 +34,11 @@ import { preferences } from 'state/slices/preferencesSlice/preferencesSlice' | |||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||
selectAccountIdsByChainId, | ||||||||||||||||||||||||||||
selectAssetIds, | ||||||||||||||||||||||||||||
selectEnabledWalletAccountIds, | ||||||||||||||||||||||||||||
selectPortfolioAssetIds, | ||||||||||||||||||||||||||||
selectPortfolioLoadingStatus, | ||||||||||||||||||||||||||||
selectSelectedCurrency, | ||||||||||||||||||||||||||||
selectSelectedLocale, | ||||||||||||||||||||||||||||
selectWalletAccountIds, | ||||||||||||||||||||||||||||
} from 'state/slices/selectors' | ||||||||||||||||||||||||||||
import { txHistoryApi } from 'state/slices/txHistorySlice/txHistorySlice' | ||||||||||||||||||||||||||||
import { useAppDispatch, useAppSelector } from 'state/store' | ||||||||||||||||||||||||||||
|
@@ -59,7 +60,7 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { | |||||||||||||||||||||||||||
const { supportedChains } = usePlugins() | ||||||||||||||||||||||||||||
const wallet = useWallet().state.wallet | ||||||||||||||||||||||||||||
const assetIds = useSelector(selectAssetIds) | ||||||||||||||||||||||||||||
const requestedAccountIds = useSelector(selectWalletAccountIds) | ||||||||||||||||||||||||||||
const requestedAccountIds = useSelector(selectEnabledWalletAccountIds) | ||||||||||||||||||||||||||||
const portfolioLoadingStatus = useSelector(selectPortfolioLoadingStatus) | ||||||||||||||||||||||||||||
const portfolioAssetIds = useSelector(selectPortfolioAssetIds) | ||||||||||||||||||||||||||||
const routeAssetId = useRouteAssetId() | ||||||||||||||||||||||||||||
|
@@ -144,19 +145,16 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { | |||||||||||||||||||||||||||
// This ensures that we have fresh portfolio data, but accounts added through account management are not accidentally blown away. | ||||||||||||||||||||||||||||
if (hasManagedAccounts) { | ||||||||||||||||||||||||||||
requestedAccountIds.forEach(accountId => { | ||||||||||||||||||||||||||||
dispatch( | ||||||||||||||||||||||||||||
portfolioApi.endpoints.getAccount.initiate( | ||||||||||||||||||||||||||||
{ accountId, upsertOnFetch: true }, | ||||||||||||||||||||||||||||
{ forceRefetch: true }, | ||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
dispatch(portfolioApi.endpoints.getAccount.initiate({ accountId, upsertOnFetch: true })) | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if (!wallet || isLedger(wallet)) return | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const walletId = await wallet.getDeviceID() | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
let chainIds = supportedChains.filter(chainId => { | ||||||||||||||||||||||||||||
return walletSupportsChain({ | ||||||||||||||||||||||||||||
chainId, | ||||||||||||||||||||||||||||
|
@@ -192,64 +190,121 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { | |||||||||||||||||||||||||||
Object.assign(accountMetadataByAccountId, accountIdsAndMetadata) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { getAccount } = portfolioApi.endpoints | ||||||||||||||||||||||||||||
const accountPromises = accountIds.map(accountId => | ||||||||||||||||||||||||||||
dispatch(getAccount.initiate({ accountId }, { forceRefetch: true })), | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const accountResults = await Promise.allSettled(accountPromises) | ||||||||||||||||||||||||||||
// Chunks of AccountIds | ||||||||||||||||||||||||||||
// For UTXOs, each chain gets its own chunk to detect chain activity over other scriptTypes | ||||||||||||||||||||||||||||
// For others, no need to use chunks, but for the sake of consistency, we keep the same structure | ||||||||||||||||||||||||||||
const accountNumberAccountIdChunks = ( | ||||||||||||||||||||||||||||
_accountIds: AccountId[], | ||||||||||||||||||||||||||||
): Record<ChainId, AccountId[]> => { | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion]: Rename this to These aren't really chunks - chunking typically refers to batching an array into multiple smaller arrays. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ab-solutely 💯: 5c094cc |
||||||||||||||||||||||||||||
return _accountIds.reduce( | ||||||||||||||||||||||||||||
(acc, _accountId) => { | ||||||||||||||||||||||||||||
const { chainId } = fromAccountId(_accountId) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if (isUtxoChainId(chainId)) { | ||||||||||||||||||||||||||||
acc[chainId] = [_accountId] | ||||||||||||||||||||||||||||
return acc | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion]: This path shouldnt be required if there arent multiple utxo account ids for a given chainId. I.e if there is no collision on chainId we can rely on the flow below this statement to correctly group accountIds by chainId - even if there is only 1 per chainId. If the intent is to "overwrite" i.e "last write wins" then leave as-is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are absolutely right! Reading this again with fresh eyes, this looks blatantly wrong. A UTXO ChainId may and will most likely contain multiple AccountIds (one per scriptType), and we need them all in one "chunk" (terminology removed btw!) to detect activity across scriptTypes, that effectively voided it. |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
if (!acc[chainId]) { | ||||||||||||||||||||||||||||
acc[chainId] = [] | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
acc[chainId].push(_accountId) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return acc | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{} as Record<ChainId, AccountId[]>, | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
let chainIdsWithActivity: string[] = [] | ||||||||||||||||||||||||||||
accountResults.forEach((res, idx) => { | ||||||||||||||||||||||||||||
if (res.status === 'rejected') return | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { data: account } = res.value | ||||||||||||||||||||||||||||
if (!account) return | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const accountId = accountIds[idx] | ||||||||||||||||||||||||||||
const { chainId } = fromAccountId(accountId) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { hasActivity } = account.accounts.byId[accountId] | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const accountNumberHasChainActivity = !isUtxoChainId(chainId) | ||||||||||||||||||||||||||||
? hasActivity | ||||||||||||||||||||||||||||
: // For UTXO AccountIds, we need to check if *any* of the scriptTypes have activity, not only the current one | ||||||||||||||||||||||||||||
// else, we might end up with partial account data, with only the first 1 or 2 out of 3 scriptTypes | ||||||||||||||||||||||||||||
// being upserted for BTC and LTC | ||||||||||||||||||||||||||||
accountResults.some((res, _idx) => { | ||||||||||||||||||||||||||||
if (res.status === 'rejected') return false | ||||||||||||||||||||||||||||
const { data: account } = res.value | ||||||||||||||||||||||||||||
if (!account) return false | ||||||||||||||||||||||||||||
const accountId = accountIds[_idx] | ||||||||||||||||||||||||||||
const { chainId: _chainId } = fromAccountId(accountId) | ||||||||||||||||||||||||||||
if (chainId !== _chainId) return false | ||||||||||||||||||||||||||||
return account.accounts.byId[accountId].hasActivity | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// don't add accounts with no activity past account 0 | ||||||||||||||||||||||||||||
if (accountNumber > 0 && !accountNumberHasChainActivity) | ||||||||||||||||||||||||||||
return delete accountMetadataByAccountId[accountId] | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// unique set to handle utxo chains with multiple account types per account | ||||||||||||||||||||||||||||
chainIdsWithActivity = Array.from(new Set([...chainIdsWithActivity, chainId])) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
dispatch(portfolio.actions.upsertPortfolio(account)) | ||||||||||||||||||||||||||||
// Chunks of AccountIds promises for accountNumber/AccountId combination | ||||||||||||||||||||||||||||
// This allows every run of AccountIds per chain/accountNumber to run in parallel vs. all sequentally, so | ||||||||||||||||||||||||||||
// we can run each chain's side effects immediately | ||||||||||||||||||||||||||||
const accountNumberAccountIdsPromises = Object.values( | ||||||||||||||||||||||||||||
accountNumberAccountIdChunks(accountIds), | ||||||||||||||||||||||||||||
).map(async accountIds => { | ||||||||||||||||||||||||||||
const results = await Promise.allSettled( | ||||||||||||||||||||||||||||
accountIds.map(async id => { | ||||||||||||||||||||||||||||
const result = await dispatch(getAccount.initiate({ accountId: id })) | ||||||||||||||||||||||||||||
return result | ||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
results.forEach((res, idx) => { | ||||||||||||||||||||||||||||
if (res.status === 'rejected') return | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { data: account } = res.value | ||||||||||||||||||||||||||||
if (!account) return | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const accountId = accountIds[idx] | ||||||||||||||||||||||||||||
const { chainId } = fromAccountId(accountId) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { hasActivity } = account.accounts.byId[accountId] | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const accountNumberHasChainActivity = !isUtxoChainId(chainId) | ||||||||||||||||||||||||||||
? hasActivity | ||||||||||||||||||||||||||||
: // For UTXO AccountIds, we need to check if *any* of the scriptTypes have activity, not only the current one | ||||||||||||||||||||||||||||
// else, we might end up with partial account data, with only the first 1 or 2 out of 3 scriptTypes | ||||||||||||||||||||||||||||
// being upserted for BTC and LTC | ||||||||||||||||||||||||||||
results.some((res, _idx) => { | ||||||||||||||||||||||||||||
if (res.status === 'rejected') return false | ||||||||||||||||||||||||||||
const { data: account } = res.value | ||||||||||||||||||||||||||||
if (!account) return false | ||||||||||||||||||||||||||||
const accountId = accountIds[_idx] | ||||||||||||||||||||||||||||
const { chainId: _chainId } = fromAccountId(accountId) | ||||||||||||||||||||||||||||
if (chainId !== _chainId) return false | ||||||||||||||||||||||||||||
return account.accounts.byId[accountId].hasActivity | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// don't add accounts with no activity past account 0 | ||||||||||||||||||||||||||||
if (accountNumber > 0 && !accountNumberHasChainActivity) { | ||||||||||||||||||||||||||||
chainIdsWithActivity = chainIdsWithActivity.filter(_chainId => _chainId !== chainId) | ||||||||||||||||||||||||||||
delete accountMetadataByAccountId[accountId] | ||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||
// unique set to handle utxo chains with multiple account types per account | ||||||||||||||||||||||||||||
chainIdsWithActivity = Array.from(new Set([...chainIdsWithActivity, chainId])) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
dispatch(portfolio.actions.upsertPortfolio(account)) | ||||||||||||||||||||||||||||
const chainIdAccountMetadata = Object.entries(accountMetadataByAccountId).reduce( | ||||||||||||||||||||||||||||
(acc, [accountId, metadata]) => { | ||||||||||||||||||||||||||||
const { chainId: _chainId } = fromAccountId(accountId) | ||||||||||||||||||||||||||||
if (chainId === _chainId) { | ||||||||||||||||||||||||||||
acc[accountId] = metadata | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
return acc | ||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
{} as AccountMetadataById, | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
Comment on lines
+262
to
+271
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [preferably blocking]: Untested but if possible this would be nicer:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Left as-is as this will lose the intent of this (keeping only the AccountIds - note plural here - for the current ChainId) and won't generalize to UTXOs (this would only keep the last AccountId for UTXO ChainIds) |
||||||||||||||||||||||||||||
dispatch( | ||||||||||||||||||||||||||||
portfolio.actions.upsertAccountMetadata({ | ||||||||||||||||||||||||||||
accountMetadataByAccountId: chainIdAccountMetadata, | ||||||||||||||||||||||||||||
walletId, | ||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
for (const accountId of Object.keys(accountMetadataByAccountId)) { | ||||||||||||||||||||||||||||
dispatch(portfolio.actions.enableAccountId(accountId)) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return results | ||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
chainIds = chainIdsWithActivity | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
await Promise.allSettled(accountNumberAccountIdsPromises) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
dispatch( | ||||||||||||||||||||||||||||
portfolio.actions.upsertAccountMetadata({ | ||||||||||||||||||||||||||||
accountMetadataByAccountId, | ||||||||||||||||||||||||||||
walletId: await wallet.getDeviceID(), | ||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
for (const accountId of Object.keys(accountMetadataByAccountId)) { | ||||||||||||||||||||||||||||
dispatch(portfolio.actions.enableAccountId(accountId)) | ||||||||||||||||||||||||||||
chainIds = chainIdsWithActivity | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion]: For both performance and readability, chainIdsWithActivity.delete(chainId)
chainIdsWithActivity.add(chainId) instead of chainIdsWithActivity = chainIdsWithActivity.filter(_chainId => _chainId !== chainId)
chainIdsWithActivity = Array.from(new Set([...chainIdsWithActivity, chainId])) and then at the end you can assign There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Much cleaner indeed! Went with keeping everything as a set and doing in-place 81b6e9b and confirmed we're still happy! https://jam.dev/c/6625fa90-5f30-4cd1-9acb-b46a9ad82dad |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} finally { | ||||||||||||||||||||||||||||
dispatch(portfolio.actions.setIsAccountMetadataLoading(false)) | ||||||||||||||||||||||||||||
// Only fetch and upsert Tx history once all are loaded, otherwise big main thread rug | ||||||||||||||||||||||||||||
const { getAllTxHistory } = txHistoryApi.endpoints | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
await Promise.all( | ||||||||||||||||||||||||||||
requestedAccountIds.map(requestedAccountId => | ||||||||||||||||||||||||||||
dispatch(getAllTxHistory.initiate(requestedAccountId)), | ||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
Comment on lines
+293
to
+300
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed Tx history is still fetched on account management new accounts toggle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. really nice, so much better for readability than relying on separate useEffect |
||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
})() | ||||||||||||||||||||||||||||
}, [ | ||||||||||||||||||||||||||||
|
@@ -271,18 +326,6 @@ export const AppProvider = ({ children }: { children: React.ReactNode }) => { | |||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||
}, [dispatch, portfolioLoadingStatus]) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// once portfolio is done loading, fetch all transaction history | ||||||||||||||||||||||||||||
useEffect(() => { | ||||||||||||||||||||||||||||
;(async () => { | ||||||||||||||||||||||||||||
if (!requestedAccountIds.length) return | ||||||||||||||||||||||||||||
if (portfolioLoadingStatus === 'loading') return | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const { getAllTxHistory } = txHistoryApi.endpoints | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
await dispatch(getAllTxHistory.initiate(requestedAccountIds)) | ||||||||||||||||||||||||||||
})() | ||||||||||||||||||||||||||||
}, [dispatch, requestedAccountIds, portfolioLoadingStatus]) | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
const marketDataPollingInterval = 60 * 15 * 1000 // refetch data every 15 minutes | ||||||||||||||||||||||||||||
useQueries({ | ||||||||||||||||||||||||||||
queries: portfolioAssetIds.map(assetId => ({ | ||||||||||||||||||||||||||||
|
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 forceRefetch here. If this is the first call, this will fetch, else leverage RTK caching to avoid spew spew