Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

fix keeping the wallet unlocked on sw restart after onboarding #203

Merged
merged 7 commits into from
Mar 6, 2023

Conversation

pedronfigueiredo
Copy link
Contributor

Explanation

We want to keep the wallet unlocked when a service worker restarts.

In persistAllKeyrings, when we're building for mv3 and want to cache the encryptionKey but a password was entered and a new encryptionKey has been generated, we want to save encryptionSalt alongside the new encryptionKey in the  keyring controller memStore.

This is so it can be saved in storage and then retrieved on the metamask controller for the extension.

Note that this PR only fixes the first time the service worker is restarted after a fresh wallet is loaded and the onboarding flow is complete. Unlocked wallet persistence for subsequent service worker restarts has been fixed on a separate PR on the extension repository.

For context, this was previously addressed, but the wallet was still locked after a long enough waiting period after the service worker restarted. Another PR has since been merged, making the wallet lock more immediately apparent.

Manual Testing Steps

  1. Build the extension wallet from develop

git checkout develop && git pull && yarn && yarn:start:mv3

  1. Load the wallet in the browser and complete the onboarding flow

  2. Terminate the service worker using chrome://inspect/#service-workers.

  3. The wallet should lock after a couple of seconds.

  4. Add the changes on index.js on this PR to the corresponding file on the node_modules folder of the Metamask Extension.

  5. Build the wallet again.

  6. Load the wallet in the browser and complete the onboarding flow

  7. Terminate the service worker using chrome://inspect/#service-workers.

  8. The wallet should remain unlocked.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@pedronfigueiredo pedronfigueiredo added the type-bug Something isn't working label Mar 3, 2023
@pedronfigueiredo pedronfigueiredo requested a review from a team as a code owner March 3, 2023 12:31
@pedronfigueiredo pedronfigueiredo self-assigned this Mar 3, 2023
test/index.js Outdated Show resolved Hide resolved
test/index.js Show resolved Hide resolved
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.

Looks great! Great to have these additional tests, but I think the code being changed here isn't yet adequately covered. I can approve once it's covered by at least one test

test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Mar 3, 2023
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.

LGTM! Left a comment on two tests that can still be improved

@pedronfigueiredo pedronfigueiredo force-pushed the fix/keep-freshly-loaded-wallet-unlocked branch from e33481e to b0a4300 Compare March 3, 2023 20:58
@pedronfigueiredo pedronfigueiredo requested a review from Gudahtt March 6, 2023 11:41
@pedronfigueiredo pedronfigueiredo merged commit 5aaa2e7 into main Mar 6, 2023
@pedronfigueiredo pedronfigueiredo deleted the fix/keep-freshly-loaded-wallet-unlocked branch March 6, 2023 17:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type-bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants