Skip to content

fix(dpp): stop hard-erroring on index-order-only contract updates - #4295

Merged
QuantumExplorer merged 3 commits into
v4.2-devfrom
claude/angry-torvalds-8d4c55
Aug 5, 2026
Merged

fix(dpp): stop hard-erroring on index-order-only contract updates#4295
QuantumExplorer merged 3 commits into
v4.2-devfrom
claude/angry-torvalds-8d4c55

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Aug 5, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

The JSON-schema compatibility validator has no keyword rule for indices, so any contract-update schema diff under /indices that survived the dedicated index checks returned Err(UnsupportedSchemaKeywordError → SchemaCompatibilityValidationError) instead of a clean consensus validation result. drive-abci surfaces that as StateTransitionExecutionResult::InternalError (code 1) for user-triggerable input.

At protocol v14 (after #4291's name-keyed index comparison) the reachable case is reordering the indices array without changing the definition set: the name-keyed comparison passes — a reorder is a semantic no-op, since indices are keyed by name — but the JSON diff under /indices still hard-errored.

What was done?

  • Added validate_schema_compatibility v1 (packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs): a new generation that strips the top-level indices key from both schemas before diffing. Index definitions are validated in exactly one place — validate_update v1's name-keyed comparison, which rejects any real index change with a clean DataContractInvalidIndexDefinitionUpdateError before schema compatibility runs. A property named indices (under /properties/indices) is still validated.
  • Wired the new generation into the dispatcher (validate_schema_compatibility/mod.rs) and bumped validate_schema_compatibility: 1 in CONTRACT_VERSIONS_V6, which only protocol v14 uses — the shipped v0 generation stays byte-identical and protocol ≤13 behavior is unchanged.
  • Documented the gate in the CONTRACT_VERSIONS_V6 and PLATFORM_V14 changelog comments.

Stripping was chosen over an "any /indices change is incompatible" rule because the only diff that can reach this check at v14 is a reorder-only update, which is harmless and should be accepted rather than cleanly rejected.

How Has This Been Tested?

  • validate_update/v1/mod.rs: should_pass_when_indices_are_reordered_without_changes — reorder-only contract update through the public validate_update dispatcher at the latest protocol version now returns a valid result instead of an Err.
  • validate_schema_compatibility/v1/mod.rs: reorder-only diff is ignored; an incompatible property change alongside an /indices diff is still reported; a property literally named indices is still validated; replay-safety pin that protocol v13 (v0) still hard-errors on an /indices diff with the exact unsupported-keyword message.
  • cargo test -p dpp (full suite), cargo test -p platform-version, cargo check -p drive-abci, cargo clippy -p dpp -p platform-version --all-targets, cargo check -p dpp --all-features --tests and --no-default-features --tests — all green.

Breaking Changes

None. Gated at protocol v14 (unreleased); v0 stays byte-identical and pre-v14 outcomes are pinned by tests. Note this makes a reorder-only update the first index-touching contract update ever accepted end-to-end — previously every such update was rejected one way or another, so no historical accepted-set changes.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Schema compatibility checks now allow document indexes to be reordered without incorrectly rejecting otherwise compatible updates.
    • Genuine schema incompatibilities and index changes continue to be detected and reported.
    • Properties named indices within document schemas remain fully validated.
    • Document update validation now confirms reordered unchanged indexes are accepted.
  • Documentation

    • Updated protocol documentation to reflect the improved schema compatibility behavior and version transition.

The JSON-schema compatibility validator has no keyword rule for
`indices`, so any contract-update schema diff under /indices that
survived the index checks returned
Err(UnsupportedSchemaKeywordError -> SchemaCompatibilityValidationError)
instead of a clean consensus validation result — surfaced by drive-abci
as StateTransitionExecutionResult::InternalError for user-triggerable
input. At protocol v14 the reachable case is reordering the indices
array without changing the definition set: validate_update v1's
name-keyed comparison passes (a reorder is a semantic no-op — indices
are keyed by name), but the JSON diff under /indices still hard-errored.

Add validate_schema_compatibility v1, which strips the top-level
`indices` key from both schemas before diffing: index definitions are
validated in exactly one place (validate_update v1's name-keyed
comparison, which rejects any real index change with a clean
DataContractInvalidIndexDefinitionUpdateError before schema
compatibility runs). A property named "indices" lives under
/properties/indices and is still validated.

Gated at protocol v14 via CONTRACT_VERSIONS_V6 (used only by v14);
v0 stays byte-identical and protocol v13 behavior is pinned by tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v4.2.0 milestone Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@QuantumExplorer, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 55 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1b0418c4-1455-432f-8333-935be0eb609e

📥 Commits

Reviewing files that changed from the base of the PR and between eac2ac0 and 3a3f4dc.

📒 Files selected for processing (1)
  • packages/rs-platform-version/src/version/v14.rs
📝 Walkthrough

Walkthrough

The change adds a v1 schema compatibility validator that ignores top-level index ordering, routes protocol v6 to it, documents the protocol v14 behavior, and adds regression tests for schema and update validation.

Changes

Schema compatibility and index reordering

Layer / File(s) Summary
Implement v1 schema compatibility validation
packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs
The v1 validator removes top-level indices before schema comparison. It reports other incompatible changes and maps validator errors to ProtocolError. Tests cover reordered indexes, property changes, nested indices properties, and v0 behavior.
Wire versioned dispatch and protocol configuration
packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/mod.rs, packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v6.rs, packages/rs-platform-version/src/version/v14.rs
Version 1 becomes a supported schema compatibility version. Protocol v6 selects version 1. Protocol v14 documentation records the updated behavior.
Validate reordered indexes in updates
packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs
A regression test confirms that reordered, unchanged index definitions pass update validation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SchemaValidation
  participant V1Validator
  participant JsonSchemaCompatibility
  SchemaValidation->>V1Validator: dispatch compatibility version 1
  V1Validator->>V1Validator: remove top-level indices
  V1Validator->>JsonSchemaCompatibility: compare filtered schemas
  JsonSchemaCompatibility-->>V1Validator: changes or validation error
  V1Validator-->>SchemaValidation: SimpleValidationResult or ProtocolError
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: stopping hard-errors on index-order-only contract updates, which is the core objective across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch claude/angry-torvalds-8d4c55
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/angry-torvalds-8d4c55

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

🔍 Review in progress — actively reviewing now (commit 3a3f4dc)
Stage: Codex precheck starting
ETA: complete ~08:57 UTC (median 22m across 30 recent reviews)
Running 5m · Last checked: 2026-08-05 08:40 UTC

@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.74074% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.60%. Comparing base (68be5d9) to head (3a3f4dc).

Files with missing lines Patch % Lines
...ct/document_type/methods/validate_update/v1/mod.rs 62.50% 9 Missing ⚠️
...ype/schema/validate_schema_compatibility/v1/mod.rs 96.32% 5 Missing ⚠️
...t_type/schema/validate_schema_compatibility/mod.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           v4.2-dev    #4295      +/-   ##
============================================
- Coverage     87.60%   87.60%   -0.01%     
============================================
  Files          2703     2704       +1     
  Lines        344990   345181     +191     
============================================
+ Hits         302242   302379     +137     
- Misses        42748    42802      +54     
Components Coverage Δ
dpp 88.78% <90.74%> (-0.04%) ⬇️
drive 86.25% <ø> (ø)
drive-abci 89.65% <ø> (ø)
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.88% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 48.02% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final validation — Codex + Sonnet

This PR adds a well-scoped, protocol-v14-gated validate_schema_compatibility v1 that strips the top-level indices key before diffing document-type schemas, closing a real gap where an index-order-only contract update hard-errored into an internal error. I verified the version wiring end-to-end (v13 = validate_update v0 + schema-compat v0, v14 = validate_update v1 + schema-compat v1), confirmed the $defs call site is unaffected (no top-level indices key there) and that a genuine /properties/indices property is still validated, and ran the new tests locally — all pass. No blocking issues; remaining findings are minor documentation/duplication/test-robustness nitpicks.

Review provenance

  • Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — security-auditor (completed), gpt-5.6-sol — rust-quality (completed), gpt-5.6-sol — ffi-engineer (completed)
  • Verifier: claude-sonnet-5 — final-verifier
  • Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — security-auditor (failed), claude-sonnet-5 — rust-quality (completed), claude-sonnet-5 — ffi-engineer (completed), claude-sonnet-5 — general (completed), claude-sonnet-5 — security-auditor (completed)

🟡 1 suggestion(s) | 💬 3 nitpick(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs:1-87: v1's correctness relies on an unenforced cross-version pairing invariant
  `validate_schema_compatibility_v1`'s safety (unconditionally stripping `indices` is fine) depends on `DocumentTypeRef::validate_update` — gated by the *separately versioned* `platform_version.dpp.validation.document_type.validate_update` field — always rejecting any real index change before this check runs, for whichever `validate_update` generation is paired with schema-compat v1 in a given `PlatformVersion`. I confirmed this holds today: v13 pairs `validate_update` v0 with schema-compat v0, and v14 pairs `validate_update` v1 with schema-compat v1 (`DPP_VALIDATION_VERSIONS_V5` + `CONTRACT_VERSIONS_V6`). But nothing in the code ties these two independent version numbers together — a future protocol version that bumps schema-compat without also bumping (or with a looser) `validate_update` would silently let real index changes bypass compatibility checking, since `indices` is unconditionally stripped. This isn't a defect in this PR (the current wiring is correct and tested), but a short code comment on `validate_schema_compatibility_v1` calling out the dependency on `validate_update` v1's guarantee would help future maintainers avoid breaking the pairing.

Comment thread packages/rs-platform-version/src/version/v14.rs
QuantumExplorer and others added 2 commits August 5, 2026 15:09
… note

The v14 header claimed the shared-prefix fix was the only pre-ranked
behavior change; the index-reorder schema-compatibility fix is a second,
orthogonal one, so name both. Also document on
validate_schema_compatibility_v1 that unconditionally stripping
`indices` is safe only while every version table pairing schema-compat
v1 also selects validate_update >= 1, which enforces index immutability
before this check runs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…alds-8d4c55

# Conflicts:
#	packages/rs-platform-version/src/version/v14.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs (1)

106-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin the v1 tests to protocol version 14.

PlatformVersion::latest() dispatches through the current version table. If a future protocol selects another schema-compatibility generation, these tests can stop exercising validate_schema_compatibility_v1. Use PlatformVersion::get(14) in these tests to keep version-1 coverage stable.

Proposed test pin
-        let platform_version = PlatformVersion::latest();
+        let platform_version =
+            PlatformVersion::get(14).expect("protocol version 14 must exist");

Also applies to: 149-150, 187-188

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs`
around lines 106 - 107, Update the v1 compatibility tests, including
should_ignore_indices_reordering and the analogous cases around the other
referenced locations, to obtain the platform version with
PlatformVersion::get(14) instead of PlatformVersion::latest(). Keep the tests
otherwise unchanged so they consistently exercise
validate_schema_compatibility_v1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs`:
- Around line 106-107: Update the v1 compatibility tests, including
should_ignore_indices_reordering and the analogous cases around the other
referenced locations, to obtain the platform version with
PlatformVersion::get(14) instead of PlatformVersion::latest(). Keep the tests
otherwise unchanged so they consistently exercise
validate_schema_compatibility_v1.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 26ce473b-9695-4b85-93a7-23853daedea1

📥 Commits

Reviewing files that changed from the base of the PR and between 93d8717 and eac2ac0.

📒 Files selected for processing (5)
  • packages/rs-dpp/src/data_contract/document_type/methods/validate_update/v1/mod.rs
  • packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/mod.rs
  • packages/rs-dpp/src/data_contract/document_type/schema/validate_schema_compatibility/v1/mod.rs
  • packages/rs-platform-version/src/version/dpp_versions/dpp_contract_versions/v6.rs
  • packages/rs-platform-version/src/version/v14.rs

@QuantumExplorer

Copy link
Copy Markdown
Member Author

Re @coderabbitai's suggestion to pin the validate_schema_compatibility v1 tests to PlatformVersion::get(14): not changed — this repo's convention is that tests for the current (tip) generation use PlatformVersion::latest(), and only tests pinning a frozen older generation name an explicit protocol version (see the v0 pin v0_should_error_on_indices_diff, which uses PlatformVersion::get(13)). When a future protocol version introduces a v2 generation, the latest-PV tests move with the tip and the v1 behavior gets its own explicit pin at that point.

🤖 Addressed by Claude Code

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@QuantumExplorer Understood. The current-generation tests should use PlatformVersion::latest(). The explicit PlatformVersion::get(13) pin correctly preserves the frozen v0 behavior. I withdraw the suggestion to pin the v1 tests to protocol v14.


✏️ Learnings added
Learnt from: QuantumExplorer
URL: https://github.com/dashpay/platform/pull/4295

Timestamp: 2026-08-05T08:15:14.408Z
Learning: In the Rust DPP schema compatibility tests, tests for the current validator generation use `PlatformVersion::latest()`. Tests for a frozen older generation use an explicit `PlatformVersion::get(...)` pin, such as the v0 `v0_should_error_on_indices_diff` test with protocol version 13. When a future validator generation becomes current, the prior-generation tests should receive an explicit protocol-version pin.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

@QuantumExplorer
QuantumExplorer merged commit b6fa0db into v4.2-dev Aug 5, 2026
35 checks passed
@QuantumExplorer
QuantumExplorer deleted the claude/angry-torvalds-8d4c55 branch August 5, 2026 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants