🏗️ [Support] Request device access within `HWDeviceProvider` #7440

thesan · 2024-07-26T15:34:55Z

✅ Checklist

npx changeset was attached.
Covered by automatic tests.
Impact of the changes:
- ...

📝 Description

Factor all device interactions out of the Trustchain SDK into a new HWDeviceProvider. This is done in order to help unit testing of the Trustchain SDK later on and to better separate concerns.

Initialise the HWDeviceProvider with the withDevice function from @ledgerhq/live-common/hw/deviceAccess. The device id is then passed to the method of the Trustchain SDK which interact with the device.

❓ Context

LIVE-13211

🧐 Checklist for the PR Reviewers

The code aligns with the requirements described in the linked JIRA or GitHub issue.
The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
There are no undocumented trade-offs, technical debt, or maintainability issues.
The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
Any new dependencies have been justified and documented.
Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

vercel · 2024-07-26T15:35:00Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
web-tools	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 6, 2024 4:24pm

4 Skipped Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-docs	⬜️ Ignored (Inspect)	Visit Preview	Aug 6, 2024 4:24pm
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	Aug 6, 2024 4:24pm
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Aug 6, 2024 4:24pm
react-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Aug 6, 2024 4:24pm

gre

it's a great iteration but i would like to suggest a simplification on the SDK interface.

if i understand correctly, we're abstracting out the need to pass transport in methods of the SDK, by instanciating the SDK with a way to create the transport instead (giving an implementation of withTransport)

on top of this, in your rework, we also need to provide a deviceId observable, this is the part where i'm a bit more lost both technically and conceptually: in term of lifecycle, some methods need the device, some don't, and they only need it in context of that method, so i think it would be preferable to keep a concept of deviceId contextual to sdk methods, as being passed as explicit parameter to these methods to express the fact that "hey i will need a device id because i will need the device". This is a pattern actually quite similar to what was done on the Coin Framework AccountBridge : we pass a deviceId (and no longer a transport), so we can still keep what you did on the withDevice implementation abstraction, but i would prefer to simplify out the need to pass in a global "device id observable" but instead just pass in the device.id (instead of the transport) where it's necessary (which also makes the diff a bit less impacting with current's sdk signature)

so each time we had "transport", it becomes "deviceId", for instance, I mean this change:

