docs(drive): update_operator_identity description and inline comments #2836
Conversation
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
WalkthroughThese changes refactor operator identity update logic, adding defensive state retrieval with error handling, early bail-outs on no changes, and more granular key lifecycle management including snapshotting, conditional re-enabling, and identity switching. Documentation is enhanced for clarity. Changes
Sequence DiagramsequenceDiagram
participant Core as Core Changes
participant V0 as Operator Update (v0)
participant Cache as Cached State
participant Identity as Identity Storage
Core->>V0: Trigger update
rect rgb(200, 220, 255)
Note over V0: Check for Changes
V0->>V0: Has operator changes?
alt No changes
V0-->>Core: Early bail-out
end
end
rect rgb(220, 200, 255)
Note over V0: Defensive Retrieval
V0->>Cache: Fetch old masternode
alt Not found
V0-->>Core: Error
end
end
rect rgb(200, 255, 220)
Note over V0: Determine Path
alt Operator pubkey unchanged
V0->>Identity: Snapshot existing keys
V0->>Identity: Disable stale keys
V0->>Identity: Re-enable/add keys
else Operator pubkey changed
V0->>Identity: Disable previous keys
V0->>Identity: Re-use or create new keys
end
end
V0-->>Core: Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
✅ 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: "53dac1d22320e621fc692ebcfaa6297d4673c58c408a07d79bda99df13444dd4"
)Xcode manual integration:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs (2)
118-139: Match keys by purpose/key_type, not only raw bytes.Comparing only key.data risks false matches across different key roles. Guard by Purpose and KeyType wherever you disable, re-enable, or search for keys.
Suggested targeted diffs:
Disable in same-identity path (payout and node-id):
- if needs_change_operator_payout_address { - if Some(key.data().as_slice()) == old_masternode.state.operator_payout_address.as_ref().map(|bytes| bytes.as_slice()) { + if needs_change_operator_payout_address + && key.purpose() == Purpose::TRANSFER + && key.key_type() == KeyType::ECDSA_HASH160 + { + if Some(key.data().as_slice()) == old_masternode + .state + .operator_payout_address + .as_ref() + .map(|bytes| bytes.as_slice()) + { return Some(key_id); - } else if let Some(operator_payout_address) = operator_payout_address_change.as_ref().unwrap() { - if key.data().as_slice() == operator_payout_address.as_slice() { + } else if let Some(operator_payout_address) = + operator_payout_address_change.as_ref().unwrap() + { + if key.data().as_slice() == operator_payout_address.as_slice() { old_operator_payout_address_to_re_enable = Some(key_id); } } } - if needs_change_platform_node_id - && old_masternode.state.platform_node_id.is_some() - { - if key.data().as_slice() == old_masternode.state.platform_node_id.as_ref().unwrap() { + if needs_change_platform_node_id + && key.purpose() == Purpose::SYSTEM + && key.key_type() == KeyType::EDDSA_25519_HASH160 + && old_masternode.state.platform_node_id.is_some() + { + if key.data().as_slice() + == old_masternode.state.platform_node_id.as_ref().unwrap() + { return Some(key_id); - } else if platform_node_id_change.as_ref().unwrap().as_slice() == key.data().as_slice() { + } else if platform_node_id_change.as_ref().unwrap().as_slice() + == key.data().as_slice() + { old_operator_node_id_to_re_enable = Some(key_id); } }Disable in identity-switch path (old identity):
- if key.data().as_slice() == old_masternode.state.pub_key_operator { + if key.purpose() == Purpose::AUTHENTICATION + && key.key_type() == KeyType::BLS12_381 + && key.data().as_slice() == old_masternode.state.pub_key_operator + { return Some(key_id); - } - if old_masternode.state.platform_node_id.is_some() - && key.data().as_slice() == old_masternode.state.platform_node_id.as_ref().unwrap() - { + } + if old_masternode.state.platform_node_id.is_some() + && key.purpose() == Purpose::SYSTEM + && key.key_type() == KeyType::EDDSA_25519_HASH160 + && key.data().as_slice() == old_masternode.state.platform_node_id.as_ref().unwrap() + { return Some(key_id); }Search on destination identity:
- .find(|(_, key)| key.data().as_slice() == new_payout_address) + .find(|(_, key)| key.purpose() == Purpose::TRANSFER + && key.key_type() == KeyType::ECDSA_HASH160 + && key.data().as_slice() == new_payout_address) - .find(|(_, key)| key.data().as_slice() == new_platform_node_id) + .find(|(_, key)| key.purpose() == Purpose::SYSTEM + && key.key_type() == KeyType::EDDSA_25519_HASH160 + && key.data().as_slice() == new_platform_node_id)Also applies to: 140-152, 249-264, 341-348, 365-371
379-385: Bug: wrong variable used when creating platform node key (can panic).Inside the identity-exists branch, key data is taken from
platform_node_id_changeinstead ofnew_platform_node_id. Ifnew_platform_node_idcame from the old state (i.e., no change),platform_node_id_changeis None and this panics.Apply:
- // TODO: should be `new_platform_node_id` - data: BinaryData::new( - platform_node_id_change - .as_ref() - .expect("platform node id confirmed is some") - .to_vec(), - ), + data: BinaryData::new(new_platform_node_id.to_vec()),
🧹 Nitpick comments (4)
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/mod.rs (1)
20-26: Docs read well; dispatch and parameter semantics are clear.LGTM. Consider consistently using “payout address” (avoid mixing with “withdrawal” elsewhere) for uniform terminology across modules.
Also applies to: 29-36
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs (3)
170-173: Variable naming nit: this tracks node ID, not a pub key.Rename for clarity.
- if let Some(old_operator_pub_key_to_re_enable) = old_operator_node_id_to_re_enable { + if let Some(old_operator_node_id_key_to_re_enable) = old_operator_node_id_to_re_enable { - keys_to_re_enable.push(old_operator_pub_key_to_re_enable); + keys_to_re_enable.push(old_operator_node_id_key_to_re_enable);
216-217: Remove stale commented increment.The extra increment is unnecessary and confusing here; please delete the commented line.
- // new_key_id += 1;
416-1196: Add tests for edge cases not covered.Please add:
- Clearing payout address (operator_payout_address_change = Some(None)) for both same-identity and identity-switch paths.
- Combined changes in one call (e.g., rotate operator key + change payout + change node ID).
- Regression for the platform-node-id construction bug fixed above.
I can draft these if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs(15 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/mod.rs
packages/rs-*/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
packages/rs-*/**/*.rs: Use rustfmt defaults for Rust code (format via cargo fmt --all)
Keep Rust code clippy-clean (cargo clippy)
Name Rust modules in snake_case
Use PascalCase for Rust types
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/mod.rs
🧠 Learnings (1)
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2215
File: packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs:19-30
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the Rust file `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/create_owner_identity/v1/mod.rs`, within the `create_owner_identity_v1` function, the `add_public_keys` method of the `Identity` struct cannot fail and does not require explicit error handling.
Applied to files:
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rspackages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Rust crates security audit
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (2)
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_identities/update_operator_identity/v0/mod.rs (2)
46-52: Early bail-out is correct and avoids unnecessary IO.Nice guard to skip work when no operator-related changes were reported.
311-327: Use of add_public_keys is correct (no error path).Calling
identity.add_public_keys(...)without handling an error is fine; only key construction can fail here.Based on learnings
| purpose: Purpose::SYSTEM, | ||
| security_level: SecurityLevel::CRITICAL, | ||
| read_only: true, | ||
| // TODO: should be `new_platform_node_id` |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue being fixed or feature implemented
Function description for update_operator_identity had errors and was needing more inline comments
What was done?
Add docs for update_operator_identity and update_operator_identity_v0
How Has This Been Tested?
NA
Breaking Changes
NA
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Bug Fixes
Documentation