-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
base: main
Are you sure you want to change the base?
Conversation
async #onPreferencesControllerStateChange({ | ||
ipfsGateway, | ||
openSeaEnabled, | ||
displayNftMedia, |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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`
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
openSeaEnabled
todisplayNftMedia
in NftControllersetApiKey
function from NftController since we do not use opensea anymore for NFT dataopenSeaApiKey
from NftController@metamask/preferences-controller
openSeaEnabled
todisplayNftMedia
setOpenSeaEnabled
tosetDisplayNftMedia
useSafeChainsListValidation
to preferences controller statesetUseSafeChainsListValidation
function to updateuseSafeChainsListValidation
value .Checklist