-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 #15050 - MV3: Keep the user logged in when service worker restarts #15558
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
I've noticed the following behaviors:
About 3 and 4, I am not completely sure this would ever happen in a real life scenario, as I don't think anyone would terminate the Service Worker manually. Just including it here in case we do need to consider it @darkwing @danjm Behavior 2 mm-popup-idle.webmBehavior 3 mm-full-manual-restart.webmBehavior 4 mm-popup-manual-restart.webm |
538ae0a
to
4d4ba9c
Compare
Some QA finding with last commits, I am getting the error
key-not-extractable-create.webm
key-not-extractable.webm |
b01909e
to
cacb5ca
Compare
9f689d7
to
20c066a
Compare
Started re-testing with the last changes and all previous errors mentioned above disappeared ⭐ I saw that the error Repro steps:
vault-map-notfunc.webm |
Thanks @seaona ! I spotted this issue yesterday, as I was missing an |
Builds ready [71a087f]
Page Load Metrics (2140 ± 87 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [c2577d9]
Page Load Metrics (2223 ± 113 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
@darkwing : Code changes look good and seems to work well 👍 I see one scenario breaking. If I login in MV2 and then start extension in MV3, I get this error while trying to login again using password: |
That should be the Ledger hardware wallet error, correct? That's a separate issue being worked on by the hardware wallets team. |
Specifically, commenting out this line prevents the problem: https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/main/index.js#L40 The issue described isn't specific to or introduced by this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Builds ready [62fc4a2]
Page Load Metrics (2342 ± 170 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Explanation
Currently, when the service worker restarts, the user gets logged out. This PR is the extension portion of the login fix. More details will be added as we get further along.
More Information
Depends on:
eth-keyring-controller
(Keep User Logged In: Export key for encrypted key login KeyringController#152)Manual Testing Steps
yarn start:mv3
Pre-Merge Checklist
+ If there are functional changes: