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: refactor native wallet support with better encryption #398

Merged
merged 8 commits into from
Nov 17, 2021
Merged

feat: refactor native wallet support with better encryption #398

merged 8 commits into from
Nov 17, 2021

Conversation

cjthompson
Copy link
Contributor

@cjthompson cjthompson commented Nov 17, 2021 •
edited
Loading

Description

  • Implement better native wallet encryption and storage
  • Load a previous native wallet that's already been saved
  • Don't automatically connect a native wallet on load

Notice

Before submitting a pull request, please make sure you have answered the following:

  • Have you followed the guidelines in our Contributing guide?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Do all new and existing tests pass? Does the linter pass?

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #376
closes #6

Testing

  • Connect a Wallet
  • Click ShapeShift
  • Try everything

Screenshots

Pairing screen

image

Load Wallet

image

Password Prompt

image

Create a Wallet

image

Backup Phrase Test

image

Saving/Encrypting Wallet

image

@cjthompson cjthompson requested a review from a team as a code owner November 17, 2021 03:28
@vercel
Copy link

vercel bot commented Nov 17, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shapeshift/web/AgyPVh1qztUyLxSuta9UpwyQ4tRB
✅ Preview: https://web-git-refactor-native-shapeshift.vercel.app

@cjthompson cjthompson force-pushed the refactor-native branch 2 times, most recently from eebc3d8 to 5cb9f61 Compare November 17, 2021 03:32
mrnerdhair
mrnerdhair reviewed Nov 17, 2021
View reviewed changes
src/context/WalletProvider/NativeWallet/components/NativePassword.tsx
</ModalHeader>
<ModalBody>
<Text mb={6} color='gray.500' translation={'walletProvider.shapeShift.nativePasswd.body'} />
<Text mb={6} color='gray.500' translation={'walletProvider.shapeShift.password.body'} />
<form onSubmit={handleSubmit(onSubmit)}>
Copy link
Contributor

@mrnerdhair mrnerdhair Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It would probably be a decent idea to make this component revocable as well... would suck if someone could pull the password itself out of react state history.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's such a think as "state history". Once the component is unmounted, the state is gone, not just hidden.

mrnerdhair
mrnerdhair reviewed Nov 17, 2021
View reviewed changes
src/components/Modals/KeyManagement/Native/Password.tsx
</ModalHeader>
<ModalBody>
<Text mb={6} color='gray.500' translation={'walletProvider.shapeShift.password.body'} />
<form onSubmit={handleSubmit(onSubmit)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

same here -- maybe we make this revocable too

@mrnerdhair
Copy link
Contributor

mrnerdhair commented Nov 17, 2021 •
edited
Loading

The missing TextEncoder / TextDecoder issue in CI should be fixed in hdwallet 1.18.1-alpha.5

@mrnerdhair
Copy link
Contributor

Correction -- part of the issue is fixed in 1.18.1-alpha.5. The remaining part is that the jose library, which we use for crypto things, doesn't have dependency injection for TextEncoder and TextDecoder -- and why should it, they're part of the spec. So I've copied over the patch-package patch used to work around the issue in hdwallet. Hopefully somebody upstream (jestjs/jest#9983, jsdom/jsdom#2524) notices sooner or later and solves the issue at the source.

@0xdef1cafe
Copy link
Collaborator

Screen.Recording.2021-11-17.at.9.16.59.AM.mov

after a page refresh

@notatooyou
Copy link

Lots of things look good about it. One issue is that on the second switch wallet I get a white screen of death. I'll look at the console next time. But I load the site, connect Native - good, then connect MM - good, then switch to Native - blows up. Also load site, connect KK - good, connect MM - good, switch to KK - blows up. It always blows up on the second time you switch wallets.

Here is what is in Console that may help troubleshoot:
Screen Shot 2021-11-17 at 10 07 53 AM

@cjthompson
Copy link
Contributor Author

Screen.Recording.2021-11-17.at.9.16.59.AM.mov
after a page refresh

Fixed

@cjthompson @kaladinlight
chore: fix translation error
8814227 
Co-authored-by: kevin <35275952+kaladinlight@users.noreply.github.com>
@cjthompson cjthompson merged commit 76a762a into develop Nov 17, 2021
@cjthompson cjthompson deleted the refactor-native branch November 17, 2021 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaladinlight kaladinlight kaladinlight left review comments

@mrnerdhair mrnerdhair mrnerdhair approved these changes

@0xdef1cafe 0xdef1cafe 0xdef1cafe approved these changes

@theoboldfrazier theoboldfrazier theoboldfrazier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't auto-pair Native wallet Native Wallet Encryption
7 participants
@cjthompson @mrnerdhair @0xdef1cafe @notatooyou @kaladinlight @theoboldfrazier @majorhayes