fix(drive-abci): fixed issue with adding a key with contract bounds #2673
Conversation
WalkthroughThis update introduces a new error type for invalid key purposes in contract bounds, integrates it into consensus error handling, and adds comprehensive tests for identity updates involving contract-bound keys. It also adds helper functions for handling optional identity public keys, adjusts dependency specifications, and refines test and query logic for identity key management. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Platform as Platform
participant Identity as Identity
participant Contract as DataContract
Test->>Platform: setup_identity_return_master_key()
Platform-->>Test: (Identity, Signer, CriticalKey, MasterKey)
Test->>Platform: register_contract_from_bytes(bytes, IdentityTestInfo, version)
Platform->>Contract: Deserialize contract from bytes
Platform->>Identity: Generate or use provided identity/signer/key
Platform->>Contract: Set contract ID using identity & nonce
Platform->>Platform: Create & sign DataContractCreateTransition
Platform->>Platform: Process state transition (GroveDB tx)
Platform-->>Test: DataContract
Test->>Platform: Create IdentityUpdateTransition (add contract-bound key)
Test->>Platform: Process state transition (GroveDB tx)
Platform-->>Test: Result (success/failure)
Test->>Platform: Commit transaction
Test->>Platform: Fetch and verify updated identity keys
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ 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: 3
🔭 Outside diff range comments (1)
packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v0/mod.rs (1)
248-256:DECRYPTIONbranch erroneously calls the encryption requirement helper
document_type.requires_identity_encryption_bounded_key()is invoked inside theDECRYPTIONarm.
This should be the symmetricrequires_identity_decryption_bounded_key()to respect contract-level settings.- let Some(requirements) = document_type - .requires_identity_encryption_bounded_key() + let Some(requirements) = document_type + .requires_identity_decryption_bounded_key()
🧹 Nitpick comments (13)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs (1)
2-2: Feature-gated import placement
The#[cfg(feature = "token-reward-explanations")]attribute correctly scopes the next import. Consider gating the unconditionalPlatformVersionimport (line 3) as well to avoid unused import warnings when the feature is disabled.packages/wasm-dpp/src/errors/consensus/consensus_error.rs (2)
67-68: Minor: keepuselist sorted for readability
The long import block for basic :: identity errors is already alphabetised; the newInvalidKeyPurposeForContractBoundsErrorentry breaks that order. Consider re-sorting to preserve quick-scan readability.
850-852: Exposed only via the generic wrapper – consider a typed Wasm error
InvalidKeyPurposeForContractBoundsErroris surfaced throughgeneric_consensus_error!, whereas most identity-related errors have dedicated*ErrorWasmwrappers. A typed wrapper keeps parity with the rest of the API and allows JS/TS consumers to pattern-match without stringly-typed inspection.Optional, but aligning here avoids special-casing on the frontend.
packages/rs-dpp/src/errors/consensus/basic/basic_error.rs (1)
592-594: Variant added without dedicated doc comment
Most variants have a short description (useful for autogenerated docs). Consider adding one here for consistency:-InvalidKeyPurposeForContractBoundsError(InvalidKeyPurposeForContractBoundsError), +/// Key purpose is not allowed for the supplied contract/document bounds +InvalidKeyPurposeForContractBoundsError(InvalidKeyPurposeForContractBoundsError),packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs (1)
3531-3533: Align tuple unpacking with updated function signature
Thesetup_identity_return_master_keynow returns four values; you’re correctly discarding the extra value. For clarity, you could name the ignored element (e.g.let (identity, signer, _master_key, key) = …) so readers immediately understand what’s being skipped.packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v0/mod.rs (1)
6-8: Minor: remove unused import
DataContractBoundsNotPresentErroris imported but never used in this file after refactor.
Consider dropping it to keep the import list lean.packages/rs-dpp/src/errors/consensus/basic/identity/invalid_key_purpose_for_contract_bounds_error.rs (1)
38-40: Expose a slice instead of&VecReturning
&Vec<Purpose>ties callers to the concrete collection type.
Exposing a slice improves flexibility and avoids unnecessaryclone()calls down the line.- pub fn allowed_key_purposes(&self) -> &Vec<Purpose> { - &self.allowed_key_purposes + pub fn allowed_key_purposes(&self) -> &[Purpose] { + &self.allowed_key_purposes }packages/rs-drive/src/drive/identity/key/fetch/mod.rs (2)
198-205: Duplicate helper, consider DRY-ing
element_to_identity_public_key_id_and_some_object_pairdiffers from the existing
element_to_identity_public_key_id_and_object_paironly by wrapping the key inSome.
You can reuse the original helper and wrap at the call-site to reduce duplication.
615-624: Remove now-redundantNotSupportedguard
KeyIDOptionalIdentityPublicKeyPairBTreeMap::try_from_query_resultsis implemented,
but the earlierKeyIDOptionalIdentityPublicKeyPairVecstill returnsNotSupported.
If vector results are also needed, implement them for consistency or add a comment
explaining why only the map variant is required.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs (2)
684-685: Inline-hex payload inflates the test file – consider externalisingThe 1 300-byte hex string embedded here makes the test hard to scan and increases compile-time parsing.
Prefer moving the byte blob to a fixture:let contract_bytes = include_bytes!("fixtures/preorder_contract.hex"); let contract_bytes = hex::decode(contract_bytes).expect("valid hex");This keeps the test source concise while preserving determinism.
1111-1112: Same large hex literal duplicated – DRY it upThis second contract definition duplicates the concern raised above. Storing both payloads under
tests/fixtures/…and loading them withinclude_bytes!(orlazy_static!) will reduce duplication and compilation overhead.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs (2)
777-786: Add docs / derives to the newIdentityTestInfoenumA short doc-comment explaining each variant will help future maintainers.
Also consider#[derive(Debug)]; it’s handy in tests and does not impose extra requirements.
788-871:register_contract_from_bytes– avoid extra cloning & ensure owner consistency
- The call to
into_partial_identity_info()need not clone the entire identity:- &identity_cow.as_ref().clone().into_partial_identity_info(), + &identity_cow.as_ref().into_partial_identity_info(),
- After
data_contract.set_id(contract_id);the contract’s owner_id remains whatever was in the raw bytes.
If the fixture wasn’t authored byidentity_cow, this mismatch could surface later (e.g., fee calculation, auth checks).
Confirm that the bytes always encode the same owner you pass here, or explicitly set it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs(1 hunks)packages/rs-dpp/src/errors/consensus/basic/basic_error.rs(2 hunks)packages/rs-dpp/src/errors/consensus/basic/identity/invalid_key_purpose_for_contract_bounds_error.rs(1 hunks)packages/rs-dpp/src/errors/consensus/basic/identity/mod.rs(2 hunks)packages/rs-dpp/src/errors/consensus/codes.rs(1 hunks)packages/rs-drive-abci/src/execution/check_tx/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/v0/mod.rs(2 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/direct_selling/mod.rs(0 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs(0 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs(5 hunks)packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs(6 hunks)packages/rs-drive/Cargo.toml(1 hunks)packages/rs-drive/src/drive/identity/key/fetch/mod.rs(3 hunks)packages/rs-drive/src/util/grove_operations/grove_get_path_query_with_optional/v0/mod.rs(1 hunks)packages/rs-platform-version/Cargo.toml(1 hunks)packages/wasm-dpp/src/errors/consensus/consensus_error.rs(2 hunks)
💤 Files with no reviewable changes (2)
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/token/direct_selling/mod.rs
- packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/transformer/v0/mod.rs
🧰 Additional context used
🧠 Learnings (3)
📓 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.
packages/rs-platform-version/Cargo.toml (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2431
File: packages/rs-drive/Cargo.toml:55-60
Timestamp: 2025-01-19T07:36:46.042Z
Learning: The grovedb dependencies in packages/rs-drive/Cargo.toml and related files are intentionally kept at specific revisions rather than using the latest stable version, with plans to update them at a later time.
packages/rs-drive/Cargo.toml (1)
Learnt from: QuantumExplorer
PR: dashpay/platform#2431
File: packages/rs-drive/Cargo.toml:55-60
Timestamp: 2025-01-19T07:36:46.042Z
Learning: The grovedb dependencies in packages/rs-drive/Cargo.toml and related files are intentionally kept at specific revisions rather than using the latest stable version, with plans to update them at a later time.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (wasm-dpp) / Linting
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (11)
packages/rs-platform-version/Cargo.toml (1)
14-14: ```shell
#!/bin/bashecho "=== Checking crates.io for grovedb-version 3.0.0 ==="
if curl -s https://crates.io/api/v1/crates/grovedb-version | grep -q '"num":"3.0.0"'; then
echo "✅ grovedb-version 3.0.0 is published on crates.io"
else
echo "❌ grovedb-version 3.0.0 not found on crates.io"
fiecho
echo "=== Locating grovedb-version in Cargo.lock ==="
awk '/name = "grovedb-version"/ { print; getline; print }' Cargo.lock || echo "❌ grovedb-version block not found in Cargo.lock"echo
echo "=== Listing all grovedb* dependencies with version fields in Cargo.toml ==="
rg -n 'grovedb(-[a-z-]+)? = {[^}]version' --glob '.toml'</details> <details> <summary>packages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/distribution_function/evaluate_interval.rs (3)</summary> `6-6`: **DistributionFunction import** Importing `DistributionFunction` outside of any feature flag is appropriate for its core usage in the module. --- `8-8`: **Feature-gated `MAX_DISTRIBUTION_CYCLES_PARAM` import** The `MAX_DISTRIBUTION_CYCLES_PARAM` constant is correctly gated to the `token-reward-explanations` feature and matches its usage in explanation code. --- `12-15`: **Chrono and timezone crate imports** The `chrono` and `chrono_tz` imports are properly feature-gated, aligning with their use in time formatting functions. </details> <details> <summary>packages/rs-dpp/src/errors/consensus/codes.rs (1)</summary> `203-205`: **Confirm code-space continuity for the new error (10533)** `InvalidKeyPurposeForContractBoundsError` is slotted at 10533, immediately after 10532 and before the next reserved block (10600 – 10699). Looks correct, but please double-check that: 1. No other pending PR introduces a conflicting 10533. 2. Protocol-version docs (if any) list the new code. </details> <details> <summary>packages/rs-drive/src/util/grove_operations/grove_get_path_query_with_optional/v0/mod.rs (1)</summary> `24-24`: **Behaviour change: `allow_subtrees_of_trees` flipped to `false`** In GroveDB 3.0.0 `query_keys_optional()` changed semantics: `(path_query, include_subtrees, proof, allow_subtrees_of_trees, …)` Switching the 3ʳᵈ boolean from `true`→`false` will now SKIP keys that sit under subtree elements. Please verify every call-site that depended on the old behaviour; silent data omission can be hard to spot in higher layers. </details> <details> <summary>packages/rs-dpp/src/errors/consensus/basic/basic_error.rs (1)</summary> `69-71`: **New variant imported – make sure versioning rules are honoured** Adding a variant is fine as it’s appended, but the header comment warns about variant-order changes & protocol versions. Ensure the protocol version bump accompanying GroveDB 3.0.0 also advertises support for this new error so mixed-version nodes don’t panic on unknown discriminants. </details> <details> <summary>packages/rs-drive/Cargo.toml (1)</summary> `55-60`: **Large dependency bump – run a full feature-matrix build** All `grovedb*` crates jump to 3.0.0. Aside from compilation, please run: 1. `cargo test --no-default-features --features <each custom feature>` 2. Integration tests that hit the `grovedb` storage back-end (e.g. Drive-ABCI). This guards against accidental feature-flag drift (several optional flags were removed in GroveDB 3). </details> <details> <summary>packages/rs-drive/src/drive/identity/key/fetch/mod.rs (1)</summary> `278-289`: **Conversion always wraps `Some`, losing the ability to represent ‘missing’ keys** `supported_query_result_element_to_identity_public_key_id_and_some_object_pair` cannot yield a `(key_id, None)` tuple. If `grovedb` ever includes results for absent elements (e.g. via key-only results), the information will be lost. Please verify that `QueryResultElement` cannot represent a missing element; otherwise retain the previous `NotSupported` or handle the `None` case explicitly. </details> <details> <summary>packages/rs-dpp/src/errors/consensus/basic/identity/mod.rs (1)</summary> `29-29`: **LGTM – new error re-export appropriately added** The public export for `invalid_key_purpose_for_contract_bounds_error` is in the correct alphabetical position and matches the module declaration below. </details> <details> <summary>packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs (1)</summary> `260-265`: I’ll re-run the search with a fixed parsing approach to skip the definition and locate any remaining call-sites: ```shell #!/usr/bin/env bash set -euo pipefail # Find all references to the helper (including the definition), extract file:line, # then skip the definition in mod.rs and show context for each actual use rg -n 'setup_identity_return_master_key\(' --type rust \ | cut -d: -f1-2 \ | sort -u \ | grep -v 'packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/mod.rs:260' \ | while IFS=: read -r file line; do echo "---- $file:$line ----" sed -n "$((line-2)),$((line+2))p" "$file" done
...on/validation/state_transition/common/validate_identity_public_key_contract_bounds/v0/mod.rs
Show resolved
Hide resolved
...rive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs
Show resolved
Hide resolved
...rive-abci/src/execution/validation/state_transition/state_transitions/identity_update/mod.rs
Show resolved
Hide resolved
QuantumExplorer
changed the title
2 participants
Issue being fixed or feature implemented
Adding a key with contract bounds would not work, this was because the inner paths did not yet exist.
What was done?
grovedband related dependencies inCargo.tomlfrom a git source to version 3.0.0.Cargo.lockfile accordingly to reflect the new dependency versions.How Has This Been Tested?
The changes have been tested by running existing unit and integration tests to ensure that the application functions correctly with the updated dependencies.
Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
Refactor