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: cross chain aggregated balance #28456

Merged
merged 91 commits into from
Nov 20, 2024

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Nov 14, 2024

Description

Branch was ininitially based on top of #28276

This PR calculates balance for current account cross chains while not taking into account the test networks.

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

Start the app with PORTFOLIO_VIEW=1

  1. Go to main page and click on Ethereum network
  2. Import your tokens
  3. You should see the total balance in Fiat of your native + ERC20 tokens
  4. Switch to another chain where you also have tokens; exp polygon
  5. Notice that your total fiat balance is now total fiat balance on Ethereum + current native balance on polygon in fiat
  6. Import any tokens you have on polygon
  7. Notice that the total fiat balance now added up the sum of ERC20 tokens you had on Polygon
  8. Click on the network filter and notice you can see aggregated balance on all networks and aggregated balance on the current network
  9. Switch to "Current network" on the network filter and the main view should now show you the aggregated balance on this network.
  10. Click on account item and notice that you now see the balance in fiat cross networks
  11. Go to settings and turn on the setting "show native token as main balance"; and go back to home page.
  12. You should now see your native balance in crypto
  13. Click on network filter and you should still be able to see the all networks and current network aggregated balance in fiat.

Screenshots/Recordings

Before

After

Screen.Recording.2024-11-19.at.22.41.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.

gambinish
gambinish previously approved these changes Nov 19, 2024
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.

LGTM 🚀

@metamaskbot
Copy link
Collaborator

Builds ready [69ed10a]
Page Load Metrics (2108 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint18802564211419594
domContentLoaded18642504207918689
load18812557210819393
domInteractive319044157
backgroundConnect1185322411
firstReactRender539675126
getState6011093136
initialActions00000
loadScripts13651962157516579
setupStore75511105
uiStartup213329212403227109
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 10.13 KiB (0.13%)
  • common: 934 Bytes (0.01%)

darkwing
darkwing previously approved these changes Nov 19, 2024
@@ -29,6 +29,8 @@
"github.com",
"goerli.infura.io",
"lattice.gridplus.io",
"linea-mainnet.infura.io",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be removed now that more stuff is mocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added mocks for the test that was failing test/e2e/tests/settings/account-token-list.spec.js
But I still had to add sepolia.infura.io to privacy snapshot

@sahar-fehri sahar-fehri dismissed stale reviews from darkwing and gambinish via 0c058aa November 19, 2024 22:42
@metamaskbot
Copy link
Collaborator

Builds ready [d380413]
Page Load Metrics (2051 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint18112301204810751
domContentLoaded17992279201010550
load18102307205111555
domInteractive288845136
backgroundConnect10135403617
firstReactRender583761509747
getState522011184622
initialActions01000
loadScripts13491787152910551
setupStore6461184
uiStartup212629882508278134
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 10.13 KiB (0.13%)
  • common: 934 Bytes (0.01%)

@@ -52,12 +52,13 @@
"security-alerts.api.cx.metamask.io",
"security-alerts.dev-api.cx.metamask.io",
"sentry.io",
"sepolia.infura.io",
Copy link
Contributor

Choose a reason for hiding this comment

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

we'd prefer to mock all places hitting this. I think we expected #28277 to cover all places and, aren't sure which caller remains.

Copy link
Contributor Author

@sahar-fehri sahar-fehri Nov 20, 2024

Choose a reason for hiding this comment

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

Agree; Inspecting the network tab on the test test/e2e/tests/settings/account-token-list.spec.js; the calls to infura that i see are eth_blockNumber, even though i added a mock for those, it is still requiring the snapshot update; not quite sure what i missed there

@metamaskbot
Copy link
Collaborator

Builds ready [fd49581]
Page Load Metrics (2165 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint53224172084387186
domContentLoaded18382405212715072
load18452421216515373
domInteractive298045147
backgroundConnect977422412
firstReactRender612741054321
getState46120852210
initialActions00000
loadScripts13901850160213062
setupStore75212105
uiStartup22852842252815675
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 10.13 KiB (0.13%)
  • common: 934 Bytes (0.01%)

@sahar-fehri sahar-fehri added this pull request to the merge queue Nov 20, 2024
Merged via the queue into develop with commit 4cfd133 Nov 20, 2024
75 checks passed
@sahar-fehri sahar-fehri deleted the feat/cross-chain-aggregated-balance branch November 20, 2024 14:00
@github-actions github-actions bot locked and limited conversation to collaborators Nov 20, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 20, 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.9.0 Issue or pull request that will be included in release 12.9.0 team-assets
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants