feat(platform)!: only members added before a document approve its settled deletion (PV14) - #5260
Conversation
…tled deletion (PV14) A seated team's leader names whom it adds (addedModerator), so it could add members who approve whatever it proposes, delete a settled document with them, and take them off again. `moderatorAbilities.deleteSettled` gains `approversPredateDocument` (default true): a member the leader added proposes or approves the deletion of a document only when its addition's $createdAt is earlier than the document's. The leader and the elected members always count. While on, the type must require $createdAt. A proposal or approval by a later member is refused with ContractTeamMemberAddedAfterDocumentError (41212); an approval that reads the team drops the approval of a member taken off and added again too late, refunded as a departed member's is. What the rule needs is not lowered, so the leader can not shrink the bar. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
the
waiting-bots
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSettled-document deletion now has a configurable rule for whether leader-added members must predate a document to propose or approve its deletion. The change records team-seat addition times, validates document creation timestamps, filters approvals, and adds consensus error 41212 for ineligible members. ChangesSettled deletion eligibility
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Transition as Contract moderation transition
participant Moderators
participant Charter as SeatedModerationCharter
participant Rule as SettledDeletionRule
Transition->>Moderators: Resolve signer seat
Moderators->>Charter: Look up seat and addition time
Charter-->>Moderators: Return moderator seat
Transition->>Rule: Check seat against document creation time
Rule-->>Transition: Return eligibility
Transition->>Transition: Refuse an ineligible proposal or approval
Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue is established for the settled-deletion eligibility change; it is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The rule strengthens deletion approvals without reducing the required quorum. The main remaining risk is compatibility if pre-change, pre-release state is reused: existing documents could lose an attainable deletion quorum. Affected deployed contracts have not been established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
…approval-deletions-76b287 # Conflicts: # packages/rs-dpp/src/errors/consensus/state/state_error.rs
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-04T06:55:40.516Z |
|
🔍 Review in progress — actively reviewing now (commit f3dcc89) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of the supplied reviewer evidence against head 342ad1f found no actionable in-scope defects. The PV14-only eligibility rule covers proposals, approvals, and previously recorded signers through both seat lookup paths, preserving billed reads, approval thresholds, and append-only consensus error serialization. No local builds or tests were run; the supplied CI snapshot reports successful Rust workspace tests and relevant JS/WASM checks, while Swift SDK build/tests, the full test suite, one browser shard, and PR Hygiene remain pending.
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: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (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 large, cross-cutting diff changes consensus acceptance rules in transform_settled_deletion_proposal_v0 and transform_team_action_approval_v0 by enforcing seat-dependent timestamp eligibility for settled deletion approvals. - 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 15% 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 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
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
- The $createdAt requirement is checked at registration only, as the approvals bound is: deleteSettled shipped in 5.0.0-beta.1, and a stored type without $createdAt must still read back (a document without it admits no added member). - approversPredateDocument defaults to true only when approvals is above 1: a rule one approval meets, the leader meets alone, so dating added members protects nothing there. - An approval checks the late addition (41212) before an approval already given (41208), so a member re-added too late is told why. - Docs warn that a rule asking for more approvals than the leader and the elected members never deletes content older than the seat. - fetch_active_members reuses fetch_active_seats; late_addition_refusal calls admits_addition directly. - Tests date additions and documents after the award's block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
Static verification of the complete diff found no in-scope blockers and confirmed one nonblocking coverage gap for legacy documents without creation timestamps. The eligibility checks, approval filtering, frozen-rule validation, and appended consensus error are consistent with the PV14 scope. The supplied CI snapshot reports passing Rust workspace and relevant JS checks, with two test suites and PR Hygiene pending; the failed Swift job log shows a simulator-destination mismatch rather than a defect attributable to this diff.
🟡 1 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: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: 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) — This cross-cutting change alters consensus-critical settled-deletion eligibility and approval counting in rs-drive-abci's seated_moderation_charter/mod.rs and contract_user_moderation/state/v0/mod.rs, alongside schema validation and PV14 activation. - 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 15% 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.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
🤖 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/contract_user_moderation/state/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/contract_user_moderation/state/v0/mod.rs:1151-1152: Exercise the missing creation timestamp fallback on legacy stored documents
The supported legacy-contract behavior depends on treating a missing document `$createdAt` as zero here and separately when calling `still_counted_approvers` at line 965. The DPP regression verifies that a stored schema without required `$createdAt` still parses, but the settled-deletion execution fixtures now require `$createdAt`, so they do not exercise either runtime fallback. The implementation currently fails closed correctly, but the compatibility guarantee is unprotected beyond parsing. Add a stored-state execution regression with a document lacking `$createdAt`: assert that leader and elected seats remain eligible, an added member receives 41212, and previously stored added-member approvals are dropped through both the point-read and whole-team-read branches. Include the explicit opt-out case to verify that it still admits added members without a creation timestamp.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Bound accumulated stale approvals and full signer-tree reads — The existing approval path retains stale signers whenever a leader-required action lacks the leader's approval. Consequently, historical approvals can exceed concurrent team capacity, while
contract_team_action_signers_queryperforms an unsized full-range read also used by the public signer read/proof endpoint. The retention branch and unsized query already exist at base commit 4cf12f2; this PR restricts eligibility rather than introducing that resource-management issue.- Follow-up: Track a separate resource-bounding issue for stale signer retention and signer reads/proofs, with regression coverage demonstrating that historical membership changes cannot invalidate the assumed query bound.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
…rry no $createdAt A contract stored before a dated rule needed $createdAt can hold a type whose rule dates added members while its documents record no creation time. Two tests write such types straight to Drive: under the dated rule no added member proposes or approves (41212), approvals a node stored before are dropped and refunded through both the per-seat and the whole-team read, and the leader and elected member still count; under the opt-out, added members complete the rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
waiting-bots
2 participants
Basic explanation
What this does: An elected moderation team can delete an old ("settled") document when enough of its members approve. Until now, the team's leader could appoint new members at will, have them approve, delete the document, and remove them again, so "the leader and two others agree" really meant "the leader agrees". Now a member the leader appointed only counts for documents created before that member was appointed. The leader and the elected members always count. The rule is on by default whenever more than one approval is needed (with one, the leader can already delete alone, so appointments change nothing); a contract can turn it off with
approversPredateDocument: false.Value: The approval count of a settled deletion means what it says again. A leader can no longer get around the other members by appointing approvers after the fact.
Risks: Medium, consensus, protocol version 14 only (unreleased). A new document type using
deleteSettledwith more than one approval must list$createdAtinrequiredunless it turns the option off; this is checked at registration only, so contracts already stored on a 5.0.0-beta.1 network still load. A team with too few members from before a document can never delete that document once settled: the required count is deliberately not lowered, since lowering it would hand the leader the same lever. In particular, content written before the team was seated counts only the leader and the elected members, so a rule asking for more approvals than those never deletes it; the docs say to sizeapprovalsaccordingly. A leader can still appoint approvers before content exists; this closes appointments made after the document.Issue being fixed or feature implemented
moderatorAbilities.deleteSettled(#5215) counts approvals from any current member of the seated team. Members the leader adds (addedModeratordocuments of the moderation charters contract) are chosen by the leader alone, up to the declaration'smaxAddedModerators, and can be taken off again by deleting the addition. So the leader can meet anyapprovalscount with members of its own choosing and remove them afterwards.What was done?
New key
deleteSettled.approversPredateDocument(boolean, defaulttruewhenapprovalsis above 1,falseotherwise;SettledDeletionRule::approvers_predate_document,SettledDeletionRule::admits_addition). While on, a member the leader added counts only when itsaddedModerator's$createdAtis strictly earlier than the document's$createdAt(an addition in the same block does not count). The leader and elected members always count. Frozen with the rest of the rule on update (40212); added to meta-schema v3.Before:
Parsed. The leader adds two members a year after a post was written; leader + those two delete it.
After: registering the same schema is refused (10231): "list
$createdAtinrequired, or setapproversPredateDocument: false" (a stored contract is still read back; a document without$createdAtadmits no added member). With"required": ["$createdAt", "$updatedAt", "text"]it parses, and each of the two late members' proposals or approvals of that post is refused with 41212.keeps the old behaviour (members count whenever added) and needs no
$createdAt; so does{ "leader": true }, whose single approval the leader meets alone.New error
ContractTeamMemberAddedAfterDocumentError(41212), appended (StateError discriminant 170, after the token shielded pool errors of feat(platform)!: token shielded pools #4760 that took 167 to 169), mapped in wasm-dpp. Raised bytransform_settled_deletion_proposal_v0after the settled check and bytransform_team_action_approval_v0after the document-changed check (late_addition_refusal). The approval's already-signed check (41208) now comes after it, so a member re-added too late, whose earlier approval no longer counts, gets 41212.Seat-aware team reads (
seated_moderation_charter):TeamSeat { Leader, Elected, Added { added_at } },SeatedModerationCharter::seat_of(the same single queryseatsmade, now keeping the addition's$createdAt;seatsdelegates) andfetch_active_seats(the same two queries asfetch_active_members).Moderators::seat_of/ModeratorSeat;deletion_authority(wasdeletion_authority_refusal) returns the signer's seat, so the new check adds no read. A stored addition without$createdAt(the schema requires it) reads as added last: it counts for no dated document, and no read of the team fails.Closing approvals (
still_counted_approvers, wasstill_seated_approvers): when an approval reads the team, approvals of members the leader took off and added again after the document are dropped and refunded, like those of members who left. Their new approval is refused.Before: member X (added before the post) approves; the leader removes X and adds X again; X's approval still counts.
After: the next approval that reads the team drops X's approval, and X approving again gets 41212.
Unchanged on purpose:
approvals_neededstill counts the team's seats, not who predates the document, so the leader can not lower the bar by removing members.Docs: book (
deletion.md,moderator-abilities.md,contract-keywords.md,contract-moderation.md,error-codes.md), v14 note item 67 (in place), rs-sdk / wasm-sdk / evo-sdk doc comments of the proposal and approval calls.In-place changes to shipped generations
contract_user_moderationstate validation v0 is selected by every protocol version, so it is edited in place, and so are the unversioned helpers it and other moderation paths share. Every edited path is unreachable or output-identical before protocol version 14:deletion_authority(used byDeleteDocumentat all versions): for the moderators a declaration names,Moderators::seat_ofisContractModerationConfig::may_moderate, as before. The seated-team branch exists only on elected contracts (protocol version 14), and makes the same single billed query asseatsdid.Moderators::may_moderate(ban, suspend, warn, field-write and deletion validation) now returnsseat_of(..).is_some(): the Declared branch is the samemay_moderatecall; the Seated branch (protocol version 14 only) makes the same single billed query as before.SeatedModerationCharter::seatsdelegates toseat_of(same query, same answer), andfetch_active_members(pot settle, fee claim) returns the keys offetch_active_seats: the same two queries in the same order, the same member set. Both exist only for seated teams (protocol version 14).transform_settled_deletion_proposal_v0,transform_team_action_approval_v0,still_counted_approvers,late_addition_refusal: reached only fromDeleteSettledDocument/ApproveTeamAction, protocol version 14 only.moderatorAbilities, accepted by meta-schema v3 only (protocol version 14).How Has This Been Tested?
tests/seated_team/settled.rs, new: a member added after a story (or in its block) refused as proposer and approver, in a block and incheck_tx, nothing stored, while one added before counts and the elected member counts on a story older than the seat; a member taken off and added again after the story dropped and refunded by a per-seat team read, refused afterwards, another member completing the rule; the same drop through a whole-team read (fetch_active_seats), together with a member who left. A re-added late member's second approval is refused 41212, not 41208. Existing tests now make additions after the award's block and write documents after the additions they rely on; the "member who left comes back" test runs on a newlegendtype that opts out. Making the whole-team read drop every added member fails the main settled-story test, which closes through that read.admits_additionalways true) fails the three new drive-abci tests.settled_deletion.rs:admits_addition(before, same time, after, option off).moderator_abilities_tests.rs: default (on above one approval, off at one) and explicit values,$createdAtrequired while on at registration and not on the stored path, not required when off, malformed value refused.validate_update/v1: turning the option off is refused, and the messages name it.state_error.rs: discriminant 170 frozen.$createdAt: rs-driveteam_action_tests.rs,structure/tests.rs, drive-abcicontract_moderation_queries.cargo nextest run -p dpp --lib --all-features(5297 passed),cargo test -p drive --lib -- team_action structure::tests moderation(81),cargo test -p drive-abci --lib -- moderation contract_fee_claim team_action data_contract(456),cargo clippy -p dpp -p drive -p drive-abci --all-targets --all-featuresclean,cargo check -p wasm-dpp --target wasm32-unknown-unknown. Not run: rs-sdk, wasm-sdk, evo-sdk and JS specs (doc comments only there).Breaking Changes
Consensus, protocol version 14 (unreleased):
deleteSettledneeds more than one approval without$createdAtinrequiredis refused unless it setsapproversPredateDocument: false. A stored contract is read back as before.SettledDeletionRulegains a public field: code building it literally must setapprovers_predate_document. feat(sdk): approvals a settled deletion needs of a team, empty seats documented #5245 builds it in rs-sdk and wasm-sdk; whichever merges second adds the field.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 ·
f3dcc89/skip-botsproceeds without the ones not yet reported/self-reviewedjs-wasm-sdk(packages/js-evo-sdk/src/contracts/facade.ts,packages/wasm-sdk/src/state_transitions/contract.rs) — shumkovdpp— you own itrs-drive-abci— you own itrs-drive— you own itrust-sdk(packages/rs-sdk/src/platform/transition/contract_user_moderation.rs) — lklimek or shumkovWhen every box is checked the
PR Hygienecheck passes and this can merge.Summary by CodeRabbit