Skip to content

fix(dpp): remove erroneous keywords field from document-meta schema and fix contract keywords docs#3471

Merged
QuantumExplorer merged 17 commits into
v3.1-devfrom
docs/keyword-clarification
May 20, 2026
Merged

fix(dpp): remove erroneous keywords field from document-meta schema and fix contract keywords docs#3471
QuantumExplorer merged 17 commits into
v3.1-devfrom
docs/keyword-clarification

Conversation

@thephez
Copy link
Copy Markdown
Collaborator

@thephez thephez commented Apr 9, 2026

Issue being fixed or feature implemented

#2523 accidentally added a keywords field to the document type meta schema. The correct location is contract-level (DataContractV1.keywords). Neither DocumentTypeV0 nor DocumentTypeV1 has a keywords field, and try_from_schema never reads it — so document-type keywords were silently accepted and discarded.

Protocol v12 set document_type_schema: 1 (v4.rs:42), making the v1 meta schema the active validation path. The errant field needs to be removed from the v1 schema and its v12-migration allowlist in lockstep.

Separately, the DataContractV1 doc comment incorrectly stated the contract-level keyword limit as 20 when the validators and TooManyKeywordsError both enforce 50.

What was done?

  • Removed the keywords block from packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json (the active meta schema)
  • Removed "keywords" from ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES in allowed_top_level_properties.rs — required to keep the allowlist in sync with the schema (enforced by the allowlist_matches_v1_meta_schema_properties parity test)
  • Left the v0 meta schema unchanged — it's a frozen historical schema and not on the active validation path
  • Fixed the doc comment on DataContractV1 to state the correct contract-level max of 50 keywords
  • Clarified inline validator comments to say "contract keywords"

How Has This Been Tested?

  • cargo test -p dpp — all 2462 dpp lib tests pass, including allowlist_matches_v1_meta_schema_properties
  • cargo fmt --all clean
  • No new tests added — change is a schema + allowlist removal backed by existing parity test coverage

Breaking Changes

Document-type keywords is no longer accepted by v1 meta-schema validation. Any existing stored contract that happens to have keywords on a document type will have it stripped during the v12 migration via strip_unknown_document_schema_properties. This is safe — keywords on document types has never been read by Rust, so stripping it cannot affect runtime semantics. Contract-level keywords (DataContractV1.keywords) are unaffected.

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

Summary by CodeRabbit

  • Documentation

    • Clarified wording to reference "contract keywords" and updated the documented per-contract keyword limit from 20 to 50.
  • Changes

    • Removed the top-level keywords property from the document meta-schema; keywords is no longer accepted there.
    • Migration now strips top-level keywords from document schemas.
  • Tests

    • Added tests verifying keywords are stripped from schemas and rejected by meta-schema validation.

Review Change Stack

@thephez thephez requested a review from QuantumExplorer as a code owner April 9, 2026 20:42
@github-actions github-actions Bot added this to the v3.1.0 milestone Apr 9, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 9, 2026

Caution

Review failed

Failed to post review comments

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Removed keywords from the v1 document meta-schema and v11→v12 migration allowlist, added tests to strip and reject document-type keywords, and updated inline comments to refer to contract keywords.

Changes

Schema & validation updates

Layer / File(s) Summary
Remove keywords from meta-schema and allowlist
packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json, packages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rs
Deleted the top-level keywords property from the v1 document meta-schema; removed "keywords" from ALLOWED_TRANSITION_TO_DOCUMENT_SCHEMA_V1_PROPERTIES; added strips_keywords_from_document_schema unit test verifying keywords are stripped and updated allowlist expectations.
Inline comment updates
packages/rs-dpp/src/data_contract/methods/validate_update/v0/mod.rs, packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_create/basic_structure/v0/mod.rs, packages/rs-dpp/src/data_contract/v1/data_contract.rs
Clarified inline comments to refer to “contract keywords” and adjusted a doc comment to state a 50-keyword contract limit; no runtime behavior changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Poem

🐇 I hopped through schemas, neat and swift,
I plucked the stray keywords, gave them a lift.
Contracts now speak of fifty held tight,
Tests wink and say the schema's polite.
🥕

🚥 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 changes: removing the erroneous keywords field from document-meta schema and fixing contract keywords documentation.
Docstring Coverage ✅ Passed Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/keyword-clarification

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 and usage tips.

@thephez thephez force-pushed the docs/keyword-clarification branch from 4390ccf to b98d374 Compare April 9, 2026 20:45
@thepastaclaw
Copy link
Copy Markdown
Collaborator

thepastaclaw commented Apr 9, 2026

✅ Review complete (commit 7b07054)

@QuantumExplorer
Copy link
Copy Markdown
Member

This might break testnet/mainnet (we will need to verify it doesn't)

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Clean, well-scoped PR that removes an erroneous keywords property from the document-meta JSON schema and fixes doc comments. The one substantive finding — modifying a runtime-compiled schema in place without protocol version gating — is technically valid but low practical risk. The doc comment nitpick is dropped because the author already considered and explicitly reverted a more verbose comment.

Reviewed commit: b98d374

🟡 1 suggestion(s)

1 additional finding

🟡 suggestion: In-place modification of runtime-compiled v0 meta-schema

packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json (lines 591-594)

Verified: DOCUMENT_META_JSON_V0 is loaded via include_str! at meta_validators/mod.rs:39 and compiled into DOCUMENT_META_SCHEMA_V0 at line 89. Both try_from_schema/v0 (line 136) and try_from_schema/v1 (line 155) use this single compiled schema to validate document type definitions at runtime.

The root meta-schema has no additionalProperties: false — the additionalProperties at line 591 is a property definition (constraining what document types must set), not a root-level restriction. So unknown properties were already silently accepted. The removed keywords definition only constrained malformed keyword values (wrong item types, >20 items, etc.).

During a rolling upgrade, a document type with malformed keywords (e.g. non-string items) would be rejected by old binaries but accepted by new ones — a theoretical consensus divergence. Practical risk is minimal since no code reads keywords from document types and the field was always discarded. Only one version of this schema exists (no v1), so there's no version-gating mechanism to leverage.

If the team's upgrade process guarantees atomic binary upgrades (all nodes update before processing new blocks), this is a non-issue. Otherwise, consider whether this warrants a versioned schema.

🤖 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/schema/meta_schemas/document/v0/document-meta.json`:
- [SUGGESTION] lines 591-594: In-place modification of runtime-compiled v0 meta-schema
  Verified: `DOCUMENT_META_JSON_V0` is loaded via `include_str!` at `meta_validators/mod.rs:39` and compiled into `DOCUMENT_META_SCHEMA_V0` at line 89. Both `try_from_schema/v0` (line 136) and `try_from_schema/v1` (line 155) use this single compiled schema to validate document type definitions at runtime.

The root meta-schema has no `additionalProperties: false` — the `additionalProperties` at line 591 is a *property definition* (constraining what document types must set), not a root-level restriction. So unknown properties were already silently accepted. The removed `keywords` definition only constrained *malformed* keyword values (wrong item types, >20 items, etc.).

During a rolling upgrade, a document type with malformed `keywords` (e.g. non-string items) would be rejected by old binaries but accepted by new ones — a theoretical consensus divergence. Practical risk is minimal since no code reads keywords from document types and the field was always discarded. Only one version of this schema exists (no v1), so there's no version-gating mechanism to leverage.

If the team's upgrade process guarantees atomic binary upgrades (all nodes update before processing new blocks), this is a non-issue. Otherwise, consider whether this warrants a versioned schema.

@thephez
Copy link
Copy Markdown
Collaborator Author

thephez commented Apr 13, 2026

Maybe good to pair this change with #3475 in the same release? Prior to that PR, keywords (or any additional property) would be accepted, but that PR makes things more strict.

If AI correctly stated that extra document schema entries were actually just silently dropped, there shouldn't be breaking issues. But I'm not familiar enough with the inner workings to be sure on that so it should be reviewed by someone more knowledgeable.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

The comment/doc cleanups are correct, but the schema fix is incomplete. This PR only removes the erroneous document-type keywords field from the v0 document meta-schema, while the current protocol path still validates document types against the v1 meta-schema that retains the same field.

Reviewed commit: 2b3585f

🔴 1 blocking

1 additional finding

🔴 blocking: The live document meta-schema still accepts document-level `keywords`

packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json (lines 591-600)

Removing keywords from document/v0/document-meta.json does not fix the active validation path. DocumentType::try_from_schema() selects DOCUMENT_META_SCHEMA_V1 whenever document_type_schema == 1, and that is the version used for the current protocol line, so contracts created today still validate successfully with document-level keywords because packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json still defines the field. The v12 migration allowlist in packages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rs also still treats keywords as a valid document-type property, so the cleanup needs to update the v1 schema and its synced allowlist together.

🤖 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/schema/meta_schemas/document/v1/document-meta.json`:
- [BLOCKING] lines 591-600: The live document meta-schema still accepts document-level `keywords`
  Removing `keywords` from `document/v0/document-meta.json` does not fix the active validation path. `DocumentType::try_from_schema()` selects `DOCUMENT_META_SCHEMA_V1` whenever `document_type_schema == 1`, and that is the version used for the current protocol line, so contracts created today still validate successfully with document-level `keywords` because `packages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json` still defines the field. The v12 migration allowlist in `packages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rs` also still treats `keywords` as a valid document-type property, so the cleanup needs to update the v1 schema and its synced allowlist together.

@thephez thephez marked this pull request as draft April 16, 2026 13:01
thephez and others added 3 commits April 16, 2026 09:02
… comments

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The field was added by mistake in #2523 — keywords belong to the
contract level (DataContractV1), not document types. DocumentTypeV0/V1
have no keywords field and try_from_schema never reads it, so the
schema entry was silently accepted then discarded.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@thephez thephez force-pushed the docs/keyword-clarification branch from 2b3585f to 6d234e9 Compare April 16, 2026 13:02
thephez and others added 2 commits April 16, 2026 10:41
v0 is a frozen historical meta schema no longer on the active
validation path (protocol v12 switched to v1). Removing it from v0
had no runtime effect but risked diverging from what was validated
when legacy contracts were created. The real fix belongs on the v1
schema, which is applied in a follow-up commit.

Reverts the schema portion of 9e56d4d.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR #2523 mistakenly placed keywords in the document-type meta schema
(correct location is contract-level, on DataContractV1). Neither
DocumentTypeV0 nor DocumentTypeV1 has a keywords field, and
try_from_schema never reads it.

v1 is the active meta schema (protocol v12 sets document_type_schema
to 1), so document-type keywords were still silently accepted in
new contracts after the earlier v0 fix. Removing the field from the
v1 schema and its v12-migration allowlist in lockstep — the parity
test (allowlist_matches_v1_meta_schema_properties) requires both to
change together.

Any existing stored contract that happens to have document-type
keywords will be cleanly stripped by the v12 migration in
strip_unknown_document_schema_properties; safe because keywords on
document types has never been read by Rust.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@thephez thephez marked this pull request as ready for review April 16, 2026 14:47
…on and stripping

Adds two tests targeting the behavior change of removing `keywords`
from the v1 document-type meta schema and its allowlist:

- `strips_keywords_from_document_schema` (rs-dpp): unit test that
  verifies the v12 migration `strip_unknown_properties_from_document_schema`
  removes a `keywords` key from a document-type schema.

- `test_document_type_keywords_rejected_by_v1_meta_schema` (drive-abci):
  integration test pinned to protocol v12 that injects `keywords` onto
  a document-type schema, calls `DataContract::from_value` with full
  validation, and asserts the failure is specifically a
  `BasicError::JsonSchemaError` whose summary references `keywords`
  (not any unrelated error).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@thephez thephez requested a review from thepastaclaw April 16, 2026 16:18
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 16, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.17%. Comparing base (442ff4f) to head (080bc89).

Files with missing lines Patch % Lines
...tion/state_transitions/data_contract_create/mod.rs 77.27% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           v3.1-dev    #3471      +/-   ##
============================================
- Coverage     87.17%   87.17%   -0.01%     
============================================
  Files          2600     2600              
  Lines        317961   318021      +60     
============================================
+ Hits         277173   277224      +51     
- Misses        40788    40797       +9     
Components Coverage Δ
dpp 87.68% <100.00%> (+<0.01%) ⬆️
drive 85.95% <ø> (ø)
drive-abci 89.68% <77.27%> (-0.01%) ⬇️
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.17% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.16% <ø> (ø)
🚀 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
Copy link
Copy Markdown
Collaborator

Validated the current head (3df26c16) and I do not think this needs another code change.\n\n- the earlier v0-only concern is addressed: v0 was restored, and the active v1 document meta-schema now drops keywords along with the synced v12 allowlist\n- existing stored contracts are still safe because the v12 migration strips unknown document-schema properties, including document-type keywords\n- Rust never reads document-type keywords on either the v0 or v1 DocumentType path, so the old field was only ever silently accepted/discarded\n\nLocal verification on this head:\n- cargo test -p dpp strips_keywords_from_document_schema ✅\n- cargo test -p drive-abci test_document_type_keywords_rejected_by_v1_meta_schema ✅\n\nSo from my side this looks ready for fresh review rather than more edits.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

I verified the referenced findings against detached HEAD e89e0fc640c16f0d71956f04f8b077d790e8877c. The functional change is consistent: keywords is removed from the v1 document-type meta-schema and the migration allowlist, and the new regression tests cover both rejection and cleanup. Two test-quality issues remain in the new create-state-transition test; the release-coordination concern is not a code defect that can be confirmed from this tree alone.

Reviewed commit: e89e0fc

💬 2 nitpick(s)

thephez and others added 2 commits April 23, 2026 09:09
Assert structured `JsonSchemaError` fields (keyword + offending property
name in params) instead of a substring match on `error_summary`, and drop
the unused platform/identity setup since `DataContract::from_value` is a
pure DPP call.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

I reviewed this as a focused schema-correction PR and did not find a concrete correctness regression in the checked-out change. The intended behavior is to stop accepting keywords on individual document-type schemas, align the active v1 schema/allowlist with the Rust implementation, and strip legacy stored occurrences during migration; that direction appears consistent and appropriately scoped.

Reviewed commit: 73b972a

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

PR #3471 removes an erroneous keywords field from the v1 document-type meta-schema and the v12 migration allowlist (ALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIES), corrects a stale DataContractV1 doc comment (20→50), and adds regression tests. All six agent reviews returned zero findings and CodeRabbit reported nothing; there is nothing left to flag.

Reviewed commit: aa60910

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Targeted, low-risk cleanup of the v1 document-type meta schema. Removing a keywords block that was never read and updating the v12 migration allowlist in lockstep is consistent with the existing parity test, and the doc-comment fix (20 → 50) aligns with the actual validator/TooManyKeywordsError limit. No consensus, dependency-direction, or feature-gate concerns. v12 is still in v3.1-dev and not yet shipped, so changing the active v1 meta schema without a version bump is safe. Three minor observations remain — all nitpicks/suggestions, no blockers.

Reviewed commit: 4249c3a

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

2 additional findings

🟡 suggestion: No end-to-end test for the v12 migration stripping `keywords` from a stored contract

packages/rs-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs (lines 22-138)

The PR description claims the v12 protocol-upgrade migration Drive::strip_unknown_document_schema_properties will silently strip any keywords that slipped onto a document type in stored state. New tests cover (a) the pure DPP helper strip_unknown_properties_from_document_schema (incl. the explicit keywords case at allowed_top_level_properties.rs:82) and (b) the v1 meta schema rejecting the field on DataContract::from_value. Neither exercises the migration end-to-end against grovedb: store a contract whose document-type schema contains keywords, run transition_to_version_12, then verify the field is gone after deserialization and the contract is still loadable. Existing test_transition_to_version_12_creates_shielded_pool_trees only covers shielded-pool tree creation. Since the migration mutates stored contract content (changing the serialized hash) and re-encodes via DataContractInSerializationFormat for both historical and non-historical contracts, an integration-style test would protect against silent regressions in either path.

💬 nitpick: v0 meta schema still defines a `keywords` block that was never read

packages/rs-dpp/schema/meta_schemas/document/v0/document-meta.json (lines 591-601)

The PR intentionally leaves the v0 meta schema unchanged (frozen historical schema). However v0 still carries the same erroneous keywords definition that motivated this PR for v1, with a description claiming a max of 20 (which never matched the validators that enforce 50 for the contract-level field). Since v0's root has no additionalProperties: false, leaving it doesn't change validation behavior, but it can mislead future readers into thinking document types have a keywords field. Consider adding a one-line "$comment" next to the v0 keywords block noting it is unused and was relocated to the contract level — purely a clarity nudge, no behavior change.

🤖 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-drive/src/drive/contract/migration/strip_unknown_document_schema_properties.rs`:
- [SUGGESTION] lines 22-138: No end-to-end test for the v12 migration stripping `keywords` from a stored contract
  The PR description claims the v12 protocol-upgrade migration `Drive::strip_unknown_document_schema_properties` will silently strip any `keywords` that slipped onto a document type in stored state. New tests cover (a) the pure DPP helper `strip_unknown_properties_from_document_schema` (incl. the explicit `keywords` case at allowed_top_level_properties.rs:82) and (b) the v1 meta schema rejecting the field on `DataContract::from_value`. Neither exercises the migration end-to-end against grovedb: store a contract whose document-type schema contains `keywords`, run `transition_to_version_12`, then verify the field is gone after deserialization and the contract is still loadable. Existing `test_transition_to_version_12_creates_shielded_pool_trees` only covers shielded-pool tree creation. Since the migration mutates stored contract content (changing the serialized hash) and re-encodes via `DataContractInSerializationFormat` for both historical and non-historical contracts, an integration-style test would protect against silent regressions in either path.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Claude reported no findings and Codex failed to produce parseable output. The PR is a narrowly-scoped, safe fix removing an erroneous keywords property from the v1 document-type meta schema and the v12 migration allowlist, with regression tests. No blocking issues, suggestions, or nitpicks to surface.

Reviewed commit: 6aa00c9

@PastaPastaPasta
Copy link
Copy Markdown
Member

Codex 5.4 believes this is safe for deployments before network switches to version 12:

I followed up on the “This might break testnet/mainnet” concern by checking live DAPI status directly on both public networks.

As of May 5, 2026, both mainnet and testnet are still pre-v12:

- Mainnet `149.28.241.190:443` (`chainId: evo1`)
  - `drive.current = 11`
  - `drive.nextEpoch = 11`
  - `drive.latest = 11`
- Testnet `68.67.122.1:1443` (`chainId: dash-testnet-51`)
  - `drive.current = 11`
  - `drive.nextEpoch = 11`
  - `drive.latest = 11`
- Testnet `68.67.122.4:1443` (`chainId: dash-testnet-51`)
  - `drive.current = 11`
  - `drive.nextEpoch = 11`
  - `drive.latest = 11`

I verified this via `Platform/getStatus` on the live gRPC endpoints, not just local assumptions.

Why this matters for this PR:
- The schema tightening here affects the active v1 document-type meta-schema used under protocol v12.
- The migration logic that strips stale document-type properties runs on the `<12 -> 12` protocol transition.
- Since mainnet and testnet are both still on protocol 11, there is no current public-network mixed-v12 deployment where old nodes would accept document-type `keywords` and patched nodes would reject them.

Conclusion:
- I do **not** see a current mainnet/testnet break risk from merging this PR now.
- The earlier concern about a protocol-12 validity split only becomes relevant if a network is **already on v12** and is then run with a mixed old/new binary set.
- In other words, this is not a present blocker for public networks today, but it remains a version-coordination concern for any already-v12 environment.

So based on current live network state, I’m satisfied this does **not** break today’s mainnet/testnet rollout.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Both agent reviewers (Claude and Codex) independently found no actionable issues in this PR. The change is a narrowly-scoped fix removing the erroneously-added document-type-level keywords property from the v1 meta-schema and the v12 migration allowlist, plus a doc comment correction (20 → 50) matching the enforced limit. Regression tests cover both the DPP-stripping and drive-abci meta-schema rejection layers.

Reviewed commit: 1239e2d

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Narrow, well-scoped fix removing the erroneously-added document-type-level keywords property from the v1 meta-schema and the v12 migration allowlist (parity-tested), plus a doc fix (20→50) and minor comment clarifications. New tests cover the DPP strip helper, v1 meta-schema rejection, and three end-to-end v12 migration paths (non-historical, single-revision historical, multi-revision historical with flag preservation). Claude's only finding was a false positive (the suggested e2e tests already exist).

Reviewed commit: 1239e2d

Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.

Code Review

Reviewed commit: 1239e2d

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Narrow, well-scoped fix removing the erroneously-added document-type-level keywords property from the v1 meta-schema and the v12 migration allowlist in lockstep, with parity tests for both rejection at validation time and stripping during the v12 migration. Doc fix (20→50) and minor comment clarifications round it out. Both agents converged on no findings; verified against the patch and worktree.

Reviewed commit: 1239e2d

Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.

Code Review

Reviewed commit: 1239e2d

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Both agents converged on no findings. The PR is a narrow, well-scoped fix removing the erroneously-added document-type-level keywords property from the v1 meta-schema and the v12 migration allowlist in lockstep, with parity tests covering both validation-time rejection and migration strip behavior. Doc/comment corrections are accurate.

Fresh dispatcher run for this queue item; this is a new review execution for the currently-gated SHA, posted despite an older same-SHA review.

Reviewed commit: 1239e2d

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

One blocking issue confirmed: the new parity test transition_allowlist_equals_v0_plus_known_v1_additions asserts the allowlist equals the union of all v0 top-level properties and the known v1 additions, but the v0 meta-schema still contains keywords (lines 592-602) while the allowlist deliberately omits it. The test will fail as written; the doc comments and tests need to model keywords as an explicit historical exception rather than asserting raw equality.

🔴 1 blocking

1 additional finding(s) omitted (not in diff).

Findings not attached inline (unchanged lines / prior carried-forward findings)
  • **[blocking] packages/rs-dpp/src/data_contract/document_type/schema/allowed_top_level_properties.rs — transition_allowlist_equals_v0_plus_known_v1_additions will fail: v0 meta-schema still includes keywords

    The test builds expected from every top-level property in schema/meta_schemas/document/v0/document-meta.json, but that file still defines keywords at lines 592-602. The same PR intentionally omits keywords from ALLOWED_TRANSITION_TO_DOCUMENT_SCHEMA_V1_PROPERTIES (and adds strips_keywords_from_document_schema to assert this), so the asserted equality is false in the checked-out tree and the test should fail when executed. Worse, a future contributor 'fixing' the test by re-adding keywords to the allowlist would silently undo this PR's migration behavior. The test needs to model the deliberate historical exception — filter keywords out of the v0 property set before the union — and the comment above it should call out why.

…t test

PR #2523 erroneously added `keywords` to the v0 document-type meta-schema;
the v11→v12 migration intentionally strips it. Filter it from the expected
set so `transition_allowlist_equals_v0_plus_known_v1_additions` reflects
that design choice.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Cumulative re-review at ffc0e9d. Six independent reviewers (3 Claude, 3 Codex) all converge: the sole prior blocking finding — transition_allowlist_equals_v0_plus_known_v1_additions would fail because v0 meta-schema still contains keywords while the v1 allowlist does not — is FIXED. Verified at HEAD: lines 168–173 add a comment block documenting keywords as a deliberate historical exception (placed on v0 doc-type meta-schema by PR #2523, intended home is contract-level, stripped by the v11→v12 migration), and line 185 adds .filter(|k| *k != "keywords") to the v0 property set before the union. No new correctness, security, consensus, or Rust-quality issues identified.

@thepastaclaw
Copy link
Copy Markdown
Collaborator

Thanks — I rechecked current head 080bc893 after the latest v3.1-dev merge and agree no code change is needed for those points. The v12 migration coverage already exercises the property-name-agnostic strip path across non-historical, historical, and multi-revision historical contracts, while the DPP helper test pins keywords specifically. I also agree the structured JsonSchemaError assertions are the right tradeoff here; for this regression they are safer than falling back to a substring match.

Local verification on 080bc893:

  • cargo test -p dpp strips_keywords_from_document_schema
  • cargo test -p dpp transition_allowlist_equals_v0_plus_known_v1_additions
  • cargo test -p drive-abci test_document_type_keywords_rejected_by_v1_meta_schema
  • cargo test -p drive-abci test_v12_migration_strips_unknown

@github-actions
Copy link
Copy Markdown
Contributor

✅ DashSDKFFI.xcframework built for this PR.

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "5b6c0b5ae8d543745e486f868e5e6e8ddef984bca9e5bbf53553988bd47f9354"
)

Xcode manual integration:

  • Download 'DashSDKFFI.xcframework' artifact from the run link above.
  • Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
  • If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

@thephez
Copy link
Copy Markdown
Collaborator Author

thephez commented May 20, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 20, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

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

Code Review

Both agents independently converged on no findings after cumulative re-review at 080bc89. The PR's scope is narrow and well-executed: removing the erroneous keywords field from the v1 document meta-schema, updating the transition allowlist, fixing doc comments, and adding a focused rejection test. No carried-forward findings from the prior empty review at ffc0e9d.

Copy link
Copy Markdown
Member

@QuantumExplorer QuantumExplorer left a comment

Choose a reason for hiding this comment

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

Reviewed, good one.

@QuantumExplorer QuantumExplorer merged commit ac90e10 into v3.1-dev May 20, 2026
34 checks passed
@QuantumExplorer QuantumExplorer deleted the docs/keyword-clarification branch May 20, 2026 23:29
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.

4 participants