fix(dpp): remove erroneous keywords field from document-meta schema and fix contract keywords docs#3471
Conversation
|
Caution Review failedFailed to post review comments Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved ChangesSchema & validation updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
4390ccf to
b98d374
Compare
|
✅ Review complete (commit 7b07054) |
|
This might break testnet/mainnet (we will need to verify it doesn't) |
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
|
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. |
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
… 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>
2b3585f to
6d234e9
Compare
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>
…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>
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
|
Validated the current head ( |
thepastaclaw
left a comment
There was a problem hiding this comment.
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)
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>
thepastaclaw
left a comment
There was a problem hiding this comment.
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
thepastaclaw
left a comment
There was a problem hiding this comment.
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
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
thepastaclaw
left a comment
There was a problem hiding this comment.
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
|
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. |
thepastaclaw
left a comment
There was a problem hiding this comment.
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
thepastaclaw
left a comment
There was a problem hiding this comment.
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
thepastaclaw
left a comment
There was a problem hiding this comment.
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
thepastaclaw
left a comment
There was a problem hiding this comment.
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
thepastaclaw
left a comment
There was a problem hiding this comment.
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_additionswill fail: v0 meta-schema still includeskeywordsThe test builds
expectedfrom every top-level property inschema/meta_schemas/document/v0/document-meta.json, but that file still defineskeywordsat lines 592-602. The same PR intentionally omitskeywordsfromALLOWED_TRANSITION_TO_DOCUMENT_SCHEMA_V1_PROPERTIES(and addsstrips_keywords_from_document_schemato 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-addingkeywordsto the allowlist would silently undo this PR's migration behavior. The test needs to model the deliberate historical exception — filterkeywordsout 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>
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
|
Thanks — I rechecked current head Local verification on
|
|
✅ 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:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
Issue being fixed or feature implemented
#2523 accidentally added a
keywordsfield to the document type meta schema. The correct location is contract-level (DataContractV1.keywords). NeitherDocumentTypeV0norDocumentTypeV1has akeywordsfield, andtry_from_schemanever 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
DataContractV1doc comment incorrectly stated the contract-level keyword limit as 20 when the validators andTooManyKeywordsErrorboth enforce 50.What was done?
keywordsblock frompackages/rs-dpp/schema/meta_schemas/document/v1/document-meta.json(the active meta schema)"keywords"fromALLOWED_DOCUMENT_SCHEMA_V1_PROPERTIESinallowed_top_level_properties.rs— required to keep the allowlist in sync with the schema (enforced by theallowlist_matches_v1_meta_schema_propertiesparity test)DataContractV1to state the correct contract-level max of 50 keywordsHow Has This Been Tested?
cargo test -p dpp— all 2462 dpp lib tests pass, includingallowlist_matches_v1_meta_schema_propertiescargo fmt --allcleanBreaking Changes
Document-type
keywordsis no longer accepted by v1 meta-schema validation. Any existing stored contract that happens to havekeywordson a document type will have it stripped during the v12 migration viastrip_unknown_document_schema_properties. This is safe —keywordson document types has never been read by Rust, so stripping it cannot affect runtime semantics. Contract-level keywords (DataContractV1.keywords) are unaffected.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Documentation
Changes
keywordsproperty from the document meta-schema;keywordsis no longer accepted there.keywordsfrom document schemas.Tests
keywordsare stripped from schemas and rejected by meta-schema validation.