feat: blazingly fast initial accounts load #7822

gomesalexandre · 2024-09-26T14:30:58Z

Description

This PR reads like a ~~mental breakdown of trying to optimize the initial load of accounts~~ recipe, rather than a definite solution. It is by no means final and discussions / blockers / inputs are more than welcome 🙏🏽.
It consists of many optimizations, some of them we may want to keep, some of them we may not. In fact, you can see some of them being ditched as part of progression here.

Here is a summary of the different paths taken in this PR, and their rationale:

87a2daf - while we originally thought that XHRs were the bottleneck, side-effects (upsertion/loading state detection) really is. In fact, not having a queue means we are spammy, but we do fetch faster. reintroduced in 10189e6 - there was a bug in my initial implementation which rugged multi-account. As we fetch more account numbers, things were very, very spewy and eventually result in failures/account degradation.
44630f9 - removes upsertOnFetch option in upsertPortfolio. <AppContext /> was now the only one consuming it, for legacy reasons which we handle somewhere else now: https://github.com/shapeshift/web/pull/3294/files#diff-bf8aa83a0397b2b1f8bbe9f8e94af3e311f0046d1445385a49c06a2221a7b602R134 kept back just in case as this seems to make things somehow safer, will take another look on a granular PR
ce1172b / 06ca68f: we were using the autobatch enhancer to mark the upsertPortfolio as a low prio action. Since it is one of the actions that lead to first interactive accounts, we should not mark these as low prio. They should be processed normally.
57a04c8 a.k.a "@woodenfurniture don't spank me": changes the isPortfolioLoaded heuristics to assume loaded when there is at least one AccountId for that walletId in the store, vs. at least one enabled AccountId for that WalletId previously. The rationale being the whole enabled logic is currently done as a side effect much, much later on (when all finish fetching). Which was one of the main reasons behind the slowness previously - accounts were fetched, upsertions were done, but the final metadata upsertion (which is where we set AccountIds as enabled) is done after all are loaded, so AccountIds being present in the slice didn't cut it, they were present early on, but not enabled until much, much later on.
The crux of this diff, which makes all other improvements actually work: 190de45. As mentioned above, we do fetch all accounts, then proceed to some side effects after all accounts are loaded. However, we really only need this for UTXOs because of the whole "if one scripType is loaded, assume the account is". Did a very naive, but sufficiently non-naive logic of grouping into chain namespaces for the time being: instead of waiting for all AccountIds to be loaded before performing side effects, we now group them into ChainNamespaces. Obviously could be improved as a follow-up by grouping all AccountIds for a given UTXO into one chunk with their side effects processed after each chunk, then another single chunk with all other AccountIds with side-effects processed after each item but meh.
fix borked multi-account in 3507515 which in turns makes us slow again. Here we go again.
eee8fd7: incremental load of accounts 🎉
10189e6 fixes infinite loop in fetching
b6dc216 - brings back upsertOnFetch for perf. Reasons
2b4defc - awaits for all to settle and metadata to be upserted before fetching Tx history
2746284 - make getAllTxHistory take an AccountId vs. accountIds: AccountId[] previously, which allows us to leverage RTK caching of calls for a given AccountId
ed84af5 - both a perf improvement and a fix to this PR: ensures we only upsert the AccountIds for the current ChainId/AccountNumber vs. the whole accounts dataset previously. This avoids useless large sets re-upserts, while at the same time fixing a bug with inactive account number being upserted.

Where to go from here

If we're happy with the changes here and decide to go with it, an issue/follow-up should be created to handle incremental loads with an ACC broadly like this:

ChainIds remaining account number fetches required should show the currently loaded accounts plus skeleton empty slots
ChainIds which we know are queued to be fetched but haven't started fetching yet (including account 0) should display skeleton empty slots

Issue (if applicable)

closes Account discovery on load from clean cache is slow #7486
closes Wallet load time increasing #7814

Risk

High Risk PRs Require 2 approvals

High - touches accounts/meta fetching

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

Accounts come in incrementally in wallet section as opposed to all at once in develop/prod
Accounts still look sane compared to develop/prod
Tx history still looks sane compared to develop/prod
Account management still works
Test the above with Ledger too

Engineering

Operations

🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

Accounts come in incrementally in wallet section / Accounts and Tx history still look sane compared to develop/prod

https://jam.dev/c/edaa2299-f301-4e7e-8478-edbd1dbea9d2

Account Management is still happy compared to develop/prod

https://jam.dev/c/75f15c70-7829-4562-8ef0-14982cfaf353

back p-queue

…tial

src/components/Layout/Header/DegradedStateBanner.tsx

src/components/ManageAccountsDrawer/components/ImportAccounts.tsx

gomesalexandre · 2024-09-27T13:36:42Z

src/context/AppProvider/AppContext.tsx

-                { forceRefetch: true },
-              ),
-            )
+            dispatch(portfolioApi.endpoints.getAccount.initiate({ accountId, upsertOnFetch: true }))


No forceRefetch here. If this is the first call, this will fetch, else leverage RTK caching to avoid spew spew

src/context/AppProvider/AppContext.tsx

gomesalexandre · 2024-09-27T13:44:08Z

src/context/AppProvider/AppContext.tsx

+        // 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)),
+          ),
+        )


Confirmed Tx history is still fetched on account management new accounts toggle

really nice, so much better for readability than relying on separate useEffect

gomesalexandre · 2024-09-27T13:45:28Z

src/state/slices/common-selectors.ts

  (walletId, walletById, enabledAccountIds): AccountId[] => {
    const walletAccountIds = (walletId && walletById[walletId]) ?? []
    return walletAccountIds.filter(accountId => (enabledAccountIds ?? []).includes(accountId))
  },
 )

+export const selectWalletAccountIds = createDeepEqualOutputSelector(


The same as above, but not necessarily enabled (yet)

gomesalexandre · 2024-09-27T13:47:09Z

src/state/slices/common-selectors.ts

@@ -38,23 +38,32 @@ export const selectWalletEnabledAccountIds = createDeepEqualOutputSelector(
  },
 )

-export const selectWalletAccountIds = createDeepEqualOutputSelector(
+export const selectEnabledWalletAccountIds = createDeepEqualOutputSelector(


enabled vs. enabledWallet above, meaning this selects enabled AccountIds which are in portfolio.wallet.byId.

In comparison, the above selects enabledAccountIds[walletId], but these may not be present in portfolio.wallet.byId (yet)

you are the 🐐 of naming sir

gomesalexandre · 2024-09-27T13:47:47Z

src/state/slices/portfolioSlice/portfolioSlice.ts

-      // Use the `prepareAutoBatched` utility to automatically
-      // add the `action.meta[SHOULD_AUTOBATCH]` field the enhancer needs
-      prepare: prepareAutoBatched<Portfolio>(),
+    upsertPortfolio: (draftState, { payload }: { payload: Portfolio }) => {


Removed prepare and consequently reducer, which isn't possible without prepare (at least at types-level)

gomesalexandre · 2024-09-27T13:49:08Z

src/state/slices/txHistorySlice/txHistorySlice.ts

-                  dispatch(txHistory.actions.setAccountIdErrored(accountId))
-                }
-              }
+    getAllTxHistory: build.query<null, AccountId>({


Review me without whitespaces, the only changes here is this fetches a single accountId, which ensures proper caching across runs.

thank you for this tip!

woodenfurniture

Code review done, looking excellent. Will test after requested changes actioned

woodenfurniture · 2024-09-27T22:52:53Z

src/state/slices/txHistorySlice/txHistorySlice.ts

@@ -206,95 +206,91 @@ export const txHistory = createSlice({
  },
 })

+// Exported as a paranoia to ensure module-time evaluation as a singleton
+export const requestQueue = new PQueue({ concurrency: 2 })


[suggestion]: this shouldnt be necessary and there isnt really a use case for exposing the internals of the query. It actually does make sense to break it out like you have though, so this change should mostly stay except for the export

Might as well remove the paranoia, indeed! 5a3b1ae

woodenfurniture · 2024-09-27T22:54:21Z

src/state/slices/txHistorySlice/txHistorySlice.ts

-                  dispatch(txHistory.actions.setAccountIdErrored(accountId))
-                }
-              }
+    getAllTxHistory: build.query<null, AccountId>({


thank you for this tip!

woodenfurniture · 2024-09-27T22:56:01Z

src/state/slices/common-selectors.ts

@@ -38,23 +38,32 @@ export const selectWalletEnabledAccountIds = createDeepEqualOutputSelector(
  },
 )

-export const selectWalletAccountIds = createDeepEqualOutputSelector(
+export const selectEnabledWalletAccountIds = createDeepEqualOutputSelector(


you are the 🐐 of naming sir

woodenfurniture · 2024-09-27T22:58:51Z

src/state/slices/common-selectors.ts

+  selectWalletId,
+  (state: ReduxState) => state.portfolio.wallet.byId,
+  (walletId, walletById): AccountId[] => {
+    const walletAccountIds = (walletId && walletById[walletId]) ?? []


[100% non-blocking]: just found it interesting that optional chaining applies here and can be concise-er

Suggested change

const walletAccountIds = (walletId && walletById[walletId]) ?? []

const walletAccountIds = walletById?.[walletId] ?? []

woodenfurniture · 2024-09-27T23:06:40Z

src/context/AppProvider/AppContext.tsx

+          // 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[]> => {


[suggestion]: Rename this to accountNumberAccountIdsByChainId or similar and update the rest of the vernacular to match.

These aren't really chunks - chunking typically refers to batching an array into multiple smaller arrays.

Ab-solutely 💯: 5c094cc

woodenfurniture · 2024-09-27T23:10:01Z

src/context/AppProvider/AppContext.tsx

+                if (isUtxoChainId(chainId)) {
+                  acc[chainId] = [_accountId]
+                  return acc
+                }


[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

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.
5c094cc

woodenfurniture · 2024-09-27T23:20:37Z

src/context/AppProvider/AppContext.tsx

+              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,
+                )
+                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


[suggestion]: For both performance and readability, chainIdsWithActivity would be much better suited as a const chainIdsWithActivity: Set<ChainId> = new Set() instead of a let chainIdsWithActivity: string[] = []. This way, above you can mutate the set rather than reassigning a new array at every step:

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 chainIds = Array.from(chainIdsWithActivity)

Much cleaner indeed! Went with keeping everything as a set and doing in-place Array.from() for the one place where chainIds is required by another API (deriveAccountIdsAndMetadata) to be an array:

81b6e9b and confirmed we're still happy! https://jam.dev/c/6625fa90-5f30-4cd1-9acb-b46a9ad82dad

woodenfurniture · 2024-09-27T23:26:35Z

src/context/AppProvider/AppContext.tsx

+                const chainIdAccountMetadata = Object.entries(accountMetadataByAccountId).reduce(
+                  (acc, [accountId, metadata]) => {
+                    const { chainId: _chainId } = fromAccountId(accountId)
+                    if (chainId === _chainId) {
+                      acc[accountId] = metadata
+                    }
+                    return acc
+                  },
+                  {} as AccountMetadataById,
+                )


[preferably blocking]: Untested but if possible this would be nicer:

Suggested change

const chainIdAccountMetadata = Object.entries(accountMetadataByAccountId).reduce(

(acc, [accountId, metadata]) => {

const { chainId: _chainId } = fromAccountId(accountId)

if (chainId === _chainId) {

acc[accountId] = metadata

}

return acc

},

{} as AccountMetadataById,

)

const chainIdAccountMetadata = {

[chainId]: accountMetadataByAccountId[toAccountId({ chainId, account })]

}

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)

woodenfurniture · 2024-09-27T23:27:24Z

src/context/AppProvider/AppContext.tsx

+        // 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)),
+          ),
+        )


really nice, so much better for readability than relying on separate useEffect

gomesalexandre added 6 commits September 26, 2024 14:39

feat: rm request queue

87a2daf

feat: rm upsertOnFetch

44630f9

feat: don't mark upsertPortfolio as low prio action

ce1172b

feat: rm prepare

06ca68f

fix: isPortfolioLoaded heuristics

57a04c8

feat: make metadata upsertion blazingly fast

190de45

gomesalexandre requested a review from a team as a code owner September 26, 2024 14:30

gomesalexandre marked this pull request as draft September 26, 2024 15:01

gomesalexandre added 8 commits September 26, 2024 17:42

fix: multi-account derp

3507515

fix: incremental load

eee8fd7

fix: filter out chains without history for account number, and revert

10189e6

back p-queue

Merge remote-tracking branch 'origin/develop' into blazingly_fast_ini…

fb7f97d

…tial

feat: improve chunks grouping

da7d405

feat: bring back upsertOnFetch

b6dc216

feat: fetch txHistory *after* all are done

2b4defc

feat: split getAllTxHistory by AccountIds

2746284

gomesalexandre added the high risk label Sep 27, 2024

gomesalexandre added 2 commits September 27, 2024 14:44

fix: upsert only per-chain accounts

ed84af5

chore: rm logs and debugger

847369c

gomesalexandre marked this pull request as ready for review September 27, 2024 13:33

gomesalexandre commented Sep 27, 2024

View reviewed changes

src/components/Layout/Header/DegradedStateBanner.tsx Outdated Show resolved Hide resolved

gomesalexandre added 2 commits September 27, 2024 15:34

feat: revert DegradedStateBanner refetch failed

84303c3

feat: importAccounts revert upsertOnFetch

6df58ff

gomesalexandre commented Sep 27, 2024

View reviewed changes

src/components/ManageAccountsDrawer/components/ImportAccounts.tsx Outdated Show resolved Hide resolved

gomesalexandre commented Sep 27, 2024

View reviewed changes

src/context/AppProvider/AppContext.tsx Outdated Show resolved Hide resolved

feat: update comment

150ae0f

gomesalexandre force-pushed the blazingly_fast_initial branch from 35f1742 to 150ae0f Compare September 27, 2024 13:42

gomesalexandre commented Sep 27, 2024

View reviewed changes

gomesalexandre mentioned this pull request Sep 27, 2024

Wallet load time increasing #7814

Open

2 tasks

woodenfurniture requested changes Sep 27, 2024

View reviewed changes

gomesalexandre added 5 commits September 30, 2024 09:59

Merge branch 'develop' into blazingly_fast_initial

64e8c63

feat: rm paranoia

5a3b1ae

feat: opt-chain selector

ac172d0

feat: remove chunk terminology and fix grouping of UTXO scriptTypes

5c094cc

feat: set over array for cleanliness

81b6e9b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: blazingly fast initial accounts load #7822

feat: blazingly fast initial accounts load #7822

gomesalexandre commented Sep 26, 2024 •

edited

Loading

gomesalexandre Sep 27, 2024

gomesalexandre Sep 27, 2024

woodenfurniture Sep 27, 2024

gomesalexandre Sep 27, 2024

gomesalexandre Sep 27, 2024

woodenfurniture Sep 27, 2024

gomesalexandre Sep 27, 2024

gomesalexandre Sep 27, 2024

woodenfurniture Sep 27, 2024

woodenfurniture left a comment

woodenfurniture Sep 27, 2024

gomesalexandre Sep 30, 2024

woodenfurniture Sep 27, 2024

woodenfurniture Sep 27, 2024

woodenfurniture Sep 27, 2024

gomesalexandre Sep 30, 2024

woodenfurniture Sep 27, 2024

gomesalexandre Sep 30, 2024

woodenfurniture Sep 27, 2024

gomesalexandre Sep 30, 2024

woodenfurniture Sep 27, 2024

gomesalexandre Sep 30, 2024

woodenfurniture Sep 27, 2024

gomesalexandre Sep 30, 2024

woodenfurniture Sep 27, 2024

	const walletAccountIds = (walletId && walletById[walletId]) ?? []
	const walletAccountIds = walletById?.[walletId] ?? []

feat: blazingly fast initial accounts load #7822

Are you sure you want to change the base?

feat: blazingly fast initial accounts load #7822

Conversation

gomesalexandre commented Sep 26, 2024 • edited Loading

Description

Where to go from here

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodenfurniture left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gomesalexandre commented Sep 26, 2024 •

edited

Loading