refactor: use withKeyring to batch account restore operation
#11409
Conversation
|
|
We could make this even more efficient by batching the queried balances, and it would look something like this:
This would result in 1 single network call and 1 single state update. Additionally, it would allow to restore accounts with balance even if there's an empty balance account in the middle |
mikesposito
added
Run Smoke E2E
mikesposito
force-pushed
the
refactor-import-additional-accounts
branch
from
51b0b10
to
6e55ca9
Compare
mikesposito
added
Run Smoke E2E
|
app/util/importAdditionalAccounts.js
Outdated
| await KeyringController.withKeyring( | ||
| { type: ExtendedKeyringTypes.hd }, | ||
| async (primaryKeyring) => { | ||
| let i = 0; | ||
| // seek out the first zero balance | ||
| while (i < MAX) { | ||
| const [newAccount] = await primaryKeyring.addAccounts(1); | ||
| const newAccountBalance = await getBalance(newAccount, ethQuery); | ||
|
|
||
| let i = 0; | ||
| // seek out the first zero balance | ||
| while (lastBalance !== ZERO_BALANCE) { | ||
| if (i === MAX) break; | ||
| await KeyringController.addNewAccountWithoutUpdate(primaryKeyring); | ||
| accounts = await KeyringController.getAccounts(); | ||
| lastBalance = await getBalance(accounts[accounts.length - 1], ethQuery); | ||
| i++; | ||
| } | ||
| if (newAccountBalance === ZERO_BALANCE) { | ||
| // remove extra zero balance account we just added and break the loop | ||
| primaryKeyring.removeAccount(newAccount); | ||
| break; | ||
| } | ||
|
|
||
| // remove extra zero balance account potentially created from seeking ahead | ||
| if (accounts.length > 1 && lastBalance === ZERO_BALANCE) { | ||
| await KeyringController.removeAccount(accounts[accounts.length - 1]); | ||
| accounts = await KeyringController.getAccounts(); | ||
| } | ||
| i++; | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
KeyringController wil do all the heavy lifting here: race condition protection, primary keyring existence check, batch state update with the resulting keyring, rollback in case of error and automatic vault update
This code has never been covered |
app/util/importAdditionalAccounts.js
Outdated
| } catch (error) { | ||
| // Errors are gracefully handled so that `withKeyring` | ||
| // will not rollback the primary keyring, and accounts | ||
| // created in previous loop iterations will remain in place. | ||
| Logger.error(error); |
|
We should also manual test it. We can handle that in the accounts team. |
|
@gantunesr I will convert the file to typescript, add tests and post a recording of the flow |
|
Description
The problem
With the current used version of
KeyringController, theaddNewAccountWithoutUpdatemethod is not doing what its name claims - the state is updated everytime the function is called.This is because
KeyringControllerhas no control over its messenger's listeners, and soPreferencesControllerandAccountsController(and so on downstream) will react to keyrings state updates everytimeaddNewAccountWithoutUpdateis called.The result is that the
importAdditionalAccountshelper present on mobile is incredibly inefficient, as accounts will be added one by one, executing all side effects at every single iteration.The solution
This PR leverages the new
withKeyringmethod from KeyringController to batch these iterations into one single atomic state update - take a look at the code for more info about how it is doneRelated issues
Related: MetaMask/core#3848
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist