-
Notifications
You must be signed in to change notification settings - Fork 40
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
[BX-1314][BX-1315] Support Additional Simplehash Networks + Configure Testnet NFT Request #1336
Conversation
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
src/core/resources/nfts/nfts.ts
Outdated
); | ||
const chains = !testnetMode | ||
? (getSimpleHashSupportedChainNames() as ChainName[]) | ||
: (getSimpleHashSupportedTestnetChainNames() as ChainName[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means that we're not filtering nfts by enabled chains from settings anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait, do we even filter by enabled chain?
Here's the packed extension for this build: |
Here's the packed extension for this build: |
nft?.network === 'mainnet' ? 'Ethereum' : capitalize(nft?.network); | ||
const networkDisplay = nft?.network | ||
? ChainNameDisplay[chainIdFromChainName(nft?.network)] | ||
: ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this ''
just used as a "falsy" value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just not showing that row if nft is undefined. Just being safe, that shouldn't be the case unless something broke.
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
Here's the packed extension for this build: |
… Testnet NFT Request (#1336)
… Testnet NFT Request (#1336)
… Testnet NFT Request (#1336)
What changed (plus any additional context for devs)
Added support for all available networks returned by Simplehash that also overlapped with Viem's supported network list. Partitioned mainnet and testnet nft network lists and now return testnet NFTs in dev mode.
Screen recordings / screenshots
PoW: https://recordit.co/jowwjwvIp2
What to test
Ensure NFTs on networks that are currently unsupported, but added to the Simplehash request in this PR appear. Also ensure that testnet nfts only appear while dev mode is activated.