feat(swift-sdk): split build/broadcast with reservation release for BIP70-style deferred submission - #4322
Conversation
…IP70-style deferred submission Swift counterpart of the Kotlin-only deferred send flow from #4308. Bridge-only wrappers over the existing core_wallet_signed_payment_finalize / _broadcast / _release FFI exports: - SignedCoreTransaction owns the reservation token: close() releases exactly once, deinit is the backstop, so an abandoned payment never strands its funding reservation until key-wallet's TTL. - CoreTransactionBuilder.finalizeSignedPayment: one atomic native select + reserve + sign + register, with an owner-guarded defensive release on post-success marshalling guards. - ManagedCoreWallet.broadcastSignedPayment and the ManagedPlatformWallet surface (buildSignedPayment / broadcastSigned / releaseReservation, token and object-owning forms) mirroring the Kotlin reference. - Parity manifest capability core.deferred_signed_payment with Swift, Kotlin, and shared registry test references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 48 seconds 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Comment |
|
🔍 Review in progress — actively reviewing now (commit e5e4c46) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Opus
The Swift bridge matches the native deferred-payment ownership and marshalling lifecycle, and no blocking defect was confirmed. The remaining in-scope suggestion is to exercise the finalize and broadcast C ABI from Swift, because the added tests currently stop above and below that host boundary.
Source: reviewer evidence — codex/gpt-5.6-sol and claude/claude-sonnet-5; final verifier — codex/gpt-5.6-sol (fallback for failed Sonnet verification). Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (Opus verifier fallback) - Opus reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— ffi-engineer (failed),claude-sonnet-5— ffi-engineer (completed)
🟡 1 suggestion(s)
🤖 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/CoreWallet/CoreTransactionBuilder.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/CoreWallet/CoreTransactionBuilder.swift:404-415: Exercise the finalize and broadcast FFI paths from Swift
The new Swift tests instantiate `SignedCoreTransaction` directly and only reach the native release symbol with an unknown token. No test invokes `buildSignedPayment`/`finalizeSignedPayment` or `broadcastSigned`, so regressions in out-parameter ordering, copying the borrowed transaction bytes before `core_wallet_transaction_free`, txid ownership, error mapping, and the post-success token-reconciliation guards would not be detected at the C/Swift boundary. The shared Rust registry tests run below this boundary, while the ownership tests run above it. Extend the existing funded local-devnet Swift integration suite with build→broadcast and build→release paths, asserting the returned txid, raw bytes, fee, and token-consumption/release behavior.
… the integration suite Review follow-up: the hermetic SignedCoreTransactionTests run above the C boundary and the Rust signed_payment_registry tests below it, so out-parameter marshalling, txid ownership, the borrowed-bytes copy, error-code mapping, and reservation consume/release went unexercised at the host boundary. DeferredSignedPaymentIntegrationTests (funded local-devnet suite, gated by RUN_INTEGRATION_TESTS=1 like its siblings) adds: - build -> broadcast: marshalled fields asserted per out param, broadcast txid must equal the finalize-computed txidHex, recipient credited after mining, double-broadcast throws the typed .reservationTokenConsumed arm, defensive release after consumption is a no-op. - build -> release: a second build against the fully reserved wallet fails, release frees the inputs so the rebuild succeeds with a fresh token, and broadcasting the released token throws .reservationTokenConsumed. Both registered as integration-kind verification for core.deferred_signed_payment in the parity manifest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
#4308 added the deferred BIP70/BIP270 send flow — finalize and sign now (reserving the funding UTXOs), hand the raw bytes to a merchant server, then broadcast on ack or release the reservation on a nack/abandonment — but exposed it Kotlin-only. The C exports (
core_wallet_signed_payment_finalize/_broadcast/_release) and the Swift error arms (codes 34–36) were already in place; this PR adds the missing Swift bridge surface.What was done?
Bridge-only per
packages/swift-sdk/CLAUDE.md— marshal in, call FFI, marshal out; every lifecycle decision stays in Rust:SignedCoreTransaction(CoreTransactionBuilder.swift): the ownership object modeled on Kotlin'sManagedPlatformWallet.SignedCoreTransaction. CarriestxidHex/rawTxBytes/feeDuffs/reservationToken; OWNS the token and releases it exactly once via explicitclose()or adeinitbackstop, so an abandoned payment never strands its funding reservation until key-wallet's TTL. The native release is idempotent, so a close after broadcast/release is a harmless no-op. The release action is injectable (internal seam) so the unit test can count releases without registry introspection — the ARC-deterministic analog of Kotlin's countableNativeCleanerrunnable.CoreTransactionBuilder.finalizeSignedPayment: mirrorsfinalizeAtomic's shape (consumed guard, per-callMnemonicResolver,withExtendedLifetime); one atomic native select + reserve + sign + register. Frees the returned txid string and transaction on every path, copies the borrowed byte view before the free, and defensively releases a minted token if a post-success marshalling guard trips (mirroring the owner-guarded release Kotlin does, per feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4185).ManagedCoreWallet.broadcastSignedPayment(token:)(internal) and the publicManagedPlatformWalletsurface:buildSignedPayment,broadcastSigned(token:)/broadcastSigned(_:),releaseReservation(token:)/releaseReservation(_:)— token and object-owning forms matching Kotlin, with the object forms holding the payment alive across the native call (withExtendedLifetime, the Swift analog of Kotlin'sreachabilityFence) and disarming the backstop once the token is consumed.core.deferred_signed_paymentcapability (Swift + Kotlinsdk: supported), declaring the three shared FFI symbols and referencing the new Swift unit tests, feat(kotlin-sdk): split build/broadcast with reservation release for BIP70-style deferred submission #4308's KotlinSignedCoreTransactionTestcases, and three sharedsigned_payment_registry.rslifecycle tests. Summary regenerated with--write-summary.How Has This Been Tested?
SignedCoreTransactionTests(5 tests, all passing viaswift test --filter SignedCoreTransactionTestsagainst the rebuilt mac slice): ownership round-trip, close-exactly-once, deinit backstop, close-then-deinit no double release, and a real-seam smoke test that linkscore_wallet_signed_payment_releaseand asserts releasing an unknown token is a silent no-op.swift build --build-tests+swift testpass in Swift 6 language mode against a freshly rebuiltDashSDKFFI.xcframework(regenerated cbindgen headers include the three new exports).python3 scripts/check_sdk_parity_manifest.py --write-summarypasses (24 capabilities).Breaking Changes
None — additive surface only; the immediate send path is untouched.
Checklist:
🤖 Generated with Claude Code