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: add getNFTContractInfo #4524

Merged
merged 8 commits into from
Jul 25, 2024
Merged

feat: add getNFTContractInfo #4524

merged 8 commits into from
Jul 25, 2024

Conversation

OGPoyraz
Copy link
Member

Explanation

This PR exposes NFT collections api through NFT controller.

References

Related to: https://github.com/MetaMask/MetaMask-planning/issues/2507
Extension PR using this PR's preview build: MetaMask/metamask-extension#25692

Changelog

@metamask/assets-controllers

  • ADDED: Add fetchNftCollectionMetadata to NFTController api

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

@OGPoyraz OGPoyraz requested a review from a team as a code owner July 15, 2024 11:32
@OGPoyraz OGPoyraz mentioned this pull request Jul 15, 2024
3 tasks
@OGPoyraz
Copy link
Member Author

@metamaskbot publish-preview

Copy link
Contributor

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/accounts-controller": "17.2.0-preview-e4ec85f",
  "@metamask-previews/address-book-controller": "5.0.0-preview-e4ec85f",
  "@metamask-previews/announcement-controller": "7.0.0-preview-e4ec85f",
  "@metamask-previews/approval-controller": "7.0.1-preview-e4ec85f",
  "@metamask-previews/assets-controllers": "35.0.0-preview-e4ec85f",
  "@metamask-previews/base-controller": "6.0.1-preview-e4ec85f",
  "@metamask-previews/build-utils": "3.0.0-preview-e4ec85f",
  "@metamask-previews/chain-controller": "0.1.0-preview-e4ec85f",
  "@metamask-previews/composable-controller": "7.0.0-preview-e4ec85f",
  "@metamask-previews/controller-utils": "11.0.1-preview-e4ec85f",
  "@metamask-previews/ens-controller": "13.0.0-preview-e4ec85f",
  "@metamask-previews/eth-json-rpc-provider": "4.1.1-preview-e4ec85f",
  "@metamask-previews/gas-fee-controller": "19.0.0-preview-e4ec85f",
  "@metamask-previews/json-rpc-engine": "9.0.1-preview-e4ec85f",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.1-preview-e4ec85f",
  "@metamask-previews/keyring-controller": "17.1.0-preview-e4ec85f",
  "@metamask-previews/logging-controller": "5.0.0-preview-e4ec85f",
  "@metamask-previews/message-manager": "10.0.0-preview-e4ec85f",
  "@metamask-previews/name-controller": "8.0.0-preview-e4ec85f",
  "@metamask-previews/network-controller": "20.0.0-preview-e4ec85f",
  "@metamask-previews/notification-controller": "6.0.0-preview-e4ec85f",
  "@metamask-previews/notification-services-controller": "0.1.1-preview-e4ec85f",
  "@metamask-previews/permission-controller": "10.0.1-preview-e4ec85f",
  "@metamask-previews/permission-log-controller": "3.0.0-preview-e4ec85f",
  "@metamask-previews/phishing-controller": "10.0.0-preview-e4ec85f",
  "@metamask-previews/polling-controller": "9.0.0-preview-e4ec85f",
  "@metamask-previews/preferences-controller": "13.0.0-preview-e4ec85f",
  "@metamask-previews/profile-sync-controller": "0.1.2-preview-e4ec85f",
  "@metamask-previews/queued-request-controller": "3.0.0-preview-e4ec85f",
  "@metamask-previews/rate-limit-controller": "6.0.0-preview-e4ec85f",
  "@metamask-previews/selected-network-controller": "16.0.0-preview-e4ec85f",
  "@metamask-previews/signature-controller": "18.0.0-preview-e4ec85f",
  "@metamask-previews/transaction-controller": "35.0.0-preview-e4ec85f",
  "@metamask-previews/user-operation-controller": "14.0.0-preview-e4ec85f"
}

@OGPoyraz OGPoyraz changed the title feat: add getNFTTokenInfo feat: add getNFTContractInfo Jul 15, 2024
@OGPoyraz
Copy link
Member Author

@metamaskbot publish-preview

Copy link
Contributor

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.
{
  "@metamask-previews/accounts-controller": "17.2.0-preview-15099438",
  "@metamask-previews/address-book-controller": "5.0.0-preview-15099438",
  "@metamask-previews/announcement-controller": "7.0.0-preview-15099438",
  "@metamask-previews/approval-controller": "7.0.1-preview-15099438",
  "@metamask-previews/assets-controllers": "35.0.0-preview-15099438",
  "@metamask-previews/base-controller": "6.0.1-preview-15099438",
  "@metamask-previews/build-utils": "3.0.0-preview-15099438",
  "@metamask-previews/chain-controller": "0.1.0-preview-15099438",
  "@metamask-previews/composable-controller": "7.0.0-preview-15099438",
  "@metamask-previews/controller-utils": "11.0.1-preview-15099438",
  "@metamask-previews/ens-controller": "13.0.0-preview-15099438",
  "@metamask-previews/eth-json-rpc-provider": "4.1.1-preview-15099438",
  "@metamask-previews/gas-fee-controller": "19.0.0-preview-15099438",
  "@metamask-previews/json-rpc-engine": "9.0.1-preview-15099438",
  "@metamask-previews/json-rpc-middleware-stream": "8.0.1-preview-15099438",
  "@metamask-previews/keyring-controller": "17.1.0-preview-15099438",
  "@metamask-previews/logging-controller": "5.0.0-preview-15099438",
  "@metamask-previews/message-manager": "10.0.0-preview-15099438",
  "@metamask-previews/name-controller": "8.0.0-preview-15099438",
  "@metamask-previews/network-controller": "20.0.0-preview-15099438",
  "@metamask-previews/notification-controller": "6.0.0-preview-15099438",
  "@metamask-previews/notification-services-controller": "0.1.1-preview-15099438",
  "@metamask-previews/permission-controller": "10.0.1-preview-15099438",
  "@metamask-previews/permission-log-controller": "3.0.0-preview-15099438",
  "@metamask-previews/phishing-controller": "10.0.0-preview-15099438",
  "@metamask-previews/polling-controller": "9.0.0-preview-15099438",
  "@metamask-previews/preferences-controller": "13.0.0-preview-15099438",
  "@metamask-previews/profile-sync-controller": "0.1.3-preview-15099438",
  "@metamask-previews/queued-request-controller": "3.0.0-preview-15099438",
  "@metamask-previews/rate-limit-controller": "6.0.0-preview-15099438",
  "@metamask-previews/selected-network-controller": "16.0.0-preview-15099438",
  "@metamask-previews/signature-controller": "18.0.0-preview-15099438",
  "@metamask-previews/transaction-controller": "35.0.0-preview-15099438",
  "@metamask-previews/user-operation-controller": "14.0.0-preview-15099438"
}

matthewwalsh0
matthewwalsh0 previously approved these changes Jul 17, 2024
@@ -104,6 +104,22 @@ const GOERLI = {
ticker: NetworksTicker.goerli,
};

const CRYPTOPUNK_COLLECTION_MOCK = {
Copy link
Member

Choose a reason for hiding this comment

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

Minor, should we name these for their conceptual usage and not literal values, such as COLLECTION_MOCK_1 etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -520,6 +520,12 @@ export class NftController extends BaseController<
});
}

#getNftCollectionApi(): string {
// False negative.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if this line is required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately yes, a thread here #4506 (comment)

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.

Hey! Thanks for this PR. I just had one comment about the test you wrote, but this otherwise looks good.

packages/assets-controllers/src/NftController.test.ts Outdated Show resolved Hide resolved
@OGPoyraz OGPoyraz merged commit a3bfbce into main Jul 25, 2024
116 checks passed
@OGPoyraz OGPoyraz deleted the feat/add-getnftcontractinfo branch July 25, 2024 09:40
AugmentedMode pushed a commit that referenced this pull request Jul 30, 2024
## Explanation

<!--
Thanks for your contribution! Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?
* Are there any changes whose purpose might not obvious to those
unfamiliar with the domain?
* If your primary goal was to update one package but you found you had
to update another one along the way, why did you do so?
* If you had to upgrade a dependency, why did you do so?
-->

This PR exposes NFT `collections` api through NFT controller.

## References

<!--
Are there any issues that this pull request is tied to? Are there other
links that reviewers should consult to understand these changes better?

For example:

* Fixes #12345
* Related to #67890
-->

Related to: MetaMask/MetaMask-planning#2507
Extension PR using this PR's preview build:
MetaMask/metamask-extension#25692

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/assets-controllers`

- **ADDED**: Add `fetchNftCollectionMetadata` to `NFTController` api

## Checklist

- [X] I've updated the test suite for new or updated code as appropriate
- [X] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [X] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants