feat(swift-sdk): add production create-document flow via platform-wallet FFI#3908
Merged
Merged
Conversation
…let FFI Add a real, schema-driven "Create Document" flow to SwiftExampleApp that broadcasts a DocumentCreate through rs-platform-wallet using the wallet's keychain-backed signer, replacing the local-only mock (DOC-09) and complementing the test-signer builder path. - rs-platform-wallet: IdentityWallet::create_document_with_signer fetches the on-chain contract, builds a rev-1 Document, selects an AUTHENTICATION+ECDSA key satisfying the doc type's security_level_requirement(), and broadcasts via the SDK's PutDocument::put_to_platform_and_wait_for_response on the 8 MB worker stack. - rs-platform-wallet-ffi: platform_wallet_create_document_with_signer C ABI. - swift-sdk: ManagedPlatformWallet.createDocument wrapper + schema-driven CreateDocumentView (owner-identity picker + DocumentFieldsView), reachable from Contracts -> contract -> document type -> New Document. Confirmed document persisted to SwiftData so it appears in the documents list. Also fixes a navigation-stability bug that made the create flow (and other Contracts drill-downs) unusable: background-sync SwiftData writes invalidated the contracts @query list whose ForEach rows owned the details sheet, tearing down the contract-details navigation ~every second (compounded by deprecated NavigationView/NavigationLink(destination:) and auto-refreshing relative-time Text). Hoisted the details sheet to a stable container, migrated to value-based navigationDestination, made relative-time strings static, and decoupled the root TabView from SPV progress ticks. Verified end-to-end on the iPhone 17 simulator (testnet): created a preorder document on the DPNS contract GWRSAV...S31Ec from a funded identity; network-confirmed, doc id 7i1hJgvVt8fJms26kGwkEZ6jVZxrfd3BrqfmAfpqXMoG, ZPERSISTENTDOCUMENT 0->1. build_ios.sh --target sim succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds end-to-end document creation: a new ChangesDocument creation end-to-end + UI stability
Sequence Diagram(s)sequenceDiagram
participant View as CreateDocumentView
participant Wallet as ManagedPlatformWallet
participant SDK as Swift SDK
participant RustFFI as Rust FFI
participant RustCore as IdentityWallet
View->>View: propertiesJSON(from: fields)
View->>Wallet: createDocument(ownerIdentityId, contractId, documentType, propertiesJSON, KeychainSigner)
Wallet->>Wallet: marshal to FFI bytes and C strings
Wallet->>RustFFI: platform_wallet_create_document_with_signer
RustFFI->>RustCore: create_document_with_signer(owner_id, contract_id, type, json, VTableSigner)
RustCore->>RustCore: fetch DataContract, parse JSON, build Document
RustCore->>RustCore: select AUTHENTICATION ECDSA_SECP256K1 key via allowed_signing_security_levels
RustCore->>RustCore: put_to_platform_and_wait_for_response
RustCore-->>RustFFI: confirmed Document id + canonical JSON
RustFFI-->>Wallet: PlatformWalletFFIResult + id bytes + JSON C string
Wallet-->>View: (Identifier, String)
View->>View: persistConfirmedDocument, update UI state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
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 |
Collaborator
|
✅ Review complete (commit a469074) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 2606-2635: The `_ = signer` discard at the beginning of the
Task.detached closure is an unreliable pattern for keeping signer alive across
the FFI call to platform_wallet_create_document_with_signer, especially in async
detached tasks where the optimizer may elide it in -O builds, causing a
use-after-free. Replace the `_ = signer` pattern by wrapping the entire nested
withUnsafeBufferPointer and withCString marshalling chain in
`withExtendedLifetime(signer) { ... }`. This matches the established pattern
used elsewhere in the file like in registerIdentityWithFundingSigner and
guarantees signer remains alive through the complete FFI call and return.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift`:
- Around line 77-82: The boolean Toggle control in the "boolean" case lacks an
accessibility label for assistive technologies like VoiceOver. Although the
property name is visible in the parent view and an accessibility identifier is
present, the Toggle itself has a hidden empty label that prevents screen readers
from understanding its purpose. Add an explicit accessibilityLabel modifier to
the Toggle that provides the property name as the label, ensuring assistive
technologies can identify what the toggle controls.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift`:
- Around line 476-484: The persistConfirmedDocument function is swallowing
errors from modelContext.save() which allows the success message "Document
created" to be reported even when local persistence fails. Modify the
persistConfirmedDocument function to properly handle errors from the
modelContext.save() call by catching any exceptions thrown and ensuring the
function fails gracefully without reporting success when persistence fails, so
the document list reflects the actual state of persistence.
- Around line 446-449: For object fields, the fieldValues are stored as JSON
strings from DocumentFieldsView, but are being passed directly to the
propertiesJSON method where they get double-encoded as nested JSON strings
instead of proper objects. Modify the code around the propertiesJSON call to
decode the JSON string values back to dictionaries before encoding, so that
object fields are properly nested in the final properties JSON. This same issue
also exists in the related field encoding logic, so apply the same fix there as
well to ensure consistent handling of object field types.
- Around line 328-332: When the selected schema (docType parameter) changes in
the schemaSection method, the parent fieldValues state needs to be cleared to
prevent submitting values from the previous schema with the newly selected
document type. Add an onChange handler that monitors the docType parameter and
resets the fieldValues dictionary to an empty state whenever the docType
changes. This ensures a clean slate when switching between different schemas in
the no-preset flow.
- Around line 262-267: The selectionSection view displays all persisted
contracts in the Picker without filtering by the active network, which allows
users to pair a current-network owner with a contract from another network.
Filter the contracts array by appState.currentNetwork in the ForEach loop that
iterates over contracts (similar to how owners are scoped to
appState.currentNetwork at line 407), so only contracts matching the currently
active network are available for selection.
🪄 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: 17088e4d-e3e8-4693-a7e2-4dac132b0c71
📒 Files selected for processing (13)
packages/rs-platform-wallet-ffi/src/document.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet/src/wallet/identity/network/document.rspackages/rs-platform-wallet/src/wallet/identity/network/mod.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/ContentView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DataContractDetailsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentTypeDetailsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/LocalDataContractsView.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Production create-document flow is structurally sound: Rust create_document_with_signer reproduces the consensus security-level rule, the FFI bindings mirror the established platform-wallet signer pattern with correct lifetime/keepalive handling, and the Swift wrapper is appropriately thin. Three in-scope improvements remain in the SwiftExampleApp form/persistence layer; nothing blocks merge.
🟡 3 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/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift:100-108: Object-typed properties cross the boundary as JSON strings, not objects
For schema `type: "object"` properties, the form renders a `TextEditor` bound to `textFields` (String), and `updateFieldValues` stores the edited text as a String in `fieldValues`. `DocumentsView.propertiesJSON(from:)` then emits `"field":"{\"foo\":1}"` instead of `"field":{"foo":1}`. On the Rust side, `serde_json::from_value::<BTreeMap<String, Value>>` parses it as `Value::Text`, and `sanitize_document_properties` only dispatches to `property_type.sanitize_value_mut`, which won't reshape a string into a map. The resulting document fails schema validation for any document type with an object-valued property. Parse object-field text into a JSON object before placing it into `fieldValues` so the existing `JSONSerialization` step emits a nested object.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift:329-339: Switching document types leaves stale field values in the payload
`DocumentFieldsView` holds its field dictionaries in `@State` and only repopulates them in `.onAppear` via `initializeFields()`. When the user changes `selectedDocumentTypeName`, SwiftUI reuses the same `DocumentFieldsView` instance (same position in the tree), so values typed for the previous type stay in `textFields`/`numberFields`/etc. and flow into `fieldValues`. The next submit can broadcast leftover fields, triggering `additionalProperties` validation failures or sending wrong values. Key the child view by the document type id so SwiftUI tears it down and rebuilds fresh state whenever the schema changes.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift:510-525: PersistentDocument stores the pre-broadcast form JSON, not the canonical confirmed document
`persistConfirmedDocument` writes the form-built `propertiesJSON` (hex-encoded byte arrays, base58-encoded identifiers) as `PersistentDocument.data`. The confirmed `Document` returned by `put_to_platform_and_wait_for_response` is discarded at the FFI boundary — `platform_wallet_create_document_with_signer` only returns the 32-byte id. For any document type with byteArray or identifier properties (e.g. DPNS `preorder.saltedDomainHash`), the persisted SwiftData row's body structurally diverges from what Platform actually stores, which will mislead later reads and re-sync logic. Either extend the FFI to return the canonical serialized document, or re-fetch the document by id before persisting.
- ManagedPlatformWallet.createDocument: pin the signer with withExtendedLifetime(signer) across the FFI call instead of the unreliable `_ = signer` discard (optimizer can elide it in -O → use-after-free), matching the other *_with_signer wrappers. - DocumentFieldsView: add accessibilityLabel(property.name) to the boolean Toggle (labelsHidden() hid it from VoiceOver). - CreateDocumentView: scope the contract picker to the active network (activeContracts) so an owner can't be paired with an off-network contract; reset field state when the selected document type changes (.id + onChange) so the no-preset flow can't submit stale values. - propertiesJSON: schema-aware — decode object-typed fields from their editor JSON string into nested objects instead of double-encoding them as JSON strings. - persistConfirmedDocument: propagate modelContext.save() errors instead of swallowing with try?; on local-save failure still report the confirmed broadcast (doc is on-chain) but surface a warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
Note on the failing
|
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Cumulative re-review of head 92a5ce4. Two of three prior findings are FIXED (object-typed properties now parse as nested JSON via the schema-aware propertiesJSON; document-type switches reset field state via .id(docType.id) + onChange). Prior #3 (local cache stores form JSON, not canonical confirmed document) is STILL VALID and carried forward — multiple agents independently confirmed the FFI returns only the 32-byte id and the Swift side persists the pre-broadcast form JSON. New latest-delta findings: optional booleans are always serialized as false (pre-existing in DocumentFieldsView, but now load-bearing because this PR turns this path into the production broadcast flow), plus two low-severity UX/state-hygiene nitpicks. Codex's blocking claim that the preview container can broadcast and persist to a throwaway store is not reachable — .modelContainer(preview.container) rebinds the @Query scope so ownerIdentities is empty and canSubmit returns false; dropped as a false positive. No in-scope blockers.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift:158-238: Optional boolean fields are always serialized as `false` in the broadcast payload
`initializeFields()` seeds every boolean property with `false` (DocumentFieldsView.swift:168), and `updateFieldValues()` unconditionally copies every entry from `boolFields` into the payload (DocumentFieldsView.swift:236-238). String/number/array/object branches all skip empty values, so an untouched optional field of those types is omitted. Booleans have no equivalent skip, so an optional boolean the user never toggled is broadcast as `false` rather than absent. For schemas where absence and `false` are distinct (audit, default-driven, or storage-cost-sensitive document types), this changes the actual document content and storage cost compared to what the user supplied. Pre-PR this code path fed only the Settings test-signer flow, so the divergence was harmless; this PR makes it the production broadcast path. Track which boolean fields the user actually interacted with (e.g. drop the seed and key on user-touched only), or omit non-required booleans whose state hasn't changed from the schema default.
- DocumentFieldsView: don't broadcast untouched optional booleans as `false`. The seed gives booleans no "empty" state, so they were always serialized; now only required booleans or ones the user actually toggled (tracked via `touchedBoolFields`) are included, keeping untouched optional booleans absent (absence ≠ false for some schemas). - DocumentsView.persistConfirmedDocument: on a failed `modelContext.save()`, delete the inserted row before rethrowing so a later save elsewhere can't silently flush it — matching the "not saved locally" warning. - DocumentsView selection: clear `selectedDocumentTypeName` when the contract changes so the document-type picker can't show a stale selection (or silently resolve a same-named type on another contract). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Cumulative re-review of ae6ec62. Latest delta is small and targeted: it resolves three of the four prior findings (prior-2 boolean payload gating via touchedBoolFields, prior-3 modelContext.delete on save failure, prior-4 onChange clearing the doc-type picker). Carried forward: prior-1 — the FFI still discards the canonical confirmed Document and surfaces only its 32-byte id, so the SwiftData cache row stores pre-broadcast form JSON rather than Platform's canonical document. No new findings in the latest delta. CodeRabbit returned a rate-limited summary with zero inline findings, so no CR reactions are produced.
🟡 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/rs-platform-wallet-ffi/src/document.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/document.rs:70-90: [Carried forward — STILL VALID] FFI discards canonical confirmed Document; SwiftData cache stores pre-broadcast form JSON instead of Platform's canonical document
Re-validated at head ae6ec628. The Rust side of the boundary still throws away the confirmed document and only returns its 32-byte id: `create_document_with_signer` returns the confirmed `Document` from `put_to_platform_and_wait_for_response`, but the FFI collapses it to `.map(|document| document.id())` (packages/rs-platform-wallet-ffi/src/document.rs:81) and writes only 32 bytes through `out_document_id` (lines 87-89). Swift therefore has no canonical bytes to persist and writes the user's form-built `propertiesJSON` (hex-encoded byte arrays, base58-encoded identifiers, raw object subtrees) directly into `PersistentDocument.data` with a hardcoded revision of 1 (packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift:546-555). Any Rust/DPP-side sanitization, default-filling, byte-level normalization, revision data, or timestamps Platform actually accepted are invisible to Swift — the SwiftData row reads back the user's submitted strings rather than the canonical document. The newly-added `persistWarning` only covers SwiftData save failures, not representation divergence after a successful save. This is owned by this PR because it is the moment the persistence path went from a Settings test-signer flow to the production broadcast path. Not a security/consensus issue (broadcast bytes are produced inside Rust from the sanitized value map), but a documented local-cache fidelity bug. Fix by either (a) extending the FFI to return canonical document bytes (e.g. a serialized `Document`) or the sanitized properties map, or (b) adding a sibling FFI that refetches by id and returns canonical bytes so the cache row reflects what Platform stored.
platform_wallet_create_document_with_signer now also returns the
confirmed Document serialized to canonical JSON (DPP
to_json_with_identifiers_using_bytes — $id/$ownerId as base58 +
normalized properties), via a new out_document_json C string freed with
platform_wallet_string_free. The Swift wrapper returns (Identifier,
String) and persists the canonical JSON into PersistentDocument.data
instead of the user's form-built propertiesJSON (which had hex byteArrays
/ base58 identifiers and no system fields). The local cache row now
reflects what Platform stored.
Verified on testnet: created a preorder doc on GWRSAV…S31Ec from idx1;
persisted body = {"$id":"AFLkAA…mnZ2","$ownerId":"BjJz…Dx91",
"saltedDomainHash":[66,31,249,…]} — canonical id/owner + normalized
byteArray, matching a DOC-01 query rather than the form input.
Addresses the carried-forward review finding (local-cache fidelity).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift (1)
306-311:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh
fieldValuesafter sanitizing byte-array input.The binding setter calls
updateFieldValues()before this sanitizer runs. When.onChangethen rewritestextFields[property.name]directly, the visible field is cleaned but the submittedfieldValuescan still contain the pre-sanitized value until another edit occurs.Proposed sync fix
.onChange(of: currentValue) { _, newValue in // Remove any non-hex characters and convert to lowercase let cleaned = newValue.lowercased().filter { "0123456789abcdef".contains($0) } if cleaned != newValue { textFields[property.name] = cleaned + updateFieldValues() } }🤖 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/Views/DocumentFieldsView.swift` around lines 306 - 311, The `.onChange` handler sanitizes the byte-array input by rewriting `textFields[property.name]` with the cleaned value, but this change is not reflected back into the `fieldValues` dictionary that gets submitted. After the assignment of the cleaned value to `textFields[property.name]`, call `updateFieldValues()` again to ensure the sanitized value is synchronized into `fieldValues` so the submitted data contains only valid hex characters.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift (1)
241-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable swipe-to-dismiss while the broadcast is in flight.
The Cancel button is disabled, but the sheet can still be interactively dismissed during the non-idempotent
DocumentCreatebroadcast. If the task later succeeds or only local persistence fails, the user loses the result/warning and may retry a document that already exists on-chain.Proposed dismissal guard
.toolbar { ToolbarItem(placement: .navigationBarLeading) { Button("Cancel") { dismiss() } .disabled(isSubmitting) } } + .interactiveDismissDisabled(isSubmitting) .alert(item: $submitError) { err in🤖 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/Views/DocumentsView.swift` around lines 241 - 247, While the Cancel button in the toolbar is disabled when isSubmitting is true, the sheet can still be dismissed via swipe gesture during the non-idempotent DocumentCreate broadcast, which risks losing the result. Add the .interactiveDismissalDisabled(isSubmitting) modifier to the sheet to prevent interactive swipe-to-dismiss gestures while the broadcast is in flight, ensuring the user cannot accidentally dismiss the view until the submission is complete.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift (1)
2594-2598: ⚡ Quick winUpdate the signer lifetime contract docs to match the implementation.
This method no longer uses
_ = signer; the safe pattern here iswithExtendedLifetime(signer)around the full FFI call. Keeping the old wording makes future signer wrappers more likely to copy the unsafe keepalive pattern.Proposed doc update
- /// Lifetime contract: the `signer` instance MUST stay alive for - /// the duration of the `await` (Rust holds a `passUnretained` - /// ctx pointer to the underlying `KeychainSigner`). A - /// `_ = signer` keepalive at the call site is the canonical way - /// to pin it. + /// Lifetime contract: the `signer` instance MUST stay alive for + /// the duration of the synchronous FFI call inside this async + /// wrapper (Rust holds a `passUnretained` ctx pointer to the + /// underlying `KeychainSigner`). The wrapper pins it with + /// `withExtendedLifetime(signer)` around the full marshalling chain.🤖 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/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift` around lines 2594 - 2598, The lifetime contract documentation comment for the signer parameter describes the keepalive pattern as `_ = signer`, but the actual implementation uses `withExtendedLifetime(signer)` for safe lifetime extension. Update the documentation comment to correctly describe the actual safe pattern being used (withExtendedLifetime) instead of the old keepalive approach, and remove the reference to `_ = signer` to prevent future implementations from copying the unsafe pattern.
🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 2656-2663: The code currently silently falls back to an empty
string when documentJsonPtr is null, but on a successful broadcast (after
result.check()), Rust guarantees to return valid canonical JSON. A null pointer
at this point indicates an FFI/ABI contract violation that should fail loudly
rather than be hidden. Replace the fallback logic in the canonicalJSON
assignment (currently using the ?? operator to default to "") with a guard
statement that explicitly handles the null case as an error, ensuring callers
cannot persist an empty document body as if it were canonical and contract
violations are properly detected.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift`:
- Around line 306-311: The `.onChange` handler sanitizes the byte-array input by
rewriting `textFields[property.name]` with the cleaned value, but this change is
not reflected back into the `fieldValues` dictionary that gets submitted. After
the assignment of the cleaned value to `textFields[property.name]`, call
`updateFieldValues()` again to ensure the sanitized value is synchronized into
`fieldValues` so the submitted data contains only valid hex characters.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift`:
- Around line 241-247: While the Cancel button in the toolbar is disabled when
isSubmitting is true, the sheet can still be dismissed via swipe gesture during
the non-idempotent DocumentCreate broadcast, which risks losing the result. Add
the .interactiveDismissalDisabled(isSubmitting) modifier to the sheet to prevent
interactive swipe-to-dismiss gestures while the broadcast is in flight, ensuring
the user cannot accidentally dismiss the view until the submission is complete.
---
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift`:
- Around line 2594-2598: The lifetime contract documentation comment for the
signer parameter describes the keepalive pattern as `_ = signer`, but the actual
implementation uses `withExtendedLifetime(signer)` for safe lifetime extension.
Update the documentation comment to correctly describe the actual safe pattern
being used (withExtendedLifetime) instead of the old keepalive approach, and
remove the reference to `_ = signer` to prevent future implementations from
copying the unsafe pattern.
🪄 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: c72fa307-6676-4ca2-b209-dbef66a4d70f
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/document.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift
- ManagedPlatformWallet.createDocument: a null canonical-JSON pointer after a successful broadcast is now thrown as a walletOperation error (FFI/ABI contract violation) instead of silently persisting an empty document body. Updated the signer lifetime-contract doc comment to describe withExtendedLifetime rather than the unsafe `_ = signer`. - DocumentFieldsView: re-sync fieldValues after the byte-array hex sanitizer rewrites the field (direct @State mutation bypasses the binding setter), so the submitted value reflects the cleaned hex. - CreateDocumentView: interactiveDismissDisabled(isSubmitting) so the sheet can't be swipe-dismissed during the non-idempotent broadcast. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Addressed the remaining CodeRabbit items from the last review in ac0a4f2 (the inline null-JSON one is replied/resolved on its thread; the rest were outside-diff / nitpick with no inline thread):
|
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Cumulative verification at head a469074. Prior finding prior-ae6ec628-1 is FIXED: the FFI now returns the canonical confirmed document JSON via an out-param and Swift persists that verbatim into PersistentDocument.data (verified in document.rs:96-145, ManagedPlatformWallet.swift:2599-2665, and DocumentsView.swift propertiesJSON / persistConfirmedDocument). No new blocking issues in the latest delta. Three new low-severity findings on the latest diff: a near-impossible CString::new failure that would silently drop a confirmed broadcast (nitpick), a defensive empty-string fallback in Swift that masks contract drift (nitpick), and a UI-layer limitation where non-byte arrays are always broadcast as string arrays regardless of schema item type (suggestion).
🟡 1 suggestion(s) | 💬 2 nitpick(s)
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/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentFieldsView.swift:266-272: Non-byte array fields are broadcast as `[String]` regardless of schema item type
The new production create flow builds `propertiesJSON` from whatever `DocumentFieldsView.fieldValues` emits, and for `array` fields that are not detected as byte arrays the value is always a comma-split `[String]` (line 269). `CreateDocumentView.propertiesJSON` then forwards those strings unchanged into the JSON payload. For any document type whose `items` schema is `integer`, `number`, `boolean`, `object`, or an identifier shape not classified as a byte array, the broadcast carries quoted strings where DPP expects typed elements; DPP's sanitize step does not coerce `"1"` into an integer array element, so otherwise-valid document types become uncreatable through this new production path. Parse array entries per the schema's `items.type` (or fall back to a raw JSON array editor for non-byte arrays so the submitted JSON preserves item types).
Comment on lines
+130
to
+145
| let result = unwrap_option_or_return!(option); | ||
| let (document_id, document_json) = unwrap_result_or_return!(result); | ||
|
|
||
| // Allocate the owned C string for the JSON body. A NUL byte inside | ||
| // the JSON would be a serializer bug, but guard against it rather | ||
| // than panicking across the FFI boundary. | ||
| let json_cstring = unwrap_result_or_return!(CString::new(document_json)); | ||
|
|
||
| let bytes = document_id.to_buffer(); | ||
| let dst = slice::from_raw_parts_mut(out_document_id, 32); | ||
| dst.copy_from_slice(&bytes); | ||
| // Transfer ownership of the JSON to the caller (freed via | ||
| // `platform_wallet_string_free`). Written last so the id out-param | ||
| // and the JSON are populated together on the success path. | ||
| *out_document_json = json_cstring.into_raw(); | ||
| PlatformWalletFFIResult::ok() |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Successful broadcast is reported as an error if CString::new sees an interior NUL; document id is never written
By the time CString::new(document_json) (line 136) runs, the state transition has already been broadcast and the confirmed document returned from Platform. If CString::new were to fail (only possible if the serialized JSON contains an interior NUL byte — a serde_json::to_string output bug, effectively never), unwrap_result_or_return! exits the FFI with an error and the 32-byte id copy on lines 138-140 never runs, so Swift sees a failed result for an on-chain success and has no id to recover from. Reorder so the id is written before the JSON allocation (or write the id even on JSON marshalling failure with a distinct error code), keeping the success/error signal honest in that degenerate case. Not blocking.
source: ['claude']
Comment on lines
+2658
to
+2663
| defer { if let p = documentJsonPtr { platform_wallet_string_free(p) } } | ||
| // Defensive: on a successful broadcast the Rust side always | ||
| // writes the canonical JSON, but fall back to an empty | ||
| // string rather than crashing if it is unexpectedly null. | ||
| let canonicalJSON = documentJsonPtr.map { String(cString: $0) } ?? "" | ||
| return (Data(documentIdBytes), canonicalJSON) |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Silent empty-string fallback masks a future regression in the Rust JSON contract
After try result.check() confirms success, the wrapper returns documentJsonPtr.map { String(cString: $0) } ?? "". The documented Rust post-condition is that *out_document_json is non-null on success, so substituting an empty string when it is unexpectedly null hides any future drift in that contract — persistConfirmedDocument would then persist an empty dataBlob into SwiftData without surfacing anything via the existing persistWarning channel. Surface the null case explicitly (precondition, throw, or warning) so a contract violation is caught instead of silently degrading local-cache fidelity. Not blocking; today's Rust side does guarantee the post-condition.
source: ['claude']
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3908 +/- ##
============================================
+ Coverage 71.20% 72.19% +0.99%
============================================
Files 20 21 +1
Lines 2837 2946 +109
============================================
+ Hits 2020 2127 +107
- Misses 817 819 +2
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Creating (broadcasting) a document in SwiftExampleApp was only reachable via Settings → Platform State Transitions → Document Create (test-signer builder, test-plan DOC-02 🧪), and the⚠️ ). There was no real, production create-document flow signed by the wallet's keychain signer.
DocumentsView"Create Document" path was a local-only mock with no broadcast (DOC-09Tracked in the iOS QA tracker #3897 (DOC-02).
What was done?
A real, schema-driven Create Document flow that broadcasts a
DocumentCreatethroughrs-platform-walletusing the wallet's keychain-backed signer (perpackages/swift-sdk/CLAUDE.md— high-level flows live inplatform-wallet, Swift only renders/marshals/persists).rs-platform-wallet—IdentityWallet::create_document_with_signer(newwallet/identity/network/document.rs): fetches the on-chainDataContract, builds a rev-1DocumentviaDocumentType::create_document_from_data, selects anAUTHENTICATION+ECDSA_SECP256K1key whose security level satisfies the doc type'ssecurity_level_requirement()(mirrors the consensus rule — not a hardcodedCRITICAL), and broadcasts via the SDK'sPutDocument::put_to_platform_and_wait_for_responseon the 8 MB worker stack. Includes unit tests for the security-level selection.rs-platform-wallet-ffi—platform_wallet_create_document_with_signerC ABI (newdocument.rs), a faithful mirror ofplatform_wallet_create_data_contract_with_signer.swift-sdk— thinManagedPlatformWallet.createDocument(...)wrapper; schema-drivenCreateDocumentView(owner-identity picker + reusedDocumentFieldsView) reachable from Contracts → contract → document type → New Document (DocumentTypeDetailsView). The confirmed document is persisted to SwiftData so it appears in the documents list. The local-only mock inDocumentsViewwas replaced by this real flow. Per-fieldaccessibilityIdentifiers added toDocumentFieldsViewfor QA automation.Bonus fix (required for the feature to be usable): the create form self-dismissed within ~1 s. Background-sync SwiftData writes invalidated the Contracts
@Querylist whoseForEachrows owned the details sheet, tearing down the contract → doc-type → create navigation every second (compounded by deprecatedNavigationView/NavigationLink(destination:)and auto-refreshing relative-timeText). Fixed by hoisting the details.sheet(item:)to a stable container, migrating to value-basednavigationDestination, making relative-time strings static (AppDate.relative), and decoupling the rootTabViewfrom SPVspvProgressticks. This also stabilizes other Contracts drill-downs.How Has This Been Tested?
cargo fmt --allclean;cargo check -p platform-wallet -p platform-wallet-fficlean;cargo test -p platform-walletsecurity-level unit tests pass.cd packages/swift-sdk && ./build_ios.sh --target sim→ BUILD SUCCEEDED (warnings-as-errors).preorderdocument (32-bytesaltedDomainHash) on the DPNS contractGWRSAVFMjXx8HpQFaNJMqBV7MBgMK4br5UESsB4S31Ecfrom a funded identity. Result: network-confirmed (put_to_platform_and_wait_for_responsereturned Ok — a rejection would surface as "Create failed"), doc id7i1hJgvVt8fJms26kGwkEZ6jVZxrfd3BrqfmAfpqXMoG,ZPERSISTENTDOCUMENTcount0 → 1, document appears in the list. After the nav fix, the form stayed presented 18s+ with no interaction (pre-fix: dismissed within ~1s). (domainwas intentionally avoided as a create target — it's a contested resource that a plain DocumentCreate can't confirm.)TEST_PLAN.md(DOC-02 🧪 → ✅, DOC-09 retired) and QA issue iOS SwiftExampleApp — TEST_PLAN QA run status & blockers #3897.Breaking Changes
None. SDK/example-app surface only; no consensus-affecting changes (no
!).Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
ManagedPlatformWallet.createDocument(...)returning the created document identifier and canonical JSON.