feat(swift-sdk): add Swift wrappers for the encrypted-txMetadata FFI exports - #4194
feat(swift-sdk): add Swift wrappers for the encrypted-txMetadata FFI exports#4194bfoss765 wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
✨ 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 5bf1065) |
|
Reviewed (two independent passes). As code, the wrappers are solid: faithful Kotlin parity (matching semantics and identical version-guard error text), verified 12/7-arg FFI signatures including the null-payload-at-len-0 contract, correct pin/free memory discipline, typed error mapping, no crypto in Swift, mirrored tests. One blocker to resolve before merge: Sequencing conflict with #4195. This PR bridges only the explicit-index export with a required Also: CI has never compiled these wrappers (the Swift job dies at the runner keychain step, and the Kotlin failure is a locked emulator — both infra) — needs a green run after the rebase onto merged #4186. |
bf8d0d7 to
9efc0b7
Compare
|
Reworked per the sequencing decision: re-stacked onto #4195 and changed Verified: the cbindgen header carries the auto-index symbol at the expected signature and |
|
Round-3 verification (two independent passes, reconciled): the rework is verified in full — the branch is exactly re-stacked on #4195's head, the Swift wrapper binds the
|
|
Covered by the same-repo stack dispatch run (29994291221) — full Rust workspace suite green at the stack head, and this PR's own CI already has Swift and Kotlin green. Nothing further from our side beyond the earlier cosmetic notes; lands after #4195. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The exact head resolves the prior sequencing concern: Swift calls the Rust auto-index export, and the reviewed ABI arguments, signer/resolver lifetime pinning, version validation, and output ownership match the underlying FFI contracts. No functional or blocking issue remains; the only in-scope defect is a stale diagnostic string that names the explicit-index export instead of the function actually called.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
💬 1 nitpick(s)
| // violation. Fail loudly rather than persist an empty body. | ||
| guard let jsonPtr = documentJsonPtr else { | ||
| throw PlatformWalletError.walletOperation( | ||
| "create_encrypted_document_with_signer returned no canonical document JSON" |
There was a problem hiding this comment.
💬 Nitpick: Null-JSON diagnostic names the wrong FFI export
This wrapper calls platform_wallet_create_encrypted_document_with_signer_auto_index at line 3531, but its null-output fallback names create_encrypted_document_with_signer, the explicit-index sibling. The mismatch does not affect control flow or memory safety, but it would direct maintainers to the wrong C symbol when diagnosing an FFI contract violation.
| "create_encrypted_document_with_signer returned no canonical document JSON" | |
| "create_encrypted_document_with_signer_auto_index returned no canonical document JSON" |
source: ['claude', 'codex']
There was a problem hiding this comment.
Fixed in 5bf106573d. The null-JSON diagnostic now names the actual export it calls, create_encrypted_document_with_signer_auto_index, instead of the non-auto-index sibling. (The doc reference at line ~3451 to create_encrypted_document_with_signer is correct — that export still exists separately.)
There was a problem hiding this comment.
Resolved in this update — Null-JSON diagnostic names the wrong FFI export no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Hi @bfoss765 — heads up before anything changes on my side, and thanks for this work. I'm going to land this feature as four same-repository branches under Two reasons:
What that means concretely:
If you'd rather take a different route, say so and I'll stop and we'll work it out first. |
1d812c7 to
560f66a
Compare
Swift wrappers for the two encrypted-document C-ABI exports that landed with the encrypted txMetadata feature (dashpay#4277): - platform_wallet_create_encrypted_document_with_signer_auto_index - platform_wallet_fetch_encrypted_documents dashpay#4277 shipped the Rust core, the C ABI, the JNI bridge and the Kotlin host, but ZERO Swift. This adds the iOS half so both hosts sit on the same surface. Adds ManagedPlatformWallet.createEncryptedDocument(...) and .fetchEncryptedDocuments(...) to Sources/SwiftDashSDK/PlatformWallet, mirroring the existing createDocument (signer + byte-buffer marshalling, withExtendedLifetime pinning, result-code .check(), string_free) and previewIdentityRegistrationKeys (internal MnemonicResolver construction + pinning) patterns. The plaintext payload is borrowed in place and handed straight to Rust's Zeroizing buffer with no extra Swift-side copy, as the neighboring seed path does. createEncryptedDocument binds the AUTO-INDEX export and takes NO encryptionKeyIndex parameter: Rust generates the per-document index (dashpay#4277 draws a non-zero 31-bit BIP-32 child index from the OS CSPRNG), so hosts never assign it. This matches the Android auto-index path, where Kotlin's createEncryptedDocument defaults encryptionKeyIndex to null. The explicit-index export remains in the C ABI for migration/compat tests and is deliberately not surfaced in Swift. The version-byte {0,1} guard and argument-order/nullability parity with the Kotlin counterpart are preserved; Kotlin's runtime `require`s are enforced structurally by stronger Swift types (Identifier, UInt32, UInt64). EncryptedDocumentVersionValidationTests mirrors the Kotlin DocumentTransactionsVersionValidationTest: the wire-meaningless version bytes (2/3/127/255) are rejected before any FFI dispatch. The negative-version case is non-compilable in Swift by construction, and the happy path is network-gated. No new error codes: the wrappers surface the existing PlatformWalletError cases only, so the ERROR_CODE_REGISTRY is untouched. Verified: cbindgen on platform-wallet-ffi at this base emits both symbols at exactly the bound signatures (auto_index takes no encryption_key_index); `swiftc -parse -swift-version 6` is clean over both touched files and over the whole SwiftDashSDK source set. A full `swift build`/`swift test` needs the DashSDKFFI.xcframework binary target, which is gitignored and not present in a source checkout — it requires a full iOS framework build (build_ios.sh) plus the dash-network / key-wallet-ffi headers that live outside this repo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
560f66a to
d2cc4ac
Compare
The auto-index wrapper calls platform_wallet_create_encrypted_document_with_signer_auto_index but its null-output error message named create_encrypted_document_with_signer (the non-auto-index sibling), misdirecting anyone diagnosing an ABI contract violation (dashpay#4194). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The one-line delta correctly updates the null-JSON diagnostic to identify the auto-index FFI export actually invoked, resolving the carried-forward finding without changing ABI behavior, ownership, or execution. The affected Swift source and existing test parse successfully under Swift 6, and the combined Codex and Sonnet reviewer evidence identifies no remaining in-scope defect.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed)
Swift wrappers for the encrypted-txMetadata FFI surface.
Rebased onto v4.2-dev — no longer stacked on #4186/#4195
What changed vs. the pre-#4277 version of this PR:
feat(platform-wallet): add encrypted txMetadata document support, merge6704a41a85) re-landed the whole encrypted-txMetadata feature as a superset. This branch is now a single commit directly onv4.2-dev, with no borrowed commits.v4.2-devhead. There is nothing left for this PR to stack on.What this adds
#4277 shipped the Rust core, the C ABI, the JNI bridge and the Kotlin host — but zero Swift. This is the iOS half, so both hosts sit on the same surface.
ManagedPlatformWallet.createEncryptedDocument(...)→platform_wallet_create_encrypted_document_with_signer_auto_indexManagedPlatformWallet.fetchEncryptedDocuments(...)→platform_wallet_fetch_encrypted_documentsBoth follow the existing wrapper patterns exactly: nested
withExtendedLifetimepinning for signer/resolver across the full FFI call, byte buffers viawithUnsafeBufferPointer, payload borrowed in place with no added Swift-side copy (the Rust-ownedZeroizingbuffer does the scrubbing; callers should scrub their own), and the returned JSON C-string freed exactly once viadeferregistered after the result check.createEncryptedDocumenttakes noencryptionKeyIndexparameter — it binds the auto-index export, matching Kotlin'screateEncryptedDocument, whoseencryptionKeyIndexdefaults tonull. The explicit-index export stays in the C ABI for migration/compat tests and is deliberately not surfaced in Swift.Argument order, types, and the pre-dispatch
version ∈ {0, 1}guard match the KotlinDocumentTransactionscounterparts; Kotlin's runtimerequires are enforced structurally by stronger Swift types (Identifier,UInt32,UInt64).EncryptedDocumentVersionValidationTestsmirrors the KotlinDocumentTransactionsVersionValidationTest(2/3/127/255 rejected before any FFI dispatch); the negative-version case is non-compilable in Swift by construction. The happy path is network-gated, noted in-test.Doc updates forced by #4277's index redesign
The wrapper's doc comments were rewritten to describe the contract that actually shipped. #4195 proposed allocating
encryptionKeyIndexas1 + <count of the identity's txMetadata documents on Platform>behind a per-wallet allocator mutex. #4277 chose a different mechanism: Rust draws a non-zero 31-bit BIP-32 child index from the OS CSPRNG (generate_encryption_key_index), with no Platform count, no allocator state, and no extra network round trip. The rationale — now reflected in these comments — is thatencryptionKeyIndexis a per-document derivation input stored on the document, not a protocol sequence number: readers derive each document's key from that document's own{keyIndex, encryptionKeyIndex}, every document carries a fresh IV, and thetxMetadataschema has no unique index on the field, so a repeated index is non-lossy.Stale
dashpay/platform#4195/ "allocates from authoritative Platform state" references were replaced with#4277/ CSPRNG wording. No call-site changes were needed: the auto-index export's C signature atv4.2-devhead is byte-for-byte what this wrapper already bound.Error codes: none added. The wrappers surface existing
PlatformWalletErrorcases only, so the error-code registry (#4261) is untouched.Verification
cbindgenonplatform-wallet-ffiat this base emits both symbols at exactly the bound signatures —..._auto_indextakes noencryption_key_index, and..._fetch_encrypted_documentsis unchanged. Argument-by-argument parity re-audited against the freshly generated header.swiftc -parse -swift-version 6clean over both touched files and over the wholeSwiftDashSDKsource set.swift build/swift testis not reproducible from a source checkout:Package.swiftdepends on theDashSDKFFI.xcframeworkbinary target, which is gitignored and must be produced bybuild_ios.sh; that build additionally needs thedash-networkandkey-wallet-fficbindgen headers, whose crates live outside this repo. The pre-feat(platform-wallet): add encrypted txMetadata document support #4277 revision of this PR was verified withswift build --build-tests+swift test --filter EncryptedDocumentVersionValidationTestsagainst a locally rebuilt framework slice, and the Swift call sites are unchanged since — only comments moved.🤖 Generated with Claude Code