Conversation
- Implement trezor hardware support via USB and Bluetooth
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
detekt found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Bumps bitkit-core version to 0.1.44 - Adds SendTransactionSection.kt
# Conflicts: # app/src/main/java/to/bitkit/ui/ContentView.kt # app/src/main/java/to/bitkit/ui/settings/AdvancedSettingsScreen.kt # app/src/main/java/to/bitkit/ui/settings/AdvancedSettingsViewModel.kt
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| throw e | ||
| } | ||
| TrezorDebugLog.log("THPRetry", "Error is retryable, attempting second connect...") | ||
| Logger.warn("Connection failed for $deviceId, retrying: ${e.message}", context = TAG) |
There was a problem hiding this comment.
CLAUDE.md violation: The exception's message is manually interpolated into the log string.
Per CLAUDE.md: "NEVER manually append the Throwable's message or any other props to the string passed as the 1st param of Logger.* calls, its internals are already enriching the final log message with the details of the Throwable passed via the e arg."
Pass e as the second positional argument instead:
| Logger.warn("Connection failed for $deviceId, retrying: ${e.message}", context = TAG) | |
| Logger.warn("Connection failed for $deviceId, retrying", e, context = TAG) |
| if (current.size > MAX_LINES) { | ||
| _lines.value = current.takeLast(MAX_LINES) | ||
| } else { | ||
| _lines.value = current |
There was a problem hiding this comment.
CLAUDE.md violation: _lines.value = ... is used directly in log() (and clear() below) instead of _lines.update { ... }.
Per CLAUDE.md: "ALWAYS use _uiState.update { }, NEVER use _stateFlow.value ="
Using .update {} is also safer for concurrency — it avoids the read-then-write race between .value and the assignment (even though this block is synchronized). Replace all three assignments with _lines.update { ... }.
| import to.bitkit.services.TrezorDebugLog | ||
| import to.bitkit.ui.shared.toast.ToastEventBus | ||
| import javax.inject.Inject | ||
|
|
There was a problem hiding this comment.
CLAUDE.md violation: TrezorUiState (and SendStep below it) are declared before the TrezorViewModel class in this file.
Per CLAUDE.md: "ALWAYS create data classes for state AFTER viewModel class in same file."
Move TrezorUiState and SendStep to below the TrezorViewModel class definition.
| ) | ||
| } | ||
| finalResult.outputs.forEach { | ||
| when (it) { |
There was a problem hiding this comment.
CLAUDE.md violation: Inline fully-qualified class names are used here (and on the Change and OpReturn branches below) instead of imports.
Per CLAUDE.md: "ALWAYS add imports instead of inline fully-qualified names."
Add import com.synonym.bitkitcore.TrezorPrecomposedOutput at the top of the file and use the short names (TrezorPrecomposedOutput.Payment, TrezorPrecomposedOutput.Change, TrezorPrecomposedOutput.OpReturn). Other files in this PR (e.g. BalanceLookupSection.kt) already import and use the short name.
| ) { | ||
| val path = "ble:${gatt.device.address}" | ||
| val connection = bleConnections[path] ?: return | ||
|
|
There was a problem hiding this comment.
Bug: Thread.sleep on the BLE GATT callback thread.
Thread.sleep(200) is called inside onDescriptorWrite, which Android invokes on its single internal BLE GATT callback thread. Blocking this thread for 200 ms delays all subsequent BLE GATT callbacks (e.g. onCharacteristicChanged, onCharacteristicWrite) for all active connections until the sleep completes. This can cause characteristic notification timeouts (your read timeout is 5000 ms, but any notification arriving in this window will be queued and potentially misattributed) and intermittent connection failures.
If a short stabilization delay is truly necessary after enabling CCCD, perform it on a separate worker thread rather than blocking the GATT callback dispatcher.
app/src/main/res/values/strings.xml
Outdated
| <string name="settings__adv__section_networks">Networks</string> | ||
| <string name="settings__adv__section_other">Other</string> | ||
| <string name="settings__adv__section_payments">Payments</string> | ||
| <string name="settings__adv__section_hardware_wallet">Hardware Wallet</string> |
There was a problem hiding this comment.
CLAUDE.md violation: settings__adv__section_hardware_wallet is inserted after settings__adv__section_payments, but alphabetically h < n < o < p, so it should appear before settings__adv__section_networks.
Per CLAUDE.md: "ALWAYS add new localizable string string resources in alphabetical order in strings.xml."
| onCompose = onCompose, | ||
| ) | ||
| SendStep.REVIEW -> ReviewSection( | ||
| result = uiState.precomposedResult!!, |
There was a problem hiding this comment.
CLAUDE.md violation / latent crash risk: uiState.precomposedResult!! (and uiState.signedTxResult!! a few lines below) use the non-null assertion operator on nullable fields.
Per CLAUDE.md: val result = nullable!!.doSomething() is listed under ❌ DON'T — use safe calls instead.
While the ViewModel currently transitions sendStep and the result field atomically in a single update {} call, the type system does not enforce this contract. Any future code path that transitions sendStep without also setting the corresponding result will crash at runtime with no warning. Prefer safe calls with an early return:
| result = uiState.precomposedResult!!, | |
| SendStep.REVIEW -> uiState.precomposedResult?.let { result -> | |
| ReviewSection( | |
| result = result, | |
| isDeviceConnected = isDeviceConnected, | |
| isSigning = uiState.isSigning, | |
| onSign = onSign, | |
| onBack = onBack, | |
| ) | |
| } |
| @Suppress("LongParameterList") | ||
| @Composable | ||
| private fun TrezorContent( | ||
| trezorState: TrezorState, |
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/DeviceListSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/to/bitkit/ui/screens/trezor/ConnectedDeviceSection.kt
Outdated
Show resolved
Hide resolved
| if (!userInitiatedClose) { | ||
| _externalDisconnect.tryEmit(path) | ||
| } | ||
| userInitiatedClose = false |
There was a problem hiding this comment.
Bug: userInitiatedClose is a single shared flag across all BLE connections
This @Volatile boolean is not per-connection — it is shared across all entries in bleConnections. With two concurrent BLE connections, closing device A sets the flag to true. If device B disconnects externally before device A's GATT callback fires, B's onConnectionStateChange reads userInitiatedClose == true and suppresses the _externalDisconnect emission for B. The flag is then reset to false, causing A's subsequent callback to spuriously emit _externalDisconnect.
Since closeAllConnections() iterates over all entries and calls closeBleDevice for each, multi-device scenarios are expected and this race is reachable.
Suggested fix: replace the shared boolean with a per-path set, e.g.:
private val userInitiatedCloseSet = ConcurrentHashMap.newKeySet<String>()
// In closeBleDevice:
userInitiatedCloseSet.add(path)
// In onConnectionStateChange:
if (!userInitiatedCloseSet.remove(path)) {
_externalDisconnect.tryEmit(path)
}Related code:
bitkit-android/app/src/main/java/to/bitkit/services/TrezorTransport.kt
Lines 983 to 988 in 4637508
| val path = "ble:${gatt.device.address}" | ||
| val connection = bleConnections[path] ?: return | ||
|
|
||
| Thread.sleep(200) |
There was a problem hiding this comment.
Bug: Thread.sleep(200) blocks the Android BLE callback thread
All BluetoothGattCallback methods are dispatched on a single internal Android Bluetooth thread. Calling Thread.sleep(200) here blocks that thread for 200 ms, stalling all queued GATT events for every active BLE connection — including onCharacteristicWrite and onCharacteristicChanged needed by read/write operations in this transport. Under normal operating conditions (sequential descriptor writes followed by characteristic reads), this directly contributes to the 5000 ms read/write timeouts firing.
Move any delay-based stabilization to a coroutine dispatcher and signal the waiting code via a CountDownLatch or Channel from the callback, rather than sleeping on the callback thread.
Related code:
bitkit-android/app/src/main/java/to/bitkit/services/TrezorTransport.kt
Lines 1104 to 1115 in 4637508
| val idMatch = knownDevices.firstNotNullOfOrNull { known -> | ||
| scannedDevices.find { it.id == known.id } | ||
| } | ||
| val match = usbDevice ?: idMatch ?: error("No known device found nearby") |
There was a problem hiding this comment.
Bug: autoReconnect connects to any USB Trezor regardless of known-device identity
val usbDevice = scannedDevices.find { it.transportType == TrezorTransportType.USB }
val idMatch = knownDevices.firstNotNullOfOrNull { known -> scannedDevices.find { it.id == known.id } }
val match = usbDevice ?: idMatch ?: error("No known device found nearby")The usbDevice fallback picks the first USB device found unconditionally, without verifying that its ID matches any known/paired device. idMatch (which verifies by ID) is only used as a fallback when no USB device is present at all.
In a hardware wallet context this is a correctness and security concern: a different device carries different keys and accounts.
Suggested fix — invert the priority:
| val match = usbDevice ?: idMatch ?: error("No known device found nearby") | |
| val match = idMatch ?: usbDevice ?: error("No known device found nearby") |
Related code:
bitkit-android/app/src/main/java/to/bitkit/repositories/TrezorRepo.kt
Lines 369 to 374 in 4637508
|
|
||
| private fun saveKnownDevices(devices: List<KnownDevice>) { | ||
| runCatching { | ||
| prefs.edit().putString(KEY_KNOWN_DEVICES, json.encodeToString(devices)).commit() |
There was a problem hiding this comment.
Bug: SharedPreferences.commit() blocks the calling thread
commit() is synchronous — it writes to disk and blocks the thread until done. saveKnownDevices() is not a suspend function and does not dispatch to an IO dispatcher, so this blocking write runs on the coroutine dispatcher thread that called connect(), autoReconnect(), or forgetDevice().
Since the boolean return value of commit() is unused here, replace with apply():
| prefs.edit().putString(KEY_KNOWN_DEVICES, json.encodeToString(devices)).commit() | |
| prefs.edit().putString(KEY_KNOWN_DEVICES, json.encodeToString(devices)).apply() |
Related code:
bitkit-android/app/src/main/java/to/bitkit/repositories/TrezorRepo.kt
Lines 492 to 496 in 4637508
|
|
||
| @Suppress("TooManyFunctions") | ||
| @HiltViewModel | ||
| class TrezorViewModel @Inject constructor( |
There was a problem hiding this comment.
CLAUDE.md: Screen-specific ViewModel placed in viewmodels/
TrezorViewModel is only consumed by TrezorScreen and its sub-composables under ui/screens/trezor/. Per the project rule:
ALWAYS co-locate screen-specific ViewModels in the same package as their screen; only place ViewModels in
viewmodels/when shared across multiple screens
This file should be at app/src/main/java/to/bitkit/ui/screens/trezor/TrezorViewModel.kt.
See: https://github.com/synonymdev/bitkit-android/blob/463750843ad62fe87d961df909faf0c174f6f52d/CLAUDE.md
| import javax.inject.Inject | ||
| import javax.inject.Singleton | ||
|
|
||
| data class TrezorState( |
There was a problem hiding this comment.
CLAUDE.md: State data class declared before Repo class
TrezorState (line 42) is declared before class TrezorRepo (line 58). Per the project rule:
ALWAYS create data classes for state AFTER viewModel class in same file
Move TrezorState to after the closing brace of TrezorRepo.
See: https://github.com/synonymdev/bitkit-android/blob/463750843ad62fe87d961df909faf0c174f6f52d/CLAUDE.md
| } | ||
|
|
||
| private val usbManager: UsbManager by lazy { | ||
| context.getSystemService(Context.USB_SERVICE) as UsbManager |
There was a problem hiding this comment.
CLAUDE.md: Raw getSystemService() casts instead of ext/Context.kt extensions
Per the project rule:
ALWAYS use or create
Contextextension properties inext/Context.ktinstead of rawcontext.getSystemService()casts
Add to ext/Context.kt:
val Context.usbManager: UsbManager get() = getSystemService(Context.USB_SERVICE) as UsbManager
val Context.bluetoothManager: BluetoothManager get() = getSystemService(Context.BLUETOOTH_SERVICE) as BluetoothManagerSee: https://github.com/synonymdev/bitkit-android/blob/463750843ad62fe87d961df909faf0c174f6f52d/CLAUDE.md
| throw e | ||
| } | ||
| TrezorDebugLog.log("THPRetry", "Error is retryable, attempting second connect...") | ||
| Logger.warn("Connection failed for $deviceId, retrying: ${e.message}", context = TAG) |
There was a problem hiding this comment.
CLAUDE.md: e.message manually appended to Logger call string
Per the project rule:
NEVER manually append the
Throwable's message or any other props to the string passed as the 1st param ofLogger.*calls, its internals are already enriching the final log message with the details of theThrowablepassed via theearg
| Logger.warn("Connection failed for $deviceId, retrying: ${e.message}", context = TAG) | |
| Logger.warn("Connection failed for '$deviceId', retrying", e, context = TAG) |
See: https://github.com/synonymdev/bitkit-android/blob/463750843ad62fe87d961df909faf0c174f6f52d/CLAUDE.md
| trezorRepo.autoReconnect() | ||
| .onSuccess { | ||
| val label = it.label ?: it.model ?: "Trezor" | ||
| ToastEventBus.send(type = Toast.ToastType.INFO, title = "Reconnected to $label") |
There was a problem hiding this comment.
CLAUDE.md: Hardcoded strings in ViewModel
All ToastEventBus.send(title = "...") calls throughout this ViewModel use hardcoded English strings ("Reconnected to $label", "Trezor initialized", "Connected to $label", "Forgot $name", "Address generated", "Transaction broadcast", etc.).
Per the project rules:
NEVER hardcode strings and always preserve string resources
ALWAYS localize in ViewModels using injected@ApplicationContext, e.g.context.getString()
Note: the rule NEVER add string resources for strings used only in dev settings screens only exempts adding entries to strings.xml — it does not exempt the ViewModel from using context.getString(). The ViewModel currently has no @ApplicationContext injection.
See: https://github.com/synonymdev/bitkit-android/blob/463750843ad62fe87d961df909faf0c174f6f52d/CLAUDE.md
|
|
||
| private fun logCredentialFileState(deviceId: String, label: String) { | ||
| val sanitizedId = deviceId.replace(":", "_").replace("/", "_") | ||
| val credDir = java.io.File(context.filesDir, "trezor-thp-credentials") |
There was a problem hiding this comment.
CLAUDE.md: Inline fully-qualified class name instead of import
Per the project rule:
ALWAYS add imports instead of inline fully-qualified names
Add import java.io.File to the imports section and use File(...) directly.
| val credDir = java.io.File(context.filesDir, "trezor-thp-credentials") | |
| val credDir = File(context.filesDir, "trezor-thp-credentials") |
See: https://github.com/synonymdev/bitkit-android/blob/463750843ad62fe87d961df909faf0c174f6f52d/CLAUDE.md
…bitkit-android into feat/trezor-hardware-support
…to feat/trezor-hardware-support # Conflicts: # app/src/main/java/to/bitkit/ui/screens/trezor/TrezorScreen.kt
| endpoint.direction == UsbConstants.USB_DIR_IN -> { | ||
| readEndpoint = endpoint | ||
| } | ||
| endpoint.type == UsbConstants.USB_ENDPOINT_XFER_INT && |
There was a problem hiding this comment.
Bug: bulkTransfer() called on interrupt endpoints - all USB reads/writes will fail
findUsbEndpoints() exclusively selects endpoints with type USB_ENDPOINT_XFER_INT (interrupt), but both readUsbChunk and writeUsbChunk call UsbDeviceConnection.bulkTransfer() on those endpoints. Per Android's documented API contract, bulkTransfer() is only valid for bulk-type endpoints (USB_ENDPOINT_XFER_BULK); calling it with interrupt endpoints returns -1 on every invocation.
This means all USB communication with the Trezor will silently fail.
Fix: Either change findUsbEndpoints to filter for USB_ENDPOINT_XFER_BULK (if the Trezor exposes bulk endpoints on the active interface), or switch to UsbRequest with queue()/requestWait() for proper interrupt transfer handling.
|
|
||
| val disconnected = disconnectLatch.await(DISCONNECT_TIMEOUT_MS, TimeUnit.MILLISECONDS) | ||
| if (!disconnected) { | ||
| Logger.warn("BLE disconnect timeout, forcing close: '$path'", context = TAG) |
There was a problem hiding this comment.
Bug: userInitiatedCloseSet leaks path on disconnect timeout, silently suppressing future external disconnects
closeBleDevice() adds path to userInitiatedCloseSet at line ~829, relying on onConnectionStateChange to remove it. However, when disconnectLatch.await() times out (the if (!disconnected) branch here), gatt.close() is called immediately after - which can prevent the GATT callback from ever firing for this disconnect. The path then remains in userInitiatedCloseSet indefinitely.
On the next reconnect + external disconnect cycle for the same device path, onConnectionStateChange calls userInitiatedCloseSet.remove(path) which returns true (stale entry), suppressing _externalDisconnect.tryEmit(path). The app's state machine will permanently believe the device is connected.
Fix: Ensure userInitiatedCloseSet.remove(path) always runs after the disconnect attempt, regardless of timeout or exception, by adding it to a finally block after the latch await.
| "Scan found ${scannedDevices.size} devices: ${scannedDevices.map { it.id }}", | ||
| ) | ||
| val exactMatch = scannedDevices.find { it.id == deviceId } | ||
| val usbDevice = scannedDevices.find { it.transportType == TrezorTransportType.USB } |
There was a problem hiding this comment.
Security bug: connectKnownDevice can silently connect to a different Trezor device
When the known device is Bluetooth and a USB device is visible, the code substitutes the first USB device found in scan results with no identity verification:
val usbDevice = scannedDevices.find { it.transportType == TrezorTransportType.USB }This selects any USB device - not necessarily the same Trezor. In a multi-device environment or with a malicious USB accessory, this would connect to the wrong hardware wallet, sign Bitcoin transactions with the wrong seed, and overwrite the stored known device record with the substitute's ID.
Compare with autoReconnect() which correctly constrains the USB fallback to known device IDs (it.id in knownIds). The same guard should apply here, or cross-transport identity verification (e.g., matching device label/model after connecting) should be performed before trusting the substituted device.
| synchronized(pairingCodeLock) { | ||
| submittedPairingCode = "" | ||
| pairingCodeRequest = PairingCodeRequest(isRequested = true, latch = latch) | ||
| _needsPairingCode.value = true |
There was a problem hiding this comment.
CLAUDE.md violation: _stateFlow.value = used instead of _stateFlow.update {}
Per CLAUDE.md: "ALWAYS use _uiState.update { }, NEVER use _stateFlow.value ="
_needsPairingCode.value = is assigned directly in 4 places:
- Line 294:
_needsPairingCode.value = true - Line 303:
_needsPairingCode.value = false - Line 312:
_needsPairingCode.value = false - Line 349:
_needsPairingCode.value = false
All four should use .update { } instead, e.g.:
_needsPairingCode.update { true }
_needsPairingCode.update { false }| private suspend fun connectWithThpRetry(deviceId: String): TrezorFeatures { | ||
| TrezorDebugLog.log("THPRetry", "First connect attempt for: $deviceId") | ||
| logCredentialFileState(deviceId, "BEFORE 1st attempt") | ||
| return try { |
There was a problem hiding this comment.
CLAUDE.md violation: try-catch used instead of the Result API
Per CLAUDE.md: "ALWAYS use the Result API instead of try-catch" and "NEVER use Exception directly, use AppError instead"
connectWithThpRetry uses a raw try { } catch (e: Exception) { } block. The @Suppress("TooGenericExceptionCaught") annotation confirms the author was aware of the broad catch. This should be refactored to use runCatching { } with Result API chaining (onSuccess, onFailure, getOrElse).
Related: isRetryableError(e: Exception) at line ~623 also uses Exception as a parameter type directly, which should be AppError per CLAUDE.md.
| suspend fun autoReconnect(walletIndex: Int = 0): Result<TrezorFeatures> = withContext(bgDispatcher) { | ||
| val knownDevices = _state.value.knownDevices.ifEmpty { loadKnownDevices() } | ||
| if (knownDevices.isEmpty()) { | ||
| return@withContext Result.failure(IllegalStateException("No known devices")) |
There was a problem hiding this comment.
CLAUDE.md violation: IllegalStateException used instead of AppError
Per CLAUDE.md: "NEVER use Exception directly, use AppError instead" and "ALWAYS inherit custom exceptions from AppError"
IllegalStateException is a raw JVM exception, not an AppError subclass. Two occurrences:
- Line 381 (in
autoReconnect):Result.failure(IllegalStateException("No known devices")) - Line 413 (in
connectKnownDevice):Result.failure(IllegalStateException("Connection already in progress"))
Both should use AppError (or a custom subclass) instead.
| unfocusedBorderColor = Colors.White32, | ||
| cursorColor = Colors.Brand, | ||
| ), | ||
| textStyle = androidx.compose.ui.text.TextStyle( |
There was a problem hiding this comment.
CLAUDE.md violation: inline fully-qualified class names instead of imports
Per CLAUDE.md: "ALWAYS add imports instead of inline fully-qualified names"
androidx.compose.ui.text.TextStyle and androidx.compose.ui.text.style.TextAlign are used as fully-qualified inline references while all other types in the same file use proper imports:
textStyle = androidx.compose.ui.text.TextStyle(
...
textAlign = androidx.compose.ui.text.style.TextAlign.Center,
...
),These should be imported at the top of the file:
import androidx.compose.ui.text.TextStyle
import androidx.compose.ui.text.style.TextAlignand used without qualification.
|
|
||
| @Suppress("LongParameterList") | ||
| @Composable | ||
| private fun TrezorContent( |
There was a problem hiding this comment.
CLAUDE.md violation: inner state composable named TrezorContent instead of Content
Per CLAUDE.md: "ALWAYS split screen composables into parent accepting viewmodel + inner private child accepting state and callbacks Content()"
The inner private composable that accepts state and callbacks is named TrezorContent - it must be named Content per the convention. The call site should be updated accordingly.
- Upgrades to the latest bitkit-core bindings v0.1.45 - Updates the implementation accordingly
2 participants
Description
The intention of this Trezor dev dashboard is to serve as a place for developers to test and troubleshoot Trezor hardware features as they emerge. This is not meant to be user facing, but to serve as a reference for how to use and interact with Trezor hardware devices once work on user-facing Trezor features begin by native devs.
Settings->Advanced->TrezorPreview
QA Notes