Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: MultichainBalanceController #12599

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

feat: MultichainBalanceController #12599

wants to merge 31 commits into from
+3,241 −396

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented Dec 6, 2024 •
edited
Loading

Description

Builds off of the work done in this PR.

  • This PR adds the ability to see your non EVM token balance and the fiat token rates (SOL and BTc accounts)
  • This is achieved by using the RatesController and the MultichainBalanceController
  • This PR also extract the balance logic from the PortfolioBalance PR into the useMultichainBalances hook
  • creates the selectors for multichain non evm

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/696

Manual testing steps

  1. checkout this branch
  2. open .js.env
  3. set export METAMASK_BUILD_TYPE="flask" // flask
  4. source .js.env
  5. yarn setup
  6. yarn start:ios
  7. create/import a wallet
  8. open the account menu actions
  9. you should see that you can add a BTC and BTC Testnet account
  10. click Add a new Bitcoin account (Beta)
  11. a popup with a suggested name should appear click confirm
  12. the newly selected account should be your Bitcoin account and the network picker should show the Bitcoin logo the network should not be clickable
  13. Optionally, create a testnet account and fund it with this faucet: https://bitcoinfaucet.uo1.net/
  14. If you have a balance, you should see the fiat balance rendered on the wallet
  15. if not you have a balance, then you should see a 0 fiat balance.
  16. If you change your selected currency (settings/general) and then return to the main wallet view, the balance should update based on the new fiat currency.
  17. The token list should also list your account balance in both fiat and and the native token balance.
  18. the token displayed should be the BTC logo and not have any network badge above it (like the eth account does)
  19. now add a bitcoin TestNet account.
  20. the portfolio balance should show the native token balance not the fiat balance.
  21. repeat the above steps but this time add a new Solana account. If your Solana account has a balance, it should be rendered on the home page in fiat. If not you should see a 0 balance.
  22. we do not currently support Solana testnet accounts.

Screenshots/Recordings

Before

After

Screen.Recording.2025-01-28.at.10.10.54.PM.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 6, 2024

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@owencraston owencraston force-pushed the feat/multichain-balance-controller branch 4 times, most recently from a99e4bf to 31f3fec Compare December 12, 2024 01:43
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
26.8% Coverage on New Code (required ≥ 60%)

See analysis details on SonarQube Cloud

@owencraston owencraston force-pushed the feat/multichain-balance-controller branch 3 times, most recently from 6de9d8b to b382ed0 Compare December 16, 2024 18:47
@owencraston owencraston force-pushed the feat/multichain-balance-controller branch from b382ed0 to 2daef89 Compare January 11, 2025 00:10
@owencraston owencraston mentioned this pull request Jan 14, 2025
Closed
@owencraston owencraston force-pushed the feat/multichain-balance-controller branch from 2daef89 to 569a930 Compare January 14, 2025 19:28
@owencraston owencraston mentioned this pull request Jan 14, 2025
Merged
7 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2025
@owencraston
chore: Update assets and accounts packages (#12984)
4c43298 
# **Description**

The primary purpose of this PR is to update the accounts controller and
the assets-controller to the latest version. In doing that, there were
several other package updates that were needed. Here is a summary of the
updates...


# 🔴  Major updates 🔴 

### "@metamask/accounts-controller": "^20.0.1" -> ^21.0.0
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/accounts-controller/CHANGELOG.md#2100)
- Breaking changes
- BREAKING: Add scopes field to KeyringAccount
(MetaMask/core#5066),
(MetaMask/core#5136)
This field is now required and will be used to identify the supported
chains (using CAIP-2 chain IDs) for every accounts.
- Changes
- Bump @metamask/base-controller from ^7.0.0 to ^7.1.1
(MetaMask/core#5079),
(MetaMask/core#5135)
- Bump @metamask/utils to ^11.0.1
(MetaMask/core#5080)
- Bump @metamask/rpc-errors to ^7.0.2
(MetaMask/core#5080)
- Use new @metamask/keyring-internal-api@^1.0.0
(MetaMask/core#4695)
This package has been split out from the Keyring API.
- Bump @metamask/keyring-api from ^10.1.0 to ^12.0.0
(MetaMask/core#4695)
- Bump @metamask/eth-snap-keyring from ^5.0.1 to ^7.0.0
(MetaMask/core#4695)
ESM/CommonJS support.

### "@metamask/assets-controllers": "^45.1.1", ->  ^46.0.0",
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/assets-controllers/CHANGELOG.md#4600)
- Breaking changes
- BREAKING: Bump @metamask/accounts-controller peer dependency from
^20.0.0 to ^21.0.0 (MetaMask/core#5140)
- Changes
- Remove use of @metamask/keyring-api
(MetaMask/core#4695)
@metamask/providers and webextension-polyfill peer dependencies are no
longer required.
- Use new @metamask/keyring-internal-api@^1.0.0
(MetaMask/core#4695)
This package has been split out from the Keyring API. Its types are
compatible with the @metamask/keyring-api package used previously.
- Bump @metamask/base-controller from ^7.0.0 to ^7.1.1
(MetaMask/core#5079),
(MetaMask/core#5135)
- Bump @metamask/keyring-api from ^12.0.0 to ^13.0.0
(MetaMask/core#5066)
- Bump @metamask/utils to ^11.0.1
(MetaMask/core#5080)
- Bump @metamask/rpc-errors to ^7.0.2
(MetaMask/core#5080)
- Added
- Add new MultichainBalancesController
(MetaMask/core#4965)
This controller has been migrated from the MetaMask extension codebase.

### "@metamask/utils": "^10.0.1" -> ^11.0.1"
-
[changelog](https://github.com/MetaMask/utils/blob/main/CHANGELOG.md#1101)
- breaking changes
- BREAKING: generateRandomMnemonic now returns Promise<void> instead of
void (MetaMask/utils#222)

### Added "@metamask/keyring-internal-api": "^2.0.0",
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-internal-api/CHANGELOG.md#200)
- This package was a peer dep on the latest accounts controller and
assets controller. Given this I figured we should add it now anyway.
- Changes needed
- update imports from `@metamask/keyring-api` to
`@metamask/keyring-internal-api`
   - add support for scopes in the InternalAccount object
- added migration (066.ts) to backfill the scopes to existing accounts.
- This change is based off a [similar change made in the
extension](MetaMask/metamask-extension#29195)
made by @ccharly

### Added @metamask/keyring-snap-client: "^2.0.0"
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-snap-client/CHANGELOG.md#200)
- Added because KeyringClient is now exported from
`@metamask/keyring-snap-client` instead of `@metamask/keyring-api'`. See
`app/components/Views/AddAccountActions/AddAccountActions.tsx` for
changes.

### "@metamask/keyring-api": "^10.1.0", -> ^13.0.0"
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-api/CHANGELOG.md#1300)
- Breaking changes
- BREAKING: Add scopes field to KeyringAccount
(MetaMask/accounts#101)
- BREAKING: Split into several smaller packages
(MetaMask/accounts#24)
        - This should improve dependencies management.
- Internal related types (internal to both clients) have been moved to
keyring-internal-* packages.
- Keyring API clients (mainly used by dapps) have been moved to
keyring-snap-client package.
        - Common utils have been moevd to keyring-utils package.

### "@metamask/eth-snap-keyring": "^5.0.1" -> ^7.0.0"
-
[changelog](https://github.com/MetaMask/accounts/blob/main/packages/keyring-eth-simple/CHANGELOG.md#700)
- breaking changes
- BREAKING: Increase minimum Node.js version to 16
(MetaMask/eth-simple-keyring#152)
- BREAKING: Bump @metamask/eth-sig-util from ^6.0.1 to ^7.0.0
(MetaMask/eth-simple-keyring#156)
- Bump @metamask/utils from ^5.0.0 to ^8.1.0
(MetaMask/eth-simple-keyring#153)
- Bump ethereum-cryptography from ^1.2.0 to ^2.1.2
(MetaMask/eth-simple-keyring#153)
- BREAKING: Bump @metamask/eth-sig-util dependency from ^7.0.3 to ^8.0.0
(MetaMask/accounts#79)
- signTypedData no longer support number for addresses, see
[here](https://github.com/MetaMask/eth-sig-util/blob/main/CHANGELOG.md#800).

## 🟡 Minor updates 🟡 

### "@metamask/base-controller": "^7.0.1", -> ^7.1.1
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/base-controller/CHANGELOG.md#711)
- Changes
    -  Bump @metamask/utils from ^10.0.0 to ^11.0.1
- Rename ControllerMessenger to Messenger
(MetaMask/core#5050)
        - ControllerMessenger has been renamed to Messenger
- RestrictedControllerMessengerConstraint has been renamed to
RestrictedMessengerConstraint
- RestrictedControllerMessenger has been renamed to RestrictedMessenger
- The RestrictedMessenger constructor parameter controllerMessenger has
been renamed to messenger, though the old name is still accepted
- The old names remain exported as deprecated aliases of the new names,
so this is not a breaking change.

### "@metamask/providers": "^18.1.0" -> ^18.3.1"
-
[changelog](https://github.com/MetaMask/providers/blob/main/CHANGELOG.md#1831)
- Changes
- Bump @metamask/json-rpc-engine from ^10.0.1 to ^10.0.2
(MetaMask/providers#397)
- Bump @metamask/json-rpc-middleware-stream from ^8.0.5 to ^8.0.6
(MetaMask/providers#397)
- Bump @metamask/rpc-errors from ^7.0.1 to ^7.0.2
(MetaMask/providers#397)
- Bump @metamask/utils from ^10.0.0 to ^11.0.1
(MetaMask/providers#397)


## 🟢  Patch updates 🟢 

### "@metamask/json-rpc-engine": "^10.0.0" -> ^10.0.2",
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/json-rpc-engine/CHANGELOG.md#1002)

### "@metamask/json-rpc-middleware-stream": "^8.0.2" -> 8.0.6"
-
[changelog](https://github.com/MetaMask/core/blob/main/packages/json-rpc-middleware-stream/CHANGELOG.md#806)
- changed
- Bump @metamask/json-rpc-engine from ^10.0.1 to ^10.0.2
(MetaMask/core#5082)
- Bump @metamask/utils from ^10.0.0 to ^11.0.1
(MetaMask/core#5080)

### "@metamask/rpc-errors": "^7.0.1" -> ^7.0.2"
-
[changelog](https://github.com/MetaMask/rpc-errors/blob/main/CHANGELOG.md#702)
- changes
- Bump @metamask/utils from ^10.0.0 to ^11.0.1
(MetaMask/rpc-errors#166)


## **Related issues**

Fixes: #12967
Fixes: #12966

Fixes: MetaMask/accounts-planning#758

Unblocks: #12599

## **Manual testing steps**

#### Basic import account flow with tokens


https://github.com/user-attachments/assets/1a8d3e59-34e4-413e-a3e8-7dbdc5f7424a



## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@owencraston owencraston force-pushed the feat/multichain-balance-controller branch 2 times, most recently from e3eefdb to f86998f Compare January 16, 2025 22:53
@socket-security Socket Security
Copy link

socket-security bot commented Jan 16, 2025 •
edited
Loading

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@owencraston owencraston force-pushed the feat/multichain-balance-controller branch from 5de2584 to e3f517e Compare January 17, 2025 20:25
@owencraston owencraston force-pushed the feat/multichain-balance-controller branch from 6b0f41b to c31b5de Compare January 20, 2025 21:58
@owencraston owencraston force-pushed the feat/multichain-balance-controller branch from b99918e to e47cae3 Compare January 22, 2025 00:07
gambinish
gambinish requested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@gambinish gambinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for reminding me about this!

I mostly reviewed the logic in the usePortfolioBalance hooks, and left a few comments. Overall I think the approach makes sense. I do see a couple places where we might be able to simplify the business logic a bit, and also a question about some potential duplication of code.

@owencraston owencraston force-pushed the feat/multichain-balance-controller branch 2 times, most recently from 3ac41d3 to 359f977 Compare January 22, 2025 22:42
@owencraston owencraston force-pushed the feat/multichain-balance-controller branch from 1f73e34 to cfa9778 Compare January 29, 2025 21:51
@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 29, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 29, 2025 •
edited by metamaskbot
Loading

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: a37506e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/68d9a51c-2bc3-4a50-894d-bddb75534b9a

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 29, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@owencraston owencraston added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccharly ccharly ccharly left review comments

@brianacnguyen brianacnguyen brianacnguyen requested changes

@Gedoxa Gedoxa Gedoxa approved these changes

@gambinish gambinish Awaiting requested review from gambinish

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Run Smoke E2E Triggers smoke e2e on Bitrise team-accounts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@owencraston @codecov-commenter @ccharly @brianacnguyen @gambinish @Gedoxa @metamaskbot