feat(sdk): indexOnly delete-by-values through the SDKs; book chapter - #4495
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis change documents index-only document types and updates document creation and deletion flows. Creation returns the confirmed document, including consensus-populated fields. Deletion preserves full document values for index-only types and validates deletion targets before signing. ChangesIndex-only document support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DocumentsFacade
participant WasmSdk
participant DocumentDeleteTransitionBuilder
participant Platform
Client->>DocumentsFacade: create(options)
DocumentsFacade->>WasmSdk: documentCreate(options)
WasmSdk->>Platform: submit document create transition
Platform-->>WasmSdk: confirmed Document with system fields
WasmSdk-->>DocumentsFacade: DocumentWasm
DocumentsFacade-->>Client: confirmed Document
Client->>WasmSdk: documentDelete(Document)
WasmSdk->>DocumentDeleteTransitionBuilder: from_document(document)
DocumentDeleteTransitionBuilder->>Platform: resolve and sign deletion
Suggested reviewers: ✨ 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 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-08-27T15:56:27.431Z |
|
🕓 Ready for review — next in queue (commit 77c526f) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The Rust/WASM routing now preserves full document values, but two in-scope blockers remain: id-only validation advances the cached nonce before returning, and WASM discards the confirmed create result needed to delete valid index-only documents that require $createdAt. The new routing also permits inconsistent builder state and lacks direct regression coverage, so changes are requested.
Source: Codex general, Rust-quality, and FFI reviewer lanes (exact backend model identifiers were not present in the supplied evidence); Claude Agent SDK final verifier (exact backend model identifier was not exposed); openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 2 suggestion(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-sdk/src/platform/documents/transitions/delete.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/documents/transitions/delete.rs:175-201: Reject id-only deletes before reserving a nonce
`get_identity_contract_nonce(..., true, ...)` increments the SDK's cached nonce before the newly added id-only/index-only validation returns an error. No transition is broadcast on this path, so each invalid call consumes only a local nonce. The cache deliberately preserves `max(cached, platform)` during refresh; after 24 such calls, the next valid transition is assigned a nonce beyond the protocol's missing-revision window and is rejected as `NonceTooFarInFuture`, while subsequent refreshes continue preserving and incrementing the poisoned value. Resolve the document type and reject the unsupported input before reserving the nonce.
- [SUGGESTION] packages/rs-sdk/src/platform/documents/transitions/delete.rs:21-30: Avoid independently mutable delete-target representations
The new public `document: Option<Document>` exists alongside public `document_id` and `owner_id`, allowing callers to construct contradictory targets. When a document is present, `sign()` obtains the nonce using `self.owner_id`, while DPP derives the batch owner and transition ID from the stored document; `self.document_id` is ignored. Mutating the formerly canonical fields after `from_document`, or assigning `document` directly, can therefore reserve a nonce for one identity while constructing and signing a transition for another. Keep one canonical target representation, make the full-document field private, or validate that the duplicated ID and owner fields match before touching the nonce cache.
- [SUGGESTION] packages/rs-sdk/src/platform/documents/transitions/delete.rs:80-94: Add direct regression coverage for SDK delete variant routing
No test exercises the client routing introduced here. The existing wasm-sdk deletion tests use the id-only plain-object form, so they never enter the new `Document` branch, and the rs-sdk tests do not verify that `from_document` retains values and selects a V1 delete for an index-only type. Add focused coverage for a successful full-document index-only delete, an id-only rejection that does not advance the nonce cache, the WASM `Document` dispatch, and a type requiring `$createdAt`; lower-layer transition, storage, and proof tests do not cover this SDK plumbing.
In `packages/wasm-sdk/src/state_transitions/document.rs`:
- [BLOCKING] packages/wasm-sdk/src/state_transitions/document.rs:461-466: Preserve the confirmed document needed for timestamp-indexed deletion
The new `Document` branch forwards the original JS document, but a newly constructed `DocumentWasm` has `created_at: None`. During creation, Platform assigns the consensus block timestamp when `$createdAt` is required, and `put_to_platform_and_wait_for_response` returns a confirmed `Document` containing that timestamp; `documentCreate` currently discards that return value and leaves the original wrapper unchanged. Reusing the documented `Document` instance for deletion therefore reaches `DocumentDeleteTransitionV1::from_document`, which rejects it because the required `$createdAt` is absent. Return the confirmed `DocumentWasm` from creation or update the caller-visible wrapper so the Document-based delete path receives all consensus-populated system fields.
c0023aa to
dd81bfa
Compare
Rebased onto v4.2-dev after #4497/#4494: the builders go through the batch factory, which now selects the DocumentIndexOnlyDeleteTransition KIND from the doctype's storage mode, so the SDK surface needed no API changes — the delete builder keeps the full document when built from one (mandatory for indexOnly types, whose values are the payload) and the wasm-sdk delete path routes Document instances through from_document. The book chapter documents the as-merged design: the delete as its own kind, the commitment-checked execution proofs with their AffectedState semantics, and the framed synthetic-id formula. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9bd1616 to
b39be70
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4495 +/- ##
=========================================
Coverage 83.00% 83.00%
=========================================
Files 2744 2744
Lines 367956 367956
=========================================
Hits 305411 305411
Misses 62545 62545
🚀 New features to boost your workflow:
|
…nfirmed document from create Review fixes (thepastaclaw on #4495): - rs-sdk: the delete builder resolves and validates its target (doctype lookup, indexOnly id-only refusal, stored-document/id/owner consistency) in a pure step BEFORE sign() reserves the identity contract nonce — a rejected call previously bumped the cached nonce with nothing broadcast, and enough of those pushed the next valid transition past the protocol's missing-revision window (NonceTooFarInFuture). A stored full document that contradicts the builder's public id/owner fields is refused instead of signing for a different identity than the nonce was reserved for. Unit tests cover all four routing paths. - wasm-sdk / js-evo-sdk: documentCreate now returns the confirmed Document as Platform committed it (consensus-populated system fields included) instead of discarding it — the Document-based delete path needs $createdAt for indexOnly types that require it, and the caller's pre-broadcast wrapper never learns the assigned block timestamp. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes (thepastaclaw + coderabbit on #4499): - The terminal route now stands in ONLY for the two find_best_index errors that mean 'no generic index matches' (WhereClauseOnNonIndexed- Property / QueryTooFarFromIndex) and only when the query carries no resolved time range — structural preflight and version-dispatch errors propagate unchanged instead of being swallowed by a route change. - The two synthesis-level startAt rejections now carry the keyset guidance instead of the obsolete 'not yet supported'. - The keyset pagination test is bounded at four iterations so a non-progress regression fails the walked assertion instead of hanging. - Repairs the doc-comment mixup my #4495 restructure left in the delete builder: sign()'s Arguments/Returns block had been orphaned above the new helper, whose unindented prose read as a lazy markdown list continuation — 7 doc_lazy_continuation errors under the CI's -D warnings, breaking the workspace test job on this PR and on v4.2-dev itself. The block is back on sign(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both sides fixed the #4495 doc_lazy_continuation lint in the delete builder: v4.2-dev's #4498 kept sign()'s Arguments/Returns block above the extracted helper with a separator line, this branch moved it back onto sign() itself. Kept this branch's placement — the block documents sign(), and it already lives there exactly once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Issue being fixed or feature implemented
Fifth and final PR of the indexOnly document types stack (rebased onto v4.2-dev after #4497 and #4494 merged): the client surface and long-term documentation.
What was done?
DocumentDeleteTransitionBuildernow keeps the full document when constructed viafrom_document(previously it discarded everything but the ids), andsign()refuses an id-only builder for an indexOnly type with guidance — the delete transition carries the document's values, and the batch factory selects theDocumentIndexOnlyDeleteTransitionKIND from the doctype's storage mode (feat(dpp)!: indexOnly delete-by-values as its own transition kind #4497's design — delete-by-values is its own operation, not a version of the delete).documentDeleteroutes a providedDocumentinstance throughfrom_documentso its values ride along; the id-only plain-object form keeps working for stored document types. Queries and proofs need no client changes —FromProof for Documentsflows through the synthesis path from feat(drive)!: indexOnly read surface — synthesize documents from index positions #4494.book/src/drive/index-only-document-types.mdcovering the layout, the row commitment, the constraint matrix, the lifecycle, the synthesis read surface, and the cost story; registered in SUMMARY and cross-linked with the ranked-trees chapter.How Has This Been Tested?
dash-sdk and wasm-sdk compile checks; the underlying flows are covered by the storage/ABCI/read-surface suites in the earlier PRs of the stack (including the SDK-equivalent executed-proof roundtrips).
Deferred with rationale: a platform-test-suite functional spec requires the legacy js-dash-sdk's delete factory to build the indexOnlyDelete kind first — the evo (wasm-sdk) flow is the covered path; tracked as a follow-up alongside startAt cursors, terminal-property where clauses, and the sum-axis/timeRange extensions.
Breaking Changes
None — client-side plumbing only.
Checklist:
Stack: #4491 (schema) → #4492 (storage) → #4493 (transitions/ABCI) → #4494 (queries/proofs) → this.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation