-
-
Notifications
You must be signed in to change notification settings - Fork 184
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: migrate NftController to BaseControllerV2 #4310
feat: migrate NftController to BaseControllerV2 #4310
Conversation
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 had some questions but looks good overall! Sorry about the confusing comment order. Tried starting a review inside of vscode, and it isn't working the way I expected.
I'll take a closer look at the test files tomorrow.
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 had some questions but looks good overall! Sorry about the confusing comment order. Tried starting a review inside of vscode, and it isn't working the way I expected.
I'll take a closer look at the test files tomorrow.
type AllowedActions = | ||
| AddApprovalRequest | ||
| NetworkControllerGetNetworkClientByIdAction; | ||
|
||
export type AllowedEvents = | ||
type AllowedEvents = |
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.
We'll still want to export these so they can be used inside of the package. To exclude these from package-level exports, we need to update the index.ts
file so that we're explicitly enumerating all exports from NftController.ts
that we want to expose externally.
Specifically, line 4:
- export * from './NftController';
+ export {
+ NftControllerMessenger,
+ NftControllerActions,
+ NftControllerGetStateAction,
...
+ } from './NftController';
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 am not sure what's being used what's not we have a lot of exports within the controller we can clean this export later ?
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.
Everything that previously had an export
keyword attached would have been package-level exports and should be added to the list.
The new exports that were added (NftControllerActions
, NftControllerGetStateAction
, NftControllerEvents
, NftControllerStateChangeEvent
) or renamed (getDefaultNftControllerState
) in this PR and are intended as package-level exports need to be listed in the changelog as well.
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.
Looks good now. Could we add a "ADDED" bullet point in the changelog section of the PR description for the new type exports? We want to log type-only changes for our packages.
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.
The following needs to be added to the changelog for this PR:
### Added
- Adds and exports new types: `NftControllerActions`, `NftControllerGetStateAction`, `NftControllerEvents`, `NftControllerStateChangeEvent` ([#4310](https://github.com/MetaMask/core/pull/4310)).
…r-to-base-controller-v2
…r-to-base-controller-v2
nock('https://nft.api.cx.metamask.io') | ||
.get( | ||
'/tokens?chainIds=1&tokens=0x2aEa4Add166EBf38b63d09a75dE1a7b94Aa24163%3A1203&includeTopBid=true&includeAttributes=true&includeLastSale=true', | ||
) | ||
.reply(404, { error: 'Not found' }); |
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.
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! Great job getting this done quickly even though there were a lot of details to cover.
Explanation
The NftController has been migrated to BaseControllerV2. As part of this migration the following changes were made:
The deprecated config property has been removed and has been replaced with flatten properties on the controller constructor options (
selectedAddress
,ipfsGateway
,openSeaEnabled
,useIPFSSubdomains
,isIpfsGatewayEnabled
andNftControllerState
).This PR includes a type conversion, changing from an interface to a type alias in TypeScript to improve type management and flexibility for
Nft
,NftContract
,NftMetadata
.getDefaultNftState
has been renamed togetDefaultNftControllerState
andNftState
has been renamed toNftControllerState
.References
Changelog
@metamask/assets-controllers
BaseControllerV2
selectedAddress
,ipfsGateway
,openSeaEnabled
,useIPFSSubdomains
andisIpfsGatewayEnabled
as parameters.Nft
,NftContract
,NftMetadata
andNftControllerState
.getDefaultNftState
has been renamed togetDefaultNftControllerState
andNftState
has been renamed toNftControllerState
.NftControllerActions
,NftControllerGetStateAction
,NftControllerEvents
,NftControllerStateChangeEvent
Checklist