Repository navigation
feat(dashpay): select App Connect encryption keys by the DIP rotation rules - #1258
PastaPastaPasta wants to merge 2 commits into
Conversation
dashpay/platform#5649 (03945d3a on v5.1-dev) adds the DIP-13 rotation' level to App Connect encryption keys: deriveConnectKey for .appEncryption now requires a rotation, and deriveLegacyAppEncryptionKey derives the earlier 8-level path. The path becomes permanent with the first mainnet key, so the wallet has to derive it before mainnet ships. The only compile break in the 55 commits since 063589bf is the removed rotation-less .appEncryption overload; the call site passes rotation 0 here. Until the next commit selects keys by the DIP rules, this derives only rotation 0', so testnet keys at the old path read as another wallet's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… rules The wallet derived one encryption key per binding and compared it with Platform's current key. With the rotation level, the identity's current key may be any rotation this wallet registered, or on testnet a key at the earlier 8-level path that today's builds registered. And a disabled current key of another wallet's read as absent under MultipleReferenceToLatest, so the login registered a new key beside it, where DIP rule 2 says to leave the slot out. Per granted binding the wallet now derives rotation 0', then 1', 2', ... while each is on the identity in any state (stopping at the first that is not), plus the earlier path on testnet and devnet only. A live current key among them is sent and nothing is registered; any other live current key, or a disabled one, leaves the slot out; only an identity with no key for the contract and purpose gets rotation 0' registered. The keys are derived after the identity's keys are fetched, and the candidates no slot carries are wiped before the update and the publish. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
|
✅ Final review complete — no blockers (commit 41b4f17) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The rotation-path selection, legacy compatibility, disabled-current-key handling, cleanup, and target membership are consistent with the stated DIP rules. One non-blocking performance issue remains: approval derives every contiguous rotation candidate synchronously on the main actor, and the simulator test check is still in progress.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large, intricate change to App Connect encryption-key derivation, rotation, private-key selection, and wiping in AppConnectV2Login.swift and PlatformDashConnectDataSource.swift, directly changing cryptographic key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-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; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift`:
- [SUGGESTION] DashWallet/Sources/Models/DashConnect/PlatformDashConnectDataSource.swift:1452-1456: Avoid deriving every rotation candidate synchronously on the main actor
`derivableEncryptionKeys` derives rotation 0 and then every contiguous rotation until the first gap, capped only by the number of identity keys for that purpose. This call is wrapped in one `MainActor.run` block, and each iteration performs a synchronous SDK/FFI derivation. The platform permits up to 15,000 identity public keys, so an identity with a long contiguous rotation history can make approval perform thousands of derivations while blocking the main actor, causing the App Connect approval UI to appear hung. Provide a bulk/range derivation API in SwiftDashSDK, or another SDK-supported off-main execution path that preserves the required private-key lifetime and wiping guarantees.
Problem
App Connect encryption keys are derived at
m/9'/coin'/5'/7'/0'/identityId'/contractId'/purpose'. The DIP adds arotation'level afterpurpose'. That path becomes permanent once the first mainnet key is registered, so it has to be right in the first mainnet release. Testnet keys that today's builds registered at the old 8-level path have to keep working.The wallet also derived just one key and compared it with Platform's current key. That broke DIP rule 2: under
MultipleReferenceToLatest, a disabled current key from another wallet read as.absent, and the login registered a new key next to it. The DIP says to leave the slot out and register nothing.Fix
For each granted encryption binding, the wallet follows the DIP "Encryption Keys" rules:
AppConnect.derivableEncryptionKeys): first rotation0'at the new 9-level path, then1',2', and so on. The search stops at the first rotation whose key is not on the identity in any state, and that key is wiped. The identity's key count caps the search. On testnet and devnet only, the wallet also derives the legacy 8-level key withderiveLegacyAppEncryptionKey. Mainnet never had keys at the legacy path, so the wallet doesn't derive it there.uniqueandlatest. Underlatest, a confirming re-read is still required, as before, because a newer live key may not show up yet on a node that is behind.0'at the new path and sends it.encryptionKeyStatenow takes every derivable public key, not just one. A newencryptionSlot(for:…)maps a settled state to send / register / omit and picks which derived private key to send. Encryption keys are now derived after the identity's keys are fetched, because the rotation search needs them. Every derived private key is wiped when the approval finishes.Stacked on #1257
Base is
feat/app-connect-dashpay-carrier(#1257). Merge that first.Platform bump
.platform-refmoves from063589bfto03945d3a(dashpay/platform#5649, squash-merged intov5.1-dev), which adds the rotation level toderiveConnectKeyand addsderiveLegacyAppEncryptionKey. The only compile break across the 55 commits is the call site that used the removedderiveConnectKey(subFeature: .appEncryption, …, purpose:, network:)overload, and this change replaces it. The other Swift SDK changes in that range are additive (SDK.protocolVersionInfo(),ProtocolVersionInfo) or internal (getEpochsInfokeeps its signature). The range also brings in the Rust side of several!platform changes (dashpay/platform#4899, dashpay/platform#5533, dashpay/platform#5538, dashpay/platform#5561, dashpay/platform#5578, dashpay/platform#5579, dashpay/platform#5584, dashpay/platform#5632). Most of them are gated on protocol version 15. This PR depends on none of them, and only the App Connect suites were run against the bump.The first commit pins the platform and passes
rotation: 0at the old call site, so it builds on its own. The second commit applies the rules.Out of scope
How tested
On the Mac Studio, headless. I built the
DashSDKFFI.xcframeworksimulator slice from platform03945d3a, then ranxcodebuild test(schemedashwallet-dashpay, Debug, iOS Simulator, arm64) with-only-testingon the App Connect suites: DashConnectUriTests, AppConnectV2LoginTests, PlatformDashConnectDataSourceTests, DashConnectStoreTests, LoginKeyDerivationTests and the new AppConnectEncryptionRotationTests.Result:
** TEST SUCCEEDED **. 186 tests ran with 0 failures:New tests in
AppConnectEncryptionRotationTests:PlatformDashConnectDataSource.derivableEncryptionKeys→ManagedPlatformWallet): ENCRYPTION rotations0'/1'for0x35…/0x6b…, and DECRYPTION rotations0'/1'forasc/desc.Two existing
AppConnectV2LoginTestsassertions expected the old rule-2 behaviour (.absent/.absentIfSettledbehind a disabled current key). They now expect.notThisWallets/.notThisWalletsIfSettled.🤖 Generated with Claude Code