feat: persist hardware wallet activities - #648
Open
jvsena42 wants to merge 16 commits into
Open
Conversation
…oes through the reconciling path
jvsena42
marked this pull request as ready for review
August 3, 2026 17:36
Greptile SummaryThe PR makes hardware-wallet activity participate fully in wallet-scoped persistence and activity operations.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| Bitkit/Managers/HwWalletManager.swift | Reconciles and serializes wallet-scoped hardware snapshots; the previously reported failed-write deduplication defect is repaired by conditionally invalidating the cache. |
| Bitkit/Services/CoreService.swift | Adds wallet-scoped activity replacement, mutation, transaction-detail persistence, and synchronized boost-cache handling. |
| Bitkit/Services/HwSnapshotMerge.swift | Implements snapshot reconciliation while preserving app-authored transfer metadata and pending transfers. |
| Bitkit/Services/WatchOnlyAccountService.swift | Replaces the removed core extended-public-key serializer with validated local decoding. |
| Bitkit/ViewModels/TransferViewModel.swift | Resolves the hardware wallet scope before signing and records the resulting transfer against that wallet. |
Sequence Diagram
sequenceDiagram
participant W as Hardware watcher
participant M as HwWalletManager
participant Q as SnapshotPersistQueue
participant C as bitkit-core
W->>M: watcher snapshot
M->>M: Merge address-type snapshots
M->>Q: Enqueue wallet-scoped persistence
Q->>C: Replace snapshot and transaction details
alt Persistence succeeds
C-->>Q: Success
else Persistence fails
C-->>Q: Error
Q->>M: Invalidate matching dedupe entry
W->>M: Next identical snapshot
M->>Q: Retry persistence
end
Reviews (2): Last reviewed commit: "fix: recover HW transfer after re-add HW..." | Re-trigger Greptile
…ot retries instead of being skipped
ovitrif
dismissed
their stale review
August 3, 2026 21:17
Replacing approve with utAck comment — PR was not fully device-tested.
jvsena42
marked this pull request as draft
August 4, 2026 10:10
This comment was marked as resolved.
This comment was marked as resolved.
…t read cannot tear it
jvsena42
marked this pull request as ready for review
August 4, 2026 12:05
Member
Author
|
Description updated with test videos and some findings, specially test case 7c. I also opened issues for findings out of the scope of this PR. Waiting for AI review before re-requesting human review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR ports bitkit-android #1044 to iOS, making watch-only hardware wallet activity a full participant in bitkit-core's wallet-scoped storage. iOS already stored hardware activity under a derived wallet id, so this fills the gaps that were left behind:
Scoping point 4's boost relationships per wallet turned
ActivityService's boostTxIds cache from a singleSet<String>into a[String: Set<String>], which introduced a data race that crashed the app during manual testing (EXC_BAD_ACCESSinDictionary._Variant.lookup, reached fromActivityListViewModel.filterOutReplacedSentTransactions). Readers run on whichever executor callsgetTxIdsInBoostTxIds, while writers run onMainActorand the core queue, so an in-place keyed write could resize the storage mid-lookup — where the previous whole-valueSetassignment was a single store and never tore. The cache is now behind anOSAllocatedUnfairLock. A lock rather than an actor becauseupdateBoostTxIdsCacheis called from inside the synchronousServiceQueue.background(.core)blocks, which must stay non-async for the reason documented onreplaceHwSnapshot; making itasyncwould either lose that serialization or decouple the cache update from the write it belongs to. Dropping the twoawait MainActor.runhops also takes a main-thread round trip off the watcher poll path.The PR also moves bitkit-core from 0.4.2 to 0.5.3. Version 0.4.2 was cut from a branch that never merged upstream, and its only unique symbol was the extended-public-key serializer used when claiming a watch-only account. That single dependency pinned the repo to a dead-end tag and blocked every upgrade, so this decodes the key locally instead, matching the approach in #632. Everything the hardware wallet integration relies on is unchanged between the two versions.
Linked Issues/Tasks
nilcheck now caches an empty set, wheremaster's.isEmptycheck re-scanned on every call); left out of scope because after the lock it is wasted work rather than a correctness problem.ServiceQueue's async overload silently defeats the serialization its callers assume. Surfaced while reviewing this PR and worked around here inreplaceHwSnapshot; the general fix touches ~40 call sites and was kept out.Screenshot / Video
receive-two-transactions.mov
tag-filter.mov
transfer-from-savings.mov
tags.mov
transfer-details.mov
recover-hw-transfer.mov
server-accounts.mov
QA Notes
Manual Tests
regression:App on-chain and LN activity → Activity Item, tags, contacts, Explore, Savings and Spending lists, boost: behaviour matches master.regression:Settings → Advanced → Server accounts → set up an account against Pubky Ring: the claim is still accepted after the local key decoding change.Automated Checks
BitkitTests/HwSnapshotMergeTests.swiftcovers the snapshot reconciliation rules — a stored row missing from a snapshot is dropped, a transfer is not, and transfer metadata is carried forward and matched by transaction id rather than activity id.BitkitTests/WatchOnlyAccountServiceTests.swiftpins the local extended-public-key decoding against bitkit-core's own test vectors, plus invalid character, bad checksum, off-curve key, and wrong-length rejection.BitkitTests/HwWalletManagerTests.swiftcovers transaction details reaching persistence scoped to the derived wallet id, details deduped across two address-type watchers, an unconfirmed transaction whose timestamp drifts between polls not re-writing to core, and wallet id resolution for a device.BitkitTests/TransferViewModelHwTests.swiftcovers an unresolvable wallet id aborting before the device signs or anything is broadcast.BitkitTests/ActivityListTest.swiftgainstestConcurrentBoostCacheAccessIsSafe, hammering the boostTxIds cache with concurrent reads across 8 seeded and 64 cold wallet ids against upserting writers. Cold reads are what insert new keys and force the dictionary to resize, which is the window that crashed. It only catches the bug under Thread Sanitizer — on the unfixed code it passed 3/3 plain runs, and TSan names the exact site (Swift access race … in ActivityService.refreshBoostTxIdsCache). Verified in both directions: unfixed + TSan fails, fixed + TSan passes. Worth deciding whether CI should gain a TSan job, since without one this is a deadlock smoke test rather than a race guard.BitkitTests/HwWalletManagerTests.swiftandBitkitTests/HwWalletManagerFundingTests.swiftrecord wallet-scoped snapshots instead of bare activity lists;BitkitTests/HwTransferMocks.swiftgains wallet id resolution.BitkitTests/ActivityHardwareTests.swiftis gone because the funding transaction is now stored only under the hardware wallet, so there is no duplicate to collapse. It is replaced by a case proving the same transaction id in two wallet scopes stays two distinct activities.UtxoSelectionTestsandDustChangeHelperTestspass on their own but hit the live Blocktank regtest deposit endpoint and flaked once with a 404 during a long run.AddressTypeIntegrationTests,BlocktankTests,LdkMigration,ChannelPurchaseFlow, andBroadcastConnectivityTestsneed a live node and hang without the regtest stack. The manual tests above were also not run — they need a Trezor Bridge emulator, so the hardware journeys rest on review and QA.node scripts/validate-translations.jsreported no errors.CoreService.swift; 99 tests passed across the six affected suites (ActivityTests,ActivityHardwareTests,HwSnapshotMergeTests,HwWalletManagerTests,HwWalletManagerFundingTests,TransferViewModelHwTests); SwiftFormat lint clean. The full offline suite has not been re-run since that change, so the 654/698 figures above still describe the earlier runs.