feat: add mobile bridge #198

stanleyyconsensys · 2023-10-04T09:19:05Z

add mobile bridge
decouple the transport middleware from the bridge, for mobile, the transport middleware is a interface to connect to ledger eth app and the transport layer; for desktop, it is refer to iframe
adding setter / getter method for metadata from the bridge as a generic method to store different data on different bridge, hence the serialize and deserialize method from the highlevel module will serialize/deserialize the metadata together with the low level module (bridge)
move bridgeUrl into low level module (bridge)
enhance time complexity in deserialize, #migrateAccountDetails method by use Set, rather than array include
reduce the number of traversal in removeAccount
remove exportAccount due to eth keyring has throw an exception, should not create another exception type

- add mobile bridge - decouple the transport middleware from the bridge, for mobile, the transport middleware is a interface to connect to ledger eth app and the transport layer; for desktop, it is refer to iframe - adding setter / getter method for metadata from the bridge as a generic method to store different data on different bridge, hence the serialize and deserialize method from the highlevel module will serialize/deserialize the metadata together with the low level module (bridge) - move bridgeUrl into low level module (bridge) - enhance time complexity in deserialize, #migrateAccountDetails method by use Set, rather than array include - reduce the number of traversal in removeAccount - remove exportAccount due to eth keyring has throw an exception, should not create another exception type

mikesposito

I think this solution of implementing a new bridge for mobile alongside the LedgerIframeBridge is very functional and practical. I left some minor comments for some API details, but it overall looks good

src/ledger-bridge.ts

mikesposito · 2023-10-04T14:10:30Z

src/ledger-iframe-bridge.ts

+  setOptions(opts: LedgerIframeBridgeOptions): void {
+    this.bridgeUrl = opts.bridgeUrl ?? BRIDGE_URL;
+  }


When calling setOptions and changing the bridgeUrl we should do something to load another iframe, destroying the old one: this looks like destroying the bridge and re-initialising it (so that we don't end up having multiple event listeners that could lead to unpredictable results)

i thought setOptions and getOption is mainly to support serialize deserialize, thats why i was using metadata, rather than option

if setOptions has to kick off some side action, it may has some unexpected behaviour in current implementation, which setOptions is couple with deserialize

one of the example
submitEncryptionKey -> unlockKeyrings -> #restoreKeyring-> #newKeyring -> deserialize + init
it will deserialize the keyring object , then trigger init

and some high level methods (verifySeedPhrase), they may just wanna to update the data for keyring via deserialize, but do nothing

may be we can just leave setOptions as a setter, for side action, just do it separately? (for iframe usecase, call init after setOptions) ?

or

we create methods deserializeData, serializeData in bridge to support high level module deserialize and serialize, and leave the setOptions and getOptions to something that fit to your idea?

the advantage is :

bridge can have custom data to serialize , rather than couple with its options

if configuration change will auto wire up with side action, i will prefer using setXXXX (e.g setBridgeUrl) rather than setOptions?

Suggested change

setOptions(opts: LedgerIframeBridgeOptions): void {

this.bridgeUrl = opts.bridgeUrl ?? BRIDGE_URL;

}

someOtherData = 0;

async deserializeData(serializeData: Record<string, unknown>) {

for (const key in serializeData) {

if (key === 'bridgeUrl') {

this.bridgeUrl = serializeData[key] as string;

}

if (key === 'someOtherData') {

this.someOtherData = serializeData[key] as number;

}

}

}

async serializeData(): Promise<Record<string, unknown>> {

return {

"someOtherData": this.someOtherData,

"bridgeUrl": this.bridgeUrl

}

}

async setBridgeUrl(bridgeUrl): Promise<void> {

if (bridgeUrl && this.bridgeUrl !== bridgeUrl) {

this.bridgeUrl = bridgeUrl

await this.destroy();

await this.init();

}

}

mikesposito · 2023-10-04T14:54:37Z

src/ledger-mobile-bridge.ts

+  setOptions(opts: LedgerMobileBridgeOptions): void {
+    this.deviceId = opts.deviceId ?? '';
+  }


what happens to the bridge is the deviceId here is changed to another value? do we need to take some side-actions or we are good to go?

i leave it to setDeviceId, and setOption mainly set back the data from the deserialize

legobeat · 2023-10-26T01:57:42Z

Blocked by:

1.0.1 #203
- (and maybe 2.0.0 #202 in order to ship deps: @metamask/eth-sig-util@^6.0.1->^7.0.0 #205 and deps: @metamask/utils@^5.0.0->^8.2.0 #206 first ?)

legobeat · 2023-10-26T02:04:11Z

move bridgeUrl into low level module (bridge)

enhance time complexity in deserialize, #migrateAccountDetails method by use Set, rather than array include
reduce the number of traversal in removeAccount
remove exportAccount due to eth keyring has throw an exception, should not create another exception type

These changes sound like they are orthogonal to adding the mobile bridge - would it be possible to break them out as separate PR(s) to keep the changes in this PR focused? :)

Could also consider breaking this apart in at least two major PRs:

Refactor (decouple the transport middleware from the bridge, add getter/setter - maybe these should be separate as well?)
- Apart from the changed behavior for bridgeUrl (which can be broken out), these should be non-breaking and not blocked by anything else from what I can see.
Actually add mobile bridge, being enabled by 1.

stanleyyconsensys · 2023-10-26T02:16:59Z

Apart

agree, i will create

1 PR focus on refactor to make the bridgeUrl become a generic option parameter when init the ledger keyring for iframe
1 PR focus on adding mobile as a bridge

but i have a doubt on 1 thing, is there any side affect if i change the keyring serialization structure?
from

{
      hdPath: string;
      accounts: string[];
      accountDetails: Record<string, AccountDetails>;
      accountIndexes: Record<string, number>;
      implementFullBIP44: boolean;
      bridgeUrl: string;
}

to

{
      hdPath: string;
      accounts: string[];
      accountDetails: Record<string, AccountDetails>;
      accountIndexes: Record<string, number>;
      implementFullBIP44: boolean;
      bridgeOptions: Record<string, unknown>;
}

legobeat · 2023-10-26T02:42:32Z

@stanleyyconsensys As I read it here, that would be a breaking change (which in itself isn't necessarily a problem, just have to be more considerate about timing and ordering for merging and releasing such changes).

One way we could make it non-breaking would be something like:

{
  hdPath: string;
  accounts: string[];
  accountDetails: Record<string, AccountDetails>;
  accountIndexes: Record<string, number>;
  implementFullBIP44: boolean;
} & (
  {
    bridgeUrl: string;
  } | {
    bridgeOptions: SomeNewBridgeOptionsType; // we should be able to do stricter than Record<string, unknown>?
  }
)

...and then the implementation would do something like:

if(opts.bridgeOptions) {
  if (opts.bridgeUrl) {
    // error
  }
  // new behavior...
  return;
}
if (opts.bridgeUrl) {
  console.warn('some message indicating that this is now deprecated and will be removed in a future version, users should switch to bridgeOptions')
  // old behavior...
 return;
}
// error because missing conf...

that way we can allow users to stay on old behavior and migrate , by paying a price of increased complexity. Then the bridgeUrl path could be fully removed in a later semver-major release to simplify it again.

stanleyyconsensys · 2023-10-26T04:59:40Z

@legobeat 1 last question regarding on making changes non breaking

as i found that the current bridge implementation on extension is really old

extension yarn.log

"@metamask/eth-ledger-bridge-keyring@npm:^0.15.0":
  version: 0.15.0
  resolution: "@metamask/eth-ledger-bridge-keyring@npm:0.15.0"
  dependencies:
    "@ethereumjs/tx": "npm:^4.1.1"
    eth-sig-util: "npm:^2.0.0"
    ethereumjs-util: "npm:^7.0.9"
    hdkey: "npm:0.8.0"
  checksum: d856b820806bfd3a9a030b2116b082b5089531e21a5e4ca53e5af027d83b2935013a853a5c18bc6c374555467d4e8e9f1a1712c263e891a7cd3cf0a4388968ae
  languageName: node
  linkType: hard

it is still using a version that tight couple everything
extension version:

class LedgerBridgeKeyring extends EventEmitter {
    constructor (opts = {}) {
    }

current 1.0 version

class LedgerBridgeKeyring extends EventEmitter {
    constructor ({ bridge }: { bridge: LedgerBridge }) {
    }

the new version

class LedgerBridgeKeyring extends EventEmitter {
    constructor ({ bridge }: { bridge: LedgerBridge<Record<string, unknown>> }) {
    }

so is it still a case that we need to take care the breaking changes?

or becoz it is public repo, we assume every one is using 1.0, hence we have to make a transition step, and make sure the client who using the library should not necessary to change their 1.0 implementation?

just wanna confirm the reason, hence i can refactor the class in a correct way

1 more thing, if what we are referring on non breaking is something that i mentioned above, we should also taking care the use case of how client implementing the bridgeUrl?

e.g, in that case we have to keep the bridgeUrl in high level class

const ledger = new LedgerBridgeKeyring({bridge})
ledger.bridgeUrl = "https://some_custom_url"

stanleyyconsensys added 2 commits October 4, 2023 17:17

fix: remove unuse type

fe30891

mikesposito reviewed Oct 4, 2023

View reviewed changes

src/ledger-bridge.ts Outdated Show resolved Hide resolved

src/ledger-bridge.ts Outdated Show resolved Hide resolved

src/ledger-bridge.ts Outdated Show resolved Hide resolved

src/ledger-bridge.ts Outdated Show resolved Hide resolved

src/ledger-bridge.ts Show resolved Hide resolved

fix: PR comment

7661b2e

mikesposito reviewed Oct 4, 2023

View reviewed changes

stanleyyconsensys added 10 commits October 6, 2023 16:37

fix: adding unit test

43db215

fix: update style

4972033

fix: update jest config

af0d1b8

fix: add back missing export

67f3607

chore: add connect method

b7fb521

fix: force deviceId to be private props

99e5646

fix: lint style

5d047fc

fix: update highlevel method to highlevel module

c5d2048

Merge branch 'main' into feat/add-mobile-bridge

c556846

fix: update testing score

ed2cf45

stanleyyconsensys marked this pull request as ready for review October 12, 2023 03:09

stanleyyconsensys requested a review from a team as a code owner October 12, 2023 03:09

stanleyyconsensys added 3 commits October 12, 2023 17:17

fix: update dispose should not execute when not connected

8ed0e74

Merge branch 'main' into feat/add-mobile-bridge

1236464

Update yarn.lock

2b8d2a8

legobeat mentioned this pull request Nov 30, 2023

Feat: refactor to enable additional bridge #210

Merged

legobeat marked this pull request as draft November 30, 2023 07:03

legobeat added the DO NOT MERGE label Nov 30, 2023

stanleyyconsensys closed this Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add mobile bridge #198

feat: add mobile bridge #198

stanleyyconsensys commented Oct 4, 2023

mikesposito left a comment

mikesposito Oct 4, 2023

stanleyyconsensys Oct 5, 2023 •

edited

Loading

mikesposito Oct 4, 2023

stanleyyconsensys Oct 4, 2023

legobeat commented Oct 26, 2023 •

edited

Loading

legobeat commented Oct 26, 2023 •

edited

Loading

stanleyyconsensys commented Oct 26, 2023

legobeat commented Oct 26, 2023 •

edited

Loading

stanleyyconsensys commented Oct 26, 2023 •

edited

Loading

-  setOptions(opts: LedgerIframeBridgeOptions): void {
-    this.bridgeUrl = opts.bridgeUrl ?? BRIDGE_URL;
-  }
+  someOtherData = 0;
+  async deserializeData(serializeData: Record<string, unknown>) {
+      for (const key in serializeData) {
+          if (key === 'bridgeUrl') {
+            this.bridgeUrl = serializeData[key] as string;
+          }
+          if (key === 'someOtherData') {
+            this.someOtherData = serializeData[key] as number;
+          }
+      }
+  }
+  async serializeData(): Promise<Record<string, unknown>> {
+    return {
+      "someOtherData": this.someOtherData,
+      "bridgeUrl": this.bridgeUrl
+    }
+  }
+  async setBridgeUrl(bridgeUrl): Promise<void> {
+    if (bridgeUrl && this.bridgeUrl !== bridgeUrl) {
+       this.bridgeUrl = bridgeUrl
+       await this.destroy();
+       await this.init();
+   }
+ }

feat: add mobile bridge #198

feat: add mobile bridge #198

Conversation

stanleyyconsensys commented Oct 4, 2023

mikesposito left a comment

Choose a reason for hiding this comment

mikesposito Oct 4, 2023

Choose a reason for hiding this comment

stanleyyconsensys Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

mikesposito Oct 4, 2023

Choose a reason for hiding this comment

stanleyyconsensys Oct 4, 2023

Choose a reason for hiding this comment

legobeat commented Oct 26, 2023 • edited Loading

Blocked by:

legobeat commented Oct 26, 2023 • edited Loading

stanleyyconsensys commented Oct 26, 2023

legobeat commented Oct 26, 2023 • edited Loading

stanleyyconsensys commented Oct 26, 2023 • edited Loading

stanleyyconsensys Oct 5, 2023 •

edited

Loading

legobeat commented Oct 26, 2023 •

edited

Loading

legobeat commented Oct 26, 2023 •

edited

Loading

legobeat commented Oct 26, 2023 •

edited

Loading

stanleyyconsensys commented Oct 26, 2023 •

edited

Loading