Merge pull request #2205 from AmbireTech/hotfix/portfolio-reloading-state

Hotfix/ Portfolio discovery is done before loading is set to true

Author: superKalo

src/controllers/portfolio/portfolio.test.ts

@@ -229,23 +229,29 @@ const getMultipleAccountsLearnedAssets = () => {
   const tokenHints2 = generateRandomAddresses(10)
 
   const turnHintsToLearnedAssets = (hints: string[]) => {
-    return hints.reduce((acc, addr) => {
-      acc[addr] = Date.now()
+    return hints.reduce(
+      (acc, addr) => {
+        acc[addr] = Date.now()
 
-      return acc
-    }, {} as LearnedAssets['erc20s'][string])
+        return acc
+      },
+      {} as LearnedAssets['erc20s'][string]
+    )
   }
 
   const turnCollectionsToLearnedAssetKeys = (
     collections: [string, bigint[]][]
   ): LearnedAssets['erc721s'][string] => {
-    return collections.reduce((acc, nft) => {
-      erc721CollectionToLearnedAssetKeys(nft).forEach((key) => {
-        acc[key] = Date.now()
-      })
+    return collections.reduce(
+      (acc, nft) => {
+        erc721CollectionToLearnedAssetKeys(nft).forEach((key) => {
+          acc[key] = Date.now()
+        })
 
-      return acc
-    }, {} as LearnedAssets['erc721s'][string])
+        return acc
+      },
+      {} as LearnedAssets['erc721s'][string]
+    )
   }
 
   return {
@@ -412,22 +418,22 @@ describe('Portfolio Controller ', () => {
           new Promise((resolve) => {
             setTimeout(() => {
               queueOrder.push('updatePortfolioState - #1 call')
-              resolve(true)
+              resolve([true, null])
             }, 2000)
           })
       )
       .mockImplementationOnce(
         () =>
           new Promise((resolve) => {
             queueOrder.push('updatePortfolioState - #2 call')
-            resolve(true)
+            resolve([true, null])
           })
       )
       .mockImplementationOnce(
         () =>
           new Promise((resolve) => {
             queueOrder.push('updatePortfolioState - #3 call')
-            resolve(true)
+            resolve([true, null])
           })
       )
 
@@ -741,12 +747,15 @@ describe('Portfolio Controller ', () => {
       const firstBatchOf50 = generateRandomAddresses(50)
       const startingLearnedAssets: LearnedAssets = {
         erc20s: {
-          [`${1}:${account.addr}`]: firstBatchOf50.reduce((acc, addr, index) => {
-            // First 20 are still owned, last 30 are no longer owned
-            acc[addr] = index <= 20 ? Date.now() : Date.now() - 24 * 60 * 60 * 1000
-
-            return acc
-          }, {} as LearnedAssets['erc20s'][string])
+          [`${1}:${account.addr}`]: firstBatchOf50.reduce(
+            (acc, addr, index) => {
+              // First 20 are still owned, last 30 are no longer owned
+              acc[addr] = index <= 20 ? Date.now() : Date.now() - 24 * 60 * 60 * 1000
+
+              return acc
+            },
+            {} as LearnedAssets['erc20s'][string]
+          )
         },
         erc721s: {}
       }
@@ -771,35 +780,44 @@ describe('Portfolio Controller ', () => {
     test('To be learned erc721 cleanup mechanism works', async () => {
       // A total of 80 collections are added. 30 of them are "no longer owned"
       // but only 10 of them should be removed as the threshold of unowned is 20
-      const firstRandomCollections = generateRandomAddresses(50).reduce((acc, addr, index) => {
-        acc.push([addr, Math.random() < 0.2 ? [] : [BigInt(index)]] as [string, bigint[]])
+      const firstRandomCollections = generateRandomAddresses(50).reduce(
+        (acc, addr, index) => {
+          acc.push([addr, Math.random() < 0.2 ? [] : [BigInt(index)]] as [string, bigint[]])
 
-        return acc
-      }, [] as [string, bigint[]][])
+          return acc
+        },
+        [] as [string, bigint[]][]
+      )
 
       const keys = firstRandomCollections.map((c) => erc721CollectionToLearnedAssetKeys(c)).flat()
 
       const startingLearnedAssets: LearnedAssets = {
         erc20s: {},
         erc721s: {
-          [`${1}:${account.addr}`]: keys.reduce((acc, key, index) => {
-            // First 20 are still owned, last 30 are no longer owned
-            acc[key] = index <= 20 ? Date.now() : Date.now() - 24 * 60 * 60 * 1000
-
-            return acc
-          }, {} as LearnedAssets['erc721s'][string])
+          [`${1}:${account.addr}`]: keys.reduce(
+            (acc, key, index) => {
+              // First 20 are still owned, last 30 are no longer owned
+              acc[key] = index <= 20 ? Date.now() : Date.now() - 24 * 60 * 60 * 1000
+
+              return acc
+            },
+            {} as LearnedAssets['erc721s'][string]
+          )
         }
       }
 
       const { controller, storageCtrl } = await prepareTest((storageC) =>
         storageC.set('learnedAssets', startingLearnedAssets)
       )
 
-      const nextRandomCollections = generateRandomAddresses(30).reduce((acc, addr, index) => {
-        acc.push([addr, Math.random() < 0.2 ? [] : [BigInt(index)]] as [string, bigint[]])
+      const nextRandomCollections = generateRandomAddresses(30).reduce(
+        (acc, addr, index) => {
+          acc.push([addr, Math.random() < 0.2 ? [] : [BigInt(index)]] as [string, bigint[]])
 
-        return acc
-      }, [] as [string, bigint[]][])
+          return acc
+        },
+        [] as [string, bigint[]][]
+      )
 
       const allCurrentlyOwnedCollections = [
         ...firstRandomCollections.slice(0, 20),
@@ -1043,9 +1061,9 @@ describe('Portfolio Controller ', () => {
 
       const toBeLearnedToken = controller
         .getAccountPortfolioState(account.addr)
-        ['1']?.result?.tokens.find(
-          (token) => token.address === '0xA0b73E1Ff0B80914AB6fe0444E65848C4C34450b'
-        )
+        [
+          '1'
+        ]?.result?.tokens.find((token) => token.address === '0xA0b73E1Ff0B80914AB6fe0444E65848C4C34450b')
 
       expect(toBeLearnedToken).toBeTruthy()
 
@@ -1090,10 +1108,9 @@ describe('Portfolio Controller ', () => {
 
       const toBeLearnedToken = controller
         .getAccountPortfolioState(account2.addr)
-        ['137']?.result?.tokens.find(
-          (token) =>
-            token.address === '0xc2132D05D31c914a87C6611C10748AEb04B58e8F' && token.amount > 0n
-        )
+        [
+          '137'
+        ]?.result?.tokens.find((token) => token.address === '0xc2132D05D31c914a87C6611C10748AEb04B58e8F' && token.amount > 0n)
       expect(toBeLearnedToken).toBeTruthy()
 
       const key = `${137}:${account2.addr}`
@@ -1112,9 +1129,9 @@ describe('Portfolio Controller ', () => {
       networksCtrl.networks.forEach((network) => {
         const nativeToken = controller
           .getAccountPortfolioState(account.addr)
-          [network.chainId.toString()]?.result?.tokens.find(
-            (token) => token.address === ZeroAddress
-          )
+          [
+            network.chainId.toString()
+          ]?.result?.tokens.find((token) => token.address === ZeroAddress)
 
         if (!nativeToken) {
           console.error('Native token not found for network:', network.name)
@@ -1808,9 +1825,9 @@ describe('Portfolio Controller ', () => {
     const getCustomTokenFromPortfolio = () => {
       return controller
         .getAccountPortfolioState(account.addr)
-        ['1']?.result?.tokens.find(
-          (token) => token.address === customToken.address && token.chainId === customToken.chainId
-        )
+        [
+          '1'
+        ]?.result?.tokens.find((token) => token.address === customToken.address && token.chainId === customToken.chainId)
     }
 
     expect(tokenIsSet).toEqual(customToken)
@@ -1870,12 +1887,9 @@ describe('Portfolio Controller ', () => {
 
     const hiddenToken = controller
       .getAccountPortfolioState(account.addr)
-      ['1']?.result?.tokens.find(
-        (token) =>
-          token.address === preference.address &&
-          token.chainId === preference.chainId &&
-          token.flags.isHidden
-      )
+      [
+        '1'
+      ]?.result?.tokens.find((token) => token.address === preference.address && token.chainId === preference.chainId && token.flags.isHidden)
     expect(hiddenToken).toBeTruthy()
   })
   test('Calling toggleHideToken a second time deletes the preference', async () => {

src/controllers/portfolio/portfolio.ts

@@ -942,16 +942,17 @@ export class PortfolioController extends EventEmitter implements IPortfolioContr
     network: Network,
     portfolioLib: Portfolio | null,
     portfolioProps: Partial<GetOptions> & {
+      hasKeys: boolean
       maxDataAgeMs?: number
+      defiMaxDataAgeMs?: number
       isManualUpdate?: boolean
-    },
-    discoveryData: FormattedPortfolioDiscoveryResponse | null
-  ): Promise<boolean> {
+    }
+  ): Promise<[boolean, FormattedPortfolioDiscoveryResponse | null]> {
     const { maxDataAgeMs, isManualUpdate } = portfolioProps
     const accountState = this.#state[accountId]
 
     // Can occur if the account is removed while updateSelectedAccount is in progress
-    if (!accountState) return false
+    if (!accountState) return [false, null]
 
     if (!accountState[network.chainId.toString()]) {
       // isLoading must be false here, otherwise canSkipUpdate will return true
@@ -964,7 +965,7 @@ export class PortfolioController extends EventEmitter implements IPortfolioContr
       maxDataAgeMs
     )
 
-    if (canSkipUpdate) return false
+    if (canSkipUpdate) return [true, null]
 
     this.#setNetworkLoading(accountId, network.chainId.toString(), true)
     const state = accountState[network.chainId.toString()]!
@@ -984,13 +985,32 @@ export class PortfolioController extends EventEmitter implements IPortfolioContr
 
       const networkPriceCache = this.priceCache[network.chainId.toString()] || new Map()
 
+      const hintsResponse =
+        this.#state[accountId]?.[network.chainId.toString()]?.result?.lastExternalApiUpdateData
+      const discoveryData = await this.getPortfolioFromApiDiscovery({
+        chainId: network.chainId,
+        accountAddr: accountId,
+        baseCurrency: 'usd',
+        externalApiHintsResponse: hintsResponse || null,
+        isManualUpdate,
+        defiMaxDataAgeMs: portfolioProps?.defiMaxDataAgeMs,
+        hasKeys: portfolioProps.hasKeys
+      })
+      const allHints = this.getAllHints(
+        accountId,
+        network.chainId,
+        isManualUpdate,
+        discoveryData?.data?.hints
+      )
+
       // Fetch the portfolio and custom defi positions in parallel
       const [portfolioResult, customPositionsResult] = await Promise.all([
         portfolioLib.get(accountId, {
           priceRecency: 60000 * 5,
           priceCache: networkPriceCache,
           blockTag: 'both',
           fetchPinned: !hasNonZeroTokens,
+          ...allHints,
           ...portfolioProps,
           disableAutoDiscovery: true
         }),
@@ -1074,7 +1094,7 @@ export class PortfolioController extends EventEmitter implements IPortfolioContr
       }
 
       this.emitUpdate()
-      return true
+      return [true, discoveryData]
     } catch (e: any) {
       this.emitError({
         level: 'silent',
@@ -1099,7 +1119,7 @@ export class PortfolioController extends EventEmitter implements IPortfolioContr
       }
       this.emitUpdate()
 
-      return false
+      return [false, null]
     }
   }
 
@@ -1350,25 +1370,7 @@ export class PortfolioController extends EventEmitter implements IPortfolioContr
             paramsMaxDataAgeMsUnused
           )
 
-          const hintsResponse =
-            this.#state[accountId]?.[network.chainId.toString()]?.result?.lastExternalApiUpdateData
-          const discoveryResponse = await this.getPortfolioFromApiDiscovery({
-            chainId: network.chainId,
-            accountAddr: accountId,
-            baseCurrency: 'usd',
-            externalApiHintsResponse: hintsResponse || null,
-            isManualUpdate,
-            defiMaxDataAgeMs: opts?.defiMaxDataAgeMs,
-            hasKeys: this.#keystore.getAccountKeys(selectedAccount).length > 0
-          })
-          const allHints = this.getAllHints(
-            accountId,
-            network.chainId,
-            isManualUpdate,
-            discoveryResponse?.data?.hints
-          )
-
-          const isSuccessful = await this.updatePortfolioState(
+          const [isSuccessful, discoveryResponse] = await this.updatePortfolioState(
             accountId,
             network,
             portfolioLib,
@@ -1385,9 +1387,8 @@ export class PortfolioController extends EventEmitter implements IPortfolioContr
                   }
                 }),
               disableAutoDiscovery: true,
-              ...allHints
-            },
-            discoveryResponse
+              hasKeys: this.#keystore.getAccountKeys(selectedAccount).length > 0
+            }
           )
 
           // Learn tokens and nfts from the portfolio lib