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

Refactor keyring redux slice to remove importing field #3309

Merged
merged 4 commits into from
Apr 27, 2023
Merged

Refactor keyring redux slice to remove importing field #3309

merged 4 commits into from
Apr 27, 2023

Conversation

jagodarybacka
Copy link
Contributor

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

Implementation of changes requested in #3089 (comment)

What

Let's remove the importing field from the redux keyring slice and handle waiting for new signer import with promises.

Testing

Everything should work as before:

  • import account with mnemonic
  • import private key
  • import private key with JSON
  • generate new wallet
  • migration from main

Latest build: extension-builds-3309 (as of Thu, 27 Apr 2023 11:38:39 GMT).

@jagodarybacka jagodarybacka self-assigned this Apr 25, 2023
@jagodarybacka jagodarybacka marked this pull request as ready for review April 26, 2023 09:45
@jagodarybacka jagodarybacka mentioned this pull request Apr 26, 2023
Merged
4 tasks
Shadowfiend
Shadowfiend reviewed Apr 26, 2023
View reviewed changes
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Left some small comments but overall looks good. Can't test right now 😔 so not hitting the approval.

background/main.ts Outdated
return this.keyringService.importSigner(signerRaw)
async importSigner(
signerRaw: SignerRawWithType
): Promise<{ success: boolean; errorMessage?: string }> {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we make this an alternate?

Suggested change
): Promise<{ success: boolean; errorMessage?: string }> {
): Promise<{ address: true } | { success: false; errorMessage: string }> {

Basically communicate "you will only see an error message in case of failure".

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree but I think you meant { success: true } | { success: false; errorMessage: string } 🤔

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 wanted to make this type this way but I have a problem making typescript happy about returning something more complicated than a simple object here 🤔 Something weird is going on with types for createBackgroundAsyncThunk, I will take a look later if I will have enough time.

background/main.ts Outdated
} catch (error) {
return {
success: false,
errorMessage: `Unexpected error during account import. Error: ${error}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking we shouldn't bubble an error out unguarded to the UI here to avoid leaking potentially sensitive info now or in the future. Logs should catch the real error.

background/redux-slices/keyrings.ts
address,
network: ui.selectedAccount.network,
})
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw I do feel like the logic in main.importSigner mostly belongs here rather than main--main should always do as little wiring as possible to connect to the services IMO.

hyphenized
hyphenized approved these changes Apr 26, 2023
View reviewed changes
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.

Tested, works well! Will leave open in case you want to address the comments

background/main.ts Outdated
return this.keyringService.importSigner(signerRaw)
async importSigner(
signerRaw: SignerRawWithType
): Promise<{ success: boolean; errorMessage?: string }> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree but I think you meant { success: true } | { success: false; errorMessage: string } 🤔

hyphenized
hyphenized approved these changes Apr 27, 2023
View reviewed changes
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, let's fix the types for async thunks in a separate ticket.

@hyphenized hyphenized merged commit 592ba71 into show-private-key-menu Apr 27, 2023
@hyphenized hyphenized deleted the keyring-importing-refactor branch April 27, 2023 12:01
@hyphenized hyphenized mentioned this pull request Apr 27, 2023
Open
@kkosiorowska kkosiorowska mentioned this pull request Jul 13, 2023
Merged
kkosiorowska pushed a commit that referenced this pull request Jul 14, 2023
v0.42.0 (#3549)
2edd8ea 
## 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
Reviewers

@Shadowfiend Shadowfiend Shadowfiend left review comments

@hyphenized hyphenized hyphenized approved these changes

@kkosiorowska kkosiorowska Awaiting requested review from kkosiorowska

@0xDaedalus 0xDaedalus Awaiting requested review from 0xDaedalus

Assignees

@jagodarybacka jagodarybacka

Labels
None yet
Projects
None yet
Milestone
Private Key Import (Audit readiness)
Development

Successfully merging this pull request may close these issues.
3 participants
@jagodarybacka @Shadowfiend @hyphenized