feat(wallet): Bitcoin Balance Fetching #23117

nuo-xu · 2024-04-17T03:03:56Z

iOS starts fetching bitcoin balance for bitcoin accounts both mainnet and testnet usingBraveWalletBitcoinWalletService
Wallet will display using availableBalance. pendingBalance UI will be handled in brave/brave-browser#37640

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

In order to test this PR, you need to set up a feature flag in two ways:

If you are building this PR locally on your Xcode, duplicate Debug scheme and add --enable-features=BraveWalletBitcoin in your Arguments Passed On Launch, then build.
If you are testing using a TF build. Add BraveWalletBitcoin under Settings/BraveCore Switches/Enable Features (Enable Enable Features and a fresh launch is required)

1. Check bitcoin balance displaying

Bitcoin balance will be displayed in

wallet panel
portfolio
asset details
accounts tab
account details

you can either create a new bitcoin and check if balance is correctly displayed as $0.00
or you can restore a wallet with bitcoin accounts that have balances and check if balance is correctly displayed with the right amount.

2. Check bitcoin accounts in filter selection

open brave wallet and create a wallet if you dont have any
create multiple bitcoin account either on mainnet or testnet
in Portfolio, select group by account, save changes
observe portfolio is now displaying in group by account
make some changes on bitcoin account selection in filter
check if Portfolio display or hide bitcoin account correctly.

ios/brave-ios/Sources/BraveWallet/Extensions/BraveWalletExtensions.swift

ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AccountActivityStore.swift

ios/brave-ios/Tests/BraveWalletTests/AssetDetailStoreTests.swift

ios/brave-ios/Tests/BraveWalletTests/PortfolioStoreTests.swift

StephenHeaps · 2024-04-17T20:46:10Z

ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AssetDetailStore.swift

        }
      }
      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)


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

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

supermassive · 2024-04-19T02:53:20Z

ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AccountsStore.swift

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


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

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

@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

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.

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] }

supermassive · 2024-04-19T02:59:33Z

ios/brave-ios/Sources/BraveWallet/Extensions/BitcoinWalletServiceExtensions.swift

+    let availableSatoshiString = String(btcBalance.availableBalance)
+    let formatter = WeiFormatter(decimalFormatStyle: .decimals(precision: 8))
+    if let valueString = formatter.decimalString(
+      for: availableSatoshiString,
+      radix: .decimal,
+      decimals: 8
+    ) {
+      return Double(valueString)
+    } else {
+      return nil
+    }
+  }


In most contexts we need total balance, in a context of send screen we need available balance. For desktop ui there is also a popup which shows all three lines of complex BTC balance.
FYI

i will update this using totalBalance. we have a separate issue to integrate bitcoin balance full info brave/brave-browser#37640
will keep in mind to use availableBalance in send tx.
thanks!

… address some review comments.

ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AccountActivityStore.swift

github-actions · 2024-04-22T18:30:11Z

[puLL-Merge] - brave/brave-core@23117

Description

This PR updates the codebase to use account.accountId.uniqueKey (account.id) instead of account.address as the cache key for various account-related data. It also integrates fetching Bitcoin balances using the BraveWalletBitcoinWalletService.

Changes

Changes

FiltersDisplaySettingsView.swift: Update to use account.id instead of account.address when filtering non-selected accounts.
AccountActivityStore.swift, AccountsStore.swift, AssetDetailStore.swift, NFTDetailStore.swift, NFTStore.swift, PortfolioStore.swift, SelectAccountTokenStore.swift:
- Update to use account.id as cache key instead of account.address for token balances and other account-related data.
- Integrate fetching Bitcoin balances using BraveWalletBitcoinWalletService for Bitcoin accounts.
CryptoStore.swift: Pass bitcoinWalletService to various stores during initialization.
BitcoinWalletServiceExtensions.swift: Add fetchBTCBalance method to fetch BTC balance for a given account ID and balance type.
WalletPreferences.swift: Add migrateCacheKeyCompleted preference to track migration of cache key from account.address to account.id.
WalletUserAssetManager.swift:
- Migrate cache key on keyring unlock if not already done.
- Use BraveWalletBitcoinWalletService to fetch BTC balances for Bitcoin accounts.
WalletUserAssetBalance.swift: Add updateAccountAddress method to update accountAddress for a given account during migration.
Various test files: Update tests to use account.id and integrate Bitcoin balance fetching.

Security Hotspots

Ensure that the migration from account.address to account.id is thoroughly tested and does not result in any data loss or inconsistencies. Verify that all relevant caches and preferences are properly migrated.
Carefully review the integration of BraveWalletBitcoinWalletService to fetch Bitcoin balances. Ensure that the service is reliable, secure, and returns accurate balance data.
Verify that the updated caching mechanism using account.id maintains data integrity and does not introduce any unintended side effects or performance issues.

kdenhartog

LGTM for post merge sec review

github-actions bot assigned nuo-xu Apr 17, 2024

nuo-xu added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 labels Apr 17, 2024

nuo-xu force-pushed the wallet/ios-bitcoin-balance branch from f75f76b to efee4f6 Compare April 17, 2024 03:07

bitcoin balance fetching

04c2f87

nuo-xu force-pushed the wallet/ios-bitcoin-balance branch from efee4f6 to 04c2f87 Compare April 17, 2024 15:19

nuo-xu requested a review from StephenHeaps April 17, 2024 16:41

nuo-xu marked this pull request as ready for review April 17, 2024 16:41

nuo-xu requested a review from a team as a code owner April 17, 2024 16:41

StephenHeaps reviewed Apr 17, 2024

View reviewed changes

ios/brave-ios/Sources/BraveWallet/Extensions/BraveWalletExtensions.swift Outdated Show resolved Hide resolved

ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AccountActivityStore.swift Outdated Show resolved Hide resolved

StephenHeaps requested review from supermassive, stoletheminerals and kdenhartog April 17, 2024 20:00

StephenHeaps reviewed Apr 17, 2024

View reviewed changes

supermassive reviewed Apr 19, 2024

View reviewed changes

migrate some caching value due to empty address for bitcoin accounts.…

69fcc8d

… address some review comments.

github-actions bot added the puLL-Merge label Apr 19, 2024

StephenHeaps approved these changes Apr 22, 2024

View reviewed changes

StephenHeaps reviewed Apr 22, 2024

View reviewed changes

ios/brave-ios/Sources/BraveWallet/Crypto/Stores/AccountActivityStore.swift Outdated Show resolved Hide resolved

remove duplicated dictionary merging

437d5d8

nuo-xu merged commit cb0f714 into master Apr 22, 2024
19 checks passed

nuo-xu deleted the wallet/ios-bitcoin-balance branch April 22, 2024 20:08

github-actions bot added this to the 1.67.x - Nightly milestone Apr 22, 2024

StephenHeaps mentioned this pull request Jun 3, 2024

Enable Bitcoin by default on iOS brave/brave-browser#38775

Closed

srirambv mentioned this pull request Jun 11, 2024

Wallet: Fetching Bitcoin Balance brave/brave-browser#36966

Closed

kdenhartog reviewed Jun 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wallet): Bitcoin Balance Fetching #23117

feat(wallet): Bitcoin Balance Fetching #23117

nuo-xu commented Apr 17, 2024

StephenHeaps Apr 17, 2024

supermassive Apr 19, 2024

supermassive Apr 19, 2024

nuo-xu Apr 19, 2024

StephenHeaps Apr 19, 2024

supermassive Apr 19, 2024

nuo-xu Apr 19, 2024

github-actions bot commented Apr 22, 2024

Changes

kdenhartog left a comment

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

feat(wallet): Bitcoin Balance Fetching #23117

feat(wallet): Bitcoin Balance Fetching #23117

Conversation

nuo-xu commented Apr 17, 2024

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

StephenHeaps Apr 17, 2024

Choose a reason for hiding this comment

supermassive Apr 19, 2024

Choose a reason for hiding this comment

supermassive Apr 19, 2024

Choose a reason for hiding this comment

nuo-xu Apr 19, 2024

Choose a reason for hiding this comment

StephenHeaps Apr 19, 2024

Choose a reason for hiding this comment

supermassive Apr 19, 2024

Choose a reason for hiding this comment

nuo-xu Apr 19, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2024

Description

Changes

Security Hotspots

kdenhartog left a comment

Choose a reason for hiding this comment