feat: add getNFTTokenInfo
#4506
Conversation
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| `${this.#getNftCollectionApi()}?${urlParams.toString()}`, | ||
| { | ||
| headers: { | ||
| Version: '1', |
There was a problem hiding this comment.
Nit; can use NFT_API_VERSION for the version
|
|
||
| for (const address of contractAddress) { | ||
| urlParams.append('contract', address); | ||
| } |
There was a problem hiding this comment.
Want to mention that this API will have a limit of 20 for the contract address param in case this is relevant to your usecase
There was a problem hiding this comment.
Thanks for calling this out, this will be very rare case to have more than 20 nfts in one transaction but good to know.
I will check later the metrics of simulations if we ever encountered this much.
|
Note: When you are passing a simple contract address to this param |
| * @param chainId - The chain ID of the network where the NFT is located. | ||
| * @returns NFT collections metadata. | ||
| */ | ||
| async fetchNftCollectionsMetadata( |
There was a problem hiding this comment.
I think this should short circuit if a chain id is provided that the NFT API doesn't support, to save a request + time.
There was a problem hiding this comment.
Good call @bergeron
I couldn't see any endpoint / constants that I can use it. Can you elaborate how do I derive that information here?
| const urlParams = new URLSearchParams({ | ||
| chainId, | ||
| }); | ||
|
|
||
| for (const address of contractAddress) { | ||
| urlParams.append('contract', address); | ||
| } | ||
|
|
||
| return await handleFetch( | ||
| `${this.#getNftCollectionApi()}?${urlParams.toString()}`, |
There was a problem hiding this comment.
handleFetch was changed some time ago to take a URL object, so instead of constructing the URL like this, what do you think about:
| const urlParams = new URLSearchParams({ | |
| chainId, | |
| }); | |
| for (const address of contractAddress) { | |
| urlParams.append('contract', address); | |
| } | |
| return await handleFetch( | |
| `${this.#getNftCollectionApi()}?${urlParams.toString()}`, | |
| const url = new URL(this.#getNftCollectionApi()); | |
| url.searchParams.append('chainId', chainId); | |
| for (const address of contractAddress) { | |
| url.searchParams.append('contract', address); | |
| } | |
| return await handleFetch( | |
| url, |
| @@ -4378,4 +4394,25 @@ describe('NftController', () => { | |||
|
|
|||
| expect(updateNftMetadataSpy).not.toHaveBeenCalled(); | |||
| }); | |||
|
|
|||
| it('fetchNftCollectionsMetadata fetches NFT collections metadata successfully', async () => { | |||
There was a problem hiding this comment.
What are your thoughts on wrapping this test in a describe block named after the method you want to test? For instance:
describe('fetchNftCollectionsMetadata', () => {
it('fetches NFT collections metadata successfully', async () => {
// ...
});
});I realize that this is not the style for other tests in this file, but it's a principle that we've been trying to move to as a whole. More about this here: https://github.com/MetaMask/contributor-docs/blob/main/docs/unit-testing.md#use-describe-to-group-tests-for-the-same-functionmethod
There was a problem hiding this comment.
I like the way of putting multiple describe blocks as well, the only reason that I didn't here was that this was the only test we run for the method.
But if we are happy to have one test inside describe block, here it is 24ac62f
OGPoyraz
dismissed stale reviews from salimtb, sahar-fehri, and pedronfigueiredo
via
1a8b435
…data' of github.com:MetaMask/core into feat/add-fetchNftMetadata
…into feat/add-fetchNftMetadata
OGPoyraz
changed the title
fetchNftCollectionMetadatagetNFTTokenInfo
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
OGPoyraz
force-pushed
the
feat/add-fetchNftMetadata
branch
from
a6b3135
to
3ee9d02
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| @@ -468,7 +470,7 @@ export class NftController extends BaseController< | |||
| } | |||
|
|
|||
| getNftApi() { | |||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | |||
| // False negative. | |||
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions | |||
| return `${NFT_API_BASE_URL}/tokens`; | |||
There was a problem hiding this comment.
would ${NFT_API_BASE_URL as string}/tokens fix the warning?
There was a problem hiding this comment.
Checking this now, funny part is that it doesn't fail locally, only in CI 😅
There was a problem hiding this comment.
I strongly think we should opt for the eslint-disable directive instead of a type assertion here, since the cause of this lint error is abnormal behavior from the lint rule, not the code itself.
The type assertion would silence other potential errors with NFT_API_BASE_URL (e.g. if it were to be typed as implicit any due to an upstream import issue, the as string would suppress useful alarms).
There was a problem hiding this comment.
I agree that assertion here will silence the potential errors but before that we most probably would have some failed unit tests at first place if such change happens. Since this is not a complex cast I don't think this makes much sense.
I am reverting this change and leave this as is.
There was a problem hiding this comment.
@MajorLift do you have any idea why this is not failing locally but on CI?
There was a problem hiding this comment.
Yes, I experienced that as well. yarn lint:eslint results were different on local as well so it wasn't just an issue with my IDE's eslint server.
I do remember that it turned out to be an issue with my local configuration, but I can't recall what solved it just now. It definitely wasn't outside of the range of usual-suspect fixes (removing eslint cache, reinstalling packages etc.). I'll leave a comment here if I can remember what it was!
| chainIds: Hex[], | ||
| tokens: { | ||
| contractAddress: string; | ||
| tokenId: string; |
There was a problem hiding this comment.
For the confirmations use case, why do we need the token ID, since our primary goal is to extract whether a specific contract address has isSpam set?
In the case of petnames, we don't have a token ID, only an address, so I assumed this API would be retrieving contract info, rather than individual token info?
|
To clear confusion, I've addressed all suggestions here and created a clean PR here: #4524 |
…for `ERC721`s (#25692) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR integrates NFT api (`/collections`) to display name and image of collection for nfts within simulations only for `ERC721` nfts. [](https://codespaces.new/MetaMask/metamask-extension/pull/25692?quickstart=1) ## **Related issues** Fixes: MetaMask/MetaMask-planning#2507 Related to: MetaMask/core#4506 ## **Manual testing steps** 1. Go to Opensea, try buying https://opensea.io/assets/ethereum/0xd4307e0acd12cf46fd6cf93bc264f5d5d1598792/41757 (you don't need to buy) - this is `ERC721` 2. See collection name and image in simulations ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before**  ### **After**  ## **Pre-merge author checklist** - [X] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
](https://codespaces.new/MetaMask/metamask-extension/pull/25692?quickstart=1){kind=link}
Explanation
This PR exposes NFT
collectionsapi 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-controllersfetchNftCollectionMetadatatoNFTControllerapiChecklist