🗝 QA - Accounts backup and private key import #3266

jagodarybacka · 2023-04-11T10:32:25Z

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
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

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

Importing accounts with JSON files

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

Functionality of account imported with private key

should be able to fetch account balance and NFTs on all built-in networks
should be able to sign transactions
should be able to perform personal sign
should be able to sign Taho pledge

Functionality of account imported with JSON file

should be able to fetch account balance and NFTs on all built-in networks
should be able to sign transactions
should be able to perform personal sign
should be able to sign Taho pledge

Accounts backup

These are the same steps as here: #3252

Exporting private key for account imported with text private key

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

Exporting private key for account imported with 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

Exporting private key for specific accounts from wallet imported with mnemonic

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

Exporting mnemonics for wallet imported with mnemonic

import wallet with the mnemonic as usual
go through the export flow
compare mnemonics

Exporting mnemonics for wallet generated in-wallet

generate wallet as usual
go through the export flow
compare mnemonics

Export vs Ledger and read-only accounts

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

Other export tests

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

Latest build: extension-builds-3266 (as of Tue, 09 May 2023 13:19:32 GMT).

VladUXUI · 2023-04-11T13:41:47Z

Editing name of Private Key label isn't working.

Let's remove the settings icon entirely. As these will have accounts from more accounts and places

VladUXUI · 2023-04-11T13:49:27Z

Can the accordion and accordion visible have the same width? so we don't have to use overflow:visible?
Because of that, the expanding action isn't as smooth.

Not so visible in this gif:D but try it out

VladUXUI · 2023-04-11T13:56:03Z

It it possible to change the mnenonic_container size based on content?
If it's 12 word seed to show as 240px instead of 370px?

VladUXUI · 2023-04-11T14:15:31Z

Let's change button small tertiary grey from green-60 to green-20, and mouse over from 40 to 20.
This is my bad for not fixing when we changed colours.
This is a global change.

VladUXUI · 2023-04-11T14:24:24Z

If i change the input in a field after it received an error, the error should cleared out

### What Implemented UI changes requested in: #3266 ### Testing - test if changes requested in linked PR were applied correctly Latest build: [extension-builds-3270](https://github.com/tahowallet/extension/suites/12183072896/artifacts/643338009) (as of Wed, 12 Apr 2023 12:00:49 GMT).

michalinacienciala · 2023-04-17T10:56:51Z

First round of comments from my QA-ing

Can we change the copy in the What is a private key?
Currently at the beginning we have:
"A private key is also known as a secret key is a string of letter and numbers that allow you to access and manage your assets on one address."
I would suggest something like this:
"A private key (also known as a secret key) is a string of letter and numbers that allow you to access and manage your assets on one address."
Also, can we change copy in the Why do i need a recovery phrase??
Currently it says:
"You need a recovery phrase in case you loose access to your computer, wallet or you forget your password.
Recovery phrase is the only way to regain access to your funds stored on those addresses.
Taho has the option to import or create multiple recovery phrases. If you have multiple, make sure that you have safely save and store them all."
We say "those addresses", but no addresses were mentioned earlier
The last sentence is not grammatically correct. We should either change "save" -> "saved", "store" -> "stored", or remove "have".
Do we plan to update https://tahowallet.notion.site/Recovery-Phrases-Private-Keys-31274e1abd2e4055aa63dae5297828b3 once the accounts backup goes live? That's where Read more on What is a private key / recovery phrase pop-up links to.
The Unlock signing screen looks different than on Figma designs. Is that intended?
Figma:

Extension:

jagodarybacka · 2023-04-17T11:20:17Z

Can we change the copy in the What is a private key?
Can we change copy in the Why do i need a recovery phrase?

If I remember correctly these were just a first version so we can adjust them if needed

Do we plan to update Notion

Yes I think so but it will be updated just before the release or so cc @Naxsun

The Unlock signing screen looks different

We agreed to use the existing password screen

cc @VladUXUI

michalinacienciala · 2023-04-17T13:58:38Z

Bug #3276 (Can't correctly import account using recovery phrase when it was earlier imported using private key and removed) reported.

michalinacienciala · 2023-04-18T06:50:32Z

