Skip to content

feat(platform-wallet): derive application encryption keys with the DIP-13 rotation level - #5649

Merged
PastaPastaPasta merged 2 commits into
v5.1-devfrom
feat/connect-key-encryption-rotation
Oct 29, 2026
Merged

PastaPastaPasta merged 2 commits into
v5.1-devfrom
feat/connect-key-encryption-rotation

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Problem

The platform wallet SDK derives DashPay Connect app encryption keys at the 8-level DIP-13 path m/9'/coin'/5'/7'/0'/identityId'/contractId'/purpose'. Once mainnet keys are registered there, the path can't change, and it has no level for rotating a pair. DIP-13 (dashpay/dips#191) and the App Connect DIP (dashpay/dips#181) now end the path with a hardened 31-bit rotation' level, with 0' as the first pair:

m/9'/coin'/5'/7'/0'/identityId'/contractId'/purpose'/rotation'

Keys already registered on testnet and devnet use the old 8-level path, so wallets still need a way to derive them to recognise those keys.

Fix

  • platform-wallet: ConnectKey::AppEncryption gains a rotation: u32 field, and new keys are derived with this variant. The new ConnectKey::AppEncryptionLegacy variant derives the old 8-level key, so the old path is only used when a caller asks for it. ConnectKey::derivation_path now returns a Result and rejects rotations of 2^31 or more with InvalidParameter instead of wrapping them onto another key.
  • platform-wallet also owns the mapping from raw values to a key. ConnectKey::from_parts(sub_feature, identity_id, leaf, purpose, rotation) accepts only the valid DIP-13 combinations, and ConnectKey::legacy_app_encryption_from_parts(identity_id, contract_id, purpose) builds the old layout. The FFI only converts C values and calls these.
  • FFI: dash_sdk_derive_connect_key_with_resolver is replaced by dash_sdk_derive_connect_key_v2_with_resolver, which takes a new rotation: u32 argument after purpose. The old symbol is removed, not kept alongside, so a caller built against the old signature fails to link instead of passing its out pointer where rotation now sits. Rotation is required for sub-feature 7 and must be 0 for the session key (sub-feature 6). The new sibling dash_sdk_derive_legacy_app_encryption_key_with_resolver(network, wallet_id, resolver, identity_id, contract_id, purpose, out_key) derives the old layout. No argument value has a special meaning such as "legacy". Both functions check the key before asking the resolver for the seed.
  • JNI and Kotlin follow the same shape:
    • deriveConnectKeyWithResolver is renamed to deriveConnectKeyV2WithResolver(..., purpose, rotation), so a stale Kotlin class or native library fails with UnsatisfiedLinkError instead of a mismatched signature. There is also a new deriveLegacyAppEncryptionKeyWithResolver.
    • PlatformWalletManager.deriveConnectKey is split into a session overload and an app-encryption overload, which requires purpose and rotation.
    • New deriveLegacyAppEncryptionKey.

Temporary local append

The pinned rust-dashcore rev only builds the 8-level path, so platform-wallet adds ChildNumber::Hardened { index: rotation } itself. dashpay/rust-dashcore#1250 (branch feat/key-wallet-encryption-rotation) adds the level to key-wallet. When platform moves to a rust-dashcore rev that includes it, two calls replace the local code:

  • DerivationPath::application_encryption_path(network, identity_id, contract_id, purpose, rotation) replaces the append.
  • DerivationPath::application_encryption_path_without_rotation replaces the legacy arm.

The pinned vectors below ensure that switch changes no keys. This PR doesn't bump rust-dashcore.

Migration (Swift / Kotlin / C callers)

  • Swift: deriveConnectKey(subFeature: .appEncryption, ..., purpose:) now requires a rotation: UInt32 argument, with no default. Existing app-encryption call sites therefore fail to compile until they pass a rotation. Pass 0 for an identity's first pair for a contract.
  • Swift, legacy keys: derive keys registered before the change with deriveLegacyAppEncryptionKey(identityId:contractId:purpose:network:storage:).
  • Swift, session keys: deriveConnectKey(subFeature: .sessionAuthentication, identityId:leaf:network:) calls don't change.
  • Kotlin: same as Swift. deriveConnectKey(walletId, APP_ENCRYPTION, identityId, leaf, purpose, rotation) and deriveLegacyAppEncryptionKey(walletId, identityId, contractId, purpose).
  • C: switch from dash_sdk_derive_connect_key_with_resolver to dash_sdk_derive_connect_key_v2_with_resolver and pass rotation (0 for the session key). Call dash_sdk_derive_legacy_app_encryption_key_with_resolver to get the old layout. The header is generated by cbindgen.
  • Kotlin native callers: switch from IdentityNative.deriveConnectKeyWithResolver to deriveConnectKeyV2WithResolver.

