Skip to content

Commit

Permalink
Fix brave/brave-ios#8482, brave/brave-ios#8519: NFT loading improveme…
Browse files Browse the repository at this point in the history 
…nt (brave/brave-ios#8551)
  • Loading branch information
@nuo-xu
nuo-xu committed Dec 13, 2023
1 parent 88c9ee0 commit c0992bb
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 41 deletions.
10 changes: 5 additions & 5 deletions Sources/BraveWallet/Crypto/NFT/NFTView.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down
20 changes: 17 additions & 3 deletions Sources/BraveWallet/Crypto/Stores/CryptoStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -576,6 +582,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 {
Expand Down
68 changes: 42 additions & 26 deletions Sources/BraveWallet/Crypto/Stores/NFTStore.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -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
}
Expand All @@ -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
Expand All @@ -315,7 +322,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
})
})
}

guard !Task.isCancelled else { return }
let (userNFTGroupsWithBalance, _) = buildNFTGroupModels(
groupBy: filters.groupBy,
Expand All @@ -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,
Expand All @@ -338,8 +345,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
selectedNetworks: selectedNetworks
)
self.userNFTGroups = userNFTGroupsWithMetadata

isShowingNFTLoadingState = false
}
}

Expand Down Expand Up @@ -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()
}
Expand All @@ -583,7 +602,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
isSpam: Bool,
isDeletedByUser: Bool
) {
isShowingNFTLoadingState = true
assetManager.updateUserAsset(
for: token,
visible: visible,
Expand All @@ -606,8 +624,6 @@ public class NFTStore: ObservableObject, WalletObserverStore {
selectedAccounts: selectedAccounts,
selectedNetworks: selectedNetworks
)

isShowingNFTLoadingState = false
}
}

Expand Down
15 changes: 8 additions & 7 deletions Tests/BraveWalletTests/NFTStoreTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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] = [
Expand Down

0 comments on commit c0992bb

Please sign in to comment.