-
Notifications
You must be signed in to change notification settings - Fork 444
Fix #6921: Persist Asset Balance in Portfolio #8726
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments (still reviewing); nice work so far this is great 🔥.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cached balance will be refreshed under certain scenarios. they will be listed in the test plan.
Don't forget to list these 🙂.
Looking good, mostly nits but a bug after dismissing wallet on iOS 17.0..<17.2 due to memory leak (need to setup observers again).
70c286f
to
bac1398
Compare
[puLL-Merge] - brave/brave-ios@8726 DescriptionThis pull request introduces several major changes to the Brave iOS wallet related to adding and editing custom assets, managing asset visibility, updating the assets list, and improving security and robustness through codebase reorganization and refining how balances are handled. ChangesChangesSources/BraveWallet/Crypto/Portfolio/AddCustomAssetView.swift
Sources/BraveWallet/Crypto/Portfolio/EditUserAssetsView.swift
Sources/BraveWallet/Crypto/Portfolio/PortfolioView.swift
Sources/BraveWallet/Crypto/Stores/CryptoStore.swift
Sources/BraveWallet/Crypto/Stores/NFTStore.swift and PortfolioStore.swift
Sources/BraveWallet/Crypto/Stores/NetworkStore.swift
Sources/BraveWallet/Crypto/Stores/UserAssetsStore.swift
Sources/BraveWallet/WalletUserAssetManager.swift
Sources/Data/models/Model.xcdatamodeld/Model28.xcdatamodel/contents
Tests updates
Security Hotspots
Overall, this pull request significantly refactors how assets are managed within the Brave iOS wallet, with a particular focus on streamlining the addition and editing process, improving balance handling, and ensuring the UI accurately reflects the current state. Proper testing, especially around data integrity and UI consistency, is crucial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @nuo-xu 🐎
Close since we have migrated to |
Summary of Changes
We are now caching asset's balance. They can be easily fetch from CD via token info or account info or both.
Cached balance will be refreshed under certain scenarios. they will be listed in the test plan.
This pull request fixes brave/brave-browser#35986
Submitter Checklist:
NSLocalizableString()
Test Plan:
Note: Currently, we have known issues Race condition between fetching token registry and asset discovery on wallet restore brave-browser#35696 and Frontend cannot trigger asset discovery in some situations brave-browser#35697 these two cause auto-discovery assets are not discovered right after restoring. We will use a workaround to verify auto-discovery assets can be discovered and their balances are being fetched correctly. But this issue helps us to verify another scenario I will explain later in other test steps.
WETH
onPolygon Mainnet
WETH
WETH
to mark it as visible assetWETH
now appears and its balance is fetched.NFTs
tab, enable NFT auto-discovery in the promptAssets
also shows up if you navigate toAssets
Assets
that this Wallet A owns with some balance. Please check if their balances were fetched correctly.Account
in PortfolioGroup
Confirmed
In summary, balances in Portfolio (Assets) will be refreshed when
Note: Transactions that are submitted by Dapps via signing request through wallet, since there is no access from Wallet to know the transaction status that is not created by Brave Wallet, it is not possible to determine when to refresh the balance based this transaction's status until we can observe all on-chain activity for an address. In this case, to refresh balance, user has to lock and unlock the wallet.
Reviewer Checklist:
QA/(Yes|No)
bug
/enhancement