Repository navigation
feat(dpp): consume and minimumAgeBlocks on any reference judged on the create alone - #5365
QuantumExplorer wants to merge 1 commit into
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>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (14)
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 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-09T16:40:59.939Z |
|
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
|
⛔ Final review complete — 1 blocking finding(s) (commit 60859a6) · triage: critical |
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.
| "then": { | ||
| "required": ["findBy"], | ||
| "properties": { | ||
| "findBy": { | ||
| "not": { "additionalProperties": { "type": "string" } } | ||
| } | ||
| } | ||
| "required": ["findBy"] | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: 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.
| "then": { | |
| "required": ["findBy"], | |
| "properties": { | |
| "findBy": { | |
| "not": { "additionalProperties": { "type": "string" } } | |
| } | |
| } | |
| "required": ["findBy"] | |
| } | |
| "then": { | |
| "required": ["findBy"], | |
| "properties": { | |
| "inList": false | |
| } | |
| } |
source: gpt-6.1-sol (phase2-reviewer: security-auditor)
| LookupKeySource::OwnerId => { | ||
| if owner_can_change(declaring) { | ||
| // A lookup judged when the document is created only, so a later transfer | ||
| // or purchase moving the writer moves no key | ||
| if !create_only && owner_can_change(declaring) { | ||
| return Some(format!( | ||
| "findBy \"{index_property}\" reads \"$ownerId\", which a transfer or \ | ||
| a purchase of the referring document changes without re-validating \ | ||
| the reference: findBy may read the writer only on a document type \ | ||
| that cannot be transferred or traded" | ||
| that cannot be transferred or traded, beside a function, or on a \ | ||
| type whose documents are never replaced" |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: gpt-6.1-sol (phase2-reviewer: rust-quality)
| pub fn is_checked_on_create_only(&self, documents_mutable: bool) -> bool { | ||
| self.hash_key().is_some() || !documents_mutable |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, platform-versioning, rust-quality, security-auditor)
|
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
This branch has not been deployed
2 participants
Basic explanation
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. The real requirement was never the hash. It was that the pointer is only checked once, when the document is created, because a later edit would check it again and find the consumed document gone.A document type whose documents can never be edited (
documentsMutable: false) has exactly that property: nothing ever re-checks its pointers. This PR lets such types useconsumeandminimumAgeBlockswith anyfindBy, hash or not. It is the first prerequisite for DPNS name tombstones, where a tombstone consumes the name's domain.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, which still needs a function, and the repeated-key preimage refusal.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;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
None. Contracts that registered before still register; this only admits declarations that were refused.
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 ·
60859a6/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.