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

Deprecate local node support and ipfs scheme #23808

Conversation

vadimstruts
Copy link
Collaborator

@vadimstruts vadimstruts commented May 23, 2024

Resolves brave/brave-browser#37735

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:

iOS IPFS Deprecation Test Plan:

  1. open Brave iOS
  2. Tap ...
  3. Go to Settings/Web3
  4. Observe that no IPFS related settings are displayed in the screen
  5. Make sure all Web3 Domain settings are all set to Ask
  6. Go back to home page
  7. input an IPFS scheme url in omnibox. For example: ipfs://QmX4nfgA35MiW5APoc4P815hMcH8hAt7edi5H3wXkFm485/1688.html
  8. No IPFS configuration interstitial page will be loaded. and IPFS url will not be resolved and loaded. A search will be performed instead.
  9. Input a SNS url in omnibox. For example: http://onybose.sol/
  10. An interstitial page will be loaded. Tap Proceed using an SNS server
  11. Observe the SNS url is resolved and loaded
  12. Go back to Settings/Web3. Observe Resolve SNS Domain Names is now set to Enabled
  13. Change that to Ask abd go back to home page
  14. Repeat step 9 and 10 but tap Disable
  15. Observe an error page is loaded
  16. Go back to Settings/Web3. Observe Resolve SNS Domain Names is now set to Disabled
  17. Go back to home page and input a ENS url in omnibox. For example: http://offchainexample.eth/ (the resolved url is an IPFS scheme url)
  18. An interstitial page will be loaded. Tap Proceed using infura server
  19. Another interstitial page will be loaded. Tap Proceed with offchain lookup
  20. observe the ENS is now resolved and loaded. The process is ENS -> offchain lookup -> IPFS url resolved. (Even though IPFS is deprecated. but for IPFS url that was the result of other Web3 Domain Names. BraveCore will resolve those IPFS urls using the public gateway https://ipfs.io/)
  21. Go to Settings/Web3. Observe Resolve ENS Domain Names and Allow ENS Offchain Lookup are now set to Enable
  22. Change them to Ask and go back home page
  23. Repeat step 18 and 19 but tap Disable
  24. Observe an error page is loaded
  25. Change Resolve ENS Domain Names to Enabled in Settings/Web3
  26. Go back to home and reload the error page
  27. Another interstitial page will be loaded. Tap Disable
  28. Observe an error page is loaded
  29. Input an unstoppable domains in omnibox. For example: jim-unstoppable.crypto
  30. Another interstitial page will be loaded. Tap Proceed using infura server
  31. Observe the url is now resolved using the public gateway https://ipfs.io/
  32. Go back to Settings/Web3 and disable Resolve Unstoppable Domains Domain Names
  33. Repeat step 30
  34. Observe an error page is loaded

@vadimstruts vadimstruts force-pushed the 37735-consider-deprecating-local-node-support-and-ipfs-scheme branch from 3265207 to d6468fe Compare May 23, 2024 17:13
@github-actions github-actions bot added CI/run-network-audit Run network-audit CI/storybook-url Deploy storybook and provide a unique URL for each build CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) feature/web3/wallet feature/web3/wallet/core labels May 23, 2024
@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@vadimstruts vadimstruts force-pushed the 37735-consider-deprecating-local-node-support-and-ipfs-scheme branch from ad4df82 to 6c92fe8 Compare May 27, 2024 10:37
@brave-builds
Copy link
Collaborator

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

@vadimstruts vadimstruts force-pushed the 37735-consider-deprecating-local-node-support-and-ipfs-scheme branch from 10e5ba4 to 7807d26 Compare May 27, 2024 17:51
@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

"//brave/components/json/rs:rust_lib",
"//brave/components/resources:strings_grit",
"//brave/components/services/brave_wallet/public/cpp/utils",
"//brave/components/services/brave_wallet/public/mojom",
"//chrome/browser:browser",
Copy link
Collaborator

Choose a reason for hiding this comment

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

components must not depend on chrome

Comment on lines 549 to 566
<message name="IDS_BRAVE_WALLET_NFT_DETAIL_IMAGE_ADDRESS" desc="NFT detail image address category">Image URL</message>
<message name="IDS_BRAVE_WALLET_NFT_DETAILS_PINNING_IN_PROGRESS" desc="NFT detail pinning in progress">In progress</message>
<message name="IDS_BRAVE_WALLET_NFT_DETAILS_PINNING_SUCCESSFUL" desc="NFT detail pinning successful">Pinned</message>
<message name="IDS_BRAVE_WALLET_NFT_DETAILS_PINNING_FAILED" desc="NFT detail pinning failed">Failed</message>
<message name="IDS_BRAVE_WALLET_NFT_DETAILS_OVERVIEW" desc="NFT detail overview">Overview</message>
Copy link
Collaborator

@supermassive supermassive May 28, 2024

Choose a reason for hiding this comment

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

