Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add device API #2864

Merged
merged 11 commits into from
Nov 6, 2024
Merged

Add device API #2864

merged 11 commits into from
Nov 6, 2024

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Oct 25, 2024

No description provided.

Copy link

socket-security bot commented Oct 25, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@ledgerhq/[email protected] None +1 259 kB ldg-github-ci
npm/@ledgerhq/[email protected] None 0 208 kB ldg-github-ci
npm/@ledgerhq/[email protected] None +1 169 kB ldg-github-ci

View full report↗︎

@Mrtenz Mrtenz changed the title Add device API interfaces Add device API Nov 1, 2024
/**
* The type of the device.
*/
export type DeviceType = 'hid' | 'bluetooth';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I don't see the benefit in this case.

type?: Type,
): Type extends DeviceType ? Struct<ScopedDeviceId<Type>> : Struct<DeviceId> {
return refine(string(), 'device ID', (value) => {
if (type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow what this struct is meant to validate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it just validates that a string consists of three parts (1:2:3).

/**
* The vendor ID of the device.
*/
vendorId?: number;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we just expand this in the future for other devices in one big object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's some type-specific filters later, we could do a discriminated union type, with filters specific to each type.

{
  type: 'some-new-type',
  filters: FiltersForSomeNewType[];
}

/**
* The result returned by the `snap_readDevice` method.
*/
export type ReadDeviceResult = Hex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this implying that we will call bytesToString on the return value from the device? Curious how that performs versus just passing an array versus whatever else

/**
* The data to write to the device.
*/
data: Hex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as read with regards to what we want this type to be and what is most performant

params: {
type: 'hid',
id: this.device.id,
data: bytesToHex(block),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How large are these blocks? It might help us make a decision on the type for data

@Mrtenz Mrtenz marked this pull request as ready for review November 6, 2024 11:17
@Mrtenz Mrtenz requested a review from a team as a code owner November 6, 2024 11:17
@Mrtenz Mrtenz merged commit 896b91c into fb/devices Nov 6, 2024
146 of 167 checks passed
@Mrtenz Mrtenz deleted the mrtenz/device-api branch November 6, 2024 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants