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

fix: fix network switch on dapp #30211

Merged
merged 6 commits into from
Feb 14, 2025
Merged

fix: fix network switch on dapp #30211

merged 6 commits into from
Feb 14, 2025

Conversation

salimtb
Copy link
Contributor

@salimtb salimtb commented Feb 7, 2025

Description

Open in GitHub Codespaces

Related issues

Fixes: #30149

Manual testing steps

  1. Go to dapp ( sushi swap for instance )
  2. switch network
  3. check the list of tokens

Screenshots/Recordings

Before

After

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

Copy link
Contributor

github-actions bot commented Feb 7, 2025

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.

@salimtb salimtb marked this pull request as ready for review February 7, 2025 18:25
@metamaskbot
Copy link
Collaborator

Builds ready [0e1dcb8]
Page Load Metrics (1900 ± 139 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint70327691814367176
domContentLoaded159726901865285137
load160527451900289139
domInteractive25122472612
backgroundConnect97731209
firstReactRender1673352110
getState563262210
initialActions01000
loadScripts115121281372247119
setupStore87717189
uiStartup187229652181301145
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 303 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [1ab8ebe]
Page Load Metrics (1687 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15112008168812158
domContentLoaded14951935165611455
load15171948168710852
domInteractive24172423215
backgroundConnect1379332110
firstReactRender17176413818
getState75721199
initialActions0483105
loadScripts10581478120610450
setupStore65917178
uiStartup17082353196515474
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 303 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Comment on lines +6126 to +6130
if (chainId && Object.keys(tokenNetworkFilter).length === 1) {
this.preferencesController.setPreference('tokenNetworkFilter', {
[chainId]: true,
});
}
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya Feb 13, 2025

Choose a reason for hiding this comment

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

Hmmm what is this condition doing here?

Object.keys(tokenNetworkFilter).length === 1

this related to portfolio view (you are only viewing tokens from 1 network, so not in portfolio view).

this.preferencesController.setPreference(... {[chainId]: true})

this is to swap the tokens I'm interested in to the newly selected network?

Copy link
Contributor

Choose a reason for hiding this comment

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

So if I am in portfolio view, and I switch networks from a dapp, I stay in portfolio view?
But what if it is a not popular network?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what we're doing is checking if the chainId is defined and whether the tokenNetworkFilter corresponds to the current network—something like { '0x1': true }. Then, if we switch from Ethereum to BNB in the dapp, we would update this to { '0x38': true }.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep nice. I've ran the branch and validated the fix.

This is a little confusing though, in the future do you think it would be worth moving this (token filter list) logic in a shared location/module/utilities? We can discuss this later.

@metamaskbot
Copy link
Collaborator

Builds ready [0bb5bc9]
Page Load Metrics (1622 ± 48 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31718981557302145
domContentLoaded1448186716069948
load14561881162210148
domInteractive237534168
backgroundConnect85021157
firstReactRender1472342311
getState45911147
initialActions01000
loadScripts994138111568943
setupStore76914178
uiStartup16632362186515172
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 303 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@salimtb salimtb added this pull request to the merge queue Feb 14, 2025
Merged via the queue into main with commit 1c4ae15 Feb 14, 2025
73 checks passed
@salimtb salimtb deleted the fix/fix-network-switch-dapp branch February 14, 2025 10:43
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2025
@metamaskbot metamaskbot added the release-12.14.0 Issue or pull request that will be included in release 12.14.0 label Feb 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.14.0 Issue or pull request that will be included in release 12.14.0 team-assets
Projects
Archived in project
4 participants