fix: wallet, portfolio and tx management (#1520)

* fix: only subscribe to txs when portfolio is loaded

* fix: cleanup effects and logging

* fix: don't reset account specifiers to same new object by reference, causing erroneous portfolio clear

* refactor: subscribe all account types simultaneously

* fix: tx history unsubscribe logic, tx loaded delay, rename typod function

* chore: comments around logging

* fix: reorder transactions and portfolio providers, fixes logic/race issue

* fix: add logic and logging to slices on clear and set actions

* fix: logic entirely working for switching/disconnecting, needs tidying

* fix: fix re-subscription problem and tidy

* chore: rename PortfolioProvider to AppProvider

* chore: add comment to weird data structure

* test: add selectIsPortfolioLoaded test

* test: fix log spew

src/AppProviders.tsx

@@ -5,12 +5,12 @@ import { Provider as ReduxProvider } from 'react-redux'
 import { HashRouter } from 'react-router-dom'
 import { PersistGate } from 'redux-persist/integration/react'
 import { ScrollToTop } from 'Routes/ScrollToTop'
+import { AppProvider } from 'context/AppProvider/AppContext'
 import { BrowserRouterProvider } from 'context/BrowserRouterProvider/BrowserRouterProvider'
 import { I18nProvider } from 'context/I18nProvider/I18nProvider'
 import { MarketDataProvider } from 'context/MarketDataProvider/MarketDataProvider'
 import { ModalProvider } from 'context/ModalProvider/ModalProvider'
 import { PluginProvider } from 'context/PluginProvider/PluginProvider'
-import { PortfolioProvider } from 'context/PortfolioProvider/PortfolioContext'
 import { TransactionsProvider } from 'context/TransactionsProvider/TransactionsProvider'
 import { KeepKeyProvider } from 'context/WalletProvider/KeepKeyProvider'
 import { WalletProvider } from 'context/WalletProvider/WalletProvider'
@@ -36,13 +36,13 @@ export function AppProviders({ children }: ProvidersProps) {
                   <WalletProvider>
                     <KeepKeyProvider>
                       <ModalProvider>
-                        <PortfolioProvider>
-                          <MarketDataProvider>
-                            <TransactionsProvider>
+                        <TransactionsProvider>
+                          <AppProvider>
+                            <MarketDataProvider>
                               <DefiManagerProvider>{children}</DefiManagerProvider>
-                            </TransactionsProvider>
-                          </MarketDataProvider>
-                        </PortfolioProvider>
+                            </MarketDataProvider>
+                          </AppProvider>
+                        </TransactionsProvider>
                       </ModalProvider>
                     </KeepKeyProvider>
                   </WalletProvider>

src/context/PortfolioProvider/PortfolioContext.tsx → src/context/AppProvider/AppContext.tsx

@@ -51,7 +51,7 @@ import { deserializeUniqueTxId } from 'state/slices/txHistorySlice/utils'
  * for some time as reselect does a really good job of memoizing things
  *
  */
-export const PortfolioProvider = ({ children }: { children: React.ReactNode }) => {
+export const AppProvider = ({ children }: { children: React.ReactNode }) => {
   const dispatch = useDispatch()
   const { chainAdapterManager, supportedChains } = usePlugins()
   const {
@@ -76,20 +76,36 @@ export const PortfolioProvider = ({ children }: { children: React.ReactNode }) =
   // once the wallet is connected, reach out to unchained to fetch
   // accounts for each chain/account specifier combination
   useEffect(() => {
-    if (isEmpty(accountSpecifiersList)) return
-    // clear the old portfolio, we have different non null data, we're switching wallet
-    console.info('dispatching portfolio clear action')
-    dispatch(portfolio.actions.clear())
+    const { getAccount } = portfolioApi.endpoints
+    // forceRefetch is enabled here to make sure that we always have the latest wallet information
+    // it also forces queryFn to run and that's needed for the wallet info to be dispatched
+    const options = { forceRefetch: true }
     // fetch each account
-    accountSpecifiersList.forEach(accountSpecifierMap => {
-      // forceRefetch is enabled here to make sure that we always have the latest wallet information
-      // it also forces queryFn to run and that's needed for the wallet info to be dispatched
-      dispatch(
-        portfolioApi.endpoints.getAccount.initiate({ accountSpecifierMap }, { forceRefetch: true }),
-      )
-    })
+    accountSpecifiersList.forEach(accountSpecifierMap =>
+      dispatch(getAccount.initiate({ accountSpecifierMap }, options)),
+    )
   }, [dispatch, accountSpecifiersList])
 
+  /**
+   * handle wallet disconnect/switch logic
+   */
+  useEffect(() => {
+    // if we have a wallet and changed account specifiers, we have switched wallets
+    // NOTE! - the wallet will change before the account specifiers does, so clearing here is valid
+    // check the console logs in the browser for the ordering of actions to verify this logic
+    const switched = Boolean(wallet && !isEmpty(accountSpecifiersList))
+    const disconnected = !wallet
+    // TODO(0xdef1cafe): keep this - change to structured debug logging
+    switched && console.info('AppContext: wallet switched')
+    disconnected && console.info('AppContext: wallet disconnected')
+    if (switched || disconnected) {
+      dispatch(accountSpecifiers.actions.clear())
+      dispatch(portfolio.actions.clear())
+    }
+    // this effect changes accountSpecifiersList, don't create infinite loop
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [dispatch, wallet])
+
   /**
    * this was previously known as the useAccountSpecifiers hook
    * this has recently been moved into redux state, as hooks are not singletons,
@@ -102,8 +118,8 @@ export const PortfolioProvider = ({ children }: { children: React.ReactNode }) =
    * break this at your peril
    */
   useEffect(() => {
-    if (!wallet) return
     if (isEmpty(assetsById)) return
+    if (!wallet) return
     ;(async () => {
       try {
         const acc: AccountSpecifierMap[] = []

src/context/PluginProvider/PluginProvider.tsx

@@ -105,6 +105,8 @@ export const PluginProvider = ({ children }: PluginProviderProps): JSX.Element =
       {
         add: chain => {
           const factory = newChainAdapters[chain]
+          // TODO(0xdef1cafe): leave this here, change to debug logging
+          console.info('PluginProvider: adding chain', chain)
           if (factory) getChainAdapters().addChain(chain, factory)
         },
         remove: chain => {
@@ -115,6 +117,8 @@ export const PluginProvider = ({ children }: PluginProviderProps): JSX.Element =
     )
 
     setRoutes(pluginRoutes)
+    // TODO(0xdef1cafe): leave this here, change to debug logging
+    console.info('PluginProvider: setting supported chains')
     setSupportedChains(getChainAdapters().getSupportedChains())
   }, [chainAdapterManager, featureFlags, plugins, pluginManager])
 

src/context/TransactionsProvider/TransactionsProvider.tsx

@@ -1,17 +1,19 @@
 import { CAIP2 } from '@shapeshiftoss/caip'
 import { utxoAccountParams } from '@shapeshiftoss/chain-adapters'
 import isEmpty from 'lodash/isEmpty'
-import React, { useCallback, useEffect } from 'react'
+import React, { useCallback, useEffect, useState } from 'react'
 import { useDispatch, useSelector } from 'react-redux'
 import { usePlugins } from 'context/PluginProvider/PluginProvider'
 import { useWallet } from 'hooks/useWallet/useWallet'
-import { walletSupportChain } from 'hooks/useWalletSupportsChain/useWalletSupportsChain'
+import { walletSupportsChain } from 'hooks/useWalletSupportsChain/useWalletSupportsChain'
 import { AccountSpecifierMap } from 'state/slices/accountSpecifiersSlice/accountSpecifiersSlice'
 import { supportedAccountTypes } from 'state/slices/portfolioSlice/portfolioSliceCommon'
+import { chainIdToFeeAssetId } from 'state/slices/portfolioSlice/utils'
 import {
   selectAccountIdByAddress,
   selectAccountSpecifiers,
   selectAssets,
+  selectIsPortfolioLoaded,
   selectPortfolioAssetIds,
   selectTxHistoryStatus,
   selectTxIds,
@@ -30,9 +32,11 @@ export const TransactionsProvider = ({ children }: TransactionsProviderProps): J
     state: { wallet, walletInfo },
   } = useWallet()
   const { chainAdapterManager, supportedChains } = usePlugins()
+  const [isSubscribed, setIsSubscribed] = useState<boolean>(false)
   const assets = useSelector(selectAssets)
   const portfolioAssetIds = useSelector(selectPortfolioAssetIds)
   const accountSpecifiers = useSelector(selectAccountSpecifiers)
+  const isPortfolioLoaded = useSelector(selectIsPortfolioLoaded)
   const txHistoryStatus = useSelector(selectTxHistoryStatus)
   const txIds = useAppSelector(selectTxIds)
 
@@ -47,90 +51,106 @@ export const TransactionsProvider = ({ children }: TransactionsProviderProps): J
     [accountSpecifiers],
   )
 
+  /**
+   * tx history unsubscribe and cleanup logic
+   */
+  useEffect(() => {
+    // account specifiers changing will trigger this effect
+    // we've disconnected/switched a wallet, unsubscribe from tx history and clear tx history
+    if (!isSubscribed) return
+    console.info('TransactionsProvider: unsubscribing from tx history')
+    supportedChains.forEach(chain => chainAdapterManager.byChain(chain).unsubscribeTxs())
+    dispatch(txHistory.actions.clear())
+    setIsSubscribed(false)
+    // setting isSubscribed to false will trigger this effect
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [accountSpecifiers, dispatch, chainAdapterManager, supportedChains])
+
+  /**
+   * tx history subscription logic
+   */
   useEffect(() => {
     if (!wallet) return
     if (isEmpty(assets)) return
-    ;(async () => {
-      for (const chain of supportedChains) {
-        const adapter = chainAdapterManager.byChain(chain)
-        const chainId = adapter.getCaip2()
-        if (!walletSupportChain({ chainId, wallet })) continue
-
-        const asset = Object.values(assets).find(asset => asset.caip2 === chainId)
-        if (!asset) {
-          throw new Error(`asset not found for chain ${chain}`)
-        }
-
-        const accountTypes = supportedAccountTypes[chain] ?? [undefined]
+    if (!isPortfolioLoaded) return // wait for all chain portfolios to be loaded before subscribing
+    if (isSubscribed) return // don't resubscribe
+    ;(async () =>
+      Promise.all(
+        supportedChains
+          .filter(chain => {
+            const chainId = chainAdapterManager.byChain(chain).getCaip2()
+            return walletSupportsChain({ chainId, wallet })
+          })
+          .map(async chain => {
+            const adapter = chainAdapterManager.byChain(chain)
+            const chainId = adapter.getCaip2()
 
-        // TODO(0xdef1cafe) - once we have restful tx history for all coinstacks
-        // this state machine should be removed, and managed by the txHistory RTK query api
-        dispatch(txHistory.actions.setStatus('loading'))
-        for await (const accountType of accountTypes) {
-          const accountParams = accountType ? utxoAccountParams(asset, accountType, 0) : {}
-          try {
-            await adapter.subscribeTxs(
-              { wallet, accountType, ...accountParams },
-              msg => {
-                const caip10 = `${msg.caip2}:${msg.address}`
-                const state = store.getState()
-                const accountId = selectAccountIdByAddress(state, caip10)
-                dispatch(
-                  txHistory.actions.onMessage({
-                    message: { ...msg, accountType },
-                    accountSpecifier: accountId,
-                  }),
-                )
-              },
-              (err: any) => console.error(err),
-            )
-          } catch (e) {
-            console.error(
-              `TransactionProvider: Error subscribing to transaction history for chain: ${chain}, accountType: ${accountType}`,
-              e,
-            )
-          }
-        }
+            // assets are known to be defined at this point - if we don't have the fee asset we have bigger problems
+            const asset = assets[chainIdToFeeAssetId(chainId)]
+            const accountTypes = supportedAccountTypes[chain]
 
-        // RESTfully fetch all tx and rebase history for this chain.
-        const chainAccountSpecifiers = getAccountSpecifiersByChainId(chainId)
-        if (isEmpty(chainAccountSpecifiers)) continue
-        chainAccountSpecifiers.forEach(accountSpecifierMap => {
-          const { getAllTxHistory, getFoxyRebaseHistoryByAccountId } = txHistoryApi.endpoints
-          const options = { forceRefetch: true }
-          dispatch(getAllTxHistory.initiate({ accountSpecifierMap }, options))
+            // TODO(0xdef1cafe) - once we have restful tx history for all coinstacks
+            // this state machine should be removed, and managed by the txHistory RTK query api
+            dispatch(txHistory.actions.setStatus('loading'))
+            try {
+              await Promise.all(
+                accountTypes.map(async accountType => {
+                  const accountParams = accountType ? utxoAccountParams(asset, accountType, 0) : {}
+                  console.info('subscribing txs for', chainId, accountType)
+                  return adapter.subscribeTxs(
+                    { wallet, accountType, ...accountParams },
+                    msg => {
+                      const caip10 = `${msg.caip2}:${msg.address}`
+                      const state = store.getState()
+                      const accountId = selectAccountIdByAddress(state, caip10)
+                      dispatch(
+                        txHistory.actions.onMessage({
+                          message: { ...msg, accountType },
+                          accountSpecifier: accountId,
+                        }),
+                      )
+                    },
+                    (err: any) => console.error(err),
+                  )
+                }),
+              )
+              // manage subscription state - we can't request this from chain adapters,
+              // and need this to prevent resubscribing when switching wallets
+              setIsSubscribed(true)
+            } catch (e: unknown) {
+              console.error(
+                `TransactionProvider: Error subscribing to transaction history for chain: ${chain}`,
+                e,
+              )
+            }
 
-          /**
-           * foxy rebase history is most closely linked to transactions.
-           * unfortunately, we have to call this for a specific asset here
-           * because we need it for the dashboard balance chart
-           *
-           * if you're reading this and are about to add another rebase token here,
-           * stop, and make a getRebaseHistoryByAccountId that takes
-           * an accountId and assetId[] in the txHistoryApi
-           */
+            // RESTfully fetch all tx and rebase history for this chain.
+            getAccountSpecifiersByChainId(chainId).forEach(accountSpecifierMap => {
+              const { getAllTxHistory, getFoxyRebaseHistoryByAccountId } = txHistoryApi.endpoints
+              const options = { forceRefetch: true }
+              dispatch(getAllTxHistory.initiate({ accountSpecifierMap }, options))
 
-          // fetch all rebase history for FOXy
-          if (!portfolioAssetIds.length) return // can't fetch without portfolio assets
-          const payload = { accountSpecifierMap, portfolioAssetIds }
-          dispatch(getFoxyRebaseHistoryByAccountId.initiate(payload, options))
-        })
-      }
-    })()
+              /**
+               * foxy rebase history is most closely linked to transactions.
+               * unfortunately, we have to call this for a specific asset here
+               * because we need it for the dashboard balance chart
+               *
+               * if you're reading this and are about to add another rebase token here,
+               * stop, and make a getRebaseHistoryByAccountId that takes
+               * an accountId and assetId[] in the txHistoryApi
+               */
 
-    return () => {
-      dispatch(txHistory.actions.clear())
-      supportedChains.forEach(chain => {
-        try {
-          const adapter = chainAdapterManager.byChain(chain)
-          adapter.unsubscribeTxs()
-        } catch (e) {
-          console.error('TransactionsProvider: Error unsubscribing from transaction history', e)
-        }
-      })
-    }
+              // fetch all rebase history for FOXy
+              const payload = { accountSpecifierMap, portfolioAssetIds }
+              dispatch(getFoxyRebaseHistoryByAccountId.initiate(payload, options))
+            })
+          }),
+      ))()
+    // assets causes unnecessary renders, but doesn't actually change
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [
-    assets,
+    isPortfolioLoaded,
+    isSubscribed,
     dispatch,
     walletInfo?.deviceId,
     wallet,
@@ -160,7 +180,7 @@ export const TransactionsProvider = ({ children }: TransactionsProviderProps): J
     if (isEmpty(assets)) return
     if (!walletInfo?.deviceId) return // we can't be loaded if the wallet isn't connected
     if (txHistoryStatus !== 'loading') return // only start logic below once we know we're loading
-    const TX_DEBOUNCE_DELAY = 10000
+    const TX_DEBOUNCE_DELAY = 5000
     const timer = setTimeout(
       () => dispatch(txHistory.actions.setStatus('loaded')),
       TX_DEBOUNCE_DELAY,

src/hooks/useWalletSupportsChain/useWalletSupportsChain.ts

@@ -12,7 +12,7 @@ type UseWalletSupportsChainArgs = { chainId: CAIP2; wallet: HDWallet | null }
 type UseWalletSupportsChain = (args: UseWalletSupportsChainArgs) => boolean
 
 // use outside react
-export const walletSupportChain: UseWalletSupportsChain = ({ chainId, wallet }) => {
+export const walletSupportsChain: UseWalletSupportsChain = ({ chainId, wallet }) => {
   if (!wallet) return false
   const ethCAIP2 = caip2.toCAIP2({ chain: ChainTypes.Ethereum, network: NetworkTypes.MAINNET })
   const btcCAIP2 = caip2.toCAIP2({ chain: ChainTypes.Bitcoin, network: NetworkTypes.MAINNET })
@@ -46,4 +46,4 @@ export const walletSupportChain: UseWalletSupportsChain = ({ chainId, wallet })
 }
 
 // TODO(0xdef1cafe): this whole thing should belong in chain adapters
-export const useWalletSupportsChain: UseWalletSupportsChain = args => walletSupportChain(args)
+export const useWalletSupportsChain: UseWalletSupportsChain = args => walletSupportsChain(args)

src/state/slices/accountSpecifiersSlice/accountSpecifiersSlice.ts

@@ -1,5 +1,6 @@
 import { createSlice } from '@reduxjs/toolkit'
 import { CAIP2 } from '@shapeshiftoss/caip'
+import isEqual from 'lodash/isEqual'
 
 // an account specifier is an x/y/zpub, or eth public key
 // as consumed by unchained, note this is *not* a CAIP10
@@ -19,8 +20,14 @@ export const accountSpecifiers = createSlice({
   name: 'accountSpecifiers',
   initialState: getInitialState(),
   reducers: {
-    clear: getInitialState,
+    clear: () => {
+      console.info('accountSpecifiersSlice: clearing account specifiers')
+      return getInitialState()
+    },
     setAccountSpecifiers(state, { payload }: { payload: AccountSpecifierMap[] }) {
+      // don't set to exactly the same thing and cause renders
+      if (isEqual(state.accountSpecifiers, payload)) return
+      console.info('accountSpecifiersSlice: dispatching account specifiers set action')
       state.accountSpecifiers = payload
     },
   },

src/state/slices/accountSpecifiersSlice/selectors.ts

@@ -1,8 +1,12 @@
 import { CAIP2 } from '@shapeshiftoss/caip'
 import { ReduxState } from 'state/reducer'
 
-export const selectAccountSpecifiers = (state: ReduxState) =>
-  state.accountSpecifiers.accountSpecifiers
+import { createDeepEqualOutputSelector } from './../../selector-utils'
+
+export const selectAccountSpecifiers = createDeepEqualOutputSelector(
+  (state: ReduxState) => state.accountSpecifiers.accountSpecifiers,
+  accountSpecifiers => accountSpecifiers,
+)
 
 // returns an array of the full `caip2:pubkeyish` type, as used in URLs for account pages
 export const selectAccountSpecifierStrings = (state: ReduxState) =>