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

EVM NFT section #15467

Merged
merged 17 commits into from
Dec 20, 2024
Merged

EVM NFT section #15467

merged 17 commits into from
Dec 20, 2024

Conversation

enjojoy
Copy link
Contributor

@enjojoy enjojoy commented Nov 20, 2024

Description

NFT section is introduced for EVM based chains, enabled through experimental features. Supportes NFTs are ERC721 and ERC1155.

Related Issue

Resolve #8004
Resolve #12165 (last issue)

Screenshots:

In experimental features:

image

Screenshot 2024-12-16 at 13 58 36

Screenshot 2024-12-16 at 13 59 57
Screenshot 2024-12-16 at 14 00 18
Screenshot 2024-12-16 at 14 00 41
Screenshot 2024-12-19 at 18 49 35
Screenshot 2024-12-19 at 18 50 08
Screenshot 2024-12-19 at 18 50 39
Screenshot 2024-12-19 at 18 51 01

@enjojoy enjojoy force-pushed the feat/nft-section branch 2 times, most recently from 92e75fd to 4b72ec9 Compare November 21, 2024 12:51
@tomasklim tomasklim assigned tomasklim and unassigned tomasklim Nov 22, 2024
@enjojoy enjojoy force-pushed the feat/nft-section branch 3 times, most recently from 7a926bb to 13d1ed5 Compare November 22, 2024 14:28
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

I understand that this is just a PR draft, but I would like to avoid duplicating code and making additional changes later.

@enjojoy enjojoy force-pushed the feat/nft-section branch 7 times, most recently from 1478947 to 1368894 Compare December 3, 2024 12:19
@enjojoy
Copy link
Contributor Author

enjojoy commented Dec 3, 2024

I understand that this is just a PR draft, but I would like to avoid duplicating code and making additional changes later.

I appreciate this approach

@enjojoy enjojoy force-pushed the feat/nft-section branch 2 times, most recently from fc7faee to deb7eaf Compare December 3, 2024 18:37
@trezor trezor deleted a comment from github-actions bot Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 21
  • More info

Learn more about 𝝠 Expo Github Action

@enjojoy enjojoy force-pushed the feat/nft-section branch 4 times, most recently from f1df078 to 3389b4c Compare December 4, 2024 13:52
@enjojoy enjojoy changed the title feat(suite): nft section EVM NFT section Dec 4, 2024
@tomasklim
Copy link
Member

Tokens table shows this when no result found
Screenshot 2024-12-19 at 16 43 55

(only in legit tab, in hidden it does not work same as for nfts)

NFT:
Screenshot 2024-12-19 at 16 43 59

@enjojoy
Copy link
Contributor Author

enjojoy commented Dec 19, 2024

Tokens table shows this when no result found Screenshot 2024-12-19 at 16 43 55

(only in legit tab, in hidden it does not work same as for nfts)

NFT: Screenshot 2024-12-19 at 16 43 59

fixed this, and for hidden tokens too

@enjojoy enjojoy requested a review from tomasklim December 19, 2024 17:59
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

Screenshot 2024-12-19 at 21 33 26 now it really does not work

Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@tomasklim tomasklim merged commit 2a6cdab into develop Dec 20, 2024
30 checks passed
@tomasklim tomasklim deleted the feat/nft-section branch December 20, 2024 10:27
@enjojoy
Copy link
Contributor Author

enjojoy commented Dec 20, 2024

@trezor/qa

NFT section is introduced as an experimental feature. You have to turn it on in the settings. Only for EVM based chains.

Standards

It works with 2 types of NFT standards:

ERC-721 (Non-Fungible Token Standard):
Each token has a unique ID (tokenId) that distinguishes it from others. (although it's possible that there can be NFTs with same IDs, especially in scam collections). But in the usual case there should be only one item of each ID.

ERC-1155 (Multi-Token Standard):
Each token can be represented multiple times.

In the table you can't tell which is which, you can check it in the explorer.

Empty collections

If a collection has been present in the account before, but now it's empty (because user sent all the nfts away using a third party wallet), it will be displayed in the "Empty collections" on the last row of the table.

Recognized/unrecognized collections

A collection can be recognized (based on information from coingecko), and unrecognized, in that case it's displayed in the hidden part. (It's similar as in tokens)

Tips for testing

For testing I recommend you to buy some cheap recognized collection, on BSC or Polygon to avoid high gas fees. You don't have to worry about unrecognized collections because there's a ton of spam collections on QA seed. (on Polygon for sure)

https://www.coingecko.com/en/nft/chains/binance-smart-chain you can check out the collections there and filter it by the lowest floor price.

Then you can find it either on OpenSea or MagicEden (or another NFT marketplace of your choice). You'll have to connect using a third party wallet (can be Metamask or Rabby)

I want to ask you to really take your time and test every possible scenario with recognized/unrecognized collections, with each standard as well as empty and hidden collections. Maybe on the L2s in debug mode as well.

This is a big feature and as you can see in the amount of comments, it's easy to overlook something.

May you have any questions, feel free to reach out to me

@@ -545,7 +546,7 @@ export const networks = {
decimals: 18,
testnet: true,
explorer: getExplorerUrls('https://holesky1.trezor.io', 'ethereum'),
features: ['rbf', 'sign-verify', 'tokens', 'staking'],
features: ['rbf', 'sign-verify', 'tokens', 'nfts', 'nft-definitions'],
Copy link
Member

Choose a reason for hiding this comment

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

removed staking from ethereum holesky :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing here #16129

thank you for the catch

@bosomt bosomt mentioned this pull request Jan 3, 2025
@bosomt
Copy link
Contributor

bosomt commented Jan 7, 2025

QA OK

  • enable/disable ✅
  • empty/not empty/multi collection ✅
  • hide/unhide ✅
  • view all transactions filter ✅
  • view in explorer ✅
  • ERC1155 NFT on Arbitrum ✅
  • ERC721 NFT on Polygon ✅
  • explorer link ✅
  • lots of hidden NFTs :)

Info:

  • Suite version: web 25.1.0 (8f3ea38)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T3T1 2.8.7 regular (revision f2aeea159f1fde98d4fc5575350a8c9fc4478367)
  • Transport: WebUsbTransport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EVM tokens NFTs section
6 participants