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

Export private keys from HD wallet addresses #3253

Merged
merged 10 commits into from
Apr 21, 2023

Conversation

jagodarybacka
Copy link
Contributor

@jagodarybacka jagodarybacka commented Apr 5, 2023

What

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

Testing

  • add HD wallet with the mnemonic
  • export private key for account from HD wallet
  • export the same private key with metamask and compare them

image

Latest build: extension-builds-3253 (as of Wed, 19 Apr 2023 17:38:25 GMT).

@jagodarybacka jagodarybacka added this to the Backup Accounts milestone Apr 5, 2023
@jagodarybacka jagodarybacka requested a review from a team as a code owner April 5, 2023 10:20
@jagodarybacka jagodarybacka self-assigned this Apr 5, 2023
@jagodarybacka jagodarybacka changed the title Export private keys from HD wallet's addresses Export private keys from HD wallet addresses Apr 5, 2023
@jagodarybacka jagodarybacka mentioned this pull request Apr 11, 2023
15 tasks
@@ -141,6 +141,13 @@ describe("Keyring Service", () => {

expect(mnemonic).toBe(HD_WALLET_MOCK.mnemonic)
})
it("should be able to export private key from HD wallet addresses", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

One nit here is that I think it might be prudent to add a describe block here for exportPrivateKey and add this test to it as well as a should require the wallet to be unlocked test - only because the fact that the wallet needs to be unlocked is so important here.

@0xDaedalus
Copy link
Contributor

0xDaedalus commented Apr 11, 2023

I think you can yarn add tahowallet/extension#export-pk-from-hd-wallet if you want to get tests passing and not be blocked by the hd-keyring PR. We'll just need to make sure we change it back before merging keyring-with-pk to main

@jagodarybacka jagodarybacka changed the base branch from keyring-with-pk to show-private-key-menu April 12, 2023 13:35
@jagodarybacka
Copy link
Contributor Author

I think it's fine for tests to fail on gh actions right now, I think it should stay on this branch until we publish another hd-keyring version anyway

@jagodarybacka jagodarybacka requested a review from a team as a code owner April 18, 2023 14:59
@kkosiorowska
Copy link
Contributor

I will take a look at that as well 👀

Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

LGTM ✅

Comment on lines +279 to +280
expect(exportMnemonic()).rejects.toThrowError(errorMessage)
expect(exportPrivateKey()).rejects.toThrowError(errorMessage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking comment
There is no difference. However, I have seen that we already use toThrow.

Suggested change
expect(exportMnemonic()).rejects.toThrowError(errorMessage)
expect(exportPrivateKey()).rejects.toThrowError(errorMessage)
expect(exportMnemonic()).rejects.toThrow(errorMessage)
expect(exportPrivateKey()).rejects.toThrow(errorMessage)

@kkosiorowska kkosiorowska merged commit 0b885b7 into show-private-key-menu Apr 21, 2023
@kkosiorowska kkosiorowska deleted the export-pk-from-hd-wallet branch April 21, 2023 13:01
kkosiorowska pushed a commit that referenced this pull request Apr 27, 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 
- exporting private keys for specific accounts from HD wallets -
#3253

### Testing
1. Export private keys - plain text
  - [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

2. Export private keys - 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
 
3. Export mnemonics
- [x] import wallet with the mnemonic as usual
- [x] go through the export flow
- [x] compare mnemonics

4. Other - Ledger, read-only
- [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

5. Exploratory testing
- [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

## UI

![Screenshot 2023-04-05 at 08 34
05](https://user-images.githubusercontent.com/23117945/230000187-17f1645d-116b-45e9-8482-76de21d6be8d.png)![image](https://user-images.githubusercontent.com/20949277/231096292-18d32627-4af6-4dc5-a575-5a2020f6af4c.png)



![image](https://user-images.githubusercontent.com/20949277/231096872-046f8b86-1d53-4855-b2ca-32032674db75.png)![image](https://user-images.githubusercontent.com/20949277/231096598-38618c15-1492-425a-957d-e2d798994c05.png)

Latest build:
[extension-builds-3252](https://github.com/tahowallet/extension/suites/12523466508/artifacts/668490612)
(as of Thu, 27 Apr 2023 12:04:59 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants