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

📚 Accounts backup #3252

Merged
merged 75 commits into from
Apr 27, 2023
Merged

📚 Accounts backup #3252

merged 75 commits into from
Apr 27, 2023

Conversation

kkosiorowska
Copy link
Contributor

@kkosiorowska kkosiorowska commented Apr 5, 2023

This PR implements the complete functionality of accounts backup:

  • it allows exporting private keys for accounts imported with a private key or a JSON keystore file
  • it allows exporting mnemonics of HD wallets

What has been done?

  • add export flow for private keys
  • add export flow for mnemonics

Outside of scope of this PR

Testing

  1. Export private keys - plain text
  • import account with a private key (plain text)
  • after it is imported - go through the export flow
  • compare exported private key with the private key you used for import
  1. Export private keys - JSON file
  • import account with JSON file
  • after it is imported go through the export flow
  • copy exported private key, note the address of this account and remove it
  • import account with copied private key, compare addresses - it should be the same account
  1. Export mnemonics
  • import wallet with the mnemonic as usual
  • go through the export flow
  • compare mnemonics
  1. Other - Ledger, read-only
  • there should be no option to export any secrets for accounts imported with Ledger
  • there should be no option to export any secrets for read-only accounts
  1. Exploratory testing
  • try removing and adding accounts again - there should be no problem exporting their accounts
  • try to export short (12 words) and long (24 words) mnemonics
  • try to export starting with both locked and unlocked wallet

UI

Screenshot 2023-04-05 at 08 34 05image

imageimage

Latest build: extension-builds-3252 (as of Thu, 27 Apr 2023 12:04:59 GMT).

@kkosiorowska kkosiorowska added this to the Backup Accounts milestone Apr 5, 2023
@kkosiorowska kkosiorowska self-assigned this Apr 5, 2023
jagodarybacka and others added 17 commits April 5, 2023 15:55
* add an invalid message for the checkbox
* hide events for the disabled button if is needed
### What
- Create a new component that will wrap a given text or other React
component and will blur it unless user if hovering their mouse over it.
- Add Secret Text component to private key export slideup, add copy
button


