feat: Integrate NFT api to display image & names in simulations includes `erc721`s #12160

OGPoyraz · 2024-11-04T13:39:26Z

Description

This PR integrates NFT api to display name and image of collection for nfts within simulations include ERC721 nfts.

Related issues

Fixes:

Manual testing steps

Go to Opensea, try to buy https://opensea.io/assets/ethereum/0xd4307e0acd12cf46fd6cf93bc264f5d5d1598792/41757 (you don't need to buy) - this is ERC721
See collection name and image in simulations

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

…e ERC721s

github-actions · 2024-11-04T13:43:44Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 3493505
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1efd9dcb-27e5-468e-b0b6-21287f1c30d2

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

sleepytanya · 2024-11-04T19:41:13Z

Bitrise buildshttps://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8d445077-9d44-4c22-b759-7dba6b39f8ea?tab=artifacts

NFT name and image within simulations for ERC721 NFTs are successfully displayed on Android and iOS devices.

iOS:

Android:

…nfts-within-simulations-on-mobile

github-actions · 2024-11-05T10:25:51Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 26e42ea
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a2be44df-43fb-4bec-a398-b6fa5ad5c34d

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

codecov-commenter · 2024-11-05T10:35:18Z

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 55.79%. Comparing base (080a77b) to head (26e42ea).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...nts/hooks/DisplayName/useNftCollectionsMetadata.ts	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12160      +/-   ##
==========================================
+ Coverage   55.76%   55.79%   +0.03%     
==========================================
  Files        1785     1786       +1     
  Lines       40325    40352      +27     
  Branches     5045     5051       +6     
==========================================
+ Hits        22486    22514      +28     
+ Misses      16292    16290       -2     
- Partials     1547     1548       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…nfts-within-simulations-on-mobile

github-actions · 2024-11-08T11:06:17Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 6e5ce92
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d71b3538-2f09-4f1e-8bc4-5cd6051956c8

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

app/components/hooks/DisplayName/useDisplayName.ts

app/components/UI/Name/Name.tsx

app/components/hooks/DisplayName/useNftCollectionsMetadata.ts

…nfts-within-simulations-on-mobile

github-actions · 2024-11-19T10:12:40Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: e742c68
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/6b9e848f-c482-47ba-bf48-bc3c4b32cc88

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

github-actions · 2024-11-19T12:14:26Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 19c9a7a
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1e6db795-df5b-4f10-9a04-a5e562f84768

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

sonarcloud · 2024-11-19T12:34:37Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

matthewwalsh0 · 2024-11-19T17:28:51Z

app/components/hooks/DisplayName/useNftCollectionsMetadata.ts

+}
+
+export function useNftCollectionsMetadata(requests: UseDisplayNameRequest[]) {
+  const chainId = requests?.[0]?.variation;


We shouldn't really assume they all have the same chain ID since they are distinct requests.

I refactored the same hook in the extension to support alternate chains in the requests, can we copy and paste here?

matthewwalsh0 · 2024-11-19T17:29:43Z

app/components/hooks/DisplayName/useNftName.ts

+  const nftCollections = useNftCollectionsMetadata(requests);
+
+  return requests.map(({ value }) => {
+    const nftCollectionProperties = nftCollections[value.toLowerCase()];


Can we add the standard type check to return undefined if it's not an Ethereum address?

matthewwalsh0 · 2024-11-19T17:31:06Z

app/components/hooks/DisplayName/useNftName.ts

+  return requests.map(({ value }) => {
+    const nftCollectionProperties = nftCollections[value.toLowerCase()];
+
+    const isNotSpam = nftCollectionProperties?.isSpam === false;


Minor, could we also just early return if it is spam?

matthewwalsh0 · 2024-11-19T17:31:18Z

app/components/hooks/DisplayName/useNftName.ts

+import { UseDisplayNameRequest } from './useDisplayName';
+import { useNftCollectionsMetadata } from './useNftCollectionsMetadata';
+
+export interface UseNFTNameResponse {


Minor, type rather than interface?

matthewwalsh0 · 2024-11-19T17:31:31Z

app/components/hooks/DisplayName/useNftName.ts

+import { useNftCollectionsMetadata } from './useNftCollectionsMetadata';
+
+export interface UseNFTNameResponse {
+  nftCollectionName: string | undefined;


Minor, in this context is name and image sufficient as the rest is implied?

Integrate NFT api to display names for nfts within simulations includ…

3493505

…e ERC721s

OGPoyraz requested a review from a team as a code owner November 4, 2024 13:39

github-actions bot added the team-confirmations Push issues to confirmations team label Nov 4, 2024

OGPoyraz changed the title ~~Integrate NFT api to display names for nfts within simulations includ…~~ feat: Integrate NFT api to display image & names in simulations includes erc721s Nov 4, 2024

OGPoyraz added the Run Smoke E2E Triggers smoke e2e on Bitrise label Nov 4, 2024

Fix lint

20f2490

sleepytanya added the QA Passed A successful QA run through has been done label Nov 4, 2024

OGPoyraz added 2 commits November 5, 2024 11:24

Fix lint and tests

99ff5a9

Merge branch 'main' into 3053-integrate-nft-api-to-display-names-for-…

26e42ea

…nfts-within-simulations-on-mobile

OGPoyraz added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 5, 2024

Merge branch 'main' into 3053-integrate-nft-api-to-display-names-for-…

6e5ce92

…nfts-within-simulations-on-mobile

OGPoyraz added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 8, 2024

pedronfigueiredo previously approved these changes Nov 8, 2024

View reviewed changes

matthewwalsh0 requested changes Nov 13, 2024

View reviewed changes

app/components/hooks/DisplayName/useDisplayName.ts Outdated Show resolved Hide resolved

app/components/UI/Name/Name.tsx Outdated Show resolved Hide resolved

app/components/hooks/DisplayName/useNftCollectionsMetadata.ts Outdated Show resolved Hide resolved

jpuri previously approved these changes Nov 13, 2024

View reviewed changes

create useNftName hook

b730f14

OGPoyraz dismissed stale reviews from jpuri and pedronfigueiredo via b730f14 November 15, 2024 11:10

OGPoyraz added 3 commits November 15, 2024 12:22

Merge branch 'main' into 3053-integrate-nft-api-to-display-names-for-…

e50f47e

…nfts-within-simulations-on-mobile

fix tests

c35b284

Fix lint

db9b5bb

OGPoyraz added 4 commits November 15, 2024 12:47

Fix lint

7994499

Fix test

8661ac0

fix tests

df74e3b

Merge branch 'main' into 3053-integrate-nft-api-to-display-names-for-…

e742c68

…nfts-within-simulations-on-mobile

OGPoyraz added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 19, 2024

OGPoyraz added 2 commits November 19, 2024 13:10

Fix tests

d72fd08

Remove unnecessary comments

19c9a7a

OGPoyraz added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels Nov 19, 2024

Update snapshot

c3c657c

OGPoyraz enabled auto-merge November 19, 2024 13:03

matthewwalsh0 requested changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Integrate NFT api to display image & names in simulations includes `erc721`s #12160

feat: Integrate NFT api to display image & names in simulations includes `erc721`s #12160

OGPoyraz commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024 •

edited by metamaskbot

Loading

sleepytanya commented Nov 4, 2024

github-actions bot commented Nov 5, 2024 •

edited by metamaskbot

Loading

codecov-commenter commented Nov 5, 2024

github-actions bot commented Nov 8, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Nov 19, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Nov 19, 2024 •

edited by metamaskbot

Loading

sonarcloud bot commented Nov 19, 2024

matthewwalsh0 Nov 19, 2024

matthewwalsh0 Nov 19, 2024

matthewwalsh0 Nov 19, 2024

matthewwalsh0 Nov 19, 2024

matthewwalsh0 Nov 19, 2024

feat: Integrate NFT api to display image & names in simulations includes erc721s #12160

Are you sure you want to change the base?

feat: Integrate NFT api to display image & names in simulations includes erc721s #12160

Conversation

OGPoyraz commented Nov 4, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Nov 4, 2024 • edited by metamaskbot Loading

Bitrise

sleepytanya commented Nov 4, 2024

github-actions bot commented Nov 5, 2024 • edited by metamaskbot Loading

Bitrise

codecov-commenter commented Nov 5, 2024

Codecov Report

github-actions bot commented Nov 8, 2024 • edited by metamaskbot Loading

Bitrise

github-actions bot commented Nov 19, 2024 • edited by metamaskbot Loading

Bitrise

github-actions bot commented Nov 19, 2024 • edited by metamaskbot Loading

Bitrise

sonarcloud bot commented Nov 19, 2024

Quality Gate passed

matthewwalsh0 Nov 19, 2024

Choose a reason for hiding this comment

matthewwalsh0 Nov 19, 2024

Choose a reason for hiding this comment

matthewwalsh0 Nov 19, 2024

Choose a reason for hiding this comment

matthewwalsh0 Nov 19, 2024

Choose a reason for hiding this comment

matthewwalsh0 Nov 19, 2024

Choose a reason for hiding this comment

feat: Integrate NFT api to display image & names in simulations includes `erc721`s #12160

feat: Integrate NFT api to display image & names in simulations includes `erc721`s #12160

OGPoyraz commented Nov 4, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Nov 5, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Nov 8, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Nov 19, 2024 •

edited by metamaskbot

Loading

github-actions bot commented Nov 19, 2024 •

edited by metamaskbot

Loading