fix(swift-sdk): drop transitive keypath from PersistentTxo unspent prefetch#3691
Conversation
…efetch `relationshipKeyPathsForPrefetching = [\.account, \.account?.wallet]` in `PlatformWalletPersistenceHandler.loadWalletList` asserted inside SwiftData internals the first time the fetch ran with non-empty restorable wallets — the chained `account?.wallet` second hop hits a code path SwiftData's prefetch resolver doesn't support, only direct single-hop relationships do. Any wallet registration via the platform-wallet Rust crate after the `restorable.isEmpty` gate opens (restart with persisted wallet, second wallet import, etc.) tripped the assertion. Drop the chained second hop. The `account → wallet` relationship still resolves on demand at the use site; only the bulk prefetch optimization for that hop is removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPlatformWalletPersistenceHandler adjusts the SwiftData prefetch configuration in ChangesSwiftData Prefetch Optimization
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "889d644cea58e1eaca5d8bfc37abe4231fc545b13ad3de7128b6d95b370f609a"
)Xcode manual integration:
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
One-line SwiftData fix is correct and well-scoped. Two non-blocking review items remain: the block comment above the fetch still describes the removed transitive prefetch, and no automated test exercises the wallet-restore path where the original crash occurred.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
2 additional finding(s)
nitpick: Stale comment still claims the fetch prefetches `account.wallet`
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 2444)
The block comment says the fetch "Prefetches account.wallet to keep the legacy-walletId routing path … from triggering one SwiftData fault per row when we resolve the parent wallet." After this PR the descriptor only prefetches \.account; the account → wallet hop now faults lazily inside the loop (lines 2485–2492). This is the same justification a future reader will reach for when considering re-adding the chained keypath, so leaving it stale invites the regression to be reintroduced. Update the comment to state that only account is prefetched, that the wallet hop intentionally faults on demand, and that the chained keypath trips a SwiftData internal assertion.
suggestion: Wallet-restore regression has no automated coverage
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (line 2462)
This single keypath edit is the entire behavioral fix, but no test exercises loadWalletList() against a populated SwiftData store. A grep confirms loadWalletList is referenced only in PlatformWalletPersistenceHandler.swift, PlatformWalletManager.swift, and WalletManagerStore.swift — never in SwiftExampleAppTests or SwiftDashSDKTests. The original crash only triggers after the restorable.isEmpty gate opens (second wallet import or app restart with persisted wallets) and only surfaces at runtime inside SwiftData, so the inline comment is the only signal preventing a future contributor from re-adding \.account?.wallet or another transitive keypath. Add an integration test that seeds a PersistentWallet with a restorable PersistentAccount and at least one unspent PersistentTxo (covering both the populated-walletId and legacy empty-walletId paths in lines 2483–2495), then invokes the load path, so the assertion is caught in CI rather than on a user's device.
🤖 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.
- [NITPICK] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:2444-2450: Stale comment still claims the fetch prefetches `account.wallet`
The block comment says the fetch "Prefetches `account.wallet` to keep the legacy-walletId routing path … from triggering one SwiftData fault per row when we resolve the parent wallet." After this PR the descriptor only prefetches `\.account`; the `account → wallet` hop now faults lazily inside the loop (lines 2485–2492). This is the same justification a future reader will reach for when considering re-adding the chained keypath, so leaving it stale invites the regression to be reintroduced. Update the comment to state that only `account` is prefetched, that the `wallet` hop intentionally faults on demand, and that the chained keypath trips a SwiftData internal assertion.
- [SUGGESTION] In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:2462-2462: Wallet-restore regression has no automated coverage
This single keypath edit is the entire behavioral fix, but no test exercises `loadWalletList()` against a populated SwiftData store. A grep confirms `loadWalletList` is referenced only in `PlatformWalletPersistenceHandler.swift`, `PlatformWalletManager.swift`, and `WalletManagerStore.swift` — never in `SwiftExampleAppTests` or `SwiftDashSDKTests`. The original crash only triggers after the `restorable.isEmpty` gate opens (second wallet import or app restart with persisted wallets) and only surfaces at runtime inside SwiftData, so the inline comment is the only signal preventing a future contributor from re-adding `\.account?.wallet` or another transitive keypath. Add an integration test that seeds a `PersistentWallet` with a restorable `PersistentAccount` and at least one unspent `PersistentTxo` (covering both the populated-`walletId` and legacy empty-`walletId` paths in lines 2483–2495), then invokes the load path, so the assertion is caught in CI rather than on a user's device.
Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
Issue being fixed or feature implemented
PlatformWalletPersistenceHandler.loadWalletList()asserts inside SwiftData internals on the first run where therestorable.isEmptygate opens — i.e. any wallet registration via the platform-wallet Rust crate after at least one wallet with populated accounts already exists on disk (app restart, second wallet import, key-migration flows, etc.). The full stack ends at the unspentPersistentTxofetch:Reproducible by creating a wallet, force-quitting, relaunching — or by importing a second wallet into an app that already has one. Symptom is a Swift runtime fatal error with
"Couldn't find \PersistentTxo.<computed (Optional<PersistentAccount>)>?.<computed (PersistentWallet)>? on PersistentTxo with fields [outpoint, vout, ...]".What was done?
Dropped the chained second hop from the prefetch hint:
SwiftData's prefetch resolver only supports direct, single-hop relationship keypaths — chained / transitive keypaths like
\.account?.wallettrip an internal assertion in the schema walker even when both ends have proper relationship metadata. Adding@Relationshipannotations toPersistentTxo.account/PersistentAccount.walletdoes not fix it (verified by trying and watching the same crash recur with the annotations present).The single-hop
\.accountprefetch is preserved, so the TXO → account bulk join still happens. Theaccount → wallethop now faults on demand inside the iteration loop — N+1 worst case, butloadWalletListruns at registration/restore time on a bounded set of TXOs and the cost is acceptable. If the chained prefetch is wanted back later it'd take a model-layer rework (e.g. denormalizing the wallet pointer onto TXO directly, or refactoring the loop to pre-fetch wallets via a[walletId: PersistentWallet]dictionary).How Has This Been Tested?
createOrImportWalletagainst an existing on-disk wallet — reproduces the crash before the fix, completes cleanly after.Breaking Changes
None. Behavior change is purely the loss of one prefetch optimization; the relationship traversal at the use site still works, just fault-loaded.
Checklist:
Summary by CodeRabbit