![image](https://user-images.githubusercontent.com/20949277/230107469-a0cd420d-8eee-4119-b2e4-ca100aba3d36.png)

### Testing

- [ ] import account with private key
- [ ] export private key from the menu - check pk visible on the screen
and test "copy" button

Latest build:
[extension-builds-3255](https://github.com/tahowallet/extension/suites/12064461721/artifacts/634749987)
(as of Thu, 06 Apr 2023 06:25:22 GMT).
This reverts commit 45a55f8.
Let's make it obvious which kind of keys import was used for
a given signer.
- add menu option to export mnemonic
- add unstyled panel content
This PR adds a wallet-unlocking step. The screen should appear whenever
the user wants to open the slide-up menu for `Show Private Key`.



https://user-images.githubusercontent.com/23117945/230336203-550e0798-10b1-419c-bcd1-0e42ad81f59f.mov

### To Test
- [x] Check if the screen shows when the wallet is locked
- [x] Check if the screen shows when the wallet is unlocked
- [x] Check if you are able to unlock the screen with the click of a
button

Latest build:
[extension-builds-3257](https://github.com/tahowallet/extension/suites/12068023649/artifacts/634995263)
(as of Thu, 06 Apr 2023 09:40:41 GMT).
- move ShowMnemonic and ShowPrivateKey slideups to separate folder
- create Explainer component to reuse explainer's styles
Copy link
Contributor

@hyphenized hyphenized left a comment

Choose a reason for hiding this comment

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

Looks good! Left a few comments, mostly non-blocking. Found an edge case after adding/removing accounts that prevented me from using the wallet though will need to check how to replicate the issue 😅 .

ui/_locales/en/messages.json Outdated Show resolved Hide resolved
ui/components/AccountItem/AccountItemOptionsMenu.tsx Outdated Show resolved Hide resolved

useEffect(() => {
const lockWallet = async () => {
await dispatch(lockKeyrings())
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this, here we're locking signing for the entire wallet where I think this should only separate access to the exporting functionality. In general when locking access we can create a wrapper component that holds the "auth" state instead of using effects to lock on mount. The result is also a bit cleaner because it ensures all routes under it are protected. We can tweak this in a follow up PR.

Copy link
Contributor

@jagodarybacka jagodarybacka Apr 19, 2023

Choose a reason for hiding this comment

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

Tbh I like the approach of locking the whole wallet, creating more complex logic of when and where the wallet is locked would be more prone to errors than having one straightforward path of either having the whole wallet locked or unlocked. Technically signing and exporting keys is happening in the same place and on the same data so it's much safer to not tweak that functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's that big of an issue as it only locks the users out again if they exit before finish exporting their secrets.

In theory, we would only need to expose a method which we would use to essentially authenticate, and then keep this "logged in" status in popup-only state so it "locks" if the user hides the wallet or the wrapper component is unmounted, but I might be missing something here 😅 .

const lockWallet = async () => {
await dispatch(lockKeyrings())
// No need to show that signing got locked
dispatch(clearSnackbarMessage())
Copy link
Contributor

Choose a reason for hiding this comment

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

We should refactor the signing locked toast outside of this effect so we can avoid having to do these forced clean ups. Not blocking.

Copy link
Contributor

Choose a reason for hiding this comment

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

No matter if the parent component was mounted or not it's nice to have an option to silently lock a wallet. I agree it may not be perfect but it was the simplest solution without adding much code as it is unlikely I think that we will be using this hook often.

ui/components/Shared/SharedCheckbox.tsx Outdated Show resolved Hide resolved
}}
center
>
{t("exportingPrivateKey.copyBtn")}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think as a user I'd expect to be able to copy after clicking the revealed text as well, just a thought.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be harder to do, since you would only copy 1 column of the text. I think having 1 button to copy it all should be enough

ui/components/AccountsBackup/RevealMnemonic.tsx Outdated Show resolved Hide resolved
const { mnemonic, startIndex, endIndex } = props
if (!mnemonic.length) return <></>

const splitMnemonic = mnemonic.split(" ").slice(startIndex, endIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Albeit more secure, it's a bit annoying to have to hover each half of the mnemonic to see it.

ui/components/AccountsBackup/ShowMnemonic.tsx Outdated Show resolved Hide resolved
ui/components/AccountsBackup/ShowPrivateKey.tsx Outdated Show resolved Hide resolved
@hyphenized
Copy link
Contributor

hyphenized commented Apr 18, 2023

Found the bug, if you remove a private key and then attempt to import the seed phrase for that account, you'll end up with a read only account

readonly.mov

Removing this account and attempting to import the seed phrase again breaks onboarding for that account

wheresmywallet.mov

Fixed in #3282, already tracked in #3276

Copy link
Contributor

@hyphenized hyphenized left a comment

Choose a reason for hiding this comment

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

All good, excellent work!

Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

### What

Let's allow exporting private keys from HD wallet addresses.

### Testing

- [x] add HD wallet with the mnemonic
- [x] export private key for account from HD wallet
- [x] export the same private key with metamask and compare them


![image](https://user-images.githubusercontent.com/20949277/231476213-bc482747-a91d-4a18-9eed-21254db91e0c.png)


Latest build:
[extension-builds-3253](https://github.com/tahowallet/extension/suites/12348519892/artifacts/655435412)
(as of Wed, 19 Apr 2023 17:38:25 GMT).
@kkosiorowska kkosiorowska requested a review from a team as a code owner April 21, 2023 13:01
@kkosiorowska
Copy link
Contributor Author

Let's 🚀

@kkosiorowska kkosiorowska merged commit 66e54ce into keyring-with-pk Apr 27, 2023
@kkosiorowska kkosiorowska deleted the show-private-key-menu branch April 27, 2023 12:21
Shadowfiend added a commit that referenced this pull request May 9, 2023
## Testing checklist

Useful tips:
- to get the private key to test import feature you can first export
private key from account imported with mnemonic
- to get JSON wallet please generate one on [this
website](https://www.myetherwallet.com/wallet/create/software?type=keystore)
- to test transactions, NFTs etc please use your test wallet and send
small amounts of base asset or cheap NFTs to the wallet you are testing.
You can use network with cheap gas like Polygon. To send NFTs please use
Opensea transfer feature

### Private keys import

Importing accounts with private keys

- [x] import with pk as a first account after extension install
- [x] import with pk as a second acocunt (first imported with other
method)
- [x] import account two times with the same pk
- [x] remove account imported with pk and add it once again
- [x] enter incorrect pk

Importing accounts with JSON files

- [x] import with JSON as a first account after extension install
- [x] import with JSON as a second acocunt (first imported with other
method)
- [x] import account two times with the same JSON file
- [x] interrupt decrypting account by clicking back button or closing
the page
- [x] remove account imported with JSON and add it once again
- [x] enter incorrect password
- [x] enter incorrect file - different file extension or just any other
JSON

Functionality of account imported with private key

- [x] should be able to fetch account balance and NFTs on all built-in
networks
- [x] should be able to sign transactions
- [x] should be able to perform [personal
sign](https://dicether.github.io/js-eth-personal-sign-examples/)
- [x] should be able to sign [Taho pledge](https://taho.xyz/web3pledge)

Functionality of account imported with JSON file

- [x] should be able to fetch account balance and NFTs on all built-in
networks
- [x] should be able to sign transactions
- [x] should be able to perform [personal
sign](https://dicether.github.io/js-eth-personal-sign-examples/)
- [x] should be able to sign [Taho pledge](https://taho.xyz/web3pledge)

### Accounts backup

These are the same steps as here: #3252

Exporting private key for account imported with text private key

- [x] import account with a private key (plain text)
- [x] after it is imported - go through the export flow
- [x] compare exported private key with the private key you used for
import

Exporting private key for account imported with JSON file

- [x] import account with JSON file
- [x] after it is imported go through the export flow
- [x] copy exported private key, note the address of this account and
remove it
- [x] import account with copied private key, compare addresses - it
should be the same account

Exporting private key for specific accounts from wallet imported with
mnemonic

- [x] import wallet with mnemonic as usual
- [x] export private key for the first account from that wallet
- [x] remove wallet and import account using exported private key - it
should be the same account
 
Exporting mnemonics for wallet imported with mnemonic

- [x] import wallet with the mnemonic as usual
- [x] go through the export flow
- [x] compare mnemonics

Exporting mnemonics for wallet generated in-wallet

- [x] generate wallet as usual
- [x] go through the export flow
- [x] compare mnemonics

Export vs Ledger and read-only accounts

- [x] there should be no option to export any secrets for accounts
imported with Ledger
- [x] there should be no option to export any secrets for read-only
accounts

Other export tests

- [x] try removing and adding accounts again - there should be no
problem exporting their accounts
- [x] try to export short (12 words) and long (24 words) mnemonics 
- [x] try to export starting with both locked and unlocked wallet

Latest build:
[extension-builds-3266](https://github.com/tahowallet/extension/suites/12773303188/artifacts/687001866)
(as of Tue, 09 May 2023 13:19:32 GMT).
Shadowfiend added a commit that referenced this pull request Jul 7, 2023
### About this PR
This PR will contain all changes for features:
- [x] allow importing private keys in the keyring service (initial scope
of this PR)
- [x] import private keys with plain text
(#3119)
- [x] import private keys with JSON
(#3177)
- [x] backup account with mnemonic and private key export
(#3252)
---

## Initial scope 
Resolves #3070

### What

Handle importing wallets with private keys in the keyring service:
* save pk imported accounts in the encrypted vault
* handle serialization and deserialization of pk imported wallets
* handle signing messages with both keyrings and pk wallets
* handle communication between service and redux store about pk wallets
* fix types and field names in the redux store

Cleanup and add more tests:
- move existing keyring integration tests to the right folder
- write more unit tests to cover private key imported wallets
- cleanup some duplicated tests
- add util functions to the test's `factories`
- add function to mock local storage

### Testing
- make sure unit tests and E2E are passing
- test manually adding accounts and signing transactions

Latest build:
[extension-builds-3089](https://github.com/tahowallet/extension/suites/14105826655/artifacts/789186059)
(as of Thu, 06 Jul 2023 12:56:07 GMT).
@kkosiorowska kkosiorowska mentioned this pull request Jul 13, 2023
kkosiorowska pushed a commit that referenced this pull request Jul 14, 2023
## What's Changed
* Add private key onboarding flow by @jagodarybacka in
#3119
* Private key JSON import by @jagodarybacka in
#3177
* Allow export of private keys and mnemonics by @jagodarybacka in
#3248
* Export private key form by @jagodarybacka in
#3255
* Unlock screen for the account backup by @kkosiorowska in
#3257
* Show mnemonic menu by @jagodarybacka in
#3259
* Fix background blur issue by @jagodarybacka in
#3265
* Account backup UI fixes by @jagodarybacka in
#3270
* Fix unhiding removed accounts by @jagodarybacka in
#3282
* New error for incorrectly decrypted JSON file by @jagodarybacka in
#3293
* Export private keys from HD wallet addresses by @jagodarybacka in
#3253
* Refactor keyring redux slice to remove `importing` field by
@jagodarybacka in #3309
* 📚 Accounts backup by @kkosiorowska in
#3252
* Catch Enter keypress on Unlock screen by @jagodarybacka in
#3355
* Rename `keyring` to `internal signer` and other improvements by
@jagodarybacka in #3331
* 🗝 QA - Accounts backup and private key import by @jagodarybacka in
#3266
* Remove private key signers if they are replaced by accounts from HD
wallet by @jagodarybacka in
#3377
* RFB 4: One-Off Keyring Design by @Shadowfiend in
#3372
* Copy to clipboard warning by @kkosiorowska in
#3488
* Allow setting custom auto-lock timer by @hyphenized in
#3477
* Use Argon2 for encrypted vaults by @jagodarybacka in
#3502
* 👑 Private keys import and accounts backup by @jagodarybacka in
#3089
* Untrusted assets should not block the addition of custom tokens by
@kkosiorowska in #3491
* Flip updated dApp connections flag by @Shadowfiend in
#3492
* v0.41.0 by @Shadowfiend in
#3531
* Switch to a given network if adding a network that is already added.
by @0xDaedalus in #3154
* Remove waiting for Loading Doggo component in E2E tests by
@jagodarybacka in #3541
* Squeeze content to better fit on Swaps page by @jagodarybacka in
#3542
* Refactor of terms for verified/unverified assets by @kkosiorowska in
#3528
* Fix ChainList styling by @fulldecent in
#3547
* Update release checklist by @jagodarybacka in
#3548
* Fix custom asset price fetching by @hyphenized in
#3508
* Sticky Defaults: Make Taho-as-default replace MetaMask in almost all
cases by @Shadowfiend in
#3546

## New Contributors
* @fulldecent made their first contribution in
#3547

**Full Changelog**:
v0.41.0...v0.42.0

Latest build:
[extension-builds-3549](https://github.com/tahowallet/extension/suites/14268975651/artifacts/801826435)
(as of Thu, 13 Jul 2023 09:51:56 GMT).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment