fix(drive): proved identity update was giving error #2642
Conversation
WalkthroughA new test was added to validate adding an authentication key to an identity using an identity update state transition. Additionally, a boolean parameter was changed from Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Identity as Identity
participant Platform as Platform
participant Proof as Proof System
Test->>Identity: Generate new authentication key
Test->>Identity: Create identity update transition (add key)
Test->>Platform: Submit transition (signed)
Platform->>Platform: Process transition and commit
Platform->>Proof: Generate proof of execution
Proof->>Test: Return proof
Test->>Proof: Verify proof
Proof->>Test: Return verification result
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
QuantumExplorer
changed the title
![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
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs (1)
229-367: Excellent comprehensive test for authentication key additionThis test thoroughly validates the end-to-end flow of adding an authentication key to an identity update transition. The implementation correctly:
- Generates a new ECDSA SECP256K1 key pair
- Creates a properly configured
IdentityPublicKeyInCreationV0with authentication purpose- Signs the new key data with its own secret key
- Signs the overall transition with the existing identity key
- Processes the transition through the platform
- Generates and verifies cryptographic proof
- Asserts proper verification results
The test structure follows established patterns and includes appropriate error handling.
Consider making the key ID assignment more explicit to avoid potential conflicts:
- let mut new_key = IdentityPublicKeyInCreationV0 { - id: 2, + let mut new_key = IdentityPublicKeyInCreationV0 { + id: 2, // Using ID 2 since master key uses ID 1This makes it clear that the key ID choice is intentional and doesn't conflict with the existing master key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs(2 hunks)packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / 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 JS packages / Build JS
🔇 Additional comments (2)
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs (1)
865-865:✅ Verification successful
LGTM - Targeted fix for proof verification
This boolean parameter change aligns with the PR objectives to fix proof verification for identity updates that add authentication keys. The change from
truetofalseappears to be a deliberate correction based on user feedback.To ensure this change is correct, it would be helpful to verify what this third boolean parameter controls in the
verify_identity_keys_by_identity_idfunction:
🏁 Script executed:
#!/bin/bash # Find the function signature to understand what the third boolean parameter controls rg -A 10 "fn verify_identity_keys_by_identity_id"Length of output: 2729
Approved: Correctly disable revision verification in IdentityUpdate ST
Verified that the third boolean argument is
with_revisioninverify_identity_keys_by_identity_id. Disabling the revision check (true → false) when processing an IdentityUpdate is intentional and aligns with the PR’s objectives.• Location:
packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs:865
• Change: with_revision toggled tofalseto skip verifying the old revision during an identity updateNo further changes needed.
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs (1)
126-143: LGTM - Necessary imports for the new testThe added imports properly support the new test functionality:
signerfor signing key dataIdentifier,ECDSA_SECP256K1for key handlingStateTransitionProofResult,IdentityPublicKeyInCreationfor state transition operationsDrivefor proof verificationAll imports are relevant and follow the existing import organization pattern.
|
Self Reviewed |
Issue being fixed or feature implemented
Added a test to verify the functionality of identity updates that include adding an authentication key.
Fixed that proved identity update was giving error.
What was done?
test_identity_update_that_adds_an_authentication_keyto check the process of adding an authentication key to an identity.verify_state_transition_was_executed_with_proofmethod to correct a boolean parameter fromtruetofalsebased on user feedback regarding an error in the proof process for identity updates.How Has This Been Tested?
The new test case was executed within the existing test suite, confirming that the identity update process works correctly when adding an authentication key.
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
Tests
Bug Fixes