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

feat: migrate NftController to BaseControllerV2 #4310

Merged
merged 26 commits into from
May 30, 2024

Conversation

cryptodev-2s
Copy link
Contributor

@cryptodev-2s cryptodev-2s commented May 23, 2024

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 and NftControllerState).

  • 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 to getDefaultNftControllerState and NftState has been renamed to NftControllerState.

References

Changelog

@metamask/assets-controllers

  • BREAKING: Convert NftController toBaseControllerV2
    • The constructor parameters have changed, rather than accepting a "config" parameter, it takes selectedAddress, ipfsGateway, openSeaEnabled, useIPFSSubdomains and isIpfsGatewayEnabled as parameters.
  • CHANGED: type conversion, changing from an interface to a type alias for Nft, NftContract, NftMetadata and NftControllerState.
  • CHANGED: getDefaultNftState has been renamed to getDefaultNftControllerState and NftState has been renamed to NftControllerState.
  • ADDED: Exports new added types: NftControllerActions, NftControllerGetStateAction, NftControllerEvents, NftControllerStateChangeEvent

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

@desi desi requested a review from a team May 23, 2024 20:18
@cryptodev-2s cryptodev-2s marked this pull request as draft May 27, 2024 14:48
@cryptodev-2s cryptodev-2s marked this pull request as ready for review May 28, 2024 21:15
Copy link
Contributor

@MajorLift MajorLift left a 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.

packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
packages/assets-controllers/src/NftController.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@MajorLift MajorLift left a 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.

Comment on lines 221 to 225
type AllowedActions =
| AddApprovalRequest
| NetworkControllerGetNetworkClientByIdAction;

export type AllowedEvents =
type AllowedEvents =
Copy link
Contributor

@MajorLift MajorLift May 29, 2024

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';

Copy link
Contributor Author

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 ?

Copy link
Contributor

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.

Copy link
Contributor

@MajorLift MajorLift May 30, 2024

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.

Copy link
Contributor

@MajorLift MajorLift May 30, 2024

Choose a reason for hiding this comment

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

@mcmire RE: #4342 (comment)

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)).

Comment on lines +1476 to +1480
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' });
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Would it make sense to convert both the url and the query string into template strings and extract constants? This and this seem a bit more readable and reusable.

Copy link
Contributor

@MajorLift MajorLift left a 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.

@cryptodev-2s cryptodev-2s merged commit 50efa9c into main May 30, 2024
147 checks passed
@cryptodev-2s cryptodev-2s deleted the feature/nft-controller-to-base-controller-v2 branch May 30, 2024 20:10
@mcmire mcmire mentioned this pull request May 30, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Convert NftController from BaseControllerV1 to BaseControllerV2
3 participants