feat: Sort/Import Tokens in Extension #27184

gambinish · 2024-09-17T03:01:04Z

Description

This PR adds Token sorting to the Asset List page, and also moves Token importing to the top of the Token List. A few of the main changes introduced:

Include NativeToken in TokenList component to be included in sorting logic, and treated (as far as sorting is concerned) as any other token in the list
Intoduce a tokenSortConfig into state that keeps track of the sort order, the key being sorted by, and the direction of the sort order. Also includes an action to update this state.
Introduce a useEffect that subscribes to tokenSortConfig as well as a few other application state variables to update and sort tokenList when appropriate.
Clean up asset-list component, and move some of it's relevant code into the useAccountTotalFiatBalance

Acceptance Criteria:

Tokens should be sorted by default by declining balance
Sort controls should sort tokens alphabetically, and by decreasing fiat token balance
Sort order should persist through refresh
Sort order should persist after app is closed and reopened
When a token gets imported, it should be included in the sort list, in the correct order in the list

A couple of disclaimers. There are still (at least) two bugs that I discovered that were not caught by tests:

~~When toggling preferred currency setting, Native Token sorted incorrectly by decreasing fiat balance~~ ✅ fixed
~~When switching between accounts, token list does not update~~ ✅ fixed

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/MMASSETS-356

Manual testing steps

Go to AssetList page, and click dropdown and select option to sort by
Tokens should sort, and remain sorted through refresh and application close/open (it is in state)
Importing tokens should import them into the sort order

Screenshots/Recordings

Screen.Recording.2024-09-16.at.7.57.32.PM.mov

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

…nsion

…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension

…nsion

…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension

…nsion

…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension

…nsion

metamaskbot · 2024-09-18T22:48:45Z

Builds ready [8b8f05d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1653 ± 77 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	400	2104	1590	317	152
		domContentLoaded	1380	2063	1630	158	76
		load	1388	2101	1653	160	77
		domInteractive	12	165	47	44	21

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 192 Bytes (0.00%)
ui: 15.2 KiB (0.21%)
common: 351 Bytes (0.00%)

darkwing

Added some code nits! Overall I really like what you've done!

darkwing · 2024-09-18T22:48:59Z

test/e2e/tests/tokens/custom-token-send-transfer.spec.js

@@ -136,6 +136,9 @@ describe('Transfer custom tokens @no-mmi', function () {
          text: '-1.5 TST',
        });

+        // this delay helps prevent flakiness. it allows driver to wait until send transer is "confirmed"
+        await driver.delay(5000);


Is there a selector we can poll for here? We really want to get away from these types of generic delays.

darkwing · 2024-09-18T22:50:55Z

ui/components/app/assets/asset-list/asset-list-control-bar/asset-list-control-bar.tsx

+          borderStyle={BorderStyle.solid}
+          color={TextColor.textDefault}
+        >
+          Sort By


This needs localization

darkwing · 2024-09-18T22:51:43Z

ui/components/app/assets/asset-list/sort-control/index.scss

+  padding-left: 16px;
+  padding-right: 16px;
+  padding-top: 16px;
+  padding-bottom: 16px;


We can just do padding: 16px;

Good call. I should probably use the design system props here actually

darkwing · 2024-09-18T22:52:11Z

ui/components/app/assets/asset-list/sort-control/sort-control.tsx

+        isSelected={tokenSortConfig.key === 'symbol'}
+        onClick={() => handleSort('symbol', 'alphaNumeric', 'asc')}
+      >
+        Alphabetically (A-Z)


Needs localization

darkwing · 2024-09-18T22:52:15Z

ui/components/app/assets/asset-list/sort-control/sort-control.tsx

+        isSelected={tokenSortConfig.key === 'tokenFiatAmount'}
+        onClick={() => handleSort('tokenFiatAmount', 'stringNumeric', 'dsc')}
+      >
+        Declining balance ($ high-low)


Needs localization

gambinish · 2024-09-18T23:38:32Z

Closed my previous PR, but want to surface this comment as it is still relevant regarding selectable-list-item component: #27084 (comment)

gambinish · 2024-09-18T23:40:47Z

ui/pages/confirmations/components/confirm/header/header-info.tsx

@@ -114,7 +114,7 @@ const HeaderInfo = () => {
          alignSelf: 'flex-end',
        }}
      >
-        <Tooltip position="bottom" title={t('accountDetails')} interactive>


This was also breaking an e2e test. I'm wondering if someone on @MetaMask/confirmations can help me understand this

sahar-fehri · 2024-09-19T07:19:09Z

ui/components/app/assets/asset-list/asset-list-control-bar/asset-list-control-bar.tsx

+  const controlBarRef = useRef<HTMLDivElement>(null); // Create a ref
+  const [isPopoverOpen, setIsPopoverOpen] = useState(false);
+  // TODO: Replace `any` with type
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any


Is this TODO to be removed?

sahar-fehri · 2024-09-19T07:44:16Z

ui/components/app/assets/asset-list/sort-control/sort-control.tsx

+  ) => {
+    dispatch(
+      setTokenSortConfig({
+        key,


Looks like we are setting tokenSortConfig only in this component and i did not see a default value in preferences-controller for tokenSortConfig,

this could result in this selector erroring

const tokenSortConfig = useSelector((state: any) => { return state.metamask.preferences.tokenSortConfig; });

Similary to what you added in fixture-builder; im thinking we can add in preferences controller default values for tokenSortConfig?

tokenSortConfig: { key: 'tokenFiatAmount', order: 'dsc', sortCriteria: 'stringNumeric', },

I think we will also need to create a new migration for users who upgrade to add tokenSortConfig preference into their state 🙏

Great points, ty for pointing this out

sahar-fehri · 2024-09-19T08:02:35Z

ui/components/app/assets/asset-list/asset-list-control-bar/asset-list-control-bar.tsx

+  // TODO: Replace `any` with type
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  const tokenSortConfig = useSelector((state: any) => {
+    return state.metamask.preferences.tokenSortConfig;


We can maybe create a selector for this?
const tokenSortConfig = useSelector(getTokenSortConfig);

and then in selectors.js

export function getTokenSortConfig(state) { const { tokenSortConfig } = getPreferences(state); return tokenSortConfig; }

sahar-fehri · 2024-09-19T08:12:08Z

ui/store/actions.ts

          if (err) {
            dispatch(displayWarning(err));
            reject(err);
            return;
          }
+          console.log('updatedPreferences', updatedPreferences);


console to remove

sahar-fehri · 2024-09-19T09:24:22Z

Looks great! 🎉

I noticed this behavior where when you click on the sort selector; then you click outside without choosing a select option; the dropdown does not close; is that intentional?

Screen.Recording.2024-09-19.at.11.21.35.mov

ui/components/app/assets/asset-list/asset-list-control-bar/asset-list-control-bar.tsx

sahar-fehri · 2024-09-19T09:36:57Z

ui/components/app/assets/asset-list/native-token/use-native-token-balance.ts

+  const showFiat = useSelector(getMultichainShouldShowFiat);
+  const primaryTokenImage = useSelector(getMultichainCurrencyImage);
+  const { useNativeCurrencyAsPrimaryCurrency } = useSelector(getPreferences);
+  const { chainId, ticker, type, rpcUrl } = useSelector(


Just mentioning thatuseNativeCurrencyAsPrimaryCurrency will be removed in the aggregated balance feature;

Yup, this is definitely something that we are going to have conflicts with, hoping it should just be an easy swap. Will keep this comment open to keep eyes on this

sahar-fehri · 2024-09-19T09:57:47Z

The sort selector looks good in MM small view; but in extended view, maybe its just me, but it feels like there is too much blank space?

gambinish · 2024-09-19T21:53:09Z

The sort selector looks good in MM small view; but in extended view, maybe its just me, but it feels like there is too much blank space?

I also noticed this. I will circle back with design for feedback

…nsion--persistence-telemetry

…and ETH

sonarcloud · 2024-09-21T00:30:38Z

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

gambinish and others added 30 commits September 5, 2024 12:23

Convert js files to ts

da7a49d

Update to non-deprecated modal

ede7e00

Update snapshots

7d8aee9

Use types instead of interfaces

24026bc

Prefer object destructuring

b787923

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

168b32e

…nsion

Init sortAssets logic

2e688f2

Working

159622c

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

3052a92

…nsion

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

a902a3f

…nsion

Merge branch 'feat/mmassets-356_sort-import-tokens-extension' of gith…

094fecb

…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

e348b10

…nsion

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

5055394

…nsion

Temporary fix to sorting logic

95881ef

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

d424655

…nsion

Merge branch 'feat/mmassets-356_sort-import-tokens-extension' of gith…

3a7c669

…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension

Init dropdown styles

6fad3b2

Init css for SelectableListItem

b7d6e47

Polish controlbar styles

0d7c10e

Cleanup

313af19

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

4a45990

…nsion

Merge branch 'feat/mmassets-356_sort-import-tokens-extension' of gith…

43d3c3b

…ub.com:MetaMask/metamask-extension into feat/mmassets-356_sort-import-tokens-extension

Use tsignore for string variable in token-cell

0f7b5ac

Remove unecessary ts-ignores, and unskip unit tests

41f528a

Merge main, address conflicts in asset-list.tsx

6dd32e4

Update snapshots

5c5b22e

Remove foo

aeee661

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

0c93057

…nsion

Convert asset-list.test to ts

2bdb02f

Update token-cell tests to ts

692001c

gambinish requested review from a team as code owners September 18, 2024 22:35

gambinish requested review from sahar-fehri, salimtb and darkwing September 18, 2024 22:35

darkwing requested changes Sep 18, 2024

View reviewed changes

gambinish mentioned this pull request Sep 18, 2024

feat: Sort/Import tokens 🚧 #27084

Closed

7 tasks

gambinish commented Sep 18, 2024

View reviewed changes

sahar-fehri reviewed Sep 19, 2024

View reviewed changes

ui/components/app/assets/asset-list/asset-list-control-bar/asset-list-control-bar.tsx Show resolved Hide resolved

sahar-fehri reviewed Sep 19, 2024

View reviewed changes

gambinish and others added 10 commits September 19, 2024 14:53

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

fe07229

…nsion--persistence-telemetry

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

f38d164

…nsion--persistence-telemetry

Merge branch 'develop' into feat/mmassets-356_sort-import-tokens-exte…

514a8b8

…nsion--persistence-telemetry

Migrate tokenSortConfig in state

58ef0a6

Debug preferences state

899c571

Add migration file

c9cecf2

Add unit tests, begin migration tests

c229c32

Handle case where primaryCurrency preference is toggled between Fiat …

2a0957d

…and ETH

Add unit tests to sort-control.test.tsx

3e0ba18

Update comment

dcb236d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Sort/Import Tokens in Extension #27184

feat: Sort/Import Tokens in Extension #27184

gambinish commented Sep 17, 2024 •

edited

Loading

metamaskbot commented Sep 18, 2024

darkwing left a comment

darkwing Sep 18, 2024

darkwing Sep 18, 2024

darkwing Sep 18, 2024

gambinish Sep 18, 2024

darkwing Sep 18, 2024

darkwing Sep 18, 2024

gambinish commented Sep 18, 2024 •

edited

Loading

gambinish Sep 18, 2024

sahar-fehri Sep 19, 2024

sahar-fehri Sep 19, 2024 •

edited

Loading

gambinish Sep 19, 2024

sahar-fehri Sep 19, 2024

sahar-fehri Sep 19, 2024

sahar-fehri commented Sep 19, 2024 •

edited

Loading

sahar-fehri Sep 19, 2024

gambinish Sep 19, 2024

sahar-fehri commented Sep 19, 2024

gambinish commented Sep 19, 2024

sonarcloud bot commented Sep 21, 2024

feat: Sort/Import Tokens in Extension #27184

Are you sure you want to change the base?

feat: Sort/Import Tokens in Extension #27184

Conversation

gambinish commented Sep 17, 2024 • edited Loading

Description

This PR adds Token sorting to the Asset List page, and also moves Token importing to the top of the Token List. A few of the main changes introduced:

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

metamaskbot commented Sep 18, 2024

darkwing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gambinish commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahar-fehri Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahar-fehri commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahar-fehri commented Sep 19, 2024

gambinish commented Sep 19, 2024

sonarcloud bot commented Sep 21, 2024

Quality Gate passed

gambinish commented Sep 17, 2024 •

edited

Loading

gambinish commented Sep 18, 2024 •

edited

Loading

sahar-fehri Sep 19, 2024 •

edited

Loading

sahar-fehri commented Sep 19, 2024 •

edited

Loading