feat: bump core to 0.3.9 and hw reliability#1062
Conversation
![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.
Greptile SummaryThis PR bumps
Confidence Score: 4/5The BLE reconnect retry loop, in-flight serialisation, and user-cancellation suppression are all well-tested and logically sound; the wallet-scoped API migration is mechanical and comprehensive. The double disconnectStaleSession call on the first BLE retry attempt and the broad exception swallowing in waitForConnectAttempt are style-level concerns unlikely to produce visible failures. The semantic shift in toMergedActivities from aggregation to last-writer-wins is intentional and test-confirmed but worth a second look. No correctness regressions were identified. TrezorRepo.kt — the connectKnownDevice failure path and the waitForConnectAttempt polling loop; HwWalletRepo.toMergedActivities deduplication change.
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/repositories/TrezorRepo.kt | Core BLE reconnect logic rewrite: adds reconnectKnownBluetoothDevice retry loop, awaitInFlightConnect polling, warmUpKnownDevice, and unconditional disconnectStaleSession on connect failure — one redundant disconnect when forceSession=true |
| app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt | Removes manual HistoryTransaction-to-Activity conversion, delegates to core-provided Activity objects; deduplication semantics changed from aggregation to last-writer-wins by sorted address type; user-cancellation no longer triggers stale-session disconnect |
| app/src/main/java/to/bitkit/models/WalletScope.kt | New singleton wrapping getDefaultWalletId() with a lazy initializer and a test override; eagerly consumed in ActivityService constructor — timing-safe as long as core is initialised before DI graph resolves ActivityService |
| app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt | Adds warmUpHardwareConnection delegation and user-cancellation silent-return in handleHardwareTransferFailure; BLE reconnect error now shows INFO toast instead of ERROR |
| app/src/main/java/to/bitkit/ext/TrezorExceptionExt.kt | New extension walking the cause chain to detect any of the three user-cancellation exception types; well-tested |
| app/src/main/java/to/bitkit/services/CoreService.kt | Mechanically routes every activity/pre-activity-metadata core call through walletId = WalletScope.default; no logic changes beyond the wallet-scoped API migration |
| app/src/main/java/to/bitkit/ui/shared/toast/ToastEventBus.kt | Guards send(Throwable) with isTrezorUserCancellation(); adds takeIf { it.isNotBlank() } guard against blank error messages |
| app/src/main/java/to/bitkit/ui/screens/trezor/TrezorViewModel.kt | Replaces HistoryTransaction list with Activity list in watcher state; derives a synthetic walletId for the dev watcher screen from the entered xpub |
| app/src/test/java/to/bitkit/repositories/TrezorRepoTest.kt | Good new tests for warmUpKnownDevice and ensureConnected BLE retry; existing verify calls updated with the new walletId parameter |
| app/src/test/java/to/bitkit/test/BaseUnitTest.kt | Adds @Before/@after hooks to set/clear WalletScope.testOverride for all unit tests — clean approach preventing lazy init from hitting native code in tests |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Screen as SpendingHwSignScreen
participant TVM as TransferViewModel
participant HWR as HwWalletRepo
participant TR as TrezorRepo
Screen->>TVM: warmUpHardwareConnection(deviceId)
TVM->>HWR: warmUpKnownDevice(deviceId)
HWR->>TR: warmUpKnownDevice(deviceId)
TR->>TR: isKnownBluetoothDevice? / isConnectInProgress?
TR->>TR: ensureConnected(deviceId) [fire and forget]
Screen->>TVM: onTransferToSpendingHwConfirm(order, deviceId)
TVM->>HWR: ensureConnected(deviceId)
HWR->>TR: ensureConnected(deviceId)
TR->>TR: connectedFeatures(deviceId)?
alt already connected
TR-->>HWR: Result.success(features)
else in-flight connect
TR->>TR: awaitInFlightConnect(deviceId)
TR-->>HWR: Result.success(features) or retry
else BLE retry loop
loop up to 4 attempts backoff 2s 4s 6s
TR->>TR: connectKnownDevice(deviceId)
end
TR-->>HWR: Result.success(features) or failure
end
HWR->>TR: signTxFromPsbt(...)
alt UserCancelled PinCancelled PassphraseCancelled
TR-->>HWR: Result.failure(TrezorException)
HWR->>HWR: skip disconnectStaleSession
HWR-->>TVM: Result.failure
TVM->>TVM: isTrezorUserCancellation true silent return
else signing success
TR-->>HWR: Result.success(signedTx)
HWR->>TR: broadcastRawTx
TR-->>HWR: txId
HWR-->>TVM: HwFundingBroadcastResult
TVM->>Screen: TransferEffect.OnHwTxSigned
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Screen as SpendingHwSignScreen
participant TVM as TransferViewModel
participant HWR as HwWalletRepo
participant TR as TrezorRepo
Screen->>TVM: warmUpHardwareConnection(deviceId)
TVM->>HWR: warmUpKnownDevice(deviceId)
HWR->>TR: warmUpKnownDevice(deviceId)
TR->>TR: isKnownBluetoothDevice? / isConnectInProgress?
TR->>TR: ensureConnected(deviceId) [fire and forget]
Screen->>TVM: onTransferToSpendingHwConfirm(order, deviceId)
TVM->>HWR: ensureConnected(deviceId)
HWR->>TR: ensureConnected(deviceId)
TR->>TR: connectedFeatures(deviceId)?
alt already connected
TR-->>HWR: Result.success(features)
else in-flight connect
TR->>TR: awaitInFlightConnect(deviceId)
TR-->>HWR: Result.success(features) or retry
else BLE retry loop
loop up to 4 attempts backoff 2s 4s 6s
TR->>TR: connectKnownDevice(deviceId)
end
TR-->>HWR: Result.success(features) or failure
end
HWR->>TR: signTxFromPsbt(...)
alt UserCancelled PinCancelled PassphraseCancelled
TR-->>HWR: Result.failure(TrezorException)
HWR->>HWR: skip disconnectStaleSession
HWR-->>TVM: Result.failure
TVM->>TVM: isTrezorUserCancellation true silent return
else signing success
TR-->>HWR: Result.success(signedTx)
HWR->>TR: broadcastRawTx
TR-->>HWR: txId
HWR-->>TVM: HwFundingBroadcastResult
TVM->>Screen: TransferEffect.OnHwTxSigned
end
Comments Outside Diff (1)
-
app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt, line 283-290 (link)Deduplication now picks last address-type alphabetically, not the aggregated value. Previously
toMergedActivitiessummedreceived/sentacross address types for the same txid. The new code sorts watchers by"$deviceId|$addressType"and overwrites the map on duplicaterawId(), so for a tx visible in bothnativeSegwitandtaproot, the taproot entry wins and the native-segwit value is silently dropped. The updated test confirms this intent (old150uL, new50uL). Worth confirming that the core library'sActivityobjects are already authoritative so no value is lost.
Reviews (1): Last reviewed commit: "fix: serialize hw reconnect and scope wa..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| }.onFailure { e -> | ||
| Logger.error("Connect known device failed", e, context = TAG) | ||
| _state.update { it.copy(error = e.message) } | ||
| disconnectStaleSession(deviceId) | ||
| } |
There was a problem hiding this comment.
disconnectStaleSession on the first BLE retry attempt. When forceSession = true (attempt 0), connectKnownDevice already calls disconnectStaleSession at the start of the connection flow. If the connection then fails, the new onFailure block calls disconnectStaleSession a second time. Assuming the function is idempotent this is harmless, but it's redundant and could cause confusion if the implementation ever gains side effects. A simple guard makes the intent explicit.
| }.onFailure { e -> | |
| Logger.error("Connect known device failed", e, context = TAG) | |
| _state.update { it.copy(error = e.message) } | |
| disconnectStaleSession(deviceId) | |
| } | |
| }.onFailure { e -> | |
| Logger.error("Connect known device failed", e, context = TAG) | |
| _state.update { it.copy(error = e.message) } | |
| if (!forceSession) { | |
| // forceSession already disconnected before the attempt; avoid a redundant call. | |
| disconnectStaleSession(deviceId) | |
| } | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| private suspend fun waitForConnectAttempt(deviceId: String) { | ||
| runCatching { | ||
| withTimeout(CONNECT_ATTEMPT_MAX_WAIT) { | ||
| while (true) { | ||
| if (connectedFeatures(deviceId) != null) return@withTimeout | ||
| if (!isConnectInProgress()) return@withTimeout | ||
| delay(CONNECT_ATTEMPT_POLL_INTERVAL) | ||
| } | ||
| } | ||
| }.onFailure { | ||
| if (it is CancellationException && it !is TimeoutCancellationException) throw it | ||
| } | ||
| connectKnownDevice(deviceId, forceSession = false) | ||
| } |
There was a problem hiding this comment.
waitForConnectAttempt silently swallows all non-CancellationException errors. Because connectedFeatures and isConnectInProgress are simple state reads this is unlikely to matter today, but limiting the swallowed set to TimeoutCancellationException only makes the contract clearer and avoids masking unexpected future failures.
| private suspend fun waitForConnectAttempt(deviceId: String) { | |
| runCatching { | |
| withTimeout(CONNECT_ATTEMPT_MAX_WAIT) { | |
| while (true) { | |
| if (connectedFeatures(deviceId) != null) return@withTimeout | |
| if (!isConnectInProgress()) return@withTimeout | |
| delay(CONNECT_ATTEMPT_POLL_INTERVAL) | |
| } | |
| } | |
| }.onFailure { | |
| if (it is CancellationException && it !is TimeoutCancellationException) throw it | |
| } | |
| connectKnownDevice(deviceId, forceSession = false) | |
| } | |
| private suspend fun waitForConnectAttempt(deviceId: String) { | |
| try { | |
| withTimeout(CONNECT_ATTEMPT_MAX_WAIT) { | |
| while (true) { | |
| if (connectedFeatures(deviceId) != null) return@withTimeout | |
| if (!isConnectInProgress()) return@withTimeout | |
| delay(CONNECT_ATTEMPT_POLL_INTERVAL) | |
| } | |
| } | |
| } catch (e: TimeoutCancellationException) { | |
| // Expected: in-flight connect ran longer than our wait window; caller will retry. | |
| } | |
| } |
|
|
||
| private val lazyDefault: String by lazy { getDefaultWalletId() } |
There was a problem hiding this comment.
lazyDefault is eagerly consumed in ActivityService's constructor. ActivityService stores private val walletId = WalletScope.default, triggering getDefaultWalletId() at construction time. Kotlin's SYNCHRONIZED lazy does not cache thrown exceptions, so a premature call would fail DI resolution rather than cache a wrong ID. Worth confirming that CoreService/ActivityService is never constructed before the core is initialised — tests use testOverride to sidestep this, but production relies on DI graph ordering.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3a41d26a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| addressType = addressType, | ||
| xpub = xpub, | ||
| electrumUrl = watcherSettings.electrumUrl, | ||
| walletId = device.walletId, |
There was a problem hiding this comment.
For users upgrading with already-paired hardware wallets, KnownDevice.walletId deserializes as the new default "" until TrezorRepo.loadKnownDevices() migrates the store during startWatcher(). This spec is built from the raw HwWalletStore value before that migration, so the first watcher can be started with a blank wallet scope; when the migrated store emits, the watcher is considered active because only the Electrum URL is compared, so it is not restarted with the derived wallet id. That leaves upgraded devices running in the wrong/empty activity scope for the session; derive/fallback the id here or include walletId in the active watcher config and restart when it changes.
Useful? React with 👍 / 👎.
| sortedBy { "${it.deviceId}|${it.addressType}" } | ||
| .flatMap { it.activities } | ||
| .forEach { activity -> | ||
| byId[activity.rawId()] = activity |
There was a problem hiding this comment.
For transactions that touch two monitored xpubs for the same hardware wallet, such as receiving to both native SegWit and Taproot addresses, each address-type watcher can report an Activity.Onchain with the same txid and only that watcher's value. This assignment keeps whichever activity sorts last and drops the other value, so the wallet activity list undercounts while the balance still sums both watchers; keep the previous txid-level merge/summing behavior instead of overwriting by rawId().
Useful? React with 👍 / 👎.
| allowBleFallback = allowBleFallback, | ||
| ) | ||
| if (result.isSuccess) return result | ||
| lastFailure = result.exceptionOrNull() |
There was a problem hiding this comment.
When a BLE reconnect attempt fails because core returns a Trezor cancellation (for example the user cancels PIN, passphrase, or device approval), this loop treats it the same as a transient missing advertisement and continues through the remaining retry attempts. That can re-prompt the user several times after an explicit cancel before finally returning the failure; return cancellation failures immediately instead of storing them in lastFailure and continuing.
Useful? React with 👍 / 👎.
2 participants
Part of #1030
This PR:
Description
The bitkit-core upgrade brings wallet-scoped storage and Trezor stack improvements from the 0.3.x line (device-busy handling, typed session errors, transport callback updates). Android wiring moves default wallet identity through
WalletScope, routes CoreService calls through scoped wallet ids, and updates hardware wallet, activity, migration, and dev Trezor tooling to match.On top of the core bump, this branch hardens hardware transfer UX:
UserCancelled,PinCancelled,PassphraseCancelled) is treated as a silent no-op: no error toast, no stale-session disconnect churn.Out of scope for this PR (still tracked elsewhere):
Preview
before.mov
after.mov
Upload your local recordings into the table and below when opening the draft PR on GitHub.
QA Notes
Manual Tests
regression:Settings → connect known USB Trezor → get address / watch balance: still works after core bump.regression:Settings → connect known BLE Trezor: pairing, PIN, and balance watch still work.regression:Upgrade from existing wallet (Room migration path): app opens, balances, activity, and hardware wallet tiles unchanged.regression:Normal Lightning send/receive and on-chain activity: unaffected by wallet-scoped core migration.Automated Checks
TrezorRepoTest.kt; Trezor cancel detection inTrezorExceptionExtTest.kt; cancel handling inTransferViewModelTest.ktandHwWalletRepoTest.kt.HwWalletRepoTest.kt,ActivityRepoTest.kt,PreActivityMetadataRepoTest.kt,TrezorViewModelTest.kt, and related view model tests.just test file "to.bitkit.repositories.TrezorRepoTest",just test file "to.bitkit.repositories.HwWalletRepoTest",just test file "to.bitkit.viewmodels.TransferViewModelTest",just test file "to.bitkit.ext.TrezorExceptionExtTest".