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.

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

feat: aggregated balance feature #27097

Merged
merged 37 commits into from
Oct 2, 2024
Merged

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Sep 12, 2024

Description

  • Removes the primary currency from Settings => General

  • Removes usage of useNativeCurrencyAsPrimaryCurrency

  • Adds a new toggle in settings: "show native tokens as main balance" which affects only the main coin overview.

  • When new setting is ON we will show to users the aggregated balance of their tokens in Fiat, else we show the balance in crypto for the native token.

This final PR is the combination of the following PRs:

1- Removal of useNativeCurrencyAsPrimaryCurrency setting and adding the new setting: #26870
2- Aggregated balance logic: #27108
3- Aggregated balance UI: #27161

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/orgs/MetaMask/projects/85/views/24?filterQuery=label%3A%22assets-aggregated%22
Figma: https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=4098-126568&node-type=instance&focus-id=4186-130770&m=dev

#27280

Manual testing steps

  1. Switch to Ethereum mainnet and go to home page
  2. You should see the popover telling you about the new feature. You should not be able to see the popover again once closed.
  3. You should be able to see the aggregated balance in fiat of you native token and imported ERC20 tokens
  4. Go to settings; notice that there is no primary currency setting and you should see the new setting.
  5. Turn on "show native token as main balance" and go back to home page; you should see your balance in crypto for the native token.
  6. Switch to any testnet, exp (Sepolia), notice that you wont be able to see aggregated balance unless you turn on the setting "Show conversion on test networks" in settings => Advanced

Screenshots/Recordings

Before

After

Screen.Recording.2024-09-23.at.11.07.15.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.

#26870)

## **Description**

This PR is the initial PR for the aggregated balance feature.

This PR:

1- Adds a new setting in Settings => General called; show native token
as main balance.

This setting is OFF by default.

When the new setting is off; in home page` coin-overview.tsx`; we should
see user balance in fiat, and when it is ON we should see user balance
in crypto.

In the main token list, we display native currency of the network as the
first token; we should see balance in crypto as Primary and native as
secondary if the new setting ON. When the toggle is OFF we should see
fiat as primary and native as secondary.

=> This PR removes the display of secondary balance in the balance
overview section in the home page.

2- Removes the fiat/native setting `useNativeCurrencyAsPrimaryCurrency`
from the Settings => General page
After removing this setting; we will be displaying by default crypto
amounts first in the UI.


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26870?quickstart=1)

## **Related issues**

Fixes:

MetaMask/MetaMask-planning#3193 (comment)

MetaMask/MetaMask-planning#3184 (comment)

## **Manual testing steps**

Test the new toggle in settings:

1. Create a new user or upgrade an old user
2. Go to Settings => General; you should see a new toggle "show native
token as main balance" and it should be OFF by default
3. Go to home page; because the toggle is OFF, you should see your
balance in fiat.
4. You should not see your secondary balance in the balance overview
section.
5. Click on Tokens tab, you should see your balances for the native
token of your network; should display fiat balance first then native
balance.
6. Go to Settings=>General and turn the new setting ON
7. Go back to home page; you should see your native balance in crypto in
the home section.
8. The native token in token list should display balance in crypto first
then in fiat.

Test the removal of the `useNativeCurrencyAsPrimaryCurrency`

1. New users or users who upgrade should not see any errors.
2. Go to settings=> general; you should not see the primary currency
setting
3. In the send flow and in UI in general you should always see crypto
amounts displayed first;
=> NOTE: The account list UI and the main tokenList for ERC20 displays
fiat first always; this was the case also when we had
`useNativeCurrencyAsPrimaryCurrency`, so the behavior remains untouched.


## **Screenshots/Recordings**

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

## Setting page
### **Before**
<img width="1797" alt="Screenshot 2024-09-04 at 17 11 41"
src="https://github.com/user-attachments/assets/b9e1f87d-588b-4d75-8f68-21aa3f04a6fa">

### **After**
<img width="1130" alt="Screenshot 2024-09-04 at 18 27 57"
src="https://github.com/user-attachments/assets/30c0429b-af20-4cff-89c4-71bbb3782c60">


## Home page

### **Before**
With crypto as primary currency:
<img width="1123" alt="Screenshot 2024-09-04 at 17 15 51"
src="https://github.com/user-attachments/assets/7b5f0f00-3921-42bc-b8ca-f7d7addcfde6">

With fiat as primary currency:
<img width="1121" alt="Screenshot 2024-09-04 at 17 16 38"
src="https://github.com/user-attachments/assets/231c7e56-276c-4733-a81b-49ab74f0ec56">


### **After**
With new setting show Native token as main balance ON

<img width="1128" alt="Screenshot 2024-09-04 at 18 29 15"
src="https://github.com/user-attachments/assets/7abcb661-82b8-4ef7-a720-0fd15ae6859e">

With new setting show Native token as main balance OFF
<img width="1128" alt="Screenshot 2024-09-04 at 18 29 47"
src="https://github.com/user-attachments/assets/872e439a-f8dc-4af3-845a-17cf2bb02d04">


## Account list

### **Before**
Is unchanged regardless of the primary currency setting
<img width="1440" alt="Screenshot 2024-09-04 at 17 19 00"
src="https://github.com/user-attachments/assets/1887d07b-8f1d-4418-b66d-575dd576a26e">

### **After**
<img width="389" alt="Screenshot 2024-09-04 at 18 30 09"
src="https://github.com/user-attachments/assets/e76dbe1b-4ca8-4c09-b466-1ecf89f1017d">


## Send page (For sending native currency)

### **Before**
With crypto as primary currency
<img width="412" alt="Screenshot 2024-09-04 at 17 21 39"
src="https://github.com/user-attachments/assets/efce0db3-cb76-4451-be05-1d98f00988f7">
With fiat as primary currency
<img width="420" alt="Screenshot 2024-09-04 at 17 28 26"
src="https://github.com/user-attachments/assets/3ed45599-f423-4111-bd50-acc83f59d984">

### **After**
This UI does not affected by the new setting.
<img width="434" alt="Screenshot 2024-09-04 at 18 31 41"
src="https://github.com/user-attachments/assets/6919d6d3-f836-4718-a7f6-fdb346cd4e34">


### Send confirmation page (For sending native currency)

### **Before**
With crypto as primary currency
<img width="427" alt="Screenshot 2024-09-04 at 17 38 01"
src="https://github.com/user-attachments/assets/88b96c32-f116-4303-aefb-b8c0ea4b35e9">

With fiat as primary currency
<img width="427" alt="Screenshot 2024-09-04 at 17 29 26"
src="https://github.com/user-attachments/assets/6bc0b041-65a1-4a52-abd3-a3584c3d4c87">

### **After**
<img width="440" alt="Screenshot 2024-09-04 at 18 32 57"
src="https://github.com/user-attachments/assets/fa72c4b9-97dc-437a-bac8-7347b4837d8c">


## Send page (For sending ERC20 token)

### **Before**
With crypto as primary currency
<img width="425" alt="Screenshot 2024-09-04 at 17 43 43"
src="https://github.com/user-attachments/assets/931a4da4-dc51-4d67-b337-f16d0d399421">
With fiat as primary currency
<img width="424" alt="Screenshot 2024-09-04 at 17 44 26"
src="https://github.com/user-attachments/assets/042b32d1-5b7e-4a4a-8c13-373fabc7daa4">

### **After**
<img width="427" alt="Screenshot 2024-09-04 at 18 33 34"
src="https://github.com/user-attachments/assets/ab05f568-3696-4ecc-843c-aea54bbe4d52">

### Send confirmation page (For sending ERC20 token)

### **Before**
With crypto as primary currency
<img width="438" alt="Screenshot 2024-09-04 at 17 45 25"
src="https://github.com/user-attachments/assets/41aa5172-7261-4ce6-8b65-f2a929365f3a">
With fiat as primary currency
<img width="431" alt="Screenshot 2024-09-04 at 17 58 24"
src="https://github.com/user-attachments/assets/6ceb0d64-2031-4101-bd9c-b040391aa862">

### **After**
<img width="442" alt="Screenshot 2024-09-04 at 18 34 03"
src="https://github.com/user-attachments/assets/1820dc03-9c71-43e5-932c-34c761ea159f">

All components in the codebase should display crypto by default.


## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-extension/blob/develop/.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.
Copy link
Contributor

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.

@sahar-fehri sahar-fehri changed the title feat: add new setting and remove useNativeCurrencyAsPrimaryCurrency s… feat: aggregated balance feature Sep 12, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [c0e47ba]
Page Load Metrics (1714 ± 133 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint18428521566527253
domContentLoaded149328291695277133
load152728461714277133
domInteractive249039167
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 917 Bytes (0.03%)
  • ui: -3.78 KiB (-0.05%)
  • common: -181 Bytes (-0.00%)

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 93.45794% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.05%. Comparing base (56c23fc) to head (ce79a77).
Report is 2 commits behind head on develop.

Files with missing lines Patch % Lines
...saction-base/confirm-transaction-base.component.js 0.00% 2 Missing ⚠️
...ages/confirmations/send/gas-display/gas-display.js 0.00% 2 Missing ⚠️
ui/store/actions.ts 0.00% 2 Missing ⚠️
...wallet-overview/aggregated-percentage-overview.tsx 97.14% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #27097      +/-   ##
===========================================
+ Coverage    70.01%   70.05%   +0.04%     
===========================================
  Files         1445     1447       +2     
  Lines        50201    50209       +8     
  Branches     14045    14034      -11     
===========================================
+ Hits         35147    35172      +25     
+ Misses       15054    15037      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

## **Description**

This PR adds the logic for aggregated balance feature.



[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27108?quickstart=1)

## **Related issues**

Fixes:
MetaMask/MetaMask-planning#3185 (comment)

## **Manual testing steps**

1. Go to settings => general and make sure that by default the "Show
native token as main balance" setting is off
2. Go to home page; you should be able to see your aggregated balance in
fiat.
3. Go to settings => general and enable the toggle for "Show native
token as main balance"
4. go back to main page; you should see you balance in crypto for the
native currency

## **Screenshots/Recordings**

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

### **Before**

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

### **After**



https://github.com/user-attachments/assets/f86cbf0d-c631-408d-8c78-efb007e86771


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

## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-extension/blob/develop/.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.
@metamaskbot
Copy link
Collaborator

Builds ready [383715c]
Page Load Metrics (1669 ± 121 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22024091554499240
domContentLoaded142723511651244117
load145924111669252121
domInteractive227633147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 917 Bytes (0.03%)
  • ui: 927 Bytes (0.01%)
  • common: -76 Bytes (-0.00%)

Copy link

sonarcloud bot commented Sep 17, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [ce79a77]
Page Load Metrics (1837 ± 107 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26723071733395190
domContentLoaded158322971805224108
load159323181837223107
domInteractive15130442914
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 917 Bytes (0.03%)
  • ui: 1.1 KiB (0.02%)
  • common: -76 Bytes (-0.00%)

## **Description**

Aggregated balance UI feature;

This PR fixed UI designs related to the aggregated balance feature;
It also fixes the popover behavior;
Users should be able to see the popover by default and should see it
only once.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27161?quickstart=1)

## **Related issues**

Fixes:
Related:

https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=4098-126757&node-type=instance&focus-id=4186-130770&m=dev

https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=4496-20506&node-type=FRAME&m=dev

## **Manual testing steps**

1. After a new user is onboarded or upgraded, they should be able to see
the aggregated balance popover.
2. Close the popover. Make sure you are not able to see the popover
again even after turning on and off the new setting "show native token
as main balance"

## **Screenshots/Recordings**

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

### **Before**

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

### **After**


https://github.com/user-attachments/assets/4c01541e-0d77-46c3-8a21-98dc3114e11d




## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.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-extension/blob/develop/.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.
@metamaskbot
Copy link
Collaborator

Builds ready [25d3df5]
Page Load Metrics (1594 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27821551537322155
domContentLoaded14581989156311656
load14822088159413263
domInteractive137033168
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.1 KiB (0.03%)
  • ui: 3.17 KiB (0.04%)
  • common: 160 Bytes (0.00%)

@sahar-fehri sahar-fehri marked this pull request as ready for review September 23, 2024 09:15
@plasmacorral
Copy link
Contributor

plasmacorral commented Sep 28, 2024

Of everything listed above, the most concerning to me are 5, 13 and 14. I would recommend to stakeholders those be resolved or researched before shipping.

@sahar-fehri
Copy link
Contributor Author

sahar-fehri commented Sep 29, 2024

Of everything listed above, the most concerning to me are 5, 13 and 14. I would recommend to stakeholders those be resolved or researched before shipping.

Hi @plasmacorral ! Thanks again 🙏 13 issue should be fixed here 339c70d

Video:

upgrade2.mov

Were you also able to see 5 and 14 on chrome? I would expect that 5 is also present on develop; that url is related to the phishing controller which is not a flow that i have updated in this PR 🤔

14- I tested the toggle again and i can't seem to find exact steps to reproduce the repeated tap behavior you had 🤔

@gambinish gambinish self-requested a review September 30, 2024 16:50
gambinish
gambinish previously approved these changes Sep 30, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [203bf04]
Page Load Metrics (1747 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint49422981685319153
domContentLoaded15442102172713163
load15522214174715173
domInteractive1494452311
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.21 KiB (0.06%)
  • ui: 2.42 KiB (0.03%)
  • common: 148 Bytes (0.00%)

@plasmacorral
Copy link
Contributor

Thanks for the video to show migration was resolved! Observations 5 and 14 were from Firefox, I did not see them in Chrome.

@plasmacorral plasmacorral removed DO-NOT-MERGE Pull requests that should not be merged QA'd but questions A QA run through has been done but you need clarification on minor issues you found labels Oct 1, 2024
Copy link

sonarcloud bot commented Oct 2, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [5b79963]
Page Load Metrics (1836 ± 101 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint149225001838223107
domContentLoaded148424941807219105
load156824991836210101
domInteractive23148513115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 2.21 KiB (0.06%)
  • ui: 2.42 KiB (0.03%)
  • common: 148 Bytes (0.00%)

@sahar-fehri sahar-fehri added this pull request to the merge queue Oct 2, 2024
Merged via the queue into develop with commit d26968f Oct 2, 2024
77 checks passed
@sahar-fehri sahar-fehri deleted the feat/aggregated-balance-feature branch October 2, 2024 13:42
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-assets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

10 participants