How tested

Vectors from BIP-39 abandon ×11 about, empty passphrase, mainnet. 0x35… is 32 bytes of 0x35, 0x6b… is 32 bytes of 0x6b, asc is bytes 00..1f and desc is bytes ff..e0. All are pinned in connect_keypairs_match_the_pinned_vectors:

path suffix public key
0x35…'/0x6b…'/1'/0' 031a882c119e2b1f8e0349b444d877afa2599efeef49a26930fd25518cb6e9d57a
0x35…'/0x6b…'/1'/1' 03d0f6f0f48d968b9554d8e7fe467d7135bacf29d5c4994ca593bd10ebeed696f6
asc'/desc'/1'/0' 02dfed57ddaaea8c0758e8da44513520038eb8d6b0b92109953c14d6ab1087259b
asc'/desc'/2'/0' 020bcde800f41ad1f924477a9502f111445224cc9ab70114ae4e5099ba02bdd034
asc'/desc'/2'/1' 021b97869c21933673076a938337c28848c8bf8094bbe6883e3a4acbf33c639072
legacy 0x35…'/0x6b…'/1' 03e989de1b62f137231cc06659c810c17100becd1f5e23d5710faa7602769fe434
legacy asc'/desc'/1' 03c4e462229f176595ec17e28e9096d3cfbc42496fb8ed4d48f8f58355d6105aa9
legacy asc'/desc'/2' 031fd0a3cfc01bebb89d43c2e12a6321c37ebdacb62c09495899e5b65a24fa5c99

Other tests check:

  • ConnectKey::from_parts and legacy_app_encryption_from_parts build each valid combination and reject the invalid ones;
  • the rotated path is exactly the legacy path plus a hardened rotation' level;
  • rotations of 2^31 or more are rejected;
  • every key kind, purpose, rotation and layout derives a distinct key;
  • the FFI entry points derive the pinned vectors, and bad purpose or rotation combinations are rejected before the resolver is used.

The existing Swift ConnectKeyDerivationTests are updated, with no tests removed, and cover the new overloads, the legacy derive and the rejection cases.

  • cargo fmt --all
  • cargo clippy -p platform-wallet -p platform-wallet-ffi -p rs-unified-sdk-jni --all-features --all-targets -- -D warnings: clean
  • cargo test -p platform-wallet --lib identity_handle::tests: 9 passed
  • cargo test -p platform-wallet-ffi --lib derive_connect_key: 9 passed
  • cargo test -p rs-unified-sdk-jni --lib connect: 1 passed
  • build_ios.sh --target mac --profile dev followed by swift test --filter ConnectKeyDerivationTests: 4 passed
  • The Kotlin SDK was not compiled locally. Its JNI signatures match the Rust exports.

🤖 Generated with Claude Code

PR Hygiene · c11751f

  • Bots — thepastaclaw ✓
  • Self-review — post /self-reviewed
  • Reviewer requests paused — your 5 review slots are occupied; this PR is excluded from reviewers' queues. Required approvals still count without a slot.
  • Build green
  • Approvals
    • kotlin-sdk (packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.kt, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt) — HashEngineering
    • rs-platform-wallet-ffi (packages/rs-platform-wallet-ffi/src/derive_connect_key.rs) — HashEngineering or ZocoLini or llbartekll or romchornyi
    • rs-platform-wallet (packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs) — HashEngineering or ZocoLini or llbartekll or romchornyi
    • files with no dedicated owner (packages/rs-unified-sdk-jni/src/identity.rs) — QuantumExplorer or shumkov
    • swift-sdk (packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift, packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ConnectKeyDerivationTests.swift) — llbartekll or romchornyi

When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.

…P-13 rotation level

DIP-13's application encryption path now ends with a hardened 31-bit
rotation level: m/9'/coin'/5'/7'/0'/identityId'/contractId'/purpose'/rotation'.

- ConnectKey::AppEncryption gains `rotation`; the pre-rotation 8-level
  layout is the explicit ConnectKey::AppEncryptionLegacy, kept so wallets
  recognise testnet/devnet keys registered before the change.
  derivation_path returns a Result and refuses rotations >= 2^31.
- The rotation level is appended locally because the pinned rust-dashcore
  only builds the 8-level path; it is replaced by key-wallet's
  application_encryption_path(..., rotation) from dashpay/rust-dashcore#1250
  once platform bumps, and the pinned vectors keep that swap a no-op.
- FFI: dash_sdk_derive_connect_key_with_resolver takes `rotation`; new
  dash_sdk_derive_legacy_app_encryption_key_with_resolver.
