fix(swift-sdk): sign document state transitions with an available AUTHENTICATION key#3922
Conversation
…HENTICATION key selectSigningKey preferred "any CRITICAL-security key" for every document operation, so an identity that owns a CRITICAL TRANSFER key (now common, since identities are derived with a transfer key) had that TRANSFER key selected to sign document-batch state transitions. The platform rejected them with "Invalid identity key purpose TRANSFER. This state transition requires AUTHENTICATION". Delegate selection to the SDK's existing KeyManager.findSigningKey, which (a) filters by purpose (.authentication, or .critical+.authentication for the contract branch) and (b) skips any candidate whose private key is not present in the Keychain - the same dual scheme the KeychainSigner trampoline uses to resolve the key at sign time. For an identity with a CRITICAL auth key that has no stored private material plus a HIGH auth key that does, it correctly falls through to the HIGH auth key instead of the CRITICAL TRANSFER key. KeyManager/KeychainManager are @mainactor, but five of the six document ops select inside an off-main DispatchQueue.global block, so the selection is made @mainactor and hoisted to the MainActor portion of each op before the continuation; the chosen IdentityPublicKey (Sendable) is captured into the work block. All six ops (create/replace/delete/transfer/update-price/ purchase) share the one fixed selector. Verified end-to-end on testnet: Purchase Document by an identity owning a CRITICAL TRANSFER key now signs with its AUTHENTICATION key, the platform accepts it, and document ownership transfers on-chain; Update Price by the same identity also succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 59 minutes and 11 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 (1)
✨ 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 c23ee75) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Swift-only fix that replaces a buggy ad-hoc 'first CRITICAL key' picker (which could select a TRANSFER key) with a delegation to KeyManager.findSigningKey filtered on AUTHENTICATION purpose and Keychain availability, and hoists the now-@mainactor selector out of the off-actor DispatchQueue closures. The implementation is correct for the SwiftExampleApp flow (the only realistic caller), preserves FFI ownership/lifecycle, and resolves the qagaussqa-style regression. One residual design suggestion about coupling the selector to the shared Keychain.
🟡 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/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift:29-51: Selector preflights the shared Keychain even though SDK methods accept an arbitrary signer handle
`selectSigningKey` now goes through `KeyManager.withSharedKeychain().findSigningKey(...)`, which only returns a candidate if its private material is in `KeychainManager.shared`. The six `documentCreate/Replace/Delete/Transfer/UpdatePrice/Purchase` methods still take a generic `signer: OpaquePointer`, and the SDK separately exposes `KeyManager.createSigner(from:)` / `dash_sdk_signer_create_from_private_key`, so a caller can validly pass a non-shared-Keychain signer (raw private key, hardware-backed callback signer, or a Keychain-backed signer bound to a different KeychainManager). For those callers, this preflight will reject a key the supplied signer can actually sign and surface 'No suitable key found' before the signer is ever consulted; conversely, picking a key by shared-Keychain presence can pick a key the supplied signer cannot sign. In this repo every current call site (SwiftExampleApp TransitionDetailView, etc.) passes a `KeychainSigner` backed by the shared `KeychainManager`, so this is a latent design issue rather than an active bug — but the cleanest shape is to rank candidates by purpose/security in Swift and delegate availability to the supplied signer (e.g. a small `can_sign_with` FFI helper) instead of `KeychainManager.shared`. Document the coupling on the SDK method signatures if you intend to keep it.
| let km = KeyManager.withSharedKeychain() | ||
|
|
||
| // For contract create/update, require CRITICAL + AUTHENTICATION | ||
| // Contract create/update require a CRITICAL + AUTHENTICATION key. | ||
| if operation == "CONTRACT CREATE" || operation == "CONTRACT UPDATE" { | ||
| if let k = keys.first(where: { $0.securityLevel == .critical && $0.purpose == .authentication }) { | ||
| if let k = km.findSigningKey(for: identity, purpose: .authentication, minimumSecurityLevel: .critical, preferCritical: true) { | ||
| print("📝 [\(operation)] Selected CRITICAL AUTHENTICATION key #\(k.id)") | ||
| return k | ||
| } | ||
| print("❌ [\(operation)] No CRITICAL AUTHENTICATION key found") | ||
| print("❌ [\(operation)] No CRITICAL AUTHENTICATION key with available private material") | ||
| return nil | ||
| } | ||
|
|
||
| // Otherwise prefer CRITICAL, then AUTHENTICATION, then any | ||
| if let k = keys.first(where: { $0.securityLevel == .critical }) { | ||
| print("📝 [\(operation)] Selected CRITICAL key #\(k.id)") | ||
| return k | ||
| } | ||
| if let k = keys.first(where: { $0.purpose == .authentication }) { | ||
| print("📝 [\(operation)] Selected AUTHENTICATION key #\(k.id)") | ||
| // Document state transitions require an AUTHENTICATION-purpose key. | ||
| // Prefer a CRITICAL auth key, but findSigningKey skips any candidate | ||
| // whose private key is not in the Keychain, so an identity whose only | ||
| // CRITICAL key is a TRANSFER key (or a CRITICAL auth key with no stored | ||
| // private material) correctly falls through to its HIGH auth key. | ||
| if let k = km.findSigningKey(for: identity, purpose: .authentication, minimumSecurityLevel: nil, preferCritical: true) { | ||
| print("📝 [\(operation)] Selected AUTHENTICATION key #\(k.id) (security \(k.securityLevel.name))") | ||
| return k | ||
| } | ||
| let k = keys.first | ||
| if let k = k { print("📝 [\(operation)] Selected fallback key #\(k.id)") } | ||
| return k | ||
| print("❌ [\(operation)] No AUTHENTICATION key with available private material") | ||
| return nil |
There was a problem hiding this comment.
🟡 Suggestion: Selector preflights the shared Keychain even though SDK methods accept an arbitrary signer handle
selectSigningKey now goes through KeyManager.withSharedKeychain().findSigningKey(...), which only returns a candidate if its private material is in KeychainManager.shared. The six documentCreate/Replace/Delete/Transfer/UpdatePrice/Purchase methods still take a generic signer: OpaquePointer, and the SDK separately exposes KeyManager.createSigner(from:) / dash_sdk_signer_create_from_private_key, so a caller can validly pass a non-shared-Keychain signer (raw private key, hardware-backed callback signer, or a Keychain-backed signer bound to a different KeychainManager). For those callers, this preflight will reject a key the supplied signer can actually sign and surface 'No suitable key found' before the signer is ever consulted; conversely, picking a key by shared-Keychain presence can pick a key the supplied signer cannot sign. In this repo every current call site (SwiftExampleApp TransitionDetailView, etc.) passes a KeychainSigner backed by the shared KeychainManager, so this is a latent design issue rather than an active bug — but the cleanest shape is to rank candidates by purpose/security in Swift and delegate availability to the supplied signer (e.g. a small can_sign_with FFI helper) instead of KeychainManager.shared. Document the coupling on the SDK method signatures if you intend to keep it.
source: ['codex']
There was a problem hiding this comment.
Good catch — agreed this is a real coupling. A few notes on why it's intentional for now, plus what I changed:
-
The availability check is necessary, not incidental. The bug is precisely that an identity's CRITICAL authentication key can have no private material on the device (qagaussqa keyId 1), so a pure purpose+security ranking would still hand back an unsignable key — just failing with a different error instead of the TRANSFER-purpose rejection. So we can't simply "rank in Swift and let the signer fail."
-
Today the coupling is consistent. Every caller of these six ops passes a
KeychainSigner, whose backing keychain defaults toKeychainManager.shared— the exact storeKeyManager.withSharedKeychain().findSigningKey(...)queries. So the preflight checks the same store the signer uses at sign time. -
The cleaner shape needs an FFI that doesn't exist yet. Delegating availability to the supplied signer would require a
dash_sdk_signer_can_sign-style export that invokes the signer vtable'scan_signslot from Swift. That slot is currently only called internally by Rust during signing; adding the export is a Rust/FFI change + xcframework rebuild, out of scope for this Swift-only fix.
I've documented the coupling and the would-be-FFI on selectSigningKey (pushed as a follow-up docs(swift-sdk) commit). Happy to file a follow-up to add the can_sign FFI and switch to signer-delegated availability if you'd like to remove the coupling for real.
There was a problem hiding this comment.
Resolved in c23ee75 — Selector preflights the shared Keychain even though SDK methods accept an arbitrary signer handle 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.
Address review feedback: the availability check in selectSigningKey is done against KeychainManager.shared, which is correct for every current caller (all pass a KeychainSigner defaulting to .shared) but is a coupling worth documenting. Note the caller contract and the FFI that would be needed to delegate availability to an arbitrary supplied signer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest delta (4123871..c23ee75) is documentation only — it extends the selectSigningKey docstring to record the implicit dependency on KeychainManager.shared. The prior shared-Keychain coupling finding is INTENTIONALLY DEFERRED via this documentation (no runtime change, all current callers use shared-Keychain KeychainSigner, fix requires a new dash_sdk_signer_can_sign FFI). No new latest-delta findings; cumulative changes (purpose=AUTHENTICATION key selection, MainActor hoisting, IdentityPublicKey Sendable) remain sound. Approving.
2 participants
Issue being fixed or feature implemented
Every SwiftExampleApp document state transition (create / replace / delete / transfer / update-price / purchase) failed on-chain for any identity that owns a CRITICAL-security TRANSFER key:
This is now common, because identities are derived with a CRITICAL TRANSFER key (keyId added by the Rust
derive_and_persistchange). Reproduced end-to-end via DOC-07 (Purchase Document) with buyerqagaussqa.Root cause:
selectSigningKey(from:operation:)inpackages/swift-sdk/Sources/SwiftDashSDK/FFI/StateTransitionExtensions.swift. For every operation except CONTRACT CREATE/UPDATE it fell to a general branch that preferred any.criticalkey regardless of purpose — selecting the CRITICAL TRANSFER key. Document-batch state transitions require an AUTHENTICATION-purpose key. The helper is shared by all six document ops, so this broke every document state transition for identities with a CRITICAL TRANSFER key.What was done?
Rewrote
selectSigningKeyto delegate to the SDK's existingKeyManager.findSigningKey, which:.authenticationfor document ops (.critical+.authenticationfor the contract branch).identityId+keyIndex, thenpublicKeyHex) theKeychainSignertrampoline uses to resolve the key at sign time.For
qagaussqa(keyId 1 AUTH/CRITICAL with no stored private key, keyId 5 AUTH/HIGH with a private key, keyId 6 TRANSFER/CRITICAL with a private key) it now correctly selects keyId 5 instead of keyId 6.KeyManager/KeychainManagerare@MainActor, but five of the six ops previously selected inside an off-mainDispatchQueue.global().asyncblock. SoselectSigningKeyis now@MainActor, and the call is hoisted to the MainActor portion of each op (before the continuation); the chosenIdentityPublicKey(which isSendable) is captured into the work block. FFI calls, signer handling, defer/cleanup, method signatures, and the SwiftExampleAppexecuteDocument*handlers are unchanged — the one shared fix covers all six ops.Swift-only change — no Rust/xcframework rebuild required.
How Has This Been Tested?
Built (
SwiftExampleApp, iPhone 17 simulator,EXCLUDED_ARCHS=x86_64) → BUILD SUCCEEDED, and driven end-to-end on testnet:qagaussqa(owns a CRITICAL TRANSFER key), contract5jpKat9U82PGcfmeYm8QZZWX6q7zDo2C32PZMxHpbiGB, typecard, documentCdg8JTz6Rbx6Hq8swZLkVCLML3acRTtsShwVfrUpG2mX(price 100000 credits). Signing succeeded (no purpose rejection); on-chain the document'sowner_idchanged from85KjYZ…to hex0A3DA9E619A40A6C…=qagaussqa, revision bumped to 3. Cross-checked against the app's SwiftData store.qagaussqa, new price 50000 →"Document price updated successfully". Exercises a different signing call site.Both ops would have signed with the CRITICAL TRANSFER key and been rejected under the old code; the platform accepting them confirms an AUTHENTICATION key is now used. The remaining ops (create/replace/delete/transfer) run the identical shared selector.
Breaking Changes
None. This is a Swift SDK behavior fix; it is not consensus-breaking and does not change any public API signature.
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code