Repository navigation
feat(dpp)!: consume and minimumAgeBlocks on any reference judged on the create alone - #5365
Conversation
…e create alone `consume` and `minimumAgeBlocks` needed a findBy function because only a commit-and-reveal key was judged on the create alone. A reference on a document type whose documents are never replaced (documentsMutable: false) is judged on the create alone too: a replace is the one write that re-validates a reference. `DocumentReferenceLookup::is_checked_on_create_only(documents_mutable)` covers both. The age and consume refusal moves from `parse_find_by`, which has no document type, to `validate_reference_lookup_sources` (`age_or_consume_error`); meta-schema v3 only requires findBy beside either keyword. The `$ownerId` source rule, `creatorRefersTo` with a deletable lookup and the create-only skips in reference validation 0 follow the new definition; the string carrier and the repeated-key preimage refusal keep testing for a hash key. Docs: meta-schema v3, v14 note 61, the book and the wasm-dpp2 TypeScript docs. Tests: dpp parses age and consume beside a plain findBy on an immutable type and refuses them on a mutable one, both parses; creatorRefersTo the same. drive-abci: a voucher/redemption contract consumes the voucher, refuses a same-block redemption (40142) and another identity's (40127). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to The change relaxes reference lookup rules for document types that cannot be replaced, and the schema, parser, runtime checks, docs and tests are consistent with it. No merge-blocking risk is evident from the supplied context. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly identifies the main change: allowing consume and minimumAgeBlocks on references checked only at creation. It is concise and specific. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 42 functions across 12 files. (5 skipped: … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
📝 Generate docstrings
-
-
- Commit to this branch
- Create a new PR
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-11T07:04:32.002Z |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit 10e5b83. Normal review starts when eligible; priority review starts as soon as a slot is available. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 60859a6. The schema relaxation introduces one blocking validation bypass: minimumAgeBlocks is accepted on inList references but discarded before enforcement; two non-blocking suggestions address owner-source regression coverage and public Rust API release metadata. This was a static review; the supplied CI snapshot still had Rust workspace tests and several functional/E2E checks pending.
🔴 1 blocking | 🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 7: gemini-3.8-flash-high (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff makes an intricate, coordinated change to consensus contract acceptance and document-reference validation, notably validate_reference_lookup_sources in rs-dpp and validate_reference_target_v0 in rs-drive-abci, expanding which references can consume documents and enforce minimum ages. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 24% left, 5h 100% left - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); 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-dpp/schema/meta_schemas/document/v3/document-meta.json`:
- [BLOCKING] packages/rs-dpp/schema/meta_schemas/document/v3/document-meta.json:871-873: Reject minimumAgeBlocks on inList references before its guard is discarded
Requiring only findBy now admits declarations such as {"type":"permanentDocument","documentType":"grant","findBy":{"$id":"grantId"},"inList":"members","minimumAgeBlocks":100}. However, parse_reference_target returns through parse_list_element_reference before reaching parse_find_by, and ListElementReference has no field for the minimum age. Its document-reference declaration exposes lookup: None, so validate_reference_lookup_sources skips it and runtime reference validation never reaches the age check, which requires Some(lookup). An immutable capability-use type can therefore declare this ownerRefersTo gate yet allow a listed member to create immediately after the grant, bypassing the declared activation delay. The previous meta-schema rejected this combination because its findBy contained only a string source. Reject minimumAgeBlocks with inList in both the meta-schema and parser unless list references explicitly preserve and enforce it, and add registration and same-block regression coverage.
In `packages/rs-dpp/src/data_contract/document_type/property/reference_lookup.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/property/reference_lookup.rs:440-449: Cover the newly admitted owner-derived lookup keys
This guard newly admits LookupKeySource::OwnerId on transferable or tradeable referring types through two routes: a plain lookup on an immutable type, or a computed lookup on a mutable type. The added voucher tests compare "$ownerId" in where rather than reading it in findBy, and the added creator lookup reads only ".", so neither exercises this relaxation. Existing writer-source tests cover refusal on mutable plain lookups. Add focused acceptance tests for both create-only routes, through validating and stored-contract parses, while retaining the mutable/plain refusal. These cases protect the distinction between using ownership to assemble the lookup key and comparing ownership after fetching the target.
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/property/reference_lookup.rs:313-314: Record the breaking public DPP method signature
DocumentReferenceLookup is publicly re-exported from dpp::data_contract::document_type, and the base exposes is_checked_on_create_only(&self). Adding the required documents_mutable argument makes downstream calls to lookup.is_checked_on_create_only() fail to compile regardless of their PlatformVersion. The protocol-14 parser gate protects consensus compatibility, not Rust source compatibility. Mark the PR title as feat(dpp)! and replace the Breaking Changes section's "None" with the signature change and caller migration so release metadata records it. Alternatively, preserve the existing signature and introduce the type-aware operation under a new name. This does not require another PlatformVersion slot.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
Conflicts with #5367, which let a string or byte array property carry a findBy read as ".", in the same places: - apply_revealed_reference_v0: #5367 no longer requires a function on a string or byte array carrier, so this branch's `hash_key().is_some()` there goes; any lookup is admitted. - Meta-schema v3 findBy description and the book (refers-to-lookup, documents): #5367's string-carrier wording beside this branch's minimumAgeBlocks/consume and "$ownerId" wording. - string_find_by_key_tests: `is_checked_on_create_only(true)`, the post type being mutable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With only findBy required beside minimumAgeBlocks or consume, meta-schema
v3 admitted them beside inList, whose findBy is { "$id": ... }. A list
element reference has no field for either, so parse_reference_target
dropped them and the age gate or the consume was never enforced. Both
are now refused: the meta-schema rule adds `not: { required: ["inList"]
}`, and parse_list_element_reference refuses them on every parse.
Tests (dpp):
- the inList refusal through both parses on a type never replaced,
which failed before the fix (the contract registered);
- a "$ownerId" findBy source on a type that can change owner, accepted
when the reference is judged on the create alone: a plain lookup on a
type never replaced (transferable and tradeable), and a function on a
mutable, transferable domain, each through both parses.
Docs: meta-schema descriptions, the book and v14 note 61.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuantumExplorer
changed the title
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at 6c54a45 and independently verified both new reviewer findings and all three prior findings. The prior findings are fixed, and no blocking defect remains; one stale parser comment and diagnostic still describe the former function-only rule. Validation was static-only: Rust workspace tests, JS and Docker builds, and Kotlin checks remained in progress in the supplied CI snapshot from 2026-10-11T06:26:35Z.
💬 1 nitpick(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
💬 Nitpick: Update stale findBy-function comment after moving age/consume gate
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs:1194-1207
This comment still says the findBy parser requires a function, and the missing-findBy diagnostic describes only a document found by a function. This PR removes that requirement from parse_find_by: age_or_consume_error now accepts a plain lookup when the declaring type's documents cannot be replaced. Update both descriptions to distinguish the requirement for findBy from the type-aware create-only check; enforcement itself is correct.
// `minimumAgeBlocks` and `consume` describe the document `findBy` finds
// when the referring document is created. They require `findBy`; the
// create-only requirement is checked once the declaring type is parsed.
if let Some(keyword) = [property_names::MINIMUM_AGE_BLOCKS, property_names::CONSUME]
.into_iter()
.find(|keyword| refers_to_map.contains_key(*keyword))
{
if !refers_to_map.contains_key(property_names::FIND_BY) {
return Err(DataContractError::InvalidContractStructure(format!(
"{reference_type} refersTo does not take {keyword} without findBy: it describes \
the document findBy finds"
)));
}
}
source: muse-spark-1.3-contributor (phase1-reviewer: general)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 13: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 18: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 19: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 20: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff intricately changes consensus contract acceptance and reference-validation semantics across schema parsing, create-only classification, and execution, notably in validate_reference_lookup_sources and DocumentReferenceLookup::is_checked_on_create_only. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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/class_methods/try_from_schema/mod.rs`:
- [NITPICK] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rs:1194-1207: Update stale findBy-function comment after moving age/consume gate
This comment still says the findBy parser requires a function, and the missing-findBy diagnostic describes only a document found by a function. This PR removes that requirement from parse_find_by: age_or_consume_error now accepts a plain lookup when the declaring type's documents cannot be replaced. Update both descriptions to distinguish the requirement for findBy from the type-aware create-only check; enforcement itself is correct.
… not a function The comment above the check and its error message still described the rule before this PR, where minimumAgeBlocks and consume needed a findBy function. The parse here only requires findBy. Whether the reference is judged on the create alone (a function, or a declaring type whose documents are never replaced) is checked once the type is parsed, and inList refuses both. Message only; the tests match its unchanged prefix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
On the nitpick in the last re-review (Update stale findBy-function comment after moving age/consume gate): fixed in 10e5b83. The comment now says this parse only requires 🤖 Addressed by Claude Code |
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
Basic explanation
What this does: A document can point at another document and, when it is created, delete that document (
consume) or require it to be old enough (minimumAgeBlocks). Until now that only worked for one kind of pointer, a commit-and-reveal hash. What these keywords really need is a pointer that is checked only once, when the document is created: a later edit would check it again and find the deleted document gone. A document type whose documents can never be edited (documentsMutable: false) has exactly that property. This PR lets such types useconsumeandminimumAgeBlockswith anyfindBy, hash or not.Value: Contracts get "spend a voucher" or "redeem a ticket" flows without inventing a hash: the create finds the writer's voucher by its serial and deletes it in the same write. This is the first prerequisite for DPNS name tombstones, where a tombstone consumes the name's domain.
Risks: Low to medium. This changes consensus rules of protocol version 14, which is unreleased: which contracts are accepted.
findBymay read the writer ($ownerId) even when documents can be transferred or sold;creatorRefersTomay point at a deletable document found byfindBy.minimumAgeBlocksandconsumesit besideinList, a reference that checks a list holds the value. That kind of reference has nowhere to keep them, so they were accepted and then silently ignored. Both are now refused at registration.Issue being fixed or feature implemented
consumeandminimumAgeBlockswere refused unless the reference'sfindByheld a hash function. The reason, inDocumentReferenceLookup::is_checked_on_create_only, was that such a reference is judged on the create alone and never again. But a reference declared on a type whose documents are never replaced is judged on the create alone too: a replace is the one write that re-validates a reference (transfers, purchases, price updates, moderator field changes and restores never do, andchangeFieldsmay not name a property a reference reads).What was done?
DocumentReferenceLookup::is_checked_on_create_only(documents_mutable): a computed key, or a declaring type whose documents are never replaced.minimumAgeBlocksandconsumemoves fromparse_find_by, which has no document type, tovalidate_reference_lookup_sources(newage_or_consume_error), which runs on every parse once the type is known. Meta-schema v3 now only requiresfindBybeside either keyword."$ownerId"findBysource on a transferable or tradeable referring type, now admitted on any reference judged on the create alone (a function infindByor a type never replaced): a transfer moves the writer part of the key, but nothing checks the reference again;creatorRefersTowith adeletableDocumentfound byfindBy;whereandanyOfrule (create_only_leaf_error) and the immutable-property exception, both no-ops on an immutable type.hash_key().is_some()). The string or byte array carrier needed the same spelling until feat(dpp)!: find a referenced document by a string or byte array property's value #5367 merged; it now admits anyfindBy, so the merge takes feat(dpp)!: find a referenced document by a string or byte array property's value #5367's version there.minimumAgeBlocksandconsumeare refused besideinList. A list element reference (ListElementReference) has no field for them, so with onlyfindByrequired they would register and be dropped unenforced. Two places refuse them: meta-schema v3, whose rule now also saysnot: { required: ["inList"] }, andparse_list_element_reference, which refuses them on every parse.Before and after
A
redemptionthat consumes the writer'svoucher, found by its serial without a function:findByholding a function beside either keyword, and the parser refused it as well ("refersTo minimumAgeBlocks and consume need a findBy function computing the key").documentsMutable: trueis still refused (10231): "the reference must be judged on the create alone: give findBy a function, or make the document type immutable".In-place changes to shipped generations
binds_a_changed_property,validate_reference_target_v0): every protocol version selects it. Only lookups reach the changed lines, and only meta-schema v3 (protocol version 14) admitsfindBy, so nothing changes before 14. A replace never reaches either line for a type whose documents are never replaced (document replace advanced structure refuses it first), so the behaviour at 14 is unchanged too.How Has This Been Tested?
should_parse_age_and_consume_without_a_function_on_a_type_never_replaced;should_refuse_age_and_consume_without_a_function_on_a_type_whose_documents_can_be_replaced(validating and stored-contract parses);should_parse_a_deletable_creator_lookup_without_a_computed_key_on_a_type_never_replacedand its refused twin on a replaceable type;should_refuse_minimum_age_blocks_and_consume_beside_in_list, on a referring type never replaced, through both parses. It failed before the fix: the contract registered with the keyword dropped;should_accept_find_by_reading_the_writer_on_a_type_never_replaced_that_can_change_owner(transferable and tradeable, both parses), beside the existing refusal on a replaceable type;should_accept_a_find_by_function_reading_the_writer_on_a_type_that_can_change_owner(a mutable, transferable domain whose reveal finds a preorder unique on its hash and owner, both parses).commit_reveal_lookup.rs, avoucher/redemptionfixture):should_consume_a_document_a_plain_lookup_finds_on_a_type_never_replaced;should_refuse_a_plain_lookup_redemption_in_the_block_of_its_voucher(40142);should_refuse_redeeming_another_identitys_voucher_through_a_plain_lookup(40127).cargo test -p dpp --lib,cargo test -p drive-abci --lib -- commit_reveal refers_to reference dpns document moderation,cargo clippy -p dpp -p drive-abci -p dash-sdk --all-targets,cargo check -p wasm-dpp2,cargo fmt --all -- --check.Breaking Changes
Consensus-breaking within protocol version 14, which is unreleased: nodes must agree on which contracts register.
minimumAgeBlocksandconsumebeside a plainfindByon a type whose documents are never replaced, and a"$ownerId"findBysource on such a type that can change owner, are refused by earlier protocol version 14 builds and accepted by this one.minimumAgeBlocksandconsumebesideinListare refused.DocumentReferenceLookup::is_checked_on_create_onlytakesdocuments_mutable, whether the declaring type's documents can be replaced. A caller that meant "has a computed key" callshash_key().is_some(); one that means "judged on the create alone" passesdocument_type.documents_mutable().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 ·
10e5b83/skip-botsproceeds without the ones not yet reported/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit
minimumAgeBlocksandconsumewithout afindByfunction. These rules apply when the reference is checked at creation.$ownerIdon transferable or tradable document types. Creator references can also target deletable documents that cannot be replaced.minimumAgeBlocksandconsumeoninListreferences and on lookups that are rechecked when documents are replaced.