- Swift: the app-encryption deriveConnectKey overload requires
  `rotation: UInt32`; deriveLegacyAppEncryptionKey derives the old layout.
  Session-key calls are unchanged.
- JNI/Kotlin mirror the same shape.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v5.1.0 milestone Oct 28, 2026
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 15 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 42f59e6 and c11751f.

Files selected for processing: 6
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
  • packages/rs-platform-wallet-ffi/src/derive_connect_key.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
  • packages/rs-unified-sdk-jni/src/identity.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between a828103 and 42f59e6.

Files selected for processing: 7
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.kt
  • packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
  • packages/rs-platform-wallet-ffi/src/derive_connect_key.rs
  • packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
  • packages/rs-unified-sdk-jni/src/identity.rs
  • packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift
  • packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ConnectKeyDerivationTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Connect key derivation now supports rotation-aware app-encryption keys and a separate legacy path for pre-rotation keys. The Rust wallet, FFI, JNI, Kotlin, and Swift layers expose these derivations and validate supported parameters.

Changes: Connect key derivation

Layer / File(s) Summary
Rotated and legacy derivation paths
packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs
AppEncryption paths now include a validated rotation level. AppEncryptionLegacy retains the prior path. Tests cover path structure, limits, vectors, and distinct derived keys.
FFI APIs and validation
packages/rs-platform-wallet-ffi/src/derive_connect_key.rs
The current API accepts rotation and validates sub-feature, purpose, and rotation combinations. A legacy API derives keys without rotation. Tests cover vectors and invalid inputs.
Kotlin and JNI APIs
packages/rs-unified-sdk-jni/src/identity.rs, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/ffi/IdentityNative.kt, packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/PlatformWalletManager.kt
The APIs pass rotation for app-encryption derivation and provide a separate legacy method. JNI result handling frees FFI output and clears private-key copies on the described paths.
Swift API and tests
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift, packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ConnectKeyDerivationTests.swift
The wallet exposes session-authentication, rotation-aware app-encryption, and legacy derivation methods. Tests check vectors, distinct keys, and invalid parameters.

Priority: Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant SDK
  participant FFI as dash_sdk_derive_connect_key_with_resolver
  participant KeyPath as ConnectKey.derivation_path
  participant Resolver as Mnemonic resolver
  participant Master as Master key derivation
  SDK->>FFI: Request key derivation
  FFI->>KeyPath: Validate derivation path
  FFI->>Resolver: Resolve seed
  Resolver-->>FFI: Return seed
  FFI->>Master: Derive key from path
  Master-->>SDK: Return derived key
Loading

Suggested reviewers: quantumexplorer, lklimek, romchornyi

Merge Risk: Minimal · previous review 42f59

The rotation-aware and legacy derivation APIs appear ready to merge after normal checks. The Kotlin SDK has not been compiled locally, but no broken caller was found.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the main change: application encryption keys are now derived with the DIP-13 rotation level. The changeset matches this. ConnectKey::AppEncryption gains a rotation field, the FFI, JNI,…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: application encryption keys are now derived with the DIP-13 rotation level. The changeset matches this. ConnectKey::AppEncryption gains a rotation field, the FFI, JNI, Kotlin, and Swift APIs accept rotation, and a separate legacy derivation path is added for pre-rotation keys. The title is specific and uses the correct domain term. It is 88 characters, which is longer than ideal for a short title, but this does not affect the assessment.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 14 functions across 7 files.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added the waiting-bots Waiting for the review bots to report on this head label Oct 28, 2026
@thepastaclaw

thepastaclaw commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Final review complete — no blockers (commit c11751f) · triage: critical

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Phase 1 + Phase 2

The rotation derivation logic and legacy path are otherwise coherent, and the Rust workspace and Kotlin checks passed. However, the PR changes the calling convention of an existing exported C function and JNI method without preserving the old symbol, which can cause undefined behavior for previously compiled consumers; the new tuple-dispatch rules also belong in platform-wallet rather than the FFI layer. The Swift build check remains in progress.

🔴 2 blocking | 🟡 2 suggestion(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This is a large, cross-language change to cryptographic key derivation and legacy-key compatibility in platform-wallet, including Rust, FFI, JNI, Swift, and Kotlin surfaces.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer
  • Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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-ffi/src/derive_connect_key.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/derive_connect_key.rs:126-136: Preserve the existing connect-key ABI when adding rotation
  The existing exported symbol `dash_sdk_derive_connect_key_with_resolver` keeps its name but inserts `rotation` before `out_key`. The previous C ABI accepted `out_key` as the eighth parameter; an older compiled caller will therefore pass its output pointer where the new function expects `rotation`, while the new function reads an unprovided ninth argument as `out_key`. The resulting invalid write can crash or corrupt memory before an error is returned. The same incompatibility exists for the unchanged JNI symbol when an older compiled Kotlin class is paired with the new native library. The migration instructions only help callers that are recompiled and do not protect already-built native consumers. Retain the old symbol and behavior, and add a versioned or differently named rotation-aware C/JNI entry point, or make an explicit ABI-major-version change for the shipped native library.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/derive_connect_key.rs:143-165: Keep Connect-key dispatch and combination validation in platform-wallet
  This FFI function interprets the raw `(sub_feature, purpose, rotation)` tuple, selects `ConnectKey::SessionAuthentication` versus `ConnectKey::AppEncryption`, and enforces which purpose and rotation combinations are valid. The new legacy entry point also directly selects `ConnectKey::AppEncryptionLegacy`. Those are wallet-domain decisions, not argument marshalling. Put the conversion and validation behind a typed constructor or request conversion owned by `rs-platform-wallet`, and have the FFI only convert C values and invoke that owner-level API. Otherwise native callers and future bindings can bypass or duplicate the canonical Connect-key invariants.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/derive_connect_key.rs:228-233: Document the safety contracts of the private unsafe helpers
  `read_leaf` dereferences a raw pointer based on a precondition enforced only by each caller's preceding `check_ptr!`, and `derive_into` similarly relies on its callers to validate `out_key`. These assumptions are currently satisfied, but the private helpers have no local `# Safety` contract, making a future caller's omission an immediate null-dereference or over-read risk. Add `# Safety` documentation describing the required pointer validity and readable lengths to both unsafe helpers.

In `packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/identity_handle.rs:182-208: Use key-wallet's canonical rotation-aware path builders when available
  The current code locally appends a hardened child to key-wallet's old eight-level application-encryption path and uses that old builder for the explicit legacy variant. This is behaviorally correct for the pinned dependency, but it duplicates path-shape and hardened-index ownership in platform-wallet. The linked rust-dashcore#1250 is still open and adds the canonical rotation-aware and without-rotation builders. Once that dependency is available, replace these local arms with those key-wallet APIs so the path definition cannot drift between crates.

@github-actions

Copy link
Copy Markdown
Contributor

Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 28, 2026
…and move key selection into platform-wallet

- Rename dash_sdk_derive_connect_key_with_resolver to
  dash_sdk_derive_connect_key_v2_with_resolver (and the JNI/Kotlin
  deriveConnectKeyWithResolver to deriveConnectKeyV2WithResolver) so a
  caller compiled against the pre-rotation signature fails to link instead
  of passing its out pointer where `rotation` now sits.
- ConnectKey::from_parts and ConnectKey::legacy_app_encryption_from_parts
  in platform-wallet now map raw (sub_feature, purpose, rotation) levels to
  a key and refuse invalid combinations; the FFI only converts C values and
  calls them.
- Document the safety requirements of the FFI helpers read_leaf and
  derive_into.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Your move: thepastaclaw left review threads unresolved; resolve them.
Full checklist in the description.

@github-actions

Copy link
Copy Markdown
Contributor

Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.

@github-actions github-actions Bot added waiting-bots Waiting for the review bots to report on this head and removed waiting-self-review Waiting for the author to post /self-reviewed labels Oct 28, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Final validation — Phase 1 + Phase 2

The reviewed head correctly centralizes Connect-key selection and validation in platform-wallet, preserves the legacy derivation path, and uses versioned C/JNI symbols to prevent ABI mismatches. The Rust, FFI, JNI, Swift, and Kotlin implementations agree on rotation bounds, path layout, error handling, and secret cleanup; no in-scope findings remain. Rust workspace tests, Kotlin tests, and the Swift schema check were still running, with no failed code checks reported.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 10: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — This large cross-language change modifies cryptographic key derivation, rotation-path handling, and FFI/JNI/Swift/Kotlin boundaries in platform-wallet.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — architecture-layering (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — ffi-engineer (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer
  • Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

@github-actions

Copy link
Copy Markdown
Contributor

Bots are done — your move: post /self-reviewed.
Full checklist in the description.

@github-actions github-actions Bot added waiting-self-review Waiting for the author to post /self-reviewed and removed waiting-bots Waiting for the review bots to report on this head labels Oct 28, 2026
@PastaPastaPasta
PastaPastaPasta merged commit 03945d3 into v5.1-dev Oct 29, 2026
43 of 45 checks passed
@PastaPastaPasta
PastaPastaPasta deleted the feat/connect-key-encryption-rotation branch October 29, 2026 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-self-review Waiting for the author to post /self-reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants