Remove private key signers if they are replaced by accounts from HD wallet #3377
Conversation
…allet To avoid storing duplicated key material let's remove duplicated accounts, HD wallet accounts should have higher priority than accounts imported with single private key.
| @@ -603,13 +614,14 @@ export default class InternalSignerService extends BaseService<Events> { | |||
| */ | |||
| async hideAccount(address: HexString): Promise<void> { | |||
There was a problem hiding this comment.
Non-blocking: it occurs to me that we should probably rename this to removeAccount, and clarify in the docs that for accounts that are derived from a keyring, this will only hide the associated address unless it's the last derived address from the keyring.
| if (isKeyring(signerWithType)) { | ||
| const { signer } = signerWithType | ||
| const keyringAddresses = await signer.getAddresses() | ||
| if (!keyringSigner && !privateKeySigner) return |
There was a problem hiding this comment.
Let's compare this against undefined instead of type coercion, and do the same in the below if statements. Especially when we're dealing with binary data and security-sensitive code, the wobbliness of type coercion is best avoided.
There was a problem hiding this comment.
Alright but we have to compare against null as this is what #findKeyring and #findPrivateKey will return in case signer is missing
There was a problem hiding this comment.
All set here. I tested to make sure that adding a private key, then its associated keyring, will remove the private key from storage once the corresponding address is derived. I also tested to make sure adding a private key after the address is already owned by the keyring does not add the private key.
The only outstanding thing is that if you have a private key, then you add the associated address via a keyring, the address associated with the private key becomes the first address under the keyring in the UI. Not a huge deal, pretty corner case, and not security critical.
## 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).
Improvement mentioned in the RFB 4: One-Off Keyring Design
What
To avoid storing duplicated key material let's remove duplicated accounts, HD wallet accounts should have higher priority than accounts imported with single private key.
Testing
This was already working as intended from a user perspective but we were leaving "orphaned" duplicated accounts in the service
Latest build: extension-builds-3377 (as of Fri, 12 May 2023 11:21:11 GMT).