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.

Sign up for GitHub

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

Jump to bottom

fix: should not reset market data after switch network #4832

Merged
merged 2 commits into from
Oct 23, 2024
Merged

fix: should not reset market data after switch network #4832

merged 2 commits into from
Oct 23, 2024

Conversation

salimtb
Copy link
Contributor

@salimtb salimtb commented Oct 22, 2024 •
edited
Loading

Explanation

This Pull Request addresses an issue where market data would unnecessarily reset following a network switch. The purpose of this update is to ensure that market data remains intact and continues to function as expected after a network change, preventing potential data loss or disruptions in user experience

References

Changelog

@metamask/assets-controller

  • CHANGED: Prevent market data reset after a network switch.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

bergeron
bergeron reviewed Oct 22, 2024
View reviewed changes
packages/assets-controllers/src/TokenRatesController.ts
@@ -346,9 +346,6 @@ export class TokenRatesController extends StaticIntervalPollingController<TokenR
);

if (this.#chainId !== chainId || this.#ticker !== ticker) {
this.update((state) => {
state.marketData = {};
});
this.#chainId = chainId;
Copy link
Contributor

@bergeron bergeron Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When we move to the new polling API (startPolling from the base class), this entire subscription and concept of a 'current chain id' will go away. The caller will become responsible for passing in the chain id(s) to poll, rather than the controller listening to the current one.

gambinish
gambinish reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@gambinish gambinish left a comment

Choose a reason for hiding this comment

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

Should we also write a test: should append marketData for each chain? Or, is that not something included in this PR?

@salimtb salimtb marked this pull request as ready for review October 22, 2024 15:52
@salimtb salimtb requested a review from a team as a code owner October 22, 2024 15:52
@salimtb
Copy link
Contributor Author

salimtb commented Oct 22, 2024

Should we also write a test: should append marketData for each chain? Or, is that not something included in this PR?

good idea , i'll add this test

@gambinish gambinish mentioned this pull request Oct 22, 2024
Merged
7 tasks
@salimtb salimtb merged commit 401ba30 into main Oct 23, 2024
119 checks passed
@salimtb salimtb deleted the salim/not-reset-market-data-after-network-switch branch October 23, 2024 12:21
@gambinish gambinish mentioned this pull request Oct 25, 2024
Closed
7 tasks
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Oct 30, 2024
@gambinish @salimtb
feat: Token Network Filter UI [Extension] (#27884)
acd1a02 
## **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. #27785
2. MetaMask/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](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27884?quickstart=1)

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
MetaMask/core#4832 and currently hardcoded to
$1000
2. Token lists will not be filtered in this PR. Blocked by
#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**


https://github.com/user-attachments/assets/4b132e47-0dcf-4e9c-8755-ccb2be1d5dc1

## **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: Salim TOUBAL <[email protected]>
github-merge-queue bot pushed a commit to MetaMask/metamask-extension that referenced this pull request Oct 30, 2024
@gambinish @salimtb
feat: Token Network Filter UI [Extension] (#27884)
c04c119 
## **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. #27785
2. MetaMask/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](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27884?quickstart=1)

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
MetaMask/core#4832 and currently hardcoded to
$1000
2. Token lists will not be filtered in this PR. Blocked by
#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**


https://github.com/user-attachments/assets/4b132e47-0dcf-4e9c-8755-ccb2be1d5dc1

## **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: Salim TOUBAL <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gambinish gambinish gambinish left review comments

@bergeron bergeron bergeron left review comments

@cryptodev-2s cryptodev-2s cryptodev-2s approved these changes

Assignees
No one assigned
Labels
team-assets
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@salimtb @bergeron @gambinish @cryptodev-2s