- await runWithDevice(device.id, async transport => {
+ await (
  sdk.getOrCreateTrustchain(
-   transport,
+   device.id,
    memberCreds,
    callbacks
  )
)

I think it may be especially important for LLM because the observable pattern will not work on LLM: we manage multiple devices on LLM unlike LLD where we always have a singleton device. so this deviceId observable lifecycle don't really exists. it also doesn't really exist on the web tools.

thesan · 2024-07-29T14:49:36Z

so each time we had "transport", it becomes "deviceId"

Got it! Thank you for having a look. I'll make these changes and apply the rework to LLM and web tools too.

gre · 2024-08-06T12:46:27Z

apps/ledger-live-desktop/src/newArch/features/WalletSync/hooks/useRemoveMember.ts

+        const newTrustchain = await sdkRef.current.removeMember(
+          deviceRef.current?.deviceId,
+          trustchainRef.current as Trustchain,
+          memberCredentialsRef.current as MemberCredentials,


we shouldn't have to cast these, instead we may want to have the same trick you did with the if(!deviceRef.current) throw
you can have a catch all if (!a || !b || !c) throw new Error("unexpected missing args") for instance

also you can do device.current.deviceId as you just have proven to TS that it's settled. if TS can't manage it, then it"s preferable to do:

const device = deviceRef.current if (!device) ... removeMember(device.deviceId,

Side question but does it mean removeMember accepts a falsy deviceId? it probably shouldn't 🤔

No I probably just forgot the ? from some intermediary code.

Got it for the other castings I agree it's not the best.

BTW are these references just to avoid the missing dependencies linting error in the use callbacks/effects ?

using React ref usually is done when we want to prevent an effect to be retriggered over and over, and when we instead just need to access the "current" state once and not to "trigger each time it changes".
i think @mcayuelas-ledger can comment more on this as he did this part, but i suspect we had the effect double triggering in the past, ending up on race condition with the device (like doing two removeMember in a row)

gre · 2024-08-06T12:47:13Z

apps/ledger-live-mobile/src/newArch/features/WalletSync/hooks/useAddMember.ts

-        });
+        const trustchainResult = await sdk.getOrCreateTrustchain(
+          device?.deviceId ?? "",
+          memberCredentialsRef.current as MemberCredentials,


same casting we could get rid of (but what about the ?.deviceId one ?)

gre · 2024-08-06T12:48:14Z

apps/ledger-live-mobile/src/newArch/features/WalletSync/hooks/useRemoveMember.ts

+          device.deviceId,
+          trustchain,
+          memberCredentials,
+          member,


( 👍 on mobile we had cleaner code I hope we can also reach on LLD, see here, no casting )

gre · 2024-08-06T12:49:14Z

libs/trustchain/tests/scenarios/removedMemberEjectedOnDeletedTrustchain.ts

@@ -13,7 +13,7 @@ export async function scenario(transport: Transport, { sdkForName }: ScenarioOpt
  const member2 = { name: name2, id: member2creds.pubkey, permissions: 0xffffffff };

  // auth with the device and init the first trustchain
-  const { trustchain } = await sdk1.getOrCreateTrustchain(transport, member1creds);
+  const { trustchain } = await sdk1.getOrCreateTrustchain("foo", member1creds);


could we just use "" instead of foo? or maybe we can also make a deviceId constant somewhere to explicit what this is.

I ended up passing the device id to the test scenario instead of the transport

thesan · 2024-07-31T07:23:15Z

libs/trustchain/src/HWDeviceProvider.ts

+   * TODO withDevice should be imported statically from @ledgerhq/live-common/hw/deviceAccess
+   *
+   * but ATM making @ledgerhq/live-common a dependency of @ledgerhq/trustchain causes:
+   * > Turbo error: Invalid package dependency graph: cyclic dependency detected:
+   * >   @ledgerhq/trustchain,@ledgerhq/live-wallet,@ledgerhq/live-common
+   *
+   * Maybe hw/deviceAccess.ts and hw/index.ts could be moved to @ledgerhq/devices
+   * This would break the cyclic dependency as @ledgerhq/live-common would depend on @ledgerhq/devices
+   * but not the other way around.


@gre @KVNLS what do you think of this suggestion ?

I prefer to keep it as is, otherwise it involves much work to be done. ( + I prefer that you inject the withDevice as a parameter rather than importing it in the trustchain sdk)

the idea to move hw/* and notably the deviceAccess / index files of it to a library is a good idea,
this goes into the general effort of moving things out of live-common,
but i think it's the scope of device team tho.
also the impact is quite important in that many parts depends on this. but this could be discussed with that team.

and yeah, no library should depends on live-common, it's live-common that can depends on library, and on this of this the general goal is to move things out of live-common, not to add more depends to it.

so in the meantime we should indeed inject function we needs. similarly i sometimes had to inject getAccountBridge as a standalone function on the wallet library, instead of depending on live-common which is the one currently implementing it

thesan · 2024-08-06T14:20:48Z

apps/ledger-live-desktop/src/newArch/features/WalletSync/hooks/useRemoveMember.ts

+        const newTrustchain = await sdkRef.current.removeMember(
+          deviceRef.current?.deviceId,
+          trustchainRef.current as Trustchain,
+          memberCredentialsRef.current as MemberCredentials,


No I probably just forgot the ? from some intermediary code.

Got it for the other castings I agree it's not the best.

thesan · 2024-08-06T14:33:06Z

apps/ledger-live-desktop/src/newArch/features/WalletSync/hooks/useRemoveMember.ts

+        const newTrustchain = await sdkRef.current.removeMember(
+          deviceRef.current?.deviceId,
+          trustchainRef.current as Trustchain,
+          memberCredentialsRef.current as MemberCredentials,


BTW are these references just to avoid the missing dependencies linting error in the use callbacks/effects ?

thesan · 2024-08-06T16:22:59Z

libs/trustchain/tests/scenarios/removedMemberEjectedOnDeletedTrustchain.ts

@@ -13,7 +13,7 @@ export async function scenario(transport: Transport, { sdkForName }: ScenarioOpt
  const member2 = { name: name2, id: member2creds.pubkey, permissions: 0xffffffff };

  // auth with the device and init the first trustchain
-  const { trustchain } = await sdk1.getOrCreateTrustchain(transport, member1creds);
+  const { trustchain } = await sdk1.getOrCreateTrustchain("foo", member1creds);


I ended up passing the device id to the test scenario instead of the transport

github-actions bot assigned thesan Jul 26, 2024

live-github-bot bot added the desktop Has changes in LLD label Jul 26, 2024

gre suggested changes Jul 29, 2024

View reviewed changes

thesan force-pushed the support/trustchain-sdk-refactor-transport branch from 84fb2cf to 3e63da4 Compare July 30, 2024 14:53

thesan changed the base branch from support/trustchain-sdk-refactor to develop July 30, 2024 14:53

vercel bot had a problem deploying to Preview – web-tools July 30, 2024 14:56 Failure

thesan force-pushed the support/trustchain-sdk-refactor-transport branch from 3e63da4 to e5596ce Compare July 30, 2024 15:04

vercel bot had a problem deploying to Preview – web-tools July 30, 2024 15:07 Failure

vercel bot had a problem deploying to Preview – web-tools July 30, 2024 15:19 Failure

thesan force-pushed the support/trustchain-sdk-refactor-transport branch from 549d3be to 5de26cb Compare July 30, 2024 16:15

thesan changed the base branch from develop to support/trustchain-sdk-refactor July 30, 2024 16:16

thesan marked this pull request as ready for review August 2, 2024 17:58

thesan requested a review from a team as a code owner August 2, 2024 17:58

mcayuelas-ledger previously approved these changes Aug 5, 2024

View reviewed changes

Theophile Sandoz added 12 commits August 5, 2024 18:55

Initialize the HWDeviceProvider with an Observable<WithDevice>

fa43dad

Update useTrustchainSdk accordingly

1dad550

Fix tests

7f37502

Pass an observable of the device id instead of a function

22c9ffb

Factor hwDeviceProvider.withHw calls into a function

cb08824

Pass the device id to sdk methods

ffce76b

Remove the "Config" object to get the SDK

80bb377

Apply changes to LLM and to the web tool

d1343f8

Fix typo in a comment

f07c897

Fix tests and scenarios

09ef660

Re-generate failing tests

166937a

Fix the web tools build

07fe865

thesan dismissed mcayuelas-ledger’s stale review via 07fe865 August 5, 2024 18:31

thesan force-pushed the support/trustchain-sdk-refactor-transport branch from 4307933 to 07fe865 Compare August 5, 2024 18:31

vercel bot deployed to Preview – web-tools August 5, 2024 18:35 View deployment

Theophile Sandoz added 2 commits August 6, 2024 09:52

Fix LLM build

96b4e7a

Update changelog

60f51d1

thesan changed the title ~~🏗️ [Support] Fully handle hw device interaction in HWDeviceProvider~~ 🏗️ [Support] Request device access within HWDeviceProvider Aug 6, 2024

gre suggested changes Aug 6, 2024

View reviewed changes

Theophile Sandoz added 2 commits August 6, 2024 16:55

Avoid type castings in the SDK method calls

1df9475

Actually apply the SDK refactor to the test scenarios

ee61937

thesan commented Aug 6, 2024

View reviewed changes

thesan requested a review from gre August 6, 2024 16:24

vercel bot deployed to Preview – web-tools August 6, 2024 16:24 View deployment

gre approved these changes Aug 8, 2024

View reviewed changes

thesan merged commit bc044e4 into develop Aug 8, 2024
55 of 56 checks passed

thesan deleted the support/trustchain-sdk-refactor-transport branch August 8, 2024 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏗️ [Support] Request device access within `HWDeviceProvider` #7440

🏗️ [Support] Request device access within `HWDeviceProvider` #7440

thesan commented Jul 26, 2024 •

edited

Loading

vercel bot commented Jul 26, 2024 •

edited

Loading

gre left a comment •

edited

Loading

thesan commented Jul 29, 2024

gre Aug 6, 2024

thesan Aug 6, 2024

thesan Aug 6, 2024

gre Aug 8, 2024 •

edited

Loading

gre Aug 6, 2024

gre Aug 6, 2024

gre Aug 6, 2024

thesan Aug 6, 2024

thesan Jul 31, 2024

KVNLS Aug 7, 2024

gre Aug 8, 2024 •

edited

Loading

thesan Aug 6, 2024

thesan Aug 6, 2024

thesan Aug 6, 2024

🏗️ [Support] Request device access within HWDeviceProvider #7440

🏗️ [Support] Request device access within HWDeviceProvider #7440

Conversation

thesan commented Jul 26, 2024 • edited Loading

✅ Checklist

📝 Description

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented Jul 26, 2024 • edited Loading

gre left a comment • edited Loading

Choose a reason for hiding this comment

thesan commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gre Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gre Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🏗️ [Support] Request device access within `HWDeviceProvider` #7440

🏗️ [Support] Request device access within `HWDeviceProvider` #7440

thesan commented Jul 26, 2024 •

edited

Loading

vercel bot commented Jul 26, 2024 •

edited

Loading

gre left a comment •

edited

Loading

gre Aug 8, 2024 •

edited

Loading

gre Aug 8, 2024 •

edited

Loading