fix(platform): reject keep-history document deletes cleanly#3930
fix(platform): reject keep-history document deletes cleanly#3930thepastaclaw wants to merge 10 commits into
Conversation
…schema documentsKeepHistory: true together with canBeDeleted: true is self-contradictory — rs-drive's force-delete path unconditionally refuses to delete documents whose type keeps history (InvalidDeletionOfDocumentThatKeepsHistory). Catch the combination at contract creation time in the V2 document-type schema parser, mirroring the existing cross-flag rule for ContestedUniqueIndexOnMutableDocumentTypeError, so SDK users get a clean InvalidContractStructure error instead of a runtime delete failure. Gated by full_validation so already-deployed contradictory contracts continue to load through the V2 parser at v12+ (the rs-drive-abci delete-transition guard surfaces them as clean invalid-paid transitions in a follow-up commit). Refs dashpay#3927 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without this guard, a delete state transition against a document type whose schema sets documentsKeepHistory: true passes the structure validator (which only checks documents_can_be_deleted()) and reaches rs-drive's force-delete path, where it triggers InvalidDeletionOfDocumentThatKeepsHistory. The processor reclassifies that drive-layer error as ExecutionResult::InternalError — the transition is neither valid nor invalid-paid, leaving the SDK with no clean accept/reject signal. Add a documents_keep_history() check beside the existing documents_can_be_deleted() check in the delete-transition advanced structure validator so the contradiction surfaces as a normal invalid (paid) consensus error. This fixes the classification even for already-deployed contradictory contracts (e.g. testnet 5CBPiadGmx3Zsjc26g5onopcx7pdxHPbrRAUD2T2yAbC), which the DPP cross-flag rule from the previous commit cannot retroactively fix. The new note-contract-keep-history-and-can-be-deleted.json fixture is loaded with full_validation=false so it bypasses the DPP guard and exercises this layer directly. Refs dashpay#3927 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit added the documents_keep_history() check directly to advanced_structure_v0, which is the dispatched validator for ALL historical protocol versions (1-11) as well as v12. Mutating v0 would alter the consensus result of any historical block containing a delete against a keep-history doctype — pre-fix those returned ExecutionResult::InternalError (no fees, no state change); post-fix they would be classified as invalid-paid (fees deducted, state delta differs), breaking bit-for-bit chain replay at PROTOCOL_VERSION_11 and earlier. Move the check into a new advanced_structure_v1 module, restore v0 to its original behavior, extend the trait dispatcher to handle both versions, and bump document_delete_transition_structure_validation from 0 to 1 in DRIVE_ABCI_VALIDATION_VERSIONS_V8 (used by PROTOCOL_VERSION_12, the v3.1 hard fork). V1-V7 still dispatch v0 so pre-v12 chain history stays reproducible. Refs dashpay#3927 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wrap the v2 DocumentType keep-history + canBeDeleted contradiction through `consensus_or_protocol_data_contract_error` so that with the `validation` feature it returns `ProtocolError::ConsensusError` instead of bare `ProtocolError::DataContractError`. drive-abci's `transform_into_action_v0` only converts the consensus variant into a clean invalid (paid) transition with a bump action — the DataContractError variant propagated as an internal execution error in validator mode, defeating the parse-time guard's whole purpose for contract create/update. Pin the variant in the existing regression test so the classification can't silently regress. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughEnforces that ChangesFix documentsKeepHistory + canBeDeleted contradiction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 663b50a) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (1)
986-1019: 💤 Low valueConsider adding a test for
full_validation: falseto verify the gating.The validation logic is correctly gated by
full_validation, but there's no test verifying that contradictory contracts are accepted whenfull_validation: false(the scenario for re-parsing already-deployed contracts). A test would confirm the gate works as documented in lines 317-321.📝 Suggested test addition
/// `full_validation: false` must allow the contradictory combination so /// already-deployed contracts (e.g. testnet `5CBPiadGmx3Zsjc26g5onopcx7pdxHPbrRAUD2T2yAbC`) /// continue to load when re-parsed at v12+. #[test] fn doctype_keep_history_with_can_be_deleted_accepted_without_full_validation() { let schema = platform_value!({ "type": "object", "properties": { "label": { "type": "string", "maxLength": 50, "position": 0, }, }, "additionalProperties": false, "documentsKeepHistory": true, "canBeDeleted": true, }); let platform_version = PlatformVersion::latest(); let config = DataContractConfig::default_for_version(platform_version) .expect("default config available on latest platform version"); let result = DocumentTypeV2::try_from_schema( Identifier::new([1; 32]), 1, config.version(), "test_doc", schema, None, &BTreeMap::new(), &config, false, // full_validation = false &mut vec![], platform_version, ); assert!( result.is_ok(), "contradictory flags must be accepted when full_validation=false \ to allow already-deployed contracts to load; got {:?}", result.unwrap_err() ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs` around lines 986 - 1019, Add a new test function after the existing doctype_keep_history_with_can_be_deleted_rejected test to verify that when full_validation is set to false, the contradictory combination of documentsKeepHistory: true and canBeDeleted: true is accepted. The test should use the same schema structure as the existing rejection test, call DocumentTypeV2::try_from_schema with full_validation parameter set to false, and assert that the result is Ok instead of Err. This confirms the validation gating mechanism works correctly for re-parsing already-deployed contracts as documented.packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs (1)
381-390: ⚡ Quick winPin this regression to protocol version 12 to keep the gate test deterministic.
Using
current_platform_version()makes this test float withlatest, so it can stop validating the exact v12 gate behavior over time. Prefer explicitly setting protocol version12here (and optionally add a paired v11 assertion for legacy classification).Suggested change
- let mut platform = TestPlatformBuilder::new() - .build_with_mock_rpc() - .set_initial_state_structure(); + let protocol_version = 12; + let mut platform = TestPlatformBuilder::new() + .with_initial_protocol_version(protocol_version) + .build_with_mock_rpc() + .set_initial_state_structure(); let contract_path = "tests/supporting_files/contract/note/note-contract-keep-history-and-can-be-deleted.json"; - let platform_state = platform.state.load(); - let platform_version = platform_state - .current_platform_version() - .expect("expected to get current platform version"); + let platform_state = platform.state.load(); + let platform_version = PlatformVersion::get(protocol_version) + .expect("expected platform version for protocol version 12");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs` around lines 381 - 390, The test is currently using current_platform_version() to retrieve the platform version, which will always use the latest protocol version and cause the test to float over time, making it unable to reliably validate v12 gate behavior. Replace the call to current_platform_version() with an explicit protocol version 12 assignment, ensuring the test pins to protocol version 12 to maintain deterministic gate validation. Optionally, add a paired assertion for legacy v11 classification to provide additional coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 986-1019: Add a new test function after the existing
doctype_keep_history_with_can_be_deleted_rejected test to verify that when
full_validation is set to false, the contradictory combination of
documentsKeepHistory: true and canBeDeleted: true is accepted. The test should
use the same schema structure as the existing rejection test, call
DocumentTypeV2::try_from_schema with full_validation parameter set to false, and
assert that the result is Ok instead of Err. This confirms the validation gating
mechanism works correctly for re-parsing already-deployed contracts as
documented.
In
`@packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs`:
- Around line 381-390: The test is currently using current_platform_version() to
retrieve the platform version, which will always use the latest protocol version
and cause the test to float over time, making it unable to reliably validate v12
gate behavior. Replace the call to current_platform_version() with an explicit
protocol version 12 assignment, ensuring the test pins to protocol version 12 to
maintain deterministic gate validation. Optionally, add a paired assertion for
legacy v11 classification to provide additional coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25cb4c10-52f5-41b2-a9ed-9d76c405cfe4
📒 Files selected for processing (6)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_delete_transition_action/advanced_structure_v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_delete_transition_action/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rspackages/rs-drive-abci/tests/supporting_files/contract/note/note-contract-keep-history-and-can-be-deleted.jsonpackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_validation_versions/v8.rs
…tests - Add DPP test asserting documentsKeepHistory + canBeDeleted is accepted when try_from_schema runs with full_validation=false (the restore / migration / cache-warmup path), pinning the gating behavior that lets already-deployed contradictory contracts continue to load at v12+. - Pin the drive-abci keep-history delete regression test to protocol version 12 via with_initial_protocol_version + PlatformVersion::get instead of floating on current_platform_version(), so the assertion stays bit-for-bit reproducible as new protocol versions ship. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Addressed both CodeRabbit nitpicks in 078c233:
Focused validation after the commit:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted fix is correct: DPP parser cross-flag guard is gated by full_validation and uses consensus_or_protocol_data_contract_error to surface as ConsensusError in validator builds; drive-abci v1 validator is wired only at PROTOCOL_VERSION_12 so v11 replay is preserved; tests cover the rejected/accepted combinations. Three minor follow-ups: a misleading docstring about the canBeDeleted config default, a missing regression test for the omitted-default boundary, and a missing pre-v12 paired test for the new delete guard. No blockers.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
🤖 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/v2/mod.rs`:
- [SUGGESTION] packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs:329-339: Add regression test for omitted `canBeDeleted` (default=true) with `documentsKeepHistory: true`
The guard fires on resolved `DocumentTypeV1` fields, and `documents_can_be_deleted_contract_default()` is `true`, so a schema with `documentsKeepHistory: true` and `canBeDeleted` omitted is now rejected — the most likely real-world misuse. Every other branch is pinned (true+true rejected, true+false accepted, false+true accepted), but the omitted-defaults case is not. A future refactor that inspects raw schema keys instead of resolved values could pass the current tests while silently reopening the contradiction. Add a `doctype_keep_history_with_can_be_deleted_omitted_rejected` test that omits `canBeDeleted` to pin the resolved-default behavior.
In `packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs:379-390: Pin pre-v12 keep-history delete classification with a paired test
The new regression runs only against the current platform version, but this PR's correctness depends on `document_delete_transition_structure_validation` activating at v12 while v11 preserves the v0 path for chain replay. Adjacent version-gated deletion behavior in this file already has paired v11/latest helpers; adding the same v11 case for the contradictory keep-history fixture would catch an accidental change to the v11 version table or to the v0 validator that would otherwise alter consensus replay silently.
| if full_validation && v1.documents_keep_history && v1.documents_can_be_deleted { | ||
| return Err(consensus_or_protocol_data_contract_error( | ||
| DataContractError::InvalidContractStructure(format!( | ||
| "document type \"{}\" sets both `documentsKeepHistory: true` and \ | ||
| `canBeDeleted: true`, but the storage layer unconditionally refuses to \ | ||
| delete a document whose type keeps history. Set one of the two flags to \ | ||
| false (or omit it).", | ||
| name, | ||
| )), | ||
| )); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add regression test for omitted canBeDeleted (default=true) with documentsKeepHistory: true
The guard fires on resolved DocumentTypeV1 fields, and documents_can_be_deleted_contract_default() is true, so a schema with documentsKeepHistory: true and canBeDeleted omitted is now rejected — the most likely real-world misuse. Every other branch is pinned (true+true rejected, true+false accepted, false+true accepted), but the omitted-defaults case is not. A future refactor that inspects raw schema keys instead of resolved values could pass the current tests while silently reopening the contradiction. Add a doctype_keep_history_with_can_be_deleted_omitted_rejected test that omits canBeDeleted to pin the resolved-default behavior.
source: ['codex', 'claude']
| #[tokio::test] | ||
| async fn test_document_delete_on_document_type_that_keeps_history_is_rejected() { | ||
| let mut platform = TestPlatformBuilder::new() | ||
| .build_with_mock_rpc() | ||
| .set_initial_state_structure(); | ||
|
|
||
| let contract_path = "tests/supporting_files/contract/note/note-contract-keep-history-and-can-be-deleted.json"; | ||
|
|
||
| let platform_state = platform.state.load(); | ||
| let platform_version = platform_state | ||
| .current_platform_version() | ||
| .expect("expected to get current platform version"); |
There was a problem hiding this comment.
🟡 Suggestion: Pin pre-v12 keep-history delete classification with a paired test
The new regression runs only against the current platform version, but this PR's correctness depends on document_delete_transition_structure_validation activating at v12 while v11 preserves the v0 path for chain replay. Adjacent version-gated deletion behavior in this file already has paired v11/latest helpers; adding the same v11 case for the contradictory keep-history fixture would catch an accidental change to the v11 version table or to the v0 validator that would otherwise alter consensus replay silently.
source: ['codex']
There was a problem hiding this comment.
Resolved in 078c233 — Pin pre-v12 keep-history delete classification with a paired test no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
Correction: this was auto-resolved incorrectly by the review dedupe/reconciliation step. The latest push does pin the regression test to protocol version 12, but the paired pre-v12/v11 classification coverage is still missing, so this finding remains valid at 078c233. Reopening/keeping this thread active.
|
CI note: Failure:
Why unrelated:
Local focused checks for this PR still pass:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
`SdkBuilder::new_mock()` builds on Mainnet, whose floor (PV_11) dominates `DEFAULT_INITIAL_PROTOCOL_VERSION` (10). Assert the actual `max(DEFAULT_INITIAL_PROTOCOL_VERSION, network floor)` the SDK seeds, not the bare constant.
Resolves a single conflict in packages/rs-sdk/tests/fetch/document_query_v0_v1.rs: upstream PR dashpay#3900 ("per-network protocol-version floor + version_pinned unification") removed the DEFAULT_INITIAL_PROTOCOL_VERSION constant and replaced it with the per-network min_protocol_version() function. The network-floor-raised test added in 3ed90bc referenced the removed constant; resolved by adopting the upstream form, which asserts the same seeded value (PROTOCOL_VERSION_11 on mainnet) without the deleted constant. PR intent (unpinned SDK on mainnet seeds to the network floor) is preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3930 +/- ##
=============================================
- Coverage 87.18% 52.54% -34.64%
=============================================
Files 2632 11 -2621
Lines 327563 1707 -325856
=============================================
- Hits 285592 897 -284695
+ Misses 41971 810 -41161
🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push (3ed90bc) only adjusts an unrelated rs-sdk seed test to assert max(DEFAULT_INITIAL_PROTOCOL_VERSION, network floor); that delta is correct. The cumulative keep-history-delete fix is well-targeted and consensus-safe: DPP parse-time rejection under full_validation, version-gated drive-abci advanced_structure_v1 at PROTOCOL_VERSION_12, and ConsensusError plumbing that yields clean invalid-paid classification. Three prior review items remain unaddressed (omitted-default test gap, paired v11 replay test, docstring misstatement) and one new related nit surfaced about misleading error-message wording around the same omitted default.
💬 1 nitpick(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
| "document type \"{}\" sets both `documentsKeepHistory: true` and \ | ||
| `canBeDeleted: true`, but the storage layer unconditionally refuses to \ | ||
| delete a document whose type keeps history. Set one of the two flags to \ | ||
| false (or omit it).", |
There was a problem hiding this comment.
💬 Nitpick: Error message suggests an unsafe 'omit it' fix
The error tells the author to 'Set one of the two flags to false (or omit it).' Omitting does not necessarily fix the contradiction: with the default contract config, canBeDeleted resolves to true when omitted (DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED in config/fields.rs:6), so documentsKeepHistory: true plus omitted canBeDeleted still hits this error. The message should direct the user to ensure the resolved value of canBeDeleted is false, either explicitly on the document type or via the contract default — otherwise authors will follow the literal instruction and be surprised that omission doesn't help.
source: ['codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push (230eeb1) is a routine merge of upstream/v3.1-dev pulling unrelated rs-sdk/swift-sdk changes; none of the keep-history/delete fix files are modified by the delta. The cumulative fix is correct and consensus-safe: DPP rejects the resolved contradictory flags under full_validation, drive-abci routes v12 delete attempts through advanced_structure_v1 via DRIVE_ABCI_VALIDATION_VERSIONS_V8, and the new error helper produces a clean invalid-paid classification. No new findings; all four prior findings still apply at this SHA and are carried forward.
Reviewed commit: 230eeb1
🟡 2 suggestion(s) | 💬 2 nitpick(s)
Carried-forward prior findings
nitpick: Docstring misstates `canBeDeleted` default and sibling test coverage
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (line 1069)
This comment claims doctype_keep_history_without_summable_accepted leaves canBeDeleted at its config default (false). Both halves are wrong against the current tree: (1) DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED is true (packages/rs-dpp/src/data_contract/config/fields.rs:6), so the config default is true, not false; (2) the sibling test explicitly sets "canBeDeleted": false — it does not exercise the omitted-default path at all. As written, this misleads future maintainers into thinking the omitted-key case is already covered and benign, when in fact it resolves to documents_can_be_deleted = true and is rejected at v12+.
Suggested fix:
/// Guard against an over-broad fix: `documentsKeepHistory: true` +
/// `canBeDeleted: false` is consistent (the doctype is append-only)
/// and must continue to parse cleanly. NOTE: the omitted-key case is
/// NOT covered here — the contract default for `canBeDeleted` is
/// `true` (DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED), so a schema
/// that omits `canBeDeleted` with `documentsKeepHistory: true`
/// resolves to the contradictory state and is rejected. Omitted-key
/// coverage belongs in a separate regression test.
suggestion: Add regression test for omitted `canBeDeleted` (default=true) with `documentsKeepHistory: true`
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (line 329)
The guard fires on the resolved DocumentTypeV1 booleans, and documents_can_be_deleted resolves to true when the schema omits canBeDeleted (DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED = true). So documentsKeepHistory: true with canBeDeleted omitted is rejected at v12+ — the most likely real-world authoring mistake. Current tests pin true+true rejection, true+false acceptance, and false+true acceptance, but not the omitted-default rejection. A future refactor that inspects raw schema keys instead of resolved values could pass the current tests while silently reopening the contradiction. Add a doctype_keep_history_with_can_be_deleted_omitted_rejected test that omits the key and asserts the resolved default still rejects under full_validation: true.
suggestion: Pin pre-v12 keep-history delete classification with a paired v11 test
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs (line 379)
test_document_delete_on_document_type_that_keeps_history_is_rejected is correctly pinned to PROTOCOL_VERSION = 12, but the PR's correctness story explicitly depends on document_delete_transition_structure_validation activating at v12 while v11 preserves the v0 path (and its pre-fix InternalError/replay behavior). Other version-gated deletion tests in this file already use paired v11/latest helpers; adding the same v11 case for the contradictory keep-history fixture would catch any accidental change to the v11 version table or to advanced_structure_v0 that would otherwise alter consensus replay silently. Without it, only the post-fix branch is exercised by CI.
nitpick: Error message suggests an unsafe 'omit it' fix
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (line 332)
The error tells the author to "Set one of the two flags to false (or omit it)." Omitting does not necessarily fix the contradiction: with the default contract config, canBeDeleted resolves to true when omitted (DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED = true), so documentsKeepHistory: true plus omitted canBeDeleted still hits this error. The message should direct the user to ensure the resolved value of canBeDeleted is false (explicitly on the document type or via the contract default), otherwise authors will follow the literal instruction and be surprised that omission doesn't help.
Suggested fix:
"document type \"{}\" sets both `documentsKeepHistory: true` and \
resolves `canBeDeleted` to true, but the storage layer unconditionally \
refuses to delete a document whose type keeps history. Ensure \
`canBeDeleted` resolves to false (either explicitly on the document \
type or through the contract default), or set `documentsKeepHistory: false`.",
No new findings in the latest delta; these are the still-valid prior findings revalidated at this head. Existing inline threads were already open, so this body-only exact-SHA review records coverage without duplicating inline comments.
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest push (a90cf62) is a one-line test-fixture adjustment in rs-drive that adds canBeDeleted: false alongside documentsKeepHistory: true so an existing ChangingDocumentTypeKeepsHistory regression test reaches its intended assertion under the V2 parser's new cross-flag rule — mechanical, consensus-safe, and contains no new in-scope issues. Cumulative review confirms the version-gated v12 drive-abci delete-transition guard, DPP full_validation rejection, and clean ConsensusError classification remain correct, and all four prior carry-forward findings (documentation, error wording, and test-coverage gaps around the canBeDeleted default and pre-v12 replay) still apply at this SHA.
Reviewed commit: a90cf62
🟡 2 suggestion(s) | 💬 2 nitpick(s)
Prior findings reconciliation
prior-1: STILL VALID —documents_can_be_deleted_contract_defaultstill defaults to true via DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED (config/fields.rs:6); the new guard at v2/mod.rs:329 fires on resolved booleans, and there is still no test that omits canBeDeleted under documentsKeepHistory:true.prior-2: STILL VALID — deletion.rs test is still pinned to v12 only; no paired v11 case for the contradictory keep-history fixture exists.prior-3: STILL VALID — Docstring at v2/mod.rs:1069-1074 still claims the sibling test leaves canBeDeleted at its config default (false); the default is true and the sibling test was updated this PR to explicitly set canBeDeleted:false.prior-4: STILL VALID — Error message at v2/mod.rs:332-335 still advises 'Set one of the two flags to false (or omit it)', but omitting canBeDeleted resolves to true via the contract default and still trips the same rejection.
Verified findings
nitpick: Docstring misstates `canBeDeleted` default and sibling-test coverage
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (line 1069)
The docstring asserts that doctype_keep_history_without_summable_accepted covers the omitted-canBeDeleted case by leaving the key at its config default (false). Both halves are wrong against the tree at this SHA: (1) DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED is true (packages/rs-dpp/src/data_contract/config/fields.rs:6), so the resolved default is true, not false; (2) the sibling test was edited in this PR to explicitly set canBeDeleted: false and no longer exercises the omitted-key path at all. As written, the comment misleads maintainers into assuming the omitted-default case is already covered and benign, when in fact omitting canBeDeleted under documentsKeepHistory: true resolves to the contradictory state and is rejected at v12+.
Suggested fix:
/// Guard against an over-broad fix: `documentsKeepHistory: true` +
/// `canBeDeleted: false` is consistent (the doctype is append-only)
/// and must continue to parse cleanly. NOTE: the omitted-key case is
/// NOT covered here — the contract default for `canBeDeleted` is
/// `true` (DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED), so a schema
/// that omits `canBeDeleted` with `documentsKeepHistory: true`
/// resolves to the contradictory state and is rejected. Omitted-key
/// coverage belongs in a separate regression test.
nitpick: Error message suggests an unsafe 'omit it' fix
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (line 332)
The validation error tells the author to "Set one of the two flags to false (or omit it)." Omitting does not necessarily fix the contradiction: with the default contract config, canBeDeleted resolves to true when omitted (DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED = true, packages/rs-dpp/src/data_contract/config/fields.rs:6), so documentsKeepHistory: true plus omitted canBeDeleted still hits this exact error. SDK users will follow the literal instruction and be surprised when omission doesn't help. Redirect the user to the resolved value instead.
Suggested fix:
"document type \"{}\" sets both `documentsKeepHistory: true` and \
resolves `canBeDeleted` to true, but the storage layer unconditionally \
refuses to delete a document whose type keeps history. Ensure \
`canBeDeleted` resolves to false (either explicitly on the document \
type or through the contract default), or set `documentsKeepHistory: false`.",
suggestion: Add regression test for omitted `canBeDeleted` (default=true) with `documentsKeepHistory: true`
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (line 329)
The new guard fires on the resolved DocumentTypeV1 booleans, and documents_can_be_deleted resolves to true when the schema omits canBeDeleted (DEFAULT_CONTRACT_DOCUMENTS_CAN_BE_DELETED = true). So documentsKeepHistory: true with canBeDeleted omitted is the most likely real-world authoring path and is rejected at v12+, but the current tests only pin the explicit true+true rejection, explicit true+false acceptance, and full_validation=false bypass. A future refactor that inspects raw schema keys instead of resolved values could pass the existing suite while silently reopening the contradiction. Add a doctype_keep_history_with_can_be_deleted_omitted_rejected test that omits the key and asserts the resolved default still rejects under full_validation: true.
suggestion: Pin pre-v12 keep-history delete classification with a paired v11 test
packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/deletion.rs (line 379)
test_document_delete_on_document_type_that_keeps_history_is_rejected is correctly pinned to PROTOCOL_VERSION = 12, but the PR's correctness story explicitly depends on document_delete_transition_structure_validation activating at v12 while v11 preserves the v0 path (and its pre-fix InternalError/replay behavior). Nearby version-gated deletion tests already use paired v11/latest helpers (e.g. run_document_delete_on_document_type_that_is_mutable_and_can_be_deleted_at_protocol_version, run_document_delete_that_does_not_yet_exist_at_protocol_version). Adding the same paired v11 case for the contradictory keep-history fixture would catch any accidental change to the v11 version table or to advanced_structure_v0 that would otherwise alter consensus replay silently. Without it, only the post-fix v12 branch is exercised in CI.
Note: inline posting deduped these carried-forward findings against existing open threads, so this exact-SHA review is posted body-only to preserve the review record without duplicating inline comments.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative re-review of PR #3930 at 663b50a. The latest delta (a90cf62..663b50a) is a one-line test fixture update in rs-drive that adds canBeDeleted: false to the keep_history_summable_e2e schema so it satisfies the new DPP cross-flag guard — no new issues introduced. All four prior findings reconcile as STILL VALID against current HEAD: the docstring at v2/mod.rs:1069-1074 still misstates both the canBeDeleted default (it is true) and the sibling-test coverage (the sibling now explicitly sets canBeDeleted: false), the error string still suggests an unsafe 'omit it' fix, no omitted-key regression test was added, and the keep-history delete test is still only pinned at v12 with no paired v11 case. All findings carried forward — none new in the delta.
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI follow-up on the current red checks at
Leaving the branch unchanged: this looks like CI/runner/coverage plumbing rather than a regression in the keep-history delete validation diff. |
1 participant
fix(platform): reject keep-history document deletes cleanly
Issue being fixed or feature implemented
Fixes #3927.
Document types could set both
documentsKeepHistory: trueandcanBeDeleted: true. That combination advertises deletability even thoughDrive refuses to delete keep-history documents, so delete attempts surfaced as
InternalErrorinstead of a clear consensus rejection.What was done?
during DPP schema parsing.
while preserving protocol/data-contract error behavior for non-validation
builds.
already-deployed contradictory contracts fail as invalid-paid transitions
instead of internal errors.
replay behavior is preserved.
keep-history/deletable fixture for the existing-contract path.
How Has This Been Tested?
Local worktree on macOS arm64, branch
fix/document-history-delete-error.cargo test -p dpp --features all_features data_contract::document_type::class_methods::try_from_schema::v2Broader DPP
try_from_schemasweep across v0/v1/v2cargo test -p dpp --features all_featurescargo test -p drive-abci --features mocks test_document_delete_on_document_type_that_keeps_history_is_rejectedcargo test -p drive-abci --features mocks data_contract_create::state::v0::testscargo test -p drive-abci --features mocksfocused batchstate-transition/deletion coverage
DPP no-default-features parser regression:
cargo test -p dpp \ doctype_keep_history_with_can_be_deleted_rejected \ --no-default-featuresDPP all-features parser regression:
cargo test -p dpp --features all_features \ doctype_keep_history_with_can_be_deleted_rejectedPre-commit hook:
cargo fmt --all -- --checkcargo checkworkspacePre-PR code-review gate was run after fixes and returned
ship.Breaking Changes
None.
Checklist
corresponding section if my code contains any
For repository code-owners and collaborators only
Summary by CodeRabbit
canBeDeleted: false.