Refactor keyring to split bridge logic #143

bergarces · 2022-11-15T16:33:11Z

The goal of this PR is to split the logic that interacts directly with the Ledger device from the keyring logic that handles the interface with KeyringController.

BREAKING: Splits keyring and bridge logic into TrezorKeyring and TrezorConnectBridge classes.
ADDED: TrezorBridge interface to use in other implementations.
CHANGED: Removes initialisation code from the constructor and to the already supported init method.

package.json

bergarces · 2022-11-16T13:13:18Z

trezor-keyring-mv2.js

+    // here: https://github.com/trezor/connect/blob/dec4a56af8a65a6059fb5f63fa3c6690d2c37e00/src/js/iframe/builder.js#L181
+    TrezorConnect.dispose();
+    return Promise.resolve();
+  }


It is worth mentioning I noticed that this method is only present for trezor and ledger keyring, but they go under different names (dispose and destroy).

Perhaps it would be a good idea to standardise the name so that we can call it if it exists. Same as we are doing with init.

This is where they are being called.
https://github.com/MetaMask/metamask-extension/blob/61b3d25ab343f287e409e8b4509e7e1320e118cd/app/scripts/metamask-controller.js#L4288

Agreed! We've used destroy for this sort of thing elsewhere, so maybe that makes sense as the standard name. We can do that in a separate PR.

mcmire

Hey @bergarces, came across this PR in my notifications and had a few suggestions. Cheers!

@types/index.d.ts

trezor-keyring.js

test/test-eth-trezor-keyring.js

trezor-keyring-mv2.js

mcmire · 2022-11-16T18:06:43Z

trezor-keyring-mv2.js

+  TREZOR_CONNECT_MANIFEST,
+} = require('./trezor-keyring');
+
+class TrezorKeyringMv2 extends TrezorKeyring {


This class doesn't appear to be the exact same as the previous TrezorKeyring class. For instance:

The initialization logic that was in the constructor has been moved to init

dispose now returns a promise

Is that intentional in this PR? Those would be breaking changes if so, in which case we no longer have to have index.js expose a default export.

Adding the init method and making the dispose method return a promise in case that other implementations of the bridge logic require it are intentional changes. I've added a PR for the KeyringController to take the init method into account, as other keyring implementations might need it as well.

What I'm not too sure about is if I should make them completely optional by not mentioning them at all in the base class, return an error or return an empty promise.

I'm starting to lean towards not defining them at all in the base class, then have a separate interface in the exported types in case they have to be added so that whoever implements them has a function signature, but would like to discuss it further.

bergarces · 2022-11-17T10:35:42Z

Hey @bergarces, came across this PR in my notifications and had a few suggestions. Cheers!

Hey @mcmire, thanks for taking the time to review and for the suggestions, I've applied most of them and I'll make sure to use them for the ledger keyring changes too.

bergarces · 2022-11-17T10:38:26Z

I'd also wanted to say... would it be better to move all source files to its own folder?

base-trezor-keyring.js

Gudahtt · 2022-11-24T21:21:26Z

Interesting - so the "bridge" portion is the child class, extending from the base keyring with the shared logic.

Have you considered passing the bridge in as a constructor parameter? Then we could have a bridge API rather than needing the bridge to extend the keyring and override methods.

Edit: Oh, perhaps that's what you meant here.

Gudahtt · 2022-11-25T16:01:15Z

^ I just thought of one more advantage to passing the bridge in: it makes it feasible to change the bridge at runtime.

Not that we'd need that to be a supported feature right away, but, this relates to the idea I mentioned at one point for splitting up the Ledger Live bridge from the Ledger iframe-based bridge. That's a toggle that exists at runtime, so we'd weant the bridge to be changeable at runtime. Which... we could do if it was a child class by saving state, reconstructing, and restoring, but that would be complex to manage. It seems more sensible to make the bridge a separate entity that we could swap out without reconstructing the keyring itself.

test/eth-trezor-keyring.test.js

bergarces · 2022-12-05T11:26:50Z

^ I just thought of one more advantage to passing the bridge in: it makes it feasible to change the bridge at runtime.

Not that we'd need that to be a supported feature right away, but, this relates to the idea I mentioned at one point for splitting up the Ledger Live bridge from the Ledger iframe-based bridge. That's a toggle that exists at runtime, so we'd weant the bridge to be changeable at runtime. Which... we could do if it was a child class by saving state, reconstructing, and restoring, but that would be complex to manage. It seems more sensible to make the bridge a separate entity that we could swap out without reconstructing the keyring itself.

Hey @Gudahtt, I've added the discussed changes. Instead of extending from BaseTrezorKeyring (now TrezorKeyring), the logic is now in its own TrezorBridgeMv2 and a TypeScript interface is exported for new bridge implementations.

Since deserialize also returns a promise, even though there's nothing asynchronous within the method, I've also moved it out of the constructor and replaced the opts by the bridge dependency. The changes needed to handle that are in the KeyringController PR.

https://github.com/MetaMask/KeyringController/pull/163/files

test/eth-bridge-mv2.test.js

trezor-keyring.js

bergarces · 2023-03-15T15:19:05Z

src/trezor-connect-bridge.ts

+    } & Partial<ConnectSettings>,
+  ) {
+    TrezorConnect.on(DEVICE_EVENT, (event) => {
+      if (event.type !== DEVICE.CONNECT) {


It's possible to force typescript type inference by using the event discriminator, that saves the use of hasProperty and avoid importing @metamask/utils as a dependency.

bergarces · 2023-03-15T15:21:17Z

src/trezor-keyring.test.ts

-
-global.window = windowShim;
-global.navigator = navigatorShim;
-global.self = selfShim;


These shims are not needed for these tests, as we are creating stubs for TrezorConnect.

socket-security · 2023-05-05T13:16:34Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

socket-security · 2023-06-09T09:58:16Z

New and updated dependency changes detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: @metamask/utils@4.0.0

Gudahtt

LGTM!

bergarces requested a review from a team as a code owner November 15, 2022 16:33

bergarces force-pushed the bridge-refactor branch from eebcb0f to b0f5036 Compare November 15, 2022 16:53

bergarces commented Nov 15, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

bergarces commented Nov 16, 2022

View reviewed changes

mcmire reviewed Nov 16, 2022

View reviewed changes

bergarces force-pushed the bridge-refactor branch 2 times, most recently from eb0fe99 to bdaedad Compare November 17, 2022 10:57