Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #6921: Persist Asset Balance in Portfolio #8726

Closed
wants to merge 7 commits into from

Conversation

nuo-xu
Copy link
Contributor

@nuo-xu nuo-xu commented Feb 1, 2024

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:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()
  • New or updated UI has been tested across:
    • Light & dark mode
    • Different size classes (iPhone, landscape, iPad)
    • Different dynamic type sizes

Test Plan:

  • Upgrade (current target 1.63.x)
  1. install a older version of Brave (1.62.x and earlier)
  2. set up a wallet with some balance
  3. Dismiss wallet then open
  4. Notice every time wallet open triggers balance fetching
  5. upgrade Brave to the 1.63.x
  6. Open wallet you will notice there will be a loading time to fetch balance since we have never cached any
  7. Dismiss wallet and open again
  8. Notice the balance still being displayed with no loading.
  • New Wallet Set up
  1. Fresh install wallet and create a new wallet from onboarding
  2. All default user assets will appear in Portfolio with 0 balance.
  3. Dismiss the wallet then open it (without locking)
  4. There will be no loading in Portfolio.
  1. Delete Brave and fresh install Brave
  2. Restore a wallet from onboarding (Wallet A)
  3. You will see default user assets are being displayed and their balance are being fetched or in within couple seconds based on internet speed.
  4. Dismiss the wallet then open it (without locking)
  5. There will be no loading in Portfolio.
  6. Notice there is no auto-discovered assets got discovered. This is the issue mentioned earlier.
  7. Let's take advantage of this issue by checking if wallet will add and fetch balance of a token that we already know this wallet has
  8. Let's say we already know Wallet A has some balance of WETH on Polygon Mainnet
  9. Tap Edit Visible Asset to bring up the token list, and search for WETH
  10. Tap on WETH to mark it as visible asset
  11. Back to Portfolio, check WETH now appears and its balance is fetched.
  12. A workaround to let core give the rest of auto-discovery assets is go to NFTs tab, enable NFT auto-discovery in the prompt
  13. A spinner will show up indicating wallet starts discovering assets.
  14. The spinner under Assets also shows up if you navigate to Assets
  15. After a while, once the spinner disappears, some new assets will show up in Assets that this Wallet A owns with some balance. Please check if their balances were fetched correctly.
  16. repeat step 4-5
  17. Lock the wallet
  18. restore a new wallet (Wallet A)
  19. Check user assets and balance are fetched correctly,
  20. Repeat step 4-5
  • Add a primary or secondary account to wallet
  1. Group visible assets by Account in Portfolio
  2. add a primary account
  3. check the native token has been added to Portfolio and its balance is fetched "0"
  4. add a secondary account with some balance
  5. check all visible assets have the same coin type are displayed under this new account and their balances are fetched correctly
  • Add/Remove a new network
  1. Group visible assets by Group
  2. add/remove a custom network from wallet settings or a dapp
  3. Check if the new network group is added in Portfolio and its native asset is displayed in the group with its balance fetched correctly. (Or removed if the network is removed)
  • Add/Remove custom token
  1. add/remove a custom token from wallet settings or a dapp
  2. Check if the new token is displayed in Portfolio with its balance fetched correctly. (Or removed if the token is removed)
  • Check balance after a transaction is confirmed/error/dropped
  1. make a send transaction of token A from Portfolio
  2. after the transaction status becomes Confirmed
  3. check if token A balance is updated
  4. Not sure how to make a transaction error out or dropped. but Once a transaction status becomes error or dropped. Some gas fee was consumed so the native token balance is relatively decreased.

In summary, balances in Portfolio (Assets) will be refreshed when

  1. A wallet is created
  2. A wallet is restored
  3. When wallet is unlocked
  4. A account is created or added
  5. A network is added
  6. An asset is added
  7. An asset's visibility status is changed to be visible
  8. A transaction is confirmed or error out or dropped

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:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@nuo-xu nuo-xu self-assigned this Feb 1, 2024
@nuo-xu nuo-xu requested a review from a team as a code owner February 1, 2024 22:44
Copy link
Contributor

@StephenHeaps StephenHeaps left a 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 🔥.

Sources/BraveWallet/WalletAssetBalanceManager.swift Outdated Show resolved Hide resolved
Sources/BraveWallet/WalletAssetBalanceManager.swift Outdated Show resolved Hide resolved
Sources/BraveWallet/WalletAssetBalanceManager.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@StephenHeaps StephenHeaps left a 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).

Sources/BraveWallet/WalletUserAssetManager.swift Outdated Show resolved Hide resolved
Sources/BraveWallet/Crypto/Stores/NetworkStore.swift Outdated Show resolved Hide resolved
Sources/Data/models/WalletUserAssetBalance.swift Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Feb 8, 2024

[puLL-Merge] - brave/brave-ios@8726

Description

This 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.

Changes

Changes

Sources/BraveWallet/Crypto/Portfolio/AddCustomAssetView.swift

  • Removed the closure onNewAssetAdded that was invoked after adding a new asset.
  • Direct dismissal of the view upon successfully adding a user asset, removing the need for a callback to trigger updates.

Sources/BraveWallet/Crypto/Portfolio/EditUserAssetsView.swift

  • Removed the assetsUpdated closure, leveraging direct UI updates upon asset editing.

Sources/BraveWallet/Crypto/Portfolio/PortfolioView.swift

  • Simplified asset addition handling by removing cryptoStore.updateAssets() callback, streamlining the process.

Sources/BraveWallet/Crypto/Stores/CryptoStore.swift

  • Added userAssetManager with expanded initialization to include more services (keyringService, txService).
  • Introduced manual fetching and caching of balances for auto-discovered assets, replacing the previous method that directly updated UI components.
  • Added observer setup and teardown for userAssetManager to ensure timely updates across the application.

Sources/BraveWallet/Crypto/Stores/NFTStore.swift and PortfolioStore.swift

  • Implemented observers for balance updates and user asset modifications, allowing for reactive UI updates without direct method calls.

Sources/BraveWallet/Crypto/Stores/NetworkStore.swift

  • Adjusted network removal logic to include balance deletion for a cleaner state upon network removal.

Sources/BraveWallet/Crypto/Stores/UserAssetsStore.swift

  • Updated asset visibility toggling logic to trigger balance fetching and notification to observers for UI updates.

Sources/BraveWallet/WalletUserAssetManager.swift

  • Introduced WalletUserAssetManager enhancements for managing balance updates, observer notifications, and simplified asset and balance removal logic.

Sources/Data/models/Model.xcdatamodeld/Model28.xcdatamodel/contents

  • Introduced the data model changes required to support the new balancing and asset management features.

Tests updates

  • Adjusted existing tests and added new ones for WalletUserAssetBalance to cover the changes in asset and balance management logic.

Security Hotspots

  1. Sources/BraveWallet/Crypto/Stores/CryptoStore.swift: The introduction of the new balance management and asset update logic increases the complexity of state management, which could lead to inconsistent UI states or data discrepancies if not properly handled.

  2. Sources/BraveWallet/WalletUserAssetManager.swift: Extensive changes in asset management and balance updates introduce potential risks around asynchronous data handling and observer notification logic, which if improperly managed, could result in race conditions or data integrity issues.

  3. Data Modeling (Sources/Data/models/Model.xcdatamodeld/): Any modifications to the data model that are not backward compatible could potentially lead to data migration issues or loss of user data. Proper migration strategies and thorough testing are essential.

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.

Copy link
Contributor

@StephenHeaps StephenHeaps left a 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 🐎

@nuo-xu
Copy link
Contributor Author

nuo-xu commented Feb 14, 2024

Close since we have migrated to brave-core. A duplicate PR is created brave/brave-core#22029

@nuo-xu nuo-xu closed this Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache token balances
2 participants