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

fix: add missing settings from mobile patch #4774

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Oct 9, 2024

Explanation

Looking at the preferences-controller patch on mobile; noticed that core is missing some of those updates.
This PR brings the updates needed on mobile to core.

References

Changelog

@metamask/assets-controllers

  • BREAKING: Rename openSeaEnabled to displayNftMedia in NftController
  • BREAKING: Remove setApiKey function from NftController since we do not use opensea anymore for NFT data
  • BREAKING: Remove openSeaApiKey from NftController

@metamask/preferences-controller

  • BREAKING: Rename openSeaEnabled to displayNftMedia
  • BREAKING: Rename setOpenSeaEnabled to setDisplayNftMedia
  • ADDED: Added useSafeChainsListValidation to preferences controller state
  • ADDED: Added setUseSafeChainsListValidation function to update useSafeChainsListValidation value .

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

@sahar-fehri sahar-fehri marked this pull request as ready for review October 9, 2024 14:35
@sahar-fehri sahar-fehri requested review from a team as code owners October 9, 2024 14:35
Comment on lines 410 to +412
async #onPreferencesControllerStateChange({
ipfsGateway,
openSeaEnabled,
displayNftMedia,
Copy link
Contributor

@bergeron bergeron Oct 9, 2024

Choose a reason for hiding this comment

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

I'm wondering whether this is going to work on both mobile and extension. I think the core preferences controller is only used by mobile. And extension has its own version. But then how will extension work since it still uses openSeaEnabled?

Copy link
Contributor Author

@sahar-fehri sahar-fehri Oct 9, 2024

Choose a reason for hiding this comment

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

Hmmmm, Was thinking to rename the variable on extension also since technically we no longer use opensea (might require us to do migrations to update user state) ;

On extension; when a user triggers and update of the openseaEnabled toggle for exp; controllerMessenger will publish the event "PreferencesController:stateChange" here which controllers like NftController subscribe to and update their state accordingly;

When controllerMessenger on extension publishes the event its also sending the currentState which is an object containing "openseaEnabled"

this.controllerMessenger.publish(
      'PreferencesController:stateChange',
      currentState,
      [],
);

Then controllers which subscribe to PreferencesController:stateChange would read the value

async #onPreferencesControllerStateChange({
    ipfsGateway,
    displayNftMedia,
    isIpfsGatewayEnabled,
  }: PreferencesState) {
.....
  }

A quicker way would be to do this instead in core:

async #onPreferencesControllerStateChange({
    ipfsGateway,
    openseaEnabled: displayNftMedia,
    isIpfsGatewayEnabled,
  }: PreferencesState) {
.....
  }
or `this.#displayNftMedia = openseaEnabled`

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @bergeron . I see some intersections in the extension with the Core PreferencesController. For example, TokenDetectionController is used in extension, in TokenDetectionController we can see the Core's PreferencesController used. If there's PreferencesController:stateChange event emitted in extension, all the subscribers will get the state changed from extension. As runtime only knows PreferencesController:stateChange, but not whether it's core or extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think we'll want to migrate extension from openSeaEnabled to displayNftMedia so that it's aligned with mobile. Doesn't have to block this PR. Just would have to do it before extension can consume this.

Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants