Repository navigation
refactor(platform): share the delete state checks and cover what delete rules leave alone - #5330
Merged
Conversation
…te rules leave alone Follow-ups to the deleteConstraints review (#5319): - Document delete state validation 0's checks (base transition, document type, stored document, owner) move into `fetch_owned_document`, which returns the owned document; version 0 keeps its outcome and billing, and version 1 builds on it instead of repeating them. - `apply_property_constraints` version 0 also reads `deleteConstraints`, so one dispatch on `parse_property_constraints` covers both keywords. - A gated delete converts the stored properties to a value once, for both the totals and the rules. - `read_delete_constraint_aggregates` documents that it leaves out the batch's own earlier writes, sound only while a batch carries one transition. - Tests: a moderator deletes what the owner's rules refuse, a banned owner's delete is still judged, a ttl expires a document its owner may not delete, and a contested type's own total is accepted in delete rules and still refused in propertyConstraints. The billing test asserts each read is charged, not that the two charges are equal. - References chapter: a deletableDocument target can refuse its owner's delete while documents count toward a rule matched by its `$id`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
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 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (16)
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 |
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-08T11:32:51.965Z |
Contributor
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
Collaborator
|
🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.
Commit 8148a3f. Normal review starts when eligible; priority review starts as soon as a slot is available. |
This was referenced Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Basic explanation
What this does: Clean-ups and extra tests for the delete rules (
deleteConstraints) that landed in #5319, from its code review, which finished after that PR merged. The checks every owner's delete runs (the document exists, the signer owns it) now live in one function that both versions of delete validation share, instead of being copied. Both rule keywords are parsed under one version switch. A delete with rules converts the stored document once instead of twice. New tests pin what the rules leave alone: moderators and expiry still delete, and a banned owner is still held to them.Value: Less duplicated consensus code to keep in step, slightly less work per gated delete, and tests that would catch a later change routing moderator deletes or
ttlexpiry through the rules, or letting a barred owner skip them. A docs gap in the References chapter is closed.Risks: Low. Nothing a user or a contract can observe changes. The one edit to a generation shipped before protocol version 14, document delete state validation 0, moves its body into a shared function with the same checks, in the same order, with the same billing and the same errors (see below). Everything else is protocol version 14 code, not yet released, or tests and docs.
Issue being fixed or feature implemented
The review of #5319 (deleteConstraints) finished after it merged. This PR applies its findings, except one design follow-up (below).
What was done?
No behaviour changes; each item is a refactor, a doc fix or a test.
Shared delete checks. Document delete state validation 0's checks (base transition, document type, stored document fetch, owner) move into
fetch_owned_document, which returns the owned document or the first refusal. Version 0 returns its errors; version 1, which judgesdeleteConstraints, builds on the document it returns instead of repeating about 45 lines.One parse dispatch.
apply_property_constraintsversion 0 now also readsdeleteConstraints(apply_delete_constraints_v0), so a single match onparse_property_constraintscovers both keywords and they can't drift apart. The separateapply_delete_constraintsdispatcher is gone.One conversion per gated delete. State validation 1 converts the stored properties to a
Valueonce and passes it to bothread_delete_constraint_aggregatesandvalidate_delete_constraints(which now takes theValue). Before, each cloned the document.A documented assumption.
read_delete_constraint_aggregatesreads committed state, so it leaves out writes earlier in the same batch. That is sound only while a document batch carries one transition, and the function now says so, as the write path's reader already did.Docs. The References chapter said deleting a referenced document never looks at referrers. It now notes that a
deleteConstraintsrule counting referrers by the target's$idholds adeletableDocumenttarget in place until the last one is gone.Tests.
contract_user_moderation/tests/delete_constraints.rs: a moderator deletes a post its owner's rule refuses; a banned owner's delete passes the moderation gate and is still judged (refused 40147 by the rule, accepted when the rule holds).document_ttl: a note its owner may not delete (refused 40147) still expires on time.property_constraint_aggregates_tests: the same own-type count on a contested type is refused inpropertyConstraintsand accepted indeleteConstraints.drive::query::path imported, a throwaway tuple removed.Not done here: letting a
permanentDocumentreference target a type whose delete rule proves nothing points at it. That is a feature of its own, listed as a follow-up in #5319.In-place changes to shipped generations
state_v0, shipped since protocol version 1): its body moves intofetch_owned_document, andvalidate_state_v0returns that helper's errors. The checks run in the same order, with the same reads billed the same way and the same errors, so its outcome is unchanged at every protocol version.check_ownershipis private to the module again.property_constraint_aggregates.rs(shared with earlier versions): the reader takes prebuilt values. It reads a total only for a rule that reads one, which only the protocol version 14 parser produces, so earlier versions read and bill nothing, as before.validate_delete_constraints, state validation 1) is selected by protocol version 14 only, which is not yet released.How Has This Been Tested?
delete_constraints property_constraints document_ttl contract_user_moderation deletion commit_reveal_lookup deletable_document_reference, including the four new tests.cargo clippy -p dpp --features all_features --all-targets,cargo clippy -p drive-abci --all-targetsandcargo fmt --checkare clean.Breaking Changes
None. No consensus behaviour changes at any protocol version.
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 ·
8148a3f/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.