Skip to content

Commit

Permalink
Use QRKeyring from KeyringController (#20502)
Browse files Browse the repository at this point in the history
## Explanation

With this PR the extension starts to use the `QRKeyring` provided by
`KeyringController`, instead of declaring and managing it directly in
the client.

<!--
Thanks for the pull request. Take a moment to answer these questions so
that reviewers have the information they need to properly understand
your changes:

* What is the current state of things and why does it need to change?
* What is the solution your changes offer and how does it work?

Are there any issues, Slack conversations, Zendesk issues, user stories,
etc. reviewers should consult to understand this pull request better?
For instance:

* Fixes #12345
* See: #67890
-->

* Fixes #18776

## Manual Testing Steps

<!--
How should reviewers and QA manually test your changes? For instance:

- Go to this screen
- Do this
- Then do this
-->

- QR Hardware Wallets should work as before.

## 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.

---------

Co-authored-by: Salah <salah-eddine.saakoun@consensys.net>
Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
  • Loading branch information
3 people authored Sep 26, 2023
1 parent 07dff1d commit 941b734
Show file tree
Hide file tree
Showing 12 changed files with 489 additions and 125 deletions.
Original file line number Diff line number Diff line change
@@ -1,21 +1,16 @@
diff --git a/dist/KeyringController.d.ts b/dist/KeyringController.d.ts
index d6d70eea2de56310b49fa9dcf1e90afae3ddb202..4518aed8e03376d76e5ab6ef0971529e65563fd3 100644
index 44c6427ec774475b87f60995738f736611875194..d359269ad413a0a626d06e44a84eb0cbbdb8c7b0 100644
--- a/dist/KeyringController.d.ts
+++ b/dist/KeyringController.d.ts
@@ -1,10 +1,11 @@
import type { TxData, TypedTransaction } from '@ethereumjs/tx';
-import { type MetaMaskKeyring as QRKeyring, type IKeyringState as IQRKeyringState } from '@keystonehq/metamask-airgapped-keyring';
+import type { MetaMaskKeyring as QRKeyring, IKeyringState as IQRKeyringState } from '@keystonehq/metamask-airgapped-keyring';
import type { RestrictedControllerMessenger } from '@metamask/base-controller';
import { BaseControllerV2 } from '@metamask/base-controller';
@@ -5,6 +5,7 @@ import { BaseControllerV2 } from '@metamask/base-controller';
import type { PersonalMessageParams, TypedMessageParams } from '@metamask/message-manager';
import type { PreferencesController } from '@metamask/preferences-controller';
import type { Eip1024EncryptedData, Hex, Keyring, Json } from '@metamask/utils';
+import type { KeyringController as EthKeyringController } from '@metamask/eth-keyring-controller';
import type { Patch } from 'immer';
declare const name = "KeyringController";
/**
@@ -155,6 +156,10 @@ export declare class KeyringController extends BaseControllerV2<typeof name, Key
@@ -171,6 +172,10 @@ export declare class KeyringController extends BaseControllerV2<typeof name, Key
* @param opts.state - Initial state to set on this controller.
*/
constructor({ removeIdentity, syncIdentities, updateIdentities, setSelectedAddress, setAccountLabel, encryptor, keyringBuilders, cacheEncryptionKey, messenger, state, }: KeyringControllerOptions);
Expand All @@ -27,10 +22,10 @@ index d6d70eea2de56310b49fa9dcf1e90afae3ddb202..4518aed8e03376d76e5ab6ef0971529e
* Adds a new account to the default (first) HD seed phrase keyring.
*
diff --git a/dist/KeyringController.js b/dist/KeyringController.js
index bce9884ea54bf8e24274dee3876db95ed862abf0..471a06b30f2275de821c33b7d94cb1de85b074c7 100644
index d8b1e1b8da74d1647e6fbe50cf94f63bf9fd28cf..e3790cef6f1a93c6624db5f49cf53e54d4d1c702 100644
--- a/dist/KeyringController.js
+++ b/dist/KeyringController.js
@@ -154,6 +154,12 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
@@ -155,6 +155,12 @@ class KeyringController extends base_controller_1.BaseControllerV2 {
this.setAccountLabel = setAccountLabel;
__classPrivateFieldGet(this, _KeyringController_instances, "m", _KeyringController_registerMessageHandlers).call(this);
}
Expand Down
11 changes: 7 additions & 4 deletions app/scripts/controllers/app-state.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export default class AppStateController extends EventEmitter {
initState,
onInactiveTimeout,
preferencesStore,
qrHardwareStore,
messenger,
extension,
} = opts;
Expand Down Expand Up @@ -78,9 +77,13 @@ export default class AppStateController extends EventEmitter {
}
});

qrHardwareStore.subscribe((state) => {
this.store.updateState({ qrHardware: state });
});
messenger.subscribe(
'KeyringController:qrKeyringStateChange',
(qrHardware) =>
this.store.updateState({
qrHardware,
}),
);

const { preferences } = preferencesStore.getState();
this._setInactiveTimeout(preferences.autoLockTimeLimit);
Expand Down
4 changes: 1 addition & 3 deletions app/scripts/controllers/app-state.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,11 @@ describe('AppStateController', () => {
},
})),
},
qrHardwareStore: {
subscribe: jest.fn(),
},
messenger: {
call: jest.fn(() => ({
catch: jest.fn(),
})),
subscribe: jest.fn(),
},
});
};
Expand Down
4 changes: 1 addition & 3 deletions app/scripts/controllers/mmi-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ describe('MMIController', function () {
})),
registerActionHandler: jest.fn(),
publish: jest.fn(),
subscribe: jest.fn(),
};

mmiController = new MMIController({
Expand Down Expand Up @@ -74,9 +75,6 @@ describe('MMIController', function () {
},
})),
},
qrHardwareStore: {
subscribe: jest.fn(),
},
messenger: mockMessenger,
}),
custodianEventHandlerFactory: jest.fn(),
Expand Down
42 changes: 27 additions & 15 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -635,21 +635,19 @@ export default class MetamaskController extends EventEmitter {
getChainId: () => this.networkController.state.providerConfig.chainId,
});

this.qrHardwareKeyring = new QRHardwareKeyring();

this.appStateController = new AppStateController({
addUnlockListener: this.on.bind(this, 'unlock'),
isUnlocked: this.isUnlocked.bind(this),
initState: initState.AppStateController,
onInactiveTimeout: () => this.setLocked(),
preferencesStore: this.preferencesController.store,
qrHardwareStore: this.qrHardwareKeyring.getMemStore(),
messenger: this.controllerMessenger.getRestricted({
name: 'AppStateController',
allowedActions: [
`${this.approvalController.name}:addRequest`,
`${this.approvalController.name}:acceptRequest`,
],
allowedEvents: [`KeyringController:qrKeyringStateChange`],
}),
extension: this.extension,
});
Expand Down Expand Up @@ -872,13 +870,24 @@ export default class MetamaskController extends EventEmitter {

const keyringControllerMessenger = this.controllerMessenger.getRestricted({
name: 'KeyringController',
allowedActions: [
'KeyringController:getState',
'KeyringController:signMessage',
'KeyringController:signPersonalMessage',
'KeyringController:signTypedMessage',
'KeyringController:decryptMessage',
'KeyringController:getEncryptionPublicKey',
'KeyringController:getKeyringsByType',
'KeyringController:getKeyringForAccount',
'KeyringController:getAccounts',
],
allowedEvents: [
'KeyringController:accountRemoved',
'KeyringController:lock',
'KeyringController:stateChange',
'KeyringController:lock',
'KeyringController:unlock',
'KeyringController:accountRemoved',
'KeyringController:qrKeyringStateChange',
],
allowedActions: ['KeyringController:getState'],
});

this.coreKeyringController = new KeyringController({
Expand Down Expand Up @@ -2304,6 +2313,7 @@ export default class MetamaskController extends EventEmitter {
addressBookController,
alertController,
appStateController,
coreKeyringController,
nftController,
nftDetectionController,
currencyRateController,
Expand All @@ -2316,7 +2326,6 @@ export default class MetamaskController extends EventEmitter {
onboardingController,
permissionController,
preferencesController,
qrHardwareKeyring,
swapsController,
tokensController,
smartTransactionsController,
Expand Down Expand Up @@ -2423,15 +2432,17 @@ export default class MetamaskController extends EventEmitter {

// qr hardware devices
submitQRHardwareCryptoHDKey:
qrHardwareKeyring.submitCryptoHDKey.bind(qrHardwareKeyring),
coreKeyringController.submitQRCryptoHDKey.bind(coreKeyringController),
submitQRHardwareCryptoAccount:
qrHardwareKeyring.submitCryptoAccount.bind(qrHardwareKeyring),
cancelSyncQRHardware:
qrHardwareKeyring.cancelSync.bind(qrHardwareKeyring),
submitQRHardwareSignature:
qrHardwareKeyring.submitSignature.bind(qrHardwareKeyring),
coreKeyringController.submitQRCryptoAccount.bind(coreKeyringController),
cancelSyncQRHardware: coreKeyringController.cancelQRSynchronization.bind(
coreKeyringController,
),
submitQRHardwareSignature: coreKeyringController.submitQRSignature.bind(
coreKeyringController,
),
cancelQRHardwareSignRequest:
qrHardwareKeyring.cancelSignRequest.bind(qrHardwareKeyring),
coreKeyringController.cancelQRSignRequest.bind(coreKeyringController),

// vault management
submitPassword: this.submitPassword.bind(this),
Expand Down Expand Up @@ -3373,7 +3384,7 @@ export default class MetamaskController extends EventEmitter {
}

/**
* Fetch account list from a trezor device.
* Fetch account list from a hardware device.
*
* @param deviceName
* @param page
Expand All @@ -3382,6 +3393,7 @@ export default class MetamaskController extends EventEmitter {
*/
async connectHardware(deviceName, page, hdPath) {
const keyring = await this.getKeyringForDevice(deviceName, hdPath);

let accounts = [];
switch (page) {
case -1:
Expand Down
82 changes: 79 additions & 3 deletions lavamoat/browserify/beta/policy.json
Original file line number Diff line number Diff line change
Expand Up @@ -1575,7 +1575,7 @@
"@metamask/eth-keyring-controller>@metamask/eth-hd-keyring": true,
"@metamask/eth-keyring-controller>@metamask/eth-simple-keyring": true,
"@metamask/keyring-controller>@metamask/eth-keyring-controller>@metamask/eth-sig-util": true,
"@metamask/keyring-controller>@metamask/utils": true,
"@metamask/keyring-controller>@metamask/eth-keyring-controller>@metamask/utils": true,
"@metamask/obs-store": true,
"webpack>events": true
}
Expand All @@ -1591,6 +1591,19 @@
"eth-sig-util>tweetnacl-util": true
}
},
"@metamask/keyring-controller>@metamask/eth-keyring-controller>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/key-tree>@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true,
"superstruct": true
}
},
"@metamask/keyring-controller>@metamask/utils": {
"globals": {
"TextDecoder": true,
Expand Down Expand Up @@ -1665,7 +1678,7 @@
"@metamask/message-manager": {
"packages": {
"@metamask/base-controller": true,
"@metamask/controller-utils": true,
"@metamask/message-manager>@metamask/controller-utils": true,
"@metamask/message-manager>@metamask/eth-sig-util": true,
"@metamask/message-manager>jsonschema": true,
"browserify>buffer": true,
Expand All @@ -1674,17 +1687,80 @@
"webpack>events": true
}
},
"@metamask/message-manager>@metamask/controller-utils": {
"globals": {
"URL": true,
"console.error": true,
"fetch": true,
"setTimeout": true
},
"packages": {
"@metamask/controller-utils>@spruceid/siwe-parser": true,
"@metamask/message-manager>@metamask/utils": true,
"browserify>buffer": true,
"eslint>fast-deep-equal": true,
"eth-ens-namehash": true,
"ethereumjs-util": true,
"ethjs>ethjs-unit": true
}
},
"@metamask/message-manager>@metamask/eth-sig-util": {
"packages": {
"@ethereumjs/tx>@ethereumjs/util": true,
"@ethereumjs/tx>ethereum-cryptography": true,
"bn.js": true,
"@metamask/message-manager>@metamask/eth-sig-util>@metamask/abi-utils": true,
"@metamask/message-manager>@metamask/eth-sig-util>@metamask/utils": true,
"browserify>buffer": true,
"eth-sig-util>ethereumjs-util>ethjs-util": true,
"eth-sig-util>tweetnacl": true,
"eth-sig-util>tweetnacl-util": true
}
},
"@metamask/message-manager>@metamask/eth-sig-util>@metamask/abi-utils": {
"packages": {
"@metamask/message-manager>@metamask/eth-sig-util>@metamask/abi-utils>@metamask/utils": true,
"superstruct": true
}
},
"@metamask/message-manager>@metamask/eth-sig-util>@metamask/abi-utils>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/key-tree>@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true,
"superstruct": true
}
},
"@metamask/message-manager>@metamask/eth-sig-util>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/key-tree>@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true,
"superstruct": true
}
},
"@metamask/message-manager>@metamask/utils": {
"globals": {
"TextDecoder": true,
"TextEncoder": true
},
"packages": {
"@metamask/key-tree>@noble/hashes": true,
"browserify>buffer": true,
"nock>debug": true,
"semver": true,
"superstruct": true
}
},
"@metamask/message-manager>jsonschema": {
"packages": {
"browserify>url": true
Expand Down
Loading

0 comments on commit 941b734

Please sign in to comment.