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

feature/sync imported accounts with mobile #8631

Merged
merged 3 commits into from
Jun 26, 2020

Conversation

estebanmino
Copy link
Contributor

The purpose of this PR is to send data of imported accounts to mobile app while doing sync. Related PR on mobile MetaMask/metamask-mobile#1591

@metamaskbot
Copy link
Collaborator

Builds ready [ea1f89f]
Page Load Metrics (594 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318438115
domContentLoaded33776059210551
load33976059410551
domInteractive33775959210551

@metamaskbot
Copy link
Collaborator

Builds ready [16ebe5d]
Page Load Metrics (810 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29132593115
domContentLoaded4618698088139
load4638718108139
domInteractive4618698088139

@Gudahtt
Copy link
Member

Gudahtt commented Jun 24, 2020

It looks like something went wrong when you tried resolving that conflict. Lots of deleted files.

@Gudahtt Gudahtt force-pushed the feature/sync-imported-accounts-with-mobile branch from 16ebe5d to 5dd8bfd Compare June 24, 2020 23:11
@Gudahtt
Copy link
Member

Gudahtt commented Jun 24, 2020

I've rebased the first two commits onto develop, re-resolving the conflicts to avoid the deletions

@metamaskbot
Copy link
Collaborator

Builds ready [5dd8bfd]
Page Load Metrics (701 ± 29 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318748199
domContentLoaded6338617006129
load6348627016129
domInteractive6328606996129

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

It looks like this results in the duplication of all accounts when you have two or more imported accounts (as you discovered and reported on Slack).

This is happening because submitPassword is called as part of the exportAccount action, which is now being called multiple times asynchronously. submitPassword should never be called while another submitPassword call is still resolving, because it will dump and re-add all keyrings. So in this case, if you have two imported accounts, it will re-add all keyrings twice resulting in duplicate accounts.

There are a few different ways I can think of to fix this:

  1. Separate the password validation step from the exportAccount step. We could update the exportAccount action to just export the account, and verify the password separately.
  2. Export accounts one-at-a-time, instead of in parallel.
  3. Add a method to the keyring controller to validate the password without dumping and re-adding all keyrings, and use that here instead.

The simplest solution would be 2), but 1) and 3) are things we should probably do regardless, just to make this faster and less flaky.

@estebanmino
Copy link
Contributor Author

@Gudahtt ok thanks for that context, I'll fix it

@estebanmino
Copy link
Contributor Author

@Gudahtt I created a new action method called exportAccounts to fix this.
I didn't do

  1. because i didn't want to break what's currently working on the extension (that I also don't have context of)
  2. I agree with you

In a sense I did 3. but not at Keyring Controller level, just an action that checks if the password is correct once, after that is validated it goes to get the private keys to the Keychain, following what exportAccount does now.

WDYT?

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Seems like a good solution!

@metamaskbot
Copy link
Collaborator

Builds ready [84816d9]
Page Load Metrics (729 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint34134552713
domContentLoaded376101072711857
load377101172911857
domInteractive376100972711857

@Gudahtt Gudahtt merged commit 73257b1 into develop Jun 26, 2020
@Gudahtt Gudahtt deleted the feature/sync-imported-accounts-with-mobile branch June 26, 2020 01:04
Gudahtt added a commit that referenced this pull request Jun 26, 2020
* origin/develop:
  feature/sync imported accounts with mobile (#8631)
  Fix account order on unconnected account alert (#8863)
Gudahtt added a commit that referenced this pull request Jun 26, 2020
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.

3 participants