feat(swift-example-app): production document replace/delete/transfer/price/purchase UI (DOC-03..07)#3945
Conversation
…chase with-signer flows + FFI (WIP) Adds the 5 document operation flows next to create_document_with_signer at the platform-wallet lib + FFI layers (DOC-03..07). Compiles (cargo check) + fmt'd. Swift wrappers + UI to follow. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…wrappers Add ManagedPlatformWallet.replaceDocument / deleteDocument / transferDocument / setDocumentPrice / purchaseDocument, modeled on createDocument. Each marshals the owner/contract/document ids + the op-specific arg, pins the KeychainSigner with withExtendedLifetime across the synchronous FFI call, calls the new platform_wallet_document_* export, and parses the canonical query-side document JSON the FFI returns (delete returns only the document id, no JSON). Persistence stays in the view layer per the swift-sdk persist/load/bridge rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Replace/Delete/Transfer/Set-price/Purchase actions to the document detail view via an action menu + hoisted, pre-filled sheets, routed through the new ManagedPlatformWallet document wrappers. Owner-gated (Transfer needs transferable, Set-price needs tradeMode); Purchase shown to a different controlled identity on priced docs. Not yet built/verified. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cts tab DocumentsView (whose DocumentDetailView hosts the new replace/delete/transfer/set-price/purchase action menu) was orphaned — never instantiated anywhere — so those actions were unreachable. Wire it up from the Contracts tab root: add a "Browse Documents" (doc.text.magnifyingglass) toolbar button alongside the existing "+" that presents DocumentsView as a sheet (it wraps its own NavigationView, so a sheet avoids nesting navigation stacks). Forward the three EnvironmentObjects it declares (AppState, PlatformWalletManager, TransitionState) explicitly, matching the sibling sheets; the SwiftData modelContext is inherited. The toolbar button carries accessibilityIdentifier "contracts.browseDocuments" for UI automation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n UI Replace/Delete/Transfer/Set-Price each driven end-to-end on testnet through the new Contracts → Browse Documents → ⋯ action menu, confirmed + persisted on the card doc FgVSYG6sTZZ9 (revision 1→4, owner moved BjJz3hdmg5Ec→QA2, then deleted). Purchase UI is implemented and correctly gated (offered only for non-controlled for-sale docs); a fresh self-buy can't be tested by consensus design. Flip DOC-03..07 + MW-04 from 🧪 to ✅ and refresh the entry-point reality-check note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 50 minutes and 10 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds production-ready document lifecycle operations (replace, delete, transfer, set-price, purchase) across the full stack: shared Rust wallet helper methods for contract/document/signing-key resolution, a centralized FFI JSON serialization helper, five new async Swift SDK wrappers on ChangesDocument Lifecycle Operations — Full Stack
Sequence Diagram(s)sequenceDiagram
actor User
participant DocumentsView
participant DocumentDetailView
participant ActionForm as Replace/Transfer/Purchase/etc. View
participant DocumentActionRunner
participant ManagedPlatformWallet
participant RustFFI as platform_wallet FFI (Rust)
participant DocumentPersistence
User->>DocumentsView: Tap document row
DocumentsView->>DocumentDetailView: Present detail sheet
User->>DocumentDetailView: Open action menu → select action
DocumentDetailView->>DocumentsView: onAction(DocumentActionMode)
DocumentsView->>ActionForm: Present via DocumentActionSheet
User->>ActionForm: Fill form + submit
ActionForm->>DocumentActionRunner: resolveSigning(identity, keyManager)
DocumentActionRunner-->>ActionForm: (signingKeyId, KeychainSigner)
ActionForm->>ManagedPlatformWallet: replaceDocument / transferDocument / purchaseDocument / ...
ManagedPlatformWallet->>RustFFI: platform_wallet_document_*(signingKeyId, signer.handle, ...)
RustFFI-->>ManagedPlatformWallet: (documentId bytes, canonicalJSON C-string)
ManagedPlatformWallet-->>ActionForm: (Identifier, canonicalJSON String)
ActionForm->>DocumentPersistence: updateDocument / updateOwner / deleteDocument
DocumentPersistence-->>ActionForm: success / warning
ActionForm-->>User: dismiss + show result
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 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 1e0518d) |
…r flows The replace/transfer/set_price/purchase document flows take 8 args (owner/purchaser, contract, type, doc id, [recipient|price], signing_key_id, signer) — over clippy's 7 limit. CI runs clippy with -D warnings, so the lint fails the build (local build_ios.sh only runs cargo build). Mirror the #[allow] the FFI entry points already carry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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
🤖 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/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift`:
- Around line 1206-1230: The shared transitionState.documentPrice state is not
being reset when the DocumentsView appears, causing stale pricing from previous
documents to enable the Purchase button prematurely. In the .onAppear modifier
that currently sets documentIdField to document.documentId, also reset
transitionState.documentPrice to zero (or nil) to ensure clean state when
opening a new document's purchase flow, preventing the submit() function from
using stale price values.
- Around line 632-642: The replaceDocument wallet operation is being called
directly from the DocumentsView, which couples the UI layer to operation
orchestration and bypasses the PlatformService boundary. Extract the
replaceDocument call along with its parameters (ownerIdentityId, contractId,
documentType, documentId, propertiesJSON, signingKeyId, signer) into a new
method in PlatformService, then replace the direct wallet call in the view with
a call to this new PlatformService method. Apply the same pattern to the other
similar wallet operation calls mentioned at lines 765-774, 929-937, 1080-1088,
and 1268-1276, ensuring all document mutations are routed through
PlatformService while keeping the view focused on input/state rendering.
🪄 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: f04f6343-9b0d-4718-a285-b9f93d2eca4e
📒 Files selected for processing (6)
packages/rs-platform-wallet-ffi/src/document.rspackages/rs-platform-wallet/src/wallet/identity/network/document.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/ContractsTabView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
transitionState.documentPrice is app-wide shared state; PurchaseDocumentView gated its submit button and submit() on it but never cleared it on entry, so a stale non-zero price from a prior probe could enable Purchase (and be broadcast) before this document's price republished. Reset it on .onAppear/.onDisappear; the disabled DocumentWithPriceView refetches the current document's price. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR adds five document mutate FFI/SDK paths and SwiftUI surfaces. Rust signing/broadcast layering is sound and mirrors the audited create path. One in-scope UI gating defect: the action menu offers Replace/Delete on every owned document without consulting the document type's documentsMutable/documentsCanBeDeleted flags, while sibling actions (transfer, set-price) are gated. Several smaller suggestions around pre-broadcast validation and test coverage.
🔴 1 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(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/DocumentsView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift:279-287: Gate Replace/Delete actions on documentsMutable / documentsCanBeDeleted
`availableActions` unconditionally appends `.replace` and `.delete` whenever `ownerIsControlled`, while `.transfer` is gated on `documentsTransferable` and `.setPrice` on `tradeMode > 0`. `PersistentDocumentType` already persists `documentsMutable` and `documentsCanBeDeleted` (used elsewhere in `DataContractDetailsView`, `TransitionInputView`, `StorageRecordDetailViews`), and those are the contract-level rules consensus enforces. For immutable or non-deletable document types, this UI now exposes a production broadcast path that can only be rejected by Platform — the user pays processing fees and gets a confusing failure. Follow the same gating pattern as transfer/set-price.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DocumentsView.swift:1236-1296: Purchase reads price from shared TransitionState — risk of stale price between sheet present and async probe
`PurchaseDocumentView.submit()` reads `transitionState.documentPrice`, a global `@EnvironmentObject` populated asynchronously by `DocumentWithPriceView` (also used by `TransitionInputView`). On sheet appear, `documentIdField` is seeded which kicks `DocumentWithPriceView.handleDocumentIdChange` to reset `documentPrice = nil` and start a 0.5s debounce before fetching. Between presentation and fetch completion, if the user previously probed a different document via the test-signer flow, `documentPrice` can briefly carry a stale non-zero value before the onChange clears it. The guard only requires `price > 0`, so a stale value passes. Either own the price state in this sheet (don't read from the shared `TransitionState`) or add a fresh-probe flag scoped to this presentation.
In `packages/rs-platform-wallet/src/wallet/identity/network/document.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/document.rs:687-734: Verify the fetched document's $price before signing a purchase
`purchase_document_with_signer` fetches the current document but then ignores its on-chain `$price`, signing whatever `price: u64` the caller supplied. The Swift purchase sheet sources its price from a shared `TransitionState.documentPrice` populated by an async sibling probe — not strictly scoped to the current `(contract, type, document_id)` — so a stale or in-flight value can be signed at the wallet boundary. If the price is stale, Platform rejects the transition after work and may bump the purchaser's nonce/charge fees. Since this function already has the fetched document, read its `$price` and reject before resolving the signing key if it is zero/not-for-sale or does not match the requested `price`. That makes the wallet authoritative for the price the signature commits to, rather than trusting an FFI caller.
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/document.rs:390-429: Add unit tests for resolve_authentication_signing_key
The PR description advertises "unit tests for the security-level resolution," but the only tests in this module (`allowed_levels_*`) cover the pre-existing `allowed_signing_security_levels` helper used by the create path — not the new `resolve_authentication_signing_key`. That function is the only Rust-side defense for the mutate flows: it must reject non-AUTHENTICATION purpose and non-ECDSA_SECP256K1 key types, and the error strings are surfaced to Swift via FFI. A direct test that constructs an identity with mixed-purpose / mixed-key-type keys and asserts both error paths would lock in the `requires AUTHENTICATION` consensus-rule defense documented in the function's comments.
| if ownerIsControlled { | ||
| actions.append(.replace) | ||
| actions.append(.delete) | ||
| if docType?.documentsTransferable == true { | ||
| actions.append(.transfer) | ||
| } | ||
| if (docType?.tradeMode ?? 0) > 0 { | ||
| actions.append(.setPrice) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Gate Replace/Delete actions on documentsMutable / documentsCanBeDeleted
availableActions unconditionally appends .replace and .delete whenever ownerIsControlled, while .transfer is gated on documentsTransferable and .setPrice on tradeMode > 0. PersistentDocumentType already persists documentsMutable and documentsCanBeDeleted (used elsewhere in DataContractDetailsView, TransitionInputView, StorageRecordDetailViews), and those are the contract-level rules consensus enforces. For immutable or non-deletable document types, this UI now exposes a production broadcast path that can only be rejected by Platform — the user pays processing fees and gets a confusing failure. Follow the same gating pattern as transfer/set-price.
| if ownerIsControlled { | |
| actions.append(.replace) | |
| actions.append(.delete) | |
| if docType?.documentsTransferable == true { | |
| actions.append(.transfer) | |
| } | |
| if (docType?.tradeMode ?? 0) > 0 { | |
| actions.append(.setPrice) | |
| } | |
| if ownerIsControlled { | |
| if docType?.documentsMutable == true { | |
| actions.append(.replace) | |
| } | |
| if docType?.documentsCanBeDeleted == true { | |
| actions.append(.delete) | |
| } | |
| if docType?.documentsTransferable == true { | |
| actions.append(.transfer) | |
| } | |
| if (docType?.tradeMode ?? 0) > 0 { | |
| actions.append(.setPrice) | |
| } |
source: ['codex']
| pub async fn purchase_document_with_signer<S>( | ||
| &self, | ||
| purchaser_identity_id: &Identifier, | ||
| contract_id: &Identifier, | ||
| document_type_name: &str, | ||
| document_id: &Identifier, | ||
| price: u64, | ||
| signing_key_id: u32, | ||
| signer: &S, | ||
| ) -> Result<Document, PlatformWalletError> | ||
| where | ||
| S: Signer<IdentityPublicKey> + Send + Sync, | ||
| { | ||
| let data_contract = self | ||
| .fetch_contract_arc_for_document_op(contract_id, document_type_name) | ||
| .await?; | ||
|
|
||
| let mut document = self | ||
| .fetch_current_document(&data_contract, document_type_name, document_id) | ||
| .await?; | ||
| document.increment_revision().map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Failed to increment document revision: {e}" | ||
| )) | ||
| })?; | ||
|
|
||
| let signing_key = self | ||
| .resolve_authentication_signing_key(purchaser_identity_id, signing_key_id) | ||
| .await?; | ||
|
|
||
| let builder = DocumentPurchaseTransitionBuilder::new( | ||
| data_contract, | ||
| document_type_name.to_string(), | ||
| document, | ||
| *purchaser_identity_id, | ||
| price as Credits, | ||
| ); | ||
| let DocumentPurchaseResult::Document(confirmed) = self | ||
| .sdk | ||
| .document_purchase(builder, &signing_key, &SignerRef(signer)) | ||
| .await | ||
| .map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Failed to purchase document: {e}" | ||
| )) | ||
| })?; | ||
| Ok(confirmed) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Verify the fetched document's $price before signing a purchase
purchase_document_with_signer fetches the current document but then ignores its on-chain $price, signing whatever price: u64 the caller supplied. The Swift purchase sheet sources its price from a shared TransitionState.documentPrice populated by an async sibling probe — not strictly scoped to the current (contract, type, document_id) — so a stale or in-flight value can be signed at the wallet boundary. If the price is stale, Platform rejects the transition after work and may bump the purchaser's nonce/charge fees. Since this function already has the fetched document, read its $price and reject before resolving the signing key if it is zero/not-for-sale or does not match the requested price. That makes the wallet authoritative for the price the signature commits to, rather than trusting an FFI caller.
source: ['codex']
| private func submit() { | ||
| guard let purchaser = eligiblePurchasers.first(where: { $0.identityIdBase58 == selectedPurchaserId }) else { | ||
| actionError = .init(message: "Select a purchaser identity held by a loaded wallet.") | ||
| return | ||
| } | ||
| guard let price = transitionState.documentPrice, price > 0 else { | ||
| actionError = .init(message: "This document is not for sale (no price found).") | ||
| return | ||
| } | ||
| let docType = document.documentType_relation | ||
| let wallet: ManagedPlatformWallet | ||
| let signingKeyId: UInt32 | ||
| do { | ||
| // The purchaser signs (and becomes the new owner), so resolve | ||
| // the AUTHENTICATION key on the purchaser, not the owner. | ||
| (wallet, signingKeyId) = try DocumentActionRunner.resolveSigning( | ||
| for: purchaser, documentType: docType, walletManager: walletManager | ||
| ) | ||
| } catch { | ||
| actionError = .init(message: error.localizedDescription) | ||
| return | ||
| } | ||
|
|
||
| isSubmitting = true | ||
| let signer = KeychainSigner(modelContainer: modelContext.container) | ||
| let purchaserId = purchaser.identityId | ||
| let contractId = document.contractIdData | ||
| let typeName = document.documentType | ||
| let docId = document.id | ||
|
|
||
| Task { | ||
| do { | ||
| let (confirmedDocId, canonicalJSON) = try await wallet.purchaseDocument( | ||
| purchaserId: purchaserId, | ||
| contractId: contractId, | ||
| documentType: typeName, | ||
| documentId: docId, | ||
| price: price, | ||
| signingKeyId: signingKeyId, | ||
| signer: signer | ||
| ) | ||
| _ = signer | ||
| await MainActor.run { | ||
| persistWarning = DocumentPersistence.applyOwnerChange( | ||
| document: document, | ||
| newOwnerId: purchaserId, | ||
| canonicalJSON: canonicalJSON, | ||
| modelContext: modelContext | ||
| ) | ||
| confirmedId = confirmedDocId.toBase58String() | ||
| isSubmitting = false | ||
| didComplete = true | ||
| } | ||
| } catch { | ||
| await MainActor.run { | ||
| actionError = .init(message: error.localizedDescription) | ||
| isSubmitting = false | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Purchase reads price from shared TransitionState — risk of stale price between sheet present and async probe
PurchaseDocumentView.submit() reads transitionState.documentPrice, a global @EnvironmentObject populated asynchronously by DocumentWithPriceView (also used by TransitionInputView). On sheet appear, documentIdField is seeded which kicks DocumentWithPriceView.handleDocumentIdChange to reset documentPrice = nil and start a 0.5s debounce before fetching. Between presentation and fetch completion, if the user previously probed a different document via the test-signer flow, documentPrice can briefly carry a stale non-zero value before the onChange clears it. The guard only requires price > 0, so a stale value passes. Either own the price state in this sheet (don't read from the shared TransitionState) or add a fresh-probe flag scoped to this presentation.
source: ['claude']
| .disabled(isSubmitting || UInt64(priceText.trimmingCharacters(in: .whitespaces)) == nil) | ||
| } | ||
| } | ||
|
|
||
| private func ownerIdentity() -> PersistentIdentity? { | ||
| identities.first { | ||
| $0.network == appState.currentNetwork | ||
| && $0.wallet != nil | ||
| && $0.identityIdBase58 == document.ownerIdBase58 | ||
| } | ||
| } | ||
|
|
||
| private func submit() { | ||
| guard let owner = ownerIdentity() else { | ||
| actionError = .init(message: DocumentActionError.identityNotFound.localizedDescription) | ||
| return | ||
| } | ||
| guard let price = UInt64(priceText.trimmingCharacters(in: .whitespaces)) else { | ||
| actionError = .init(message: "Enter a valid price in credits.") | ||
| return | ||
| } | ||
| let docType = document.documentType_relation | ||
| let wallet: ManagedPlatformWallet |
There was a problem hiding this comment.
💬 Nitpick: SetDocumentPriceView accepts price=0; broadcast is doomed and burns fees
The submit button is disabled only when UInt64(priceText) fails to parse, so 0 is a valid input and reaches submit(). Platform rejects a set-price transition with a zero price (set-price is an explicit list operation; unlisting is a separate transition). The user pays processing fees on the failed broadcast. Mirror PurchaseDocumentView's (price ?? 0) == 0 guard: reject 0 in the UI guard or surface a 'Price must be greater than zero' message before submit.
source: ['claude']
| async fn resolve_authentication_signing_key( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| signing_key_id: u32, | ||
| ) -> Result<IdentityPublicKey, PlatformWalletError> { | ||
| let wm = self.wallet_manager.read().await; | ||
| let info = wm.get_wallet_info(&self.wallet_id).ok_or_else(|| { | ||
| PlatformWalletError::WalletNotFound( | ||
| "Wallet info not found in wallet manager".to_string(), | ||
| ) | ||
| })?; | ||
| let manager = &info.identity_manager; | ||
| let identity = manager | ||
| .identity(owner_identity_id) | ||
| .map(|m| m.identity.clone()) | ||
| .ok_or(PlatformWalletError::IdentityNotFound(*owner_identity_id))?; | ||
| let key = identity | ||
| .get_public_key_by_id(signing_key_id) | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Signing key {signing_key_id} not found on identity {owner_identity_id}" | ||
| )) | ||
| })? | ||
| .clone(); | ||
| if key.purpose() != Purpose::AUTHENTICATION { | ||
| return Err(PlatformWalletError::InvalidIdentityData(format!( | ||
| "Signing key {signing_key_id} on identity {owner_identity_id} has purpose {:?}, \ | ||
| but a document state transition must be signed with an AUTHENTICATION key", | ||
| key.purpose() | ||
| ))); | ||
| } | ||
| if key.key_type() != KeyType::ECDSA_SECP256K1 { | ||
| return Err(PlatformWalletError::InvalidIdentityData(format!( | ||
| "Signing key {signing_key_id} on identity {owner_identity_id} has key type {:?}, \ | ||
| but a document state transition must be signed with an ECDSA_SECP256K1 key", | ||
| key.key_type() | ||
| ))); | ||
| } | ||
| Ok(key) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add unit tests for resolve_authentication_signing_key
The PR description advertises "unit tests for the security-level resolution," but the only tests in this module (allowed_levels_*) cover the pre-existing allowed_signing_security_levels helper used by the create path — not the new resolve_authentication_signing_key. That function is the only Rust-side defense for the mutate flows: it must reject non-AUTHENTICATION purpose and non-ECDSA_SECP256K1 key types, and the error strings are surfaced to Swift via FFI. A direct test that constructs an identity with mixed-purpose / mixed-key-type keys and asserts both error paths would lock in the requires AUTHENTICATION consensus-rule defense documented in the function's comments.
source: ['claude']
| static func applyOwnerChange( | ||
| document: PersistentDocument, | ||
| newOwnerId: Identifier, | ||
| canonicalJSON: String, | ||
| modelContext: ModelContext | ||
| ) -> String? { | ||
| let blob = canonicalJSON.data(using: .utf8) ?? document.data | ||
| document.updateProperties(blob) | ||
| document.ownerId = newOwnerId.toBase58String() | ||
| document.ownerIdData = newOwnerId | ||
| document.revision = nextRevision(from: canonicalJSON, fallback: document.revision) | ||
| // Re-link the owner relationship if the new owner is local. | ||
| document.ownerIdentity = nil | ||
| document.linkToLocalIdentityIfNeeded(in: modelContext) | ||
| return save(modelContext) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Local cache owner derived from Swift input rather than parsed canonical JSON
DocumentPersistence.applyOwnerChange writes document.ownerId / document.ownerIdData from the Swift-supplied newOwnerId (recipient or purchaser) rather than reading $ownerId from the FFI-returned canonical JSON. The canonical JSON is the authoritative confirmed-on-chain shape; trusting Swift's expectation creates a silent divergence vector if the two ever disagree (cache race, future wrapper bug). The $revision extraction already follows the stronger pattern via nextRevision(from:). On-chain state is unaffected — local-cache integrity only.
source: ['claude']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push delta (1e0518d) only adds four #[allow(clippy::too_many_arguments)] lint suppressions on the new document mutate wrappers — no behavioral change. All six prior verified findings remain STILL VALID at HEAD (re-checked DocumentsView.swift:279-294, 1038, 1227-1296, 1323-1338 and document.rs:390-429, 691-738). One additional in-scope cumulative finding: resolve_authentication_signing_key validates purpose and key type but not security level, leaving the wallet boundary weaker than the create path's allowed_signing_security_levels check.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
5 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/rs-platform-wallet/src/wallet/identity/network/document.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/document.rs:390-429: Also reject signing keys whose security level is not document-allowed
`resolve_authentication_signing_key` validates purpose (AUTHENTICATION) and key type (ECDSA_SECP256K1) but does not validate the security level against what the document type's signing rule allows. The create path uses `allowed_signing_security_levels(required_level)` for this — explicitly excluding MASTER for non-master-required document operations and restricting to CRITICAL..=requirement. The mutate flows take an explicit `signing_key_id` from the UI key picker but accept any AUTHENTICATION ECDSA key regardless of security level, so a UI bug or future FFI caller can produce a wallet-signed transition Platform will reject — same fee-burn/nonce-bump exposure as the price issue above. Mirror the create-path check by passing the document type's `signing_security_level_requirement()` here and rejecting keys whose `security_level()` is not in `allowed_signing_security_levels(required)`.
| async fn resolve_authentication_signing_key( | ||
| &self, | ||
| owner_identity_id: &Identifier, | ||
| signing_key_id: u32, | ||
| ) -> Result<IdentityPublicKey, PlatformWalletError> { | ||
| let wm = self.wallet_manager.read().await; | ||
| let info = wm.get_wallet_info(&self.wallet_id).ok_or_else(|| { | ||
| PlatformWalletError::WalletNotFound( | ||
| "Wallet info not found in wallet manager".to_string(), | ||
| ) | ||
| })?; | ||
| let manager = &info.identity_manager; | ||
| let identity = manager | ||
| .identity(owner_identity_id) | ||
| .map(|m| m.identity.clone()) | ||
| .ok_or(PlatformWalletError::IdentityNotFound(*owner_identity_id))?; | ||
| let key = identity | ||
| .get_public_key_by_id(signing_key_id) | ||
| .ok_or_else(|| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Signing key {signing_key_id} not found on identity {owner_identity_id}" | ||
| )) | ||
| })? | ||
| .clone(); | ||
| if key.purpose() != Purpose::AUTHENTICATION { | ||
| return Err(PlatformWalletError::InvalidIdentityData(format!( | ||
| "Signing key {signing_key_id} on identity {owner_identity_id} has purpose {:?}, \ | ||
| but a document state transition must be signed with an AUTHENTICATION key", | ||
| key.purpose() | ||
| ))); | ||
| } | ||
| if key.key_type() != KeyType::ECDSA_SECP256K1 { | ||
| return Err(PlatformWalletError::InvalidIdentityData(format!( | ||
| "Signing key {signing_key_id} on identity {owner_identity_id} has key type {:?}, \ | ||
| but a document state transition must be signed with an ECDSA_SECP256K1 key", | ||
| key.key_type() | ||
| ))); | ||
| } | ||
| Ok(key) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Also reject signing keys whose security level is not document-allowed
resolve_authentication_signing_key validates purpose (AUTHENTICATION) and key type (ECDSA_SECP256K1) but does not validate the security level against what the document type's signing rule allows. The create path uses allowed_signing_security_levels(required_level) for this — explicitly excluding MASTER for non-master-required document operations and restricting to CRITICAL..=requirement. The mutate flows take an explicit signing_key_id from the UI key picker but accept any AUTHENTICATION ECDSA key regardless of security level, so a UI bug or future FFI caller can produce a wallet-signed transition Platform will reject — same fee-burn/nonce-bump exposure as the price issue above. Mirror the create-path check by passing the document type's signing_security_level_requirement() here and rejecting keys whose security_level() is not in allowed_signing_security_levels(required).
source: ['codex']
| static func applyOwnerChange( | ||
| document: PersistentDocument, | ||
| newOwnerId: Identifier, | ||
| canonicalJSON: String, | ||
| modelContext: ModelContext | ||
| ) -> String? { | ||
| let blob = canonicalJSON.data(using: .utf8) ?? document.data | ||
| document.updateProperties(blob) | ||
| document.ownerId = newOwnerId.toBase58String() | ||
| document.ownerIdData = newOwnerId | ||
| document.revision = nextRevision(from: canonicalJSON, fallback: document.revision) | ||
| // Re-link the owner relationship if the new owner is local. | ||
| document.ownerIdentity = nil | ||
| document.linkToLocalIdentityIfNeeded(in: modelContext) | ||
| return save(modelContext) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Derive cache owner from FFI canonical JSON, not Swift input
DocumentPersistence.applyOwnerChange refreshes data from the confirmed canonical JSON but writes ownerId/ownerIdData from the Swift-supplied newOwnerId. The canonical JSON is the authoritative confirmed-on-chain shape, and $revision already follows the stronger pattern via nextRevision(from:). On-chain state is unaffected — this is local-cache integrity only — but a future wrapper bug or race could silently diverge the cached owner from the persisted canonical document body, which then feeds ownerIsControlled-driven action gating. Parse $ownerId from the canonical JSON for symmetry with the revision path.
source: ['claude', 'codex']
2 participants
Issue being fixed or feature implemented
Document write transitions replace / delete / transfer / set-price / purchase (
DOC-03..DOC-07) were reachable in SwiftExampleApp only through theSettings → Platform State Transitionstest-signer builder. This adds production, per-document entry points that sign with the wallet keychain — mirroring the production create flow (DOC-02, #3908).What was done?
Four layers, signing through the architecturally-correct platform-wallet path (not a Swift-assembled flow):
rs-platform-wallet) —IdentityWallet::{replace,delete,transfer,set_document_price,purchase}_document_with_signer: fetch the contract/current document, schema-sanitize (replace), bump the revision, resolve an AUTHENTICATION+ECDSA signing key (the purchaser for buy), broadcast on the 8 MB worker stack, return the confirmedDocument. Unit tests for the security-level resolution.platform_wallet_document_{replace,delete,transfer,set_price,purchase}C ABI: read identifiers, route through the wallet methods, write the confirmed id + canonical query-side JSON out-params (null on error, no unwraps).ManagedPlatformWallet) —replaceDocument/deleteDocument/transferDocument/setDocumentPrice/purchaseDocumentwrappers over the FFI, using theKeychainSigner.DocumentsView: a Browse Documents toolbar entry on the Contracts tab → document detail → an ownership-gated ⋯ action menu, with a dedicated broadcast sheet per action. Gating: owner-controlled docs show Replace/Delete (+ Transfer ifdocumentsTransferable, + Set Price iftradeMode); a non-controlled for-sale doc shows Purchase (buyer ≠ owner).How Has This Been Tested?
build_ios.sh --target sim(FFI rebuilt) + SwiftExampleApp build: green.cargo fmt --checkon both Rust crates: clean.Driven end-to-end on testnet through the new production UI (iPhone 17 sim), on the
carddocFgVSYG6sTZZ9…(contract5jpKat9U82PG, type Mutable/Deletable/Transferable/TradeMode):DOC-03Replaceattack7→42,name→"As-replaced", revision 1→2DOC-06Set Price$price: 1000000, revision →3DOC-05TransferBjJz3hdmg5Ec…→8267geu4…(QA2), revision →4DOC-04DeleteDOC-07Purchase: UI implemented and gating verified — Purchase is surfaced only for a non-controlled for-sale doc (consensus rejects self-purchase), and the menu correctly omitted it for every wallet-owned doc. It shares the identical broadcast/persist path proven by the four ops above; a fresh end-to-end buy needs a counterparty-owned listing (the wallet here controls every local identity). TEST_PLANDOC-03..DOC-07+MW-04flipped 🧪→✅ with these notes.Breaking Changes
None. New FFI/SDK surface + UI only; no consensus or wire changes.
Checklist:
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation