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.

Sign up for GitHub

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

Jump to bottom

Removing an NFT from a collection removes all NFTs of that collection #24238

Closed
bbondy opened this issue Jul 24, 2022 · 3 comments · Fixed by brave/brave-core#14435
Closed

Removing an NFT from a collection removes all NFTs of that collection #24238

bbondy opened this issue Jul 24, 2022 · 3 comments · Fixed by brave/brave-core#14435
Assignees
@muliswilliam
Labels
feature/web3/wallet Integrating Ethereum+ wallet support OS/Android Fixes related to Android browser functionality OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/include
Milestone
1.44.x - Release

Comments

@bbondy
Copy link
Member

bbondy commented Jul 24, 2022 •
edited

If I go and try to remove one NFTs in a collection as other NFTs I have in the same collection, all remove from my portfolio. I guess we are probably removing by contract ID only and not taking into account the Token ID.
@bbondy bbondy added priority/P3 The next thing for us to work on. It'll ride the trains. OS/Android Fixes related to Android browser functionality OS/Desktop labels Jul 24, 2022
@muliswilliam
Copy link
Collaborator

The first issue i.e balances is fixed here: brave/brave-core#13370. Expected to be merged soon

muliswilliam added a commit to brave/brave-core that referenced this issue Jul 25, 2022
@muliswilliam
Revert "chore: add tokenId check when removing asset from visible ass…
422db96 
…ets list"

This reverts commit 440134d. This will be fixed in brave/brave-browser#24238
@bbondy bbondy changed the title 2 issues with dealing with NFTs Removing an NFT from a collection removes all NFTs of that collection Jul 25, 2022
@bbondy
Copy link
Member Author

bbondy commented Jul 25, 2022

Thanks, I edited my issue title and description to focus around the 2nd in that case.

muliswilliam added a commit to brave/brave-core that referenced this issue Jul 26, 2022
@muliswilliam
Revert "chore: add tokenId check when removing asset from visible ass…
2a7c904 
…ets list"

This reverts commit 440134d. This will be fixed in brave/brave-browser#24238
muliswilliam added a commit to brave/brave-core that referenced this issue Jul 28, 2022
@muliswilliam
Revert "chore: add tokenId check when removing asset from visible ass…
73b9bed 
…ets list"

This reverts commit 440134d. This will be fixed in brave/brave-browser#24238
muliswilliam added a commit to brave/brave-core that referenced this issue Jul 28, 2022
@muliswilliam
NFT Details Screen (#13370)
0b6165f 
* Add NFT details screen

* chore: remove console.log

* chore: remove chrome://image prefix

* chore: fix loading of nft asset use url params

* chore: add skeleton loader for nft image

* chore: check token id when deleting assets

* chore: remove redundant lines

* chore: add tokenId to dep list

* feat: display visible asset modal asset icon in chrome-untrusted

* chore: trim url

* chore: add tokenId check when removing asset from visible assets list

* chore: fix tokenBalanceRegistry keys to avoid duplicates for erc721 tokens

* chore: refactor nft details screen

* chore: add border-radius to nft image

* feat: create react app for rendering nft details in a chrome-untrusted tab

* Add string to access via loadTimeData object

* Add configurations nft ui untrusted tab

* Update build requirements for nft ui untrusted tab

* Send message to nft iframe to update state

* Added iframe component

* Create components for diplaying nft content in chrome untrusted tab

* Added utilities for communicating with nft chrome untrusted tab using postmessage

* chore: remove unused component

* chore: validate url to avoid xss attack

* feat: change nft icon display from query parameter(imageUrl) to postmessage api

* use specific targetOrigin in postMessage

* chore: remove console.log

* chore: fix feedback

* chore: feedback

* chore: add license header and documentation

* Revert "chore: add tokenId check when removing asset from visible assets list"

This reverts commit 440134d. This will be fixed in brave/brave-browser#24238

* chore: fix unstable test behaviour

* chore: fix tests

* chore: rename function

* chore: refactor to avoid adding unnecessary string

* chore: change copyright year

* chore: add braveWalletLedgerBridgeUrl string

* chore: use getWalletPageProxy

* chore: useCallback
AlexNguyen1612 pushed a commit to AlexNguyen1612/brave-core that referenced this issue Jul 31, 2022
@muliswilliam @AlexNguyen1612
NFT Details Screen (brave#13370)
1f6783f 
* Add NFT details screen

* chore: remove console.log

* chore: remove chrome://image prefix

* chore: fix loading of nft asset use url params

* chore: add skeleton loader for nft image

* chore: check token id when deleting assets

* chore: remove redundant lines

* chore: add tokenId to dep list

* feat: display visible asset modal asset icon in chrome-untrusted

* chore: trim url

* chore: add tokenId check when removing asset from visible assets list

* chore: fix tokenBalanceRegistry keys to avoid duplicates for erc721 tokens

* chore: refactor nft details screen

* chore: add border-radius to nft image

* feat: create react app for rendering nft details in a chrome-untrusted tab

* Add string to access via loadTimeData object

* Add configurations nft ui untrusted tab

* Update build requirements for nft ui untrusted tab

* Send message to nft iframe to update state

* Added iframe component

* Create components for diplaying nft content in chrome untrusted tab

* Added utilities for communicating with nft chrome untrusted tab using postmessage

* chore: remove unused component

* chore: validate url to avoid xss attack

* feat: change nft icon display from query parameter(imageUrl) to postmessage api

* use specific targetOrigin in postMessage

* chore: remove console.log

* chore: fix feedback

* chore: feedback

* chore: add license header and documentation

* Revert "chore: add tokenId check when removing asset from visible assets list"

This reverts commit 440134d. This will be fixed in brave/brave-browser#24238

* chore: fix unstable test behaviour

* chore: fix tests

* chore: rename function

* chore: refactor to avoid adding unnecessary string

* chore: change copyright year

* chore: add braveWalletLedgerBridgeUrl string

* chore: use getWalletPageProxy

* chore: useCallback
@muliswilliam muliswilliam mentioned this issue Aug 2, 2022
Merged
25 tasks
@brave-builds brave-builds added this to the 1.44.x - Nightly milestone Aug 3, 2022
husobee pushed a commit to brave/brave-core that referenced this issue Aug 25, 2022
@muliswilliam @husobee
NFT Details Screen (#13370)
95a7f77 
* Add NFT details screen

* chore: remove console.log

* chore: remove chrome://image prefix

* chore: fix loading of nft asset use url params

* chore: add skeleton loader for nft image

* chore: check token id when deleting assets

* chore: remove redundant lines

* chore: add tokenId to dep list

* feat: display visible asset modal asset icon in chrome-untrusted

* chore: trim url

* chore: add tokenId check when removing asset from visible assets list

* chore: fix tokenBalanceRegistry keys to avoid duplicates for erc721 tokens

* chore: refactor nft details screen

* chore: add border-radius to nft image

* feat: create react app for rendering nft details in a chrome-untrusted tab

* Add string to access via loadTimeData object

* Add configurations nft ui untrusted tab

* Update build requirements for nft ui untrusted tab

* Send message to nft iframe to update state

* Added iframe component

* Create components for diplaying nft content in chrome untrusted tab

* Added utilities for communicating with nft chrome untrusted tab using postmessage

* chore: remove unused component

* chore: validate url to avoid xss attack

* feat: change nft icon display from query parameter(imageUrl) to postmessage api

* use specific targetOrigin in postMessage

* chore: remove console.log

* chore: fix feedback

* chore: feedback

* chore: add license header and documentation

* Revert "chore: add tokenId check when removing asset from visible assets list"

This reverts commit 440134d. This will be fixed in brave/brave-browser#24238

* chore: fix unstable test behaviour

* chore: fix tests

* chore: rename function

* chore: refactor to avoid adding unnecessary string

* chore: change copyright year

* chore: add braveWalletLedgerBridgeUrl string

* chore: use getWalletPageProxy

* chore: useCallback
@LaurenWags LaurenWags added the feature/web3/wallet Integrating Ethereum+ wallet support label Aug 31, 2022
@srirambv
Copy link
Contributor

Verification passed on

Brave 1.44.97 Chromium: 106.0.5249.40 (Official Build) (64-bit)
Revision 4d5f098fca6ab7f4b6b7c240be3d9593c2357709-refs/branch-heads/5249@{#531}
OS Linux
  • Verified steps from brave/brave-core#14435
  • Verified able to delete NFT via add custom asset list
  • Verified deleting an NFT from a collection doesn't remove all of the NFT's from that collection
24238.mp4

Verification passed on

Brave 1.44.97 Chromium: 106.0.5249.55 (Official Build) (64-bit)
Revision 4d5f098fca6ab7f4b6b7c240be3d9593c2357709-refs/branch-heads/5249@{#531}
OS Windows 11 Version 22H2 (Build 22621.521)
  • Verified steps from brave/brave-core#14435
  • Verified able to delete NFT via add custom asset list
  • Verified deleting an NFT from a collection doesn't remove all of the NFT's from that collection
24238.mp4

Verification passed on

Brave 1.44.97 Chromium: 106.0.5249.40 (Official Build) (arm64)
Revision 4d5f098fca6ab7f4b6b7c240be3d9593c2357709-refs/branch-heads/5249@{#531}
OS macOS Version 12.4 (Build 21F79)
  • Verified steps from brave/brave-core#14435
  • Verified able to delete NFT via add custom asset list
  • Verified deleting an NFT from a collection doesn't remove all of the NFT's from that collection
24238.mov

@kjozwiak kjozwiak mentioned this issue Sep 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@muliswilliam muliswilliam

Labels
feature/web3/wallet Integrating Ethereum+ wallet support OS/Android Fixes related to Android browser functionality OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/include
Projects
Web3
Archived in project
Milestone
1.44.x - Release
Development

Successfully merging a pull request may close this issue.

fix(wallet): Fix removal of NFTs in a collection brave/brave-core
5 participants
@bbondy @srirambv @LaurenWags @brave-builds @muliswilliam