There is still lots of WALLET_NFT_PINNING strings. Should we also clear them?

Comment on lines 57 to 61
constexpr char kBraveIpfsFeatureMigrated[] = "brave.ipfs";
constexpr char kBraveIpfsCompanionMigrated[] = "brave.ipfs_companion_enabled";
constexpr char kPinnedNFTAssetsMigrated[] = "brave.wallet.user_pin_data";
constexpr char kAutoPinEnabledMigrated[] = "brave.wallet.auto_pin_enabled";

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's clear there only wallet prefs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also please add a comment with deprecation date (06/2024?) so we can remove them completely from code after a year

@@ -62,7 +62,6 @@ inline constexpr char kAdControlType[] = "brave.ad_default";
inline constexpr char kGoogleLoginControlType[] = "brave.google_login_default";
inline constexpr char kWebTorrentEnabled[] = "brave.webtorrent_enabled";
inline constexpr char kHangoutsEnabled[] = "brave.hangouts_enabled";
inline constexpr char kIPFSCompanionEnabled[] = "brave.ipfs_companion_enabled";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be cleared with all other ipfs prefs I think

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
Collaborator

Choose a reason for hiding this comment

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

getTokenLogo no longer needs to be async

Copy link
Collaborator

Choose a reason for hiding this comment

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

how that would work if token.logo still starts with ipfs:// ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need thiese styles anymore?

@@ -47,31 +35,12 @@ interface Props {
export const LocalIpfsNodeScreen = (props: Props) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this file also?

Comment on lines 38 to 40
route.includes(WalletRoutes.Swap) ||
route.includes(WalletRoutes.Send) ||
route.includes(WalletRoutes.LocalIpfsNode)
if (isPanel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why Swap and Send appear here?

@vadimstruts vadimstruts force-pushed the 37735-consider-deprecating-local-node-support-and-ipfs-scheme branch from 539c662 to bf9c57d Compare May 31, 2024 12:16
@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@brave-builds
Copy link
Collaborator

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

@vadimstruts vadimstruts force-pushed the 37735-consider-deprecating-local-node-support-and-ipfs-scheme branch from cb5e312 to 3476084 Compare June 4, 2024 08:07
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
Signed-off-by: Vadym Struts <[email protected]>
@vadimstruts vadimstruts force-pushed the 37735-consider-deprecating-local-node-support-and-ipfs-scheme branch from b2f567a to 8dd42ab Compare July 8, 2024 20:09
@brave-builds
Copy link
Collaborator

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

@vadimstruts vadimstruts merged commit 43735ef into master Jul 9, 2024
19 checks passed
@vadimstruts vadimstruts deleted the 37735-consider-deprecating-local-node-support-and-ipfs-scheme branch July 9, 2024 10:34
@github-actions github-actions bot added this to the 1.69.x - Nightly milestone Jul 9, 2024
Comment on lines -65 to -69
"chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc": [
"CHROME_BROWSER_WEB_APPLICATIONS_COMMANDS_CLEAR_BROWSING_DATA_COMMAND_H_",
"CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_PROVIDER_H_",
"CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_UTILS_H_",
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this particular change intentional? I don't see how it's relevant to this PR, but I could definitely be missing something. Asking because check_chromium_src.py is now failing with the following messages:

  Override chromium_src\chrome\browser\browsing_data\chrome_browsing_data_remover_delegate.cc
  defines symbol CHROME_BROWSER_WEB_APPLICATIONS_COMMANDS_CLEAR_BROWSING_DATA_COMMAND_H_ but the symbol could not be found in
  C:\work\brave-browser\src\chrome\browser\browsing_data\chrome_browsing_data_remover_delegate.cc
  and is not used internally in the override.
-------------------------
[Error] chromium_src:
  Override chromium_src\chrome\browser\browsing_data\chrome_browsing_data_remover_delegate.cc
  defines symbol CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_PROVIDER_H_ but the symbol could not be found in
  C:\work\brave-browser\src\chrome\browser\browsing_data\chrome_browsing_data_remover_delegate.cc
  and is not used internally in the override.
-------------------------
[Error] chromium_src:
  Override chromium_src\chrome\browser\browsing_data\chrome_browsing_data_remover_delegate.cc
  defines symbol CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_UTILS_H_ but the symbol could not be found in
  C:\work\brave-browser\src\chrome\browser\browsing_data\chrome_browsing_data_remover_delegate.cc
  and is not used internally in the override.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it was mistake, returned it back with PR: #24624

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, thank you for fixing!

@chenjia404
Copy link

Can I install this feature separately? https://github.com/brave/brave-browser/wiki/IPFS-Features#import-and-sharing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-macos-x64 Run CI builds for macOS x64 CI/run-network-audit Run network-audit CI/run-upstream-tests Run upstream unit and browser tests on Linux and Windows (otherwise only on Linux) CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate local node support and ipfs:// scheme