-
Notifications
You must be signed in to change notification settings - Fork 868
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
feat(wallet) Add NFT Pinning #16998
feat(wallet) Add NFT Pinning #16998
Conversation
@muliswilliam @cypt4 just a friendly note that nothing should be accessible/triggered without enabling the feature flag, and please open a security & privacy issue to prepare for enabling the feature flag linking to this PR and the core PR, thanks. |
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/assets/svg-icons/nft-ipfs/nfts-illustration.svg
Outdated
Show resolved
Hide resolved
const userVisibleTokensInfo = useUnsafeWalletSelector( | ||
WalletSelectors.userVisibleTokensInfo | ||
) |
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.
Can we move this down under the // redux
comment?
|
||
return BraveWallet.TokenPinStatusCode.STATUS_NOT_PINNED | ||
}, [nftsPinningStatus, pinnableNfts.length, pinnedNftsCount]) | ||
|
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.
Can we add a // methods
comment here?
components/brave_wallet_ui/components/desktop/views/nfts/nft-view.tsx
Outdated
Show resolved
Hide resolved
..._ui/components/desktop/views/portfolio/components/ipfs-node-status/ipfs-node-status.style.ts
Outdated
Show resolved
Hide resolved
...allet_ui/components/desktop/views/portfolio/components/ipfs-node-status/ipfs-node-status.tsx
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/components/shared/nft-icon/nft-icon-with-network-icon.tsx
Outdated
Show resolved
Hide resolved
components/brave_wallet_ui/nft/components/nft-details/nft-details-styles.ts
Outdated
Show resolved
Hide resolved
a2929e4
to
5853915
Compare
components/brave_wallet_ui/components/shared/nft-icon/nft-icon-with-network-icon.tsx
Show resolved
Hide resolved
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.
Looks good to me!
Nice work! 🔥
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.
LGTM
hideAccountFilter, | ||
hideAutoDiscovery | ||
}: Props) => { | ||
userAssetList, |
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.
Do we need this changes?
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.
It's ok to be merged since it's behind a feature flag. The review is still in process, so we shouldn't enable the feature by default
3a38de0
to
4f33c10
Compare
Resolves brave/brave-browser#26828
Sec review : https://github.com/brave/security/issues/1182
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
1. First Time NFT Pinning Flow:
NFT.Pinning.for.existing.tokens-1.mov
2. NFT Pinning for a newly added token
Pinning.new.token.mov