[iOS] Bitcoin account creation #22737
Conversation
nuo-xu
added
CI/skip-android
nuo-xu
requested review from
supermassive,
StephenHeaps,
stoletheminerals and
kdenhartog
ios/brave-ios/Sources/BraveWallet/Crypto/Accounts/Details/AccountDetailsView.swift
Outdated
Show resolved
Hide resolved
| @@ -253,6 +253,7 @@ private struct SiteConnectionDetailView: View { | |||
| ForEach(siteConnection.connectedAddresses, id: \.self) { address in | |||
| AccountView( | |||
| address: address, | |||
| seed: address, // TODO: Should be the same for blockie seed and the address in dapps for all networks including bitcoin. Bitcoin dapp site permission, we should store the AccountInfo.AccountId.UniqueKey.sha256. Might need to update the UI if the permission in bitcoin dapp will be handled differently. | |||
There was a problem hiding this comment.
This TODO is very early, no Bitcoin DApp support yet, but it's good we checked early 😄! iOS TODO's should have issue linked. I think either open iOS specific issue that links to brave/brave-browser#34966 & include iOS issue # in this TODO comment, or drop this comment.
There was a problem hiding this comment.
can we somehow assert this view is eth sol only?
There was a problem hiding this comment.
@supermassive i dont think we need an assertion here since we only fetch permission status for .dapps
And supported coin type is only for
.eth and .sol which is stored here
ios/brave-ios/Sources/BraveWallet/Crypto/Accounts/Activity/AccountActivityView.swift
Show resolved
Hide resolved
ios/brave-ios/Sources/BraveWallet/Crypto/WalletActions/DepositTokenView.swift
Outdated
Show resolved
Hide resolved
| bitcoinAccounts: [String: BraveWallet.BitcoinAccountInfo] | ||
| ) -> QRCodeViewModel { | ||
| if let bitcoinAccount = bitcoinAccounts[account.accountId.uniqueKey] { | ||
| return .init(name: account.name, address: bitcoinAccount.nextReceiveAddress.addressString) |
There was a problem hiding this comment.
Hmm, when next_receive_address updates, does AccountsChanged fire? Uncommon case, but I think this address could change with Deposit screen open for Bitcoin account, if so then we likely want to update this address & QR Code on Deposit. Might be issue on desktop too cc @supermassive
There was a problem hiding this comment.
I don't believe so. Desktop polls the receive address every 10 minutes, though I'm not sure if there is a better way (IIRC we only monitor blockchain when there is a pending tx from our wallet). @supermassive any thoughts?
| isPresentingExportAccount = true | ||
| } label: { | ||
| Label(Strings.Wallet.exportButtonTitle, braveSystemImage: "leo.key") | ||
| if store.account.coin != .btc { |
There was a problem hiding this comment.
maybe .zec here also?
There was a problem hiding this comment.
Alright, I think now that handling also .zec is not what we need
| .listRowBackground(Color(.secondaryBraveGroupedBackground)) | ||
| } | ||
| accountNameSection | ||
| if isJSONImported { | ||
| originPasswordSection | ||
| } | ||
| privateKeySection | ||
| if selectedCoin != .btc && preSelectedCoin != .btc { |
| @@ -180,7 +180,7 @@ struct PortfolioAssetGroupHeaderView: View { | |||
| if case .network(let networkInfo) = group.groupType { | |||
| NetworkIconView(network: networkInfo, length: 32) | |||
| } else if case .account(let accountInfo) = group.groupType { | |||
| Blockie(address: accountInfo.address) | |||
| Blockie(address: accountInfo.blockieSeed) | |||
There was a problem hiding this comment.
Can we have some Blockie factory which is able to create an instance from just accountInfo?
There was a problem hiding this comment.
I think its fine to keep it only taking the String value since we are not only using it for AccountInfo but also for string type address from TxInfo, dapps' requests and even for custom network's icon based on network name.
blockieSeed is added as en extension computed variable in AccountInfo which can easily check what is the correct seed value for this AccountInfo
| @@ -162,15 +162,15 @@ struct EditSiteConnectionView: View { | |||
| if sizeCategory.isAccessibilityCategory { | |||
| VStack { | |||
| AddressView(address: account.address) { | |||
| AccountView(address: account.address, name: account.name) | |||
| AccountView(address: account.address, seed: account.address, name: account.name) | |||
There was a problem hiding this comment.
should AccountView be parametrized by account instead of its fields?
There was a problem hiding this comment.
adding a new constructor that only takes an AccountInfo. the old constructer is still needed for places that don't have AccountInfo for example SiteConnectionDetailView
| private static func sameBalanceSort(lhs: AssetGroupViewModel, rhs: AssetGroupViewModel) -> Bool { | ||
| if case .account(let lhsAccount) = lhs.groupType, case .account(let rhsAccount) = rhs.groupType | ||
| { | ||
| if lhsAccount.coin == .fil && rhsAccount.coin == .fil { | ||
| if lhsAccount.keyringId == .filecoin && rhsAccount.keyringId != .filecoin { | ||
| return true | ||
| } else if lhsAccount.keyringId != .filecoin && rhsAccount.keyringId == .filecoin { | ||
| return false | ||
| } | ||
| } else if lhsAccount.coin == .btc && rhsAccount.coin == .btc { | ||
| if lhsAccount.keyringId == .bitcoin84 && rhsAccount.keyringId != .bitcoin84 { | ||
| return true | ||
| } else if lhsAccount.keyringId != .bitcoin84 && rhsAccount.keyringId == .bitcoin84 { | ||
| return false | ||
| } | ||
| } else { | ||
| if lhsAccount.keyringId == .solana && rhsAccount.keyringId != .solana { | ||
| return true | ||
| } else if lhsAccount.keyringId != .solana && rhsAccount.keyringId == .solana { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| if case .network(let lhsNetwork) = lhs.groupType, case .network(let rhsNetwork) = rhs.groupType | ||
| { | ||
| let isLHSPrimaryNetwork = WalletConstants.primaryNetworkChainIds.contains(lhsNetwork.chainId) | ||
| let isRHSPrimaryNetwork = WalletConstants.primaryNetworkChainIds.contains(rhsNetwork.chainId) | ||
| if isLHSPrimaryNetwork && !isRHSPrimaryNetwork { | ||
| return true | ||
| } else if !isLHSPrimaryNetwork && isRHSPrimaryNetwork { | ||
| return false | ||
| } else if isLHSPrimaryNetwork, isRHSPrimaryNetwork, | ||
| lhsNetwork.chainId != rhsNetwork.chainId, | ||
| lhsNetwork.chainId == BraveWallet.SolanaMainnet | ||
| { | ||
| // Solana Mainnet to be first primary network | ||
| return true | ||
| } else if isLHSPrimaryNetwork, isRHSPrimaryNetwork, | ||
| lhsNetwork.chainId != rhsNetwork.chainId, | ||
| rhsNetwork.chainId == BraveWallet.SolanaMainnet | ||
| { | ||
| // Solana Mainnet to be first primary network | ||
| return false | ||
| } | ||
| return lhs.id < rhs.id | ||
| } | ||
| return lhs.id < rhs.id | ||
| } |
There was a problem hiding this comment.
Is it possible to sort this as an array of tuples?
https://holyswift.app/using-tuples-to-complex-sorting-operations-in-swift/
let sortedMovieList = movieList.sorted {
($0.title, $0.year, $0.director) < ($1.title, $1.year, $1.director)
}
for movie in sortedMovieList {
print(movie)
}
There was a problem hiding this comment.
this requires each property inside the tuple conforms Comparable protocol. In your example, they are either String or Int, they are all Comparable using the default < implementation. In our case, we have more complex logic of comparison of lhs and rhs object rather than the default < implementation.
There was a problem hiding this comment.
It doesn't feel right if swift's sugar doesn't support that, neverthelss if tuple sort could not be applied here then at least this algorithm needs a unit test.
nuo-xu
force-pushed
the
wallet/ios-bitcoin-account-creation
branch
from
09a44fc
to
bd8465e
Compare
…eation. bitcoin migration and related UI updates.
…in to bitcoin. fix walletpnanel infinite loading after switching to a bitcoin account. fix wrong buy url
nuo-xu
force-pushed
the
wallet/ios-bitcoin-account-creation
branch
from
bd8465e
to
4a3a205
Compare
|
[puLL-Merge] - brave/brave-core@22737 DescriptionThis PR adds support for Bitcoin in the Brave Wallet iOS app. It allows users to create Bitcoin accounts, view balances, and deposit BTC. ChangesChanges
Security Hotspots
|
|
There is an know issue that is caused by brave/brave-browser#37192 |
There was a problem hiding this comment.
Just the known issue with switching from Bitcoin Mainnet account <-> Bitcoin Testnet account not updating the network to Bitcoin Mainnet / Testnet respectively. Bitcoin remains behind feature flag so the fix will be handled in separate PR with brave/brave-browser#37256.
LGTM 🐎.
| private var keyringServiceObserver: KeyringServiceObserver? | ||
| private var walletServiceObserver: WalletServiceObserver? | ||
|
|
||
| @Published var prefilledToken: BraveWallet.BlockchainToken? | ||
| var prefilledAccount: BraveWallet.AccountInfo? | ||
| @Published var allAccounts: [BraveWallet.AccountInfo] = [] | ||
| @Published var allNetworks: [BraveWallet.NetworkInfo] = [] | ||
| @Published var bitcoinAccounts: [String: BraveWallet.BitcoinAccountInfo] = [:] |
There was a problem hiding this comment.
nit: comment for what the dictionary key is
| contractAddress: "", | ||
| name: "Bitcoin", | ||
| logo: "", | ||
| isErc20: false, |
There was a problem hiding this comment.
nit: we should probably be making BlockchainToken an interface specific to the coin type rather than making these overlap between all of the networks
| ] | ||
| let mockBtcBalanceInWei = |
There was a problem hiding this comment.
nit: this also looks like a leaky abstraction here
Resolves brave/brave-browser#36811
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
In order to test this PR, you need to set up a feature flag in two ways:
Debugscheme and add--enable-features=BraveWalletBitcoinin yourArguments Passed On Launch, then build.BraveWalletBitcoinunder Settings/BraveCore Switches/Enable Features (EnableEnable Featuresand a fresh launch is required)Note: Bitcoin balance fetching will be addressed in a separate issue brave/brave-browser#36966
All BTC balance will be displayed as 0 value. Send BTC will be addressed in a separate issue brave/brave-browser#36968
Test BTC default user asset:
Test Bitcoin Account creation
Bitcoin Mainnetis pre-selected and cannot be changedAddAsset...on top right in navigation barAddTest Bitcoin Account Update
...on Bitcoin Account Card then chooseEditin the context menu or go to account details. Tap...on top right in navigation bar and chooseEditin the context menuDoneTest BTC or Bitcoin Account Deposit
Flow is the same
Check there is no import or export available for bitcoin accounts.