Skip to content

feat: persist hardware wallet activities - #648

Open
jvsena42 wants to merge 16 commits into
masterfrom
feat/persist-hardware-activities
Open

feat: persist hardware wallet activities#648
jvsena42 wants to merge 16 commits into
masterfrom
feat/persist-hardware-activities

Conversation

@jvsena42

@jvsena42 jvsena42 commented Jul 31, 2026

Copy link
Copy Markdown
Member

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:

  1. Persists the Electrum transaction details the hardware watcher emits, so Explore shows inputs and outputs for a hardware transaction instead of an empty section.
  2. Reconciles each watcher snapshot against what is already stored rather than only adding to it, so a reorged or replaced transaction stops showing. Transfer information the app wrote itself survives the reconcile, including a pending transfer the watcher has not seen yet.
  3. Records a transfer to spending funded from a hardware wallet against that wallet, so it reads as a single hardware transfer rather than a main-wallet row shadowed by a hardware duplicate. The wallet is resolved before the device is asked to sign, so a transfer is never recorded against the wrong wallet.
  4. Scopes the remaining activity operations to the wallet that owns the activity: seen state, deletion, contacts, and boost relationships. Marking everything as seen and clearing all activity previously skipped hardware wallets entirely.
  5. Enables tags and contacts on hardware activities, which were hidden behind an interim gate while those operations were unscoped. Boost stays unavailable, since a watch-only wallet has no signing keys.
  6. Tells the user when transaction details are unavailable instead of quietly hiding the section.

Scoping point 4's boost relationships per wallet turned ActivityService's boostTxIds cache from a single Set<String> into a [String: Set<String>], which introduced a data race that crashed the app during manual testing (EXC_BAD_ACCESS in Dictionary._Variant.lookup, reached from ActivityListViewModel.filterOutReplacedSentTransactions). Readers run on whichever executor calls getTxIdsInBoostTxIds, while writers run on MainActor and the core queue, so an in-place keyed write could resize the storage mid-lookup — where the previous whole-value Set assignment was a single store and never tore. The cache is now behind an OSAllocatedUnfairLock. A lock rather than an actor because updateBoostTxIdsCache is called from inside the synchronous ServiceQueue.background(.core) blocks, which must stay non-async for the reason documented on replaceHwSnapshot; making it async would either lose that serialization or decouple the cache update from the write it belongs to. Dropping the two await MainActor.run hops 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

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

  • 1. Settings → Advanced → Dev Settings → Trezor → pair the Bridge emulator: Home shows the hardware wallet tile with its balance and blue activity rows.
  • 2. Fund two hardware addresses with different amounts → mine a block → Home → Show All: both blue received rows appear exactly once.
  • 3a. Blue hardware receive → Activity Item → Tag: the tag button is available and adding a tag succeeds.
    • 3b. back → reopen the same activity: the tag is still there.
    • 3c. Activity Item → Explore: Inputs and Outputs are populated instead of empty.
  • 4. All Activity → tag filter → select the hardware tag: the tagged blue row remains and untagged rows are gone.
  • 5a. Hardware Wallet → Transfer To Spending → 25% → Continue → approve on device → Continue Using Bitkit: Home shows exactly one new blue Transfer / From Savings row, not a main-wallet row plus a hardware duplicate.
    • 5b. Tap that row → Activity Item: shows "From savings".
    • 5c. Wait for the next watcher poll: the row keeps its transfer styling and does not revert to a plain send.
  • 6. All Activity → switch Sent, Received, Other, All: hardware receives appear under Received, the hardware transfer under Other, and All shows the combined list with no duplicate.
  • 7a. Settings → Hardware Wallets Settings → remove the paired device: its activity disappears from Home and All Activity.
    • 7b. Re-pair the same wallet: its activity rebuilds without duplicate rows.
    • 7c. Do a hardware transfer to spending, wait for the channel to become usable, remove the device, re-pair it → the rediscovered funding tx reads as a transfer, not a plain send.
  • 8. regression: App on-chain and LN activity → Activity Item, tags, contacts, Explore, Savings and Spending lists, boost: behaviour matches master.
  • 9. 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

  • Unit tests added: BitkitTests/HwSnapshotMergeTests.swift covers 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.
  • Unit tests added: BitkitTests/WatchOnlyAccountServiceTests.swift pins 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.
  • Unit tests added: BitkitTests/HwWalletManagerTests.swift covers 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.
  • Unit tests added: BitkitTests/TransferViewModelHwTests.swift covers an unresolvable wallet id aborting before the device signs or anything is broadcast.
  • Unit tests added: BitkitTests/ActivityListTest.swift gains testConcurrentBoostCacheAccessIsSafe, 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.
  • Unit tests modified: BitkitTests/HwWalletManagerTests.swift and BitkitTests/HwWalletManagerFundingTests.swift record wallet-scoped snapshots instead of bare activity lists; BitkitTests/HwTransferMocks.swift gains wallet id resolution.
  • Test coverage removed: the hardware-duplicate collapsing test in BitkitTests/ActivityHardwareTests.swift is 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.
  • Local run: 654 tests across the 60 offline unit suites passed on the iPhone 16 simulator. UtxoSelectionTests and DustChangeHelperTests pass on their own but hit the live Blocktank regtest deposit endpoint and flaked once with a 404 during a long run.
  • Not run locally: AddressTypeIntegrationTests, BlocktankTests, LdkMigration, ChannelPurchaseFlow, and BroadcastConnectivityTests need 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.
  • Local run: SwiftFormat lint passed for all changed Swift files, and node scripts/validate-translations.js reported no errors.
  • Local product QA (2026-08-03/04, iPhone 17 sim + Bridge emulator): manual checklist items 1–8 verified. Item 9 (Pubky Ring watch-only claim) not run. Offline unit suite: 698 BitkitTests passed (skipped live/hang-prone suites matching CI + DustChangeHelperTests).
  • After the boostTxIds lock fix (2026-08-04): build succeeded for the app plus the notification and widget extension targets, which share 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.
  • CI: standard build and test checks run by the PR bot.

@jvsena42 jvsena42 self-assigned this Jul 31, 2026
@ovitrif ovitrif added this to the 2.5.0 milestone Jul 31, 2026
@jvsena42
jvsena42 marked this pull request as ready for review August 3, 2026 17:36
@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown

Greptile Summary

The PR makes hardware-wallet activity participate fully in wallet-scoped persistence and activity operations.

  • Reconciles watcher snapshots, persists transaction details, and preserves transfer metadata.
  • Scopes activity mutations and transfer records to their owning wallet.
  • Serializes per-wallet persistence and protects the wallet-scoped boost cache.
  • Updates bitkit-core and replaces its removed extended-public-key serializer locally.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

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
Loading

Reviews (2): Last reviewed commit: "fix: recover HW transfer after re-add HW..." | Re-trigger Greptile

Comment thread Bitkit/Managers/HwWalletManager.swift
ovitrif

This comment was marked as outdated.

@ovitrif ovitrif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck.

The failed HW snapshot dedupe retry is fixed in 8d28c65, and the rest of the hardware activity persistence work looks good to merge.

@ovitrif
ovitrif dismissed their stale review August 3, 2026 21:17

Replacing approve with utAck comment — PR was not fully device-tested.

@jvsena42
jvsena42 marked this pull request as draft August 4, 2026 10:10
@jvsena42

This comment was marked as resolved.

@jvsena42

jvsena42 commented Aug 4, 2026

Copy link
Copy Markdown
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

@jvsena42
jvsena42 requested a review from ovitrif August 4, 2026 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants