fix(platform-wallet): token transitions require a CRITICAL signing key#3551
Merged
QuantumExplorer merged 5 commits intoApr 28, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe pull request updates DPP identity key security level assignment logic. The FFI identity derivation now assigns three distinct security levels (MASTER, CRITICAL, HIGH) based on key index, replacing a two-level scheme. Additionally, token state-transition signing key selection now requires CRITICAL security level exclusively instead of accepting MASTER or HIGH. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Collaborator
Review GateCommit:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/tokens/helpers.rs (1)
8-13: Optional: avoid hard-coded line numbers in cross-file doc reference.The reference to
state_transitions/document/batch_transition/methods/v0/mod.rs:133-138will silently rot the moment that file is touched. Consider citing the symbol (e.g.,combined_security_level_requirement) or the function name rather than a line range.📝 Suggested tweak
-//! `[SecurityLevel::CRITICAL]` only — MASTER, HIGH, and MEDIUM are -//! all rejected by Drive. See -//! `state_transitions/document/batch_transition/methods/v0/mod.rs:133-138`. +//! `[SecurityLevel::CRITICAL]` only — MASTER, HIGH, and MEDIUM are +//! all rejected by Drive. See `combined_security_level_requirement` +//! in `state_transitions/document/batch_transition/methods/v0/mod.rs`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-platform-wallet/src/wallet/identity/network/tokens/helpers.rs` around lines 8 - 13, Replace the hard-coded line range reference with a symbol-based reference: remove ":133-138" and cite the `combined_security_level_requirement` symbol (or the exact function/method name) instead, so the doc comment points to the function by name rather than fragile line numbers; update the doc text around `CRITICAL` to reference `combined_security_level_requirement` (and optionally the module path if helpful) in place of the numeric range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/tokens/helpers.rs`:
- Around line 8-13: Replace the hard-coded line range reference with a
symbol-based reference: remove ":133-138" and cite the
`combined_security_level_requirement` symbol (or the exact function/method name)
instead, so the doc comment points to the function by name rather than fragile
line numbers; update the doc text around `CRITICAL` to reference
`combined_security_level_requirement` (and optionally the module path if
helpful) in place of the numeric range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcb4f201-973e-4197-8a7a-d6f2d91d2c6d
📒 Files selected for processing (2)
packages/rs-platform-wallet-ffi/src/identity_derive_and_persist.rspackages/rs-platform-wallet/src/wallet/identity/network/tokens/helpers.rs
Contributor
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "272688be34fe917e209a83ab54c4e7514ec8a57f5c21ef2bc57726310fa3b41f"
)Xcode manual integration:
|
…ing key
Token state transitions on Platform fail with the consensus error
"Invalid public key security level MASTER. The state transition
requires one of CRITICAL" — observed in the iOS SwiftExampleApp Mint
flow against testnet. Root cause is a two-part bug:
1. Identity registration in `dash_sdk_derive_and_persist_identity_keys`
produces only `[MASTER, HIGH, HIGH, ...]`. There is no CRITICAL
key in the default 3-key set.
2. The token-action signing-key resolver in `helpers.rs` searches
`[SecurityLevel::MASTER, SecurityLevel::HIGH]`, which would still
be wrong even if a CRITICAL key existed.
rs-dpp's `combined_security_level_requirement`
(`state_transitions/document/batch_transition/methods/v0/mod.rs:133-138`)
collapses the allowed key set for *any* batch containing a token
transition to `[SecurityLevel::CRITICAL]` — MASTER, HIGH, and MEDIUM
are all rejected by Drive. Without a CRITICAL key on the identity,
the user cannot mint / burn / freeze / transfer tokens at all.
Fixes:
- `identity_derive_and_persist.rs`: registration now produces the
canonical 3-key layout (matches rs-dpp's
`main_keys_with_random_authentication_keys_*`):
key 0: MASTER AUTHENTICATION ECDSA_SECP256K1 (IdentityUpdate)
key 1: CRITICAL AUTHENTICATION ECDSA_SECP256K1 (token transitions)
key > 1: HIGH AUTHENTICATION ECDSA_SECP256K1 (general document ops)
Updated the module-doc table and the `happy_path_persists_three_
keys_and_returns_pubkeys` test to assert the new layout.
- `helpers.rs::token_resolve_signing_key`: search criteria narrowed
to `[SecurityLevel::CRITICAL]`. Updated module doc + error message
to point users at the migration path when the search misses (see
below).
Migration for existing identities:
Identities registered before this fix have only MASTER + HIGH keys.
After this fix:
- New identities work out of the box — they get a CRITICAL key at
registration time.
- Existing identities cannot perform token actions until they add a
CRITICAL AUTHENTICATION ECDSA_SECP256K1 key via IdentityUpdate, or
the user re-registers a fresh identity.
The error message from `token_resolve_signing_key` calls this out
explicitly so the user sees a useful next step rather than a generic
"key not found" wall.
Validation:
- cargo fmt --all clean
- cargo check -p platform-wallet -p platform-wallet-ffi clean
- cargo test -p platform-wallet-ffi --lib identity_derive_and_persist:
4/4 pass (happy_path_persists_three_keys updated to expect MASTER/
CRITICAL/HIGH)
- build_ios.sh --target sim with -warnings-as-errors: BUILD SUCCEEDED
- xcodebuild SwiftExampleApp iPhone 17 sim: BUILD SUCCEEDED
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`PlatformWalletPersistenceHandler.persistIdentities` was hard-coding new `PersistentIdentity` rows with `isLocal: false`, ignoring the `identityIndex: UInt32?` field on `IdentityEntrySnapshot` whose own docstring already says it's the natural distinguisher: > `nil` for out-of-wallet (observed) identities — they have no > derivation context. `Some(_)` mirrors the BIP-9 HD identity index > used during registration. Symptom: every identity registered through the iOS wallet got written as `isLocal=false`. The `RecipientPickerView`'s @query filters `isLocal == true`, so the local-identity picker came up empty even when the user had just registered themselves. Same for any other view that relies on the `localIdentitiesPredicate` helper. Fixes: - New rows: derive `isLocal` from `entry.identityIndex != nil`. A populated HD index means the identity was registered by this wallet; `nil` means an out-of-wallet observation (DashPay contact, payment recipient, etc.). - Existing rows: self-heal in the upsert path. Whenever a snapshot arrives with an `identityIndex`, flip stale `isLocal=false` rows to true. Repairs identities that were persisted before this fix (e.g. anything created via the previous PR's CreateIdentity flow) on their next sync event. LoadIdentityView's explicit `isLocal=false` (used when the user manually loads a remote identity by id) is correct and untouched. Validation: - build_ios.sh --target sim with -warnings-as-errors: BUILD SUCCEEDED Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ot isLocal Reverts the ill-conceived isLocal-from-identityIndex change and addresses the original recipient-picker bug at the right layer. Two semantics were tangled in `PersistentIdentity.isLocal`: 1. **"Local Only" badge** — identity exists in the local DB but Platform hasn't confirmed it. Drives the orange badge in IdentitiesView and IdentityDetailView. 2. **"Owned by my wallet"** — identity has an HD derivation context, this device controls signing for it. `localIdentitiesPredicate(network:)` was filtering on `isLocal == true` to answer #2, but the persister upserts confirmed-on-network identities with `isLocal == false` (correctly), so wallet-owned identities matching the network filter never showed up in the recipient pickers. The previous commit tried to "fix" this by making the persister write `isLocal == true` for any wallet-derived identity — but that broke the badge: every wallet-owned identity flipped to "Local Only" in IdentitiesView even when on-network. Wallet ownership already travels on a different column: the `wallet: PersistentWallet?` relationship the persister populates via `row.wallet = fetchWalletForLink(walletId:)`. That's the right join. Changes: - Persister: revert to `isLocal: false` for new rows; remove the self-heal block that was promoting on-network rows to "Local Only". - `PersistentIdentity.localIdentitiesPredicate` (both the static and network-scoped variants): filter on `identity.wallet != nil` instead of `identity.isLocal == true`. The flag and the ownership are now decoupled. Existing callers (RecipientPickerView, TokenActionPermissionsView "Acting as" picker) get the right population without changes. The "Local Only" badge stays accurate: identities created locally that haven't reached Platform sit at the model default `isLocal: true` and render orange; the persister flips them to `false` once Platform confirms. Validation: - build_ios.sh --target sim with -warnings-as-errors: BUILD SUCCEEDED Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…et callback The Rust pipeline for token-balance sync was already complete: TokenWallet::sync() queries Platform per watched (identity, token) pair, populates PlatformWalletInfo.token_balances, builds a TokenBalanceChangeSet, and dispatches it through self.persister.store(cs.into()). The chain broke at the FFI persister layer — there was no vtable slot for token balances — so the sub- changeset hit a no-op and nothing reached SwiftData. Result: PersistentTokenBalance rows always read 0, and every view that drives balance display / validation off @query (Burn, Transfer, DestroyFrozen, recipient pickers) was broken. Adds the missing wiring end-to-end: Rust (rs-platform-wallet-ffi) - New TokenBalanceUpsertFFI (32-byte identity + 32-byte token + u64 balance) and TokenBalanceRemovalFFI (32-byte identity + 32-byte token) #[repr(C)] structs. - New on_persist_token_balances_fn slot on PersistenceCallbacks (additive — no existing callback signatures change). - FFIPersister::store() dispatches changeset.token_balances onto the new callback, materializing both upserts and removals. - New platform_wallet_token_watch_and_sync FFI fn at rs-platform-wallet-ffi/src/tokens/sync.rs: takes an array of (identity, token) pairs, runs TokenWallet::watch for each, then TokenWallet::sync() once. Single round-trip from Swift. Swift SDK - PersistenceCallbacks gains the matching @convention(c) slot; makeCallbacks() registers a C trampoline persistTokenBalancesCallback that copies the FFI buffers into Swift snapshot structs (TokenBalanceUpsertSnapshot / TokenBalanceRemovalSnapshot) before the callback returns — same lifetime pattern as IdentityEntrySnapshot. - New PlatformWalletPersistenceHandler.persistTokenBalances upserts/removes PersistentTokenBalance rows under the existing begin/end changeset bracket, links the identity + token relationships, and saves atomically with the rest of the changeset round. - Token id key normalization: Rust ships 32 bytes, Swift base58- encodes for PersistentTokenBalance.tokenId (matches the existing string column the legacy code path uses). - New ManagedPlatformWallet.watchAndSyncTokenBalances(pairs:) async wrapper that marshals (identity, token) tuples through the new FFI fn. App (SwiftExampleApp) - IdentityDetailView.reloadTokenBalances now calls watchAndSyncTokenBalances right after computing its idToToken map — same pairs that drive its existing display fetch. The persister fires synchronously, PersistentTokenBalance rows populate, and every @Query-driven view downstream sees the fresh balances without per-view triggers. Legacy getIdentityTokenBalances stays for now as belt-and-suspenders display state; can be retired in a follow-up that flips the view to a @query<PersistentTokenBalance>. Validation - cargo fmt --all clean - cargo check --workspace clean - cargo test -p platform-wallet --lib: 110/110 pass - cargo test -p platform-wallet-ffi --lib: 69/69 pass - build_ios.sh --target sim with -warnings-as-errors: BUILD SUCCEEDED - xcodebuild SwiftExampleApp iPhone 17 sim: BUILD SUCCEEDED Smoke path: fresh install + wallet restore → IdentityDetailView for any wallet identity → PersistentTokenBalance rows populate via the persister → Burn / Transfer / picker views read non-zero balances via @query without manual refresh. Deferred - Per-view re-sync on appear for Burn / Transfer / DestroyFrozen if the user navigates there without visiting IdentityDetailView first. Natural follow-up: add a token-balance tick to PlatformBalanceSyncService alongside the existing periodic platform-credit sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
fix/token-action-signing-key-security-level
branch
from
2bc01ca to
7ea4327
Compare
…iesPredicate The previous commit changed the predicate body from `isLocal == true` to `wallet != nil` (correctly: views needed to filter to identities the user can act as / sign for, not the "Local Only" badge state). But the function name kept the misleading `local` prefix, conflating two unrelated concepts: - `isLocal` flag — drives the "Local Only" / "On Network" UI badge, i.e. whether Platform has confirmed the identity yet. - `wallet != nil` relationship — whether this device's wallet derived and owns the identity (HD slot recorded by the persister). These are orthogonal: an identity can be wallet-owned and `isLocal` (just registered, awaiting confirmation), wallet-owned and on-network (confirmed), or out-of-wallet (DashPay contact, payment recipient). Renamed to `walletOwnedIdentitiesPredicate` (and the network-scoped sibling) to match what the body actually does. Updated the three callers — RecipientPickerView and the two `_localIdentities = Query` assignments in TokenActionPermissionsView. `localIdentitiesPredicate` is gone with no replacement. If a view genuinely wants to filter on the `isLocal` flag in the future, add a new `localOnlyIdentitiesPredicate` then; nothing currently does. Validation: - build_ios.sh --target sim with -warnings-as-errors: BUILD SUCCEEDED Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Token state transitions on Platform fail with consensus error `Invalid public key security level MASTER. The state transition requires one of CRITICAL` — observed in the iOS SwiftExampleApp Mint flow against testnet (screenshot). Two-part fix:
rs-dpp's `combined_security_level_requirement` collapses the allowed key set for any batch containing a token transition to `[CRITICAL]` only.
Changes
`identity_derive_and_persist.rs` — registration now produces the canonical 3-key layout from rs-dpp's `main_keys_with_random_authentication_keys_*`:
`helpers.rs::token_resolve_signing_key` — search criteria narrowed to `[SecurityLevel::CRITICAL]` with a doc comment explaining why and a helpful error message pointing existing-identity users at the migration path.
Migration for existing identities
Identities registered before this fix only have MASTER + HIGH keys. After merge:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit