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: upgrade assets controllers to version 44 #28472

Merged
merged 47 commits into from
Nov 16, 2024

Conversation

bergeron
Copy link
Contributor

@bergeron bergeron commented Nov 14, 2024

Description

Upgrades the assets controllers to version 44. And starts replacing some instances of https://github.com/MetaMask/eth-token-tracker with reading state from the TokenBalancesController

Open in GitHub Codespaces

Related issues

Manual testing steps

No visual changes. Token balances should render correctly like before on the tokens page, and when switching accounts and chains.

Screenshots/Recordings

Before

After

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.

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.

@bergeron bergeron changed the base branch from develop to brian/upgrade-assets-controllers-43 November 14, 2024 23:43
@bergeron bergeron marked this pull request as ready for review November 15, 2024 23:12
@bergeron bergeron requested review from a team as code owners November 15, 2024 23:12
@bergeron bergeron removed the request for review from a team November 15, 2024 23:21
},
{
address: '0x0bc529c00C6401aEF6D220BE8C6Ea1667F6Ad93e',
symbol: 'YFI',
balance: '1409247882142934',
decimals: 18,
string: '0.001409247882142934',
string: 0.00141,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like the values are changing, but it actually keeps them (mostly) the same. The old behavior of mocking ./useTokenTracker was modifying this array during the test, so this just updates to the same final result.

Screenshot 2024-11-15 at 11 22 22 AM

@@ -54,10 +54,11 @@ export const useAccountTotalFiatBalance = (
const primaryTokenImage = useSelector(getNativeCurrencyImage);
const nativeCurrency = useSelector(getNativeCurrency);

const { loading, tokensWithBalances } = useTokenTracker({
const loading = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have an easy way to replicate the loading flag. Both because the token balances controller doesn't currently set any state indicating an update is in progress. And because in the multichain world, it may not be a single request we're waiting on. Each network will update and return separately. We don't have UI to indicate that specific balances are loading.

Copy link
Contributor Author

@bergeron bergeron Nov 15, 2024

Choose a reason for hiding this comment

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

Reading balances from state will have some UI benefits though. Since we don't have to wait for requests to return, we can always immediately show what we have in state, without an initial 'loading' flash. The values may be stale though, so in the future we may want to indicate which balances are very stale. Perhaps the controller could timestamp in state the time of the last update for each balance.

@metamaskbot
Copy link
Collaborator

Builds ready [613a07a]
Page Load Metrics (2019 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint18592323202612158
domContentLoaded18292243198111153
load18502327201911957
domInteractive32173523215
backgroundConnect995362613
firstReactRender522871377335
getState432551286330
initialActions01000
loadScripts13421745151010249
setupStore682172110
uiStartup209229992506266128
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 919 Bytes (0.01%)
  • ui: 2.2 KiB (0.03%)
  • common: 18.7 KiB (0.23%)

@bergeron bergeron added this pull request to the merge queue Nov 16, 2024
Merged via the queue into develop with commit 8b02ae4 Nov 16, 2024
77 checks passed
@bergeron bergeron deleted the brian/upgrade-assets-controllers-44 branch November 16, 2024 00:50
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants