Refactor keyring to split bridge logic #156

bergarces · 2022-11-16T20:48:15Z

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 LedgerKeyring and LedgerIframeBridge classes.
ADDED: LedgerBridge interface to use in other implementations.
CHANGED: Removes initialisation code from the constructor and to the already supported init method.

base-ledger-keyring.js

bergarces · 2022-11-17T13:11:07Z

base-ledger-keyring.js

+    throw new Error('Method init not implemented')
+  }
+
+  destroy () {


In the trezor keyring there's a similar method called dispose. It'd be a breaking change, but we could rename this one to dispose and only call it if it exists, instead of calling one or the other on the extension depending on the keyring type.

bergarces · 2022-11-17T13:17:20Z

base-ledger-keyring.js

+    this.hdPath = hdPath
+  }
+
+  async unlock (hdPath, updateHdk = true) {


I've done a simple refactor to methods that call the protected bridge methods.

Instead of returning an object with a success boolean and the payload. I'm just returning the payload and wrapping the call in a try/catch. It shouldn't be a breaking change as it's an internal implementation detail for now.

It made creating a different implementation (other than the existing MV2 one) easier, as I was able to just return the payload without wrapping it into an object with a boolean.

Opinions welcome.

base-ledger-keyring.js

ledger-keyring-mv2.js

types/index.d.ts

bergarces · 2022-12-05T10:28:53Z

Moved to draft until there's an agreement about the changes needed for the Trezor keyring and Keyring controller.

ledger-keyring-mv2.js

Co-authored-by: legobeat <[email protected]>

bergarces · 2023-04-26T07:55:37Z

@bergarces any updates?

Hey @legobeat, thanks for the ping, I rebased the whole thing when the typescript support was added but then I completely forgot to follow up.

I'll get some time to solve conflicts and ping interested parties to get this and the trezor PRs merged so I can prepare one for extension and MV3 HW support.

bergarces · 2023-05-08T12:24:16Z

src/ledger-iframe-bridge.ts

+  async init(bridgeUrl: string) {
+    this.bridgeUrl = bridgeUrl;
+
+    this.#setupIframe();


Now that this is not in the constructor, but on an async method, it should be perfectly possible to await this method by returning a promise that resolves on iframe.onLoad. That way we can remove all the iframeLoaded and delayedPromise references and code.

I chose not to do it in this PR so that it's easier to review, but it's a very simply change afterwards.

mcmire · 2023-05-08T20:12:25Z

src/ledger-iframe-bridge.ts

+  }
+
+  async destroy() {
+    window.removeEventListener('message', this.#eventListener.bind(this));


Should we bind #eventListener in the constructor instead of re-binding it here? I'm wondering whether this would work, since in order to remove an event listener it has to match an event listener that was added exactly — but bind creates a new function.

It's copied from the existing code, so I didn't consider the implications of calling bind in both instances. I agree that this approach probably does not work.

Before this library was changed to TypeScript, eventListener was a field within the class, which was initialised at constructor time, so I have pushed a minor refactor to replicate that behaviour (using init instead of the constructor). This guarantees that the event listener passed to addEventListener and removeEventListener is the same.

mcmire · 2023-05-08T20:12:45Z

src/ledger-iframe-bridge.ts

+    return this.#deviceActionMessage(
+      IFrameMessageAction.LedgerUnlock,
+      params,
+    ) as Promise<GetPublicKeyResponse>;


Why do we have to typecast this? Is there any way to avoid this? (edit: see below)

Fixed as suggested.

mcmire · 2023-05-08T20:14:21Z

src/ledger-iframe-bridge.ts

+  }
+
+  async #deviceActionMessage(
+    action:


The types you've specified here would allow an invalid combination of arguments to be provided. For instance, you could pass an action of IFrameMessageAction.LedgerSignTransaction, but params of LedgerSignTypedDataParams. What do you think about using overloads to specify the desired combinations instead? That might prevent the typecasting above.

Thanks. I was thinking so hard on how to make typescript infer the return type with generics that I completely overlooked using overloads, which should work perfectly in this case.

cryptotavares · 2023-06-01T21:39:33Z

src/ledger-iframe-bridge.ts

+      // If the iframe isn't loaded yet, let's store the desired transportType value and
+      // optimistically return a successful promise
+      if (!this.iframeLoaded) {
+        this.delayedPromise = {


Why do we need a delayedPromise for this? Couldn't we just persist the desiredTransportType in case the iframe is not loaded?

Ah I just noticed that we are just moving it from ledger-keyring.. nevermind then, let's keep this PR as simple as possible. Code improvements can be done in subsequent PRs

The delayed promise was needed because all that logic was in the constructor, which cannot be asynchronous.

Now that logic is moved to an asynchronous init method, it can be removed, but that should be done in a subsequent PR.

cryptotavares · 2023-06-01T21:55:22Z

src/ledger-keyring.ts

+    }
+
+    if (updateHdk && payload.chainCode) {
+      this.hdk.publicKey = Buffer.from(payload.publicKey, 'hex');


we have changed the payload.publicKey type checking from runtime to build time. Just want to make sure that that is enough.

Should be if we trust the return types from ledger SDK.

Before, multiple checks where needed due to #sendMessage using an IFrameMessageResponse that would not consider the action, and therefore return just an union type. Now we have a more accurate IFrameMessageResponse<TAction extends IFrameMessageAction> that is based on the action sent to ledger.

Those checks were not there before this package was updated to Typescript, they were only added to avoid casting types.

cryptotavares

Left a very small comment but looks good to me!

Gudahtt · 2023-06-14T15:34:14Z

src/ledger-iframe-bridge.test.ts

+  }
+
+  beforeEach(async function () {
+    bridge = new LedgerIframeBridge();


Nit: Generally we prefer to use helper functions and construct things per-test. This ensures the tests are readable individually without consulting the beforeEach, and it removes barriers to testing scenarios that have a different setup.

Gudahtt

LGTM!

bergarces marked this pull request as ready for review November 17, 2022 13:05

bergarces requested a review from a team as a code owner November 17, 2022 13:05

bergarces commented Nov 17, 2022

View reviewed changes

base-ledger-keyring.js Outdated Show resolved Hide resolved

bergarces commented Nov 17, 2022

View reviewed changes

base-ledger-keyring.js Outdated Show resolved Hide resolved

bergarces force-pushed the bridge-refactor branch from 850d4c4 to 3a58b64 Compare November 17, 2022 14:13

legobeat reviewed Nov 17, 2022

View reviewed changes