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: Token Network Filter UI [Extension] #27884

Merged
merged 43 commits into from
Oct 30, 2024

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Oct 16, 2024

Description

Adds Token network filter controls. Note that this is not fully functional, and is currently blocked by two PRs before it can be fully integrated:

  1. feat: show all tokens for all chains in the token list #27785
  2. fix: should not reset market data after switch network core#4832

In the meantime, this PR is set behind a feature flag FILTER_TOKENS_TOGGLE and can be run as follows:

FILTER_TOKENS_TOGGLE=1 yarn webpack --watch
Alternatively: FILTER_TOKENS_TOGGLE=1 yarn start

Open in GitHub Codespaces

Included in this PR:

  1. Adds new tokenNetworkFilter preference to PreferencesController to manage which chains should be considered when filtering tokens
  2. Adds an action to update this preference by All Networks (no filters) and Current Network { [chainId]: true) } this is meant to be flexible enough to support multiple chains in the future.
  3. Adds filterAssets function in a similar style to sortAssets it should be configuration based, and should be extensible enough to support filtering assets by deeply nested values (NFT traits), and to also support complex filter types (like price ranges).
  4. Dropdown should show the balance for the selected network

Not included in this PR:

  1. Aggregated balance across chains. Blocked by fix: should not reset market data after switch network core#4832 and currently hardcoded to $1000
  2. Token lists will not be filtered in this PR. Blocked by feat: show all tokens for all chains in the token list #27785

Related issues

https://github.com/orgs/MetaMask/projects/85/views/35?pane=issue&itemId=82217837
https://consensyssoftware.atlassian.net/browse/MMASSETS-430

Manual testing steps

Token Filter selection should persist through refresh
Current chain balance should reflect the balance of the current chain
Should visibly match designs: https://www.figma.com/design/aMYisczaJyEsYl1TYdcPUL/Portfolio-View?node-id=5750-47217&node-type=canvas&t=EjOUPnqy7tWZE6sV-0

Screenshots/Recordings

Screen.Recording.2024-10-22.at.10.46.39.AM.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

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.

@gambinish gambinish changed the title feat: Token Network Filter UI feat: Token Network Filter UI [Extension] Oct 16, 2024
@gambinish
Copy link
Contributor Author

Demo of WIP:

Screen.Recording.2024-10-17.at.7.43.44.AM.mov

