Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(wallet): Bitcoin Balance Fetching #23117

Merged
merged 3 commits into from
Apr 22, 2024
Merged

feat(wallet): Bitcoin Balance Fetching #23117

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04c2f87
bitcoin balance fetching
nuo-xu Apr 16, 2024
69fcc8d
migrate some caching value due to empty address for bitcoin accounts.…
nuo-xu Apr 19, 2024
437d5d8
remove duplicated dictionary merging
nuo-xu Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ios/brave-ios/Sources/BraveWallet/Crypto/FiltersDisplaySettingsView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ struct Filters {
Preferences.Wallet.nonSelectedAccountsFilter.value =
accounts
.filter({ !$0.isSelected })
.map(\.model.address)
.map(\.model.filterAccountKey)
Preferences.Wallet.nonSelectedNetworksFilter.value =
networks
.filter({ !$0.isSelected })
Expand Down
26 changes: 22 additions & 4 deletions ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AccountActivityStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class AccountActivityStore: ObservableObject, WalletObserverStore {
private let blockchainRegistry: BraveWalletBlockchainRegistry
private let solTxManagerProxy: BraveWalletSolanaTxManagerProxy
private let ipfsApi: IpfsAPI
private let bitcoinWalletService: BraveWalletBitcoinWalletService
private let assetManager: WalletUserAssetManagerType
/// Cache for storing `BlockchainToken`s that are not in user assets or our token registry.
/// This could occur with a dapp creating a transaction.
Expand Down Expand Up @@ -75,6 +76,7 @@ class AccountActivityStore: ObservableObject, WalletObserverStore {
blockchainRegistry: BraveWalletBlockchainRegistry,
solTxManagerProxy: BraveWalletSolanaTxManagerProxy,
ipfsApi: IpfsAPI,
bitcoinWalletService: BraveWalletBitcoinWalletService,
userAssetManager: WalletUserAssetManagerType
) {
self.account = account
Expand All @@ -88,6 +90,7 @@ class AccountActivityStore: ObservableObject, WalletObserverStore {
self.blockchainRegistry = blockchainRegistry
self.solTxManagerProxy = solTxManagerProxy
self.ipfsApi = ipfsApi
self.bitcoinWalletService = bitcoinWalletService
self.assetManager = userAssetManager
self._isSwapSupported = .init(
wrappedValue: account.coin == .eth || account.coin == .sol
Expand Down Expand Up @@ -223,10 +226,25 @@ class AccountActivityStore: ObservableObject, WalletObserverStore {
)

self.isLoadingAccountFiat = true
let tokenBalances = await self.rpcService.fetchBalancesForTokens(
account: account,
networkAssets: allUserNetworkAssets
)
var tokenBalances: [String: Double] = [:]
if account.coin == .btc {
let networkAsset = allUserNetworkAssets.first {
$0.network.supportedKeyrings.contains(account.keyringId.rawValue as NSNumber)
}
if let btc = networkAsset?.tokens.first,
let btcBalance = await self.bitcoinWalletService.fetchBTCBalance(
accountId: account.accountId
)
{
tokenBalances = [btc.id: btcBalance]
}
} else {
tokenBalances = await self.rpcService.fetchBalancesForTokens(
account: account,
networkAssets: allUserNetworkAssets
)
tokenBalanceCache.merge(with: tokenBalances)
}
StephenHeaps marked this conversation as resolved.
Show resolved Hide resolved
tokenBalanceCache.merge(with: tokenBalances)
StephenHeaps marked this conversation as resolved.
Show resolved Hide resolved

// fetch price for every user asset
Expand Down
43 changes: 30 additions & 13 deletions ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AccountsStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class AccountsStore: ObservableObject, WalletObserverStore {

let currencyFormatter: NumberFormatter = .usdCurrencyFormatter

/// Cache of token balances for each account. [account.address: [token.id: balance]]
/// Cache of token balances for each account. [account.cacheBalanceKey: [token.id: balance]]
private var tokenBalancesCache: [String: [String: Double]] = [:]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a thing like branded types in typescrpit but for swift?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think thats typealias in swift. i will update. thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nuo-xu I'm not sure a typealias would work the same here, maybe I am misunderstanding. A typealias would essentially be typealias TokenBalanceKey = String, but would only validate the type a String, not validate the key is the expected value/format (both address and accountId.uniqueKey are String type).

I think better approach for us would be

  1. dropping tokenBalancesCache from each store/view model & fetch from cache when balance is needed
    • Not sure we need to fetch from CoreData cache, then cache again in the store/view model. If we need in memory cache dictionary likely better in WalletUserAssetManager itself so all stores/view models can share it.
  2. Add helper methods as needed in WalletUserAssetManager which accept a BlockchainToken, AccountInfo, etc. so the cache manager itself has control of the keys used in the cache(s).
    • We already have some/most of these I believe

pseudocode:

func getBalances(for account: BraveWallet.AccountInfo) -> Double? {
    return balancesCache[account.accountId.uniqueKey]
}

/// Cache of prices for each token. The key is the token's `assetRatioId`.
private var pricesCache: [String: String] = [:]
Expand All @@ -42,6 +42,7 @@ class AccountsStore: ObservableObject, WalletObserverStore {
private let rpcService: BraveWalletJsonRpcService
private let walletService: BraveWalletBraveWalletService
private let assetRatioService: BraveWalletAssetRatioService
private let bitcoinWalletService: BraveWalletBitcoinWalletService
private let userAssetManager: WalletUserAssetManagerType

private var keyringServiceObserver: KeyringServiceObserver?
Expand All @@ -56,12 +57,14 @@ class AccountsStore: ObservableObject, WalletObserverStore {
rpcService: BraveWalletJsonRpcService,
walletService: BraveWalletBraveWalletService,
assetRatioService: BraveWalletAssetRatioService,
bitcoinWalletService: BraveWalletBitcoinWalletService,
userAssetManager: WalletUserAssetManagerType
) {
self.keyringService = keyringService
self.rpcService = rpcService
self.walletService = walletService
self.assetRatioService = assetRatioService
self.bitcoinWalletService = bitcoinWalletService
self.userAssetManager = userAssetManager
self.setupObservers()
}
Expand Down Expand Up @@ -143,11 +146,25 @@ class AccountsStore: ObservableObject, WalletObserverStore {
body: { group in
for account in accounts {
group.addTask {
let balancesForTokens: [String: Double] = await self.rpcService.fetchBalancesForTokens(
account: account,
networkAssets: allNetworkAssets
)
return [account.address: balancesForTokens]
var balancesForTokens: [String: Double] = [:]
if account.coin == .btc {
let networkAssets = allNetworkAssets.first {
$0.network.supportedKeyrings.contains(account.keyringId.rawValue as NSNumber)
}
if let btc = networkAssets?.tokens.first,
let btcBalance = await self.bitcoinWalletService.fetchBTCBalance(
accountId: account.accountId
)
{
balancesForTokens = [btc.id: btcBalance]
}
} else {
balancesForTokens = await self.rpcService.fetchBalancesForTokens(
account: account,
networkAssets: allNetworkAssets
)
}
return [account.cacheBalanceKey: balancesForTokens]
}
}
return await group.reduce(
Expand All @@ -159,13 +176,13 @@ class AccountsStore: ObservableObject, WalletObserverStore {
}
)
for account in accounts {
if let updatedBalancesForAccount = balancesForAccounts[account.address] {
if let updatedBalancesForAccount = balancesForAccounts[account.cacheBalanceKey] {
// if balance fetch failed that we already have cached, don't overwrite existing
if var existing = self.tokenBalancesCache[account.address] {
if var existing = self.tokenBalancesCache[account.cacheBalanceKey] {
existing.merge(with: updatedBalancesForAccount)
self.tokenBalancesCache[account.address] = existing
self.tokenBalancesCache[account.cacheBalanceKey] = existing
} else {
self.tokenBalancesCache[account.address] = updatedBalancesForAccount
self.tokenBalancesCache[account.cacheBalanceKey] = updatedBalancesForAccount
}
}
}
Expand Down Expand Up @@ -223,7 +240,7 @@ class AccountsStore: ObservableObject, WalletObserverStore {
for account: BraveWallet.AccountInfo,
tokens: [BraveWallet.BlockchainToken]
) -> [BraveWallet.BlockchainToken] {
guard let tokenBalancesForAccount = tokenBalancesCache[account.address] else {
guard let tokenBalancesForAccount = tokenBalancesCache[account.cacheBalanceKey] else {
return []
}
var tokensFiatForAccount: [(token: BraveWallet.BlockchainToken, fiat: Double)] = []
Expand All @@ -247,7 +264,7 @@ class AccountsStore: ObservableObject, WalletObserverStore {
for account: BraveWallet.AccountInfo,
tokens: [BraveWallet.BlockchainToken]
) -> Double {
guard let accountBalanceCache = tokenBalancesCache[account.address] else { return 0 }
guard let accountBalanceCache = tokenBalancesCache[account.cacheBalanceKey] else { return 0 }
return accountBalanceCache.keys.reduce(0.0) { partialResult, tokenId in
guard let tokenBalanceForAccount = tokenBalanceForAccount(tokenId: tokenId, account: account)
else {
Expand All @@ -271,7 +288,7 @@ class AccountsStore: ObservableObject, WalletObserverStore {
tokenId: String,
account: BraveWallet.AccountInfo
) -> Double? {
tokenBalancesCache[account.address]?[tokenId]
tokenBalancesCache[account.cacheBalanceKey]?[tokenId]
}
}

Expand Down
24 changes: 17 additions & 7 deletions ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AssetDetailStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ class AssetDetailStore: ObservableObject, WalletObserverStore {
private let solTxManagerProxy: BraveWalletSolanaTxManagerProxy
private let ipfsApi: IpfsAPI
private let swapService: BraveWalletSwapService
private let bitcoinWalletService: BraveWalletBitcoinWalletService
private let assetManager: WalletUserAssetManagerType
/// A list of tokens that are supported with the current selected network for all supported
/// on-ramp providers.
Expand Down Expand Up @@ -138,6 +139,7 @@ class AssetDetailStore: ObservableObject, WalletObserverStore {
solTxManagerProxy: BraveWalletSolanaTxManagerProxy,
ipfsApi: IpfsAPI,
swapService: BraveWalletSwapService,
bitcoinWalletService: BraveWalletBitcoinWalletService,
userAssetManager: WalletUserAssetManagerType,
assetDetailType: AssetDetailType
) {
Expand All @@ -150,6 +152,7 @@ class AssetDetailStore: ObservableObject, WalletObserverStore {
self.solTxManagerProxy = solTxManagerProxy
self.ipfsApi = ipfsApi
self.swapService = swapService
self.bitcoinWalletService = bitcoinWalletService
self.assetManager = userAssetManager
self.assetDetailType = assetDetailType

Expand Down Expand Up @@ -456,19 +459,26 @@ class AssetDetailStore: ObservableObject, WalletObserverStore {
@MainActor group -> [AccountBalance] in
for accountAssetViewModel in accountAssetViewModels {
group.addTask { @MainActor in
let balance = await self.rpcService.balance(
for: token,
in: accountAssetViewModel.account,
network: network
)
return [AccountBalance(accountAssetViewModel.account, balance)]
var tokenBalance: Double?
if accountAssetViewModel.account.coin == .btc {
tokenBalance = await self.bitcoinWalletService.fetchBTCBalance(
accountId: accountAssetViewModel.account.accountId
)
} else {
tokenBalance = await self.rpcService.balance(
for: token,
in: accountAssetViewModel.account,
network: network
)
}
return [AccountBalance(accountAssetViewModel.account, tokenBalance)]
}
}
return await group.reduce([AccountBalance](), { $0 + $1 })
}
for tokenBalance in tokenBalances {
if let index = accountAssetViewModels.firstIndex(where: {
$0.account.address == tokenBalance.account.address
$0.account.cacheBalanceKey == tokenBalance.account.cacheBalanceKey
}) {
accountAssetViewModels[index].decimalBalance = tokenBalance.balance ?? 0.0
accountAssetViewModels[index].balance = String(format: "%.4f", tokenBalance.balance ?? 0.0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not specific to Bitcoin, but we need to update balance to be a Double or BDouble on this view model and format as a String in UI code instead of view model. Right now the totalBalance computed property uses this string to calculate the total balance, but this value is rounded to 4 decimal places, however total balance is being displayed with 6 decimal places.
So if I have a balance of 0.00001 BTC it will show as 0.000000 BTC total balance.

Since this isn't specific to Bitcoin, I opened a separate issue for this rounding bug:
brave/brave-browser#37670

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is actually an idea somehere deep in backlog to return balance in weis, satoshis, lamports, etc. So any rounding happens only when we want to display that balance on screen.
Using double in money related ares is discouraged in general

Expand Down
7 changes: 6 additions & 1 deletion ios/brave-ios/Sources/BraveWallet/Crypto/Stores/CryptoStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,8 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
keyringService: keyringService,
rpcService: rpcService,
walletService: walletService,
txService: txService
txService: txService,
bitcoinWalletService: bitcoinWalletService
)
self.origin = origin

Expand All @@ -184,6 +185,7 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
assetRatioService: assetRatioService,
blockchainRegistry: blockchainRegistry,
ipfsApi: ipfsApi,
bitcoinWalletService: bitcoinWalletService,
userAssetManager: userAssetManager
)
self.nftStore = .init(
Expand Down Expand Up @@ -213,6 +215,7 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
rpcService: rpcService,
walletService: walletService,
assetRatioService: assetRatioService,
bitcoinWalletService: bitcoinWalletService,
userAssetManager: userAssetManager
)
self.marketStore = .init(
Expand Down Expand Up @@ -504,6 +507,7 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
solTxManagerProxy: solTxManagerProxy,
ipfsApi: ipfsApi,
swapService: swapService,
bitcoinWalletService: bitcoinWalletService,
userAssetManager: userAssetManager,
assetDetailType: assetDetailType
)
Expand Down Expand Up @@ -541,6 +545,7 @@ public class CryptoStore: ObservableObject, WalletObserverStore {
blockchainRegistry: blockchainRegistry,
solTxManagerProxy: solTxManagerProxy,
ipfsApi: ipfsApi,
bitcoinWalletService: bitcoinWalletService,
userAssetManager: userAssetManager
)
accountActivityStore = store
Expand Down
4 changes: 3 additions & 1 deletion ios/brave-ios/Sources/BraveWallet/Crypto/Stores/NFTStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ public class NFTStore: ObservableObject, WalletObserverStore {
return Filters(
accounts: allAccounts.map { account in
.init(
isSelected: !nonSelectedAccountAddresses.contains(where: { $0 == account.address }),
isSelected: !nonSelectedAccountAddresses.contains(where: {
$0 == account.filterAccountKey
}),
model: account
)
},
Expand Down
Loading
Loading