feat(swift-sdk): surface payload-only special-tx involvement in account transaction lists#4108
Conversation
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRust wallet changesets now carry marked-used addresses and highest-used watermarks through event processing and FFI persistence. Swift persistence models track payload-only transaction involvement, include it in transaction views and counts, and preserve referenced transactions during wallet deletion. ChangesRust usage delta persistence
Swift payload-only transaction involvement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WalletEvent
participant build_core_changeset
participant collect_usage_deltas
participant CoreChangeSet
participant FFIPersister
WalletEvent->>build_core_changeset: process transaction or block event
build_core_changeset->>collect_usage_deltas: derive usage deltas
collect_usage_deltas->>CoreChangeSet: store marked-used addresses and watermarks
CoreChangeSet->>FFIPersister: persist usage deltas
FFIPersister->>FFIPersister: group marked-used addresses by pool
sequenceDiagram
participant PlatformWalletPersistenceHandler
participant PersistentTransaction
participant PersistentAccount
participant AccountDetailView
PlatformWalletPersistenceHandler->>PersistentTransaction: upsert transaction
PlatformWalletPersistenceHandler->>PersistentAccount: record involved account
PersistentAccount->>PersistentTransaction: expose involvedTransactions
AccountDetailView->>PersistentAccount: read account involvement
AccountDetailView->>PersistentTransaction: display unified transaction list
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit 339431a) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This PR adds an explicit PersistentTransaction.involvedAccounts / PersistentAccount.involvedTransactions join to represent payload-only special-transaction involvement, and correctly unions it into the TXO-derived transaction sets across the account and wallet views. The join and its population are sound, but the existing global orphan sweep in deleteWalletData is not updated for the new invariant: deleting any wallet can silently erase payload-only transactions belonging to another live wallet.
Source: Sol reviewer gpt-5.6-sol; Sonnet reviewer claude-sonnet-5; verifier claude-sonnet-5.
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3759-3764: Wallet deletion's global orphan sweep deletes other wallets' payload-only transactions
deleteWalletData ends with an unscoped FetchDescriptor<PersistentTransaction>() and deletes every row whose outputs, inputs, and pendingInputs are empty. This PR changes the invariant behind that predicate: payload-only special transactions can now legitimately belong to a live account solely through involvedAccounts while having no TXO rows. Deleting an unrelated wallet therefore sweeps the other wallet's payload-only transaction, SwiftData nullifies its involvedTransactions relationship, and the transaction disappears from every list this PR adds. Include involvedAccounts.isEmpty in the orphan predicate and cover the unrelated-wallet deletion path with a regression test.
|
Addressed the blocking finding in bd08764: the orphan sweep in 🤖 Addressed by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift (1)
57-67: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExclude sibling-network accounts from the transaction count.
The same
walletIdmay exist on multiple networks. Includewallet.networkRawin this query or the badge can count another network’s payload-only transactions.Proposed change
let walletId = wallet.walletId +let networkRaw = wallet.networkRaw ... _walletAccounts = Query( - filter: `#Predicate`<PersistentAccount> { $0.wallet.walletId == walletId } + filter: `#Predicate`<PersistentAccount> { + $0.wallet.walletId == walletId + && $0.wallet.networkRaw == networkRaw + } )Also applies to: 80-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift` around lines 57 - 67, Update WalletDetailView.init(wallet:) so both the PersistentTxo and PersistentAccount queries match the wallet’s networkRaw in addition to walletId. Use the current wallet.networkRaw value in each predicate, ensuring transaction counts and account data exclude records from sibling networks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/core_wallet_types.rs`:
- Around line 341-350: Update the FFI account bucketing logic that builds
`ffi_accounts`/`by_account` from `cs.records` to also include every key in
`cs.account_highest_used`, creating an empty record bucket when an account has
no records. Preserve existing record grouping and ensure watermark-only accounts
are emitted.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift`:
- Around line 41-43: Replace the selectedTransaction state and transaction sheet
presentation in AccountDetailView with a NavigationLink-driven drill-down to
TransactionDetailView. Update the transaction row/button handling and the
corresponding presentation block to pass the selected PersistentTransaction
through the navigation link, removing the sheet-based flow while preserving the
existing transaction selection behavior.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swift`:
- Around line 57-67: Update WalletDetailView.init(wallet:) so both the
PersistentTxo and PersistentAccount queries match the wallet’s networkRaw in
addition to walletId. Use the current wallet.networkRaw value in each predicate,
ensuring transaction counts and account data exclude records from sibling
networks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff9ae0c8-85b6-434b-98dd-6e651ce94b1d
📒 Files selected for processing (12)
packages/rs-platform-wallet-ffi/src/core_wallet_types.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/core_bridge.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentTransaction.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/TransactionListView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WalletDeletionTests.swift
… txs A special transaction can involve an account purely through its payload — a ProRegTx whose owner / voting key hash matches a Provider Owner / Voting Keys pool address — creating no TXO in that account. Per-account transaction membership was recovered exclusively through the TXO graph, so those txs never appeared in the account's transaction count (AccountDetailView), the wallet-level count (WalletDetailView), or the wallet transaction list (TransactionListView). - PersistentTransaction.involvedAccounts <-> PersistentAccount .involvedTransactions: explicit many-to-many recording every account whose changeset bucket carried the tx record (a superset of the TXO-derived membership; .nullify on both ends — participation, not value ownership). - PlatformWalletPersistenceHandler.upsertTransaction appends the matched account idempotently (by persistentModelID) — it is already invoked once per matched account since the Rust changeset buckets records by account_type, including payload-only matches. - AccountDetailView unions involvedTransactions with the TXO-derived set for the pool-summary count and a new Transactions card that lists the account's txs (opens TransactionDetailView). - TransactionListView / WalletDetailView union the same join so payload-only txs show consistently at wallet level. Verified on a mainnet wallet: after a compact-filter rescan from height 1030600, ProRegTx 5076f9ed…d605da85 (height 1030673, zero TXOs in the account) is counted and listed by the Provider Owner Keys account detail view. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
deleteWalletData's post-delete orphan sweep matched any PersistentTransaction with empty outputs / inputs / pendingInputs. Payload-only special txs have no TXOs anywhere yet belong to a live account through involvedAccounts, so deleting an unrelated wallet swept the other wallet's payload-only history. Include involvedAccounts.isEmpty in the sweep predicate; the deleted wallet's own payload-only rows are still collected because its account deletion nullifies their join links before the sweep runs. Adds testDeleteWalletDataPreservesSiblingPayloadOnlyTransactions covering the unrelated-wallet deletion path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…work The same 32-byte walletId legitimately exists once per network (same mnemonic imported on mainnet and testnet), so matching the payload- only involvement query on walletId alone folded the sibling network's accounts — and their payload-only txs — into this wallet row's transaction badge. Add wallet.networkRaw to the predicate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
claude/jovial-swanson-861a07
branch
from
339431a to
e1e3dcf
Compare
|
Rebased onto v4.1-dev now that #4107 is merged — the cherry-picked commit and the watermark-only bucket fix both dropped out (the merged #4107 already carries that fix plus an equivalent regression test), leaving only this PR's own commits. Also addressed the outside-diff finding in e1e3dcf: 🤖 Addressed by Claude Code |
2 participants
Issue being fixed or feature implemented
A special transaction can involve an account purely through its payload — e.g. a ProRegTx whose owner key hash matches a Provider Owner Keys pool address — creating no TXO in that account. Per-account transaction membership in the Swift SDK was recovered exclusively through the TXO graph (
coreAddresses.flatMap(\.txos)→ creating/spending tx), so payload-only transactions never appeared in the account's Transactions count, the wallet-level count, or the wallet transaction list. This is the remaining UI gap (G3) of the mainnet owner-key "Absent" investigation; #4107 fixed the Rust-side persistence seam (G1/G2) this builds on.Stacked on #4107 — the first commit here is #4107's
fix(platform-wallet)commit cherry-picked; review only the second commit. Will rebase once #4107 merges.What was done?
PersistentTransaction.involvedAccounts↔PersistentAccount.involvedTransactions: new explicit many-to-many recording every account whoseWalletChangeSetFFIbucket carried the tx record — a superset of TXO-derived membership that includes payload-only involvement..nullifydelete rule on both ends (participation, not value ownership; wallet-wipe order unaffected).PlatformWalletPersistenceHandler.upsertTransaction(account:tx:)appends the matched account idempotently (compared bypersistentModelID). It is already invoked once per matched account — the Rust changeset bucketscs.recordsbyrecord.account_type, including payload-only matches — so no FFI change is needed.AccountDetailView: the pool-summary Transactions count now unions the TXO-derived set withinvolvedTransactions, and a new Transactions card lists the account's distinct txs (unconfirmed first, thenfirstSeendesc), each openingTransactionDetailView. Rendered for both address-pool and provider key-material accounts; skipped for PlatformPayment.TransactionListView/WalletDetailView: union the same join so payload-only txs appear consistently in the wallet-level list and count.How Has This Been Tested?
./build_ios.sh --target sim+ SwiftExampleApp simulator build, both clean.5076f9ed731bcb7b5ddf47e21bf1262d22d99665ca89bb6329e6e706d605da85(height 1,030,673, zero TXOs in the account) now shows in the Provider Owner Keys account detail: Transactions 21 / TXOs 0, listed in the new card, and its detail sheet opens with the right txid/height. SwiftData ground truth confirmed via sqlite: join rows written for (ProRegTx ↔ Provider Owner Keys),externalHighestUsedadvanced from −1, owner/voting pool addresses flippedisUsed=1(fix(platform-wallet): persist address usage discovered during SPV block processing #4107's seam verified end-to-end as well).Breaking Changes
None.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes