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

Fix recreate vault import accounts #1756

Merged
merged 9 commits into from
Aug 12, 2020

Conversation

andrepimenta
Copy link
Member

@andrepimenta andrepimenta commented Aug 7, 2020

Description

This fixes imported accounts getting deleted when the vault is recreated

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #1105

@andrepimenta andrepimenta requested a review from a team as a code owner August 7, 2020 18:41
const importedAccounts = [];

// Get imported accounts
if (KeyringController.state.keyrings.length === 2) {
Copy link
Member

Choose a reason for hiding this comment

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

On the extension we have one keyring for each imported account - I am guessing mobile is the same? So this should handle all keyrings from index 1 onwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right! Thanks, fixing this now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now 👍

@estebanmino estebanmino added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Aug 7, 2020
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Few issues

Issue 1: Wallet with 1 imported account produces an error "Error: Invalid password"

Issue 2: Wallet with 2 imported accounts does allow you create password, however, the imported accounts are lost

Here goes my set up/steps:

  1. I built v0.2.19

Screen Shot 2020-08-07 at 6 31 59 PM

  1. I then created 3 wallets (no password set/nor no back up)
    wallet 1 = 1 account with 1 imported account
    wallet 2 = 1 account with 2 imported accounts
    wallet 3 = 3 accounts with 1 imported account

image

  1. I then switched to this branch and ran yarn clean, yarn watch:clean, and yarn start:android

  2. Upon building this branch, after being prompted to create a password these are the results; seen here = https://recordit.co/189ncJf7qX

wallet 1 = upon attempting to create a password it gave me the "Invalid password" error
wallet 2 = it did allow me to create password, however, my imported accounts are no longer there
wallet 3 = same error as wallet 1

Screen Shot 2020-08-07 at 6 52 19 PM

@ibrahimtaveras00 ibrahimtaveras00 added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Aug 7, 2020
@rickycodes
Copy link
Member

Issue 2: Wallet with 2 imported accounts does allow you create password, however, the imported accounts are lost

I believe this is due to what @Gudahtt mentioned here: #1756 (comment)

Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Issue 1 has been fixed 👍 in regards to no longer showing the error on password creation
Issue 2 still happens to both wallets with 1 imported account and wallets with 2 or more imported accounts

@andrepimenta andrepimenta added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Aug 11, 2020
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Fixes look good, QA Passed 👍

@ibrahimtaveras00 ibrahimtaveras00 added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Aug 11, 2020
importedAccounts = [...importedAccounts, ...simpleKeyringAccounts];
}
} catch (e) {
console.warn(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we use the logger here to get the errors in sentry?

await KeyringController.importAccountWithStrategy('privateKey', [importedAccounts[i]]);
}
} catch (e) {
console.warn(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@andrepimenta andrepimenta merged commit 91b4840 into develop Aug 12, 2020
@andrepimenta andrepimenta deleted the bugfix/recreate-vault-import-accounts branch August 12, 2020 17:22
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
* fix recreate vault

* Improve getting simple key pair keys

* Introduce try catch around imported accounts

* Fix for multiple imported accounts

* remove unecessary async await

* Use old password

* use logger on recreate vault
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The imported account is automatically deleted
6 participants