-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: restore Ledger keyring after restoring the vault #8740
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8740 +/- ##
=======================================
Coverage 43.27% 43.27%
=======================================
Files 1271 1271
Lines 30916 30918 +2
Branches 3092 3092
=======================================
+ Hits 13378 13379 +1
- Misses 16765 16766 +1
Partials 773 773 ☔ View full report in Codecov by Sentry. |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/58b40dc4-ecd6-4af4-87b6-0b74f15f6544 |
|
Britrise e2e tests pass even |
|
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.
It seems to follow the same pattern as the restoreQRKeyring
, but I don't feel 100% comfortable with my knowledge on what's happening on the vault, It would be cool if this have the attention of the accounts team
Just attached |
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.
Just a minor comment
Co-authored-by: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com>
|
E2e tests pass even |
Description
This PR will fix #5721 issue and #8713, the fix contain following change.
Vault.js
, get the old ledger keyring first beforeKeyringController.createNewVaultAndRestore
.restoreLedgerKeyring
method inVault.js
to regenerate the ledger keyring with existing account.Related issues
Fixes: #5721
#8713
Manual testing steps
For #5721
ledger
account is still there.For #8713
ledger
account is in the account list.ledger
account in the account list.Screenshots/Recordings
Before
Screen_Recording_20240227_184709_MetaMask.mp4
Pre-merge author checklist
Pre-merge reviewer checklist