fix(dpp): correct revision in identity update transition #2940
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
The `try_from_identity_with_signer` function was setting the wrong revision value in `IdentityUpdateTransitionV0`. It used `identity.revision()` but Platform validation expects `identity.revision() + 1`. This caused all identity updates via this method to fail with "Identity has invalid revision" errors. The fix aligns with the correct pattern already used in `identity_factory.rs:265`. Added regression test to prevent future occurrences. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe revision field initialization in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs (1)
209-252: LGTM! Effective regression test with good coverage.The test validates the fix across multiple revision scenarios and includes clear documentation explaining the regression. Testing both revision 0 and revision 5 provides confidence in the correctness of the pattern.
Optional: Consider testing the actual function
The current test validates the pattern by constructing transitions manually, which is appropriate given that
try_from_identity_with_signerrequires aSignerimplementation and is feature-gated. If you have a mockSigneravailable for testing, you could add an integration test that callstry_from_identity_with_signerdirectly to verify the fix in the actual code path. However, the current approach is acceptable for regression testing.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
🧠 Learnings (8)
📓 Common learnings
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2673
File: packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs:1164-1197
Timestamp: 2025-06-18T03:44:14.385Z
Learning: QuantumExplorer determined that a CodeRabbit suggestion about fixing signable_bytes calculation in identity update tests with contract-bound keys was incorrect - the code flow is working as intended without the suggested changes.
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
📚 Learning: 2024-10-08T13:28:03.529Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-drive-abci/src/platform_types/platform_state/mod.rs:141-141
Timestamp: 2024-10-08T13:28:03.529Z
Learning: When converting `PlatformStateV0` to `PlatformStateForSavingV1` in `packages/rs-drive-abci/src/platform_types/platform_state/mod.rs`, only version `0` needs to be handled in the match on `platform_state_for_saving_structure_default` because the changes are retroactive.
Applied to files:
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
📚 Learning: 2024-10-06T16:11:34.946Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 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-06T16:11:34.946Z
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-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
📚 Learning: 2024-11-20T20:43:41.185Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/tests/strategy_tests/masternodes.rs:212-220
Timestamp: 2024-11-20T20:43:41.185Z
Learning: In `packages/rs-drive-abci/tests/strategy_tests/masternodes.rs`, the pattern of generating a `PrivateKey`, converting it to bytes, and reconstructing a `BlsPrivateKey` from those bytes is intentional. Avoid suggesting to simplify this code in future reviews.
Applied to files:
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
📚 Learning: 2024-10-21T01:03:42.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2227
File: packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:299-299
Timestamp: 2024-10-21T01:03:42.458Z
Learning: In the `test_serialize_deserialize_validator_set_v0` test within `packages/rs-dpp/src/core_types/validator_set/v0/mod.rs`, deterministic BLS keys cannot be easily used; therefore, using `BlsPublicKey::generate()` is acceptable.
Applied to files:
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs
📚 Learning: 2024-11-20T16:05:40.200Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2257
File: packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs:148-151
Timestamp: 2024-11-20T16:05:40.200Z
Learning: In `packages/rs-drive-abci/src/platform_types/signature_verification_quorum_set/v0/for_saving.rs`, when converting public keys from `QuorumForSavingV0` to `VerificationQuorum`, it's acceptable to use `.expect()` for public key conversion, as the conversion has been verified and panics are acceptable in this context.
Applied to files:
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.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). (16)
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (wasm-dpp2) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (2)
packages/rs-dpp/src/state_transition/state_transitions/identity/identity_update_transition/v0/v0_methods.rs (2)
64-64: LGTM! Critical fix correctly increments revision.The revision is now correctly set to
identity.revision() + 1, which resolves the "Identity has invalid revision" validation error. This aligns with the standard pattern for state transitions that modify identity state.
174-202: LGTM! Well-structured test helper.The helper function appropriately constructs test identities with configurable revision values. The hardcoded key data and identity ID are acceptable for unit testing purposes.
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
The
try_from_identity_with_signerfunction was setting the wrong revision value inIdentityUpdateTransitionV0. It usedidentity.revision()but Platform validation expectsidentity.revision() + 1.This caused all identity updates via this method to fail with "Identity has invalid revision" errors.
What was done?
The fix aligns with the correct pattern already used in
identity_factory.rs:265.How Has This Been Tested?
Added regression test to prevent future occurrences.
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.