Some more findings:

When I tried to import .json file with JSON content which did not contain the fields needed for key decryption (it was just some random, non-crypto related json file), I saw Wrong password, make sure you enter the password you used when encrypting this file. I think we should either detect that the file does not follow expected format (probably harder, as may require defining JSON Schema for Web3 Secret Storage Definition Standard) or change the error message to something like Wrong password or incorrect file structure, make sure you enter the password you used when encrypting this file and that the file is not corrupted.
In the Import private key window, once file gets successfully decrypted we still show Private key | JSON tabs. Is that intended?
I know it's on the designs and it can be used to add several accounts in a row, but for adding several accounts I think a button like Add another account would be more user-friendly. I'm not saying we need such button, but maybe we should remove the Private key | JSON tabs from the success screen?

jagodarybacka · 2023-04-18T10:24:01Z

Added issue to improve it Better error message for JSON import #3279
@VladUXUI can you respond to that?

VladUXUI · 2023-04-18T10:49:55Z

@michalinacienciala, why do you think the tabs shouldn’t be there anymore?

michalinacienciala · 2023-04-20T09:08:18Z

@michalinacienciala, why do you think the tabs shouldn’t be there anymore?

@VladUXUI, I'm not saying the tabs certainly must go, I was just wondering if they are there for some reason. When I was going through the import and got to the Success screen the clickable tabs at this stage looked a bit strange to me from the UX perspective - it felt a bit to me that they're leftover artifacts from the previous stage. As a user I wouldn't expect them to still be there clickable when the key got already imported (the key got imported - why would I want to go back to a different tab?).

I certainly don't think that how this currently works needs to be changed in order for the functionality to be QA approved. But if, for example, disabling the clickability of the tabs would be a small change, maybe it would be worth to add it? Unless, of course, the clickability is there for a reason.

beemeeupnow · 2023-04-28T17:41:37Z

Testing that did not work marked with FAILED
Testing that worked but had UX issue marked with UX
Feedback and results will be in the last section of this update. (along with explanations for failed/UX items)

General regression testing

Completed in this order

installed test extension
create fresh wallet on extension installation
add another address for that wallet and confirm changing to it works
create another fresh wallet after that via "Add Wallet"
use "Copy Address" from the list of accounts for at least the first and last one listed
import existing wallet via "Add Wallet" (24-word mnemonic)
import the address from last import as a Read-Only address (should not break or change to read-only)

testing order I performed after that, in order

begin JSON testing

open the Add Wallet screen and select Private Key, go to JSON tab
test drag and drop, then click the X on the listed file to remove it
test "Browse files", then click the X on the listed file to remove it
test bad file extensions (e.g. "jpg", "txt")
test invalid JSON (missing "version" key)
remove file and add correct JSON
test wrong password for JSON
enter correct password, continue to decrypt and hit Ctrl-W to close tab as soon as it shows "Decrypting" (Note: The account showed up, unsure if expected without "Finalize" having been clicked)
UX try importing same JSON again (got an error message instead of "address already imported" or similar)

remove extension and install again

during onboarding, import private key belonging to first address of 12-word mnemonic
FAILED export private key of the imported mnemonic (try with signing locked, then with signing unlocked)
sign web3 pledge with the imported private key address
test signing with private key account at https://dicether.github.io/js-eth-personal-sign-examples/ (eth_sign, typed data v3+v4)
import JSON wallet
sign web3 pledge with imported JSON wallet address
test signing with private key account at https://dicether.github.io/js-eth-personal-sign-examples/ (eth_sign, typed data v3+v4)
remove account for imported private key and import it again
remove account for imported JSON and import it again

remove extension and install again

during onboarding, import JSON key
add Ledger and confirm there is no export option in the menus
import private key of test wallet and confirm funds show in UI

recommended tests from PR description:

Private keys import

Importing accounts with private keys

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

Importing accounts with JSON files

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

Functionality of account imported with private key

should be able to fetch account balance and NFTs on all built-in networks (tested account balance fetches on Ethereum, Optimism, Arbitrum and Polygon)
should be able to sign transactions (not tested today)
should be able to perform personal sign
should be able to sign Taho pledge

