feat: persist hardware wallet activities - #648
Conversation
Greptile SummaryThe PR promotes hardware-wallet activity to wallet-scoped persisted storage and preserves transaction details and transfer metadata during watcher reconciliation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported failed-snapshot deduplication issue is addressed by conditionally invalidating the cached snapshot after a write failure, allowing the next identical watcher snapshot to retry.
|
| Filename | Overview |
|---|---|
| Bitkit/Managers/HwWalletManager.swift | Aggregates watcher snapshots, serializes wallet-scoped persistence and deletion, and now invalidates snapshot deduplication state after failed writes. |
| Bitkit/Services/HwSnapshotMerge.swift | Reconciles watcher snapshots with stored activity while preserving transfer metadata and deferring deletion for incomplete snapshots. |
| Bitkit/Services/CoreService.swift | Adds wallet-scoped activity operations and synchronized wallet-specific boost transaction caches. |
| Bitkit/ViewModels/TransferViewModel.swift | Resolves the hardware wallet scope before signing and records the resulting transfer in that scope. |
| Bitkit/Services/WatchOnlyAccountService.swift | Replaces the removed core serializer dependency with locally validated extended-public-key decoding. |
| BitkitTests/HwWalletManagerTests.swift | Covers wallet-scoped snapshot persistence, transaction-detail deduplication, and retrying an identical snapshot after a transient persistence failure. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
W[Hardware wallet watchers] --> M[Merge address-type snapshots]
M --> C{Snapshot complete?}
C -->|No| U[Upsert without pruning]
C -->|Yes| R[Reconcile and prune missing rows]
U --> S[Wallet-scoped core storage]
R --> S
S --> A[Combined activity views]
T[Transfer to spending] --> I[Resolve hardware wallet ID before signing]
I --> S
Reviews (4): Last reviewed commit: "feat: enable asign HW transactions to a ..." | Re-trigger Greptile
Replacing approve with utAck comment — PR was not fully device-tested.
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
|
draft for solve conflicts |
Dismissing approval — PR has merge conflicts and is still draft.
…oes through the reconciling path
…ot retries instead of being skipped
…t read cannot tear it
15104b1 to
0fc7ed8
Compare
ovitrif
left a comment
There was a problem hiding this comment.
Assigning a contact to a hardware activity still leaves that transaction out of the contact Activity screen, because get(contact:) only queries the default wallet scope.
Pull request was converted to draft
ovitrif
left a comment
There was a problem hiding this comment.
Thanks for addressing the contact Activity gap. Hardware assignments now reach contact history across wallet scopes, and this looks good to merge.
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.