fix(sdk): persist the contract bounds kind on Android and iOS - #4800
QuantumExplorer wants to merge 2 commits into
Conversation
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (58)
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 |
|
|
…nd-trip Android stored a key's bounds as an id plus an optional document type name and inferred the kind on restore, so a key bound to a contract group (kind 3) came back as a single-contract bound to the group id and the local signing preflight refused valid batches until the identity was refreshed. - public_keys gains a nullable contractBoundsKind column (schema 12, manual MIGRATION_11_12 in the style of the earlier bumps). The persist callback records the kind the native row carries; restore honours it and keeps the old inference for legacy rows with a NULL kind. - ContractBounds.ContractGroup in the SDK type, kind 3 in IdentityPubkeyCodec (an id and no document type), kind docs updated in the bridges and the JNI blob layout. - Tests: kind 3 and every other kind round-trip through the handler, legacy rows still infer 0, 1 and 2, the codec bytes for kind 3, the schema version and column, and an 11 to 12 migration test (instrumented, compiled only). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d-trip Swift stored a key's bounds as an id plus an optional document type name and inferred the kind on restore, and the persist callback dropped kinds it did not know. A key bound to a contract group (kind 3) therefore restored unbounded after a restart. - PersistentPublicKey gains an optional contractBoundsKind. The live models move to schema version 5: version 4 is frozen with the project's freeze script (36 generated files, reproducible with --check), the plan gains a lightweight 4 to 5 stage, and a version 5 store fixture written by this build joins the migration tests. A nil kind marks a legacy row and keeps the old inference. - The persist path stores the kind the FFI row carries, including 3; the restore path emits the stored kind (kind 3 with the id and no document type, a kind 2 row without its name demoted to 1 as Rust does). - ManagedPlatformWallet.ContractBounds gains contractGroup(id:), with the pin and parse paths for kind 3. The DPP-layer bounds enum is unchanged (generated frozen schemas switch over it exhaustively), so that projection reports no bounds for a group-bound row instead of a false single contract. - The example app's storage inspector shows the bounds variant. - Tests: kind 3 round trip, kinds 0 to 2 unchanged, legacy inference, unusable rows, pin and parse of kind 3, unknown kinds still rejected, and the version 4 to 5 migration. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
a5121a6 to
337cf7b
Compare
PR HygieneState: waiting-bots · commit
Self-review is an author attestation that you have read the diff: This report does not bypass CI or repository protection rules. |
|
@coderabbitai review No review for |
|
@thepastaclaw review No review for |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
⚠️ DEGRADED — Final validation — Phase 1 + Phase 2
⚠️ DEGRADED review. The primary review models were unavailable (gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache), so this review ran on stand-in models:gpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor. Both review phases and the independent verifiers still ran, but on weaker models, with Phase 1 capped athigheffort. Treat the verdict as provisional; a full-strength re-review will run on the next push once the primary models are back.
Mobile-only follow-up that correctly round-trips the FFI contract-bounds kind 3 (ContractGroup) through the Room 11→12 column and the SwiftData V4→V5 schema. Two in-scope suggestions remain: the Kotlin restore path coerces unknown/stale stored kinds to SingleContract while Swift degrades them to unbounded, and the Swift contractBounds setter comment no longer describes the kind column it now resets.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: ffi-engineer); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: security-auditor); reviewer 4: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: architecture-layering); reviewer 6: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: phase2-reviewer, role: security-auditor); final verifier: muse-spark-1.3-contributor (standing in for gpt-6-astra) (agent: astra-verifier, role: final-verifier)
- Degraded mode:
gpt-6-astraunavailable: All credentials for model gpt-6-astra are cooling down (last error: usage_limit_reached: The usage limit has been reache (detected by probe, since 2026-09-18T05:22:01Z); stand-insgpt-5.6-luna→muse-spark-1.3-contributor,gpt-5.6-sol→muse-spark-1.3-contributor,gpt-5.6-terra→muse-spark-1.3-contributor,gpt-6-astra→muse-spark-1.3-contributor; Phase 1 effort capped athigh - Triage:
normalbymuse-spark-1.3-contributor(standing in forgpt-6-astra) (effort low) — Adds a nullable contractBoundsKind column with Room 11-to-12 and SwiftData 4-to-5 migrations plus codec round-trip handling, a contained wallet-persistence fix that does not change consensus, funds, crypto, or signing logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— architecture-layering (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— ffi-engineer (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— security-auditor (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(lane failed),glm-5.3-flash(zai below 15% reserve: 5h 99% left, weekly 14% left) - Fresh verifier:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — final-verifier; agentastra-verifier - Phase 2 reviewers:
muse-spark-1.3-contributor(standing in forgpt-6-astra) — general (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — architecture-layering (completed, effort high); agentphase2-reviewer,muse-spark-1.3-contributor(standing in forgpt-6-astra) — security-auditor (completed, effort high); agentphase2-reviewer
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt:2868-2875: Unknown or zero stored bounds kind coerced to SingleContract instead of degrading to unbounded
The restore branch only matches stored kinds 3 and 1 explicitly, then falls through to doc-type inference (`docType != null → 2 else → 1`). A row with a decodable id blob and a stored kind of 0 or any future/unknown kind therefore restores as kind 1 (or 2 when a doc-type string happens to be present) — asserting a SingleContract bound the key does not have. The Swift restore path in the same PR (`(1...3).contains(boundsKind)`, else kind 0) degrades those rows to unbounded, so the two platforms disagree on the same row, and the persist side stores the native kind verbatim, so a future kind would be stored then mis-restored on Android only. Branch on the stored kind first and degrade anything unrecognized to 0; also use isNullOrEmpty so an empty-string doc-type does not restore as kind 2 on Android while Swift demotes it to 1.
In `packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentPublicKey.swift:131-135: contractBounds setter comment omits the kind column it now resets
The setter now also resets `contractBoundsKind` (0/1), but the comment still tells callers that writing `contractBoundsDocumentTypeName` after the setter is enough for a `.singleContractDocumentType` round-trip. Since `effectiveContractBoundsKind` prefers the stored kind, a doc-type written after this setter without also writing kind 2 restores as `.singleContract` and silently drops the qualifier. The only current caller overwrites the kind afterwards, so nothing is broken today, but the guidance is wrong for the next caller — update it in the same diff that changed the setter body.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Unknown future bounds kinds fail open in the local wallet view — Speculative hardening for a kind that does not exist yet; the actionable part (Kotlin degrading unknown kinds to 0 like Swift) is kept as an in-scope suggestion above, and the remainder asks for a separate fail-closed redesign tracked outside this PR.
- Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
| val boundsId = pk.contractBoundsData?.let { contractBoundsJsonToId(it) } | ||
| val (kind, id) = when { | ||
| boundsId == null -> 0.toByte() to ByteArray(0) | ||
| pk.contractBoundsKind == 3 -> 3.toByte() to boundsId | ||
| pk.contractBoundsKind == 1 -> 1.toByte() to boundsId | ||
| pk.contractBoundsDocumentTypeName != null -> 2.toByte() to boundsId | ||
| else -> 1.toByte() to boundsId | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Unknown or zero stored bounds kind coerced to SingleContract instead of degrading to unbounded
The restore branch only matches stored kinds 3 and 1 explicitly, then falls through to doc-type inference (docType != null → 2 else → 1). A row with a decodable id blob and a stored kind of 0 or any future/unknown kind therefore restores as kind 1 (or 2 when a doc-type string happens to be present) — asserting a SingleContract bound the key does not have. The Swift restore path in the same PR ((1...3).contains(boundsKind), else kind 0) degrades those rows to unbounded, so the two platforms disagree on the same row, and the persist side stores the native kind verbatim, so a future kind would be stored then mis-restored on Android only. Branch on the stored kind first and degrade anything unrecognized to 0; also use isNullOrEmpty so an empty-string doc-type does not restore as kind 2 on Android while Swift demotes it to 1.
| val boundsId = pk.contractBoundsData?.let { contractBoundsJsonToId(it) } | |
| val (kind, id) = when { | |
| boundsId == null -> 0.toByte() to ByteArray(0) | |
| pk.contractBoundsKind == 3 -> 3.toByte() to boundsId | |
| pk.contractBoundsKind == 1 -> 1.toByte() to boundsId | |
| pk.contractBoundsDocumentTypeName != null -> 2.toByte() to boundsId | |
| else -> 1.toByte() to boundsId | |
| } | |
| val boundsId = pk.contractBoundsData?.let { contractBoundsJsonToId(it) } | |
| val storedKind = pk.contractBoundsKind | |
| val (kind, id) = when { | |
| boundsId == null -> 0.toByte() to ByteArray(0) | |
| storedKind == 3 -> 3.toByte() to boundsId | |
| storedKind == 1 -> 1.toByte() to boundsId | |
| storedKind == 2 && !pk.contractBoundsDocumentTypeName.isNullOrEmpty() -> 2.toByte() to boundsId | |
| storedKind == 2 -> 1.toByte() to boundsId | |
| storedKind == 0 -> 0.toByte() to ByteArray(0) | |
| storedKind == null && !pk.contractBoundsDocumentTypeName.isNullOrEmpty() -> 2.toByte() to boundsId | |
| storedKind == null -> 1.toByte() to boundsId | |
| else -> 0.toByte() to ByteArray(0) | |
| } |
source: muse-spark-1.3-contributor (phase1-reviewer: architecture-layering, ffi-engineer, security-auditor); muse-spark-1.3-contributor (phase2-reviewer: general, architecture-layering, security-auditor)
| // round-trip should write `contractBoundsDocumentTypeName` | ||
| // explicitly after this setter, or go through | ||
| // `PersistentPublicKey.from(IdentityPublicKey, identityId:)` | ||
| // which sets both columns atomically. | ||
| // which sets every column atomically. |
There was a problem hiding this comment.
🟡 Suggestion: contractBounds setter comment omits the kind column it now resets
The setter now also resets contractBoundsKind (0/1), but the comment still tells callers that writing contractBoundsDocumentTypeName after the setter is enough for a .singleContractDocumentType round-trip. Since effectiveContractBoundsKind prefers the stored kind, a doc-type written after this setter without also writing kind 2 restores as .singleContract and silently drops the qualifier. The only current caller overwrites the kind afterwards, so nothing is broken today, but the guidance is wrong for the next caller — update it in the same diff that changed the setter body.
| // round-trip should write `contractBoundsDocumentTypeName` | |
| // explicitly after this setter, or go through | |
| // `PersistentPublicKey.from(IdentityPublicKey, identityId:)` | |
| // which sets both columns atomically. | |
| // which sets every column atomically. | |
| // Callers that want the full `.singleContractDocumentType` | |
| // round-trip must write `contractBoundsDocumentTypeName` AND | |
| // `contractBoundsKind` (2) explicitly after this setter — the | |
| // stored kind wins over inference on restore, so a doc-type | |
| // without kind 2 comes back as `.singleContract` — or go |
source: muse-spark-1.3-contributor (phase2-reviewer: general)
github-actions
Bot
added
the
bot-review-missed
2 participants
Issue being fixed or feature implemented
Follow-up to #4793, which has merged; this branch is rebased onto
v4.2-devand carries only the two mobile commits. Until it merges,v4.2-devemits a bounds kind the Kotlin and Swift layers do not know. #4793 lets an AUTHENTICATION key carry contract bounds naming a contract group, which crosses the wallet FFI ascontract_bounds_kind == 3with the group id and no document type.Neither mobile store can round-trip that. Both persist a key's bounds as an id plus an optional document type name and infer the kind on restore, so a group bound comes back as a single-contract bound to the group id on Android (the local signing preflight then refuses valid batches until the identity is refreshed) and unbounded on Swift. Consensus enforces the stored bounds either way; this is about the wallet's own view of the key.
Split out of #4793 so the Rust change and the mobile persistence can be reviewed separately.
What was done?
Android (
kotlin-sdk)public_keysgains a nullablecontractBoundsKindcolumn. Database version 11 to 12 with a manualMIGRATION_11_12, in the style of the earlier bumps;12.jsonwas generated by Room.ContractBounds.ContractGroupin the SDK type, kind 3 inIdentityPubkeyCodec(an id and no document type), kind docs updated in the bridges.iOS (
swift-sdk)PersistentPublicKeygains an optionalcontractBoundsKind; a nil kind marks a legacy row and keeps the old inference. The persist path stores the kind the FFI row carries, the restore path emits it (kind 3 with the id and no document type).ManagedPlatformWallet.ContractBoundsgainscontractGroup(id:)with the pin and parse paths for kind 3. The example app's storage inspector shows the bounds variant.DashModelContainer("when the next property lands: freeze every model here into the version being retired, add a version, add a stage, commit a store written by this build"), the live models move to schema version 5, version 4 is frozen withscripts/freeze_schema_models.py(36 generated files, reproducible with--check), the plan gains a lightweight 4 to 5 stage, anddash-v5.storewas written by this build.Decision for reviewers: is schema version 4 shipped?
Version 4 arrived with #4589 on 2026-09-08 and has only been in the 4.2 dev tags. The migration tests treat it as shipped ("adding a version to the plan is shipping it"), which is why this PR cuts version 5. The same doc also allows changing the live version before it ships by rewriting its fixture on purpose. If dev builds do not count as shipped, this shrinks to: the column on the live version 4 model plus a regenerated
dash-v4.store, with no version 5 and none of the 36 frozen files, at the cost that stores written by dev builds since 2026-09-08 fail to open and need a reset. Say which and I will change it.How Has This Been Tested?
swift testfor the migration suite (14, including the new 4 to 5 migration and the fixture) and the new bounds suite (8);scripts/freeze_schema_models.py --check(73 frozen files match) with its script tests; anxcodebuildof SwiftExampleApp for the simulator with warnings as errors.287e8aebb7, the head of feat(platform)!: allow authentication keys bound to a contract group #4793 before the split, where every CI workflow passed, including the Kotlin SDK build, the Swift SDK build and tests, and the Swift frozen schema check.Breaking Changes
None for consensus. Local stores migrate forward: Room 11 to 12, SwiftData schema 4 to 5 (lightweight).
Notes for reviewers
ContractBoundsenum deliberately gets no group case: generated frozen schema copies switch over it exhaustively, so that projection reports no bounds for a group-bound row rather than a false single contract. The FFI restore path carries kind 3.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code