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

DetectedTokens getting reloaded from allDetectedTokens even after the tokens are imported for a chain and selectedAddress #1015

Merged
merged 6 commits into from
Dec 16, 2022

Conversation

NiranjanaBinoy
Copy link
Contributor

@NiranjanaBinoy NiranjanaBinoy commented Dec 12, 2022

Fixes 16854
When used to import the detectedTokens by calling addTokens we should clear the detectedTokens and allDetectedTokens[chainId][selectedAddress] ie entry in allDetectedTokens for that particular chainId and selectedAddress, otherwise the data from allDetectedTokens getting repopulated/reloaded into the detectedtokens when the networkState or preferenceState changes.

Edit: upon further inspection, we had to do the same clearing for allTokens and allIgnoredtokens as well.

…etNewAllTokensState while updating newAllDetectedTokens
@NiranjanaBinoy NiranjanaBinoy requested a review from a team as a code owner December 12, 2022 18:37
@NiranjanaBinoy NiranjanaBinoy self-assigned this Dec 12, 2022
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Is it possible to add tests for this?

jpuri
jpuri previously approved these changes Dec 13, 2022
@jpuri
Copy link
Contributor

jpuri commented Dec 13, 2022

Code changes look good but +1 to @mcmire point about tests.

…g sure empty lists are not added just because used switched to an account or network
mcmire
mcmire previously approved these changes Dec 15, 2022
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Looks good!

adonesky1
adonesky1 previously approved these changes Dec 15, 2022
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

lgtm!

@NiranjanaBinoy NiranjanaBinoy merged commit b442601 into main Dec 16, 2022
@NiranjanaBinoy NiranjanaBinoy deleted the fix-recurring-detected-token-entry branch December 16, 2022 14:37
@Gudahtt Gudahtt mentioned this pull request Dec 20, 2022
Gudahtt added a commit that referenced this pull request Dec 20, 2022
Changelogs:

# `announcement-controller`
## [2.0.0]
### Changed
- **BREAKING:** Migrate to BaseControllerV2
([#959](#959))
- The announcement controller now extends `BaseControllerV2` rather than
`BaseController`, which includes the following changes:
- The constructor now accepts a single "args" object rather than
positional parameters.
- A restricted controller messenger instance must be passed into the
constructor.
- The controller configuration has been replaced by an
`allAnnouncements` constructor parameter.
- The following properties previously inherited from `BaseController`
are no longer present:
      - `defaultConfig`
      - `defaultState`
      - `disabled`
      - `config`
      - `state`
- The following methods previously inherited from `BaseController` are
no longer present:
      - `configure`
      - `notify`
      - `subscribe`
      - `unsubscribe`
      - `update`
    - The `name` property is now readonly.

# `assets-controllers`
## [3.0.1]
### Changed
- Export `isTokenDetectionSupportedForNetwork` function
([#1034](#1034))
- Update `@metamask/contract-metadata` from 1.35.0 to 2.1.0
([#1013](#1013))

### Fixed
- Fix token controller state updates
([#1015](#1015))
- Attempts to empty the list of "added", "ignored", or "detected" tokens
were not saved in state correctly, resulting in that operation being
undone after switching account or network.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
… tokens are imported for a chain and selectedAddress (#1015)
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
Changelogs:

# `announcement-controller`
## [2.0.0]
### Changed
- **BREAKING:** Migrate to BaseControllerV2
([#959](#959))
- The announcement controller now extends `BaseControllerV2` rather than
`BaseController`, which includes the following changes:
- The constructor now accepts a single "args" object rather than
positional parameters.
- A restricted controller messenger instance must be passed into the
constructor.
- The controller configuration has been replaced by an
`allAnnouncements` constructor parameter.
- The following properties previously inherited from `BaseController`
are no longer present:
      - `defaultConfig`
      - `defaultState`
      - `disabled`
      - `config`
      - `state`
- The following methods previously inherited from `BaseController` are
no longer present:
      - `configure`
      - `notify`
      - `subscribe`
      - `unsubscribe`
      - `update`
    - The `name` property is now readonly.

# `assets-controllers`
## [3.0.1]
### Changed
- Export `isTokenDetectionSupportedForNetwork` function
([#1034](#1034))
- Update `@metamask/contract-metadata` from 1.35.0 to 2.1.0
([#1013](#1013))

### Fixed
- Fix token controller state updates
([#1015](#1015))
- Attempts to empty the list of "added", "ignored", or "detected" tokens
were not saved in state correctly, resulting in that operation being
undone after switching account or network.
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
… tokens are imported for a chain and selectedAddress (#1015)
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
Changelogs:

# `announcement-controller`
## [2.0.0]
### Changed
- **BREAKING:** Migrate to BaseControllerV2
([#959](#959))
- The announcement controller now extends `BaseControllerV2` rather than
`BaseController`, which includes the following changes:
- The constructor now accepts a single "args" object rather than
positional parameters.
- A restricted controller messenger instance must be passed into the
constructor.
- The controller configuration has been replaced by an
`allAnnouncements` constructor parameter.
- The following properties previously inherited from `BaseController`
are no longer present:
      - `defaultConfig`
      - `defaultState`
      - `disabled`
      - `config`
      - `state`
- The following methods previously inherited from `BaseController` are
no longer present:
      - `configure`
      - `notify`
      - `subscribe`
      - `unsubscribe`
      - `update`
    - The `name` property is now readonly.

# `assets-controllers`
## [3.0.1]
### Changed
- Export `isTokenDetectionSupportedForNetwork` function
([#1034](#1034))
- Update `@metamask/contract-metadata` from 1.35.0 to 2.1.0
([#1013](#1013))

### Fixed
- Fix token controller state updates
([#1015](#1015))
- Attempts to empty the list of "added", "ignored", or "detected" tokens
were not saved in state correctly, resulting in that operation being
undone after switching account or network.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: DetectedTokens getting reloaded after it is imported while switching accounts
5 participants