feat(platform-wallet): persist typed BLS/EdDSA provider keys as core address rows#4127
Conversation
…address rows Widen CoreAddressEntryFFI from an ECDSA-only 33-byte slot to a typed 48-byte key (key_type_tag ECDSA/BLS/EdDSA + public_key_len), so BLS operator and Ed25519 platform-node public keys survive the SwiftData persist/restore round-trip in the row itself. Populate the managed platform-node pool at registration so its keys ride the normal address-pool pipeline, and retire the account-level derived-platform-node-keys batch plumbing plus the typed-key merge workaround in restore_address_pool. The storage explorer labels keys by curve, and the Node Keys screen reads the typed address rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR replaces fixed ECDSA-only address keys with typed ECDSA, BLS, and EdDSA persistence across Rust FFI and SwiftData. Platform-node keys are populated into address pools before snapshotting, restored through core-address rows, and displayed from persisted typed addresses. ChangesTyped key persistence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformWalletManager
participant ManagedWalletInfo
participant FFIPersister
participant SwiftData
participant AccountDetailView
PlatformWalletManager->>ManagedWalletInfo: populate typed EdDSA platform-node rows
ManagedWalletInfo->>FFIPersister: snapshot typed core-address pools
FFIPersister->>SwiftData: persist public key bytes and key type
SwiftData->>AccountDetailView: load typed core-address records
AccountDetailView->>AccountDetailView: derive platform-node display data
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 |
|
⛔ Blockers found — Sonnet deferred (commit e6fd9b5) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift (2)
4682-4689: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid hardcoding the struct layout size.
Consider using
MemoryLayout.size(ofValue: e.public_key)instead of the hardcoded48. If the FFI struct definition forpublic_keychanges in the future, this will automatically adapt and prevent potential out-of-bounds errors or unintentional truncation.♻️ Proposed refactor
- if row.publicKey.count <= 48 { + if row.publicKey.count <= MemoryLayout.size(ofValue: e.public_key) { copyBytes(row.publicKey, into: &e.public_key) e.public_key_len = UInt8(row.publicKey.count) e.key_type_tag = row.keyType } else {🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift` around lines 4682 - 4689, Update the public-key length check in the PlatformWallet persistence mapping to compare row.publicKey.count against MemoryLayout.size(ofValue: e.public_key) instead of the hardcoded 48, while preserving the existing copy and fallback behavior.
6167-6179: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSimplify initialization and prevent out-of-bounds traps.
You can safely initialize the
Databuffer using the collection'sprefixmethod. This guarantees bounds safety up tosrc.countand eliminates the need for manual memory copying and explicit subscripting, which would trap ifkeyLenever exceeded the tuple's bounds due to an FFI mismatch or memory corruption.🛡️ Proposed refactor
- let publicKey: Data - if keyLen > 0 { - var pk = Data(count: keyLen) - withUnsafeBytes(of: entry.public_key) { src in - pk.withUnsafeMutableBytes { dst in - dst.copyMemory(from: UnsafeRawBufferPointer(rebasing: src[0..<keyLen])) - } - } - publicKey = pk - } else { - publicKey = Data() - } + let publicKey = keyLen > 0 ? withUnsafeBytes(of: entry.public_key) { Data($0.prefix(keyLen)) } : Data()🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift` around lines 6167 - 6179, Update the publicKey initialization in the entry parsing block to construct Data directly from the public_key byte collection’s prefix(keyLen), preserving an empty Data result for nonpositive lengths. Remove the manual buffer allocation, unsafe byte closures, and direct src[0..<keyLen] slicing so lengths exceeding the source bounds are safely truncated.
🤖 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.
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- Around line 4682-4689: Update the public-key length check in the
PlatformWallet persistence mapping to compare row.publicKey.count against
MemoryLayout.size(ofValue: e.public_key) instead of the hardcoded 48, while
preserving the existing copy and fallback behavior.
- Around line 6167-6179: Update the publicKey initialization in the entry
parsing block to construct Data directly from the public_key byte collection’s
prefix(keyLen), preserving an empty Data result for nonpositive lengths. Remove
the manual buffer allocation, unsafe byte closures, and direct src[0..<keyLen]
slicing so lengths exceeding the source bounds are safely truncated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d72dd934-e998-4f29-b62c-3dd4fac1e640
📒 Files selected for processing (14)
packages/rs-platform-wallet-ffi/src/core_address_types.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/manager/wallet_lifecycle.rspackages/rs-platform-wallet/src/wallet/provider_key_at_index.rspackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAccount.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentCoreAddress.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
💤 Files with no reviewable changes (2)
- packages/swift-sdk/Sources/SwiftDashSDK/Persistence/Models/PersistentAccount.swift
- packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs
…implify key copy Address CodeRabbit review: derive the 48-byte slot bound from the FFI field itself and build the persisted key Data via a bounds-safe prefix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The typed provider-key persistence and round-trip tests pass, but the Storage Explorer exposes a curve-incorrect private-key action for the newly persisted BLS and Ed25519 rows. The two prior migration findings remain technically valid but are intentionally deferred under the documented pre-release policy of recreating stale development stores.
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 (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/StorageRecordDetailViews.swift:1602: Do not derive secp256k1 secrets for typed provider-key rows
The Storage Explorer queries every PersistentCoreAddress and routes each row to this detail view, while this PR now persists BLS and Ed25519 provider keys as ordinary rows with keyType 1 and 2. The private-key section is rendered unconditionally and calls coreAddressPrivateKey, whose Rust implementation looks up only the stored derivation path, always performs secp256k1 private derivation, and always returns a WIF. For a provider row this can therefore display a plausible but unrelated secp256k1 secret instead of the BLS or Ed25519 key matching the displayed public key. Hide this generic action for non-ECDSA rows or route them through providerKeyAtIndex, as the account detail screen already does.
…nd-example-app Conflicts: Cargo.lock (theirs + re-resolve) and identity_top_up.rs (base #4093's rework — MIN_TOP_UP_DUFFS floor, out_new_balance sentinel, guard tests — supersedes the branch's older zero-guard). Breaking-FFI adaptations for the Android JNI/Kotlin side: - #4093/#4126: shielded identity-create/transfer/unshield/withdraw gained a mnemonic_resolver_handle param — threaded through the JNI exports, FundingNative externs, and PlatformWalletManager; the transfer/unshield/withdraw trio now runs under teardownGate.op (they borrow the manager's resolver, so the round-46 ungated rationale no longer holds). - #4127 typed provider keys: the pool-entry write trampoline forwards exactly public_key_len meaningful bytes (blob self-describing by length: 33 ECDSA / 48 BLS / 32 EdDSA); the load path rebuilds the 48-byte slot + key_type_tag from the stored blob length. No Kotlin bridge/descriptor change needed. - AccountSpecFFI lost derived_platform_node_keys(+count) (moved into the typed core-address rows) — the round-42 null-init is dropped. - #4126 Orchard viewing keys: the three new vtable slots are None — binds keep resolving the seed via the mnemonic resolver (the documented fallback); the seedless-bind persistence port is tracked as a follow-up. Verified: cargo fmt, JNI check (shielded), workspace clippy --all-features -D warnings, platform-wallet-ffi lib tests (170), gradle :sdk+:app tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
CoreAddressEntryFFIcarried a fixed 33-byte ECDSA-onlypublic_keyslot, so non-secp256k1 provider keys were dropped on the SwiftData persist round-trip:publicKey, kept alive in-memory only by a merge workaround inrestore_address_pool.derivedPlatformNodeKeysbatch (ProviderPlatformNodeKeyFFIonAccountSpecFFI) with its own restore path (restore_platform_node_pool).What was done?
Rust / FFI
CoreAddressEntryFFIto a typed key:public_key: [u8; 48]+public_key_len(0 = none / 33 ECDSA / 48 BLS / 32 EdDSA) +key_type_tag(newKeyTypeTagFFIenum).build_core_address_entry_ffimarshals all threePublicKeyTypevariants with per-curve length validation;address_info_from_ffirebuilds the typed key, forgiving on invalid tag/len pairs.register_walletnow populates the managed platform-node pool from the pre-derived batch (newpopulate_platform_node_poolinrs-platform-wallet) before the address-pool snapshot, so EdDSA keys persist as ordinary typed address rows and the in-memory pool matches what a restore reconstructs.ProviderKeyAccountEntry.derived_platform_node_keys,ProviderPlatformNodeKeyFFI, theAccountSpecFFIptr/len pair, andrestore_platform_node_poolare gone (AccountAddressPoolFFIlayout guard updated 136 → 120).restore_address_pool— the row itself is now authoritative for its typed key.platform_wallet_platform_node_id_from_ed25519_pubkey(wrapsdashcore::PlatformNodeId::from_ed25519_public_key) so the host can render node ids from persisted pubkeys.Swift
PersistentCoreAddressgainskeyType; the persistence handler marshalspublic_key_len/key_type_tagboth ways.PersistentAccount.derivedPlatformNodeKeysand its marshaling removed (pre-release, no migration shims — stale dev stores are deleted + re-imported).How Has This Been Tested?
cargo test -p platform-wallet -p platform-wallet-ffi --all-features— all green, including new tests:typed_public_key_survives_ffi_round_trip_into_fresh_pool— BLS-48 / EdDSA-32 / ECDSA-33 throughbuild_core_address_entry_ffi → address_info_from_ffi → restore_address_poolinto a fresh pool, byte-for-byte, no merge path.populate_platform_node_pool_lands_typed_eddsa_rows— registration-side pool population pins typed rows + node-id-derived addresses + watermark.cargo check --all-targetsonplatform-wallet,platform-wallet-ffi,rs-unified-sdk-ffi;cargo fmt --all --checkclean../build_ios.sh --target sim— full Rust + SwiftDashSDK + SwiftExampleApp build succeeded.keyType=1, 48-byte keys and Provider Platform Node Keys rows withkeyType=2, 32-byte keys (20 rows, previously zero); storage explorer renders "BLS Public Key" (96 hex) and "Ed25519 Public Key" (64 hex); typed rows survive terminate → relaunch (restore path); Node Keys screen lists all keys from persistence with node ids matchingSHA256(pubkey)[..20]exactly.Breaking Changes
None on-chain / consensus. The FFI ABI and SwiftData schema changed together in-repo (pre-release; existing dev stores fail container creation and need delete + re-import, per project policy).
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit