test(dpp,drive-abci): pin keepHistory + canBeDeleted contradiction#3928
Draft
thephez wants to merge 2 commits into
Draft
test(dpp,drive-abci): pin keepHistory + canBeDeleted contradiction#3928thephez wants to merge 2 commits into
thephez wants to merge 2 commits into
Conversation
Add failing tests reproducing a contradictory document-type config that nothing currently catches up front: documentsKeepHistory: true together with canBeDeleted: true. rs-drive unconditionally refuses deleting a keep-history document, so canBeDeleted: true advertises a capability the storage layer always rejects. The dpp tests assert contract creation should reject the combination (and that keepHistory + canBeDeleted: false stays valid), pinning the missing guard to the try_from_schema flag-parsing region. The drive-abci test asserts the delete transition should be refused up front as an invalid (paid) transition. It fails because the delete-transition structure validator only checks documents_can_be_deleted(), so the doomed delete passes validation and the Drive-layer InvalidDeletionOfDocumentThatKeepsHistory surfaces as an ExecutionResult::InternalError (neither valid nor invalid) — the root of the "delete hangs on the network" symptom. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…pHistory + canBeDeleted Extend the keepHistory + canBeDeleted reproduction beyond explicit document-schema flags. Add contract-default cases to the dpp try_from_schema tests: both flags inherited from contract defaults, and each one-sided mix (one explicit, one inherited). canBeDeleted defaults to true, so a type that only sets documentsKeepHistory: true still hits the contradiction — the guard must inspect the resolved booleans, not the raw schema keys. Refactor the existing tests onto basic_message_schema / parse_with_config helpers. Add a third layer: DocumentFactoryV0 and SpecializedDocumentFactoryV0 must reject a delete transition for a keep-history document type locally, before it can be signed and broadcast. Parametrize the drive-abci delete test and add an inherited-defaults variant with a matching fixture, so the structure validator path is covered for resolved (not just explicit) flags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Issue being fixed or feature implemented
Reproduces #3927. A document type can set both
documentsKeepHistory: trueandcanBeDeleted: true, but rs-drive unconditionally refuses to delete a document whose type keeps history — socanBeDeleted: trueadvertises a capability the storage layer always rejects. Nothing catches this contradiction up front: contract creation accepts it, the delete-transition structure validator only checksdocuments_can_be_deleted()(notdocuments_keep_history()), and the delete fails deep in Drive as anExecutionResult::InternalError(neither valid nor invalid) instead of a clean rejection.This PR adds failing tests that pin the bug and mark exactly where each guard belongs. It does not include the fix — it's a red-test reproduction to anchor the follow-up.
What was done?
Added failing tests across the three layers where the contradiction should be caught but currently isn't, plus passing companions so a future guard can't be over-broad.
Contract creation — rs-dpp
try_from_schema/v2/mod.rs:doctype_keep_history_with_can_be_deleted_rejected: explicitdocumentsKeepHistory: true+canBeDeleted: trueon the document schema must be rejected, naming both flags (fails today).canBeDeleteddefaults totrue, a type that only setsdocumentsKeepHistory: truestill hits the contradiction. These pin that the guard must inspect the resolved booleans, not just the raw schema keys:doctype_keep_history_with_can_be_deleted_inherited_from_defaults_rejected(both from contract defaults),doctype_keep_history_default_with_explicit_can_be_deleted_rejectedanddoctype_explicit_keep_history_with_can_be_deleted_default_rejected(each one-sided mix). All fail today.doctype_keep_history_with_can_be_deleted_false_accepted:keepHistory + canBeDeleted: falsestays valid (passes — over-broad guard rejected).Local transition build — rs-dpp document factories:
DocumentFactoryV0(delete_keep_history_document_is_rejected_locally) andSpecializedDocumentFactoryV0(create_state_transition_delete_with_keep_history_document_returns_error) must reject a delete transition for a keep-history document type locally, before it can be signed and broadcast (fail today). This is the layer the SDK-side fix would surface at.Node delete validation — rs-drive-abci
batch/tests/document/deletion.rs:test_document_delete_on_document_type_that_keeps_history_is_rejectedasserts the delete is refused up front as an invalid (paid) transition, mirroring the existingcan_not_be_deletedtest (fails today; the doomed delete is currently classified as an internal error)....inherits_keep_history_and_can_be_deleted...) runs the same path against a contract whose flags come from config defaults — the structure validator only sees the resolvedDocumentTypeRef, so the guard must cover this too.tests/supporting_files/contract/note/note-contract-keep-history-and-can-be-deleted.json(explicit flags, mirroring live testnet contract5CBPiadGmx3Zsjc26g5onopcx7pdxHPbrRAUD2T2yAbCnotetype) and...-by-default.json(flags from contract defaults).Suggested fix (separate PR): reject the resolved flag combination in
DocumentType::try_from_schema, reject the delete in the DPP document factories / SDK before broadcast, and add adocuments_keep_history()check next to thedocuments_can_be_deleted()check in the delete-transition structure validator.How Has This Been Tested?
Run at
PlatformVersion::latest():cargo test -p dpp --lib keep_history— the six rejection tests above fail as intended; the passing companions and unrelated keep-history tests stay green (no regressions).cargo test -p drive-abci --lib keep_history— the keep-history delete tests fail on theinvalid_paid_count == 1assertion, confirming the delete is not rejected at validation today.Also reproduced end-to-end against testnet via
@dashevo/evo-sdk(documents.createthendocuments.deleteagainst the contract above; the delete returnsInvalidDeletionOfDocumentThatKeepsHistory).Note: this PR is intentionally red — the new tests fail until the guards land (tracked in #3927).
Breaking Changes
None — tests and test fixtures only.
Checklist:
For repository code-owners and collaborators only