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

Refactor wallet importer #11354

Merged
merged 3 commits into from
Dec 2, 2021
Merged

Refactor wallet importer #11354

merged 3 commits into from
Dec 2, 2021

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Dec 1, 2021

Resolves brave/brave-browser#18534
Resolves brave/brave-browser#18570

  1. ExternalWalletsImporter is dedicated for checking external wallet installation, initialization and import data from it.
  2. Mojo API now has only IsExternalWalletInstalled, IsExternalWalletInitialized and ImportFromExternalWallet with enum wallet type.
  3. Accept trailing comma in importer as a precaution

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
  • 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, npm run lint, 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:

MetaMask

  1. Install MetaMask and don't set it up yet
  2. Navigate to brave://wallet/, there won't be Import from MetaMask button available
  3. Finish setup on MetaMask
  4. Navigate to brave://wallet/, now it shows Import from MetaMask button
  5. Finish import process and check seeds and accounts are correct.

Crypto Wallets

  1. Navigate to brave://flags/#native-brave-wallet and choose Disabled and relaunch
  2. Navigate to brave://wallet/ and click "I Understand" but don't create/restore wallet
  3. Navigate to brave://flags/#native-brave-wallet and choose Default and relaunch
  4. Navigate to brave://wallet/, it won't show previous wallet detect message
  5. Repeat step 1 and setup Crypto Wallets and repeat step 3
  6. Navigate to brave://wallet/, it now shows Existing crypto wallets detected
  7. Finish import process and check seeds and accounts are correct.

@darkdh darkdh requested a review from bbondy December 1, 2021 01:37
@darkdh darkdh requested a review from a team as a code owner December 1, 2021 01:37
@darkdh darkdh self-assigned this Dec 1, 2021
@darkdh darkdh force-pushed the wallet-importer-refactor branch 4 times, most recently from 2ac180b to 54a37ee Compare December 1, 2021 21:13
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Nice cleanup :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants