Skip to content

feat(wallet): iOS Wallet Network Settings #23437

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

Merged
merged 6 commits into from
May 9, 2024
Merged

Conversation

nuo-xu
Copy link
Contributor

@nuo-xu nuo-xu commented May 6, 2024

Resolves brave/brave-browser#37741
The Show Test Networks toggle is now removed
All networks including primary networks, sub-networks, test networks, custom neworks, are all listed in this screen

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:

  • network lists
  1. in an older version of nightly Brave, make sure Show Test Networks is turned off
  2. install a nightly with this PR in.
  3. open wallet and go to wallet settings / networks
  4. you should see test networks for each coin type are listed with the eye-closed icon indicates they are hidden
  5. Repeat step 1 to 4, but turn on Show Test Networks. You should see test networks for each coin type are listed with eye-opened icon indicates they are not hidden.
  • default networks
  1. Go to wallet settings/networks
  2. For each coin type, there is one network with eye-opened icon but grey-out indicates hiding this network is disabled. This also means this is the default network for this coin type.
  3. tap the 3-dot button, only Edit is available
  • add custom networks
  1. try to add a new custom network
  2. it will listed in the network list.
  3. tap the 3-dot button, you can Edit/Delete/Set as Default
  • remove custom networks
  1. Remove a custom network via the 3-dot context menu
  2. This network should be removed from the list
  • edit networks
  1. Edit a network via the 3-dot context menu
  2. save the change should be successfully
  • Hide networks
  1. hide some networks by tapping the eye-opened icon
  2. it will become eye-closed icon if its hidden
  3. after that, check hidden networks should not be listed in filter or network selection screen. Assets that are grouped in hidden networks should not be displayed
  • unhide networks
  1. unhide some networks by tapping the eye-closed icon
  2. it will become eye-opened icon if its not hidden
  3. after that, check hidden networks should be listed in filter or network selection screen. Assets that are grouped in these networks should be displayed

@nuo-xu nuo-xu added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 labels May 6, 2024
@nuo-xu nuo-xu requested a review from StephenHeaps May 6, 2024 14:16
@nuo-xu nuo-xu self-assigned this May 6, 2024
@nuo-xu nuo-xu requested a review from a team as a code owner May 6, 2024 14:16
@nuo-xu nuo-xu force-pushed the wallet/ios-network-settings branch from 3bd3459 to c152ce1 Compare May 8, 2024 19:37
Copy link
Contributor

github-actions bot commented May 9, 2024

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

Here is my review of the PR:

Description

This PR makes several changes related to how network visibility is handled in the Brave Wallet. The main changes are:

  1. Test networks are now hidden by default in core and can be unhidden through new APIs rather than using a preference
  2. The custom networks screen was renamed to just "Networks" and now allows hiding/unhiding any network
  3. A default network can now be set for each coin type
  4. The UserAssetsStore is now opened/closed as needed rather than being a property on other stores

The motivation seems to be to improve the UX around network selection and visibility and to avoid issues with preferences getting out of sync with core.

Security Hotspots

This change does not appear to introduce any new security vulnerabilities. The main sensitive data being handled are the user's crypto assets, but the changes here are focused on the UI layer and do not fundamentally alter how assets are stored or accessed.

Changes

Changes

Changes by file:

CryptoTabsView.swift

  • Opens and closes the UserAssetsStore as needed rather than using a global instance

FiltersDisplaySettingsView.swift

  • Updates the allNetworksSelected computed property to check the new hiddenChains property rather than using a preference

NetworkFilterView.swift

  • Similar change as above to check hiddenChains

NetworkPicker.swift

  • Removes availableChains computed property that checked preferences
  • Updates sheet to present NetworkDetailsView instead of CustomNetworkDetailsView

NetworkSelectionRootView.swift
NetworkSelectionView.swift

  • Remove checks for showTestNetworks preference

PortfolioView.swift

  • Similar changes to open/close UserAssetsStore

AssetSearchView.swift

  • Use visibleChains instead of allChains

AccountsStore.swift
NFTStore.swift
TransactionsActivityStore.swift
UserAssetsStore.swift

  • Remove handling of showTestNetworks preference changes

NetworkStore.swift

  • Add hiddenChains and visibleChains properties
  • Add defaultNetworks property to track default for each coin
  • Update updateChainList() to populate the above
  • Add methods to add/remove hidden networks and set default

Web3SettingsView.swift

  • Rename Networks screen

WalletPreferences.swift

  • Add preference to track migration of showTestNetworks

RpcServiceExtensions.swift

  • Update allNetworksForSupportedCoins() to respect hidden rather than preferences
  • Add methods to get/set hidden networks

SuggestedNetworkView.swift
CustomNetworkDetailsView.swift
CustomNetworkListView.swift

  • Rename custom network screens to just "Network"
  • Update to allow hiding any network and setting default

In summary, this is a significant refactor of how network visibility is handled, but the changes are fairly contained to the network and asset management layers. Thorough testing of network-related features would be prudent given the scope of changes.

@nuo-xu nuo-xu merged commit 4b0a76e into master May 9, 2024
19 checks passed
@nuo-xu nuo-xu deleted the wallet/ios-network-settings branch May 9, 2024 20:26
@github-actions github-actions bot added this to the 1.68.x - Nightly milestone May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS/Wallet]: Network Settings inside Wallet settings to hide/unhide networks
2 participants