BT transport #11855

Nodonisko · 2024-04-03T09:17:28Z

Description

POC of BT transport layer in Suite Mobile. Implemented using react-native-ble-plx lib which is used by all crypto apps that I explored. It's also nice for 3rd parties because they won't need to add extra native dependency and it will also help us avoid some conflicts (hopefully) that could occur when multiple libraries for BT access are used.

Also please keep in mind this is very first version, even there is lot edge cases handled, some stuff is missing for example erasing bonds (compared to @szymonlesisz PR). I also did not encountered any packet duplication (but I guess it can happen), but I think it's fine to omit it now and wait for new protocol which will solve it anyway.

Video: https://satoshilabs.slack.com/files/U02RLDSFCPP/F071BCP430S/screen-20240430-154355.mp4

Summary of changes

`@trezor/transport-native-ble`

There is nativeBleManager.ts which is acting like desktop api in @szymonlesisz branch for desktop. It must be single instance and we must be able to import this instance in app UI to handle stuff like connecting, scanning etc.
2.bleApi.ts - it's basically BT version of usbApi

`@suite-native/app`

EXPO_PUBLIC_BLUETOOTH_ENABLED env variable - this needs to be true before build to include some native code for requesting permission for BT (libraries will be included in all types build anyway, but at least it won't show extra permission in stores). By default this will be enabled for Debug and Develop builds so anyone can turn ON BT for testing.

`@suite-native/bluetooth`

Basically UI for BT (Scanning, connecting, permissions, errors, edge cases...)
Feature flag in @suite-native/bluetooth - This is special kind of static feature flag because it must be persisted because app must be restarted when it's changed. Once we will support multi-transport restart woudn't be neccessary.

Related Issue

Resolve #11751

Screenshots:

socket-security · 2024-04-30T12:50:14Z

👍 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.

View full report↗︎

Nodonisko · 2024-04-30T14:13:33Z

packages/transport-native-ble/src/api/bleApi.ts

+
+export class BleApi extends AbstractApi {
+    chunkSize = 244;
+    devices: BleDevice[] = [];


unused, remove

PeKne

Looks great, can't wait to test it

PeKne · 2024-05-02T07:38:39Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+    }
+
+    private async enumerate() {
+        const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs);


Can not be this.bleManager accessed instead? Since the _bleManager should be initialized only once, it should be safe.

Suggested change

const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs);

const connectedDevices = this.bleManager.connectedDevices(devicesUUIDs);

PeKne · 2024-05-02T07:40:53Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+        this.bleManager.startDeviceScan(devicesUUIDs, scanOptions, (error, scannedDevice) => {
+            if (error) {
+                debugLog('Scan error');
+                console.error(error);


Should not this be a debugLog?

PeKne · 2024-05-02T07:46:11Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+            // await awaitsBleOn(bleManagerInstance());
+
+            // Returns a list of known devices by their identifiers
+            const devices = await bleManagerInstance().devices([deviceOrId]);


Suggested change

const devices = await bleManagerInstance().devices([deviceOrId]);

const devices = await this.bleManager.devices([deviceOrId]);

PeKne · 2024-05-02T07:46:29Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+                // Returns a list of the peripherals currently connected to the system
+                // which have discovered services, connected to system doesn't mean
+                // connected to our app, we check that below.
+                const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs);


Suggested change

const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs);

const connectedDevices = await this.bleManager.connectedDevices(devicesUUIDs);

PeKne · 2024-05-02T07:48:55Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+
+                // Nb ConnectionOptions dropped since it's not used internally by ble-plx.
+                try {
+                    device = await bleManagerInstance().connectToDevice(deviceOrId, connectOptions);


this.bleManager

PeKne · 2024-05-02T08:20:21Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+        let writeCharacteristic: Characteristic | undefined;
+        let notifyCharacteristic: Characteristic | undefined;
+        for (const characteristic of characteristics) {
+            if (characteristic.isWritableWithoutResponse) {


are characteristic UUIDs static or dynamic? I'm wondering if this matching should not be done based on characteristic UUID. But it would be possible only if all the Trezors would have the write/notify characteristic with the same UUID. Does it work like that or is the UUID assigned dynamicaly?

Of course no need to change it in this PR since the prototype is still in development.

PeKne · 2024-05-02T08:22:41Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+        return this.appConnectedDevices.map(d => d.bleDevice);
+    };
+
+    public findDevice = (deviceId: string) => {


nit for better clarity:

Suggested change

public findDevice = (deviceId: string) => {

public findConnectedDevice = (deviceId: string) => {

PeKne · 2024-05-02T09:23:27Z

suite-native/discovery/src/discoveryConfigSlice.ts

@@ -76,6 +76,11 @@ export const selectDiscoverySupportedNetworks = memoizeWithArgs(
    (state: DeviceRootState, areTestnetsEnabled: boolean) =>
        pipe(
            selectDeviceSupportedNetworks(state),
+            symbols => {


leftover that should be removed?

PeKne · 2024-05-02T09:23:56Z

suite-native/discovery/src/discoveryMiddleware.ts

@@ -17,6 +18,10 @@ export const prepareDiscoveryMiddleware = createMiddlewareWithExtraDeps(
            dispatch(discoveryActions.removeDiscovery(action.payload.state));
        }

+        if (action.type.startsWith(DISCOVERY_MODULE_PREFIX)) {


debug leftover

PeKne · 2024-05-02T09:24:08Z

suite-native/discovery/src/discoveryThunks.ts

@@ -474,6 +474,8 @@ export const createDescriptorPreloadedDiscoveryThunk = createThunk(
            return;
        }

+        console.log('createDescriptorPreloadedDiscoveryThunk', deviceState, supportedNetworks);


remove this

vytick · 2024-05-02T11:22:04Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+
+export const NUS_SERVICE_UUID = '6e400001-b5a3-f393-e0a9-e50e24dcca9e';
+// const NUS_CHARACTERISTIC_NOTIFY = '6e400003-b5a3-f393-e0a9-e50e24dcca9e';
+// const NUS_CHARACTERISTIC_TX = '6e400002-b5a3-f393-e0a9-e50e24dcca9e';


vytick · 2024-05-02T11:29:24Z

packages/transport-native-ble/src/api/bleApi.ts

+        };
+    }
+
+    public async enumerate() {


enumerateDevices ?

vytick · 2024-05-02T11:33:56Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+
+        if (typeof deviceOrId === 'string') {
+            debugLog(`Trying to open device: ${deviceOrId}`);
+            // await awaitsBleOn(bleManagerInstance());


to be deleted?

vytick · 2024-05-02T11:42:42Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+        let characteristics: Characteristic[] =
+            await device.characteristicsForService(NUS_SERVICE_UUID);
+
+        // debugLog('Characteristics: ', JSON.stringify(characteristics));


vytick · 2024-05-14T20:47:13Z

suite-native/bluetooth/src/components/BluetoothAdapterStateManager.tsx

+    }
+
+    if (bluetoothAdapterState === AdapterState.Unsupported) {
+        return <AlertBox title={'Bluetooth Unsupported on this device'} variant="error" />;


please, remove curly braces

vytick · 2024-05-14T20:47:34Z

suite-native/bluetooth/src/components/BluetoothAdapterStateManager.tsx

+        return (
+            <VStack spacing="small">
+                <AlertBox
+                    title={'Bluetooth is turned off. Please turn of Bluetooth to continue.'}


curly braces

vytick · 2024-05-14T20:48:57Z

suite-native/bluetooth/src/components/ScannedDeviceItem.tsx

+import { AlertBox, Box, Button, HStack, Loader, Text, VStack } from '@suite-native/atoms';
+import { Translation } from '@suite-native/intl';
+import { prepareNativeStyle, useNativeStyles } from '@trezor/styles';
+import { BLEScannedDevice, nativeBleManager } from '@trezor/transport-native-ble';


checks failing due to unused imports

mroz22

looks OK, couple of notes.

mroz22 · 2024-06-24T13:23:41Z

packages/transport-native-ble/src/api/bleApi.ts

+        // TODO: implement proper device type
+        return devices.map(d => ({ path: d.id, type: DEVICE_TYPE.TypeT2 }));


yes, we probably don't know the type yet? and also we maybe don't know the unique values that are use to match the type yet?

it might be useful to add return value DescriptorApiLevel to this function. There were some additions to this type, you probably will need to stuff some dummy values (for example product field). they are not that much important for higher layers. But who knows, maybe you have some value like this also available.

mroz22 · 2024-06-24T13:24:31Z

packages/transport-native-ble/src/api/bleApi.ts

+
+const DEBUG_LOGS = true;
+
+const debugLog = (...args: any[]) => {


now it should be possible to pass logger instance to BleApi from outside. So feel free to use it if you find it useful.

mroz22 · 2024-06-24T13:25:38Z

packages/transport-native-ble/src/api/bleApi.ts

+        return this.success(undefined);
+    }
+
+    public async openInternal(_path: string, _first: boolean) {


openInternal is private so it doesn't need to be here imho

mroz22 · 2024-06-24T13:29:32Z

packages/transport-native-ble/src/api/nativeBleManager.ts

+    bleDevice: Device;
+}
+
+// TODO(possibly?): would be nice to have state machine to handle all possible states and transitions


also it would be nice to have this somehow unified with desktop bluetooth implementation. lets agree on some common grounds and ideally use some shared types?

mroz22 · 2024-06-24T13:31:53Z

packages/transport-native-ble/src/logs.ts

+// This is for debug purposes it should be removed once BT goes to production
+export const log = (message: string) => {
+    logs.push(message);
+};


one attempt to unify logs is this https://github.com/trezor/trezor-suite/blob/develop/packages/utils/src/logs.ts#L12

mroz22 · 2024-06-24T13:32:34Z

packages/transport/src/transports/abstractApi.ts

+                // TODO: descriptors are somewhere mutated which causes problems on mobile BLE
+                descriptors: descriptors.map(d => ({ ...d })),


this should be now solved. the mutation happend in background.ts.

Nodonisko changed the title ~~BT transport + discovery~~ BT transport Apr 3, 2024

Nodonisko mentioned this pull request Apr 3, 2024

Bluetooth #11751

Open

Nodonisko force-pushed the feat/bluetooth-mobile branch from b189c0e to 8b7b77a Compare April 10, 2024 18:48

Nodonisko force-pushed the feat/bluetooth-mobile branch 2 times, most recently from 450bb2a to 4f69c75 Compare April 29, 2024 18:53

Nodonisko marked this pull request as ready for review April 30, 2024 12:52

Nodonisko requested review from mroz22 and a team as code owners April 30, 2024 12:52

Nodonisko commented Apr 30, 2024

View reviewed changes

PeKne reviewed May 2, 2024

View reviewed changes

juriczech added the mobile Suite Lite issues and PRs label May 2, 2024

vytick reviewed May 2, 2024

View reviewed changes

vytick reviewed May 14, 2024

View reviewed changes

mroz22 reviewed Jun 24, 2024

View reviewed changes

Nodonisko force-pushed the feat/bluetooth-mobile branch from cf42223 to d2199b1 Compare September 18, 2024 13:23

Nodonisko requested review from szymonlesisz and martykan as code owners September 18, 2024 14:49

Nodonisko force-pushed the feat/bluetooth-mobile branch from b1224e8 to 8146dd4 Compare September 18, 2024 14:50

Nodonisko force-pushed the feat/bluetooth-mobile branch from 859b656 to 34043ec Compare October 11, 2024 13:57

Nodonisko added 2 commits October 23, 2024 15:09

feat(mobile): BT support

aeb6bad

fix: ios BT permissions

5d9f983

Nodonisko force-pushed the feat/bluetooth-mobile branch from 34043ec to 5d9f983 Compare October 23, 2024 15:32

fix(bt): fix connect after rebase

7173e33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BT transport #11855

BT transport #11855

Nodonisko commented Apr 3, 2024 •

edited

Loading

socket-security bot commented Apr 30, 2024 •

edited

Loading

Nodonisko Apr 30, 2024

PeKne left a comment

PeKne May 2, 2024

PeKne May 2, 2024

PeKne May 2, 2024

PeKne May 2, 2024

PeKne May 2, 2024

PeKne May 2, 2024 •

edited

Loading

PeKne May 2, 2024

PeKne May 2, 2024

PeKne May 2, 2024

PeKne May 2, 2024

vytick May 2, 2024

vytick May 2, 2024

vytick May 2, 2024

vytick May 2, 2024

vytick May 14, 2024

vytick May 14, 2024

vytick May 14, 2024

mroz22 left a comment

mroz22 Jun 24, 2024

mroz22 Jun 24, 2024

mroz22 Jun 24, 2024

mroz22 Jun 24, 2024

mroz22 Jun 24, 2024

mroz22 Jun 24, 2024

mroz22 Jun 24, 2024

	const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs);
	const connectedDevices = this.bleManager.connectedDevices(devicesUUIDs);

	const devices = await bleManagerInstance().devices([deviceOrId]);
	const devices = await this.bleManager.devices([deviceOrId]);

	const connectedDevices = await bleManagerInstance().connectedDevices(devicesUUIDs);
	const connectedDevices = await this.bleManager.connectedDevices(devicesUUIDs);

	public findDevice = (deviceId: string) => {
	public findConnectedDevice = (deviceId: string) => {

		// TODO: implement proper device type
		return devices.map(d => ({ path: d.id, type: DEVICE_TYPE.TypeT2 }));


		const DEBUG_LOGS = true;

		const debugLog = (...args: any[]) => {

		// TODO: descriptors are somewhere mutated which causes problems on mobile BLE
		descriptors: descriptors.map(d => ({ ...d })),

BT transport #11855

Are you sure you want to change the base?

BT transport #11855

Conversation

Nodonisko commented Apr 3, 2024 • edited Loading

Description

Summary of changes

@trezor/transport-native-ble

@suite-native/app

@suite-native/bluetooth

Related Issue

Screenshots:

socket-security bot commented Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

PeKne left a 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

PeKne May 2, 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

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

mroz22 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nodonisko commented Apr 3, 2024 •

edited

Loading

`@trezor/transport-native-ble`

`@suite-native/app`

`@suite-native/bluetooth`

socket-security bot commented Apr 30, 2024 •

edited

Loading

PeKne May 2, 2024 •

edited

Loading