feat: watcher emits persistence-ready watch-only activities#109
Conversation
…pper Subtask 2 of #107. Lets core own the History->Activity mapping that iOS and Android would otherwise hand-reconstruct (and drift on). Part A — root-cause fix for fee rate: HistoryTransaction now carries an Option<f64> fee_rate (sat/vB), computed in map_bdk_tx_to_history from the fee and the raw tx vsize. This flows into WatcherEvent::TransactionsChanged, so the watcher no longer forces callers to hardcode feeRate: 1. Part B — watch_only_activity_from_history(wallet_id, txs) -> Vec<Activity>: a pure mapper (no DB writes). Direction -> PaymentType (SelfTransfer -> Sent), deterministic id = tx_id so re-emitted lists upsert in place, address left empty for watch-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55ee1fc3f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Aligns the mapper with how bitkit-ios and bitkit-android actually build onchain activities: - id = tx_id (confirmed: iOS CoreService and Android HwWalletRepo both key onchain activities on the txid). - Merge HistoryTransaction rows by txid before classifying. A hardware device's accounts/script-types are watched separately, so the same tx can appear once per account (an internal send+receive). Summing received/sent and classifying the whole tx yields one activity instead of duplicates that would collapse under id = tx_id. Mirrors Android's toMergedActivities(). Direction/amount come from core's classify_tx (the canonical classifier), fee/fee_rate are taken from the account that paid (received-only rows carry none), and first-seen txid order is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update: aligned with bitkit-ios / bitkit-android conventionsChecked how both apps key and build onchain activities to settle the open
While there I found the apps do one more thing the 1:1 mapper didn't: they merge
So the mapper is now a faithful drop-in for the per-platform logic rather than just a field copy. Tests cover received / sent / self-transfer / same-txid merge / ordering / unconfirmed defaults / deterministic id. |
…gen bindings Addresses Codex review on #109: - P2: get_transaction_history and the watcher's event builder called list_transactions(false), so tx.transaction was None and the new HistoryTransaction.fee_rate stayed None for every history/watcher consumer. Switch both to list_transactions(true) so vsize (hence fee_rate) is available. - P1: the mapper's results carry address="" and timestamp=0, which violate the activity DB CHECK constraints (length(address) > 0, timestamp > 0). bitkit-ios and bitkit-android keep these as in-memory display models and never insert them. Reworded the doc to say so and dropped the "callers upsert_activity" guidance that would have produced DB errors. - Regenerated Swift + Python bindings for the new fee_rate field and watch_only_activity_from_history export. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks, I hope the fact that we didn't share the full context why the 2 apps differ in implementation is not going to cause a lot of trouble to get this PR in its merge-able state. The reason Android works more in-memory is mainly because it didn't have the later #103 PR at the time of the implementation. The scope of that task actually surfaced as an idea during the Android implementation of Trezor subtask 1:
Since iOS had the primitives in place, Joao and myself synced to make sure iOS doesn't have to carry over this requirement to refactor later, which Android's first PR (ported by iOS' PR) embedded. After #103 was merged, I updated the Android epic and its subtask (6) for optimisation with specs that came as a result of investigation how to levarage the walletId/wallet-scoped activities. Links:
|
TxId might be problematic for cases where we send coins from Trezor to app's onchain wallet. My understanding is that activities for such actions will both have the same txId. This is one of the reasons we had to include more discriminators in the |
ovitrif
left a comment
There was a problem hiding this comment.
Leaving these together so the intended direction is clear. The watcher does not need to persist into the DB itself, but the watcher payload should be normal Core activity data that Bitkit can persist.
…view)
Reworks the watch-only flow per review: instead of a downstream display-only
mapper, the watcher itself builds Core activity data the app can persist
through the normal activity APIs.
- WatcherEvent::TransactionsChanged now carries activities: Vec<Activity> +
transaction_details: Vec<TransactionDetails> (replacing Vec<HistoryTransaction>),
built inside the watcher where the BDK Wallet, raw txs, and is_mine checks
are still available.
- WatcherParams gains wallet_id; the boundary stays at the watched address type
(no cross-address-type txid merging).
- Correct, persistence-compatible semantics from the watched wallet's view:
* Received: real owned address, value = received, fee/fee_rate = 0 (the fee was
the sender's, even when BDK attaches one to the row).
* Sent: destination address, value = sent - received - fee, fee = fee
(Bitkit renders value + fee).
* SelfTransfer: value = 0, fee = fee (so value + fee isn't doubled).
* Positive timestamp (now) for unconfirmed txs so rows are DB-valid.
- Extracted map_bdk_tx_to_detail (shared with get_transaction_detail) and added
watch_only_activity_from_detail; removed the old standalone mapper.
- Harden upsert: preserve user-set contact (COALESCE) and keep the existing
timestamp while a tx is still unconfirmed, so watcher refreshes don't churn
user/first-seen state.
- Regenerated Swift + Python bindings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ovitrif thanks for the thorough review — this was exactly the right call. Pushed a redesign in 540f894 that moves activity-building into the watcher and emits persistence-ready |
ben-kaufman
left a comment
There was a problem hiding this comment.
Review comments from local pass.
Addresses @ben-kaufman review: - update_onchain_activity_by_id is a literal replacement again, so update_activity callers can still clear contact and move an unconfirmed tx's timestamp. - Field preservation now lives in the upsert path: upsert_onchain_activities keeps the existing contact when the incoming one is None (COALESCE) and keeps the existing timestamp while a tx is unconfirmed (snapping to the block time on confirmation). Singular upsert_activity routes through the batch upsert so both entry points behave the same. - TransactionDetails.amount_sats is now fee-excluded (net + the wallet's own fee), matching its documented contract and the activity value; receive-only rows are unaffected since their fee is 0. Tests: pure-replacement update, batch + singular upsert preservation, amount_sats for received/sent. Regenerated all platform bindings + artifacts via build.sh all. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ovitrif on the |
ben-kaufman
left a comment
There was a problem hiding this comment.
Full review looks good. I rechecked the watcher/activity flow, generated surfaces, and the resolved threads.
…ivity # Conflicts: # Package.swift # bindings/android/lib/src/main/jniLibs/arm64-v8a/libbitkitcore.so # bindings/android/lib/src/main/jniLibs/armeabi-v7a/libbitkitcore.so # bindings/android/lib/src/main/jniLibs/x86/libbitkitcore.so # bindings/android/lib/src/main/jniLibs/x86_64/libbitkitcore.so # bindings/ios/BitkitCore.xcframework.zip # bindings/ios/BitkitCore.xcframework/ios-arm64-simulator/libbitkitcore.a # bindings/ios/BitkitCore.xcframework/ios-arm64/libbitkitcore.a # bindings/python/bitkitcore/libbitkitcore.dylib # src/modules/activity/tests.rs # src/modules/activity/types.rs # src/modules/onchain/implementation.rs # src/modules/onchain/listener.rs # src/modules/onchain/tests.rs
|
LGTM, reviewed after merge. Thanks everyone for the help 🙏🏻 |
Summary
Subtask 2 of #107, reworked per @ovitrif's review. The onchain xpub watcher now emits persistence-ready Core activity data that Bitkit stores through the existing activity APIs — rather than the app hand-reconstructing activities per-platform (and drifting).
What it does
Watcher builds the payload (where the data still exists).
WatcherEvent::TransactionsChangednow carries:activities: Vec<Activity>transaction_details: Vec<TransactionDetails>…instead of
Vec<HistoryTransaction>. They're built inside the watcher using the BDKWallet, raw txs (list_transactions(true)), confirmation time, fee/vsize, andis_mineownership — the decode path is shared withget_transaction_detailvia the extractedmap_bdk_tx_to_detail.Wallet boundary = the watched address type.
WatcherParamsgainswallet_id. One activity per tx, scoped to that wallet; the same txid under two address types becomes two wallet-scoped activities (no cross-address-type merge).Persistence-compatible semantics, from the watched wallet's perspective:
received0(sender paid it)sent - received - feefee0feeSelf-transfer is
value = 0, fee = feeso Bitkit'svalue + feetotal isn't doubled. Unconfirmed txs get a positive (now) timestamp so rows are DB-valid and sort to the top while pending.Upsert hardening so watcher refreshes don't clobber user/first-seen state:
contactis preserved when the incoming value isNone(COALESCE), and an unconfirmed tx keeps its existing timestamp until it confirms. (seen_atand tags were already preserved.)Also:
HistoryTransactionkeeps the newfee_ratefield forget_transaction_historyconsumers; both that path and the watcher now uselist_transactions(true). The earlier standalonewatch_only_activity_from_historymapper is removed.Tests
cargo test --lib modules::onchain::(55) andmodules::activity::(167) pass. New coverage:watch_only_activity_from_detail: received-drops-sender-fee, sent value/fee split, self-transfer zero-value, unconfirmednowtimestamp, matchingTransactionDetails.test_upsert_preserves_contact_and_unconfirmed_timestamp.Swift + Python bindings regenerated (iOS xcframework / Android Kotlin / dylib rebuilt at release via build.sh, per repo convention).
🤖 Generated with Claude Code