Restore multiple consecutive accounts with balances. #4898

dmvt · 2018-07-27T03:45:33Z

Fixes #654

Solution/Spec

When first restoring MetaMask from a previous seed phrase, in a loop:

Generate an additional account
Check if that new account has a balance
If it does, restart the loop
If it does not, remove that account and end the loop.
Should include a test to demonstrate it works.

Submitting a pull request

When you're done with your project / bugfix / feature and ready to submit a PR, there are a couple guidelines we ask you to follow:

Test it: For any new programmatic functionality, we like unit tests when possible, so if you can keep your code cleanly isolated, please do add a test file to the tests folder.
Add to the CHANGELOG: Help us keep track of all the moving pieces by adding an entry to the CHANGELOG.md with a link to your PR.
Meet the spec: Make sure the PR adds functionality that matches the issue you're closing. This is especially important for bounties: sometimes design or implementation details are included in the conversation, so read carefully!
Close the issue: If this PR closes an open issue, add the line Fixes #$ISSUE_NUMBER.
Keep it simple: Try not to include multiple features in a single PR, and don't make extraneous changes outside the scope of your contribution. All those touched files make things harder to review ;)
PR against develop: Submit your PR against the develop branch. This is where we merge new features so they get some time to receive extra testing before being pushed to master for production. If your PR is a hot-fix that needs to be published urgently, you may submit a PR against the master branch, but this PR will receive tighter scrutiny before merging.
Get reviewed by a core contributor: Make sure you get a :thumbsup, :+1, or LGTM from a user with a Member badge before merging.

dmvt · 2018-07-27T03:47:38Z

Feedback is appreciated. I've got 4 failing tests in MetaMaskController due the 2000ms timeouts. I'd love any suggestions on how to deal with that properly.

dmvt · 2018-08-01T04:31:08Z

This is ready for review. I'm not getting the errors that CI is seeing locally and without config access of the ability to rerun the build, I'm not sure what I can do to green these in CI.

2-am-zzz · 2018-08-01T17:23:12Z

YES. Excited for this. However, I suppose this warrants a few design questions. I apologize if some of these qs are self-explaining in the code--I haven't touched the codebase in awhile:

Does this detection account for zero-ether and non-zero-token accounts?
How would this account for a situation where acct0 and acct2 have non-zero balances, but acct1 does? Should we even account for this edge case if not already?

dmvt · 2018-08-01T18:44:58Z

Thanks @Zanibas. No apologies needed. I pretty much stuck to the letter of the original issue.

Only ETH balance is considered at this time.
It doesn't account for that situation, unfortunately. I did play with seeking several accounts ahead instead of just one, but it introduced too many issues. Each new account generation and balance check has a decent time and performance cost to it due to the need to lookup it's balance. There is also a race condition that happens when trying to create a new account and then immediately removing it that causes the entire extension to hang.

2-am-zzz · 2018-08-01T19:44:48Z

I'd like as second opinion from either @danjm or @danfinlay, but this as an MVP sounds sufficient, but would like to open an additional issue for issue #1 and to tackle the edge case described in #2.

The more common case I can imagine for #2 is a case where people are leaving their first account blank and instead populating the second account (as a light countermeasure to anyone snooping on their phrase).

dmvt · 2018-08-06T15:44:52Z

@Zanibas @danfinlay Hope you guys had a good weekend. I'd love to wrap this bounty up this week. How's the code looking after some reflection time?

danjm · 2018-08-07T01:15:03Z

test/e2e/beta/from-import-beta-ui.spec.js

@@ -26,8 +24,7 @@ describe('Using MetaMask with an existing account', function () {

  const testSeedPhrase = 'phrase upgrade clock rough situate wedding elder clever doctor stamp excess tent'
  const testAddress = '0xE18035BF8712672935FDB4e5e431b1a0183d2DFC'
-  const testPrivateKey2 = '14abe6f4aab7f9f626fe981c864d0adeb5685f289ac9270c27b8fd790b4235d6'
-  const tinyDelayMs = 500
+  const testPrivateKey2 = 'f5e21e00974fa60353d003938363db0296542d2d24208e88699536940f18fc80'


Not sure if this is because of the merge conflict. If not, why is testPrivateKey2 changed in this PR?

testPrivateKey2 has been changed here because the tests that use it, 'Imports an account with private key', are attempting to import an account which is not already present in the accounts list. Because all of the previous private keys have been given balances for one reason or another in the previous tests, they are part of the accounts list and can't be imported a second time.

This change is no longer required now that the network switches at the beginning of the 'Imports an account with private key' test chain

dmvt · 2018-08-08T15:18:28Z

@Zanibas @danjm @danfinlay The merge conflict has been resolved and allowed me to revert the private key change. The two failing CI checks are still passing locally. I've tried it on two different machines and both pass. I did notice that if I'm running too many applications on my laptop when I run the tests, they both fail due to timeout errors, so the failures in CircleCI might just be because the VMs are underpowered.

owocki · 2018-08-15T15:59:26Z

@Zanibas @danfinlay got 3 minutes to let @dmvt nkow how he's doing here?

whymarrh · 2018-08-16T12:55:40Z

@dmvt hey, thanks for this! (And apologies for the slow review.) Would you mind rebasing this on develop (or merging in develop) and fixing up the few conflicts? The e2e tests are a lot more stable now on develop so once we update this PR we should be able to get those to pass.

whymarrh

This PR looks pretty good, I've left a few comments.

Additionally, we should add a test case with the situation @Zanibas described above where we have an account without a balance sandwiched between accounts with balances. Having this test will document that our stopping at the first account with a zero balance is intentional.

whymarrh · 2018-08-16T12:59:44Z

app/scripts/metamask-controller.js

+   * Get an account balance from the AccountTracker or request it directly from the network.
+   * @param {string} account - The account address
+   */
+  getBalance (account) {


Nit: Can we rename this to address?

whymarrh · 2018-08-16T13:01:39Z

app/scripts/metamask-controller.js

+
+      if (cached && cached.balance) {
+        resolve(cached.balance)
+      } else {


I think we can return inside the if block and then we won't need to nest the rest of the fn inside else

Unfortunately no... the accountTracker is not necessarily up to date at this point in the code. Many times it does not yet have a balance.

Hmm, what I was getting at is that if we add a return statement after resolving the promise we don't need to have the rest of the function inside an else block since there isn't anything after this conditional.

It's certainly not a blocking comment though, more a small code-flow thing.

whymarrh · 2018-08-16T13:02:50Z

app/scripts/metamask-controller.js

+        this.networkController.ethQuery.getBalance(account, (error, balance) => {
+          if (error) {
+            reject(error)
+            console.error(error)


There is a log logger that we use instead of the using console directly—can we switch this line here to use that?

whymarrh · 2018-08-16T13:09:56Z

test/unit/app/controllers/metamask-controller-test.js

@@ -134,13 +136,23 @@ describe('MetaMaskController', function () {
  describe('#createNewVaultAndRestore', function () {
    it('should be able to call newVaultAndRestore despite a mistake.', async function () {
      const password = 'what-what-what'
+      sandbox.stub(metamaskController, 'getBalance')
+      metamaskController.getBalance.callsFake(() => {
+        return new Promise((resolve) => { resolve('0x0') })


We might be able to simplify this by using Promise.resolve():

metamaskController.getBalance.callsFake(() => Promise.resolve('0x0'))

Similarly for the instances below

whymarrh · 2018-08-16T13:17:45Z

test/unit/app/controllers/metamask-controller-test.js

+    it('should ask the network for a balance when not known by accountTracker', async () => {
+      const accounts = {}
+      const balance = '0x14ced5122ce0a000'
+      const realNetworkController = metamaskController.networkController


Instead of managing this ourselves we can use sinon here:

const ethQueryGetBalance = sinon.stub(metamaskController.networkController.ethQuery, "getBalance").callsFake(/* the fake impl that's below */)

(I think this should work.)

And then we can call ethQueryGetBalance.restore() below and maybe even assert that it was indeed called.

whymarrh · 2018-08-16T13:26:17Z

On fixing the e2e conflicts, we might be able to defer to what's in develop (@danjm?)

dmvt · 2018-08-16T13:44:00Z

Thanks @whymarrh. I'll get those changes and the rebase in later today.

danjm · 2018-08-16T13:54:27Z

@whymarrh @dmvt yeah, defer to develop for conflicts in the e2e test files.

I think the e2e tests will still fail because of changes in this PR. @dmvt , once the conflicts are fixed, I can fix the e2e tests. They can be troublesome and I think fixing them would be outside the scope of the bounty.

dmvt · 2018-08-17T06:19:13Z

I believe that addresses all of your comments @whymarrh.

whymarrh · 2018-08-17T12:36:06Z

app/scripts/metamask-controller.js

-      const vault = await this.keyringController.createNewVaultAndRestore(password, seed)
+      const vault = await keyringController.createNewVaultAndRestore(password, seed)
+
+      const ethQuery = new EthQuery(this.networkController._provider)


We should be able to use this.provider here instead of reaching into the network controller

whymarrh · 2018-08-17T12:37:42Z

@dmvt thanks! Two last comments (I think), one about the provider and additionally, we should add a test case with the situation @Zanibas described above where we have an account without a balance sandwiched between accounts with balances. Having this test will document that our stopping at the first account with a zero balance is intentional.

dmvt · 2018-08-17T14:32:26Z

@whymarrh Good catch on the this.network. That worked like a charm. Regarding the test, it already does that.

    it('should restore any consecutive accounts with balances', async () => {
      sandbox.stub(metamaskController, 'getBalance')
      metamaskController.getBalance.withArgs(TEST_ADDRESS).callsFake(() => {
        return Promise.resolve('0x14ced5122ce0a000')
      })
      metamaskController.getBalance.withArgs(TEST_ADDRESS_2).callsFake(() => {
        return Promise.resolve('0x0')
      })
      // If we were continuing past the above zero balance the deepEqual would fail because of this stub:
      metamaskController.getBalance.withArgs(TEST_ADDRESS_3).callsFake(() => {
        return Promise.resolve('0x14ced5122ce0a000')
      })
      await metamaskController.createNewVaultAndRestore('foobar1337', TEST_SEED)
      assert.deepEqual(metamaskController.getState().identities, {
        [TEST_ADDRESS]: { address: TEST_ADDRESS, name: DEFAULT_LABEL },
        [TEST_ADDRESS_2]: { address: TEST_ADDRESS_2, name: DEFAULT_LABEL_2 },
      })
    })

whymarrh · 2018-08-17T14:48:37Z

Aw shoot, you're right. My bad.

whymarrh

This looks good, thanks @dmvt!

danfinlay · 2018-08-17T17:43:10Z

Thanks for all the hard work, sorry I fell behind on this @dmvt! Great job on this, it's going to save a lot of people stress when restoring!

dmvt · 2018-08-17T18:29:52Z

Happy to help!

dmvt force-pushed the multi-account-restore branch 4 times, most recently from ce7e90c to 678a096 Compare August 1, 2018 03:48

dmvt changed the title ~~WIP - on restore adds accounts until one with a zero balance is found~~ on restore adds accounts until one with a zero balance is found Aug 1, 2018

dmvt changed the title ~~on restore adds accounts until one with a zero balance is found~~ Restore multiple consecutive accounts with balances. Aug 1, 2018

dmvt force-pushed the multi-account-restore branch 3 times, most recently from 89da9bc to c0ab438 Compare August 1, 2018 04:11

gitcoinbot mentioned this pull request Aug 1, 2018

check for non-zero-balance accounts when restoring from a seed #654

Closed

2-am-zzz self-requested a review August 1, 2018 17:18

bdresser requested a review from danjm August 6, 2018 16:22

danjm reviewed Aug 7, 2018

View reviewed changes

dmvt force-pushed the multi-account-restore branch from c0ab438 to ee9d40e Compare August 8, 2018 14:50

whymarrh suggested changes Aug 16, 2018

View reviewed changes

dmvt force-pushed the multi-account-restore branch 2 times, most recently from 7c22874 to 6be670b Compare August 17, 2018 06:09

whymarrh suggested changes Aug 17, 2018

View reviewed changes

Restores accounts until one with a zero balance is found

df799d7

dmvt force-pushed the multi-account-restore branch from 6be670b to df799d7 Compare August 17, 2018 14:26

whymarrh approved these changes Aug 17, 2018

View reviewed changes

danfinlay merged commit 755369e into MetaMask:develop Aug 17, 2018

dmvt deleted the multi-account-restore branch August 17, 2018 18:29

tmashuang mentioned this pull request Sep 17, 2018

Version 4.10.0 #5281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore multiple consecutive accounts with balances. #4898

Restore multiple consecutive accounts with balances. #4898

dmvt commented Jul 27, 2018 •

edited

Loading

dmvt commented Jul 27, 2018

dmvt commented Aug 1, 2018

2-am-zzz commented Aug 1, 2018

dmvt commented Aug 1, 2018

2-am-zzz commented Aug 1, 2018

dmvt commented Aug 6, 2018

danjm Aug 7, 2018

dmvt Aug 7, 2018

dmvt Aug 8, 2018

dmvt commented Aug 8, 2018

owocki commented Aug 15, 2018

whymarrh commented Aug 16, 2018

whymarrh left a comment

whymarrh Aug 16, 2018

whymarrh Aug 16, 2018

dmvt Aug 17, 2018

whymarrh Aug 17, 2018

whymarrh Aug 16, 2018

whymarrh Aug 16, 2018

whymarrh Aug 16, 2018

whymarrh Aug 16, 2018

whymarrh commented Aug 16, 2018

dmvt commented Aug 16, 2018

danjm commented Aug 16, 2018

dmvt commented Aug 17, 2018

whymarrh Aug 17, 2018

whymarrh commented Aug 17, 2018

dmvt commented Aug 17, 2018

whymarrh commented Aug 17, 2018

whymarrh left a comment

danfinlay commented Aug 17, 2018

dmvt commented Aug 17, 2018

Restore multiple consecutive accounts with balances. #4898

Restore multiple consecutive accounts with balances. #4898

Conversation

dmvt commented Jul 27, 2018 • edited Loading

Solution/Spec

Submitting a pull request

dmvt commented Jul 27, 2018

dmvt commented Aug 1, 2018

2-am-zzz commented Aug 1, 2018

dmvt commented Aug 1, 2018

2-am-zzz commented Aug 1, 2018

dmvt commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmvt commented Aug 8, 2018

owocki commented Aug 15, 2018

whymarrh commented Aug 16, 2018

whymarrh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whymarrh commented Aug 16, 2018

dmvt commented Aug 16, 2018

danjm commented Aug 16, 2018

dmvt commented Aug 17, 2018

Choose a reason for hiding this comment

whymarrh commented Aug 17, 2018

dmvt commented Aug 17, 2018

whymarrh commented Aug 17, 2018

whymarrh left a comment

Choose a reason for hiding this comment

danfinlay commented Aug 17, 2018

dmvt commented Aug 17, 2018

dmvt commented Jul 27, 2018 •

edited

Loading