Skip to content

Commit

Permalink
Fix brave/brave-ios#8435: NFT images failing to show in Activity tab …
Browse files Browse the repository at this point in the history
…Send transactions (brave/brave-ios#8436)

* Store optional `NFTMetadata` in `SendDetails` (Solana NFT), and `Eth721TransferDetails`. Fetch NFT metadata on Activity tab and use for NFT icon display.

* Update unit test to include nft transaction and verify NFTMetadata is fetched.
  • Loading branch information
StephenHeaps authored Nov 20, 2023
1 parent f44f07a commit 9929f91
Show file tree
Hide file tree
Showing 14 changed files with 231 additions and 47 deletions.
5 changes: 4 additions & 1 deletion Sources/BraveWallet/Crypto/AssetIconView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,10 @@ struct NFTIconView: View {
}

var body: some View {
NFTImageView(urlString: url?.absoluteString ?? "", isLoading: isLoadingMetadata) {
NFTImageView( // logo populated for auto-discovered NFTs
urlString: url?.absoluteString ?? token.logo,
isLoading: isLoadingMetadata
) {
LoadingNFTView(shimmer: false)
}
.cornerRadius(5)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ class AccountActivityStore: ObservableObject, WalletObserverStore {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetRatios,
nftMetadata: [:],
solEstimatedTxFee: solEstimatedTxFees[transaction.id],
currencyFormatter: currencyFormatter
)
Expand Down
1 change: 1 addition & 0 deletions Sources/BraveWallet/Crypto/Stores/AssetDetailStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ class AssetDetailStore: ObservableObject, WalletObserverStore {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetRatios,
nftMetadata: [:],
solEstimatedTxFee: solEstimatedTxFees[transaction.id],
currencyFormatter: self.currencyFormatter
)
Expand Down
1 change: 1 addition & 0 deletions Sources/BraveWallet/Crypto/Stores/CryptoStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
blockchainRegistry: blockchainRegistry,
txService: txService,
solTxManagerProxy: solTxManagerProxy,
ipfsApi: ipfsApi,
userAssetManager: userAssetManager
)
self.marketStore = .init(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ public class TransactionConfirmationStore: ObservableObject, WalletObserverStore
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetRatios,
nftMetadata: [:],
solEstimatedTxFee: solEstimatedTxFee,
currencyFormatter: currencyFormatter
) else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class TransactionDetailsStore: ObservableObject, WalletObserverStore {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetRatios,
nftMetadata: [:],
solEstimatedTxFee: solEstimatedTxFee,
currencyFormatter: currencyFormatter
) else {
Expand Down
44 changes: 44 additions & 0 deletions Sources/BraveWallet/Crypto/Stores/TransactionsActivityStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {

private var solEstimatedTxFeesCache: [String: UInt64] = [:]
private var assetPricesCache: [String: Double] = [:]
/// Cache of metadata for NFTs. The key is the token's `id`.
private var metadataCache: [String: NFTMetadata] = [:]

private let keyringService: BraveWalletKeyringService
private let rpcService: BraveWalletJsonRpcService
Expand All @@ -39,6 +41,7 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {
private let blockchainRegistry: BraveWalletBlockchainRegistry
private let txService: BraveWalletTxService
private let solTxManagerProxy: BraveWalletSolanaTxManagerProxy
private let ipfsApi: IpfsAPI
private let assetManager: WalletUserAssetManagerType
private var keyringServiceObserver: KeyringServiceObserver?
private var txServiceObserver: TxServiceObserver?
Expand All @@ -56,6 +59,7 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {
blockchainRegistry: BraveWalletBlockchainRegistry,
txService: BraveWalletTxService,
solTxManagerProxy: BraveWalletSolanaTxManagerProxy,
ipfsApi: IpfsAPI,
userAssetManager: WalletUserAssetManagerType
) {
self.keyringService = keyringService
Expand All @@ -65,6 +69,7 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {
self.blockchainRegistry = blockchainRegistry
self.txService = txService
self.solTxManagerProxy = solTxManagerProxy
self.ipfsApi = ipfsApi
self.assetManager = userAssetManager

self.setupObservers()
Expand Down Expand Up @@ -154,6 +159,7 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetPricesCache,
nftMetadata: metadataCache,
solEstimatedTxFees: solEstimatedTxFeesCache
)
guard !self.transactionSections.isEmpty else { return }
Expand All @@ -174,6 +180,42 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetPricesCache,
nftMetadata: metadataCache,
solEstimatedTxFees: solEstimatedTxFeesCache
)

let nftsWithoutMetadata = transactionSections.flatMap(\.transactions)
.compactMap { parsedTx in
switch parsedTx.details {
case .erc721Transfer(let details):
return details.fromToken
case .solSplTokenTransfer(let details):
if let fromToken = details.fromToken, fromToken.isNft {
return fromToken
}
return nil
default:
return nil
}
}
.filter { token in // filter out already fetched metadata
!metadataCache.keys.contains(where: { $0.caseInsensitiveCompare(token.contractAddress) == .orderedSame })
}
guard !Task.isCancelled, !nftsWithoutMetadata.isEmpty else { return }
// fetch nft metadata for all NFTs
let allMetadata = await rpcService.fetchNFTMetadata(tokens: nftsWithoutMetadata, ipfsApi: ipfsApi)
for (key, value) in allMetadata { // update cached values
metadataCache[key] = value
}
guard !Task.isCancelled else { return }
self.transactionSections = buildTransactionSections(
transactions: allTransactions,
networksForCoin: networksForCoin,
accountInfos: allAccountInfos,
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetPricesCache,
nftMetadata: metadataCache,
solEstimatedTxFees: solEstimatedTxFeesCache
)
}
Expand All @@ -186,6 +228,7 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {
userAssets: [BraveWallet.BlockchainToken],
allTokens: [BraveWallet.BlockchainToken],
assetRatios: [String: Double],
nftMetadata: [String: NFTMetadata],
solEstimatedTxFees: [String: UInt64]
) -> [TransactionSection] {
// Group transactions by day (only compare day/month/year)
Expand All @@ -211,6 +254,7 @@ class TransactionsActivityStore: ObservableObject, WalletObserverStore {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetRatios,
nftMetadata: nftMetadata,
solEstimatedTxFee: solEstimatedTxFees[transaction.id],
currencyFormatter: currencyFormatter,
decimalFormatStyle: .decimals(precision: 4)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ extension TransactionParser {
userAssets: [BraveWallet.BlockchainToken],
allTokens: [BraveWallet.BlockchainToken],
assetRatios: [String: Double],
nftMetadata: [String: NFTMetadata],
solEstimatedTxFee: UInt64?,
currencyFormatter: NumberFormatter
) -> TransactionSummary {
Expand All @@ -54,6 +55,7 @@ extension TransactionParser {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetRatios,
nftMetadata: nftMetadata,
solEstimatedTxFee: solEstimatedTxFee,
currencyFormatter: currencyFormatter,
decimalFormatStyle: .balance // use 4 digit precision for summary
Expand Down
36 changes: 32 additions & 4 deletions Sources/BraveWallet/Crypto/Transactions/TransactionParser.swift
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ enum TransactionParser {
gasFee = .init(fee: gasFeeString, fiat: "$0.00")
}
}
case .btc:
case .btc, .zec:
break
@unknown default:
break
Expand Down Expand Up @@ -103,6 +103,7 @@ enum TransactionParser {
userAssets: [BraveWallet.BlockchainToken],
allTokens: [BraveWallet.BlockchainToken],
assetRatios: [String: Double],
nftMetadata: [String: NFTMetadata],
solEstimatedTxFee: UInt64?,
currencyFormatter: NumberFormatter,
decimalFormatStyle: WeiFormatter.DecimalFormatStyle? = nil
Expand Down Expand Up @@ -185,6 +186,7 @@ enum TransactionParser {
fromValue: fromValue,
fromAmount: fromValueFormatted,
fromFiat: fromFiat,
fromTokenMetadata: nil,
gasFee: gasFee(
from: transaction,
network: network,
Expand Down Expand Up @@ -230,6 +232,7 @@ enum TransactionParser {
fromValue: fromValue,
fromAmount: fromAmount,
fromFiat: fromFiat,
fromTokenMetadata: nil,
gasFee: gasFee(
from: transaction,
network: network,
Expand Down Expand Up @@ -359,6 +362,12 @@ enum TransactionParser {
return nil
}
let token = token(for: tokenContractAddress, network: network, userAssets: userAssets, allTokens: allTokens)
let tokenNFTMetadata: NFTMetadata?
if let token {
tokenNFTMetadata = nftMetadata[token.id]
} else {
tokenNFTMetadata = nil
}

return .init(
transaction: transaction,
Expand All @@ -372,6 +381,7 @@ enum TransactionParser {
fromToken: token,
fromValue: "1", // Can only send 1 erc721 at a time
fromAmount: "1",
nftMetadata: tokenNFTMetadata,
owner: owner,
tokenId: tokenId,
gasFee: gasFee(
Expand Down Expand Up @@ -413,6 +423,7 @@ enum TransactionParser {
fromValue: fromValue,
fromAmount: fromValueFormatted,
fromFiat: fromFiat,
fromTokenMetadata: nil,
gasFee: gasFee(
from: transaction,
network: network,
Expand All @@ -431,6 +442,12 @@ enum TransactionParser {
return nil
}
let fromToken = token(for: splTokenMintAddress, network: network, userAssets: userAssets, allTokens: allTokens)
let tokenNFTMetadata: NFTMetadata?
if let fromToken {
tokenNFTMetadata = nftMetadata[fromToken.id]
} else {
tokenNFTMetadata = nil
}
let fromValue = "\(amount)"
var fromValueFormatted = ""
var fromFiat = "$0.00"
Expand Down Expand Up @@ -464,6 +481,7 @@ enum TransactionParser {
fromValue: fromValue,
fromAmount: fromValueFormatted,
fromFiat: fromFiat,
fromTokenMetadata: tokenNFTMetadata,
gasFee: gasFee(
from: transaction,
network: network,
Expand Down Expand Up @@ -696,7 +714,9 @@ struct ParsedTransaction: Equatable {
case let .solDappTransaction(details),
let .solSwapTransaction(details):
return details.gasFee
case .erc721Transfer, .other:
case let .erc721Transfer(details):
return details.gasFee
case .other:
return nil
case let .filSend(details):
return details.gasFee
Expand Down Expand Up @@ -801,6 +821,8 @@ struct SendDetails: Equatable {
let fromAmount: String
/// The amount formatted as currency
let fromFiat: String?
/// Metadata if `fromToken` is an NFT
let fromTokenMetadata: NFTMetadata?

/// Gas fee for the transaction
let gasFee: GasFee?
Expand Down Expand Up @@ -836,6 +858,8 @@ struct Eth721TransferDetails: Equatable {
let fromValue: String
/// From amount formatted
let fromAmount: String
/// Metadata for the NFT being sent
let nftMetadata: NFTMetadata?

/// Owner (must not be confused with the caller (fromAddress)
let owner: String
Expand Down Expand Up @@ -906,6 +930,7 @@ extension BraveWallet.TransactionInfo {
userAssets: [BraveWallet.BlockchainToken],
allTokens: [BraveWallet.BlockchainToken],
assetRatios: [String: Double],
nftMetadata: [String: NFTMetadata],
solEstimatedTxFee: UInt64? = nil,
currencyFormatter: NumberFormatter,
decimalFormatStyle: WeiFormatter.DecimalFormatStyle? = nil
Expand All @@ -917,6 +942,7 @@ extension BraveWallet.TransactionInfo {
userAssets: userAssets,
allTokens: allTokens,
assetRatios: assetRatios,
nftMetadata: nftMetadata,
solEstimatedTxFee: solEstimatedTxFee,
currencyFormatter: currencyFormatter,
decimalFormatStyle: decimalFormatStyle
Expand Down Expand Up @@ -984,11 +1010,13 @@ extension BraveWallet.TransactionInfo {
if let erc721ContractAddress {
return [erc721ContractAddress]
}
case .solanaSplTokenTransfer, .solanaSplTokenTransferWithAssociatedTokenAccountCreation:
if let splTokenMintAddress = txDataUnion.solanaTxData?.splTokenMintAddress {
return [splTokenMintAddress]
}
case .ethSend, .erc1155SafeTransferFrom, .other:
break
case .solanaSystemTransfer,
.solanaSplTokenTransfer,
.solanaSplTokenTransferWithAssociatedTokenAccountCreation,
.solanaDappSignTransaction,
.solanaDappSignAndSendTransaction,
.solanaSwap:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ struct TransactionSummaryViewContainer: View {
SendTransactionSummaryView(
sentFromAccountName: parsedTransaction.namedFromAddress,
token: details.fromToken,
nftMetadata: details.fromTokenMetadata,
network: parsedTransaction.network,
valueSent: details.fromAmount,
fiatValueSent: details.fromFiat ?? "",
Expand Down Expand Up @@ -67,6 +68,7 @@ struct TransactionSummaryViewContainer: View {
SendTransactionSummaryView(
sentFromAccountName: parsedTransaction.namedFromAddress,
token: details.fromToken,
nftMetadata: details.nftMetadata,
network: parsedTransaction.network,
valueSent: nil,
fiatValueSent: nil,
Expand Down Expand Up @@ -101,6 +103,7 @@ struct SendTransactionSummaryView: View {

let sentFromAccountName: String
let token: BraveWallet.BlockchainToken?
let nftMetadata: NFTMetadata?
let network: BraveWallet.NetworkInfo
let valueSent: String?
let fiatValueSent: String?
Expand All @@ -110,13 +113,15 @@ struct SendTransactionSummaryView: View {
init(
sentFromAccountName: String,
token: BraveWallet.BlockchainToken?,
nftMetadata: NFTMetadata? = nil,
network: BraveWallet.NetworkInfo,
valueSent: String?,
fiatValueSent: String?,
status: BraveWallet.TransactionStatus,
time: Date
) {
self.sentFromAccountName = sentFromAccountName
self.nftMetadata = nftMetadata
self.token = token
self.network = network
self.valueSent = valueSent
Expand Down Expand Up @@ -162,7 +167,7 @@ struct SendTransactionSummaryView: View {
NFTIconView(
token: token,
network: network,
url: nil,
url: nftMetadata?.imageURL,
shouldShowNetworkIcon: true,
length: length,
maxLength: maxLength,
Expand Down
Loading

0 comments on commit 9929f91

Please sign in to comment.