feat(platform-wallet): surface provider operator/node keys and reveal address private keys#4072
Conversation
… address private keys Provider Operator (BLS) and Provider Platform Node (EdDSA) accounts were created by key-wallet at wallet creation but excluded from all_accounts(), so they never persisted, restored, or displayed. Provider account xpubs now ride a dedicated changeset vec, round-trip through the FFI account specs, and rebuild watch-only on restore. Because SLIP-10 Ed25519 is hardened-only (no public derivation, so no gap-limit extension is ever possible), register_wallet pre-derives 20 platform-node keys (pubkey + hash160 node id) while the seed is available and persists them; the Node Keys screen lists them with no keychain prompt. BLS operator keys derive on demand from the account xpub. Also adds a confirm-gated "View Private Key" reveal (hex + WIF) on the Core address detail view, and per-index reveals (hex) for both provider key accounts, all derived in Rust via the mnemonic-resolver pattern with zeroized secrets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds core-address and provider-key derivation across Rust, FFI, Swift APIs, persistence, and example-app views. Provider account registrations now persist extended public keys and pre-derived platform-node keys, while key reveal flows expose hex, WIF, node IDs, and indexed provider keys. ChangesWallet key discovery and persistence
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AccountDetailView
participant ManagedPlatformWallet
participant RustFFI
participant MnemonicResolver
participant PlatformWallet
AccountDetailView->>ManagedPlatformWallet: request indexed provider key
ManagedPlatformWallet->>RustFFI: call provider key FFI
RustFFI->>MnemonicResolver: resolve mnemonic when required
RustFFI->>PlatformWallet: derive key at index
PlatformWallet-->>RustFFI: public key, node ID, optional private key
RustFFI-->>ManagedPlatformWallet: C-string outputs
ManagedPlatformWallet-->>AccountDetailView: Swift provider key
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Review complete (commit f497398) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift`:
- Around line 1687-1704: Update reveal() to launch the private-key lookup in a
Task, mirroring AccountDetailView.revealPrivateKey(index:), so
coreAddressPrivateKey(address:) runs off the main thread. Set isRevealing and
clear revealError before starting the task, then update privateKey or
revealError from the task and reset isRevealing after completion, ensuring
SwiftUI can render the progress state.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4514d01-67fa-4175-a537-b6baab656fa1
📒 Files selected for processing (19)
packages/rs-platform-wallet-ffi/src/address_private_key.rspackages/rs-platform-wallet-ffi/src/lib.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/provider_key_at_index.rspackages/rs-platform-wallet-ffi/src/wallet_registration_persistence.rspackages/rs-platform-wallet-ffi/src/wallet_restore_types.rspackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/lib.rspackages/rs-platform-wallet/src/manager/wallet_lifecycle.rspackages/rs-platform-wallet/src/wallet/apply.rspackages/rs-platform-wallet/src/wallet/core_address_key.rspackages/rs-platform-wallet/src/wallet/mod.rspackages/rs-platform-wallet/src/wallet/provider_key_at_index.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift
… reveal from main thread The ProviderPlatformNodeKeyFFI struct was inserted between AccountSpecFFI's doc comment and the struct it documents, merging the two doc blocks and tripping clippy::doc_lazy_continuation (-D warnings on CI). Moved the new struct above the doc comment so both attach to their own items. Also mirrors AccountDetailView's Task-offloaded reveal in the address detail view so the synchronous resolver FFI doesn't block the main thread and the "Revealing…" progress state can actually render (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest push (f109ff9..7bb69d1) re-anchors the AccountSpecFFI doc comment to satisfy clippy and moves the core-address reveal off the main thread via a Swift Task. It introduces no new defect or ABI/layout change.
Prior finding reconciliation
- prior-1 — STILL VALID: CString marshalling can still leave an un-zeroized transient hex/WIF heap buffer.
- prior-2 — STILL VALID: the new cross-curve derivation entry points still have no Rust unit tests.
- prior-3 — STILL VALID: revealed key/WIF values still go to the general pasteboard without expiration; the anchor moved to lines 1717-1723.
Carried-forward prior findings
All three prior findings are carried forward below.
New findings in the latest delta
None.
Source: reviewers opus (general, security-auditor, rust-quality, ffi-engineer); gpt-5.6-sol (same four lanes, failed: revoked refresh token); verifier opus.
🟡 2 suggestion(s) | 💬 1 nitpick(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-ffi/src/address_private_key.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/address_private_key.rs:170-184: Secret hex/WIF can leave an un-zeroized plaintext copy in freed heap during CString marshalling
Provenance: carried forward from prior review of SHA f109ff9f (prior-1). Revalidated at current head 7bb69d1e — the latest delta touched only wallet_restore_types.rs and StorageRecordDetailViews.swift, so lines 170-184 here are byte-identical and the finding STILL applies.
The comment on lines 170-173 asserts the hex + WIF byte buffers 'move directly into the returned CStrings (no residual plaintext copy left on the heap).' That is not guaranteed. `hex::encode(&private_key[..])` (line 177) builds a `String` sized for exactly 2*32 = 64 chars (len == cap), and `CoreAddressPrivateKey.wif` (line 178) is a freshly-formatted `String` with no spare capacity. `CString::new` takes the buffer via `Into<Vec<u8>>` and must append a NUL terminator — with zero spare capacity this reallocates and frees the original buffer containing the plaintext private-key hex / WIF WITHOUT zeroizing it. `platform_wallet_address_private_key_free` (lines 199-214) later scrubs only the final CString buffer, not the transient pre-realloc allocation. The result: a private secp256k1 scalar (and its WIF, which fully encodes the same key) can linger in reclaimed heap after the call — exactly the leak this PR's zeroization design set out to prevent, and the one asymmetry in an otherwise disciplined secret-handling story. The same pattern applies to `private_key_hex` in provider_key_at_index.rs.
Fix by encoding into a buffer pre-sized to `len + 1` (so no realloc occurs) or into a `Zeroizing<String>`/`Zeroizing<Vec<u8>>` scrubbed on drop, and making `CoreAddressPrivateKey.wif` a zeroizing buffer, rather than relying on the move being copy-free.
In `packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs:89-158: New cryptographic key-derivation functions ship with no Rust unit tests
Provenance: carried forward from prior review of SHA f109ff9f (prior-2). Revalidated at current head 7bb69d1e — grep for `#[cfg(test)]` / `#[test]` / `mod tests` in both provider_key_at_index.rs and core_address_key.rs returns zero matches, and the latest delta added no tests, so the gap STILL exists.
The three derivation entry points introduced by this PR — `derive_platform_node_public_keys`, `PlatformWallet::derive_provider_key_at_index`, and `PlatformWallet::derive_core_address_private_key` (core_address_key.rs) — contain non-trivial, exact-value cross-curve logic (BLS non-hardened `ckd_pub` vs Ed25519 SLIP-10 hardened-only, `node_id = hash160(ed25519 pubkey)`, WIF network byte) whose consistency invariants are enforced only by `debug_assert_eq!` (compiled out of release builds). The only new Rust tests added are wallet-id scoping / duplicate-register tests in wallet_lifecycle.rs, and the PR checklist leaves 'added or updated relevant tests' unchecked — validation was manual on the simulator. A regression in an upstream key-wallet derivation crate or a path/index off-by-one would silently produce wrong key material with no failing CI test to catch it; for provider-operator / platform-node key material this means an operator could be handed a key that does not match its on-chain registration.
A small deterministic test using the canonical all-`abandon` mnemonic already used in this crate — asserting (a) `derive_platform_node_public_keys` matches `derive_provider_key_at_index(PlatformNode, i, ..)` per index, (b) `node_id == hash160(public_key)`, and (c) a known BIP32/BIP39 vector for `derive_core_address_private_key`'s hex/WIF — would lock these invariants down cheaply.
…ation tests, expire pasteboard CString::new over the zero-spare-capacity hex/WIF buffers reallocates to append the NUL and frees the plaintext original un-zeroized, stranding a private scalar (and its WIF) in reclaimed heap. Added secret_string_into_raw — copies into a NUL-sized buffer so CString::new can't realloc, wipes the Zeroizing source — and routed both the address-key and provider-key private hex through it (thepastaclaw). Added golden-vector unit tests for derive_platform_node_public_keys over the canonical abandon mnemonic: node_id == hash160(pubkey) at every index, per-index distinctness, cross-network divergence, and pinned index-0 bytes so an upstream SLIP-10/DIP-9 derivation regression fails CI. Set a 60s pasteboard expiration on the revealed key/WIF copy so the secret doesn't linger on the system-wide clipboard. Related: dashpay/rust-dashcore#858 (BLS/Ed25519 xpriv types not zeroized on drop). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
The latest remediation delta 7bb69d1e..f497398b fixes the secret CString marshalling issue and adds sound platform-node derivation vectors. It introduces no new defect, but two prior findings remain partially unresolved.
Prior findings reconciliation
- prior-1 — FIXED: secret hex/WIF/provider-key marshalling now avoids an un-zeroized transient heap allocation.
- prior-2 — STILL VALID (partially fixed): the new tests cover only
derive_platform_node_public_keys; the production BLS operator and core-address/WIF derivation paths remain untested. - prior-3 — STILL VALID (partially fixed): core-address clipboard copies now expire, but the provider private-key copy path added by this PR still uses the general pasteboard without expiration.
Carried-forward prior findings
One suggestion and one nitpick are carried forward below.
New findings in the latest delta
None.
Source: claude-general: opus completed; codex-general: gpt-5.6-sol failed; claude-security-auditor: opus completed; codex-security-auditor: gpt-5.6-sol failed; claude-rust-quality: opus completed; codex-rust-quality: gpt-5.6-sol failed; claude-ffi-engineer: opus completed; codex-ffi-engineer: gpt-5.6-sol failed; verifier: opus completed; focused prior-2 verifier: opus completed.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
Verified carried-forward findings
suggestion: BLS operator and core-address (WIF) key-derivation paths still have no Rust unit tests
packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs:227-456
Partial fix of prior-2. The three tests added at f497398 cover only the standalone free function derive_platform_node_public_keys (public-key half). Two of the three entry points the original finding named remain untested: (1) PlatformWallet::derive_provider_key_at_index (227-456) — its ProviderKeyKind::Operator branch with ExtendedBLSPrivKey::new_master, bls_public_key.derive_pub, and the release-compiled-out debug_assert_eq! BLS cross-check, plus the include_private reveal and resolved_master watch-only paths for both curves; and (2) PlatformWallet::derive_core_address_private_key in core_address_key.rs:73 (WIF/network-byte logic), whose file still has no test module. The free function duplicates the Ed25519 derivation rather than delegating to derive_provider_key_at_index, so the new tests do not exercise the method production actually calls or cross-check that the two agree per index. A BLS-operator and core-address/WIF golden-vector test, plus an equality check between the free function and derive_provider_key_at_index(PlatformNode, ...), would close the residual gap.
nitpick: Revealed provider private-key hex copied to the system-wide pasteboard without expiration (residual of prior-3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/AccountDetailView.swift:884-890
Carried forward from prior-3. This delta fixed the primary anchor in StorageRecordDetailViews.swift (now uses UIPasteboard.general.setItems(_:options:) with a 60s .expirationDate), but the sibling copyDerived here was not updated. Verified the full path: the revealed private-key row is rendered at AccountDetailView.swift:729-734 from revealedPrivateKeys[index] (populated by the includePrivate: true reveal at 863-874, i.e. BLS operator / Ed25519 platform-node secret material), and derivedValueRow wires .onTapGesture { copyDerived(value, copyKey:) } at line 790. So tapping the revealed key runs copyDerived, which does UIPasteboard.general.string = value with no expiration. This is the system-wide pasteboard: other installed apps and clipboard managers can read it, and Universal Clipboard syncs it across the user's devices, giving an unbounded exposure window. AccountDetailView.swift is a file changed by this PR and this is exactly the reveal path the PR added, so the fix should mirror the one already applied to the core-address copy. Impact is bounded (developer example app), matching the original nitpick severity. (SearchWalletsForIdentitiesView.swift:276 has the same pattern but is not a file changed by this PR, so it is out of scope.)
private func copyDerived(_ value: String, copyKey: String) {
// Raw private-key hex to the system-wide pasteboard: other apps and
// clipboard managers can read it and Universal Clipboard syncs it
// across devices. Set a short expiry so the secret doesn't linger.
// Fine for this demo app; a production wallet should avoid clipboard
// export of secrets (or gate it far more tightly).
UIPasteboard.general.setItems(
[["public.utf8-plain-text": value]],
options: [.expirationDate: Date().addingTimeInterval(60)]
)
derivedCopiedKey = copyKey
DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) {
if derivedCopiedKey == copyKey { derivedCopiedKey = nil }
}
}
| private func copyDerived(_ value: String, copyKey: String) { | ||
| UIPasteboard.general.string = value | ||
| derivedCopiedKey = copyKey | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { | ||
| if derivedCopiedKey == copyKey { derivedCopiedKey = nil } | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Revealed provider private-key hex copied to the system-wide pasteboard without expiration (residual of prior-3)
Carried forward from prior-3. This delta fixed the primary anchor in StorageRecordDetailViews.swift (now uses UIPasteboard.general.setItems(_:options:) with a 60s .expirationDate), but the sibling copyDerived here was not updated. Verified the full path: the revealed private-key row is rendered at AccountDetailView.swift:729-734 from revealedPrivateKeys[index] (populated by the includePrivate: true reveal at 863-874, i.e. BLS operator / Ed25519 platform-node secret material), and derivedValueRow wires .onTapGesture { copyDerived(value, copyKey:) } at line 790. So tapping the revealed key runs copyDerived, which does UIPasteboard.general.string = value with no expiration. This is the system-wide pasteboard: other installed apps and clipboard managers can read it, and Universal Clipboard syncs it across the user's devices, giving an unbounded exposure window. AccountDetailView.swift is a file changed by this PR and this is exactly the reveal path the PR added, so the fix should mirror the one already applied to the core-address copy. Impact is bounded (developer example app), matching the original nitpick severity. (SearchWalletsForIdentitiesView.swift:276 has the same pattern but is not a file changed by this PR, so it is out of scope.)
| private func copyDerived(_ value: String, copyKey: String) { | |
| UIPasteboard.general.string = value | |
| derivedCopiedKey = copyKey | |
| DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { | |
| if derivedCopiedKey == copyKey { derivedCopiedKey = nil } | |
| } | |
| } | |
| private func copyDerived(_ value: String, copyKey: String) { | |
| // Raw private-key hex to the system-wide pasteboard: other apps and | |
| // clipboard managers can read it and Universal Clipboard syncs it | |
| // across devices. Set a short expiry so the secret doesn't linger. | |
| // Fine for this demo app; a production wallet should avoid clipboard | |
| // export of secrets (or gate it far more tightly). | |
| UIPasteboard.general.setItems( | |
| [["public.utf8-plain-text": value]], | |
| options: [.expirationDate: Date().addingTimeInterval(60)] | |
| ) | |
| derivedCopiedKey = copyKey | |
| DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { | |
| if derivedCopiedKey == copyKey { derivedCopiedKey = nil } | |
| } | |
| } |
source: ['claude']
Resolve the base advance (11 commits, incl. #4072 provider operator/node keys + reveal address private keys) that left the PR CONFLICTING on GitHub. The one real conflict was in changeset/mod.rs, where both branches extended the `pub use changeset::{…}` re-export block; kept both sides — our InvitationChangeSet/InvitationEntry/InvitationStatus and the base's ProviderKeyAccountEntry/ProviderKeyExtendedPubKey/ProviderPlatformNodePubKey. Verified post-merge: cargo fmt --check --all clean; platform-wallet and platform-wallet-ffi compile; the invitation funding-index regression tests pass (4/4). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LecRMfPQNRrH2pYubp7wtn
2 participants
Issue being fixed or feature implemented
The Swift wallet never showed the Provider Operator keys account (BLS) or the Provider Platform Node ID keys account (EdDSA), even though key-wallet derives both at wallet creation:
AccountCollection::all_accounts()deliberately excludes them (they live in dedicatedOption<BLSAccount>/Option<EdDSAAccount>fields), so every persist/display walk skipped them and restored wallets never rebuilt them.There was also no way to view the private key for a wallet address, or for the individual provider keys.
What was done?
Provider accounts persist / restore / display
provider_key_account_registrationsvec onPlatformWalletChangeSet(typedProviderKeyExtendedPubKeyenum), snapshotted atregister_wallet, bincode-encoded into the FFI account specs, persisted by Swift, and rebuilt watch-only viaBLSAccount::new/EdDSAAccount::newon restore.Per-index provider keys
PlatformWallet::derive_provider_key_at_index+platform_wallet_provider_key_at_index: BLS operator keys derive non-hardened from the account xpub (no seed); Ed25519 platform-node keys are SLIP-10 hardened-only, so all node-key derivations require the seed.register_walletpre-derives 20 platform-node keys (pubkey + hash160 node id) while the seed is in hand and persists them (PersistentAccount.derivedPlatformNodeKeys, written once, Swift-owned thereafter). The Node Keys screen lists all 20 with no keychain prompt; pre-existing wallets fall back to a resolver-gated "Load Keys" path.Private key reveal
PlatformWallet::derive_core_address_private_key+platform_wallet_address_private_key: one FFI call that looks the address up across every account pool, derives via the mnemonic-resolver pattern (two-phase locking, resolver never called under the wallet-manager guard), and returns hex + WIF. Surfaced as a confirm-gated "View Private Key" section on the Core address detail view with tap-to-copy.Zeroizing/non_secure_erased on both sides of the FFI; the free functions zeroize the marshalled hex/WIF strings.How Has This Been Tested?
cargo checkonplatform-wallet+platform-wallet-ffiincluding--all-targets,--all-features --tests, and the iOSshieldedfeature graph;cargo fmt; fullbuild_ios.sh --target sim+ SwiftExampleApp build (warnings-as-errors) green.ckd_pubderivation in-process.Breaking Changes
None.
AccountSpecFFIgained appended fields (size pin updated 120→136) andPersistentAccountgained a defaulted SwiftData attribute — pre-release, no migration concerns.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit