fix(platform-wallet): apply disabled-key flags to local cache after identity update#3915
Conversation
…dentity update The post-broadcast local-cache apply for the disable-key side of an identity update was an unimplemented TODO: it logged a warning and left the in-memory `ManagedIdentity` cache stale, so a disabled key's `disabled_at` only flipped in SwiftData after a full network re-fetch. Add `ManagedIdentity::disable_keys`, the disable-side counterpart to `add_key`. It stamps `disabled_at` on the matching `IdentityPublicKey` records in the cached DPP `Identity` and fires the persister with the same combined changeset shape `add_key` uses, so the client's `PersistentPublicKey.disabledAt` rows update immediately without a re-fetch. Each emitted key entry reuses the `(wallet_id, identity_index, key_index)` derivation breadcrumb so the disabled key keeps its private-key linkage rather than getting wiped on the upsert. Revision bump stays with the caller; missing key ids are skipped. Follow-up to promoting test case ID-12 (disable identity key) to a production UI action. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
More reviews will be available in 22 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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 |
|
✅ Review complete (commit 431d63b) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR cleanly mirrors add_key for the disable side and is largely correct, but it introduces one consequential side effect: stamping disabled_at in the local cache means the existing MASTER-key selector in update_identity_with_external_signer can now pick a disabled master after a rotation. Test coverage of the persistence contract (the actual fix payload — derivation breadcrumb + wallet_id) is also missing.
🔴 1 blocking | 🟡 2 suggestion(s)
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/rs-platform-wallet/src/wallet/identity/network/update.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/update.rs:138-151: MASTER signer selector must skip disabled keys now that the cache stamps `disabled_at`
This PR newly stamps `disabled_at` on cached `IdentityPublicKey`s in `disable_keys`, so the cached identity loaded at lines 117–120 can now contain disabled keys. The MASTER selector here iterates `public_keys()` and picks the first match by id without checking `disabled_at`. A valid master-key rotation disables the old MASTER and adds a new MASTER in the same update; after this PR applies that rotation locally, a subsequent update on the same identity will deterministically select the old (lower-id) MASTER even though `sign_external`/DPP reject keys with `disabled_at` set, and the update will fail locally despite the cache containing a valid enabled MASTER that could sign. This is a direct regression caused by making the local cache accurate. Add `&& key.disabled_at().is_none()` to the predicate.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/mod.rs:360-406: Test never exercises the breadcrumb — the load-bearing part of the fix
The PR's stated purpose is that each emitted `IdentityKeyEntry` carries `(wallet_id, identity_index, key_index)` so the Swift upsert preserves `privateKeyKeychainIdentifier`. `test_disable_keys_stamps_disabled_at` constructs a `ManagedIdentity::new(identity, 0)` (leaving `wallet_id == None`) and uses `noop_persister()`, so the `match (self.wallet_id, self.identity_index)` always falls into the `None` arm and the persister discards any changeset. A regression that drops the breadcrumb, computes the wrong `key_index`, or stops calling `persister.store(...)` entirely would still pass this test. Add a recording persister, set `managed.wallet_id = Some(..)` and `identity_index = Some(..)`, then assert the captured `PlatformWalletChangeSet` contains one upsert with `wallet_id` set, `derivation_indices == Some(IdentityKeyDerivationIndices { identity_index, key_index: key_id })`, and the matching disabled public key. Cover the no-match early-return at lines 388–390 in the same pass.
In `packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/managed_identity/identity_ops.rs:362-371: Document the `key_index == key_id` cross-module invariant
`disable_keys` reconstructs the breadcrumb as `IdentityKeyDerivationIndices { identity_index, key_index: key_id }`, equating the DPP `KeyID` with the DIP-9 hardened derivation index. This holds today because every `add_key` call site passes `key_index = key.id()` (e.g. `update.rs:231`, registration), and `MASTER_KEY_INDEX == 0` happens to equal the master key id. But this is an implicit cross-module invariant, not a typed one — if a future wallet adds keys with non-monotonic IDs (revoke/re-add, imports), the persisted breadcrumb will point the Swift Keychain at the wrong derivation slot, silently defeating the PR's stated goal. Either cache the derivation index per key at `add_key` time and read it back here, or at minimum add a doc-comment and a `debug_assert` recording the invariant and the failure mode.
| #[test] | ||
| fn test_disable_keys_stamps_disabled_at() { | ||
| use dpp::identity::accessors::{IdentityGettersV0, IdentitySettersV0}; | ||
| use dpp::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0; | ||
| use dpp::identity::identity_public_key::v0::IdentityPublicKeyV0; | ||
| use dpp::identity::{IdentityPublicKey, KeyType, Purpose, SecurityLevel}; | ||
| use dpp::platform_value::BinaryData; | ||
|
|
||
| let make_key = |id: u32| { | ||
| IdentityPublicKey::V0(IdentityPublicKeyV0 { | ||
| id, | ||
| key_type: KeyType::ECDSA_SECP256K1, | ||
| purpose: Purpose::AUTHENTICATION, | ||
| security_level: SecurityLevel::HIGH, | ||
| contract_bounds: None, | ||
| read_only: false, | ||
| data: BinaryData::new(vec![0x02; 33]), | ||
| disabled_at: None, | ||
| }) | ||
| }; | ||
|
|
||
| let mut identity = create_test_identity(); | ||
| let mut keys = BTreeMap::new(); | ||
| keys.insert(0, make_key(0)); | ||
| keys.insert(1, make_key(1)); | ||
| identity.set_public_keys(keys); | ||
|
|
||
| let mut managed = ManagedIdentity::new(identity, 0); | ||
| let p = noop_persister(); | ||
|
|
||
| // Disable key 1 plus a non-existent key 9 (must be skipped). | ||
| managed.disable_keys(&[1, 9], 1_700_000_000, &p); | ||
|
|
||
| let pk0 = managed.identity.get_public_key_by_id(0).unwrap(); | ||
| let pk1 = managed.identity.get_public_key_by_id(1).unwrap(); | ||
| assert_eq!(pk0.disabled_at(), None, "untouched key must stay enabled"); | ||
| assert_eq!( | ||
| pk1.disabled_at(), | ||
| Some(1_700_000_000), | ||
| "targeted key must be stamped disabled" | ||
| ); | ||
| // The skipped key id must not have materialized a phantom row. | ||
| assert!( | ||
| managed.identity.get_public_key_by_id(9).is_none(), | ||
| "non-existent key id must not be created" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Test never exercises the breadcrumb — the load-bearing part of the fix
The PR's stated purpose is that each emitted IdentityKeyEntry carries (wallet_id, identity_index, key_index) so the Swift upsert preserves privateKeyKeychainIdentifier. test_disable_keys_stamps_disabled_at constructs a ManagedIdentity::new(identity, 0) (leaving wallet_id == None) and uses noop_persister(), so the match (self.wallet_id, self.identity_index) always falls into the None arm and the persister discards any changeset. A regression that drops the breadcrumb, computes the wrong key_index, or stops calling persister.store(...) entirely would still pass this test. Add a recording persister, set managed.wallet_id = Some(..) and identity_index = Some(..), then assert the captured PlatformWalletChangeSet contains one upsert with wallet_id set, derivation_indices == Some(IdentityKeyDerivationIndices { identity_index, key_index: key_id }), and the matching disabled public key. Cover the no-match early-return at lines 388–390 in the same pass.
source: ['claude', 'codex']
| let (wallet_id, derivation_indices) = match breadcrumb { | ||
| Some((wallet_id, identity_index)) => ( | ||
| Some(wallet_id), | ||
| Some(crate::changeset::IdentityKeyDerivationIndices { | ||
| identity_index, | ||
| key_index: key_id, | ||
| }), | ||
| ), | ||
| None => (None, None), | ||
| }; |
There was a problem hiding this comment.
🟡 Suggestion: Document the key_index == key_id cross-module invariant
disable_keys reconstructs the breadcrumb as IdentityKeyDerivationIndices { identity_index, key_index: key_id }, equating the DPP KeyID with the DIP-9 hardened derivation index. This holds today because every add_key call site passes key_index = key.id() (e.g. update.rs:231, registration), and MASTER_KEY_INDEX == 0 happens to equal the master key id. But this is an implicit cross-module invariant, not a typed one — if a future wallet adds keys with non-monotonic IDs (revoke/re-add, imports), the persisted breadcrumb will point the Swift Keychain at the wrong derivation slot, silently defeating the PR's stated goal. Either cache the derivation index per key at add_key time and read it back here, or at minimum add a doc-comment and a debug_assert recording the invariant and the failure mode.
source: ['claude']
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3915 +/- ##
============================================
+ Coverage 71.20% 72.19% +0.99%
============================================
Files 20 21 +1
Lines 2837 2946 +109
============================================
+ Hits 2020 2127 +107
- Misses 817 819 +2
🚀 New features to boost your workflow:
|
2 participants
Issue being fixed or feature implemented
The post-broadcast local-cache apply for the disable-key side of an identity update was an unimplemented TODO in
update_identity_with_external_signer: it logged a warning and left the in-memoryManagedIdentitycache stale. As a result, a disabled key'sdisabled_atflag only flipped in SwiftData after a full network re-fetch (viaIdentityDetailView.refreshIdentityData()).This blocks promoting test case ID-12 (disable identity key) to a production UI action, where the Identity Keys list needs to reflect the disabled flag immediately.
What was done?
Added
ManagedIdentity::disable_keys, the disable-side counterpart to the existingadd_key:disabled_aton the matchingIdentityPublicKeyrecords in the cached DPPIdentity, so every signing / introspection path sees the disabled flag immediately.add_keyuses (scalar identity snapshot +IdentityKeysChangeSetupserts), so the client'sPersistentPublicKey.disabledAtrows update without a network re-fetch. The FFI derives the SwiftdisabledAtfromentry.public_key.disabled_at().(wallet_id, identity_index, key_index)derivation breadcrumb thatadd_keycarries — reconstructed from the managed identity's own slot. This matters because the Swift upsert wipesprivateKeyKeychainIdentifierwhenderivationIndicesis absent; carrying the breadcrumb makes the client re-derive idempotently and keep the key's private-key linkage instead of dropping it.Wired it into the call site in
update.rs, replacing the TODO/tracing::warn!block. The local wall-clock time is passed as a placeholderdisabled_at; the next Platform refresh reconciles it to the authoritative on-chain block time.How Has This Been Tested?
cargo check -p platform-wallet— passes.cargo test -p platform-wallet --lib managed_identity— 21 passed, 0 failed, including the newtest_disable_keys_stamps_disabled_at(asserts the targeted key is stamped, an untouched key stays enabled, and a non-existent key id is skipped without creating a phantom row).cargo fmt --all— clean.Breaking Changes
None. Adds a new public method to
platform-wallet; no consensus or wire-format changes.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code