@gambinish gambinish marked this pull request as ready for review October 22, 2024 18:04
@gambinish gambinish requested a review from a team as a code owner October 22, 2024 18:04
@gambinish gambinish added team-assets needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. labels Oct 22, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [441d4cf]
Page Load Metrics (2190 ± 100 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31224301886664319
domContentLoaded18102688215219895
load185326952190207100
domInteractive318347168
backgroundConnect9127413416
firstReactRender822171173416
getState697302914
initialActions01000
loadScripts13382018159715976
setupStore12101392914
uiStartup205629722470221106
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 5.5 KiB (0.07%)
  • common: 359 Bytes (0.00%)

andreahaku
andreahaku previously approved these changes Oct 25, 2024
Copy link

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

LGTM

@darkwing
Copy link
Contributor

Seeing this with a yarn start locally:

SCR-20241028-ntqu
RPC (npm:@metamask/message-signing-snap): 
Object
 -> 
Object
 TypeError: Cannot read properties of undefined (reading '0x1') '\n  at AssetListControlBar (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-4.js:1765:3)\n  at AssetList (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-4.js:1928:3)\n  at div\n  at chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-9.js:23983:3\n  at div\n  at Box (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-7.js:3824:3)\n  at div\n  at Box (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-7.js:3824:3)\n  at Tabs (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-7.js:12540:3)\n  at div\n  at chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-9.js:23983:3\n  at AccountOverviewTabs (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-6.js:2362:3)\n  at AccountOverviewLayout (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-6.js:2316:3)\n  at AccountOverviewEth\n  at AccountOverview (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-6.js:2536:48)\n  at div\n  at div\n  at div\n  at Home (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-10.js:8410:5)\n  at ConnectFunction (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-8.js:39902:41)\n  at C (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:16054:37)\n  at Route (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:15797:29)\n  at Authenticated (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-8.js:1193:5)\n  at ConnectFunction (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-8.js:39902:41)\n  at Switch (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:15999:29)\n  at div\n  at chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-9.js:23983:3\n  at div\n  at Routes (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-11.js:4667:1)\n  at ConnectFunction (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-8.js:39902:41)\n  at C (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:16054:37)\n  at MetamaskNotificationsProvider (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-7.js:14417:3)\n  at CurrencyRateProvider (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-7.js:14294:3)\n  at LegacyI18nProvider (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-10.js:323:1)\n  at I18nProvider (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-10.js:306:53)\n  at LegacyMetaMetricsProvider (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-7.js:14604:1)\n  at MetaMetricsProvider (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-7.js:14514:3)\n  at RenderedRoute (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:16767:7)\n  at Routes (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:17468:7)\n  at Router (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:17402:17)\n  at CompatRouter (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:13274:7)\n  at Router (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:15416:30)\n  at HashRouter (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-3.js:13755:35)\n  at Provider (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/common-8.js:39609:20)\n  at Index (chrome-extension://oehplhbehlbgjdapfkoemdgjlgbjpkkg/ui-10.js:9298:5)'

@metamaskbot
Copy link
Collaborator

Builds ready [cf78624]
Page Load Metrics (1824 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint37719931750327157
domContentLoaded1608193317938239
load1616199118248943
domInteractive216943168
backgroundConnect879272110
firstReactRender50198913014
getState46217189
initialActions01000
loadScripts1165143613107034
setupStore1199342814
uiStartup18102354202611756
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 5.49 KiB (0.07%)
  • common: 359 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [82255bc]
Page Load Metrics (1888 ± 101 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint167926641884213102
domContentLoaded167126521861210101
load168026681888210101
domInteractive1792442110
backgroundConnect979262110
firstReactRender543061085526
getState47215199
initialActions01000
loadScripts11602116136720196
setupStore1270322411
uiStartup187729032118229110
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 5.49 KiB (0.07%)
  • common: 359 Bytes (0.00%)

const allNetworks = useSelector(getNetworkConfigurationsByChainId);
const isTestnet = useSelector(getIsTestnet);
const { tokenNetworkFilter, showNativeTokenAsMainBalance } =
useSelector(getPreferences);
Copy link
Contributor

@salimtb salimtb Oct 30, 2024

Choose a reason for hiding this comment

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

in the future , we can use this component somewhere else , like on this PR for example , my suggestion is to make it more generic.
same think for the popover

src={currentNetwork?.rpcPrefs?.imageUrl}
/>
</Box>
</SelectableListItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

as future improvement , we need to implement a storybook + unit test for this.
can you add a todo for that ?

{ name: 'Token4', symbol: 'T4', chainId: '0x89', balance: 150 },
];

test('filters by inclusive chainId', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: should we use it by convention here ? most of tests are using it but i'm wondering if we should respect this rule

@gambinish gambinish added this pull request to the merge queue Oct 30, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [d45f754]
Page Load Metrics (2079 ± 131 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint38326891976449215
domContentLoaded171425972056269129
load172426092079273131
domInteractive19121552814
backgroundConnect768272110
firstReactRender614091498842
getState560192110
initialActions01000
loadScripts125919611537227109
setupStore1184342613
uiStartup192633792405414199
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 22 Bytes (0.00%)
  • ui: 5.49 KiB (0.07%)
  • common: 359 Bytes (0.00%)

Merged via the queue into develop with commit c04c119 Oct 30, 2024
76 checks passed
@gambinish gambinish deleted the feat/mmassets-432_network-filter-extension branch October 30, 2024 16:45
@github-actions github-actions bot locked and limited conversation to collaborators Oct 30, 2024
@metamaskbot metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 30, 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.7.0 Issue or pull request that will be included in release 12.7.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants