Fix #8482, #8519: NFT loading improvement

Sources/BraveWallet/Crypto/NFT/NFTView.swift

@@ -114,12 +114,12 @@ struct NFTView: View {
           }
         }
         .pickerStyle(.inline)
-        .disabled(nftStore.isShowingNFTLoadingState)
+        .disabled(nftStore.isLoadingJunkNFTs)
       } label: {
         HStack(spacing: 12) {
           Text(nftStore.displayType.dropdownTitle)
             .font(.subheadline.weight(.semibold))
-          if !nftStore.isShowingNFTLoadingState {
+          if !nftStore.isLoadingJunkNFTs {
             Text("\(nftStore.totalDisplayedNFTCount)")
               .padding(.horizontal, 8)
               .padding(.vertical, 4)
@@ -141,9 +141,9 @@ struct NFTView: View {
       Spacer()
       addCustomAssetButton
         .padding(.trailing, 10)
-        .disabled(nftStore.isShowingNFTLoadingState)
+        .disabled(nftStore.isLoadingJunkNFTs)
       filtersButton
-        .disabled(nftStore.isShowingNFTLoadingState)
+        .disabled(nftStore.isLoadingJunkNFTs)
     }
     .padding(.horizontal)
     .frame(maxWidth: .infinity, alignment: .leading)
@@ -274,7 +274,7 @@ struct NFTView: View {
   var body: some View {
     LazyVStack(spacing: 16) {
       nftHeaderView
-      if nftStore.isShowingNFTLoadingState {
+      if nftStore.isLoadingJunkNFTs {
         SkeletonLoadingNFTView()
       } else if nftStore.isShowingNFTEmptyState {
         emptyView

Sources/BraveWallet/Crypto/Stores/CryptoStore.swift

@@ -259,11 +259,17 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
         // CoreData are added after.
         guard let self else { return }
         if !self.isUpdatingUserAssets {
+          let dispatchGroup = DispatchGroup()
           for asset in discoveredAssets {
-            self.userAssetManager.addUserAsset(asset, completion: nil)
+            dispatchGroup.enter()
+            self.userAssetManager.addUserAsset(asset) {
+              dispatchGroup.leave()
+            }
           }
-          if !discoveredAssets.isEmpty {
-            self.updateAssets()
+          dispatchGroup.notify(queue: .main) {
+            if !discoveredAssets.isEmpty {
+              self.updateAutoDiscoveredAssets()
+            }
           }
         } else {
           self.autoDiscoveredAssets.append(contentsOf: discoveredAssets)
@@ -552,6 +558,14 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
     nftStore.update()
   }
 
+  func updateAutoDiscoveredAssets() {
+    // at this point, all auto-discovered assets have been added to CD
+    // update `Portfolio/Assets`
+    portfolioStore.update()
+    // fetch junk NFTs from SimpleHash which will also update `Portfolio/NFTs`
+    nftStore.fetchJunkNFTs()
+  }
+
   func prepare(isInitialOpen: Bool = false) {
     Task { @MainActor in
       if isInitialOpen {

Sources/BraveWallet/Crypto/Stores/NFTStore.swift

@@ -133,8 +133,8 @@ public class NFTStore: ObservableObject, WalletObserverStore {
   @Published var displayType: NFTDisplayType = .visible
   /// View model for all NFT include visible, hidden and spams
   @Published private(set) var userNFTGroups: [NFTGroupViewModel] = []
-  /// showing shimmering loading state when the view finishes loading NFT information
-  @Published var isShowingNFTLoadingState: Bool = false
+  /// showing shimmering loading state when the view is fetching non-fungible tokens without fetching its metadata or balance
+  @Published var isLoadingJunkNFTs: Bool = false
 
   private let keyringService: BraveWalletKeyringService
   private let rpcService: BraveWalletJsonRpcService
@@ -152,7 +152,11 @@ public class NFTStore: ObservableObject, WalletObserverStore {
   /// Cache of metadata for NFTs. The key is the token's `id`.
   private var metadataCache: [String: NFTMetadata] = [:]
   /// Spam from SimpleHash in form of `NetworkAssets`
-  private var simpleHashSpamNFTs: [NetworkAssets] = []
+  private var simpleHashSpamNFTs: [NetworkAssets] = [] {
+    didSet {
+      update()
+    }
+  }
 
   var isObserving: Bool {
     rpcServiceObserver != nil && keyringServiceObserver != nil && walletServiveObserber != nil
@@ -249,7 +253,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
   private var nftBalancesCache: [String: [String: Int]] = [:]
 
   func update() {
-    self.isShowingNFTLoadingState = true
     self.updateTask?.cancel()
     self.updateTask = Task { @MainActor in
       self.allAccounts = await keyringService.allAccounts().accounts
@@ -261,14 +264,13 @@ public class NFTStore: ObservableObject, WalletObserverStore {
       let selectedAccounts = filters.accounts.filter(\.isSelected).map(\.model)
       let selectedNetworks = filters.networks.filter(\.isSelected).map(\.model)
 
-      // all spam NFTs marked by SimpleHash
-      simpleHashSpamNFTs = await walletService.simpleHashSpamNFTs(for: selectedAccounts, on: selectedNetworks)
+      // First display grids with placeholder since we haven't fetched balance
+      // or metadata
       let unionedSpamNFTs = computeSpamNFTs(
         selectedNetworks: selectedNetworks,
         selectedAccounts: selectedAccounts,
         simpleHashSpamNFTs: simpleHashSpamNFTs
       )
-
       let (userNFTGroups, allUserNFTs) = buildNFTGroupModels(
         groupBy: filters.groupBy,
         spams: unionedSpamNFTs,
@@ -277,13 +279,14 @@ public class NFTStore: ObservableObject, WalletObserverStore {
       )
       self.userNFTGroups = userNFTGroups
 
+      // Then, we fetch balance and update the UI
       // if we're not hiding unowned or grouping by account, balance isn't needed
       if filters.isHidingUnownedNFTs || filters.groupBy == .accounts {
         let allAccounts = filters.accounts.map(\.model)
         nftBalancesCache = await withTaskGroup(
           of: [String: [String: Int]].self,
           body: { @MainActor [nftBalancesCache, rpcService] group in
-            for nft in allUserNFTs { // for each NFT
+            for nft in allUserNFTs { // for all NFTs that have not yet been fetched its balance
               guard let networkForNFT = allNetworks.first(where: { $0.chainId == nft.chainId }) else {
                 continue
               }
@@ -292,13 +295,17 @@ public class NFTStore: ObservableObject, WalletObserverStore {
                   of: [String: Int].self,
                   body: { @MainActor group in
                     for account in allAccounts where account.coin == nft.coin {
-                      group.addTask { @MainActor in
-                        let balanceForToken = await rpcService.balance(
-                          for: nft,
-                          in: account,
-                          network: networkForNFT
-                        )
-                        return [account.address: Int(balanceForToken ?? 0)]
+                      if let cachedBalance = nftBalancesCache[nft.id]?[account.address] { // cached balance
+                        return [account.address: cachedBalance]
+                      } else { // no balance for this account
+                        group.addTask { @MainActor in
+                          let balanceForToken = await rpcService.balance(
+                            for: nft,
+                            in: account,
+                            network: networkForNFT
+                          )
+                          return [account.address: Int(balanceForToken ?? 0)]
+                        }
                       }
                     }
                     return await group.reduce(into: [String: Int](), { partialResult, new in
@@ -315,7 +322,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
             })
           })
       }
-
       guard !Task.isCancelled else { return }
       let (userNFTGroupsWithBalance, _) = buildNFTGroupModels(
         groupBy: filters.groupBy,
@@ -325,11 +331,12 @@ public class NFTStore: ObservableObject, WalletObserverStore {
       )
       self.userNFTGroups = userNFTGroupsWithBalance
 
-      // fetch nft metadata for all NFTs
-      let allMetadata = await rpcService.fetchNFTMetadata(tokens: allUserNFTs, ipfsApi: ipfsApi)
-      for (key, value) in allMetadata { // update cached values
-        metadataCache[key] = value
-      }
+      // Last, we fet fetch nft metadata for NFTs that do not have metadata loaded
+      // and update the UI
+      let nftsMissingMetadata = allUserNFTs.filter { metadataCache[$0.id] == nil }
+      let fetchedMetadata = await rpcService.fetchNFTMetadata(tokens: nftsMissingMetadata, ipfsApi: ipfsApi)
+      metadataCache.merge(with: fetchedMetadata)
+
       guard !Task.isCancelled else { return }
       let (userNFTGroupsWithMetadata, _) = buildNFTGroupModels(
         groupBy: filters.groupBy,
@@ -338,8 +345,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
         selectedNetworks: selectedNetworks
       )
       self.userNFTGroups = userNFTGroupsWithMetadata
-
-      isShowingNFTLoadingState = false
     }
   }
 
@@ -569,6 +574,20 @@ public class NFTStore: ObservableObject, WalletObserverStore {
     return (groups, allUserNFTs)
   }
 
+  func fetchJunkNFTs() {
+    Task { @MainActor in
+      // all spam NFTs marked by SimpleHash (for all accounts on all networks)
+      self.isLoadingJunkNFTs = true
+      let allAccounts = await keyringService.allAccounts().accounts
+        .filter { account in
+          WalletConstants.supportedCoinTypes().contains(account.coin)
+        }
+      let allNetworks = await rpcService.allNetworksForSupportedCoins()
+      self.simpleHashSpamNFTs = await walletService.simpleHashSpamNFTs(for: allAccounts, on: allNetworks)
+      self.isLoadingJunkNFTs = false
+    }
+  }
+
   @MainActor func isNFTDiscoveryEnabled() async -> Bool {
     await walletService.nftDiscoveryEnabled()
   }
@@ -583,7 +602,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
     isSpam: Bool,
     isDeletedByUser: Bool
   ) {
-    isShowingNFTLoadingState = true
     assetManager.updateUserAsset(
       for: token,
       visible: visible,
@@ -606,8 +624,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
         selectedAccounts: selectedAccounts,
         selectedNetworks: selectedNetworks
       )
-
-      isShowingNFTLoadingState = false
     }
   }
 

Tests/BraveWalletTests/NFTStoreTests.swift

@@ -121,7 +121,7 @@ class NFTStoreTests: XCTestCase {
     }
     let walletService = BraveWallet.TestBraveWalletService()
     walletService._addObserver = { _ in }
-    walletService._simpleHashSpamNfTs = {walletAddress, chainIds, coin, _, completion in
+    walletService._simpleHashSpamNfTs = { walletAddress, chainIds, coin, _, completion in
       if walletAddress == self.ethAccount1.address, chainIds.contains(BraveWallet.MainnetChainId), coin == .eth {
         completion([self.spamEthNFT], nil)
       } else {
@@ -414,7 +414,7 @@ class NFTStoreTests: XCTestCase {
   }
 
   // MARK: Group By `None`
-  func testUpdateSpamGroupOnlyFromSimpleHash() async {
+  func testSpamOnlyFromSimpleHash() async {
     let mockEthUserAssets: [BraveWallet.BlockchainToken] = [
       .previewToken.copy(asVisibleAsset: true),
       .mockUSDCToken.copy(asVisibleAsset: false), // Verify non-visible assets not displayed #6386
@@ -476,13 +476,13 @@ class NFTStoreTests: XCTestCase {
         XCTAssertEqual(spamNFTs[safe: 0]?.nftMetadata?.name, self.mockERC721Metadata.name)
       }.store(in: &cancellables)
 
-    store.update()
+    store.fetchJunkNFTs()
     await fulfillment(of: [userSpamNFTsException], timeout: 1)
     cancellables.removeAll()
   }
 
   // MARK: Group By `None`
-  func testUpdateSpamGroupFromSimpleHashAndUserMarked() async {
+  func testSpamFromSimpleHashAndUserMarked() async {
     let mockEthUserAssets: [BraveWallet.BlockchainToken] = [
       .previewToken.copy(asVisibleAsset: true),
       .mockUSDCToken.copy(asVisibleAsset: false), // Verify non-visible assets not displayed #6386
@@ -546,13 +546,13 @@ class NFTStoreTests: XCTestCase {
         XCTAssertEqual(spamNFTs[safe: 1]?.nftMetadata?.name, self.mockERC721Metadata.name)
       }.store(in: &cancellables)
 
-    store.update()
+    store.fetchJunkNFTs()
     await fulfillment(of: [userSpamNFTsException], timeout: 1)
     cancellables.removeAll()
   }
 
   // MARK: Group By `None`
-  func testUpdateSpamGroupDuplicationFromSimpleHashAndUserMarked() async {
+  func testSpamDuplicationFromSimpleHashAndUserMarked() async {
     let mockEthUserAssets: [BraveWallet.BlockchainToken] = [
       .previewToken.copy(asVisibleAsset: true),
       .mockUSDCToken.copy(asVisibleAsset: false), // Verify non-visible assets not displayed #6386
@@ -614,7 +614,7 @@ class NFTStoreTests: XCTestCase {
         XCTAssertEqual(spamNFTs[safe: 0]?.nftMetadata?.name, self.mockERC721Metadata.name)
       }.store(in: &cancellables)
 
-    store.update()
+    store.fetchJunkNFTs()
     await fulfillment(of: [userSpamNFTsException], timeout: 1)
     cancellables.removeAll()
   }
@@ -688,6 +688,7 @@ class NFTStoreTests: XCTestCase {
     await fulfillment(of: [groupByAccountVisibleExpectation], timeout: 1)
     cancellables.removeAll()
   }
+
   // MARK: Group by `Accounts` with `displayType`: `hidden`
   func testUpdateGroupByAccountsHiddenNFTs() async {
     let mockEthUserAssets: [BraveWallet.BlockchainToken] = [