brave / brave-ios

Brave iOS Browser
https://brave.com
Mozilla Public License 2.0
1.7k stars 441 forks source link

Fix #6921: Persist Asset Balance in Portfolio #8726

Closed nuo-xu closed 7 months ago

nuo-xu commented 8 months ago

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:

Test Plan:

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:

github-actions[bot] commented 7 months ago

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

nuo-xu commented 7 months ago

Close since we have migrated to brave-core. A duplicate PR is created https://github.com/brave/brave-core/pull/22029