Functionality of account imported with JSON file

should be able to fetch account balance and NFTs on all built-in networks (tested account balance fetches on Ethereum, Optimism, Arbitrum and Polygon)
should be able to sign transactions (not tested today)
should be able to perform personal sign
should be able to sign Taho pledge

Accounts backup

These are the same steps as here: #3252

Exporting private key for account imported with text private key

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

Exporting private key for account imported with JSON file

import account with JSON file
FAILED 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

Exporting private key for specific accounts from wallet imported with mnemonic

import wallet with mnemonic as usual
FAILED export private key for the first account from that wallet
FAILED remove wallet and import account using exported private key - it should be the same account

Exporting mnemonics for wallet imported with mnemonic

import wallet with the mnemonic as usual
go through the export flow
compare mnemonics

Exporting mnemonics for wallet generated in-wallet

generate wallet as usual
go through the export flow
compare mnemonics

Export vs Ledger and read-only accounts

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

Other export tests

try removing and adding accounts again - there should be no problem exporting their accounts worked for mnemonic, could not test for private keys
try to export short (12 words) and long (24 words) mnemonics
try to export starting with both locked and unlocked wallet

Feedback and results

last page of onboarding/account import ("done" page) should indicate "It is safe to close this tab"
Alt-T shortcut was not working. I disabled the existing Taho extension before loading this one and tried restarting the browser.
The menu for the bottom account is obscured when there are enough to fill the page. Is there a way to make this auto-scroll so that the full menu is visible?
(show image Address_Menu_hidden.png)
Failed to export private key of address imported by mnemonic. It kept asking for unlock password, then would go back to the main Wallet view.
UX when trying to import the same private key twice (or PK of existing mnemonic address): There should be some message clearly indicating the "problem" or result. As-is there is just no UI change when you click "Import account"
When trying to import private key of an address belonging to a mnemonic (e.g. second, third) that had been added then removed in the wallet UI, there is no UI change or error message. (e.g. could un-hide the account or something)
further to the above point, even after removing all addresses for the mnemonic you are still unable to import any previously-used address via private key (no UI change or error message), but if you then add the associated address as Read-only they magically show up in the Private Key section!
after getting into the odd state mentioned above by adding the address as "Read-only" and it then showing under the Private Key section, unable to show the private key for that account/address
When trying to import the same JSON twice, it shows "Wrong password or incorrect file" error instead of indicating that the account already exists.

jagodarybacka · 2023-05-01T10:56:38Z

last page of onboarding/account import ("done" page) should indicate "It is safe to close this tab"

@VladUXUI what do you think?

Alt-T shortcut was not working

Already ticketed #3139

The menu for the bottom account is obscured when there are enough to fill the page.

@VladUXUI how would we like to handle it?

Failed to export private key of address imported by mnemonic
UX when trying to import the same private key twice
...

and other issues related to adding the same account more than one time with the same method or different:

this should be fixed now, we will just silently switch to the account if it was already imported. There is an "upgrade" path so account will be listed under the correct section: read-only -> private key -> mnemonic so for example if you have an account loaded with private key and you will add a mnemonic that includes that account then it should be listed under given mnemonic wallet's section

@michalinacienciala please retest these scenarios

michalinacienciala · 2023-05-01T13:14:16Z

I've started retesting.
I was able to reproduce this problem on the latest build:

Failed to export private key of address imported by mnemonic. It kept asking for unlock password, then would go back to the main Wallet view.

It occurs for me only when I press Enter on the Unlock screen. When I click 'Unlock' button manually, I'm correctly transferred to the next stage of exporting pk. @beemeeupnow, I assume you also used Enter key on the Unlock view?

beemeeupnow · 2023-05-01T13:58:59Z

I've started retesting. I was able to reproduce this problem on the latest build:

Failed to export private key of address imported by mnemonic. It kept asking for unlock password, then would go back to the main Wallet view.

It occurs for me only when I press Enter on the Unlock screen. When I click 'Unlock' button manually, I'm correctly transferred to the next stage of exporting pk. @beemeeupnow, I assume you also used Enter key on the Unlock view?

Yes, I always press the Enter key there. I'm glad you identified that!

michalinacienciala · 2023-05-01T15:59:21Z

Another thing I noticed during retests is a bit inconsistent behavior when it comes to assigning second/third/etc addresses to the account imported as a mnemonic.

Let's say we have:
address_1
address_2
and they are both associated with mnemonic_1

If we import mnemonic_1 and and import private key belonging to address_2 then address_2 gets added under Private Key account (to make it appear under mnemonic_1, we must click Add address in Import 1). So in this case we did not automatically recognize that address_2 belongs to account_1.

If we import mnemonic_1 and then click Add address - the address_2 gets assigned to the account with mnemonic_1. Then if we remove the address_2 and import private key belonging to address_2 then address_2 gets added to the account associated with the mnemonic_1. So in this case we did automatically recognize that address_2 belongs to account_1.

Is that something intended?
I assume it happens because in the second case we remember that the address_2 was belonging to mnemonic_1.

jagodarybacka · 2023-05-01T16:14:58Z

@michalinacienciala so this is happening because of how we are deriving addresses from mnemonics.

By default we are only deriving one address. So the application is unaware of more accounts than needed (and their secrets). That allows you to import a second address with a private key and it is not recognized by default as a mnemonic member.
You have to manually derive that address by clicking "Add address".

I would say this behavior is fine. It is "safer" on the technical level because we are not making the wallet aware of accounts that are not being used by the user. Unless the user will fully remove the mnemonic it can add (once derived, added and then removed - in fact hidden) accounts again with any method and recognize them as belonging to the mnemonic. That was the behavior we had for a long time so I would hesitate to change it.

As long as accounts are fully functional regardless if they are listed under the mnemonic or private key section this shouldn't bother our users.

@beemeeupnow thanks, will try to make the keyboard usable there :)

michalinacienciala · 2023-05-01T16:30:33Z

@jagodarybacka, thanks for explaining 👍
I have retested all the tests marked by Beem as untested or failed (and did some other tests relating to importing same accounts with different methods). Apart of this issue with Enter on the unlock screen and UX issues discussed earlier I haven't found any problems.

### What Changes requested during PR review #3089 Highlights: - massive rename from `keyring` to `internal signer` - cleanups and adding doc comments - fixing and improving types - fixing minor bugs Please check commits to get a better understanding of the scope. This is already merged to the QA branch #3266 so QA will be done on these changes as well. Latest build: [extension-builds-3331](https://github.com/tahowallet/extension/suites/12618917536/artifacts/675458826) (as of Tue, 02 May 2023 13:11:00 GMT).

Shadowfiend

Let's land this fellow!

@jagodarybacka

## 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).

SUPPORT_PRIV_KEYS flag flip

4014026

jagodarybacka mentioned this pull request Apr 12, 2023

Account backup UI fixes #3270

Merged

Merge branch 'show-private-key-menu' into import-export-secrets-qa

66e2c27

jagodarybacka mentioned this pull request Apr 18, 2023

Better error message for JSON import #3279

Closed

Merge branch 'show-private-key-menu' into import-export-secrets-qa

508d5d4

chlolands self-requested a review April 20, 2023 20:40

Merge branch 'show-private-key-menu' into import-export-secrets-qa

5c88654

Base automatically changed from show-private-key-menu to keyring-with-pk April 27, 2023 12:21

hyphenized and others added 2 commits April 27, 2023 14:22

Merge branch 'keyring-with-pk' into import-export-secrets-qa

61a3574

Merge branch 'keyring-rename' into import-export-secrets-qa

9ef1684

jagodarybacka changed the base branch from keyring-with-pk to keyring-rename April 28, 2023 11:04

jagodarybacka mentioned this pull request Apr 28, 2023

Rename keyring to internal signer and other improvements #3331

Merged

jagodarybacka added 2 commits May 1, 2023 10:53

Merge branch 'keyring-rename' into import-export-secrets-qa

0d37c7e

Merge branch 'keyring-rename' into import-export-secrets-qa

210908a

jagodarybacka force-pushed the import-export-secrets-qa branch from 3c888c3 to 210908a Compare May 1, 2023 10:39

Merge branch 'keyring-rename' into import-export-secrets-qa

ce39c70

Base automatically changed from keyring-rename to keyring-with-pk May 2, 2023 14:16

Merge branch 'keyring-with-pk' into import-export-secrets-qa

3c60a13

jagodarybacka added this to the Private Key Import (Audit readiness) milestone May 9, 2023

jagodarybacka self-assigned this May 9, 2023

jagodarybacka marked this pull request as ready for review May 9, 2023 15:48

jagodarybacka requested review from Shadowfiend, kkosiorowska and hyphenized and removed request for chlolands May 9, 2023 15:49

Shadowfiend approved these changes May 9, 2023

View reviewed changes

Shadowfiend merged commit 2b52afc into keyring-with-pk May 9, 2023

Shadowfiend deleted the import-export-secrets-qa branch May 9, 2023 15:50

kkosiorowska mentioned this pull request Jul 13, 2023

v0.42.0 #3549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🗝 QA - Accounts backup and private key import #3266

🗝 QA - Accounts backup and private key import #3266

jagodarybacka commented Apr 11, 2023 •

edited by github-actions bot

Loading

VladUXUI commented Apr 11, 2023 •

edited

Loading

VladUXUI commented Apr 11, 2023 •

edited

Loading

VladUXUI commented Apr 11, 2023

VladUXUI commented Apr 11, 2023

VladUXUI commented Apr 11, 2023

michalinacienciala commented Apr 17, 2023 •

edited

Loading

jagodarybacka commented Apr 17, 2023

michalinacienciala commented Apr 17, 2023 •

edited

Loading

michalinacienciala commented Apr 18, 2023 •

edited

Loading

jagodarybacka commented Apr 18, 2023

VladUXUI commented Apr 18, 2023

michalinacienciala commented Apr 20, 2023

beemeeupnow commented Apr 28, 2023 •

edited

Loading

jagodarybacka commented May 1, 2023

michalinacienciala commented May 1, 2023

beemeeupnow commented May 1, 2023

michalinacienciala commented May 1, 2023

jagodarybacka commented May 1, 2023

michalinacienciala commented May 1, 2023

Shadowfiend left a comment

🗝 QA - Accounts backup and private key import #3266

🗝 QA - Accounts backup and private key import #3266

Conversation

jagodarybacka commented Apr 11, 2023 • edited by github-actions bot Loading

Testing checklist

Private keys import

Accounts backup

VladUXUI commented Apr 11, 2023 • edited Loading

VladUXUI commented Apr 11, 2023 • edited Loading

VladUXUI commented Apr 11, 2023

VladUXUI commented Apr 11, 2023

VladUXUI commented Apr 11, 2023

michalinacienciala commented Apr 17, 2023 • edited Loading

jagodarybacka commented Apr 17, 2023

michalinacienciala commented Apr 17, 2023 • edited Loading

michalinacienciala commented Apr 18, 2023 • edited Loading

jagodarybacka commented Apr 18, 2023

VladUXUI commented Apr 18, 2023

michalinacienciala commented Apr 20, 2023

beemeeupnow commented Apr 28, 2023 • edited Loading

General regression testing

testing order I performed after that, in order

recommended tests from PR description:

Private keys import

Accounts backup

Feedback and results

jagodarybacka commented May 1, 2023

michalinacienciala commented May 1, 2023

beemeeupnow commented May 1, 2023

michalinacienciala commented May 1, 2023

jagodarybacka commented May 1, 2023

michalinacienciala commented May 1, 2023

Shadowfiend left a comment

Choose a reason for hiding this comment

jagodarybacka commented Apr 11, 2023 •

edited by github-actions bot

Loading

VladUXUI commented Apr 11, 2023 •

edited

Loading

VladUXUI commented Apr 11, 2023 •

edited

Loading

michalinacienciala commented Apr 17, 2023 •

edited

Loading

michalinacienciala commented Apr 17, 2023 •

edited

Loading

michalinacienciala commented Apr 18, 2023 •

edited

Loading

beemeeupnow commented Apr 28, 2023 •

edited

Loading