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

feat(wallet) Remove IPFS NFT pinning feature #24064

Closed
wants to merge 4 commits into from
Closed

feat(wallet) Remove IPFS NFT pinning feature #24064

wants to merge 4 commits into from

Conversation

muliswilliam
Copy link
Contributor

@muliswilliam muliswilliam commented Jun 7, 2024 •
edited
Loading

This PR removes NFT pinning using IPFS from wallet frontend. This feature was introduced in this PR #16998

Resolves brave/brave-browser#38891

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • NFT Pinning feature should be available in the wallet

@muliswilliam muliswilliam requested review from a team as code owners June 7, 2024 12:59
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core puLL-Merge labels Jun 7, 2024
@muliswilliam muliswilliam force-pushed the remove-nft-pinning branch 2 times, most recently from 8e1b183 to 1965f6d Compare June 7, 2024 13:43
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

josheleonard
josheleonard reviewed Jun 7, 2024
View reviewed changes
components/brave_wallet_ui/common/async/base-query-cache.ts Outdated
return token.logo.startsWith('ipfs://')
? (await this.getIpfsGatewayTranslatedNftUrl(token.logo)) || ''
: `chrome://erc-token-images/${token.logo}`
return `chrome://erc-token-images/${token.logo}`
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move this function out from the base cache now that it no longer needs an async call into core? Maybe to asset-utils?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought about that change in context of my current task: brave/brave-browser#37735

I think we should not remove an ability to display NFT which have a address like: ipfs://ipfs://Qm...7MA (I know it works for now, because we have IPFS schema support and tranlsation IPFS address on redirect)

I mean we should not change that lines, because when we remove local IPFS node and IPFS schema at all, we will not translate IPFS addresses automatically like it works now. We will need to do it manually (by call this.getIpfsGatewayTranslatedNftUrl(token.logo)).
For example here is my draft PR: https://github.com/brave/brave-core/blob/35025563bf90a24b76e4ddca3b974435b9023f5b/components/brave_wallet_ui/common/async/base-query-cache.ts#L373-L375, I leaved that place unchanged.

I mean in case of NFT's address like ipfs://ipfs://Qm...7MA we should call this.getIpfsGatewayTranslatedNftUrl(token.logo) for translation from ipfs://ipfs://Qm...7MA to https://ipfs.io/ipfs/Qm...7MA

WDYT?

Copy link
Contributor Author

@muliswilliam muliswilliam Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@vadimstruts I agree, let's keep this function as is. I will make the change

components/brave_wallet_ui/common/constants/local-storage-keys.ts
@@ -9,7 +9,6 @@ export const LOCAL_STORAGE_KEYS = {
HIDE_PORTFOLIO_NFTS_TAB: 'BRAVE_WALLET_HIDE_PORTFOLIO_NFTS_TAB',
PORTFOLIO_ASSET_FILTER_OPTION: 'PORTFOLIO_ASSET_FILTER_OPTION',
PORTFOLIO_TIME_LINE_OPTION: 'PORTFOLIO_TIME_LINE_OPTION',
IS_IPFS_BANNER_HIDDEN: 'BRAVE_WALLET_IS_IPFS_BANNER_HIDDEN',
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move to the deprecated list so we can delete the data from the user's local storage?

@github-actions GitHub Actions
Copy link
Contributor

[puLL-Merge] - brave/brave-core@24064

Description

This PR removes the NFT pinning feature and related code from the Brave Wallet codebase. The NFT pinning feature allowed users to pin their NFTs to a local IPFS node for extra security and backup. The removal impacts UI components, API endpoints, constants, styles, and utility functions across the Brave Wallet UI.

The motivation for this change is not explicitly stated, but it appears the NFT pinning feature is being removed or discontinued.

Changes

Changes

The main changes in this PR, organized by filename, are:

brave_wallet_constants.h:

  • Removes string constants related to NFT pinning

brave_wallet_service.cc:

  • Removes extra space in DiscoverEthAllowancesCallback function

Various files under components/brave_wallet_ui/:

  • Deletes SVG and PNG icon assets related to NFT pinning
  • Removes API endpoints, hooks, selectors, slices related to NFT pinning
    • Removes pinning related fields from WalletState and PageState interfaces
  • Deletes UI components for NFT pinning banner, status, tooltips, info screens
  • Removes NFT pinning routes and options from navigation and routing
  • Updates mocks and stories to remove NFT pinning
  • Simplifies token logo fetching logic in asset-utils.ts
  • Removes NFT pinning specific string utilities

components/resources/wallet_strings.grdp:

  • Removes all NFT pinning related strings

In general, this is a cleanup PR that removes a discontinued feature and all its associated code.

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

supermassive
supermassive reviewed Jun 12, 2024
View reviewed changes
components/brave_wallet/browser/brave_wallet_service.cc
@@ -1555,7 +1555,7 @@ void BraveWalletService::Reset() {
}

void BraveWalletService::DiscoverEthAllowances(
DiscoverEthAllowancesCallback callback) {
DiscoverEthAllowancesCallback callback) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadimstruts vadimstruts vadimstruts left review comments

@supermassive supermassive supermassive left review comments

@josheleonard josheleonard josheleonard approved these changes

Assignees
No one assigned
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet puLL-Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove IPFS NFT feature pinning from wallet
5 participants
@muliswilliam @brave-builds @supermassive @josheleonard @vadimstruts