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

Selected network per origin #17986

Merged
merged 15 commits into from
Apr 20, 2023
Merged

Selected network per origin #17986

merged 15 commits into from
Apr 20, 2023

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Apr 11, 2023

Resolves brave/brave-browser#29638
Resolves brave/brave-browser#24414

Recommend reviewing this PR in the sequential commits order.

Muli-chain refactoring is necessary before we allow using different network per origin because current tx components are built on a global selected network per coin basis. The refactor is to make sure we can handle multiple transactions in different networks per coin at the same time.
When dealing with per origin selected network, we need to persist it in kBraveWalletSelectedNetworksPerOrigin which is different than kBraveWalletSelectedNetworks which is what we currently used. And we won't need to migrate kBraveWalletSelectedNetworks because it will still be used when Origin is not available as a fallback option, we call it default network now. Users can change it in brave://settings/wallet/networks or through panel in internal pages like brave://wallet

Desktop UI now also shows chain_id from tx or from request instead of selected network when showing SendTransaction, SignMessage, SignTransaction and SignAllTransactions.

Note that this PR only update desktop UI for supporting selected network per origin and leverage some fine grained chain_id API, Android doesn't get it for free. We make sure the behavior on Android doesn't change after this PR.

Follow-up issues:

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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 lint, 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:

Test different selected network for different dapps

  1. Fresh profile and open brave://settings/wallet/networks and check what current default networks are for each coin
  2. Navigate to brave.com and open wallet panel, we should see network for EVM is default network
  3. Change the network to "Aurora" and reopen the panel, it should still be "Aurora"
  4. Navigate to github.com, and open wallet panel, we should see network for EVM is default network
  5. Change the network to "Polygon" and reopen the panel, it should still be "Polygon"
  6. Navigate to brave.com and check network should be "Aurora"
  7. Navigate to github.com and check network should be "Polygon"
  8. Navigate to other sites and network should be default network.
  9. Navigate to internal wallet page should always be default network.
  10. Filecoin and Solana should have same behavior (Note that panel would remember last selected coin)

Change default network in panel

  1. Navigate to any brave:// pages.
  2. Open wallet panel and change network.
  3. Navigate to brave://settings/wallet/networks and default for that coin should be changed.

Different networks for different transactions

  1. Navigate to https://metamask.github.io/test-dapp/ and select network to be Goerli.
  2. Navigate to 127.0.0.1:8081 (setup from https://github.com/bbondy/eth-manual-tests) and select network to be Sepolia.
  3. Connect and accept from both sites.
  4. Create transactions from both sites.
  5. We can see two transaction requests with different networks in panel.

Selected network should not affect Transaction

  1. Navigate to https://metamask.github.io/test-dapp/ and select network to be Goerli
  2. Connect and approve and send a legacy transaction
  3. We should see the network display is Goerli, dismiss the panel
  4. Go to other page that doesn't have Goerli as selected network
  5. Open wallet panel and the unapproved transaction should still shows Goerli

Send Transaction

  1. Follow steps of Selected network should not affect Transaction
  2. And then approve transaction
  3. It should show up on Wallet activity as submitted.
  4. Wait for a while it should be confirmed.

Add and switch chain

  1. Navigate to https://chainlist.org/ and connect
  2. Add any chain that is not in wallet yet.
  3. Approve the switch request.
  4. Open panel and check network.
  5. Navigate to other domains, they should not be switched.
  6. Navigate to wallet page, it should still be default network.

@darkdh darkdh self-assigned this Apr 11, 2023
@darkdh darkdh force-pushed the network-per-origin branch 4 times, most recently from 3b4782a to aafd9cd Compare April 12, 2023 02:34
@darkdh darkdh marked this pull request as ready for review April 12, 2023 16:32
@darkdh darkdh requested review from a team as code owners April 12, 2023 16:32
@darkdh darkdh requested review from bbondy and yrliou April 12, 2023 16:32
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

Strings changes ++

@darkdh darkdh requested review from SergeyZhukovsky and a team April 12, 2023 17:48
@darkdh
Copy link
Member Author

darkdh commented Apr 12, 2023

Adding @SergeyZhukovsky for reviewing Fix Android build

Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

strings ++

@darkdh darkdh requested a review from a team as a code owner April 12, 2023 23:32
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++ Android part. Do we need to create an issue for Android to pass origin?

@supermassive supermassive self-requested a review April 13, 2023 03:39
@darkdh
Copy link
Member Author

darkdh commented Apr 13, 2023

++ Android part. Do we need to create an issue for Android to pass origin?

Already created: brave/brave-browser#29635

darkdh and others added 13 commits April 19, 2023 13:10
It's used to filter out unwanted origin events, ex. dapp on origin A
should not be notified when network changed on origin B.
…ShouldRun

We should be able to update all the pending txs without specifying
chain_id especially for cases like wallet unlock. When specifying
chain_id, we won't need to pull out all the pending txs, ex. approve tx.
Since now we will consider origin when determining active network,
brave://settings/wallet/networks will be used to manage default network
when per origin network is not set yet.
This only preseves backward compatibility with what current Android
behavior is to comply with new interfaces. It doesn't update Android with
new core network per origin capacity nor some new interfaces with
fine-grained chain id option.
SignAllTransactionsRequest

To simplify front end displaying correct chain_id initiated from the
dapp rather than the current selected network.
We now have cached pending_chain_ids and new pending_chain_ids as input.
When the function is called, it will stop out dated chain_id tracker and start
tracker for new chain_id. If wallet is locked, all trackers will be
stopped.
It should also be stopped when chain_id changed.
…ForOrigin

Both of them call non mojo GetChainIdSync
@darkdh
Copy link
Member Author

darkdh commented Apr 20, 2023

P3ARotationSchedulerTest.ConstellationRotation and AdBlockServiceTest.SubscribeToCustomSubscription failures on macOS are not related to this PR

@wknapik wknapik enabled auto-merge (squash) April 20, 2023 00:39
@wknapik wknapik disabled auto-merge April 20, 2023 00:39
@wknapik wknapik merged commit f5c9e0b into master Apr 20, 2023
@wknapik wknapik deleted the network-per-origin branch April 20, 2023 00:39
@github-actions github-actions bot added this to the 1.52.x - Nightly milestone Apr 20, 2023
@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 labels Apr 20, 2023
@wknapik
Copy link
Collaborator

wknapik commented Apr 20, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet
Projects
None yet
10 participants