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: Multichain AssetList #28386

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Nov 8, 2024

Description

This PR is currently ready for QA testing.

Once these dependent PRs get merged, the diff in this PR should theoretically become much smaller.

Known issues:

  1. Aggregated balances are not yet aggregated across chains. This is currently in progress.
  2. Multichain token detection not yet integrated, as of now this is still per chain
  3. Unit tests are isolated to this PR: fix: Multichain AssetList unit tests #28451
  4. e2e test are isolated to this PR: fix: Multichain AssetList e2e tests #28524

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/orgs/MetaMask/projects/85/views/35?pane=issue&itemId=82217837

Manual testing steps

These are the happy paths we are currently ready to start QA:

  1. Load up a wallet in Metamask that has several accounts, with several networks added to each account. Each account should have tokens for each network. If you are loading this branch locally, you can run this with the following feature flag: yarn && PORTFOLIO_VIEW=1 yarn webpack --watch
  2. When "All Networks" are selected, you should see all your tokens across networks
  3. You can toggle to the current network, which should filter your tokens for that specific network
  4. Assets should correctly navigate to their details page (global network selector should remain unchanged)
  5. Sending/Swapping tokens from the AssetDetails page, from a different network that the globally selected network, should switch networks to the one of the token to be sent, before navigating the user to the send/swap screens. A toast message should inform the user that the network has been changed on their behalf.
  6. When on the AssetList, switching networks should also update the token list to the tokens on the selected account
  7. Importing tokens should update the AssetList with the tokens from the chain they are being imported on.
  8. Token sorting should be respected by both included sort criteria

Screenshots/Recordings

Screen.Recording.2024-11-11.at.4.15.34.PM.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 and others added 30 commits October 17, 2024 08:23
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
…om:MetaMask/metamask-extension into feat/mmassets-432_network-filter-extension
…ask-extension into brian/asset-controller-39
github-merge-queue bot pushed a commit that referenced this pull request Nov 21, 2024
## **Description**

Cherry picks design updates for `AssetListControlBar` introduced from
#28386 separately in
it's own PR to help minimize diff in main feature branch.

Also includes unit test and e2e updates impacted from these changes.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28575?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

Run with feature flag and without feature flag:

`yarn webpack --watch`
`PORTFOLIO_VIEW=1 yarn webpack --watch`

Validate that sort works, validate that import works, validate that
refresh list works.

## **Screenshots/Recordings**

Without feature flag:


https://github.com/user-attachments/assets/445d4fd1-93d1-4cee-bd7b-bcc36518d7ca

With feature flag (network filter not yet integrated)



https://github.com/user-attachments/assets/d1aa8812-9787-49b5-9696-39e56d82ed56

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.

---------

Co-authored-by: Jonathan Bursztyn <[email protected]>
* @param {object} state - Redux state
* @returns {object} An object of tokens with balances for the given account. Data relationship will be chainId => balance
*/
export function getSelectedAccountNativeTokenCachedBalanceByChainId(state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add tests for these selectors.

Comment on lines +85 to +96
useEffect(() => {
const testnetChains: string[] = TEST_CHAINS;
const mainnetChainIds = Object.keys(allNetworks).filter(
(chain) => !testnetChains.includes(chain),
);
setChainsToShow(mainnetChainIds);
}, []);

const allOpts: Record<string, boolean> = {};
Object.keys(allNetworks).forEach((chain) => {
allOpts[chain] = true;
});
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 think that we should refactor this logic into a hook. It's being used in multiple places, and should just use the getChainIdsToPoll selector.

I would prefer to address this cleanup in a separate PR, if possible.

@gambinish gambinish mentioned this pull request Nov 21, 2024
7 tasks
@gambinish
Copy link
Contributor Author

closing in favor of #28593

@gambinish gambinish closed this Nov 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.