feat: integrate NFT api to display names for nfts within simulations for ERC721s
#25692
Conversation
OGPoyraz
added
DO-NOT-MERGE
OGPoyraz
changed the title
ui/hooks/useDisplayName.ts
Outdated
| @@ -15,6 +16,7 @@ export type UseDisplayNameResponse = { | |||
| name: string | null; | |||
| hasPetname: boolean; | |||
| contractDisplayName?: string; | |||
| image?: string | null; | |||
There was a problem hiding this comment.
I am not super happy to use image here since hook name is also useDisplayName. But I think this perfect place to implement this because image is going to be used in the same component where this is used.
Maybe we can think to rename it to useDisplayNameProps
| .map(({ value }) => value), | ||
| chainId, | ||
| }; | ||
| }, [JSON.stringify(requests), chainId]); |
There was a problem hiding this comment.
JSON.stringify creates a new string every time, so this defeats the purpose of the dependency array. Doesn't simply requests work here, or is requests mutable?
There was a problem hiding this comment.
It does create a new string but string is not a reference type so it will actually check the equality before and after with given strings.
If we pass the requests here, we must guarantee that reference is not changing which we cannot do that since requests drops here as a new object evertyime.
In ui/hooks/useDisplayName.ts this is what passed to this hook.
const nameRequests = requests.map(({ value, type }) => ({
value,
type,
}));
In order to make this work with reference, we need in parent to guarantee that reference is not changing.
const nameRequests = useMemo(
() => requests.map(({ value, type }) => ({
value,
type,
}));
);
So this could be another solution. Which one looks better, wdyt?
|
The quota with our NFT provider is not infinite, and may not be enough to send requests on every transaction simulation. Before adding this feature we need to estimate the number of requests, and minimize requests where possible. Like only fetching when we can't identify the asset through other means. Or first detecting it's an NFT and not an ERC20 through on chain means like ERC-165. Or checking if we have it in NFT state first. Or if we know the contract address and token id, we should just call The quota currently allows around 200 req/s across all NFT endpoints. But our rough calculations were already near that limit just with plans to turn on nft detection setting for everyone + support more chains. And that's already maximally lazy, only fetching when you click into an NFT tab. |
Builds ready [96e71b5]
Page Load Metrics (256 ± 246 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25692 +/- ##
===========================================
- Coverage 69.77% 69.75% -0.02%
===========================================
Files 1376 1378 +2
Lines 48403 48523 +120
Branches 13348 13370 +22
===========================================
+ Hits 33773 33846 +73
- Misses 14630 14677 +47 ☔ View full report in Codecov by Sentry. |
ui/components/app/name/name.tsx
Outdated
| standard?: TokenStandard; | ||
|
|
||
| /** Token ID, if applicable. */ | ||
| tokenId?: Hex; |
There was a problem hiding this comment.
Petnames is fundamentally about storing and proposing names for obscure string values, in this case Ethereum addresses.
We don't want to couple other concepts to it, rather we should be able to extract the data we need from the contract address and the chain ID / variation alone.
ui/hooks/useDisplayName.ts
Outdated
| const nameEntry = nameEntries[index]; | ||
| const firstPartyContractName = firstPartyContractNames[index]; | ||
| const singleContractInfo = contractInfo[index]; | ||
| const watchedNftName = watchedNftNames[value.toLowerCase()]?.name; | ||
| const nftCollectionProperties = | ||
| nftCollections[ | ||
| `${value.toLowerCase()}:${hexToDecimal(tokenId as string)}` |
There was a problem hiding this comment.
This ultimately shouldn't be necessary as the API can source whether a contract address isSpam from the address alone, I've noted this in the core PR also.
ui/hooks/useDisplayName.ts
Outdated
| let nftCollectionName; | ||
| let nftCollectionImage; | ||
|
|
||
| if (!nftCollectionProperties?.isSpam) { |
There was a problem hiding this comment.
For safety, should we change this to isSpam === false?
| name, | ||
| image, | ||
| // We don't have a way to determine if a collection is spam from the tokenURI | ||
| isSpam: null, |
There was a problem hiding this comment.
If we can't determine if a contract isSpam, then the data is unusable for the petnames use case.
That's the central mechanism we need to determine if it's safe to display a default petname.
OGPoyraz
force-pushed
the
2507-integrate-nft-api-to-display-names-for-nfts-within-simulations
branch
from
96e71b5
to
cae7441
Compare
## 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>
## 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>
…or-nfts-within-simulations
|
…or-nfts-within-simulations
…or-nfts-within-simulations
Builds ready [d8f2b41]
Page Load Metrics (1550 ± 106 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ui/hooks/useDisplayName.ts
Outdated
| let nftCollectionImage; | ||
|
|
||
| if (nftCollectionProperties?.isSpam === false) { | ||
| nftCollectionName = nftCollectionProperties?.name; |
There was a problem hiding this comment.
Minor, but could we define an isSpam variable, then assign both of these using const and a ternary?
| }, [requests]); | ||
|
|
||
| useEffect(() => { | ||
| const fetchCollections = async () => { |
There was a problem hiding this comment.
Minor, but since this is a large function, we could define it outside of the component and useEffect (but still in this file) for greater readability, and even split it up further if needed.
Then we could also use useAsyncResult to handle cancellation on subsequent renders, and also detect loading state.
| ) { | ||
| const [collectionsMetadata, setCollectionsMetadata] = | ||
| useState<CollectionsData>({}); | ||
| const chainId = useSelector(getCurrentChainId); |
There was a problem hiding this comment.
Minor, we ideally need to move away from global network so this could be a property or at least a fallback.
There was a problem hiding this comment.
Done, added providedChainId parameter to useNftCollectionsMetadata
…or-nfts-within-simulations
…or-nfts-within-simulations
…or-nfts-within-simulations
|
Builds ready [ab8f93f]
Page Load Metrics (1870 ± 128 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
OGPoyraz
deleted the
2507-integrate-nft-api-to-display-names-for-nfts-within-simulations
branch
metamaskbot
added
release-12.6.0
|
Missing release label release-12.5.0 on PR. Adding release label release-12.5.0 on PR and removing other release labels(release-12.6.0), as PR was added to branch 12.5.0 when release was cut. |
Description
This PR integrates NFT api (
/collections) to display name and image of collection for nfts within simulations only forERC721nfts.Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2507
Related to: MetaMask/core#4506
Manual testing steps
ERC721Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist