feat(swift-sdk): wire data contract update through the platform-wallet path#3882
Conversation
…t path The SwiftExampleApp generic transition builder defined a `dataContractUpdate` form but its handler (`executeDataContractUpdate`) was stubbed to throw "Data contract update is not yet wired through the platform-wallet path", so contract updates could not be broadcast from the app. Wire it end-to-end, mirroring the existing `createDataContract` shape: - platform-wallet: add `IdentityWallet::update_data_contract_with_signer`. It picks the owner's CRITICAL/AUTHENTICATION/ECDSA key, fetches the live contract to read its current version, assembles the new serialization format at `current_version + 1`, builds a `DataContractUpdateTransition` (fetch+bump identity-contract nonce), signs via the external signer, and broadcasts on the 8 MB-stack worker. There is no `UpdateContract` SDK extension mirroring `PutContract`, so the transition is assembled directly. - The config `$formatVersion` is selected from `contract_versions.config.default_current_version` (V1 under v12) instead of the hardcoded "0" the create path still uses — v12 rejects a V0 config on update. The create-path V0 build is fixed separately by fix/data-contract-config-v1; this change does not reintroduce V0. - rs-platform-wallet-ffi: expose `platform_wallet_update_data_contract_with_signer` (adds a `contract_id` input over the create binding). - swift-sdk: add the thin `ManagedPlatformWallet.updateDataContract(...)` wrapper and call it from `executeDataContractUpdate`, re-keying the form-flattened groups array back into the position map the V1 format wants. All orchestration stays in Rust per swift-sdk/CLAUDE.md. Co-Authored-By: Claude Opus 4.8 (1M context) <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 15 minutes and 24 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 We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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)
📝 WalkthroughWalkthroughThis PR implements data contract updates across the full platform stack. The wallet core validates version bumps and injects platform-selected config defaults, the FFI boundary marshals C arguments and dispatches to the wallet, the Swift SDK binds parameters type-safely, and the example UI wires form inputs through validation to the update API. ChangesData Contract Update Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 e6ad47a) |
![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
🧹 Nitpick comments (1)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift (1)
1916-1959: 🏗️ Heavy liftMove the contract-update orchestration into
PlatformService.This view now owns wallet resolution, contract-id decoding, JSON serialization, signer construction, and the
platform-walletcall. That keeps growing business logic in the UI layer instead of routing identity/document work through the app service boundary.As per coding guidelines,
packages/swift-sdk/SwiftExampleApp/**/*.swift: "Handle identity and document operations through PlatformService in SwiftExampleApp".🤖 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/TransitionDetailView.swift` around lines 1916 - 1959, Move the contract-update orchestration out of TransitionDetailView: create a new method on PlatformService (e.g., updateDataContract(ownerIdentity: Identity, contractId: String, newDocumentSchemas: Any?, newTokenSchemas: Any?, newGroups: Any?, modelContext: ModelContext) async throws -> Data) that performs wallet resolution (use walletManager.wallet(for:)), decodes the base58 contract id with Data.identifier(fromBase58:), serializes JSON with toJSONString (providing the "{}" default for document schemas), constructs the KeychainSigner(modelContainer:), and calls wallet.updateDataContract(ownerIdentityId:contractId:documentSchemasJSON:tokenSchemasJSON:groupsJSON:signer:), returning the updatedContractId; then replace the referenced block in TransitionDetailView to call PlatformService.updateDataContract(...) and handle the thrown errors, keeping TransitionDetailView free of wallet/encoding/signing logic and using PlatformService for the operation.Source: Coding guidelines
🤖 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/rs-platform-wallet/src/wallet/identity/network/contract.rs`:
- Around line 376-387: Before using the fetched DataContract to determine the
new version/nonce, validate that the contract's owner matches the supplied
owner_identity_id and reject with a clear PlatformWalletError if it does not;
specifically, after DataContract::fetch(...) and before reading version or
bumping the nonce, compare the existing.owner (or equivalent owner field) to
owner_identity_id and return an InvalidIdentityData (or appropriate) error on
mismatch so you don't consume nonce state for bad requests; apply the same
ownership check to the other DataContract::fetch usage in this file that also
reads version for updates.
---
Nitpick comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift`:
- Around line 1916-1959: Move the contract-update orchestration out of
TransitionDetailView: create a new method on PlatformService (e.g.,
updateDataContract(ownerIdentity: Identity, contractId: String,
newDocumentSchemas: Any?, newTokenSchemas: Any?, newGroups: Any?, modelContext:
ModelContext) async throws -> Data) that performs wallet resolution (use
walletManager.wallet(for:)), decodes the base58 contract id with
Data.identifier(fromBase58:), serializes JSON with toJSONString (providing the
"{}" default for document schemas), constructs the
KeychainSigner(modelContainer:), and calls
wallet.updateDataContract(ownerIdentityId:contractId:documentSchemasJSON:tokenSchemasJSON:groupsJSON:signer:),
returning the updatedContractId; then replace the referenced block in
TransitionDetailView to call PlatformService.updateDataContract(...) and handle
the thrown errors, keeping TransitionDetailView free of wallet/encoding/signing
logic and using PlatformService for the operation.
🪄 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: a4277d90-4c4a-4f3c-b73d-705abf4fb7cf
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/data_contract.rspackages/rs-platform-wallet/src/wallet/identity/network/contract.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The new wallet/FFI/Swift path for data-contract updates works mechanically, but its semantics are broken: the Rust side builds a full-replacement contract payload purely from caller-supplied JSON (fetching the existing contract only for its version), while the Swift call site forwards only document/token/group schemas and labels them as 'new...' additions. Description, keywords, and config will be silently wiped on every update, and the form's additive UX guarantees that the wallet broadcasts contracts the network will reject (token/group/document-type removals).
🔴 1 blocking
🤖 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/contract.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/contract.rs:388-457: Update payload replaces the full contract instead of merging changes onto the fetched contract
`update_data_contract_with_signer` fetches the live contract (line 376) only to read `existing.version()`, then assembles the entire new `DataContractInSerializationFormat` payload from caller-supplied JSON. Because `DataContractUpdateTransition::new_from_data_contract` broadcasts a complete contract definition (not a diff), any field the caller omits is dropped:
- `keywords`, `description`, `config`: Swift never forwards these on update (`TransitionDetailView.swift:1952-1959` only passes documents/tokens/groups). The V1 deserializer fills the missing fields with serde defaults (`description = None`, `keywords = []`, `config = DataContractConfigV1::default_with_version()`), so every update silently overwrites the on-chain values. Description and keywords are freely mutable on update, so this wipes them without any network-side guardrail.
- `documentSchemas` / `tokens` / `groups`: The Swift form is built around additive semantics — fields are named `newDocumentSchemas`, `newTokenSchemas`, `newGroups`, and the user help text implies existing schemas are preserved. But the wallet ships them as the complete replacement set, so a token-only update sends `documentSchemas = {}` and a doc-only update sends `tokens` absent. DPP update validation rejects document-type / token / group removals, so the common flow of 'add one new document type to an existing contract' will broadcast and fail with a confusing 'attempted removal' error.
The inline comment at TransitionDetailView.swift:1930 acknowledges full-replacement semantics, but the form fields and call site don't reflect that — the Swift side needs to either gather the full contract (pre-filled from the fetched live contract for unchanged fields) and forward all of it, or the Rust side needs to seed the new payload from `existing` and merge the caller-supplied additions in before bumping the version. Either fix needs to land before this is usable end-to-end.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "8b1d542211a9d1b0a13e5a6d27241a7bd841e0f2778076dc8e6bf13a80f3a51b"
)Xcode manual integration:
|
…and validate owner `update_data_contract_with_signer` rebuilt the entire `DataContractInSerializationFormat` payload from the caller's JSON and reused the fetched contract only for its version. Since a `DataContractUpdateTransition` broadcasts a complete contract definition (not a diff), every omitted section was reset to serde defaults: keywords/description/config were silently wiped, and document-type/token/group sets were dropped — which DPP rejects as illegal removals, so "add one document type" failed with a confusing "attempted removal" error. Now the update payload is seeded from the fetched on-chain contract (serialized via `DataContractInSerializationFormat`, already a valid V1, so the config version is preserved with no manual tagging) and the caller's sections are overlaid: - documentSchemas / tokens / groups: merged key-by-key (add or replace an entry; never drop an existing one). - keywords / description / config: overridden only when the caller supplies them; otherwise the on-chain values are kept. A supplied config without a `$formatVersion` tag inherits the base contract's on-chain config version. - id + owner stay as the fetched contract's; version = existing + 1. The merge lives in a pure, unit-tested `merge_contract_update_payload` helper. Also validates `existing.owner_id() == owner_identity_id` immediately after the fetch and before the identity-contract nonce fetch/bump, so an owner/contract mismatch fails as a local validation error instead of consuming nonce state. The Swift call site and FFI signature are unchanged (they already forward the additive sections); only stale full-replacement comments were updated to describe the merge semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Re: the nitpick to move the contract-update orchestration from |
…ntract-update # Conflicts: # packages/rs-platform-wallet/src/wallet/identity/network/contract.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The prior blocking finding from 70c1269 is FIXED in f364298: update_data_contract_with_signer now seeds the payload from DataContractInSerializationFormat::try_from_platform_versioned of the fetched contract and overlays caller sections via merge_contract_update_payload (additive for documentSchemas/tokens/groups, override-only-when-supplied for keywords/description/config), preserves id+owner from the fetched contract, and adds a pre-nonce owner-mismatch check. The codex-general 'blocking' regression on the create-path V0 config tag is OUT OF SCOPE — the V0 injection is pre-existing code at e15b620, untouched by this PR, and the author's prior commit message explicitly tracks the create-path V0 fix in a separate branch (fix/data-contract-config-v1). One narrow architectural suggestion remains on the V1-shape assumption.
🟡 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/src/wallet/identity/network/contract.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/contract.rs:437-461: Merge assumes the base format is V1-shaped
`DataContractInSerializationFormat::try_from_platform_versioned` dispatches on `platform_version.dpp.contract_versions.contract_serialization_version.default_current_version` and can return either the V0 or V1 shape. `merge_contract_update_payload` unconditionally writes `tokens` / `groups` / `keywords` / `description` / `config` keys when supplied, which only belong on the V1 variant. Under v12 the default is V1, so this is fine today, but if the dispatch ever resolves to V0 (older versions, downgraded configs, future code paths pinning the version) the merged JSON would carry V1-only keys that the V0 round-trip through `serde_json::from_str::<DataContractInSerializationFormat>` would reject. Consider either pinning the seed to V1 explicitly (`DataContractInSerializationFormatV1::try_from_platform_versioned` + `.into()`) or gating the V1-only overlays on `matches!(base_format, DataContractInSerializationFormat::V1(_))`. A debug_assert or doc-note on the assumption would also help future readers.
| let base_format = DataContractInSerializationFormat::try_from_platform_versioned( | ||
| &existing, | ||
| platform_version, | ||
| ) | ||
| .map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Failed to convert existing contract to serialization format: {e}" | ||
| )) | ||
| })?; | ||
| let base_value = serde_json::to_value(&base_format).map_err(|e| { | ||
| PlatformWalletError::InvalidIdentityData(format!( | ||
| "Failed to serialize existing contract format: {e}" | ||
| )) | ||
| })?; | ||
|
|
||
| let merged_value = merge_contract_update_payload( | ||
| base_value, | ||
| new_version, | ||
| parse_required_json("documents_schema_json", documents_schema_json)?, | ||
| parse_optional_json("tokens_schema_json", tokens_schema_json)?, | ||
| parse_optional_json("groups_schema_json", groups_schema_json)?, | ||
| parse_optional_json("keywords_json", keywords_json)?, | ||
| description.filter(|s| !s.is_empty()), | ||
| parse_optional_json("config_json", config_json)?, | ||
| )?; |
There was a problem hiding this comment.
🟡 Suggestion: Merge assumes the base format is V1-shaped
DataContractInSerializationFormat::try_from_platform_versioned dispatches on platform_version.dpp.contract_versions.contract_serialization_version.default_current_version and can return either the V0 or V1 shape. merge_contract_update_payload unconditionally writes tokens / groups / keywords / description / config keys when supplied, which only belong on the V1 variant. Under v12 the default is V1, so this is fine today, but if the dispatch ever resolves to V0 (older versions, downgraded configs, future code paths pinning the version) the merged JSON would carry V1-only keys that the V0 round-trip through serde_json::from_str::<DataContractInSerializationFormat> would reject. Consider either pinning the seed to V1 explicitly (DataContractInSerializationFormatV1::try_from_platform_versioned + .into()) or gating the V1-only overlays on matches!(base_format, DataContractInSerializationFormat::V1(_)). A debug_assert or doc-note on the assumption would also help future readers.
source: ['claude-general']
There was a problem hiding this comment.
Resolved in e6ad47a — Merge assumes the base format is V1-shaped 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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior blocking finding (full contract replacement in update_data_contract_with_signer) is fully resolved: the implementation now fetches the live contract, validates ownership before consuming nonce state, and merges caller-supplied document/token/group sections onto the fetched serialization-format JSON via the new unit-tested merge_contract_update_payload helper. Six independent agents converge on FIXED with no new in-scope blocking issues. Codex-general's secondary concern about V0 config preservation is a real edge case but not a regression of this PR (which enables updates for V1+ contracts where none worked before) — recorded as a maintainer follow-up rather than blocking.
2 participants
Issue being fixed or feature implemented
The SwiftExampleApp's generic transition builder defines a
dataContractUpdatetransition form, but its execution handler
(
TransitionDetailView.executeDataContractUpdate) was stubbed to throw"Data contract update is not yet wired through the platform-wallet path".
As a result, contract updates could not be broadcast from the app — only fresh
contract creates worked.
What was done?
Wired
executeDataContractUpdateend-to-end through theplatform-walletpath, mirroring the existing
createDataContractflow. A new wallet-sidemethod was added (no existing update path existed):
platform-wallet—IdentityWallet::update_data_contract_with_signer(
packages/rs-platform-wallet/src/wallet/identity/network/contract.rs).It picks the owner identity's CRITICAL + AUTHENTICATION + ECDSA key, fetches
the live contract from Platform to read its current version, assembles the
new serialization-format payload at
current_version + 1(DPP requires astrictly incremented version), deserializes/validates it via
DataContractInSerializationFormat+try_from_platform_versioned, thenfetches+bumps the identity-contract nonce, builds a
DataContractUpdateTransition, signs via the external signer, broadcasts onthe 8 MB-stack worker, and waits for the confirmed contract. There is no
UpdateContractSDK extension mirroringPutContract, so the transition isassembled directly here.
Config version fix (cross-cutting) — the config
$formatVersionisselected from
platform_version.dpp.contract_versions.config.default_current_version(which is
1/V1 under v12) rather than the hardcoded"0"the create pathstill uses. v12 rejects a V0 config on update. The create-path V0 build is
fixed separately by
fix/data-contract-config-v1; this change deliberatelydoes not touch the create path and does not reintroduce V0.
rs-platform-wallet-ffi— exposedplatform_wallet_update_data_contract_with_signer(
packages/rs-platform-wallet-ffi/src/data_contract.rs), adding acontract_idinput over the create binding. The C header is regenerated bycbindgen at build time, so no manual header edit is needed.
swift-sdk— added the thinManagedPlatformWallet.updateDataContract(...)wrapper (
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift)and called it from
executeDataContractUpdate(
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionDetailView.swift),re-keying the form-flattened
newGroupsarray back into the position-keyedmap the V1 format expects. All orchestration stays in Rust per
swift-sdk/CLAUDE.md; Swift only marshals.How Has This Been Tested?
cargo fmt --allcargo check -p platform-wallet— cleancargo check -p platform-wallet-ffi— cleancargo clippy -p platform-wallet -p platform-wallet-ffi— clean (no findings)iOS build + combined end-to-end testing is handled separately by the
orchestrator and was intentionally not run here.
Breaking Changes
None. This is SDK/app surface (a new wallet method + FFI binding + Swift
wrapper), not a consensus change.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit