Repository navigation
fix(dpp)!: refuse a dotted property path in sum and average keywords (PV14) - #5104
Conversation
|
Warning Review limit reachedNext included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: dashpay/platform/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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 |
|
🕓 Queued for automated review — 2nd in line, estimated start in ~30 min (commit 458a3ae)
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The production change is confined to the PV14-only document meta-schema, preserving earlier protocol validation and non-validating loading of stored contracts. No blocking defects were found; two non-blocking gaps remain in the user-facing documentation and update-path regression coverage. Verification was based on source inspection at the exact head; tests were not independently rerun.
🟡 2 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Document the PV14 top-level-only aggregate property rule
book/src/contract-keywords/aggregates.md:154-160
This chapter describes all four aggregate keywords and their registration requirements, but it does not mention the restriction introduced here. Its value tables still say only that the value is an integer property's name of 1–64 characters. Add the top-level-only requirement for summable, averageable, documentsSummable, and documentsAverageable, explaining that PV14 rejects dotted paths such as payment.amount with JsonSchemaError (10101). This makes the breaking registration change discoverable in the contract-author guide rather than only in the meta-schema and protocol-version notes.
source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, platform-versioning, rust-quality)
🟡 Suggestion: Add an update-path regression for dotted aggregate names
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/contract_structure_error_tests.rs:103-113
The new tests cover dotted-name rejection through contract creation, but the update suite still covers only the summed-u64 and terminal parser errors. The PR also promises that an update carrying a dotted aggregate name is admitted by check_tx and rejected in a block with a processing charge and an identity-contract nonce bump. The existing generic consensus-error handling supports that behavior, so this is a coverage gap rather than an observed implementation defect. Add a regression using update_contract_adding_schema to introduce a new document type with summable: "payment.amount", then assert the pattern JsonSchemaError, paid rejection, and contract nonce bump. This would cover the update-specific action and nonce behavior for the new meta-schema rejection.
source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, platform-versioning, rust-quality)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The behavior change is a small, contained addition of four property-name regex constraints to the PV14 document meta-schema, with the remaining changes primarily regression tests and version documentation, so it is not large or intricate despite affecting consensus validation. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 87% left, weekly 13% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort medium); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort medium); agentphase2-reviewer
🤖 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 `book/src/contract-keywords/aggregates.md`:
- [SUGGESTION] book/src/contract-keywords/aggregates.md:154-160: Document the PV14 top-level-only aggregate property rule
This chapter describes all four aggregate keywords and their registration requirements, but it does not mention the restriction introduced here. Its value tables still say only that the value is an integer property's name of 1–64 characters. Add the top-level-only requirement for `summable`, `averageable`, `documentsSummable`, and `documentsAverageable`, explaining that PV14 rejects dotted paths such as `payment.amount` with `JsonSchemaError` (10101). This makes the breaking registration change discoverable in the contract-author guide rather than only in the meta-schema and protocol-version notes.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/contract_structure_error_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/contract_structure_error_tests.rs:103-113: Add an update-path regression for dotted aggregate names
The new tests cover dotted-name rejection through contract creation, but the update suite still covers only the summed-u64 and terminal parser errors. The PR also promises that an update carrying a dotted aggregate name is admitted by `check_tx` and rejected in a block with a processing charge and an identity-contract nonce bump. The existing generic consensus-error handling supports that behavior, so this is a coverage gap rather than an observed implementation defect. Add a regression using `update_contract_adding_schema` to introduce a new document type with `summable: "payment.amount"`, then assert the pattern `JsonSchemaError`, paid rejection, and contract nonce bump. This would cover the update-specific action and nonce behavior for the new meta-schema rejection.
|
Bots are done — your move: post |
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At faf90fe, the complete PR diff correctly confines the aggregate-name restriction to the unreleased PV14 meta-schema and preserves historical validation and stored-contract loading. No blocking defect was found; the update-path regression and contract-author documentation suggestions remain applicable. Verification was by source inspection and git diff --check; tests were not independently rerun.
🟡 2 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Add an update-path regression for dotted aggregate names
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/contract_structure_error_tests.rs:103-113
The existing update regressions exercise parser-level structural errors that check_tx already rejects, whereas this new meta-schema rule admits the transition in check_tx and rejects it during full block validation. The added create test covers that distinction only for creation; updates use a separate conversion path and BumpIdentityDataContractNonceAction. Add a PV14 case using update_contract_adding_schema with a dotted summable name, asserting check_tx admission, PaidConsensusError containing JsonSchemaError with keyword pattern at /indices/0/summable, the contract nonce advancing to UPDATE_NONCE, and a processing-fee deduction. This protects the update-specific behavior explicitly promised by this PR without duplicating the four-keyword matrix already covered in DPP.
source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, platform-versioning, rust-quality); gpt-6-astra (phase2-reviewer: general, platform-versioning, rust-quality)
🟡 Suggestion: Document the PV14 top-level-only aggregate property rule
book/src/contract-keywords/aggregates.md:154-160
The contract-author reference still describes these values as integer-property names of 1–64 characters and lists existence, required membership, and sum bounds, but never states the new top-level-only restriction. Add a versioned rule here explaining that from PV14, summable, averageable, documentsSummable, and documentsAverageable must name top-level properties matching ^[a-zA-Z0-9_]{1,64}$, and that dotted paths are rejected on both contract creation and update. The schema descriptions and PV14 implementation note already explain this, but readers of the keyword guide currently cannot discover the new registration constraint or its activation boundary.
source: muse-spark-1.3-contributor (phase1-reviewer: general, architecture-layering, platform-versioning, rust-quality); gpt-6-astra (phase2-reviewer: general, platform-versioning)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: architecture-layering); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: platform-versioning); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 12: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
lowbygpt-6-astra(effort low) — The behavioral change is a small, contained PV14-only meta-schema restriction on four aggregate property names, with most of the diff devoted to regression tests rather than intricate consensus logic. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— architecture-layering (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— platform-versioning (completed, effort high); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 13% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 87% left, weekly 13% left) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort medium); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort medium); agentphase2-reviewer,gpt-6-astra— general (completed, effort medium); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort medium); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort medium); agentphase2-reviewer
🤖 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-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/contract_structure_error_tests.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/data_contract_update/contract_structure_error_tests.rs:103-113: Add an update-path regression for dotted aggregate names
The existing update regressions exercise parser-level structural errors that check_tx already rejects, whereas this new meta-schema rule admits the transition in check_tx and rejects it during full block validation. The added create test covers that distinction only for creation; updates use a separate conversion path and BumpIdentityDataContractNonceAction. Add a PV14 case using update_contract_adding_schema with a dotted summable name, asserting check_tx admission, PaidConsensusError containing JsonSchemaError with keyword pattern at /indices/0/summable, the contract nonce advancing to UPDATE_NONCE, and a processing-fee deduction. This protects the update-specific behavior explicitly promised by this PR without duplicating the four-keyword matrix already covered in DPP.
In `book/src/contract-keywords/aggregates.md`:
- [SUGGESTION] book/src/contract-keywords/aggregates.md:154-160: Document the PV14 top-level-only aggregate property rule
The contract-author reference still describes these values as integer-property names of 1–64 characters and lists existence, required membership, and sum bounds, but never states the new top-level-only restriction. Add a versioned rule here explaining that from PV14, summable, averageable, documentsSummable, and documentsAverageable must name top-level properties matching ^[a-zA-Z0-9_]{1,64}$, and that dotted paths are rejected on both contract creation and update. The schema descriptions and PV14 implementation note already explain this, but readers of the keyword guide currently cannot discover the new registration constraint or its activation boundary.
|
Bots are done — your move: post |
…(PV14)
summable and averageable on an index, and documentsSummable and
documentsAverageable on a document type, name the integer property each
document adds to the sum. Drive reads that value from the top level of
the document, but the parser resolves the name among the flattened
properties and required fields, which also hold dotted paths, and the
meta-schemas bounded only the name's length. A contract naming a nested
property such as "payment.amount" registered, and then every document
create of the type failed as an internal error.
Document meta-schema v3, selected only by protocol version 14, gives the
four keywords the property-name pattern ^[a-zA-Z0-9_]{1,64}$. A create or
update carrying a dotted name is refused under full validation with a
JsonSchemaError (10101), paid in a block. Meta-schema v2 is unchanged, so
protocol version 13 keeps admitting the name, and a stored contract keeps
loading because it is parsed without full validation. No contract on
mainnet or testnet uses a dotted name in any of these keywords.
PV14 note 54.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
faf90fe to
458a3ae
Compare
Basic explanation
What this does: Four contract keywords name the number property that the platform adds up across documents:
summableandaverageableon an index, anddocumentsSummableanddocumentsAverageableon a document type. Contracts could register a dotted name that document inserts can never read. A dotted name is the path to a property inside an object, such as"payment.amount". Contract registration accepted it, but when a document is saved the platform looks up the name only among the document's top-level properties. It never finds it, so every document create of that type failed. From protocol version 14 the platform refuses the dotted name when the contract is registered or updated, with an error that names the keyword.Example:
Before: a contract with this document type registered fine, and then every document create of
itemfailed as an internal error:{ "type": "object", "properties": { "payment": { "type": "object", "properties": { "amount": { "type": "integer", "minimum": 0, "maximum": 1000, "position": 0 } }, "required": ["amount"], "additionalProperties": false, "position": 0 }, "label": { "type": "string", "maxLength": 20, "position": 1 } }, "required": ["payment", "label"], "indices": [ { "name": "byLabel", "properties": [{ "label": "asc" }], "summable": "payment.amount" } ], "additionalProperties": false }After (protocol version 14): the contract is refused at registration:
Value: A contract that could never hold a document is now refused when it is registered, with a clear error, instead of registering and then failing on every document create.
Risks: Low. The change is in the document meta-schema that only protocol version 14 uses, and version 14 is unreleased. Protocol versions 12 and 13 behave exactly as before. A contract already stored with a dotted name still loads. Its owner could no longer update it at protocol version 14, but no contract on mainnet or testnet uses a dotted name in any of these four keywords.
Issue being fixed or feature implemented
This has been possible since protocol version 12, when these keywords were introduced.
The parser checks the name in
apply_doctype_aggregates(packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs). It looks the name up inflattened_propertiesandrequired_fields, and both hold dotted paths for properties nested in objects, so"payment.amount"passes as a required integer. The document meta-schemas (v1, v2 and v3) bounded only the name's length. Drive, however, reads the value withdocument.properties().get(sum_property)inread_document_sum_contribution(packages/rs-drive/src/drive/document/mod.rs), which is a top-level lookup, so it never finds the value.Measured at protocol version 13 with the contract above:
The proposer leaves the document create out of its block, and
process_proposalrejects a block that contains it. Nothing is written and no fee is charged. The contract simply can never hold a document of that type.A scan of every contract create and update on mainnet and testnet found no contract that uses a dotted value in any of the four keywords. Mainnet uses none of these keywords at all.
What was done?
packages/rs-dpp/schema/meta_schemas/document/v3/document-meta.json:summable,averageable,documentsSummableanddocumentsAverageableget"pattern": "^[a-zA-Z0-9_]{1,64}$". This is the same property-name pattern v3 already uses for property names and other keywords that name a property (for exampleboundTo). It refuses the.. It also refuses any other character that v3 does not allow in a property name, but such a name could not match a property anyway. Each keyword's description and the file's$commentnow say this. Meta-schema v3 is selected only by protocol version 14 (CONTRACT_VERSIONS_V6,document_type_schema: 3). Protocol version 12 selects v1 and protocol version 13 selects v2, and neither is touched.apply_doctype_aggregatesis not changed, because it runs without full validation and is shared with the shipped parser generation 2.packages/rs-platform-version/src/version/v14.rs: PV14 note 54.data_contract_create/contract_structure_error_tests.rs,register_contract_with_schemanow also returns the platform, owner and key it registered with, so a test can go on to use the contract. The three existing tests take.outcome.How the refusal surfaces at protocol version 14:
BasicError::JsonSchemaError(code 10101) with keywordpatternand the keyword's path (/indices/<n>/summable,/indices/<n>/averageable,/documentsSummableor/documentsAverageable). Meta-schema checks run only under full validation.check_txparses a contract without full validation (should_fully_validate_contract_on_transform_into_actionisfalseforCheckTx), socheck_txadmits the transition, as it does for any meta-schema violation. In a block it is a paid rejection: the create bumps the identity nonce, an update bumps the identity contract nonce, and the processing fee is charged.How Has This Been Tested?
New dpp tests (
try_from_schema/v3/dotted_aggregate_name_tests.rs):should_refuse_a_dotted_name_in_each_aggregate_keyword_at_registration: at the latest protocol version, all four keywords with"payment.amount"are refused with aJsonSchemaError, keywordpattern, at the keyword's path.should_accept_a_top_level_name_in_each_aggregate_keyword_at_registration: a top-level name is still accepted in all four.should_keep_accepting_a_dotted_name_at_protocol_version_13: meta-schema v2 still accepts the dotted name in all four.should_load_a_contract_registered_at_protocol_version_13_with_a_dotted_name_at_protocol_version_14: a contract registered and serialized at protocol version 13 deserializes at protocol version 14 without full validation, as Drive loads a stored contract.New drive-abci tests (
data_contract_create/contract_structure_error_tests.rs):should_refuse_a_dotted_summable_name_with_a_paid_consensus_error: at protocol version 14,check_txadmits the create, and the block returnsPaidConsensusErrorwith theJsonSchemaErrorat/indices/0/summable, bumps the nonce and charges the owner.should_keep_registering_a_dotted_summable_name_at_protocol_version_13: at protocol version 13 the contract registers, and a document create of the type still fails as before (check_txerror andInternalErrorin the block, owner untouched).Commands:
cargo test -p dpp --lib --all-features dotted_aggregate_name: 4 passed. The refusal test fails with the pattern removed.cargo test -p dpp --lib --all-featureswith the filterstry_from_schema(493 passed),meta_validators(35),validate_schema_compatibility(17),summable(26),averageable(31).cargo test -p drive-abci --lib contract_structure_error_tests: 8 passed (create and update).cargo test -p drive-abci --lib summable: 3 passed.cargo test -p platform-version: all passed.cargo fmt --all;cargo clippy -p dpp --tests --all-features -- -D warnings,cargo clippy -p drive-abci --tests -- -D warningsandcargo clippy -p platform-version --tests -- -D warnings: clean.Breaking Changes
Consensus change at protocol version 14 (unreleased): a data contract create or update whose document type names a dotted path in
summable,averageable,documentsSummableordocumentsAverageableis refused withJsonSchemaError(10101), paid. As a side effect, the owner of a contract already stored with such a name could not update it from protocol version 14. No such contract exists on mainnet or testnet. Protocol versions 12 and 13 are unchanged.In-place changes to shipped generations
None. Meta-schema v3 is selected only by protocol version 14, which is unreleased, and meta-schemas v1 and v2 are unchanged.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
458a3ae/skip-botsproceeds without the ones not yet reported/self-reviewedonce the bots are doneWhen every box is checked the
PR Hygienecheck passes and this can merge.