Skip to content

Commit

Permalink
fix: Prevent HD keyring from being removed (#24096)
Browse files Browse the repository at this point in the history
## **Description**

A patch has been added for the `@metamask/keyring-controller` package
that will prevent the HD keyring from being removed. This is a
precautionary change, see
MetaMask/mobile-planning#1507 for additional
details.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24096?quickstart=1)

## **Related issues**

This is a port of this PR: MetaMask/core#4168
Here is the same patch on mobile:
MetaMask/metamask-mobile#9092

## **Manual testing steps**

This is a precaution against a bug that probably does not exist. We
don't have reproduction steps.

However, here are some use cases related to this area of code, which we
can test to ensure nothing has broken:
* Lock and unlock the wallet
* Restart the extension then unlock it again
* Add a new account, lock the wallet, restart the extension, then check
to ensure the new account is still present.

## **Screenshots/Recordings**

N/A

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
Gudahtt committed Apr 25, 2024
1 parent e0b9c05 commit ae3b8aa
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 8 deletions.
Original file line number Diff line number Diff line change
@@ -1,21 +1,69 @@
diff --git a/dist/KeyringController.js b/dist/KeyringController.js
index fc649ea6fc97b905d811b236de638172fb10b548..beab676ab85e5e372eda7846e98b7d34af6317f5 100644
index fc649ea6fc97b905d811b236de638172fb10b548..bb288fda7e2ef970ceefe2ac22bcf653a6a55b8a 100644
--- a/dist/KeyringController.js
+++ b/dist/KeyringController.js
@@ -1092,9 +1092,13 @@ _KeyringController_keyringBuilders = new WeakMap(), _KeyringController_keyrings
@@ -576,6 +576,18 @@ class KeyringController extends base_controller_1.BaseController {
return { type, data };
})));
serializedKeyrings.push(...__classPrivateFieldGet(this, _KeyringController_unsupportedKeyrings, "f"));
+ /**
+ * ============================== PATCH INFORMATION ==============================
+ * The HD keyring is the default keyring for all wallets if this keyring is missing
+ * for some reason we should avoid saving the keyrings
+ *
+ * The upstream fix is here: https://github.com/MetaMask/core/pull/4168
+ *
+ * This patch can be found on the core branch `extension-keyring-controller-v13-patch`
+ */
+ if (!serializedKeyrings.some((keyring) => keyring.type === KeyringTypes.hd)) {
+ throw new Error(constants_1.KeyringControllerError.NoHdKeyring);
+ }
let vault;
let newEncryptionKey;
if (__classPrivateFieldGet(this, _KeyringController_cacheEncryptionKey, "f")) {
@@ -1092,9 +1104,16 @@ _KeyringController_keyringBuilders = new WeakMap(), _KeyringController_keyrings
}, _KeyringController_addQRKeyring = function _KeyringController_addQRKeyring() {
return __awaiter(this, void 0, void 0, function* () {
// QRKeyring is not yet compatible with Keyring type from @metamask/utils
- const qrKeyring = (yield __classPrivateFieldGet(this, _KeyringController_instances, "m", _KeyringController_newKeyring).call(this, KeyringTypes.qr, {
- accounts: [],
- }));
+ /**
+ * Patch for @metamask/keyring-controller v13.0.0
+ * Below code change will fix the issue 23804, The intial code added a empty accounts as argument when creating a new QR keyring.
+ * cause the new Keystone MetamaskKeyring default properties all are undefined during deserialise() process.
+ * Please refer to PR 23903 for detail.
+ * Patch for @metamask/keyring-controller v13.0.0
+ * Below code change will fix the issue 23804, The intial code added a empty accounts as argument when creating a new QR keyring.
+ * cause the new Keystone MetamaskKeyring default properties all are undefined during deserialise() process.
+ * Please refer to PR 23903 for detail.
+ *
+ * This patch can be found on the core branch `extension-keyring-controller-v13-patch`
+ */
+ // @ts-expect-error See patch note
+ const qrKeyring = (yield __classPrivateFieldGet(this, _KeyringController_instances, "m", _KeyringController_newKeyring).call(this, KeyringTypes.qr));
const accounts = yield qrKeyring.getAccounts();
yield __classPrivateFieldGet(this, _KeyringController_instances, "m", _KeyringController_checkForDuplicate).call(this, KeyringTypes.qr, accounts);
__classPrivateFieldGet(this, _KeyringController_keyrings, "f").push(qrKeyring);
diff --git a/dist/constants.d.ts b/dist/constants.d.ts
index 0c02177576b840c8412bd5c047010439927cf4af..805c0d5f78578efdda95a6da6d66dce13c9003c6 100644
--- a/dist/constants.d.ts
+++ b/dist/constants.d.ts
@@ -25,6 +25,7 @@ export declare enum KeyringControllerError {
MissingVaultData = "KeyringController - Cannot persist vault without vault information",
ExpiredCredentials = "KeyringController - Encryption key and salt provided are expired",
NoKeyringBuilder = "KeyringController - No keyringBuilder found for keyring",
- DataType = "KeyringController - Incorrect data type provided"
+ DataType = "KeyringController - Incorrect data type provided",
+ NoHdKeyring = "KeyringController - No HD Keyring found"
}
//# sourceMappingURL=constants.d.ts.map
\ No newline at end of file
diff --git a/dist/constants.js b/dist/constants.js
index 58b3a15b796396de78b9dc252baf23d5bd40ae0a..10768a8a6ad111c1f6552ba43ce8eca3c570c8eb 100644
--- a/dist/constants.js
+++ b/dist/constants.js
@@ -30,5 +30,6 @@ var KeyringControllerError;
KeyringControllerError["ExpiredCredentials"] = "KeyringController - Encryption key and salt provided are expired";
KeyringControllerError["NoKeyringBuilder"] = "KeyringController - No keyringBuilder found for keyring";
KeyringControllerError["DataType"] = "KeyringController - Incorrect data type provided";
+ KeyringControllerError["NoHdKeyring"] = "KeyringController - No HD Keyring found";
})(KeyringControllerError = exports.KeyringControllerError || (exports.KeyringControllerError = {}));
//# sourceMappingURL=constants.js.map
\ No newline at end of file
4 changes: 2 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4887,7 +4887,7 @@ __metadata:

"@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A13.0.0#~/.yarn/patches/@metamask-keyring-controller-npm-13.0.0-d94816a680.patch":
version: 13.0.0
resolution: "@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A13.0.0#~/.yarn/patches/@metamask-keyring-controller-npm-13.0.0-d94816a680.patch::version=13.0.0&hash=be29a2"
resolution: "@metamask/keyring-controller@patch:@metamask/keyring-controller@npm%3A13.0.0#~/.yarn/patches/@metamask-keyring-controller-npm-13.0.0-d94816a680.patch::version=13.0.0&hash=648493"
dependencies:
"@ethereumjs/util": "npm:^8.1.0"
"@keystonehq/metamask-airgapped-keyring": "npm:^0.13.1"
Expand All @@ -4902,7 +4902,7 @@ __metadata:
async-mutex: "npm:^0.2.6"
ethereumjs-wallet: "npm:^1.0.1"
immer: "npm:^9.0.6"
checksum: 4995160ba60b792b8df4694cfde6762423e72d5ea9a637e39e40b602becd42c08b67dfa8b35b26690be3af5c93d6a4ef453e1ccdc7aff5100c0af5b71c226787
checksum: 4d1de0ff4c29f1aff6c1f2bf62157684aa6e3b5e0447cee72935dc63f3095658dfa3e1b685654c75a83e5e3da8c3595cca0020fbb83e9ad210a0654c33150452
languageName: node
linkType: hard

Expand Down

0 comments on commit ae3b8aa

Please sign in to comment.