feat(platform): better contract state transitions#3021
Open
QuantumExplorer wants to merge 10 commits intov3.1-devfrom
Open
feat(platform): better contract state transitions#3021QuantumExplorer wants to merge 10 commits intov3.1-devfrom
QuantumExplorer wants to merge 10 commits intov3.1-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces version 1 (V1) support for data contract state transitions, enabling delta-based updates alongside existing full-contract V0 transitions. Changes include new validation methods for keywords, tokens, and schema definitions; a contract update application mechanism; comprehensive V1 validation logic in the ABCI layer; and corresponding updates to platform version configuration and WebAssembly bindings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
QuantumExplorer
changed the title
![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: 15
🧹 Nitpick comments (15)
packages/rs-dpp/src/errors/consensus/codes.rs (1)
234-235: Place these new contract-update errors in the DataContract code range.These variants are mapped in the
10800-10899Address block, but they represent data-contract update validation. Keeping them in the DataContract range avoids range-based misclassification in downstream handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/errors/consensus/codes.rs` around lines 234 - 235, The two enum variants Self::DataContractUpdateTransitionOverlappingFieldsError and Self::DataContractUpdateTransitionConflictingKeywordError are currently mapped to 10818/10819 (Address block) but should be in the DataContract code range; update their numeric mappings in codes.rs to use the DataContract range (choose the next free codes in the DataContract block, e.g., 10418 and 10419 or other unused 10400-10499 values) so these contract-update validation errors are classified under DataContract codes.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/basic_structure/v0/mod.rs (1)
23-31: Avoid fail-open behavior when V1 reaches the V0 validator.If
DataContractUpdateTransition::V1(_)reaches this path, returningSimpleConsensusValidationResult::new()silently accepts it. Prefer an explicit version-mismatch validation error so misrouting fails closed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/basic_structure/v0/mod.rs` around lines 23 - 31, The match arm that currently returns Ok(SimpleConsensusValidationResult::new()) for DataContractUpdateTransition::V1(_) should instead produce an explicit version-mismatch validation failure so V1 transitions don't silently pass V0 validation; replace the early successful return with construction and return of a SimpleConsensusValidationResult containing a version-mismatch validation error (or return the appropriate Err validation result used elsewhere in this module) so callers see an explicit error when DataContractUpdateTransition::V1 reaches the V0 validator (refer to DataContractUpdateTransition::V0, DataContractUpdateTransition::V1, and SimpleConsensusValidationResult).packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs (1)
175-179: Consider sourcing groups fromnew_data_contractdirectly in this V0 path.Because Line 482 now hard-fails V1 input in
transform_into_action_v0, the V1 arm here is effectively unreachable and can drift. Pulling fromnew_data_contract.groups()would keep this validator’s source-of-truth tighter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs` around lines 175 - 179, The current V0 arm pulls groups from v0.data_contract.groups(), which can diverge from the authoritative post-update contract; change the V0 branch in the match over DataContractUpdateTransition (where DataContractUpdateTransition::V0(v0) => ...) to source groups from v0.new_data_contract.groups() (i.e., use the new_data_contract on the V0 struct) so the validator always uses the updated contract as the single source-of-truth; leave the V1 arm as-is or remove if unreachable.packages/rs-drive/src/util/batch/drive_op_batch/contract.rs (1)
96-107:UpdateContractWithKnownDiffdoes not currently use the known diff.Line 100 forwards only
new_contracttoapply_contract_operations, whileold_contract(Line 52) is ignored. Consider either wiring a diff-aware apply path or simplifying the variant shape to match current behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive/src/util/batch/drive_op_batch/contract.rs` around lines 96 - 107, The variant DataContractOperationType::UpdateContractWithKnownDiff is ignoring the known diff/old_contract and simply calls drive.apply_contract_operations with new_contract; update the handling so the known diff is used: either modify drive.apply_contract_operations (or add a new method like apply_contract_operations_with_diff) to accept the old_contract/diff and pass those values from UpdateContractWithKnownDiff, or simplify the enum by removing old_contract/diff fields and rename the variant to UpdateContract to reflect current behavior; ensure you update the match arm for UpdateContractWithKnownDiff to call the diff-aware method (or adjust the variant construction/consumers) so the code and variant shape remain consistent (refer to UpdateContractWithKnownDiff, new_contract, old_contract, and apply_contract_operations).packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/methods/registration_cost/mod.rs (1)
47-55: Avoid cloning fulldocument_schemasin the V0 fee path.Line 47-Line 51 clones the whole schema map only to read from it, which adds unnecessary allocation during fee estimation.
♻️ Suggested refactor
+use std::collections::BTreeMap; + mod v1; @@ DataContractCreateTransition::V0(v0) => { - let document_schemas = v0 - .data_contract - .document_schemas() - .cloned() - .unwrap_or_default(); + let empty_document_schemas = BTreeMap::new(); + let document_schemas = v0 + .data_contract + .document_schemas() + .unwrap_or(&empty_document_schemas); Ok(registration_cost_from_fields( - &document_schemas, + document_schemas, v0.data_contract.tokens(), v0.data_contract.keywords().len(), base_fee,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/methods/registration_cost/mod.rs` around lines 47 - 55, The code clones the entire document_schemas map before calling registration_cost_from_fields, causing unnecessary allocation; instead, obtain a reference to the existing map and pass that reference (or iterate over its entries) into registration_cost_from_fields rather than cloning—locate the use around v0.data_contract.document_schemas(), and change the call site that builds document_schemas to use a borrowed value (or an iterator) when invoking registration_cost_from_fields(&document_schemas, v0.data_contract.tokens(), v0.data_contract.keywords().len()) so no full clone is performed.packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/identity_signed.rs (1)
7-17: Indentation uses 2 spaces instead of 4 spaces.The method bodies use 2-space indentation, but the coding guidelines require 4-space indent for Rust files.
♻️ Proposed fix for indentation
impl StateTransitionIdentitySigned for DataContractCreateTransitionV1 { fn signature_public_key_id(&self) -> KeyID { - self.signature_public_key_id + self.signature_public_key_id } fn set_signature_public_key_id(&mut self, key_id: crate::identity::KeyID) { - self.signature_public_key_id = key_id + self.signature_public_key_id = key_id } fn security_level_requirement(&self, _purpose: Purpose) -> Vec<SecurityLevel> { - vec![CRITICAL, HIGH] + vec![CRITICAL, HIGH] } }As per coding guidelines: "Use 4-space indent for Rust files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/identity_signed.rs` around lines 7 - 17, Indentation in the methods signature_public_key_id, set_signature_public_key_id, and security_level_requirement uses 2 spaces; update each method body to use 4-space indentation to comply with Rust coding guidelines (adjust the lines inside fn signature_public_key_id(&self) -> KeyID { ... }, fn set_signature_public_key_id(&mut self, key_id: crate::identity::KeyID) { ... }, and fn security_level_requirement(&self, _purpose: Purpose) -> Vec<SecurityLevel> { ... } so the inner statements are indented with four spaces).packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/value_conversion.rs (1)
81-90: Use an update-transition default version source here.
from_objectandfrom_value_mapcurrently default viacontract_create_state_transition. In this update-transition converter, that cross-type coupling is brittle and can deserialize to the wrong default if create/update defaults diverge.♻️ Suggested fix
- .contract_create_state_transition + .contract_update_state_transitionApply in both fallback blocks.
Also applies to: 109-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/value_conversion.rs` around lines 81 - 90, The code is defaulting to contract_create_state_transition versions in the update-transition converter; change both fallback uses so they read the default_current_version from platform_version.dpp.state_transition_serialization_versions.contract_update_state_transition (not contract_create_state_transition) in the value conversion functions (e.g., the blocks that set version in from_object/from_value_map in value_conversion.rs) so the update transition uses its own default version source.packages/strategy-tests/src/lib.rs (1)
1807-1807: Consider using the delta constructor here to exercise V1 behavior.This call explicitly emits a V0 update transition, which can underrepresent the protocol-default V1 update path in strategy runs. If the goal is “current behavior” simulation, prefer constructing update transitions from old/new contracts.
♻️ Suggested direction
- DataContractUpdateTransition::from_data_contract_v0( - DataContract::V0(contract_ref.clone()), - *identity_contract_nonce, - platform_version, - ) + DataContractUpdateTransition::from_contract_update( + &old_data_contract, + &DataContract::V0(contract_ref.clone()), + *identity_contract_nonce, + platform_version, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/strategy-tests/src/lib.rs` at line 1807, The test is forcing a V0 update transition via DataContractUpdateTransition::from_data_contract_v0; instead construct the update transition using the “delta” (old/new contract) constructor to exercise V1 behavior — replace the call to DataContractUpdateTransition::from_data_contract_v0(DataContract::V0(contract_ref.clone()), *identity_contract_nonce, platform_version) with the update-transition constructor that builds from the previous and new DataContract instances (e.g., DataContractUpdateTransition::from_old_and_new(old_contract, new_contract, *identity_contract_nonce, platform_version) or the crate's equivalent), ensuring you pass the prior contract and the modified contract so the produced transition uses the delta/V1 path.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/mod.rs (1)
39-55: Move theaction.is_some()guard before the version switch.The same guard is duplicated in both version branches; hoisting it reduces drift risk.
Refactor sketch
- match platform_version + if action.is_some() { + return Err(Error::Execution(ExecutionError::CorruptedCodeExecution( + "data contract update is calling validate state, and the action is already known. It should not be known at this point", + ))); + } + + match platform_version .drive_abci .validation_and_processing .state_transitions .contract_update_state_transition .state { 0 => { - if action.is_some() { - return Err(Error::Execution(ExecutionError::CorruptedCodeExecution("data contract update is calling validate state, and the action is already known. It should not be known at this point"))); - } self.validate_state_v0( platform, block_info, @@ 1 => { - if action.is_some() { - return Err(Error::Execution(ExecutionError::CorruptedCodeExecution("data contract update is calling validate state, and the action is already known. It should not be known at this point"))); - } match self {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/mod.rs` around lines 39 - 55, The duplicated guard action.is_some() in both match arms should be hoisted before the version match to avoid duplication and drift: in the state transition handling code where you currently branch on version (the branches that call validate_state_v0 and validate_state_v1), check action.is_some() once at the top and return the same ExecutionError if true, then proceed to the match to call validate_state_v0 or validate_state_v1 as appropriate; remove the duplicate guard from each branch so only the single pre-match check remains.packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/mod.rs (2)
127-145: Extract shared mapping logic for V1 create-transition construction.The two conversion impls duplicate nearly identical field mapping. A private helper would reduce maintenance drift.
Refactor sketch
+impl DataContractCreateTransitionV1 { + fn from_data_contract_parts( + data_contract: DataContract, + identity_nonce: IdentityNonce, + ) -> Self { + let contract_system_version = match &data_contract { + DataContract::V0(_) => 0, + DataContract::V1(_) => 1, + }; + Self { + contract_system_version, + owner_id: data_contract.owner_id(), + config: *data_contract.config(), + schema_defs: data_contract.schema_defs().cloned(), + document_schemas: data_contract + .document_schemas() + .into_iter() + .map(|(k, v)| (k, v.clone())) + .collect(), + groups: data_contract.groups().clone(), + tokens: data_contract.tokens().clone(), + keywords: data_contract.keywords().clone(), + description: data_contract.description().cloned(), + identity_nonce, + user_fee_increase: 0, + signature_public_key_id: 0, + signature: Default::default(), + } + } +}Also applies to: 161-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/mod.rs` around lines 127 - 145, Extract the duplicated field-mapping into a private helper used by both conversion impls: create a function (e.g. map_create_transition_common) that accepts the source DataContract (and contract_system_version if needed) and returns a DataContractCreateTransitionV1 constructed with the shared fields (owner_id, config, schema_defs, document_schemas mapping, groups, tokens, keywords, description, identity_nonce, user_fee_increase, signature_public_key_id, signature). Replace the two inline constructions at the current block (the Ok(DataContractCreateTransitionV1 { ... })) and the other similar block around lines 161-179 to call this helper so the mapping logic is centralized and maintained in one place.
123-126: Extractcontract_system_versionassignment to use the dedicated accessor method.Both occurrences (lines 123–126 and 157–160) manually derive
contract_system_versionfrom theDataContractenum variant. Replace the match expression withdata_contract.system_version_type(), which provides the canonical system version derivation and is already used elsewhere in the codebase (e.g.,document_create_transition/v0/mod.rs). This eliminates logic duplication and reduces drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/mod.rs` around lines 123 - 126, Replace the manual match-based assignment to contract_system_version (which derives it from the DataContract enum) with the canonical accessor call data_contract.system_version_type(); locate the two occurrences where contract_system_version is computed from DataContract::V0/V1 and change them to use data_contract.system_version_type() to eliminate duplicated logic and match other usages (e.g., document_create_transition/v0).packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/accessors/mod.rs (1)
17-24: Avoid silent no-op semantics for V1set_data_contract.Returning
falseis easy to ignore at call sites. Consider a stronger contract (e.g.,Result<(), _>or#[must_use]on the trait method) so unsupported V1 mutation can’t be dropped silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/accessors/mod.rs` around lines 17 - 24, The setter currently returns bool and silently no-ops for V1; change set_data_contract on DataContractUpdateTransition to return a Result<(), YourErrorType> (or at least #[must_use] Result) so callers cannot ignore failures: in the implementation for DataContractUpdateTransition::V0(transition) return Ok(()) after assigning transition.data_contract, and for DataContractUpdateTransition::V1(_) return Err(UnsupportedVersionError) (or a descriptive error), and update the trait/signature where set_data_contract is declared to reflect the Result type so callers must handle the unsupported-V1 case.packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/value_conversion.rs (1)
31-60: Note:to_objectandto_cleaned_objecthave identical implementations.The two methods currently perform the same operations. If this is intentional for consistency with other implementations, this is fine. Otherwise,
to_cleaned_objectcould delegate toto_object.♻️ Optional: Deduplicate by delegation
fn to_cleaned_object(&self, skip_signature: bool) -> Result<Value, ProtocolError> { - let mut object: Value = platform_value::to_value(self)?; - if skip_signature { - Self::signature_property_paths() - .into_iter() - .try_for_each(|path| { - object - .remove_values_matching_path(path) - .map_err(ProtocolError::ValueError) - .map(|_| ()) - })?; - } - Ok(object) + self.to_object(skip_signature) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/value_conversion.rs` around lines 31 - 60, The implementations of to_object and to_cleaned_object on DataContractUpdateTransitionV1 are identical; change to_cleaned_object to delegate to to_object to remove duplication: replace the body of to_cleaned_object with a direct call to self.to_object(skip_signature) (propagating the Result/ProtocolError as-is) so both methods remain correct and avoid duplicate logic; ensure the signature and return type of to_cleaned_object remain unchanged and that it still implements StateTransitionValueConvert.packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/mod.rs (1)
130-238: Consider validating contract IDs match infrom_contract_update.The method assumes
old_contractandnew_contractrefer to the same contract (same ID) but doesn't validate this. If the caller passes mismatched contracts, the resulting transition would have an incorrect delta.🛡️ Optional: Add contract ID validation
pub fn from_contract_update( old_contract: &DataContract, new_contract: &DataContract, identity_nonce: IdentityNonce, ) -> Result<Self, ProtocolError> { use crate::data_contract::accessors::v1::DataContractV1Getters; + // Ensure we're updating the same contract + if old_contract.id() != new_contract.id() { + return Err(ProtocolError::CorruptedCodeExecution( + "old_contract and new_contract must have the same ID".to_string(), + )); + } + // Compute schema_defs delta🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/mod.rs` around lines 130 - 238, The from_contract_update function (DataContractUpdateTransitionV1::from_contract_update) currently assumes old_contract and new_contract have the same id; add an early validation that old_contract.id() == new_contract.id() and return a ProtocolError (or appropriate error variant) if they differ to avoid producing an invalid delta; place this check at the top of from_contract_update before computing deltas and use the existing ProtocolError type (or create a clear variant/message) so callers receive a deterministic error when IDs mismatch.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/v1_tests.rs (1)
42-43: Test names should follow 'should …' convention.Per coding guidelines, new tests should be named descriptively starting with 'should …'. All tests in this file use the
test_*prefix pattern instead. Consider renaming, for example:
test_data_contract_update_can_add_new_group_v1→should_add_new_group_in_v1_updatetest_data_contract_update_can_not_add_new_group_with_gap_v1→should_fail_adding_group_with_gap_in_v1_updatetest_overlapping_schema_defs_should_fail_v1→should_fail_with_overlapping_schema_defs_in_v1_updateAs per coding guidelines: "Name tests descriptively, starting with 'should …'"
Also applies to: 185-186, 344-345, 461-462, 602-603, 715-716, 821-822, 928-929, 1025-1026, 1120-1121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/v1_tests.rs` around lines 42 - 43, Rename the test functions that use the `test_*` prefix to follow the "should …" convention: change `test_data_contract_update_can_add_new_group_v1` → `should_add_new_group_in_v1_update`, `test_data_contract_update_can_not_add_new_group_with_gap_v1` → `should_fail_adding_group_with_gap_in_v1_update`, `test_overlapping_schema_defs_should_fail_v1` → `should_fail_with_overlapping_schema_defs_in_v1_update` (and similarly rename the other listed tests at lines mentioned in the review), then update any internal references or imports/assert messages that refer to the old test names to use the new names so compilation and test discovery continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-dpp/src/data_contract/methods/apply_update/v0/mod.rs`:
- Around line 101-104: The code inserts entries from new_document_schemas
directly into document_schemas which can overwrite existing types and bypass the
update-rule validation that only checks updated_document_schemas; modify the
apply_update logic so that before inserting each (name, schema) from
new_document_schemas you check whether document_schemas already contains name
and if so either raise an error/abort the update or treat it as an updated
schema (i.e., move/merge it into updated_document_schemas) and run the same
validation path used for updated_document_schemas; update the handling around
new_document_schemas, updated_document_schemas, and document_schemas to ensure
no existing document type can be silently replaced without validation.
- Around line 107-110: The code in apply_update (v0 mod) currently clones
self.groups() into groups and then unconditionally calls groups.insert(*pos,
group.clone()) for each (pos, group) in update_values.new_groups, which allows
overwriting existing positions; change this to reject or error when a position
already exists instead of replacing it: when iterating update_values.new_groups,
check whether groups.contains_key(pos) (or that self.groups().get(pos) is None)
and return an appropriate error/result if the slot is occupied, otherwise insert
the new group; update any function signatures or error types used by
apply_update_v0 to propagate the validation failure.
In
`@packages/rs-dpp/src/data_contract/methods/validate_schema_defs_update/v0/mod.rs`:
- Around line 47-50: The error is mislabelled: in the branch that validates
updates to existing schema defs you are constructing
IncompatibleDataContractSchemaError::new with the operation string "add" —
change that operation label to "update" (keep the same arguments: contract_id
and the path format!("/$defs/{}", schema_def_name)) so the error correctly
reflects an update failure rather than an add.
- Around line 68-123: The compatibility check is currently skipped when only
new_schema_defs is provided because the outer if uses
!updated_schema_defs.is_empty(); change the guard to run when either
updated_schema_defs or new_schema_defs is non-empty (e.g., if
!updated_schema_defs.is_empty() || !new_schema_defs.is_empty()) and keep the
existing early check for old_schema_defs (if let Some(old_defs_map)). Build
merged_defs by cloning old_defs_map and inserting both updated_schema_defs and
new_schema_defs entries (as you already do), then run
validate_schema_compatibility(&old_defs_schema, &new_defs_schema,
platform_version) and return SimpleConsensusValidationResult with
IncompatibleDataContractSchemaError items if not valid.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/accessors/mod.rs`:
- Around line 13-15: The match arms that call panic! for
DataContractCreateTransition::V1 (e.g., the data_contract() accessor in mod.rs
and the similar branches around lines 31-33) must be converted to a fallible
API: instead of panicking, have the accessor return Option or Result (matching
the pattern used for update transitions) and return None or an Err variant when
the variant is V1; update the data_contract() method signature and its callers
to accept the new return type and mirror the error/Option semantics used by the
update transition accessors so no runtime panic occurs on a legitimate variant
mismatch.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/v0_methods.rs`:
- Around line 64-75: The code uses .expect() when extracting matching_key from
identity.loaded_public_keys which can panic; replace the .expect(...) chain with
a safe conversion to Result by using
.find_map(...).ok_or(ProtocolError::NonConsensusError(...))? so the function
(which returns Result<StateTransition, ProtocolError>) propagates an appropriate
ProtocolError instead of panicking; reference the matching_key binding,
identity.loaded_public_keys iterator, and the key_id comparison when
implementing the ok_or(...) call and use the ? operator to return early on
error.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/value_conversion.rs`:
- Around line 42-55: The to_cleaned_object function currently converts self to a
Value but never normalizes it; update to_cleaned_object to call clean_value on
the produced Value (e.g., apply clean_value(&mut object) or the appropriate
clean helper used by to_object) before removing signature fields from
signature_property_paths(), and keep the existing removal logic
(remove_values_matching_path) with ProtocolError::ValueError mapping; ensure the
cleaned (normalized) object is what's returned by to_cleaned_object rather than
the raw to_value result.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/methods/registration_cost/mod.rs`:
- Line 1: The v1 submodule declared as v1 is unused: either remove the `mod v1;`
declaration or refactor to call its implementation; specifically either delete
`mod v1;` and any v1 files, or replace the current call to
`registration_cost_from_fields()` (from `data_contract_create_transition`) with
a call to `v1::registration_cost_v1()` (or delegate to it) in this module so
`registration_cost_v1` is actually used; update imports/usages in `mod.rs` to
reference `v1::registration_cost_v1` and remove the external
`registration_cost_from_fields` import if you switch to v1.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/methods/registration_cost/v1/mod.rs`:
- Around line 37-55: The code currently ignores parse errors from
index_value.to_map() and Index::try_from(), which can skip charging index
registration fees; update the loop over index_values so that on any parse
failure (either failing to map the index_value or failing to parse
Index::try_from) you still increment cost by the conservative base fee (use
fee_version.document_type_base_non_unique_index_registration_fee) to avoid
undercharging; keep the existing logic that selects contested/unique fees when
Index parsing succeeds (checking index.contested_index and index.unique), but
when parsing fails fall back to adding
document_type_base_non_unique_index_registration_fee via cost =
cost.saturating_add(...).
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/v0_methods.rs`:
- Around line 31-49: The new_from_data_contract constructor currently builds a
DataContractUpdateTransition::V1 (DataContractUpdateTransitionV1) with many
fields left empty, which can drop contract data; update new_from_data_contract
to either (A) construct and return the V0 transition instead of creating a
partial V1, or (B) fully populate the V1 struct by mapping all relevant fields
from the provided DataContract (populate new_schema_defs, new_document_schemas,
new_groups, new_tokens, add_keywords/remove_keywords, update_description,
update_contract_system_version, etc.), ensuring identifiers like id, owner_id,
revision remain set; modify the code path that creates
DataContractUpdateTransition::V1 to extract and clone the corresponding values
from data_contract or switch to the existing V0 factory to avoid silent
omissions.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/mod.rs`:
- Line 29: Replace the unstable call to PlatformVersion::latest() used in the
test (the platform_version variable) with a pinned explicit test platform
version constant (e.g. define a const TEST_PLATFORM_VERSION = <desired version>
and construct PlatformVersion from that) so the test behaviour doesn't drift;
update all occurrences (the instances using PlatformVersion::latest() at the
other noted spots) to use this constant and ensure the tests import/construct
PlatformVersion from the pinned value.
In `@packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs`:
- Around line 75-80: The current logic sets keeps_history via
st.data_contract().map(|dc| dc.config().keeps_history()).unwrap_or(false), which
defaults missing contract data to false and can choose the wrong proof path;
change this to treat absent contract data as historical (e.g., use
unwrap_or(true)) or explicitly fetch the full contract config when
data_contract() is None so keeps_history reflects the real contract — update the
assignment that defines keeps_history and the subsequent if keeps_history branch
(referencing st.data_contract(), dc.config().keeps_history(), and the if
keeps_history conditional).
In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`:
- Around line 127-141: The V1 branch (DataContractUpdateTransition::V1)
currently only ensures a contract with v1.id exists via Drive::verify_contract
and returns VerifiedDataContract without ensuring the update delta was applied;
change it to reconstruct or derive the expected post-update contract from the
prior contract and the v1 delta (or apply the v1 fields onto the prior contract
fetched from the proof), then compare relevant fields (ids, modified properties,
schema, version) against the contract returned by Drive::verify_contract and
return VerifiedDataContract only if the contract in the proof matches the
expected post-update state; use the v1 variable, the original contract from
Drive::verify_contract, and the DataContractUpdateTransition::V1 delta fields to
perform the validation and return a ProofError when mismatches are detected.
In
`@packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v3.rs`:
- Around line 73-77: The FeatureVersionBounds for document_base_state_transition
has max_version set to 0 while default_current_version is 1; update the bounds
so max_version is at least 1. Locate the document_base_state_transition struct
initialization (symbol: FeatureVersionBounds) in v3.rs (and apply the same fix
in v2.rs) and change max_version from 0 to 1 to make the bounds consistent with
default_current_version and the handling in from_document.rs.
In `@packages/wasm-dpp2/src/state_transitions/base/state_transition.rs`:
- Around line 467-482: The setOwnerId implementation currently treats a missing
embedded contract as a silent no-op; update setOwnerId to fail fast when
contract_update.data_contract() is None for update transitions instead of
returning Ok(()). In the setOwnerId method (handling contract_update), check for
data_contract() being None and return an appropriate Err (use the module's
existing error type) with a clear message indicating that V1 delta update
transitions do not include an embedded contract and owner cannot be set, rather
than silently succeeding; keep the existing handling that modifies
DataContractInSerializationFormat::V0/V1 when a contract is present and call
contract_update.set_data_contract(new_contract) as before.
---
Nitpick comments:
In `@packages/rs-dpp/src/errors/consensus/codes.rs`:
- Around line 234-235: The two enum variants
Self::DataContractUpdateTransitionOverlappingFieldsError and
Self::DataContractUpdateTransitionConflictingKeywordError are currently mapped
to 10818/10819 (Address block) but should be in the DataContract code range;
update their numeric mappings in codes.rs to use the DataContract range (choose
the next free codes in the DataContract block, e.g., 10418 and 10419 or other
unused 10400-10499 values) so these contract-update validation errors are
classified under DataContract codes.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/methods/registration_cost/mod.rs`:
- Around line 47-55: The code clones the entire document_schemas map before
calling registration_cost_from_fields, causing unnecessary allocation; instead,
obtain a reference to the existing map and pass that reference (or iterate over
its entries) into registration_cost_from_fields rather than cloning—locate the
use around v0.data_contract.document_schemas(), and change the call site that
builds document_schemas to use a borrowed value (or an iterator) when invoking
registration_cost_from_fields(&document_schemas, v0.data_contract.tokens(),
v0.data_contract.keywords().len()) so no full clone is performed.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/identity_signed.rs`:
- Around line 7-17: Indentation in the methods signature_public_key_id,
set_signature_public_key_id, and security_level_requirement uses 2 spaces;
update each method body to use 4-space indentation to comply with Rust coding
guidelines (adjust the lines inside fn signature_public_key_id(&self) -> KeyID {
... }, fn set_signature_public_key_id(&mut self, key_id: crate::identity::KeyID)
{ ... }, and fn security_level_requirement(&self, _purpose: Purpose) ->
Vec<SecurityLevel> { ... } so the inner statements are indented with four
spaces).
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/mod.rs`:
- Around line 127-145: Extract the duplicated field-mapping into a private
helper used by both conversion impls: create a function (e.g.
map_create_transition_common) that accepts the source DataContract (and
contract_system_version if needed) and returns a DataContractCreateTransitionV1
constructed with the shared fields (owner_id, config, schema_defs,
document_schemas mapping, groups, tokens, keywords, description, identity_nonce,
user_fee_increase, signature_public_key_id, signature). Replace the two inline
constructions at the current block (the Ok(DataContractCreateTransitionV1 { ...
})) and the other similar block around lines 161-179 to call this helper so the
mapping logic is centralized and maintained in one place.
- Around line 123-126: Replace the manual match-based assignment to
contract_system_version (which derives it from the DataContract enum) with the
canonical accessor call data_contract.system_version_type(); locate the two
occurrences where contract_system_version is computed from DataContract::V0/V1
and change them to use data_contract.system_version_type() to eliminate
duplicated logic and match other usages (e.g., document_create_transition/v0).
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/accessors/mod.rs`:
- Around line 17-24: The setter currently returns bool and silently no-ops for
V1; change set_data_contract on DataContractUpdateTransition to return a
Result<(), YourErrorType> (or at least #[must_use] Result) so callers cannot
ignore failures: in the implementation for
DataContractUpdateTransition::V0(transition) return Ok(()) after assigning
transition.data_contract, and for DataContractUpdateTransition::V1(_) return
Err(UnsupportedVersionError) (or a descriptive error), and update the
trait/signature where set_data_contract is declared to reflect the Result type
so callers must handle the unsupported-V1 case.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/mod.rs`:
- Around line 130-238: The from_contract_update function
(DataContractUpdateTransitionV1::from_contract_update) currently assumes
old_contract and new_contract have the same id; add an early validation that
old_contract.id() == new_contract.id() and return a ProtocolError (or
appropriate error variant) if they differ to avoid producing an invalid delta;
place this check at the top of from_contract_update before computing deltas and
use the existing ProtocolError type (or create a clear variant/message) so
callers receive a deterministic error when IDs mismatch.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/value_conversion.rs`:
- Around line 31-60: The implementations of to_object and to_cleaned_object on
DataContractUpdateTransitionV1 are identical; change to_cleaned_object to
delegate to to_object to remove duplication: replace the body of
to_cleaned_object with a direct call to self.to_object(skip_signature)
(propagating the Result/ProtocolError as-is) so both methods remain correct and
avoid duplicate logic; ensure the signature and return type of to_cleaned_object
remain unchanged and that it still implements StateTransitionValueConvert.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/value_conversion.rs`:
- Around line 81-90: The code is defaulting to contract_create_state_transition
versions in the update-transition converter; change both fallback uses so they
read the default_current_version from
platform_version.dpp.state_transition_serialization_versions.contract_update_state_transition
(not contract_create_state_transition) in the value conversion functions (e.g.,
the blocks that set version in from_object/from_value_map in
value_conversion.rs) so the update transition uses its own default version
source.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/basic_structure/v0/mod.rs`:
- Around line 23-31: The match arm that currently returns
Ok(SimpleConsensusValidationResult::new()) for
DataContractUpdateTransition::V1(_) should instead produce an explicit
version-mismatch validation failure so V1 transitions don't silently pass V0
validation; replace the early successful return with construction and return of
a SimpleConsensusValidationResult containing a version-mismatch validation error
(or return the appropriate Err validation result used elsewhere in this module)
so callers see an explicit error when DataContractUpdateTransition::V1 reaches
the V0 validator (refer to DataContractUpdateTransition::V0,
DataContractUpdateTransition::V1, and SimpleConsensusValidationResult).
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/mod.rs`:
- Around line 39-55: The duplicated guard action.is_some() in both match arms
should be hoisted before the version match to avoid duplication and drift: in
the state transition handling code where you currently branch on version (the
branches that call validate_state_v0 and validate_state_v1), check
action.is_some() once at the top and return the same ExecutionError if true,
then proceed to the match to call validate_state_v0 or validate_state_v1 as
appropriate; remove the duplicate guard from each branch so only the single
pre-match check remains.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rs`:
- Around line 175-179: The current V0 arm pulls groups from
v0.data_contract.groups(), which can diverge from the authoritative post-update
contract; change the V0 branch in the match over DataContractUpdateTransition
(where DataContractUpdateTransition::V0(v0) => ...) to source groups from
v0.new_data_contract.groups() (i.e., use the new_data_contract on the V0 struct)
so the validator always uses the updated contract as the single source-of-truth;
leave the V1 arm as-is or remove if unreachable.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/v1_tests.rs`:
- Around line 42-43: Rename the test functions that use the `test_*` prefix to
follow the "should …" convention: change
`test_data_contract_update_can_add_new_group_v1` →
`should_add_new_group_in_v1_update`,
`test_data_contract_update_can_not_add_new_group_with_gap_v1` →
`should_fail_adding_group_with_gap_in_v1_update`,
`test_overlapping_schema_defs_should_fail_v1` →
`should_fail_with_overlapping_schema_defs_in_v1_update` (and similarly rename
the other listed tests at lines mentioned in the review), then update any
internal references or imports/assert messages that refer to the old test names
to use the new names so compilation and test discovery continue to work.
In `@packages/rs-drive/src/util/batch/drive_op_batch/contract.rs`:
- Around line 96-107: The variant
DataContractOperationType::UpdateContractWithKnownDiff is ignoring the known
diff/old_contract and simply calls drive.apply_contract_operations with
new_contract; update the handling so the known diff is used: either modify
drive.apply_contract_operations (or add a new method like
apply_contract_operations_with_diff) to accept the old_contract/diff and pass
those values from UpdateContractWithKnownDiff, or simplify the enum by removing
old_contract/diff fields and rename the variant to UpdateContract to reflect
current behavior; ensure you update the match arm for
UpdateContractWithKnownDiff to call the diff-aware method (or adjust the variant
construction/consumers) so the code and variant shape remain consistent (refer
to UpdateContractWithKnownDiff, new_contract, old_contract, and
apply_contract_operations).
In `@packages/strategy-tests/src/lib.rs`:
- Line 1807: The test is forcing a V0 update transition via
DataContractUpdateTransition::from_data_contract_v0; instead construct the
update transition using the “delta” (old/new contract) constructor to exercise
V1 behavior — replace the call to
DataContractUpdateTransition::from_data_contract_v0(DataContract::V0(contract_ref.clone()),
*identity_contract_nonce, platform_version) with the update-transition
constructor that builds from the previous and new DataContract instances (e.g.,
DataContractUpdateTransition::from_old_and_new(old_contract, new_contract,
*identity_contract_nonce, platform_version) or the crate's equivalent), ensuring
you pass the prior contract and the modified contract so the produced transition
uses the delta/V1 path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c23af316-a312-4742-a59c-7fd789a98554
📒 Files selected for processing (111)
packages/rs-dpp/src/data_contract/data_contract_facade.rspackages/rs-dpp/src/data_contract/factory/mod.rspackages/rs-dpp/src/data_contract/factory/v0/mod.rspackages/rs-dpp/src/data_contract/methods/apply_update/mod.rspackages/rs-dpp/src/data_contract/methods/apply_update/v0/mod.rspackages/rs-dpp/src/data_contract/methods/mod.rspackages/rs-dpp/src/data_contract/methods/registration_cost/v1/mod.rspackages/rs-dpp/src/data_contract/methods/validate_groups/mod.rspackages/rs-dpp/src/data_contract/methods/validate_groups/v0/mod.rspackages/rs-dpp/src/data_contract/methods/validate_keywords/mod.rspackages/rs-dpp/src/data_contract/methods/validate_keywords/v0/mod.rspackages/rs-dpp/src/data_contract/methods/validate_schema_defs_update/mod.rspackages/rs-dpp/src/data_contract/methods/validate_schema_defs_update/v0/mod.rspackages/rs-dpp/src/data_contract/methods/validate_tokens/mod.rspackages/rs-dpp/src/data_contract/methods/validate_tokens/v0/mod.rspackages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rspackages/rs-dpp/src/data_contract/mod.rspackages/rs-dpp/src/data_contract/serialized_version/mod.rspackages/rs-dpp/src/data_contract/update_values/mod.rspackages/rs-dpp/src/errors/consensus/basic/basic_error.rspackages/rs-dpp/src/errors/consensus/basic/data_contract/data_contract_update_transition_conflicting_keyword_error.rspackages/rs-dpp/src/errors/consensus/basic/data_contract/data_contract_update_transition_overlapping_fields_error.rspackages/rs-dpp/src/errors/consensus/basic/data_contract/mod.rspackages/rs-dpp/src/errors/consensus/codes.rspackages/rs-dpp/src/state_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/accessors/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/identity_signed.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/json_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/methods/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/methods/registration_cost/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/methods/registration_cost/v1/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/state_transition_estimated_fee_validation.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/state_transition_like.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/identity_signed.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/json_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/state_transition_like.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/types.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/value_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/v1/version.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/value_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/version.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/accessors/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/accessors/v0/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/identity_signed.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/json_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/methods/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/methods/registration_cost/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/methods/registration_cost/v1/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/methods/update_contract_cost/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/methods/update_contract_cost/v1/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/state_transition_estimated_fee_validation.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/state_transition_like.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/identity_signed.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/json_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/mod.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/state_transition_like.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/types.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/v0_methods.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/value_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/v1/version.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/value_conversion.rspackages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_update_transition/version.rspackages/rs-drive-abci/src/execution/check_tx/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/basic_structure/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/state/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/basic_structure/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/basic_structure/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/basic_structure/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/identity_contract_nonce/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/state/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/v0_tests.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/v1_tests.rspackages/rs-drive/src/prove/prove_state_transition/v0/mod.rspackages/rs-drive/src/state_transition_action/action_convert_to_operations/contract/data_contract_update_transition.rspackages/rs-drive/src/state_transition_action/contract/data_contract_create/transformer.rspackages/rs-drive/src/state_transition_action/contract/data_contract_create/v0/transformer.rspackages/rs-drive/src/state_transition_action/contract/data_contract_update/mod.rspackages/rs-drive/src/state_transition_action/contract/data_contract_update/transformer.rspackages/rs-drive/src/state_transition_action/contract/data_contract_update/v0/transformer.rspackages/rs-drive/src/state_transition_action/contract/data_contract_update/v1/mod.rspackages/rs-drive/src/state_transition_action/contract/data_contract_update/v1/transformer.rspackages/rs-drive/src/state_transition_action/system/bump_identity_data_contract_nonce_action/transformer.rspackages/rs-drive/src/state_transition_action/system/bump_identity_data_contract_nonce_action/v0/transformer.rspackages/rs-drive/src/state_transition_action/system/bump_identity_nonce_action/transformer.rspackages/rs-drive/src/state_transition_action/system/bump_identity_nonce_action/v0/transformer.rspackages/rs-drive/src/util/batch/drive_op_batch/contract.rspackages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v1.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v2.rspackages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v3.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/mod.rspackages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v3.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rspackages/rs-platform-version/src/version/v12.rspackages/strategy-tests/src/lib.rspackages/wasm-dpp/src/data_contract/data_contract_facade.rspackages/wasm-dpp/src/data_contract/state_transition/data_contract_update_transition/mod.rspackages/wasm-dpp/src/errors/consensus/consensus_error.rspackages/wasm-dpp2/src/data_contract/transitions/update.rspackages/wasm-dpp2/src/state_transitions/base/state_transition.rs
Comment on lines
+101
to
+104
| // Add new document schemas | ||
| for (name, schema) in update_values.new_document_schemas { | ||
| document_schemas.insert(name.clone(), schema.clone()); | ||
| } |
Contributor
There was a problem hiding this comment.
Block overwriting existing document types through new_document_schemas.
At Line 101-Line 104, entries in new_document_schemas can replace existing document types. Those replacements then skip update-rule validation (Line 257-Line 283 only checks updated_document_schemas), which creates a validation bypass.
🛡️ Suggested fix
// Validate that updated document schemas correspond to existing document types
for document_type_name in update_values.updated_document_schemas.keys() {
if self
.document_type_optional_for_name(document_type_name)
.is_none()
{
return Ok(ConsensusValidationResult::new_with_error(
DocumentTypeUpdateError::new(
self.id(),
document_type_name,
"document type does not exist and cannot be updated",
)
.into(),
));
}
}
+
+ // Validate that new document schemas do not already exist
+ for document_type_name in update_values.new_document_schemas.keys() {
+ if self
+ .document_type_optional_for_name(document_type_name)
+ .is_some()
+ {
+ return Ok(ConsensusValidationResult::new_with_error(
+ DocumentTypeUpdateError::new(
+ self.id(),
+ document_type_name,
+ "document type already exists and must be provided in updated_document_schemas",
+ )
+ .into(),
+ ));
+ }
+ }Also applies to: 257-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-dpp/src/data_contract/methods/apply_update/v0/mod.rs` around
lines 101 - 104, The code inserts entries from new_document_schemas directly
into document_schemas which can overwrite existing types and bypass the
update-rule validation that only checks updated_document_schemas; modify the
apply_update logic so that before inserting each (name, schema) from
new_document_schemas you check whether document_schemas already contains name
and if so either raise an error/abort the update or treat it as an updated
schema (i.e., move/merge it into updated_document_schemas) and run the same
validation path used for updated_document_schemas; update the handling around
new_document_schemas, updated_document_schemas, and document_schemas to ensure
no existing document type can be silently replaced without validation.
Comment on lines
+107
to
+110
| let mut groups = self.groups().clone(); | ||
| for (pos, group) in update_values.new_groups { | ||
| groups.insert(*pos, group.clone()); | ||
| } |
Contributor
There was a problem hiding this comment.
new_groups currently allows replacing existing group positions.
At Line 107-Line 110, groups.insert(*pos, ...) overwrites existing groups without rejection. For an additive new_groups field, this permits unintended modification of existing authorization groups.
🔒 Suggested guard
-use crate::consensus::basic::data_contract::InvalidDataContractVersionError;
+use crate::consensus::basic::data_contract::{
+ DataContractUpdateTransitionOverlappingFieldsError, InvalidDataContractVersionError,
+};
@@
// Merge groups
let mut groups = self.groups().clone();
+ for group_position in update_values.new_groups.keys() {
+ if self.groups().contains_key(group_position) {
+ return Ok(ConsensusValidationResult::new_with_error(
+ DataContractUpdateTransitionOverlappingFieldsError::new(
+ self.id(),
+ "groups".to_string(),
+ group_position.to_string(),
+ )
+ .into(),
+ ));
+ }
+ }
for (pos, group) in update_values.new_groups {
groups.insert(*pos, group.clone());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut groups = self.groups().clone(); | |
| for (pos, group) in update_values.new_groups { | |
| groups.insert(*pos, group.clone()); | |
| } | |
| let mut groups = self.groups().clone(); | |
| for group_position in update_values.new_groups.keys() { | |
| if self.groups().contains_key(group_position) { | |
| return Ok(ConsensusValidationResult::new_with_error( | |
| DataContractUpdateTransitionOverlappingFieldsError::new( | |
| self.id(), | |
| "groups".to_string(), | |
| group_position.to_string(), | |
| ) | |
| .into(), | |
| )); | |
| } | |
| } | |
| for (pos, group) in update_values.new_groups { | |
| groups.insert(*pos, group.clone()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-dpp/src/data_contract/methods/apply_update/v0/mod.rs` around
lines 107 - 110, The code in apply_update (v0 mod) currently clones
self.groups() into groups and then unconditionally calls groups.insert(*pos,
group.clone()) for each (pos, group) in update_values.new_groups, which allows
overwriting existing positions; change this to reject or error when a position
already exists instead of replacing it: when iterating update_values.new_groups,
check whether groups.contains_key(pos) (or that self.groups().get(pos) is None)
and return an appropriate error/result if the slot is occupied, otherwise insert
the new group; update any function signatures or error types used by
apply_update_v0 to propagate the validation failure.
Comment on lines
+47
to
+50
| IncompatibleDataContractSchemaError::new( | ||
| contract_id, | ||
| "add".to_string(), | ||
| format!("/$defs/{}", schema_def_name), |
Contributor
There was a problem hiding this comment.
Use the correct operation label in the missing-updated-def error path.
This branch validates updates to existing defs, but reports operation "add", which misclassifies the failure.
Proposed fix
- "add".to_string(),
+ "update".to_string(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| IncompatibleDataContractSchemaError::new( | |
| contract_id, | |
| "add".to_string(), | |
| format!("/$defs/{}", schema_def_name), | |
| IncompatibleDataContractSchemaError::new( | |
| contract_id, | |
| "update".to_string(), | |
| format!("/$defs/{}", schema_def_name), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-dpp/src/data_contract/methods/validate_schema_defs_update/v0/mod.rs`
around lines 47 - 50, The error is mislabelled: in the branch that validates
updates to existing schema defs you are constructing
IncompatibleDataContractSchemaError::new with the operation string "add" —
change that operation label to "update" (keep the same arguments: contract_id
and the path format!("/$defs/{}", schema_def_name)) so the error correctly
reflects an update failure rather than an add.
Comment on lines
+68
to
+123
| // If we have updates, validate compatibility | ||
| if !updated_schema_defs.is_empty() { | ||
| if let Some(old_defs_map) = old_schema_defs { | ||
| // Build merged new defs | ||
| let mut merged_defs = old_defs_map.clone(); | ||
| for (name, def) in updated_schema_defs { | ||
| merged_defs.insert(name.clone(), def.clone()); | ||
| } | ||
| for (name, def) in new_schema_defs { | ||
| merged_defs.insert(name.clone(), def.clone()); | ||
| } | ||
|
|
||
| // Validate compatibility | ||
| if old_defs_map != &merged_defs { | ||
| let old_defs_json = Value::from(old_defs_map) | ||
| .try_into_validating_json() | ||
| .map_err(ProtocolError::ValueError)?; | ||
|
|
||
| let new_defs_json = Value::from(&merged_defs) | ||
| .try_into_validating_json() | ||
| .map_err(ProtocolError::ValueError)?; | ||
|
|
||
| let old_defs_schema = json!({ | ||
| "$defs": old_defs_json | ||
| }); | ||
|
|
||
| let new_defs_schema = json!({ | ||
| "$defs": new_defs_json | ||
| }); | ||
|
|
||
| let compatibility_validation_result = validate_schema_compatibility( | ||
| &old_defs_schema, | ||
| &new_defs_schema, | ||
| platform_version, | ||
| )?; | ||
|
|
||
| if !compatibility_validation_result.is_valid() { | ||
| let errors = compatibility_validation_result | ||
| .errors | ||
| .into_iter() | ||
| .map(|operation| { | ||
| IncompatibleDataContractSchemaError::new( | ||
| contract_id, | ||
| operation.name, | ||
| operation.path, | ||
| ) | ||
| .into() | ||
| }) | ||
| .collect(); | ||
|
|
||
| return Ok(SimpleConsensusValidationResult::new_with_errors(errors)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Compatibility validation is skipped when only new_schema_defs are provided.
The compatibility block is gated by !updated_schema_defs.is_empty(), so new_schema_defs-only updates return valid without schema compatibility checks.
Proposed fix
- // If we have updates, validate compatibility
- if !updated_schema_defs.is_empty() {
- if let Some(old_defs_map) = old_schema_defs {
+ // If we have any defs changes, validate compatibility
+ if let Some(old_defs_map) = old_schema_defs {
+ if !updated_schema_defs.is_empty() || !new_schema_defs.is_empty() {
// Build merged new defs
let mut merged_defs = old_defs_map.clone();
for (name, def) in updated_schema_defs {
merged_defs.insert(name.clone(), def.clone());
}
for (name, def) in new_schema_defs {
merged_defs.insert(name.clone(), def.clone());
}
@@
- }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-dpp/src/data_contract/methods/validate_schema_defs_update/v0/mod.rs`
around lines 68 - 123, The compatibility check is currently skipped when only
new_schema_defs is provided because the outer if uses
!updated_schema_defs.is_empty(); change the guard to run when either
updated_schema_defs or new_schema_defs is non-empty (e.g., if
!updated_schema_defs.is_empty() || !new_schema_defs.is_empty()) and keep the
existing early check for old_schema_defs (if let Some(old_defs_map)). Build
merged_defs by cloning old_defs_map and inserting both updated_schema_defs and
new_schema_defs entries (as you already do), then run
validate_schema_compatibility(&old_defs_schema, &new_defs_schema,
platform_version) and return SimpleConsensusValidationResult with
IncompatibleDataContractSchemaError items if not valid.
Comment on lines
+13
to
+15
| DataContractCreateTransition::V1(_) => { | ||
| panic!("data_contract() accessor is not available for DataContractCreateTransitionV1 - use individual field accessors instead") | ||
| } |
Contributor
There was a problem hiding this comment.
Replace V1 panics with fallible accessors.
These branches hard-panic for a valid transition variant. That turns a version mismatch into a runtime crash path. Please migrate create-transition access to Option/Result-style APIs (as done for update transitions) and keep these trait methods non-panicking.
Also applies to: 31-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-dpp/src/state_transition/state_transitions/contract/data_contract_create_transition/accessors/mod.rs`
around lines 13 - 15, The match arms that call panic! for
DataContractCreateTransition::V1 (e.g., the data_contract() accessor in mod.rs
and the similar branches around lines 31-33) must be converted to a fallible
API: instead of panicking, have the accessor return Option or Result (matching
the pattern used for update transitions) and return None or an Err variant when
the variant is V1; update the data_contract() method signature and its callers
to accept the new return type and mirror the error/Option semantics used by the
update transition accessors so no runtime panic occurs on a legitimate variant
mismatch.
| seed: u64, | ||
| credits: Credits, | ||
| ) -> (Identity, SimpleSigner, IdentityPublicKey) { | ||
| let platform_version = PlatformVersion::latest(); |
Contributor
There was a problem hiding this comment.
Pin test platform version instead of using latest().
Using PlatformVersion::latest() in shared test scaffolding can cause version-drift flakes when defaults change. Prefer an explicit test target version constant for stable expectations.
Also applies to: 84-84, 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/tests/mod.rs`
at line 29, Replace the unstable call to PlatformVersion::latest() used in the
test (the platform_version variable) with a pinned explicit test platform
version constant (e.g. define a const TEST_PLATFORM_VERSION = <desired version>
and construct PlatformVersion from that) so the test behaviour doesn't drift;
update all occurrences (the instances using PlatformVersion::latest() at the
other noted spots) to use this constant and ensure the tests import/construct
PlatformVersion from the pinned value.
Comment on lines
+75
to
+80
| // V0 has the full data contract, V1 has partial updates | ||
| let keeps_history = st | ||
| .data_contract() | ||
| .map(|dc| dc.config().keeps_history()) | ||
| .unwrap_or(false); // V1 defaults to non-historical | ||
| if keeps_history { |
Contributor
There was a problem hiding this comment.
unwrap_or(false) can generate incorrect proofs for historical contracts.
When V1 omits full contract data, defaulting keeps_history to false can select the wrong path query. For contracts that actually keep history, this will produce invalid proof inputs.
🔧 Proposed fix
- // V0 has the full data contract, V1 has partial updates
- let keeps_history = st
- .data_contract()
- .map(|dc| dc.config().keeps_history())
- .unwrap_or(false); // V1 defaults to non-historical
- if keeps_history {
- contract_ids_to_historical_path_query(&st.modified_data_ids())
- } else {
- contract_ids_to_non_historical_path_query(&st.modified_data_ids())
- }
+ let contract_ids = st.modified_data_ids();
+ match st.data_contract().map(|dc| dc.config().keeps_history()) {
+ Some(true) => contract_ids_to_historical_path_query(&contract_ids),
+ Some(false) => contract_ids_to_non_historical_path_query(&contract_ids),
+ None => {
+ let historical = contract_ids_to_historical_path_query(&contract_ids);
+ let non_historical = contract_ids_to_non_historical_path_query(&contract_ids);
+ PathQuery::merge(
+ vec![&historical, &non_historical],
+ &platform_version.drive.grove_version,
+ )?
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-drive/src/prove/prove_state_transition/v0/mod.rs` around lines 75
- 80, The current logic sets keeps_history via st.data_contract().map(|dc|
dc.config().keeps_history()).unwrap_or(false), which defaults missing contract
data to false and can choose the wrong proof path; change this to treat absent
contract data as historical (e.g., use unwrap_or(true)) or explicitly fetch the
full contract config when data_contract() is None so keeps_history reflects the
real contract — update the assignment that defines keeps_history and the
subsequent if keeps_history branch (referencing st.data_contract(),
dc.config().keeps_history(), and the if keeps_history conditional).
Comment on lines
+127
to
+141
| DataContractUpdateTransition::V1(v1) => { | ||
| // V1 has individual fields, not a full data contract | ||
| // We verify the contract exists and get it from the proof | ||
| let (root_hash, contract) = Drive::verify_contract( | ||
| proof, | ||
| None, // We don't know keeps_history from V1 transition alone | ||
| false, | ||
| true, | ||
| v1.id.into_buffer(), | ||
| platform_version, | ||
| )?; | ||
| let contract = contract.ok_or(Error::Proof(ProofError::IncorrectProof(format!("proof did not contain contract with id {} expected to exist because of state transition (update)", v1.id))))?; | ||
| // For V1, we can't do a full mismatch check since we only have partial updates | ||
| // The contract in the proof should have the updates applied | ||
| Ok((root_hash, VerifiedDataContract(contract))) |
Contributor
There was a problem hiding this comment.
V1 proof path verifies existence, not update execution.
The V1 branch only checks that a contract with v1.id exists in the proof, then returns it. It does not validate that the delta fields in the transition were actually applied. This can allow stale/incorrect contract state to be accepted as a successful update proof.
Please validate V1 delta effects against the returned contract (or reconstruct expected post-update contract from prior state and compare) before returning VerifiedDataContract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-drive/src/verify/state_transition/verify_state_transition_was_executed_with_proof/v0/mod.rs`
around lines 127 - 141, The V1 branch (DataContractUpdateTransition::V1)
currently only ensures a contract with v1.id exists via Drive::verify_contract
and returns VerifiedDataContract without ensuring the update delta was applied;
change it to reconstruct or derive the expected post-update contract from the
prior contract and the v1 delta (or apply the v1 fields onto the prior contract
fetched from the proof), then compare relevant fields (ids, modified properties,
schema, version) against the contract returned by Drive::verify_contract and
return VerifiedDataContract only if the contract in the proof matches the
expected post-update state; use the v1 variable, the original contract from
Drive::verify_contract, and the DataContractUpdateTransition::V1 delta fields to
perform the validation and return a ProofError when mismatches are detected.
Comment on lines
+73
to
+77
| document_base_state_transition: FeatureVersionBounds { | ||
| min_version: 0, | ||
| max_version: 0, | ||
| default_current_version: 1, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and read lines 73-77
echo "=== Reading target lines from file ==="
head -80 packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v3.rs | tail -10Repository: dashpay/platform
Length of output: 467
🏁 Script executed:
#!/bin/bash
# Run the Python script from the review comment to detect bounds violations
python - <<'PY'
import re, pathlib
path = pathlib.Path("packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v3.rs")
text = path.read_text()
pattern = re.compile(
r'(\w+):\s*FeatureVersionBounds\s*\{\s*min_version:\s*(\d+),\s*max_version:\s*(\d+),\s*default_current_version:\s*(\d+),\s*\}',
re.S
)
violations = []
for name, min_v, max_v, default_v in pattern.findall(text):
min_v, max_v, default_v = int(min_v), int(max_v), int(default_v)
if not (min_v <= default_v <= max_v):
violations.append((name, min_v, max_v, default_v))
if not violations:
print("No bounds violations found.")
else:
for v in violations:
print(f"{v[0]} violates bounds: min={v[1]}, max={v[2]}, default={v[3]}")
PYRepository: dashpay/platform
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Search for FeatureVersionBounds struct definition to understand its purpose and any validation
echo "=== Searching for FeatureVersionBounds definition ==="
rg "struct FeatureVersionBounds|pub struct FeatureVersionBounds" -A 10Repository: dashpay/platform
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Search for how FeatureVersionBounds is validated or used
echo "=== Searching for validation of FeatureVersionBounds ==="
rg "min_version|max_version|default_current_version" -B 2 -A 2 | head -100Repository: dashpay/platform
Length of output: 9821
🏁 Script executed:
#!/bin/bash
# Search more specifically for the struct definition
echo "=== Searching for FeatureVersionBounds struct ==="
rg "struct FeatureVersionBounds" -A 15Repository: dashpay/platform
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Look for any validation logic that checks bounds
echo "=== Searching for validation or checks on bounds ==="
rg "default_current_version.*<.*max_version|max_version.*<.*default_current_version" -B 3 -A 3Repository: dashpay/platform
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Search in the entire codebase for FeatureVersionBounds definition
fd -e rs -e toml | xargs grep -l "struct FeatureVersionBounds" 2>/dev/nullRepository: dashpay/platform
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Read the entire file to see imports and context
wc -l packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v3.rsRepository: dashpay/platform
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Read the full file
cat -n packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v3.rsRepository: dashpay/platform
Length of output: 5890
🏁 Script executed:
#!/bin/bash
# Look for where FeatureVersionBounds is imported from
rg "use.*FeatureVersionBounds|from.*FeatureVersionBounds" --type rsRepository: dashpay/platform
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Search for any bounds checking or version validation logic
rg "is_version_supported|check.*version|version.*check" -B 2 -A 2 | head -80Repository: dashpay/platform
Length of output: 8329
🏁 Script executed:
#!/bin/bash
# Search for the versioned_feature_core crate
rg "versioned.feature.core|versioned-feature-core" --type tomlRepository: dashpay/platform
Length of output: 1146
🏁 Script executed:
#!/bin/bash
# Check if there are any validation checks in this particular file or related test files
fd -name "*.rs" -path "*/dpp_state_transition_serialization_versions/*" | head -20Repository: dashpay/platform
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Look for any test that might validate these bounds
rg "document_base_state_transition" -B 2 -A 2Repository: dashpay/platform
Length of output: 3881
🏁 Script executed:
#!/bin/bash
# Check the same configuration in v1.rs and v2.rs
echo "=== Checking v1.rs ==="
sed -n '73,77p' packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v1.rs
echo "=== Checking v2.rs ==="
sed -n '73,77p' packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v2.rsRepository: dashpay/platform
Length of output: 445
🏁 Script executed:
#!/bin/bash
# Look at how document_base_state_transition is actually used
cat -n packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_base_transition/from_document.rs | head -50Repository: dashpay/platform
Length of output: 2302
🏁 Script executed:
#!/bin/bash
# Search for any validation logic related to version bounds in the external crate or documentation
# Check Cargo.toml for external crate info
cat packages/rs-platform-version/Cargo.toml | grep -A 2 "versioned-feature-core"Repository: dashpay/platform
Length of output: 267
🏁 Script executed:
#!/bin/bash
# Check all versions for the same pattern
echo "=== Checking all version files for document_base_state_transition ==="
for f in packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/*.rs; do
echo "=== $f ==="
sed -n '/document_base_state_transition:/,/},/p' "$f"
doneRepository: dashpay/platform
Length of output: 1936
🏁 Script executed:
#!/bin/bash
# Search for any validation logic that checks if default_current_version is within bounds
rg "default_current_version.*max_version|is_version_supported|validate.*bounds" -B 3 -A 3Repository: dashpay/platform
Length of output: 22338
🏁 Script executed:
#!/bin/bash
# Look at the error message more closely - seems like line 43 says known_versions vec![0] but code handles version 1
cat -n packages/rs-dpp/src/state_transition/state_transitions/document/batch_transition/batched_transition/document_base_transition/from_document.rs | sed -n '41,45p'Repository: dashpay/platform
Length of output: 330
Fix bounds config: max_version is less than default_current_version.
Line 75 sets max_version to 0 while line 76 sets default_current_version to 1, violating the bounds constraint. The code at lines 28-40 in from_document.rs actually handles version 1, confirming this version should be supported. The same invalid configuration exists in v2.rs. Change max_version to 1.
Proposed fix
document_base_state_transition: FeatureVersionBounds {
min_version: 0,
- max_version: 0,
+ max_version: 1,
default_current_version: 1,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| document_base_state_transition: FeatureVersionBounds { | |
| min_version: 0, | |
| max_version: 0, | |
| default_current_version: 1, | |
| }, | |
| document_base_state_transition: FeatureVersionBounds { | |
| min_version: 0, | |
| max_version: 1, | |
| default_current_version: 1, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_serialization_versions/v3.rs`
around lines 73 - 77, The FeatureVersionBounds for
document_base_state_transition has max_version set to 0 while
default_current_version is 1; update the bounds so max_version is at least 1.
Locate the document_base_state_transition struct initialization (symbol:
FeatureVersionBounds) in v3.rs (and apply the same fix in v2.rs) and change
max_version from 0 to 1 to make the bounds consistent with
default_current_version and the handling in from_document.rs.
Comment on lines
+467
to
+482
| if let Some(data_contract) = contract_update.data_contract() { | ||
| let new_contract = match data_contract.clone() { | ||
| DataContractInSerializationFormat::V0(mut v0) => { | ||
| v0.owner_id = owner_id; | ||
|
|
||
| DataContractInSerializationFormat::V0(v0) | ||
| } | ||
| DataContractInSerializationFormat::V1(mut v1) => { | ||
| v1.owner_id = owner_id; | ||
| DataContractInSerializationFormat::V0(v0) | ||
| } | ||
| DataContractInSerializationFormat::V1(mut v1) => { | ||
| v1.owner_id = owner_id; | ||
|
|
||
| DataContractInSerializationFormat::V1(v1) | ||
| } | ||
| }; | ||
| DataContractInSerializationFormat::V1(v1) | ||
| } | ||
| }; | ||
|
|
||
| contract_update.set_data_contract(new_contract); | ||
| contract_update.set_data_contract(new_contract); | ||
| } |
Contributor
There was a problem hiding this comment.
setOwnerId silently no-ops for V1 update transitions without embedded contract.
At Line 467, if data_contract() is None (expected for V1 delta transitions), this branch returns Ok(()) without changing owner data. That makes setOwnerId report success while doing nothing.
💡 Suggested fix (fail fast instead of silent no-op)
DataContractUpdate(mut contract_update) => {
if let Some(data_contract) = contract_update.data_contract() {
let new_contract = match data_contract.clone() {
DataContractInSerializationFormat::V0(mut v0) => {
v0.owner_id = owner_id;
DataContractInSerializationFormat::V0(v0)
}
DataContractInSerializationFormat::V1(mut v1) => {
v1.owner_id = owner_id;
DataContractInSerializationFormat::V1(v1)
}
};
contract_update.set_data_contract(new_contract);
+ } else {
+ return Err(WasmDppError::invalid_argument(
+ "Cannot set owner for V1 data contract update transition without embedded data contract",
+ ));
}
self.0 = DataContractUpdate(contract_update);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(data_contract) = contract_update.data_contract() { | |
| let new_contract = match data_contract.clone() { | |
| DataContractInSerializationFormat::V0(mut v0) => { | |
| v0.owner_id = owner_id; | |
| DataContractInSerializationFormat::V0(v0) | |
| } | |
| DataContractInSerializationFormat::V1(mut v1) => { | |
| v1.owner_id = owner_id; | |
| DataContractInSerializationFormat::V0(v0) | |
| } | |
| DataContractInSerializationFormat::V1(mut v1) => { | |
| v1.owner_id = owner_id; | |
| DataContractInSerializationFormat::V1(v1) | |
| } | |
| }; | |
| DataContractInSerializationFormat::V1(v1) | |
| } | |
| }; | |
| contract_update.set_data_contract(new_contract); | |
| contract_update.set_data_contract(new_contract); | |
| } | |
| if let Some(data_contract) = contract_update.data_contract() { | |
| let new_contract = match data_contract.clone() { | |
| DataContractInSerializationFormat::V0(mut v0) => { | |
| v0.owner_id = owner_id; | |
| DataContractInSerializationFormat::V0(v0) | |
| } | |
| DataContractInSerializationFormat::V1(mut v1) => { | |
| v1.owner_id = owner_id; | |
| DataContractInSerializationFormat::V1(v1) | |
| } | |
| }; | |
| contract_update.set_data_contract(new_contract); | |
| } else { | |
| return Err(WasmDppError::invalid_argument( | |
| "Cannot set owner for V1 data contract update transition without embedded data contract", | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/wasm-dpp2/src/state_transitions/base/state_transition.rs` around
lines 467 - 482, The setOwnerId implementation currently treats a missing
embedded contract as a silent no-op; update setOwnerId to fail fast when
contract_update.data_contract() is None for update transitions instead of
returning Ok(()). In the setOwnerId method (handling contract_update), check for
data_contract() being None and return an appropriate Err (use the module's
existing error type) with a clear message indicating that V1 delta update
transitions do not include an embedded contract and owner cannot be set, rather
than silently succeeding; keep the existing handling that modifies
DataContractInSerializationFormat::V0/V1 when a contract is present and call
contract_update.set_data_contract(new_contract) as before.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Implements V1 versions of DataContractCreateTransition and DataContractUpdateTransition that store contract fields directly rather than embedding the full DataContractInSerializationFormat. This is part of
Protocol Version 12.
Key problems solved:
What was done?
New State Transition Versions
DataContractInSerializationFormat
New Methods
New Validation
Platform Version Updates
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Error Handling
Tests