feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim)#4204
feat(kotlin-sdk): one-time Orchard key shielded-invite API (inviter + claim)#4204bfoss765 wants to merge 25 commits into
Conversation
📝 WalkthroughWalkthroughThe PR adds policy-based Android keystore aliases, durable identity-key repair tracking with Room v8 migration support, structured signing error propagation across Kotlin, Rust, JNI, and Swift, one-time Orchard shielded identity creation, managed-identity error translation, and related tests and parity documentation. ChangesPlatform wallet and SDK updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…otFound Split out of dashpay#4183 (port of dashpay#4060) per review: the unmanaged-identity read fixes are conflict-free on v4.1-dev and should not be blocked on the Keystore rework. The FFI's blanket Option -> result conversion reports an identity the wallet does not manage as PlatformWalletFFIResultCode::NotFound (98), so the Kotlin callers' zero-handle checks were dead and every local read over an unmanaged identity threw instead of returning absence. - Dashpay: route getManagedIdentity through translateManagedIdentityNotFoundToZero so contacts()/syncState()/ payments()/sendContactRequest() treat "not managed" as null/empty/ false; ErrorInvalidHandle still propagates. - Sweep the remaining dead `== 0L` sites: ManagedPlatformWallet .inMemoryIdentityStates (one unmanaged/just-removed id no longer throws through the whole listing) and IdentityRegistration .contestedDpnsNames (the intended "identity is not managed by this wallet" NotFound now actually surfaces). - platform-wallet-ffi: platform_wallet_get_managed_identity keeps the three outcomes distinct (classify_managed_identity_outcome) so a stale/removed wallet surfaces ErrorInvalidHandle and never masquerades as an unmanaged identity; unit tests pin both arms. - DashSdkError: name the code (PLATFORM_WALLET_NOT_FOUND_CODE = 98); the 98 -> DashSdkError.NotFound mapping is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Split out of dashpay#4183 (port of dashpay#4060) per review: the typed signing error is conflict-free on v4.1-dev and should not be blocked on the Keystore rework. The KeystoreSigner "missing key" completion error travels as free text through Rust and used to come back as an opaque PlatformWallet.Generic (or WalletOperation) failure. It is now built from a shared MESSAGE_MARKER constant and recognized on the Kotlin boundary as the typed DashSdkError.PlatformWallet.SigningKeyUnavailable, so hosts can route users to key repair instead of showing a generic error. The marker is only consulted on the catch-all codes (6 / else), so the dedicated retry-semantics types are never overridden. Known limitation (kept as-is from dashpay#4183 by request): the discriminator is message-text-based (message.contains on the marker); a structured error code across the FFI boundary is follow-up work in the parent PR line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Introduce KeySecurityPolicy (AUTH_GATED default / DEVICE_BOUND opt-in) and move new identity-key wrapping off the single KEYS_ALIAS onto two dedicated RSA aliases, one per policy — Keystore auth parameters are fixed at key generation, so the policies can never share an alias. The legacy KEYS_ALIAS becomes read-only: it retains whichever superseded key (auth-gated AES-GCM, or the pre-split RSA pair) an upgraded install still holds, reachable only through the decryptLegacy* migration fallbacks. The dashpay#4172 hardening survives the split alias-parameterized: write-time fingerprint capture (encryptForIdentityKeysAlias — one lookup for blob + fingerprint + producing alias), the non-generating fingerprint read, and the KeyPermanentlyInvalidatedException recovery with its generation-checked, per-alias deletion (deleteIdentityKeysAliasIfCurrentGeneration, which refuses the legacy alias by contract). WalletStorage grows the policy-taking constructor and routes identity-key writes/reads through the policy alias. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…und alias, honestly The wallet must work without a screen lock (dashpay#4060), but the AUTH_GATED authentication gate cannot exist without one — KeyMint rejects generate_key. Instead of silently generating a gate-less key under the auth-gated alias (a policy lie), identity-key writes on a lockless device are redirected to KEYS_ALIAS_DEVICE_BOUND, whose gate-less parameters are inherent: - encryptForIdentityKeys resolves the effective write alias (probe first, KeyMint safety-net rejection second) and returns the producing alias with the blob; WalletStorage records it as a per-blob privkeyalias. tag written atomically with the blob + fingerprint, and reads decrypt under the RECORDED alias — keys written while lockless stay readable after a lock screen is enrolled and new writes move to the gated alias. A missing tag means the policy alias (backward compatible with earlier data). - effectiveKeySecurityPolicy() (KeystoreManager + WalletStorage) surfaces the degradation; PlatformWalletManager logs it once at construction. - Strict mode: KeystoreManager(requireAuthGated = true) throws the new KeySecurityPolicyUnavailableException instead of degrading. - MASTER_ALIAS (AES) and the DEVICE_BOUND alias keep the in-place setUnlockedDeviceRequired degradation (no auth gate to lie about); the auth-gated alias is never routed through it. - isNoSecureLockScreenKeyGenFailure is tightened: a nested KeyStoreException classifies only inside the key-gen ProviderException's cause chain AND with a lock-screen message or the observed rejection's numeric code (internal Keystore 4 / KeyMint 10309, read reflectively) — a bare nested Keystore error with an unrelated message no longer triggers the retry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…bing capability split retrievePrivateKey becomes an explicit three-rung ladder that composes the dashpay#4172 fingerprint gate with the legacy-blob recovery paths: 1. Legacy AES-GCM blobs shape-dispatch FIRST — the write-time fingerprint gate never applies to them (they predate it or carry a superseded key's fingerprint); the retained legacy KEYS_ALIAS AES key recovers the value, which is migrated to the policy alias. 2. Empty-IV RSA blobs whose stored fingerprint matches the RECORDED alias's current key (non-generating read, alias-tag routed) decrypt on the fast path. KeyPermanentlyInvalidatedException rethrows after KeystoreManager's generation-checked cleanup — after the deletion the fingerprint can no longer match, so reads and repair re-DERIVE rather than trusting a stale certificate (closes the invalidation brick loop). Other crypto failures fall through (defense in depth). 3. A mismatching or MISSING fingerprint is a routing signal into the former-KEYS_ALIAS-RSA recovery ladder, not an immediate null — pre-split blobs carry the former key's fingerprint and must reach decryptLegacyRsaKeysBlob. The policy alias is never touched here, and a read never provisions a keypair. Capability is split per finding 3: isPrivateKeyDecryptable stays the CHEAP check (presence + fingerprint only — the Rust canSignWith callback thread must never decrypt, prompt, or generate), extended with presence rungs for the legacy schemes; the real-decrypt health check moves to the new probeIdentityKeyRecoverability (DEVICE_BOUND-sibling disproof included, skipped for blobs the sibling legitimately owns by tag), which the example app's WalletKeyHealthSheet now calls. UserNotAuthenticatedException propagates from every rung — a closed auth window is never a wrong key. WalletStorageUpgradeMatrixTest pins every ladder row through the fake Keystore seam, including the new invalidation-cleanup, alias-tag-routing, fast-path-failure, and never-decrypts-on-the-cheap-path rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…key failures Port of the dashpay#4053 pending-keys machinery: when the identity-key persist callback's derive/store fails, the key lands in Room watch-only and every signature with it will fail — previously a single Log.w was the only trace. PlatformWalletPersistenceHandler now records a PendingIdentityKey entry (wallet/identity/keyId, derivation breadcrumbs, reason, timestamp) in a StateFlow keyed by public-key hex, exposed through PlatformWalletManager.pendingIdentityKeys so hosts can surface a repair path. Transactional with the changeset round (finding de3cf44a71fc): while a store round is open, record/clear mutations are staged as pure map deltas in the round's ChangesetBuffer and published in ONE atomic MutableStateFlow.update only after the Room transaction commits; a rolled-back or aborted round discards them with the buffer, leaving the pre-round map untouched. Standalone upserts publish immediately, as does markIdentityKeyRepaired — which repairIdentityKey now calls on success, since the repair stores the scalar through the deriver and bypasses the only persist path that clears pending entries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rt (Room 7→8) pendingIdentityKeys was process-lifetime only: the durable public_keys row kept just privateKeyKeychainIdentifier = null, dropping the derivation breadcrumbs a repair needs — kill the app and the repair signal was gone. - Room migration 7→8 adds nullable public_keys.derivationIdentityIndex / derivationKeyIndex. Room is the durability substrate deliberately: the wallet-deletion cascade removes these rows, so pending entries die with their wallet automatically (a DataStore side-table would leak). - onPersistIdentityKeyUpsert records the breadcrumbs whenever Rust supplied derivation indices — success AND failure paths (the breadcrumb is not a failure marker; the null identifier is). - reconstructPendingIdentityKeysFromPersistence rebuilds the map on launch: a breadcrumbed, non-read-only row re-seeds pending when its identifier is null OR the stored blob fails the CHEAP capability check — the second disjunct resurrects repair slots for blobs stranded by a Keystore keypair replacement, not just never-derived keys. One atomic update, live entries never overwritten, reason = "reconstructed from persistence after restart". PlatformWalletManager.loadPersistedWallets runs it before the manager is handed to the host. - DashDatabaseMigrationTest gains the 7→8 case (NULL for pre-existing rows, explicit values accepted); the 4→latest and 1→latest chains extend to 8; schema 8.json exported. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
repairIdentityKey previously routed through storeIfAbsent, whose shape+fingerprint usability short-circuit can be satisfied by a blob that does not actually decrypt — the derive was skipped and markIdentityKeyRepaired fired anyway, clearing the pending signal while the key stayed broken (dashpay#4060 finding 6). - WalletStorage.replacePrivateKey: forced store with the same outside-the-lock derive discipline (and tombstone checks) as storeIfAbsent, but on re-entry it UNCONDITIONALLY replaces blob + fingerprint + alias tag in one atomic edit — never the addOwnerIfUsableLocked short-circuit. - PrivateKeyDeriver.deriveAndStore gains force: Boolean = false; IdentityKeyPrivateKeyDeriver routes force → replacePrivateKey. The persistence-callback call site keeps force = false (idempotent upserts must not re-derive on every sync). - PlatformWalletManager.repairIdentityKey: deriveAndStore(force = true), then VERIFY with the real-decrypt probeIdentityKeyRecoverability before clearing pending state — a wrong-key crypto failure fails the repair with a typed PlatformWallet.SigningKeyUnavailable (UserNotAuthenticatedException counts as verified: the key is present and opens after auth, and the just-written fingerprint rules out the wrong-key-behind-locked-gate ambiguity). Only after verification: markIdentityKeyRepaired + the Room rows' privateKeyKeychainIdentifier update, so the durable pending-repair reconstruction does not resurrect the repaired key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t.NotFound PlatformWalletFFIResultCode::NotFound (98) — the blanket Option→result miss (unknown wallet id, unmanaged identity, …) — previously collapsed into the top-level DashSdkError.NotFound shared with rs-sdk-ffi codes 7/8. It now maps to the new DashSdkError.PlatformWallet.NotFound, keeping 98 inside the wallet-error family in exact parity with Swift's PlatformWalletError.notFound, so callers can match a wallet-level absence without sniffing Generic codes. BREAKING: Kotlin hosts catching DashSdkError.NotFound from platform-wallet operations (e.g. rescanSpvFilters with an unknown wallet id) now receive DashSdkError.PlatformWallet.NotFound; rs-sdk-ffi codes 7/8 still map to the top-level NotFound. Dashpay's managed-identity local reads are unaffected — translateManagedIdentityNotFoundToZero intercepts the RAW code (dashpay#4051) before this mapping ever runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the signer wire Replace end-to-end message sniffing for the signer's "missing key" failure with a typed discriminator (dashpay#4060 finding 7): - rs-sdk-ffi: DashSDKSignerErrorCode { Generic = 0, SigningKeyUnavailable = 1, AuthenticationFailed = 2 (reserved) }; SignCompletionCallback and dash_sdk_sign_async_completion gain error_code: i32 (before error_message). SignResult stays Result<Vec<u8>, ProtocolError> (a new rs-dpp ProtocolError variant would carry serialization blast radius), so code 1 rides the single Rust-owned machine prefix DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX through ProtocolError::Generic — typed at both ABI edges, one constant bridging the string segment. This is an internal coordinated ABI change: every piece versions together in this monorepo. - rs-platform-wallet-ffi: PlatformWalletFFIResultCode:: ErrorSigningKeyUnavailable = 31 (codes 26-28 are reserved for dashpay#4185's reservation-token errors and 29/30 for dashpay#4184's asset-lock errors on sibling branches — documented in the enum as dashpay#4184 does). The From<dpp::ProtocolError> conversion restores the typed code from the prefix FIRST (before the loose keyword sniffs), and the From<PlatformWalletError> blanket impl restores it on the catch-all only (dedicated retry-semantics codes are never overridden) — covering the Sdk(dash_sdk::Error::Protocol(..)) wrapping path. - JNI/Kotlin: SignerNative.completeSign(token, signature, errorCode, errorMessage); KeystoreSigner passes SIGNER_ERROR_CODE_KEY_UNAVAILABLE on the null-key branch (keeping the MESSAGE_MARKER text for the transition window) and Generic everywhere else. DashSdkError maps 31 → PlatformWallet.SigningKeyUnavailable; the dashpay#4191 marker sniff on the catch-all codes remains as a deprecated old-native fallback with a removal note tied to the next minor release. - Swift: KeychainSigner trampolines forward the code (missing-row / missing-scalar outcomes classify as 1); PlatformWalletResultCode gains errorSigningKeyUnavailable = 31 → PlatformWalletError .signingKeyUnavailable (Kotlin parity). - Tests: rs-sdk-ffi completion-code tests (prefix present for code 1, absent for generic), platform-wallet-ffi prefix→31 tests on both conversion points, Kotlin code-31 + fallback-marker tests, Swift mapping and trampoline-classifier tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…as split The inherited dashpay#4172 instrumented tests hardcode the legacy KEYS_ALIAS and the single-alias API; after the alias split the default-policy storage writes under KEYS_ALIAS_AUTH_GATED, so simulating keypair replacement by deleting the legacy alias no longer invalidates anything (the CI failure shumkov diagnosed on storeIfAbsentRederivesWhenTheKeysAliasKeypairWasReplaced). - WalletStorageOwnershipTest: the replaced-keypair, rejected-blob, and no-regeneration probes delete/inspect KEYS_ALIAS_AUTH_GATED; the fingerprint-capture test uses encryptForIdentityKeysAlias (and pins the captured producing alias); the stale-invalidation-cleanup test uses the alias-parameterized deleteIdentityKeysAliasIfCurrentGeneration. - KeystoreSignerInstrumentedTest: canSignWith rejection simulates replacement of the policy alias. The five DashPayUnlockAndSyncTest / WalletManagerRoundTripTest failures from PR dashpay#4183's CI run were NOT test or SDK defects: the branch predated dashpay#4172's workflow hardening (screen_off_timeout / stayon / dismiss-keyguard + the hard deviceLocked=0 guard), so the emulator re-locked mid-run and every MASTER_ALIAS operation threw InvalidKeyException. Rebuilding on the current base inherits the fixed workflow; those tests are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ity, and Room v8 shift - sdk-parity-manifest.json (+ regenerated PARITY_SUMMARY.md): new capability entries for the KeySecurityPolicy alias split (Swift not-applicable — iOS Keychain has no per-alias auth-parameter analog), the now-CONVERGENT platform-wallet code-98 typed mapping (was a divergence), the Kotlin-only durable pending-repair surface (Swift gap recorded), and the structured SigningKeyUnavailable discriminator (both hosts, unit-verification pointers on each). shared_symbols gains dash_sdk_sign_async_completion. - KOTLIN_SWIFT_SHARED_PARITY_SPEC.md: rationale for each divergence / convergence, the deprecated MESSAGE_MARKER fallback with its next-minor-release removal horizon, and the migration ladder update (v8 = derivation breadcrumbs; invitations shift to v9). - KOTLIN_MIGRATION_LEFTOVERS.md: marker-fallback removal and the on-device KeyPermanentlyInvalidatedException residual (unit-tier only via the fake seam, same as dashpay#4172). - Swift: testPlatformWalletNotFoundFFIResultMapping pins the convergent 98 mapping the manifest references. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…closed auth window as recoverable probeIdentityKeyRecoverability treated UserNotAuthenticatedException as "recoverable" unconditionally. UNAE is thrown at cipher.init — before the ciphertext is examined — so after a Keystore loss + regeneration the fresh auth-gated key (whose window is closed in the steady state; it is only open ~30 s after an auth) reported a blob it can never open as healthy: the key-health sheet offered no repair while pendingIdentityKeys simultaneously listed the same key (dashpay#4060 round-2 finding 1). Prompt-free fix: UNAE counts as recoverable ONLY while the blob's stored write-time fingerprint still matches the recorded alias's CURRENT key (non-generating read). A mismatch disproves ownership → not recoverable → repair is offered; a fingerprint-matched blob behind a closed window stays recoverable (genuinely just needs auth). The legacy former-RSA rung keeps the documented residual (no fingerprint surface for the retained legacy key). New matrix rows pin both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ped signer code on the first attempt A KPIE escaping the sign path (retrieveKeyWithAuth — the biometric-gate retry only handles UserNotAuthenticatedException) previously fell into the signAsync catch-all and completed with SIGNER_ERROR_CODE_GENERIC: the host saw an opaque wallet-operation failure on the FIRST post-invalidation sign and only got the typed SigningKeyUnavailable on subsequent attempts (once the fingerprint gate reported the blob non-current). dashpay#4060 round-2 finding 2. - signWithStoredKey catches KPIE explicitly and completes with SIGNER_ERROR_CODE_KEY_UNAVAILABLE (marker text + invalidation reason kept in the message), so the typed code rides the wire immediately. - The signAsync catch-all classifies via the new pure completionErrorCodeFor(t) (KPIE → key-unavailable, everything else generic) as belt-and-braces for KPIE thrown outside the retrieval call. - KeystoreSignerCompletionCodeTest pins the classification (the full signer cannot be constructed on the JVM — its constructor creates a native handle — so the classification is a pure companion function). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… for legacy-alias keys A KeyPermanentlyInvalidatedException on a LEGACY-alias-backed key was invisible everywhere: the legacy Keystore aliases are read-only (no deletion boundary), so hasLegacyKeysKey() stays true forever — canSignWith keeps offering the key, the cheap capability check keeps reporting the blob usable, the restart reconstruction never seeds a repair slot, and every sign fails opaquely (dashpay#4060 round-2 finding 3). Fix without making the cheap check expensive (it is untouched): the sign path's KPIE classification now invokes an onSigningKeyInvalidated hook, wired by PlatformWalletManager to PlatformWalletPersistenceHandler.recordSigningKeyInvalidated, which nulls privateKeyKeychainIdentifier on the key's Room rows — the same durable signal the restart reconstruction reads — and re-runs the reconstruction so pendingIdentityKeys seeds after the FIRST failed sign (reason: "signing key permanently invalidated"), making repair reachable outside the health sheet. Best-effort: bookkeeping failure never eats the typed completion. Harmless for policy-alias keys (their generation-checked deletion already flips the fingerprint gate; this just accelerates the in-process seed). Pre-v8 rows without breadcrumbs still get the identifier null-out and seed once the next persist round back-fills their breadcrumbs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g-2 TOCTOU window Round-2 nits (dashpay#4060): - The MESSAGE_MARKER fallback's stated rationale — "old native library + new Kotlin partial builds" — was not viable: the sign-completion JNI arity change (3→4 args) makes that pairing a type-confused native call on every completion, so mixed artifacts are unsupported outright. The fallback's real purpose is the dashpay#4191 merge-order transition (whose marker-based classification predates the typed code 31) plus defense in depth for conversion paths that lose the machine prefix. KDoc/comments in DashSdkError, KeystoreSigner, DashSdkErrorTest, and the parity/leftovers docs now say so; removal horizon unchanged (next minor release). - retrievePrivateKey rung 2 gains a comment naming the accepted fingerprint-read→decrypt TOCTOU window (pre-existing dashpay#4172 parity): a concurrent rotation fails as a wrong-key crypto error into the recovery ladder — never stale plaintext. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…former-RSA rung
Round-3 verification finding: tryFormerRsaRecovery collapsed every
GeneralSecurityException to null ('not this key'), which swallowed a
KPIE from an invalidated pre-alias-split key — the signer's classifier
never saw it, so the typed completion and the durable repair seeding
(finding-3 hook) never fired for legacy blobs. KPIE now propagates like
UserNotAuthenticatedException, with a matrix row pinning the rethrow and
the post-regen recovery path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s and verify the pubkey before persisting Blocker 1 (shumkov): the repair path derived from caller-supplied indices (the example app passed the DPP key id as keyIndex), and its only check was probeIdentityKeyRecoverability — which proves the stored blob DECRYPTS, not that the derived key is the RIGHT one. A wrong index derives a different valid scalar that round-trips fine, clears pending state, and persists an unusable key. Fix: - IdentityKeyPrivateKeyDeriver: on the force (repair) path derive the KEYPAIR and verify the derived public key equals publicKeyData BEFORE any store; a mismatch throws IdentityKeyDerivationMismatchException without persisting. - PlatformWalletPersistenceHandler.repairIdentityKeyDurably: new orchestration that reads the derivation slot from the PERSISTED public_keys breadcrumbs (derivationIdentityIndex/derivationKeyIndex), never a caller index; a row without breadcrumbs fails the repair without clearing pending. Hoisted here (not the JVM-unconstructable manager) so it is unit-testable and shares the authoritative pendingIdentityKeys state. - PlatformWalletManager.repairIdentityKey: drop the identityIndex/keyIndex params and delegate; supply only the wallet-scoped probe. - WalletKeyHealthSheet: stop passing report.identityIndex / key.keyId. Tests (PlatformWalletPersistenceHandlerTest): correct breadcrumbs derive, verify, clear pending, and record the durable identifier (deriver called with the persisted 3/5, not a caller index); mismatched breadcrumbs are rejected (mismatch exception), nothing persisted, blob never probed, pending intact; a row without breadcrumbs fails with SigningKeyUnavailable and never derives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Blocker 3 (shumkov): in the repair path the Room privateKeyKeychainIdentifier update was wrapped in runCatching (swallowed on failure) while the pending state was cleared regardless. A failed durable write then resurrected the repair after restart (the reconstruction reads the persisted rows) while the live session believed it was done — session and restart disagreed. Fix: the durable write in repairIdentityKeyDurably now fails CLOSED — it runs through an injectable persistDurableIdentifier seam (default: the production public_keys write) and its exception propagates; markIdentityKeyRepaired only runs after the write commits. A failed durable write leaves pending intact and the repair retryable, so the session and a restart agree. Test: a repair whose derive + verify succeed but whose durable write throws propagates the failure and leaves the pending entry in place. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…horitative signals Blocker 2 (shumkov): isNoSecureLockScreenKeyGenFailure classified any KeyStoreException whose message contained "generate_key" as a lock-screen rejection. But "generate_key" names the failing operation, not its cause — a transient KeyMint generation failure on a device WITH a lock screen carries it too, so a transient failure would silently and PERMANENTLY downgrade an AUTH_GATED key to DEVICE_BOUND instead of retrying. Fix: drop the bare "generate_key" substring match. Classification now rests only on the two authoritative signals — explicit lock-screen text, or the lock-screen rejection numeric code (internal Keystore 4 / KeyMint 10309, which the real android.security.KeyStoreException exposes via getNumericErrorCode()). Tests: the on-device shape now classifies via the numeric code (not the incidental generate_key text); two new negatives — a bare generate_key failure, and a transient generate_key failure under a key-gen ProviderException with a non-lock-screen numeric code — must NOT classify, so AUTH_GATED is retried, not downgraded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…olicy-alias migration Finding 4 (shumkov): migrateToPolicyAlias wrapped its encrypt + DataStore edit in runCatching, which swallows kotlin CancellationException along with genuine rewrite failures. A coroutine cancelled during the migration's suspend points was silently absorbed instead of unwinding — a structured-concurrency violation. Fix: replace runCatching with an explicit try/catch that rethrows CancellationException; only genuine rewrite failures stay best-effort (the recovered value is still returned; migration retries on the next read). Test: a cancellation landing at the migration encrypt (via the fake keystore's onNextPolicyEncrypt hook) now propagates out of retrievePrivateKey rather than being swallowed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…fallback Finding 5 (shumkov): the DEVICE_BOUND documentation promised hardware-backed storage while the implementation permits a software AndroidKeyStore fallback (generation prefers StrongBox, falls back to the TEE, and finally to a software-backed AndroidKeyStore key on devices with no secure element — generateWithLockScreenDegradation never fails generation on a missing one). Align the docs: "device-bound" means non-exportable AndroidKeyStore, not a hardware-storage guarantee; backing is hardware-isolated only where the device provides it. AUTH_GATED shares the same backing characteristics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rom b2 line Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…m_one_time_key), reconciled to base identity API Ports the L2-invitation CLAIM side from the b2 line (8008dc78b8): Verbatim grafts (byte-for-byte from b2, deps all present in base): - operations.rs: free fn identity_create_from_one_time_key (note-scan + Halo2 proof) and its supporting note-scan helper scan_notes_for_foreign_key (sync.rs), plus the one_time_key_tests module. - platform_wallet.rs: PlatformWalletManager::identity_create_from_one_time_key. - shielded_send.rs (FFI): platform_wallet_manager_shielded_identity_create_from_one_time_key (base's FFI-layer decode_identity_pubkeys/IdentityPubkeyFFI matches b2). Reconciled to base's API (NOT byte-for-byte): - funding.rs (JNI): decode_pubkeys_blob + hand-built IdentityPubkeyFFI literal (b2) -> decode_registration_pubkeys_blob + row.to_ffi() (base), plus base's tagged-payload return with ErrorShieldedBroadcastUnconfirmed handling. - Kotlin: IdentityKeyPreview.encodeForRegistration + withContext + raw return (b2) -> List<IdentityPubkey> via IdentityPubkeyCodec.encode + teardownGate.op + decodeShieldedCreatePayload (base), mirroring the tested inviter side. Pubkey-decode semantics preserved: identical key_id / pubkey bytes / order / count; role/read_only/contract-bounds source shifts from Rust-derived (b2) to caller-stamped blob (base) — base's authoritative pipeline-wide convention, already adopted by the tested inviter side. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7a450ea to
31e7bc1
Compare
|
🕓 Ready for review — 9 ahead in queue (commit 5ba2794) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The one-time Orchard invitation API follows the existing Type-20 construction, but four blocking issues remain in proof-wait compatibility, ambiguous-broadcast recovery, bearer-key handling, and FFI panic safety. The foreign-key scan also permits untrusted unfunded invitations to trigger an uncancellable genesis-to-tip scan.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 🟡 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/shielded/operations.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1706-1709: Use the affected-state wait for the Type-20 claim
The current v4.1-dev proof contract introduced by 31c69cf793 marks IdentityCreateFromShieldedPool proofs as affected-state snapshots because they authenticate the resulting identity and nullifiers but cannot bind the complete Orchard request. That commit changed the pool-funded sibling to `wait_for_affected_state`, while this new path still calls the strict `wait_for_response`. After rebasing, every valid claim proof will therefore produce `ExecutionNotProved`, enter the ambiguous fallback, and may be reported unconfirmed despite successful execution. On this older head, the same call accepts a snapshot without making that weaker guarantee explicit. Rebase onto the current proof API and use the affected-state wait, matching the sibling Type-20 path.
- [BLOCKING] packages/rs-platform-wallet/src/wallet/shielded/operations.rs:1733-1737: Persist a recovery path before returning an unconfirmed claim
This branch returns `ShieldedBroadcastUnconfirmed` without persisting the byte-identical transition or any pending claim metadata. Unlike the pool-funded sibling at lines 1392-1407, there is no `PendingRedrive`; `poke_sync_on_unconfirmed` consequently starts a sync that has no record to process, and the foreign invitation notes are intentionally absent from every subwallet. If an ambiguously accepted transition was actually dropped, nothing safely rebroadcasts it. The Kotlin wrapper compounds this because `TeardownGate.withOp` uses prompt-cancellable `withContext(Dispatchers.IO)`: JNI cannot be interrupted, so cancellation can discard the tagged identity ID or typed unconfirmed exception after native broadcast completion. Persist a pending claim containing the identity ID/index, submitted-key metadata, and re-drivable transition before broadcasting, then reconcile it across cancellation and restart; an equivalent mechanism must safely rebroadcast and treat `NullifierAlreadySpent` as evidence that the original claim executed.
In `packages/rs-unified-sdk-jni/src/funding.rs`:
- [BLOCKING] packages/rs-unified-sdk-jni/src/funding.rs:829-833: Do not marshal the spending key through ordinary unsanitized buffers
`oneTimeSk` is bearer spend authority for a funded invitation, but the generic `read_id32` leaves both its intermediate `Vec<u8>` and returned `[u8; 32]` unsanitized. The FFI and wallet layers create further plain-array copies. The generation direction similarly leaves native `sk`, combined `out`, and Kotlin's 75-byte source blob containing the spending key until their memory is reused or garbage-collected. This repository already uses `read_key32_zeroizing` for JNI private keys and explicitly scrubs invitation bearer scalars in `identity/network/invitation.rs`. Carry this key through `Zeroizing` buffers where possible and explicitly wipe all transient native and JVM arrays after copying out the one intentionally caller-owned key.
In `packages/rs-platform-wallet-ffi/src/shielded_send.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/shielded_send.rs:1580-1590: OS RNG failure aborts before the JNI panic guard can run
`generate_one_time_orchard_key` calls `OsRng.fill_bytes`, whose rand_core implementation panics when `try_fill_bytes` reports an entropy-source failure. Because that panic occurs inside this `extern "C"` export, it cannot unwind to the outer JNI `guard`; Rust aborts at the C ABI boundary first. An operating-system RNG failure can therefore terminate the Android process instead of producing a Java/native error. Change the generator to use `try_fill_bytes` and propagate a `PlatformWalletFFIResult` error, or place an internal panic fence inside this C export.
In `packages/rs-platform-wallet/src/wallet/shielded/sync.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/shielded/sync.rs:829-855: Unfunded invitation keys force an unbounded full-history scan
Every syntactically valid invitation key starts at position 0 and keeps fetching, proof-verifying, and trial-decrypting chunks until enough value is found or the complete shielded history is exhausted. An attacker can generate valid but unfunded invitation keys, so the value-based early exit provides no bound for hostile input. The supplied birth-height hint is ignored, and cancellation of the Kotlin coroutine cannot interrupt the blocking JNI operation. Add an authenticated or otherwise safe starting position, a maximum resumable scan budget, or another preflight mechanism that prevents one invitation from forcing an unlimited genesis-to-tip scan.
| let proof_result = match st | ||
| .wait_for_response::<StateTransitionProofResult>(sdk, None) | ||
| .await | ||
| { |
There was a problem hiding this comment.
🔴 Blocking: Use the affected-state wait for the Type-20 claim
The current v4.1-dev proof contract introduced by 31c69cf marks IdentityCreateFromShieldedPool proofs as affected-state snapshots because they authenticate the resulting identity and nullifiers but cannot bind the complete Orchard request. That commit changed the pool-funded sibling to wait_for_affected_state, while this new path still calls the strict wait_for_response. After rebasing, every valid claim proof will therefore produce ExecutionNotProved, enter the ambiguous fallback, and may be reported unconfirmed despite successful execution. On this older head, the same call accepts a snapshot without making that weaker guarantee explicit. Rebase onto the current proof API and use the affected-state wait, matching the sibling Type-20 path.
source: ['codex']
| None => { | ||
| return Err(PlatformWalletError::ShieldedBroadcastUnconfirmed { | ||
| identity_id, | ||
| reason: wait_err.to_string(), | ||
| }); |
There was a problem hiding this comment.
🔴 Blocking: Persist a recovery path before returning an unconfirmed claim
This branch returns ShieldedBroadcastUnconfirmed without persisting the byte-identical transition or any pending claim metadata. Unlike the pool-funded sibling at lines 1392-1407, there is no PendingRedrive; poke_sync_on_unconfirmed consequently starts a sync that has no record to process, and the foreign invitation notes are intentionally absent from every subwallet. If an ambiguously accepted transition was actually dropped, nothing safely rebroadcasts it. The Kotlin wrapper compounds this because TeardownGate.withOp uses prompt-cancellable withContext(Dispatchers.IO): JNI cannot be interrupted, so cancellation can discard the tagged identity ID or typed unconfirmed exception after native broadcast completion. Persist a pending claim containing the identity ID/index, submitted-key metadata, and re-drivable transition before broadcasting, then reconcile it across cancellation and restart; an equivalent mechanism must safely rebroadcast and treat NullifierAlreadySpent as evidence that the original claim executed.
source: ['codex']
| let Some(wid) = read_id32(env, &wallet_id, "walletId") else { | ||
| return ptr::null_mut(); | ||
| }; | ||
| let Some(sk) = read_id32(env, &one_time_sk, "oneTimeSk") else { | ||
| return ptr::null_mut(); |
There was a problem hiding this comment.
🔴 Blocking: Do not marshal the spending key through ordinary unsanitized buffers
oneTimeSk is bearer spend authority for a funded invitation, but the generic read_id32 leaves both its intermediate Vec<u8> and returned [u8; 32] unsanitized. The FFI and wallet layers create further plain-array copies. The generation direction similarly leaves native sk, combined out, and Kotlin's 75-byte source blob containing the spending key until their memory is reused or garbage-collected. This repository already uses read_key32_zeroizing for JNI private keys and explicitly scrubs invitation bearer scalars in identity/network/invitation.rs. Carry this key through Zeroizing buffers where possible and explicitly wipe all transient native and JVM arrays after copying out the one intentionally caller-owned key.
source: ['codex']
| pub unsafe extern "C" fn platform_wallet_generate_one_time_orchard_key( | ||
| out_sk_32: *mut u8, | ||
| out_address_43: *mut u8, | ||
| ) -> PlatformWalletFFIResult { | ||
| check_ptr!(out_sk_32); | ||
| check_ptr!(out_address_43); | ||
|
|
||
| let (sk, address) = generate_one_time_orchard_key(); | ||
| std::ptr::copy_nonoverlapping(sk.as_ptr(), out_sk_32, 32); | ||
| std::ptr::copy_nonoverlapping(address.as_ptr(), out_address_43, 43); | ||
| PlatformWalletFFIResult::ok() |
There was a problem hiding this comment.
🔴 Blocking: OS RNG failure aborts before the JNI panic guard can run
generate_one_time_orchard_key calls OsRng.fill_bytes, whose rand_core implementation panics when try_fill_bytes reports an entropy-source failure. Because that panic occurs inside this extern "C" export, it cannot unwind to the outer JNI guard; Rust aborts at the C ABI boundary first. An operating-system RNG failure can therefore terminate the Android process instead of producing a Java/native error. Change the generator to use try_fill_bytes and propagate a PlatformWalletFFIResult error, or place an internal panic fence inside this C export.
source: ['codex']
| let prepared = PreparedIncomingViewingKey::new(ivk); | ||
| let stream = sync_shielded_notes_stream(sdk, &prepared, 0, None); | ||
| futures::pin_mut!(stream); | ||
|
|
||
| let mut found: Vec<ShieldedNote> = Vec::new(); | ||
| let mut total: u64 = 0; | ||
| while let Some(batch) = stream.next().await { | ||
| let batch = batch.map_err(|e| PlatformWalletError::ShieldedSyncFailed(e.to_string()))?; | ||
| for dn in batch.decrypted { | ||
| let value = dn.note.value().inner(); | ||
| let nullifier = dn.note.nullifier(fvk).to_bytes(); | ||
| found.push(ShieldedNote { | ||
| position: dn.position, | ||
| cmx: dn.cmx, | ||
| nullifier, | ||
| block_height: batch.block_height, | ||
| is_spent: false, | ||
| value, | ||
| note_data: serialize_note(&dn.note), | ||
| }); | ||
| total = total.saturating_add(value); | ||
| } | ||
| // A one-time key holds exactly its funding — stop once it's covered. | ||
| if total >= stop_at_value { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Unfunded invitation keys force an unbounded full-history scan
Every syntactically valid invitation key starts at position 0 and keeps fetching, proof-verifying, and trial-decrypting chunks until enough value is found or the complete shielded history is exhausted. An attacker can generate valid but unfunded invitation keys, so the value-based early exit provides no bound for hostile input. The supplied birth-height hint is ignored, and cancellation of the Kotlin coroutine cannot interrupt the blocking JNI operation. Add an authenticated or otherwise safe starting position, a maximum resumable scan budget, or another preflight mechanism that prevents one invitation from forcing an unlimited genesis-to-tip scan.
source: ['codex']
Addresses reviewer thepastaclaw's blocking findings on PR dashpay#4204. Two of the four blockers are fixed here; the other two are structural and reported back for a decision rather than guessed (crypto/money path). Blocker dashpay#4 (FFI RNG abort) — shielded_send.rs / keys.rs: `generate_one_time_orchard_key` used `OsRng::fill_bytes`, which panics on an OS entropy-source failure. It is called from a `#[no_mangle] extern "C"` export, so that panic aborts the process across the C ABI before any JNI panic guard can convert it. Switch to `RngCore::try_fill_bytes`, return a typed `PlatformWalletError::ShieldedKeyDerivation`, and have the FFI export map it to `ErrorWalletOperation` instead of aborting. Test call sites and callers updated for the new `Result` return. Blocker dashpay#3 (bearer spend key hygiene) — funding.rs: `oneTimeSk` is bearer spend authority but was marshalled via the generic `read_id32`, leaving its intermediate JNI `Vec<u8>` and returned `[u8; 32]` unsanitized. Add a `read_key32_zeroizing` helper (mirroring `transactions::read_key32_zeroizing`): the returned key is `Zeroizing` and the intermediate JNI copy is scrubbed. `sk` derefs to `[u8; 32]`, so the downstream `sk.as_ptr()` FFI call is unchanged. NOT fixed here (reported for decision): Blocker dashpay#1 (affected-state wait): `wait_for_affected_state` does not exist in this head's SDK, and the pool-funded sibling still uses `wait_for_response` on this branch. The reviewer's fix is predicated on rebasing onto the v4.1-dev proof API (31c69cf); it must be done in lockstep for both Type-20 paths. Blocker dashpay#2 (persist claim recovery record): the redrive mechanism is keyed by SubwalletId + activity entry and driven by the per-subwallet sync loop. Claim notes belong to a foreign one-time key tracked in no subwallet, so a correct fix needs a new subwallet-less pending-claim record + reconciliation path, not a reuse of `arm_redrive_record`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed fixes for two of the four blockers in
The other two need more than a minimal edit — flagging for direction:
The unbounded genesis-to-tip scan on an unfunded claim is noted as a separate follow-up (a bound needs a chosen scan-budget so legitimate funded claims don't return empty). |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md (1)
674-675: 📐 Maintainability & Code Quality | 🔵 TrivialOptional: silence MD038 while preserving the prefix's trailing space.
markdownlint flags the space inside
`signer_error:key_unavailable: `. Since the trailing space is a meaningful part of the machine prefix, consider noting it in prose (e.g. "ends with a colon+space") instead of relying on the space inside the code span, so the lint passes without dropping the semantics.🤖 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 `@docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md` around lines 674 - 675, Update the documentation around the ProtocolError::Generic prefix to avoid placing a trailing space inside the inline code span, while preserving the prefix semantics by describing that it ends with a colon followed by a space in surrounding prose.Source: Linters/SAST tools
🤖 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 `@docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md`:
- Around line 57-69: Update the documentation text around the
`ProtocolError::Generic` prefix so the literal `signer_error:key_unavailable:`
code span contains no trailing space; describe or place the separator space
outside the backticks while preserving the prefix’s meaning.
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`:
- Around line 2673-2742: Update repairIdentityKeyDurably to coordinate the
durable repair with deleteWalletData and recordSigningKeyInvalidated through
withCallbackExclusion before invoking deriver.deriveAndStore(force = true),
preserving the existing breadcrumb, verification, persistence, and pending-state
behavior. Ensure native derivation calls are not executed while the callback
exclusion is held, following the established exclusion usage pattern.
In `@packages/rs-sdk-ffi/src/signer.rs`:
- Around line 179-188: Strip DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX from
signer error messages before surfacing them. Update both conversion paths—the
catch-all using error.to_string() and the ProtocolError implementation
formatting "DPP protocol error: {msg}"—to remove only this prefix while
preserving the existing error text and SigningKeyUnavailable mapping.
---
Nitpick comments:
In `@docs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.md`:
- Around line 674-675: Update the documentation around the
ProtocolError::Generic prefix to avoid placing a trailing space inside the
inline code span, while preserving the prefix semantics by describing that it
ends with a colon followed by a space in surrounding prose.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54334ff6-0af1-44db-88ae-6cec9e5e8d8b
📒 Files selected for processing (59)
docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.mddocs/sdk/KOTLIN_SWIFT_SHARED_PARITY_SPEC.mddocs/sdk/sdk-parity-manifest.jsonpackages/kotlin-sdk/KotlinExampleApp/app/src/main/java/org/dashfoundation/example/ui/wallet/WalletKeyHealthSheet.ktpackages/kotlin-sdk/PARITY_SUMMARY.mdpackages/kotlin-sdk/sdk/schemas/org.dashfoundation.dashsdk.persistence.DashDatabase/8.jsonpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabaseMigrationTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerInstrumentedTest.ktpackages/kotlin-sdk/sdk/src/androidTest/kotlin/org/dashfoundation/dashsdk/security/WalletStorageOwnershipTest.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/SignerNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/identity/IdentityRegistration.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/DashDatabase.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/PublicKeyDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/entities/PublicKeyEntity.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/IdentityKeyPrivateKeyDeriver.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicy.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyUnavailableException.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreManager.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/KeystoreSigner.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/tokens/Dashpay.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedPlatformWallet.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandlerTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeySecurityPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreKeyGenPolicyTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/KeystoreSignerCompletionCodeTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageUpgradeMatrixTest.ktpackages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/tokens/ManagedIdentityNotFoundTranslationTest.ktpackages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet/src/wallet/shielded/keys.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/rs-sdk-ffi/src/signer.rspackages/rs-sdk-ffi/src/test_utils.rspackages/rs-sdk-ffi/src/token/claim.rspackages/rs-sdk-ffi/src/token/config_update.rspackages/rs-sdk-ffi/src/token/destroy_frozen_funds.rspackages/rs-sdk-ffi/src/token/emergency_action.rspackages/rs-sdk-ffi/src/token/freeze.rspackages/rs-sdk-ffi/src/token/mint.rspackages/rs-sdk-ffi/src/token/purchase.rspackages/rs-sdk-ffi/src/token/set_price.rspackages/rs-sdk-ffi/src/token/transfer.rspackages/rs-sdk-ffi/src/token/unfreeze.rspackages/rs-unified-sdk-jni/src/funding.rspackages/rs-unified-sdk-jni/src/signer.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/KeychainSigner.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swiftpackages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swift
| - **`SigningKeyUnavailable` MESSAGE_MARKER fallback removal.** The signer's | ||
| "missing key" failure now travels as a typed completion code (rs-sdk-ffi | ||
| `DashSDKSignerErrorCode::SigningKeyUnavailable` → platform-wallet code 31 → | ||
| `DashSdkError.PlatformWallet.SigningKeyUnavailable`). The message-marker | ||
| sniff on the catch-all codes is retained ONLY for the #4191 merge-order | ||
| transition and for conversion paths that lose the machine prefix — NOT for | ||
| mixed old-native/new-Kotlin builds, which the completion JNI arity change | ||
| (3→4 args) makes unsupported outright; delete it (and `MESSAGE_MARKER`'s | ||
| matcher role) in the next minor release. Accepted residual until rs-dpp grows a typed variant: the | ||
| Rust-internal segment rides the `signer_error:key_unavailable: ` prefix | ||
| through `ProtocolError::Generic` (typed at both ABI edges, one Rust-owned | ||
| constant bridging the string segment). | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix MD038 lint: trailing space inside code span.
markdownlint flags the trailing space inside `signer_error:key_unavailable: ` at line 66. Rephrase to keep the literal value visible without a raw trailing space in the code span (e.g. move the trailing space out of the backticks or note it separately) to avoid a lint failure.
📝 Suggested fix
- Rust-internal segment rides the `signer_error:key_unavailable: ` prefix
+ Rust-internal segment rides the `signer_error:key_unavailable:` (trailing space) prefix📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **`SigningKeyUnavailable` MESSAGE_MARKER fallback removal.** The signer's | |
| "missing key" failure now travels as a typed completion code (rs-sdk-ffi | |
| `DashSDKSignerErrorCode::SigningKeyUnavailable` → platform-wallet code 31 → | |
| `DashSdkError.PlatformWallet.SigningKeyUnavailable`). The message-marker | |
| sniff on the catch-all codes is retained ONLY for the #4191 merge-order | |
| transition and for conversion paths that lose the machine prefix — NOT for | |
| mixed old-native/new-Kotlin builds, which the completion JNI arity change | |
| (3→4 args) makes unsupported outright; delete it (and `MESSAGE_MARKER`'s | |
| matcher role) in the next minor release. Accepted residual until rs-dpp grows a typed variant: the | |
| Rust-internal segment rides the `signer_error:key_unavailable: ` prefix | |
| through `ProtocolError::Generic` (typed at both ABI edges, one Rust-owned | |
| constant bridging the string segment). | |
| - **`SigningKeyUnavailable` MESSAGE_MARKER fallback removal.** The signer's | |
| "missing key" failure now travels as a typed completion code (rs-sdk-ffi | |
| `DashSDKSignerErrorCode::SigningKeyUnavailable` → platform-wallet code 31 → | |
| `DashSdkError.PlatformWallet.SigningKeyUnavailable`). The message-marker | |
| sniff on the catch-all codes is retained ONLY for the `#4191` merge-order | |
| transition and for conversion paths that lose the machine prefix — NOT for | |
| mixed old-native/new-Kotlin builds, which the completion JNI arity change | |
| (3→4 args) makes unsupported outright; delete it (and `MESSAGE_MARKER`'s | |
| matcher role) in the next minor release. Accepted residual until rs-dpp grows a typed variant: the | |
| Rust-internal segment rides the `signer_error:key_unavailable:` (trailing space) prefix | |
| through `ProtocolError::Generic` (typed at both ABI edges, one Rust-owned | |
| constant bridging the string segment). |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 66-66: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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 `@docs/dashpay/KOTLIN_MIGRATION_LEFTOVERS.md` around lines 57 - 69, Update the
documentation text around the `ProtocolError::Generic` prefix so the literal
`signer_error:key_unavailable:` code span contains no trailing space; describe
or place the separator space outside the backticks while preserving the prefix’s
meaning.
Source: Linters/SAST tools
| internal suspend fun repairIdentityKeyDurably( | ||
| walletId: ByteArray, | ||
| publicKeyData: ByteArray, | ||
| verifyRecoverable: suspend (pubkeyHex: String) -> Boolean, | ||
| persistDurableIdentifier: suspend (storageIdentifier: String) -> Unit = { storageIdentifier -> | ||
| database.publicKeyDao().getByPublicKeyData(publicKeyData).forEach { row -> | ||
| if (row.privateKeyKeychainIdentifier != storageIdentifier) { | ||
| database.publicKeyDao() | ||
| .update(row.copy(privateKeyKeychainIdentifier = storageIdentifier)) | ||
| } | ||
| } | ||
| }, | ||
| ): String? { | ||
| val pubkeyHex = publicKeyData.toHex() | ||
| val deriver = privateKeyDeriver | ||
| ?: throw DashSdkError.PlatformWallet.SigningKeyUnavailable( | ||
| "identity-key repair for $pubkeyHex has no private-key deriver wired; " + | ||
| "the key remains unusable and pending state is left intact", | ||
| ) | ||
|
|
||
| // BLOCKER 1: read the derivation indices from the persisted row, never | ||
| // from the caller. A row lacking breadcrumbs cannot be safely repaired | ||
| // (we would have to guess the slot), so fail WITHOUT clearing pending. | ||
| val breadcrumbs = database.publicKeyDao().getByPublicKeyData(publicKeyData) | ||
| .firstNotNullOfOrNull { row -> | ||
| val identityIndex = row.derivationIdentityIndex | ||
| val keyIndex = row.derivationKeyIndex | ||
| if (identityIndex != null && keyIndex != null) identityIndex to keyIndex else null | ||
| } ?: throw DashSdkError.PlatformWallet.SigningKeyUnavailable( | ||
| "cannot repair identity key $pubkeyHex: no derivation breadcrumbs are " + | ||
| "persisted for it (derivationIdentityIndex/derivationKeyIndex are " + | ||
| "null) — the correct slot is unknown; pending state left intact", | ||
| ) | ||
| val (identityIndex, keyIndex) = breadcrumbs | ||
|
|
||
| // force = true routes through WalletStorage.replacePrivateKey and, in | ||
| // the production deriver, derives the KEYPAIR and verifies the derived | ||
| // public key equals publicKeyData BEFORE any store — a mismatch throws | ||
| // IdentityKeyDerivationMismatchException here, so nothing below runs | ||
| // and pending is never cleared (BLOCKER 1). | ||
| val storageIdentifier = deriver.deriveAndStore( | ||
| walletId = walletId, | ||
| publicKeyData = publicKeyData, | ||
| identityIndex = identityIndex, | ||
| keyIndex = keyIndex, | ||
| force = true, | ||
| )?.identifier ?: return null | ||
|
|
||
| // Independent confirmation the stored blob actually decrypts. | ||
| if (!verifyRecoverable(pubkeyHex)) { | ||
| throw DashSdkError.PlatformWallet.SigningKeyUnavailable( | ||
| "identity-key repair stored a blob that does not decrypt for pubkey " + | ||
| "$pubkeyHex (slot $identityIndex/$keyIndex) — the key remains " + | ||
| "unusable; pending state left intact", | ||
| ) | ||
| } | ||
|
|
||
| // BLOCKER 3: the durable write fails CLOSED. Record the identifier on | ||
| // the Room rows so the restart reconstruction does not resurrect this | ||
| // key — but if that write throws, DO NOT clear pending. A swallowed | ||
| // failure that still cleared live state would resurrect the repair | ||
| // after restart while the session believed it was done. Let it | ||
| // propagate; pending stays intact and the repair is retryable. | ||
| persistDurableIdentifier(storageIdentifier) | ||
|
|
||
| // Durable write committed and blob verified — now it is safe to drop | ||
| // the pending-repair signal. | ||
| markIdentityKeyRepaired(pubkeyHex) | ||
| return storageIdentifier | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how repairIdentityKey / repairIdentityKeyDurably is invoked and whether it holds callbackExclusion.
fd -t f 'PlatformWalletManager.kt' packages/kotlin-sdk | xargs -I{} sh -c 'echo "== {} =="; rg -n -C5 "repairIdentityKey|repairIdentityKeyDurably|withCallbackExclusion|recordSigningKeyInvalidated" {}'Repository: dashpay/platform
Length of output: 4147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -t f 'PlatformWalletManager.kt|PlatformWalletPersistenceHandler.kt|WalletStorage.kt' packages/kotlin-sdk | sort
echo
echo "== PlatformWalletManager relevant sections =="
file=$(fd -t f 'PlatformWalletManager.kt' packages/kotlin-sdk | head -n 1)
wc -l "$file"
rg -n -C 20 "fun repairIdentityKey|teardownGate|withCallbackExclusion|withPrivateKeyExclusion|deleteWalletData|recordSigningKeyInvalidated|identityIdsForWallet|privateKeyDeriver" "$file"
echo
echo "== PlatformWalletPersistenceHandler relevant definitions and usages =="
handler=$(fd -t f 'PlatformWalletPersistenceHandler.kt' packages/kotlin-sdk | head -n 1)
wc -l "$handler"
rg -n -C 12 "fun withCallbackExclusion|withCallbackExclusion|lock|callbackExclusion|fun .*repairIdentityKey|repairIdentityKeyDurably|recordSigningKeyInvalidated|deriveAndStore|deleteWallet|deleteWalletData|identityIdsForWallet|publicKeyDao|walletStorage|withPrivateKeyExclusion" "$handler"Repository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== call graph-ish: all usages of PlatformWalletPersistenceHandler public methods across Kotlin SDK =="
rg -n -C 4 "PlatformWalletPersistenceHandler|repairIdentityKeyDurably|repairIdentityKey\(|withCallbackExclusion\(|withCallbackExclusion|deleteWalletData|identityIdsForWallet|recordSigningKeyInvalidated" packages/kotlin-sdk --glob '*.kt'
echo
echo "== exact lines range persistence handler =="
handler=$(fd -t f 'PlatformWalletPersistenceHandler.kt' packages/kotlin-sdk | head -n 1)
sed -n '2620,2775p' "$handler" | cat -n -v +s
sed -n '880,1060p' "$handler" | cat -n -v +s
sed -n '1320,1500p' "$handler" | cat -n -v +s
sed -n '1600,1800p' "$handler" | cat -n -v +s
echo
echo "== exact manager repair / delete sections =="
manager=$(fd -t f 'PlatformWalletManager.kt' packages/kotlin-sdk | head -n 1)
sed -n '470,525p' "$manager" | cat -n -v +s
sed -n '880,965p' "$manager" | cat -n -v +sRepository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
handler=$(fd -t f 'PlatformWalletPersistenceHandler.kt' packages/kotlin-sdk | head -n 1)
manager=$(fd -t f 'PlatformWalletManager.kt' packages/kotlin-sdk | head -n 1)
wallet=$(fd -t f 'WalletStorage.kt' packages/kotlin-sdk | head -n 1)
echo "== persistence handler lock/section definitions =="
sed -n '220,340p' "$handler" | cat -n -v +s
echo
echo "== persistence handler deleteWalletDataLocked/wrapped deleteWalletData =="
rg -n -C 12 "deleteWalletDataLocked|fun deleteWalletData|withCallbackExclusion\\(" "$handler"
echo
echo "== persistence handler identity-key persistence APIs =="
rg -n -C 10 "recordSigningKeyInvalidated|pendingIdentityKeys|deriveAndStore|IdentityChangeSet|Identity.*Private|PrivateKeyDeriver|public_key" "$handler" | head -n 240
echo
echo "== manager repairIdentityKey =="
sed -n '487,522p' "$manager" | cat -n -v +s
echo
echo "== wallet storage locking definitions =="
sed -n '1,260p' "$wallet" | cat -n -v +sRepository: dashpay/platform
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
handler=$(fd -t f 'PlatformWalletPersistenceHandler.kt' packages/kotlin-sdk | head -n 1)
manager=$(fd -t f 'PlatformWalletManager.kt' packages/kotlin-sdk | head -n 1)
wallet=$(fd -t f 'WalletStorage.kt' packages/kotlin-sdk | head -n 1)
echo "== persistence handler lock/section definitions =="
sed -n '220,340p' "$handler"
echo
echo "== persistence handler deleteWalletDataLocked/wrapped deleteWalletData =="
rg -n -C 12 "deleteWalletDataLocked|fun deleteWalletData|withCallbackExclusion\\(" "$handler"
echo
echo "== persistence handler identity-key persistence APIs =="
rg -n -C 10 "recordSigningKeyInvalidated|pendingIdentityKeys|deriveAndStore|IdentityChangeSet|Identity.*Private|PrivateKeyDeriver|public_key" "$handler" | head -n 240
echo
echo "== manager repairIdentityKey =="
sed -n '487,522p' "$manager"
echo
echo "== wallet storage locking definitions =="
sed -n '1,260p' "$wallet"Repository: dashpay/platform
Length of output: 23034
Run identity-key repair under withCallbackExclusion before deriveAndStore(force = true).
repairIdentityKey only gates through teardownGate, while deleteWalletData uses withCallbackExclusion before cascading and recordSigningKeyInvalidated holds that exclusion on entry. A concurrent repair can derive/store an identity scalar while a deletion/persist sequence is updating/wiping the same wallet’s Room rows, leaving usable ciphertext behind the deletion or racing a row update. Move the durable derive/store path under withCallbackExclusion, avoiding native calls while the exclusion is held.
🤖 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/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/PlatformWalletPersistenceHandler.kt`
around lines 2673 - 2742, Update repairIdentityKeyDurably to coordinate the
durable repair with deleteWalletData and recordSigningKeyInvalidated through
withCallbackExclusion before invoking deriver.deriveAndStore(force = true),
preserving the existing breadcrumb, verification, persistence, and pending-state
behavior. Ensure native derivation calls are not executed while the callback
exclusion is held, following the established exclusion usage pattern.
| /// Stable machine prefix that carries the | ||
| /// [`DashSDKSignerErrorCode::SigningKeyUnavailable`] discriminator through | ||
| /// the `ProtocolError::Generic` string segment between this crate's signer | ||
| /// completion and `platform-wallet-ffi`'s error conversion (which maps it to | ||
| /// `PlatformWalletFFIResultCode::ErrorSigningKeyUnavailable`). The | ||
| /// discriminator is TYPED at both ABI edges (an `i32` across C, a result | ||
| /// code across the Kotlin/Swift FFI); this Rust-owned constant is the one | ||
| /// bridge across the `ProtocolError` segment — never sniff human-readable | ||
| /// message text. | ||
| pub const DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX: &str = "signer_error:key_unavailable: "; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B3 -A15 'DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX' packages/rs-platform-wallet-ffi/src/error.rsRepository: dashpay/platform
Length of output: 4571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate error.rs sections =="
sed -n '340,390p' packages/rs-platform-wallet-ffi/src/error.rs
echo
sed -n '440,485p' packages/rs-platform-wallet-ffi/src/error.rs
echo
sed -n '485,505p' packages/rs-platform-wallet-ffi/src/error.rs
echo
echo "== references to SigningKeyUnavailable / DASH_SDK prefix =="
rg -n 'ErrorSigningKeyUnavailable|DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX|signer_error:key_unavailable' packages/rs-sdk-ffi packages/rs-platform-wallet-ffiRepository: dashpay/platform
Length of output: 6908
Strip the signer prefix before surfacing the error message.
ErrorSigningKeyUnavailable is restored correctly from rs_sdk_ffi::DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX, but both conversion paths still pass the message through unchanged: error.to_string() in the catch-all and format!("DPP protocol error: {msg}") in the ProtocolError impl. This leaves signer_error:key_unavailable: in the Kotlin/Swift error message text next to SigningKeyUnavailable.
🤖 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/rs-sdk-ffi/src/signer.rs` around lines 179 - 188, Strip
DASH_SDK_SIGNER_ERR_KEY_UNAVAILABLE_PREFIX from signer error messages before
surfacing them. Update both conversion paths—the catch-all using
error.to_string() and the ProtocolError implementation formatting "DPP protocol
error: {msg}"—to remove only this prefix while preserving the existing error
text and SigningKeyUnavailable mapping.
|
Thanks for splitting the two structural items out rather than guessing on a fund-safety path. Direction from my side:
The RNG/extern-C abort fix looks correct on inspection. Current head |
What
Adds the one-time Orchard key shielded-invite API to the Kotlin SDK. Client-side only — no L2 protocol / consensus changes (nothing under
rs-dpp,rs-drive,dapi).generateOneTimeOrchardKey()/orchardAddressFromSpendingKey()+ theOneTimeOrchardKeytype: generate a one-time Orchard spending key and the raw address the inviter funds a note to.shieldedIdentityCreateFromOneTimeKey(...): a claimer, handed the one-time spending key, spends the funded note to create/top-up a shielded identity.Backing Rust:
rs-platform-wallet(shielded/keys.rs,operations.rs,sync.rs,platform_wallet.rs),rs-platform-wallet-ffi(shielded_send.rs),rs-unified-sdk-jni(funding.rs).The claim side consumes
decode_registration_pubkeys_blob+IdentityPubkeyCodec, both introduced by #4183. This branch is stacked on #4183, so until #4183 merges the diff below also contains #4183's changes. It will retarget to a clean diff once #4183 lands. Net-new files to review here:packages/rs-platform-wallet/src/wallet/shielded/keys.rspackages/rs-platform-wallet/src/wallet/shielded/operations.rspackages/rs-platform-wallet/src/wallet/shielded/sync.rspackages/rs-platform-wallet/src/wallet/shielded/mod.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/rs-platform-wallet-ffi/src/shielded_send.rspackages/rs-unified-sdk-jni/src/funding.rspackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/FundingNative.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.ktpackages/rs-platform-wallet/Cargo.toml(optionalranddep for theshieldedfeature)Security note — identity key roles
The claim path decodes registration pubkeys through base's
decode_registration_pubkeys_blob/row.to_ffi(), so key roles (purpose / security level) are caller-stamped (base's uniform registration convention) rather than derived in Rust. The role mapping is unchanged: key_id0 → AUTH/MASTER,1 → AUTH/CRITICAL,2 → AUTH/HIGH,3 → TRANSFER/CRITICAL. The reconciled JNI return also preserves the identity id on the unconfirmed-broadcast path.Validation
cargo test -p platform-wallet --features shielded— 624 lib tests + 3 new claim tests pass; inviter key-roundtrip tests pass.cargo build -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --features shielded— clean../gradlew :sdk:assemble— BUILD SUCCESSFUL (compileDebug/ReleaseKotlin).Consumer follow-up (tracked separately, not in this PR)
The Android wallet's
SdkShieldedUsernameCreation/SdkShieldedInviteCreationstill call the claim API withList<IdentityKeyPreview>; they need adapting toList<IdentityPubkey>(stamping the roles above) before the full wallet builds against this SDK.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation