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 bitkit-core from 0.1.75 to 0.3.9 and migrates all activity, hardware-wallet, and backup APIs to wallet-scoped identifiers, while adding BLE Trezor reconnect reliability (retry with backoff, warm-up pre-connect, in-flight serialization) and silent suppression of user-cancellation toasts on the sign screen.
Confidence Score: 5/5Safe to merge. The wallet-scoped API migration is mechanical and comprehensive, the BLE retry logic is well-bounded (4 attempts, growing delays), and cancel suppression is guarded by a cause-chain walk rather than a simple type check. The three main workstreams — wallet-scoped core migration, BLE reconnect hardening, and user-cancellation suppression — all have matching unit tests that exercise the new code paths, including edge cases like retry-until-scan-succeeds, cancel during reconnect, and disconnectStaleSession guarding against clearing an unrelated device. No logic errors were found in the retry loop bounds, the in-flight serialization, or the backup compat layer. No files require special attention. The BackupRestoreCompat heuristics (field-presence checks for walletId injection) are worth keeping an eye on if the backup schema evolves, but they are correct for the current format.
|
| Filename | Overview |
|---|---|
| app/src/main/java/to/bitkit/repositories/TrezorRepo.kt | Major expansion: adds BLE retry loop (reconnectKnownBluetoothDevice), in-flight connect serialization (awaitInFlightConnect / waitForConnectAttempt), warm-up pre-connect, resolveKnownReconnectDevice, HwWalletId-based wallet ID derivation, and a guard in disconnectStaleSession to avoid clearing an unrelated connected device. Well-tested with new TrezorRepoTest cases. |
| app/src/main/java/to/bitkit/repositories/HwWalletRepo.kt | Drops HistoryTransaction in favour of Activity from WatcherEvent; adds walletId scoping to watchers; replaces multi-tx merge logic (received/sent aggregation) with a new mergedActivity() that works directly with OnchainActivity values from the core; cancel-guards disconnectStaleSession on signing failure. |
| app/src/main/java/to/bitkit/viewmodels/TransferViewModel.kt | Adds warmUpHardwareConnection, isTrezorUserCancellation checks at two points in handleHardwareTransferFailure, and a BLE-aware error message (INFO toast instead of ERROR for known BLE devices). New TransferViewModelTest cases cover cancel and reconnect-cancel paths. |
| app/src/main/java/to/bitkit/ext/TrezorExceptionExt.kt | New file: isTrezorUserCancellation() walks the full cause chain to detect UserCancelled, PinCancelled, PassphraseCancelled. Tested exhaustively including wrapped exceptions. |
| app/src/main/java/to/bitkit/models/WalletScope.kt | New singleton that lazily resolves the default wallet ID via the core native call. testOverride allows unit tests to sidestep the real core. Production relies on DI graph ensuring core is initialised first. |
| app/src/main/java/to/bitkit/ext/BackupRestoreCompat.kt | New file: normalizeLegacyWalletIdsInBackupJson injects WalletScope.default walletId into old backup JSON objects via structural heuristics (field presence checks). Only patches objects that don't already have walletId, ensuring idempotency on new-format backups. |
| app/src/main/java/to/bitkit/services/CoreService.kt | ActivityService now holds a private walletId = WalletScope.default and passes it to every core API call. Mechanical but comprehensive migration. |
| app/src/main/java/to/bitkit/services/MigrationService.kt | Adds WalletScope.default to ActivityTags, LightningActivity, and OnchainActivity constructors during migration paths, keeping Room upgrade state consistent with the new wallet-scoped schema. |
| app/src/main/java/to/bitkit/ui/shared/toast/ToastEventBus.kt | send(Throwable) now silently drops user-cancellation errors before emitting a toast, and falls back to 'Unknown error' when the error message is blank. |
| app/src/main/java/to/bitkit/ui/screens/transfer/hardware/SpendingHwSignScreen.kt | Adds LaunchedEffect(deviceId) that calls warmUpHardwareConnection to pre-connect the specific BLE device when the sign screen opens, avoiding a global foreground reconnect. |
| app/src/main/java/to/bitkit/services/TrezorTransport.kt | Mechanical: adds errorCode = null to every TrezorTransportWriteResult / TrezorTransportReadResult construction site to match the updated core 0.3.9 struct signature. No logic changes. |
| app/src/main/java/to/bitkit/models/HwWalletId.kt | New thin wrapper around core's deriveWalletId; requires non-empty xpubs and passes only the values (not keys) to the native function. |
| gradle/libs.versions.toml | Bumps bitkit-core from 0.1.75 to 0.3.9; single-line change. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as SpendingHwSignScreen
participant TVM as TransferViewModel
participant HWR as HwWalletRepo
participant TR as TrezorRepo
participant BLE as TrezorService (BLE)
UI->>TVM: warmUpHardwareConnection(deviceId)
TVM->>HWR: warmUpKnownDevice(deviceId)
HWR->>TR: warmUpKnownDevice(deviceId)
TR-->>TR: skip if connected / in-progress / non-BLE
TR->>TR: ensureConnected(deviceId) [background]
UI->>TVM: onTransferToSpendingHwConfirm(order, deviceId)
TVM->>HWR: ensureConnected(deviceId)
HWR->>TR: ensureConnected(deviceId)
TR-->>TR: connectedFeatures? return early
TR-->>TR: isConnectInProgress? awaitInFlightConnect
TR->>TR: reconnectKnownBluetoothDevice(deviceId)
loop up to 4 attempts (0s, 2s, 4s, 6s delays)
TR->>BLE: scan()
BLE-->>TR: devices list
TR->>BLE: connectKnownDevice(deviceId)
BLE-->>TR: TrezorFeatures or failure
alt user cancelled
TR-->>TVM: Result.failure(UserCancelled)
TVM-->>UI: silent no-op (no toast)
else success
TR-->>TVM: Result.success(features)
end
end
TVM->>HWR: signAndBroadcastFunding(tx, deviceId)
alt user cancels on device
HWR-->>TVM: Result.failure(UserCancelled)
TVM-->>UI: silent no-op (no disconnectStaleSession)
else success
HWR-->>TVM: Result.success(broadcastResult)
TVM-->>UI: transfer complete
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 UI as SpendingHwSignScreen
participant TVM as TransferViewModel
participant HWR as HwWalletRepo
participant TR as TrezorRepo
participant BLE as TrezorService (BLE)
UI->>TVM: warmUpHardwareConnection(deviceId)
TVM->>HWR: warmUpKnownDevice(deviceId)
HWR->>TR: warmUpKnownDevice(deviceId)
TR-->>TR: skip if connected / in-progress / non-BLE
TR->>TR: ensureConnected(deviceId) [background]
UI->>TVM: onTransferToSpendingHwConfirm(order, deviceId)
TVM->>HWR: ensureConnected(deviceId)
HWR->>TR: ensureConnected(deviceId)
TR-->>TR: connectedFeatures? return early
TR-->>TR: isConnectInProgress? awaitInFlightConnect
TR->>TR: reconnectKnownBluetoothDevice(deviceId)
loop up to 4 attempts (0s, 2s, 4s, 6s delays)
TR->>BLE: scan()
BLE-->>TR: devices list
TR->>BLE: connectKnownDevice(deviceId)
BLE-->>TR: TrezorFeatures or failure
alt user cancelled
TR-->>TVM: Result.failure(UserCancelled)
TVM-->>UI: silent no-op (no toast)
else success
TR-->>TVM: Result.success(features)
end
end
TVM->>HWR: signAndBroadcastFunding(tx, deviceId)
alt user cancels on device
HWR-->>TVM: Result.failure(UserCancelled)
TVM-->>UI: silent no-op (no disconnectStaleSession)
else success
HWR-->>TVM: Result.success(broadcastResult)
TVM-->>UI: transfer complete
end
Reviews (4): Last reviewed commit: "fix: scope hw watchers to native segwit" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ae87c021 + 25c3a7925:
resolvedWalletId()derives/fallback when store value is blankactiveWatcherWalletIdstracked; watcher restarts when wallet id changes (not just electrum URL)- Shared
deriveHardwareWalletId()aligns connect, restore, and watcher paths
| 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 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ae87c021 — restored txid-level sum merge via mergedActivity() (group by rawId, sum received/sent). Tests expect merged 150uL, not last-writer 50uL. Also merged feeRate via maxOf in 25c3a7925.
| 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 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ae87c021 — reconnectKnownBluetoothDevice returns immediately on isTrezorUserCancellation(). Covered by TrezorRepoTest (ensureConnected stops bluetooth retry after user cancellation).
|
Addressed bot review feedback in
Deferred: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25c3a79255
ℹ️ 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".
| is HardwareReconnectError -> { | ||
| Logger.error("Failed to reconnect hardware device", e, context = TAG) | ||
| showHardwareReconnectError() | ||
| showHardwareReconnectError(deviceId) |
There was a problem hiding this comment.
When the reconnect step itself is where the user cancels (PIN, passphrase, or on-device approval), ensureHardwareConnected wraps that TrezorException in HardwareReconnectError. This branch runs before the generic isTrezorUserCancellation() guard below, so the cancellation still gets logged as a reconnect failure and shows the hardware reconnect/connect error toast; check the cause chain before showing this error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 32dbef704e:
ensureHardwareConnected()rethrowsisTrezorUserCancellation()instead of wrapping inHardwareReconnectErrorHardwareReconnectErrorbranch also checks cancellation before showing toast
Covered by TransferViewModelTest (onTransferToSpendingHwConfirm does not toast when user cancels during reconnect).
| val value = when (txType) { | ||
| PaymentType.RECEIVED -> received.safe() - sent.safe() | ||
| PaymentType.SENT -> (sent.safe() - received.safe()).safe() - fee.safe() |
There was a problem hiding this comment.
When a sent hardware transaction is reported by multiple address-type watchers, the values being merged are already OnchainActivity.value display amounts with the fee stored separately. This branch sums those net values into sent and then subtracts the max fee again, so a tx split across two watched xpubs displays too little while the watcher balances still include the correct amount; merge sent watcher activities by summing the reported values without applying the fee a second time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 32dbef704e — SENT merge uses summed net watcher values (sent - received) without subtracting max(fee) again; fee is stored once separately.
Covered by HwWalletRepoTest (merges duplicate sent tx activities without subtracting fee twice).
| barcode-scanning = { module = "com.google.mlkit:barcode-scanning", version = "17.3.0" } | ||
| biometric = { module = "androidx.biometric:biometric", version = "1.4.0-alpha05" } | ||
| bitkit-core = { module = "com.synonym:bitkit-core-android", version = "0.1.75" } | ||
| bitkit-core = { module = "com.synonym:bitkit-core-android", version = "0.3.9" } |
There was a problem hiding this comment.
This core bump makes serialized activity/metadata models include a required walletId field, and the rest of the diff had to add that value at every construction site. Existing cloud backups written by the previous core version do not contain walletId, but the restore path still decodes MetadataBackupV1 and ActivityBackupV1 directly before any migration/defaulting can run, so restoring those older backups fails with a missing-field decode error; add legacy backup normalization or serializers/defaults that fill WalletScope.default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 32dbef704e:
- Added
BackupRestoreCompat.ktto injectWalletScope.defaultwhen legacy backup JSON lackswalletId BackupReporestore usesdecodeActivityBackupV1Compat()/decodeMetadataBackupV1Compat()
Covered by BackupRestoreCompatTest.
| barcode-scanning = { module = "com.google.mlkit:barcode-scanning", version = "17.3.0" } | ||
| biometric = { module = "androidx.biometric:biometric", version = "1.4.0-alpha05" } | ||
| bitkit-core = { module = "com.synonym:bitkit-core-android", version = "0.1.75" } | ||
| bitkit-core = { module = "com.synonym:bitkit-core-android", version = "0.3.9" } |
There was a problem hiding this comment.
This PR is a user-facing feat: change (core bump plus hardware wallet reliability), but the diff does not add any file under changelog.d/next/ or changelog.d/hotfix/. The repo instructions in AGENTS.md require exactly one changelog fragment for user-facing feat:/fix: PRs, so the release notes will omit these changes unless a fragment is added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added changelog.d/next/1062.added.md in the latest commit on this branch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32dbef704e
ℹ️ 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".
| if (txid in knownTxIds || !emittedReceivedTxIds.add(txid)) return@forEach | ||
| val sats = mergedActivities.findOnchain(txid)?.v1?.value ?: onchain.v1.value | ||
| _receivedTxs.emit(HwWalletReceivedTx(txid = txid, sats = sats)) |
There was a problem hiding this comment.
When the same new inbound tx is reported by multiple address-type watchers, the first watcher to arrive adds the txid to emittedReceivedTxIds and emits the sheet using the amount currently in watcherData; the later watcher is then skipped by this guard. In that scenario the wallet balance/activity eventually merge both outputs, but AppViewModel has already shown the received sheet with only the first watcher’s value, so defer/dedupe the emission until the merged activity includes all watcher reports or update the emitted amount instead of suppressing the later event.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in scope for 2.4.0 Trezor v1 by locking hardware watchers to native segwit only (HwWalletRepo.SUPPORTED_WATCHER_ADDRESS_TYPES), matching product scope in #bitkit-native.
With a single native-segwit watcher per device, the multi-watcher receive-sheet race is not reachable on the planned v1 path. Multi-type HW monitoring and merged receive-sheet amounts remain follow-up work (#1044 / 2.5.0).
| if (!forceSession) { | ||
| disconnectStaleSession(deviceId) |
There was a problem hiding this comment.
When a manual reconnect is attempted for a different known device and that scan/connect fails (for example the selected Trezor is not nearby), this new cleanup calls disconnectStaleSession(deviceId). That helper always invokes trezorService.disconnect() and clears _state.connected, so an unrelated Trezor that was already connected gets dropped just because another device failed to reconnect; only disconnect when the current session belongs to the failed deviceId or when forceSession requested it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in upcoming commit on this branch: disconnectStaleSession is now a no-op when another Trezor is already connected, so a failed reconnect for device A no longer clears the live session for device B.
See TrezorRepoTest — disconnectStaleSession should not disconnect unrelated connected device and connectKnownDevice failure should not disconnect unrelated connected device.
Lock Trezor watch-only sync to native segwit for v1, preserve unrelated connected devices on reconnect failure, and add changelog. Co-authored-by: Cursor <cursoragent@cursor.com>
Trigger a new store sync when retrying watcher stop; empty knownDevices no longer matches the native-segwit-only path. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
This compatibility layer should be handled by #1046 , gated by synonymdev/bitkit-core#113
I recommend drop this file and wait for Bitkit-core implementation
Summarizing, we need to implement wallet-scope backup migrations because of the new walletId. The logic must be in Bitkit-core for single source of true
There was a problem hiding this comment.
Agree long-term: wallet-scoped backup migration belongs in Core (#113) and the app integration in #1046.
BackupRestoreCompat is a minimal bridge added after Codex flagged legacy VSS restore failing on missing walletId once we bumped to 0.3.9 (P1 on this PR). It keeps existing hot-wallet cloud backup restore working until Core exposes migration APIs — it does not add Trezor/HW tag product scope for 2.4.0.
Happy to drop it as soon as synonymdev/bitkit-core#113 lands and wire the proper flow in #1046.
There was a problem hiding this comment.
nit: might also be patch-able by making the serializer more lenient on Kotlin's Json instance, maybe a derivation of our global di.json instance (if that wasn't already tried and deemed not applicable here); or perhaps by playing with an optional walletId on a domain replica of the original serialized model originating from core, which adds optional fields, given that the json instance is already configured with isLenient = true
4 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".