feat(drive): add document history retrieval#3725
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end document-history support: new GetDocumentHistory proto/gRPC API, Drive fetch/prove/verify implementations with version dispatch and limit validation, proof-verifier integration, and SDK/JS/WASM client bindings and tests. ChangesDocument History Query & Proof Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3725 +/- ##
============================================
- Coverage 87.17% 87.14% -0.03%
============================================
Files 2601 2611 +10
Lines 318220 318943 +723
============================================
+ Hits 277408 277957 +549
- Misses 40812 40986 +174
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rs (1)
25-27: ⚡ Quick winAdd freeze assertions for the new historical query method slots.
These fields become part of historical method tables too; adding explicit freeze checks (like the existing
primary_key_tree_typeguard) will prevent accidental version bumps in already-shipped protocol tables.Proposed test additions
#[cfg(test)] mod historical_method_table_freeze { @@ #[test] fn v3_primary_key_tree_type_selects_v1_dispatch() { assert_eq!( DRIVE_DOCUMENT_METHOD_VERSIONS_V3.primary_key_tree_type, 1, @@ ); } + + #[test] + fn v2_document_history_query_slots_are_frozen_at_v0_dispatch() { + assert_eq!(DRIVE_DOCUMENT_METHOD_VERSIONS_V2.query.fetch_document_history_query, 0); + assert_eq!(DRIVE_DOCUMENT_METHOD_VERSIONS_V2.query.fetch_document_history, 0); + assert_eq!(DRIVE_DOCUMENT_METHOD_VERSIONS_V2.query.prove_document_history, 0); + } + + #[test] + fn v3_document_history_query_slots_are_frozen_at_v0_dispatch() { + assert_eq!(DRIVE_DOCUMENT_METHOD_VERSIONS_V3.query.fetch_document_history_query, 0); + assert_eq!(DRIVE_DOCUMENT_METHOD_VERSIONS_V3.query.fetch_document_history, 0); + assert_eq!(DRIVE_DOCUMENT_METHOD_VERSIONS_V3.query.prove_document_history, 0); + } }🤖 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-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rs` around lines 25 - 27, Add freeze assertions for the new historical method slots so they cannot be changed after shipping: in the same place where primary_key_tree_type is guarded, add checks that fetch_document_history_query, fetch_document_history, and prove_document_history are frozen (or assert their versions/flags) to prevent accidental bumps in historical method tables; mirror the exact pattern used for primary_key_tree_type to locate and implement these guards inside the drive_document_method_versions module.
🤖 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.
Inline comments:
In `@packages/rs-drive/src/verify/document/verify_document_history/v0/mod.rs`:
- Around line 17-27: The current verify_document_history_v0 decodes every proved
history entry using a single DocumentTypeRef which breaks when the contract
schema changed; update verify_document_history_v0 so that for each history entry
you resolve the DocumentTypeRef that was active at that entry's timestamp (i.e.
lookup the contract revision for the entry's revision/timestamp and use that
revision's document type) before decoding, or alternatively detect if the
requested range spans multiple contract revisions and return a clear error
rejecting cross-revision history requests; make this change where entries are
iterated/decoded (and similarly for the logic referenced around lines 62-69) so
each Document is decoded against the correct historical schema.
---
Nitpick comments:
In
`@packages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rs`:
- Around line 25-27: Add freeze assertions for the new historical method slots
so they cannot be changed after shipping: in the same place where
primary_key_tree_type is guarded, add checks that fetch_document_history_query,
fetch_document_history, and prove_document_history are frozen (or assert their
versions/flags) to prevent accidental bumps in historical method tables; mirror
the exact pattern used for primary_key_tree_type to locate and implement these
guards inside the drive_document_method_versions module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5743c558-0198-4f06-ab5c-4d3a5b2526c6
📒 Files selected for processing (41)
packages/dapi-grpc/build.rspackages/dapi-grpc/protos/platform/v0/platform.protopackages/js-evo-sdk/src/documents/facade.tspackages/js-evo-sdk/tests/unit/facades/documents.spec.tspackages/rs-dapi-client/src/transport/grpc.rspackages/rs-drive-abci/src/query/document_history/mod.rspackages/rs-drive-abci/src/query/document_history/v0/mod.rspackages/rs-drive-abci/src/query/mod.rspackages/rs-drive-abci/src/query/service.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/types.rspackages/rs-drive/src/drive/document/get_fetch/fetch_document_history/mod.rspackages/rs-drive/src/drive/document/get_fetch/fetch_document_history/v0/mod.rspackages/rs-drive/src/drive/document/get_fetch/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/drive/document/prove/mod.rspackages/rs-drive/src/drive/document/prove/prove_document_history/mod.rspackages/rs-drive/src/drive/document/prove/prove_document_history/v0/mod.rspackages/rs-drive/src/drive/document/query/fetch_document_history_query/mod.rspackages/rs-drive/src/drive/document/query/fetch_document_history_query/v0/mod.rspackages/rs-drive/src/drive/document/query/mod.rspackages/rs-drive/src/error/drive.rspackages/rs-drive/src/verify/document/mod.rspackages/rs-drive/src/verify/document/verify_document_history/mod.rspackages/rs-drive/src/verify/document/verify_document_history/v0/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rspackages/rs-platform-version/src/version/mocks/v2_test.rspackages/rs-sdk/src/mock/sdk.rspackages/rs-sdk/src/platform.rspackages/rs-sdk/src/platform/documents/document_history_query.rspackages/rs-sdk/src/platform/documents/mod.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/query.rspackages/wasm-sdk/src/queries/document.rs
| pub(super) fn verify_document_history_v0( | ||
| proof: &[u8], | ||
| contract_id: [u8; 32], | ||
| document_type_name: &str, | ||
| document_type: DocumentTypeRef, | ||
| document_id: [u8; 32], | ||
| start_at_ms: u64, | ||
| limit: Option<u16>, | ||
| offset: Option<u16>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<(RootHash, Option<BTreeMap<u64, Document>>), Error> { |
There was a problem hiding this comment.
Resolve the document type per historical revision.
This implementation decodes every proved history entry with one DocumentTypeRef. That only holds if the contract schema never changed. Once a data contract update modifies this document type, older revisions in the same history range will be verified against the latest schema and can fail to decode or be interpreted incorrectly. Please either resolve the contract revision that was active for each timestamp, or explicitly reject history ranges that cross contract revisions until that lookup is wired in.
Also applies to: 62-69
🤖 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/src/verify/document/verify_document_history/v0/mod.rs`
around lines 17 - 27, The current verify_document_history_v0 decodes every
proved history entry using a single DocumentTypeRef which breaks when the
contract schema changed; update verify_document_history_v0 so that for each
history entry you resolve the DocumentTypeRef that was active at that entry's
timestamp (i.e. lookup the contract revision for the entry's revision/timestamp
and use that revision's document type) before decoding, or alternatively detect
if the requested range spans multiple contract revisions and return a clear
error rejecting cross-revision history requests; make this change where entries
are iterated/decoded (and similarly for the logic referenced around lines 62-69)
so each Document is decoded against the correct historical schema.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The document-history plumbing is mostly coherent, but three user-visible issues remain in the shipped surface. One is blocking: the protobuf service added getDocumentHistory, but the checked-in @dashevo/dapi-grpc generated clients were not regenerated, so published consumers of that package cannot call the new RPC. The other two issues are pagination-semantics regressions: empty history pages are treated as missing resources in the non-proof path and collapsed to None in proof verification.
Note: Inline posting through review_poster.py failed with GitHub HTTP 422, so I posted the same verified findings as a top-level review body.
Reviewed commit: b5a9d0b
🔴 1 blocking | 🟡 2 suggestion(s)
Verified findings
blocking: The new RPC is declared in the proto, but the checked-in generated DAPI clients do not expose it
packages/dapi-grpc/protos/platform/v0/platform.proto (line 38)
@dashevo/dapi-grpc ships generated client artifacts from packages/dapi-grpc/clients and re-exports them from node.js and browser.js. After adding rpc getDocumentHistory(...), the repository still contains no generated getDocumentHistory / GetDocumentHistory* symbols under packages/dapi-grpc/clients/platform/v0, so workspace and published-package consumers that rely on those checked-in clients cannot construct or invoke the new method. The Rust path works because it regenerates from the proto during build, but the package that advertises generated clients is stale in the committed source.
suggestion: Empty document-history pages are converted into `NotFound` in the non-proof path
packages/rs-drive-abci/src/query/document_history/v0/mod.rs (line 120)
This branch turns any empty result set into QueryError::NotFound, which breaks normal pagination semantics. Requests such as offset past the last revision or start_at_ms after the newest revision are valid queries against an existing document history and should return an empty page, not a transport error. As written, raw DAPI callers using prove = false cannot paginate until exhaustion without treating the final page as failure, and they cannot distinguish an empty page from an actually invalid document ID.
suggestion: Proof verification collapses an empty history page into `None`
packages/rs-drive/src/verify/document/verify_document_history/v0/mod.rs (line 83)
verify_document_history_v0() returns None whenever the verified map is empty. That loses information for paginated history queries, because a valid empty page and a genuinely absent history become indistinguishable after proof verification. The analogous contract-history verifier returns Some(empty_map), which preserves collection semantics. Returning Some(documents) here keeps the proof-decoded result lossless and aligns the verifier with the collection behavior expected by limit/offset queries.
Ok((root_hash, Some(documents)))
🤖 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.
- [BLOCKING] In `packages/dapi-grpc/protos/platform/v0/platform.proto`:38-39: The new RPC is declared in the proto, but the checked-in generated DAPI clients do not expose it
`@dashevo/dapi-grpc` ships generated client artifacts from `packages/dapi-grpc/clients` and re-exports them from `node.js` and `browser.js`. After adding `rpc getDocumentHistory(...)`, the repository still contains no generated `getDocumentHistory` / `GetDocumentHistory*` symbols under `packages/dapi-grpc/clients/platform/v0`, so workspace and published-package consumers that rely on those checked-in clients cannot construct or invoke the new method. The Rust path works because it regenerates from the proto during build, but the package that advertises generated clients is stale in the committed source.
- [SUGGESTION] In `packages/rs-drive-abci/src/query/document_history/v0/mod.rs`:120-123: Empty document-history pages are converted into `NotFound` in the non-proof path
This branch turns any empty result set into `QueryError::NotFound`, which breaks normal pagination semantics. Requests such as `offset` past the last revision or `start_at_ms` after the newest revision are valid queries against an existing document history and should return an empty page, not a transport error. As written, raw DAPI callers using `prove = false` cannot paginate until exhaustion without treating the final page as failure, and they cannot distinguish an empty page from an actually invalid document ID.
- [SUGGESTION] In `packages/rs-drive/src/verify/document/verify_document_history/v0/mod.rs`:83-89: Proof verification collapses an empty history page into `None`
`verify_document_history_v0()` returns `None` whenever the verified map is empty. That loses information for paginated history queries, because a valid empty page and a genuinely absent history become indistinguishable after proof verification. The analogous contract-history verifier returns `Some(empty_map)`, which preserves collection semantics. Returning `Some(documents)` here keeps the proof-decoded result lossless and aligns the verifier with the collection behavior expected by limit/offset queries.
Address automated review findings by regenerating DAPI clients and preserving empty document history pages.
PastaPastaPasta
force-pushed
the
codex/document-history-retrieval
branch
from
5bf74f3 to
4e481a1
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/rs-drive/src/verify/document/verify_document_history/v0/mod.rs (1)
62-70:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDecode each history entry against the schema active at that timestamp.
All entries are deserialized with a single
document_type(Line 68). If history spans contract revisions, older entries can be decoded with the wrong schema. Please resolve document type per historical revision (or explicitly reject cross-revision ranges).🤖 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/src/verify/document/verify_document_history/v0/mod.rs` around lines 62 - 70, The code currently decodes every history entry using the single variable document_type inside the maybe_document mapping (see maybe_element.map and the call to Document::from_bytes), which will mis-decode entries from older contract revisions; change the logic so that for each element you resolve the correct document_type for that element's historical revision (e.g., read the element's revision/timestamp/metadata from maybe_element or its container) and pass that resolved type into Document::from_bytes(document_bytes, resolved_document_type, platform_version), or alternatively detect when the history spans multiple incompatible revisions and return an explicit error (reject cross-revision ranges). Ensure you update the code path around maybe_element, into_item_bytes(), and Document::from_bytes to use the per-entry resolved_document_type instead of the outer document_type.
🤖 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.
Duplicate comments:
In `@packages/rs-drive/src/verify/document/verify_document_history/v0/mod.rs`:
- Around line 62-70: The code currently decodes every history entry using the
single variable document_type inside the maybe_document mapping (see
maybe_element.map and the call to Document::from_bytes), which will mis-decode
entries from older contract revisions; change the logic so that for each element
you resolve the correct document_type for that element's historical revision
(e.g., read the element's revision/timestamp/metadata from maybe_element or its
container) and pass that resolved type into Document::from_bytes(document_bytes,
resolved_document_type, platform_version), or alternatively detect when the
history spans multiple incompatible revisions and return an explicit error
(reject cross-revision ranges). Ensure you update the code path around
maybe_element, into_item_bytes(), and Document::from_bytes to use the per-entry
resolved_document_type instead of the outer document_type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2189f8e5-9f7c-4bfc-9c9f-75296b7fa464
📒 Files selected for processing (12)
packages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/PlatformPromiseClient.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.jspackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb.jspackages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb_service.jspackages/rs-drive-abci/src/query/document_history/v0/mod.rspackages/rs-drive/src/drive/document/get_fetch/fetch_document_history/mod.rspackages/rs-drive/src/verify/document/verify_document_history/v0/mod.rs
✅ Files skipped from review due to trivial changes (3)
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts
- packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.js
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/rs-drive-proof-verifier/src/proof.rs (1)
1432-1509: ⚡ Quick winAdd
GetDocumentHistoryerror-path coverage.This impl adds request decoding, contract lookup, document-type resolution, and pagination parsing, but the test module below does not exercise those branches yet. Mirroring the malformed-request tests used for the adjacent
FromProofimpls would make regressions here much cheaper to catch.🤖 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-proof-verifier/src/proof.rs` around lines 1432 - 1509, The new FromProof for platform::GetDocumentHistoryRequest lacks unit tests for its error paths; add tests mirroring the malformed-request cases used by adjacent FromProof impls to exercise request decoding failures (EmptyVersion / malformed v0 fields), u32_to_u16_opt pagination errors, provider.get_data_contract returning None (not found), data_contract.document_type_for_name errors, and response proof/metadata absence (NoProofInResult / EmptyResponseMetadata); create tests that call DocumentHistory::maybe_from_proof_with_metadata with crafted requests/responses to trigger each branch and assert the expected Error variant, referencing maybe_from_proof_with_metadata, u32_to_u16_opt, provider.get_data_contract, data_contract.document_type_for_name, and verify_document_history as anchors when crafting the cases.
🤖 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.
Inline comments:
In `@packages/rs-drive-proof-verifier/src/proof.rs`:
- Around line 1503-1505: The return currently maps maybe_history to
IndexMap::from_iter and can yield Some(IndexMap::new()) for empty histories;
update the tuple return so the mapped history is converted to an Option that
normalizes empty maps to None by calling into_option() on the mapped result
(i.e., replace maybe_history.map(IndexMap::from_iter) with
maybe_history.map(IndexMap::from_iter).into_option() or equivalent) so the
behavior matches FromProof and other collection helpers; keep mtd.clone()
unchanged.
In `@packages/rs-sdk/src/platform/fetch.rs`:
- Around line 339-341: The impl of the Fetch trait for
drive_proof_verifier::types::DocumentHistory is missing the required associated
type Query; update the impl for Fetch (the block implementing Fetch for
drive_proof_verifier::types::DocumentHistory) to declare type Query =
platform_proto::GetDocumentHistoryQuery (or the correct query type used by this
API) alongside the existing type Request so the impl satisfies the Fetch trait’s
associated types.
In `@packages/rs-sdk/src/platform/query.rs`:
- Around line 189-209: The impl for Query<proto::GetDocumentHistoryRequest> on
DocumentHistoryQuery has the wrong signature: replace the existing fn
query(self, prove: bool) with the trait-required signature fn query(&self,
settings: &crate::platform::QuerySettings<'_>) ->
Result<proto::GetDocumentHistoryRequest, Error>; read the proof flag and any
pagination parameters from settings (instead of the old boolean) and build the
proto::GetDocumentHistoryRequest using self.document_type_name,
self.data_contract_id, self.document_id, self.limit, self.offset,
self.start_at_ms, and settings.prove (or equivalent), returning the constructed
request wrapped in Ok to satisfy the Query trait.
---
Nitpick comments:
In `@packages/rs-drive-proof-verifier/src/proof.rs`:
- Around line 1432-1509: The new FromProof for
platform::GetDocumentHistoryRequest lacks unit tests for its error paths; add
tests mirroring the malformed-request cases used by adjacent FromProof impls to
exercise request decoding failures (EmptyVersion / malformed v0 fields),
u32_to_u16_opt pagination errors, provider.get_data_contract returning None (not
found), data_contract.document_type_for_name errors, and response proof/metadata
absence (NoProofInResult / EmptyResponseMetadata); create tests that call
DocumentHistory::maybe_from_proof_with_metadata with crafted requests/responses
to trigger each branch and assert the expected Error variant, referencing
maybe_from_proof_with_metadata, u32_to_u16_opt, provider.get_data_contract,
data_contract.document_type_for_name, and verify_document_history as anchors
when crafting the cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0f1f9eb-5ee3-4071-897a-68445222073d
📒 Files selected for processing (51)
packages/dapi-grpc/build.rspackages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/PlatformPromiseClient.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/web/PlatformPromiseClient.jspackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb.jspackages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb_service.jspackages/dapi-grpc/protos/platform/v0/platform.protopackages/js-evo-sdk/src/documents/facade.tspackages/js-evo-sdk/tests/unit/facades/documents.spec.tspackages/rs-dapi-client/src/transport/grpc.rspackages/rs-drive-abci/src/query/document_history/mod.rspackages/rs-drive-abci/src/query/document_history/v0/mod.rspackages/rs-drive-abci/src/query/mod.rspackages/rs-drive-abci/src/query/service.rspackages/rs-drive-proof-verifier/src/proof.rspackages/rs-drive-proof-verifier/src/types.rspackages/rs-drive/src/drive/document/get_fetch/fetch_document_history/mod.rspackages/rs-drive/src/drive/document/get_fetch/fetch_document_history/v0/mod.rspackages/rs-drive/src/drive/document/get_fetch/mod.rspackages/rs-drive/src/drive/document/mod.rspackages/rs-drive/src/drive/document/prove/mod.rspackages/rs-drive/src/drive/document/prove/prove_document_history/mod.rspackages/rs-drive/src/drive/document/prove/prove_document_history/v0/mod.rspackages/rs-drive/src/drive/document/query/fetch_document_history_query/mod.rspackages/rs-drive/src/drive/document/query/fetch_document_history_query/v0/mod.rspackages/rs-drive/src/drive/document/query/mod.rspackages/rs-drive/src/error/drive.rspackages/rs-drive/src/verify/document/mod.rspackages/rs-drive/src/verify/document/verify_document_history/mod.rspackages/rs-drive/src/verify/document/verify_document_history/v0/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v0.rspackages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rspackages/rs-platform-version/src/version/mocks/v2_test.rspackages/rs-sdk/src/mock/sdk.rspackages/rs-sdk/src/platform.rspackages/rs-sdk/src/platform/documents/document_history_query.rspackages/rs-sdk/src/platform/documents/mod.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/query.rspackages/wasm-sdk/src/queries/document.rs
✅ Files skipped from review due to trivial changes (6)
- packages/rs-drive-abci/src/query/mod.rs
- packages/rs-sdk/src/platform.rs
- packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js
- packages/rs-drive/src/drive/document/prove/mod.rs
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.js
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding 1 is FIXED: getDocumentHistory is now declared in the proto and exposed by the checked-in generated DAPI clients in both Node and web artifacts. Prior finding 2 is FIXED: the ABCI non-proof path now returns an empty document_entries list instead of converting empty pages into NotFound, and prior finding 3 is FIXED: proof verification now preserves empty history pages as Some(empty) rather than collapsing them to absence. There are no carried-forward prior findings at 5bf74f34797caf6b8dff9d5979f799b55e9e4122, and I did not confirm any new PR-specific correctness issues in the latest delta.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The immediately prior review at commit 5bf74f3 reported no findings, so there are no prior items to classify or carry forward. Carried-forward prior findings: none. New findings in the latest delta: two blocking SDK trait-migration errors that leave the new DocumentHistory fetch path uncompilable at head 4e481a15456194a842b6e5b387e8c3f802ba4fb4.
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3725 4e481a1
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._
Reviewed commit: 4e481a1
🔴 2 blocking
Verified findings
blocking: `DocumentHistoryQuery` no longer implements the `Query` trait’s current method signature
packages/rs-sdk/src/platform/query.rs (line 189)
Query<T> now requires fn query(&self, settings: &QuerySettings<'_>) -> Result<T, Error>, but this impl still defines the removed fn query(self, prove: bool) form. That means DocumentHistoryQuery does not implement Query<GetDocumentHistoryRequest> at all, so DocumentHistory::fetch(...) cannot accept it and the new WASM/JS document-history entry points lose their request encoder.
impl Query<proto::GetDocumentHistoryRequest> for DocumentHistoryQuery {
fn query(
&self,
settings: &crate::platform::QuerySettings<'_>,
) -> Result<proto::GetDocumentHistoryRequest, Error> {
let prove = settings.prove;
if !prove {
unimplemented!("queries without proofs are not supported yet");
}
Ok(proto::GetDocumentHistoryRequest {
version: Some(proto::get_document_history_request::Version::V0(
proto::get_document_history_request::GetDocumentHistoryRequestV0 {
data_contract_id: self.data_contract_id.to_vec(),
document_type_name: self.document_type_name.clone(),
document_id: self.document_id.to_vec(),
limit: self.limit,
offset: self.offset,
start_at_ms: self.start_at_ms,
prove,
},
)),
})
}
}
blocking: `Fetch for DocumentHistory` is missing the required `Query` associated type
packages/rs-sdk/src/platform/fetch.rs (line 339)
The Fetch trait now requires both type Query and type Request. This impl only defines type Request, so it is incomplete and cannot compile. For DocumentHistory, the Query associated type must stay GetDocumentHistoryRequest, because the existing FromProof implementation is defined for that proto request type.
impl Fetch for drive_proof_verifier::types::DocumentHistory {
type Query = platform_proto::GetDocumentHistoryRequest;
type Request = platform_proto::GetDocumentHistoryRequest;
}
🤖 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.
- [BLOCKING] In `packages/rs-sdk/src/platform/query.rs`:189-208: `DocumentHistoryQuery` no longer implements the `Query` trait’s current method signature
`Query<T>` now requires `fn query(&self, settings: &QuerySettings<'_>) -> Result<T, Error>`, but this impl still defines the removed `fn query(self, prove: bool)` form. That means `DocumentHistoryQuery` does not implement `Query<GetDocumentHistoryRequest>` at all, so `DocumentHistory::fetch(...)` cannot accept it and the new WASM/JS document-history entry points lose their request encoder.
- [BLOCKING] In `packages/rs-sdk/src/platform/fetch.rs`:339-340: `Fetch for DocumentHistory` is missing the required `Query` associated type
The `Fetch` trait now requires both `type Query` and `type Request`. This impl only defines `type Request`, so it is incomplete and cannot compile. For `DocumentHistory`, the `Query` associated type must stay `GetDocumentHistoryRequest`, because the existing `FromProof` implementation is defined for that proto request type.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/rs-drive-proof-verifier/src/proof.rs`:
- Around line 3342-3354: StaticDataContractProvider::get_data_contract currently
returns the fixture unconditionally; change it to validate the requested id
against the provider's stored contract id and only return
Some(self.data_contract.clone()) when they match, otherwise return Ok(None) (or
the appropriate not-found result). Locate the struct StaticDataContractProvider
and its get_data_contract implementation and compare the incoming
&dpp::prelude::Identifier with the data_contract's id (use the DataContract id
accessor on the stored Arc) and return None on mismatch; apply the same
validation fix to the other analogous stub implementation referenced around the
4977-4995 region.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ecb9183-676b-4960-9885-937a899d6342
📒 Files selected for processing (4)
packages/rs-drive-proof-verifier/Cargo.tomlpackages/rs-drive-proof-verifier/src/proof.rspackages/rs-sdk/src/platform/fetch.rspackages/rs-sdk/src/platform/query.rs
✅ Files skipped from review due to trivial changes (1)
- packages/rs-drive-proof-verifier/Cargo.toml
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding 1 is FIXED: DocumentHistoryQuery now implements Query<GetDocumentHistoryRequest> with the current fn query(&self, settings: &QuerySettings<'_>) signature in packages/rs-sdk/src/platform/query.rs. Prior finding 2 is FIXED: Fetch for drive_proof_verifier::types::DocumentHistory now defines both type Query and type Request in packages/rs-sdk/src/platform/fetch.rs. Carried-forward prior findings: none. New findings in the latest delta: none; the only additional CodeRabbit comment targets a test-only stub and does not describe a real defect in the production path.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "2b68288b48d4e01329aa031ec1383a195d8f87d508146e8af7475ffb28c1dc76"
)Xcode manual integration:
|
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: none. Prior findings status: none to carry forward, and the previously fixed compile/API-contract areas remain fixed at 1e99d65ca4a2b80563d074a89e9af88eef1ec957; new findings in the latest delta: none. Separately, cumulative review of the current head still finds one blocking SDK panic outside the latest diff, so the PR is not approvable as-is.
Note: Inline posting hit GitHub HTTP 422, so I posted the same verified finding as a top-level review body.
Reviewed commit: 1e99d65
🔴 1 blocking
Verified findings
blocking: `DocumentHistoryQuery` still panics when the SDK is configured with proofs disabled
packages/rs-sdk/src/platform/query.rs (line 194)
Fetch::fetch() always builds its encoder context from sdk.query_settings() before any network round-trip. For DocumentHistory, that reaches this Query<GetDocumentHistoryRequest> impl, which calls unimplemented!() whenever SdkBuilder::with_proofs(false) was used. That makes a public library API abort the process instead of behaving like the existing document-fetch surface. The surrounding SDK already established the accepted pattern for proof-backed document reads in document_query.rs: those encoders force prove: true because the FromProof decoder path requires proof responses. DocumentHistory uses the same proof-verification fetch path, so this branch should hardcode proof mode as well instead of panicking. This issue is present on the current head but predates the latest delta.
let _ = settings;
let prove = true;
🤖 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.
- [BLOCKING] In `packages/rs-sdk/src/platform/query.rs`:194-197: `DocumentHistoryQuery` still panics when the SDK is configured with proofs disabled
`Fetch::fetch()` always builds its encoder context from `sdk.query_settings()` before any network round-trip. For `DocumentHistory`, that reaches this `Query<GetDocumentHistoryRequest>` impl, which calls `unimplemented!()` whenever `SdkBuilder::with_proofs(false)` was used. That makes a public library API abort the process instead of behaving like the existing document-fetch surface. The surrounding SDK already established the accepted pattern for proof-backed document reads in `document_query.rs`: those encoders force `prove: true` because the `FromProof` decoder path requires proof responses. `DocumentHistory` uses the same proof-verification fetch path, so this branch should hardcode proof mode as well instead of panicking. This issue is present on the current head but predates the latest delta.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The prior DocumentHistoryQuery proofs-disabled panic finding is STILL VALID on head 4c61c67c6d9460a5e5f1ec2d614e861d907859e9 and remains a blocking SDK bug. Carried-forward prior findings: 1 confirmed blocker in rs-sdk. New findings in the latest delta: none in the 1e99d65c..4c61c67c transport wiring itself, but there is one additional current-head cumulative decoder issue in the existing proof path that still affects the PR as merged.
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3725 4c61c67
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._
Reviewed commit: 4c61c67
🔴 1 blocking | 🟡 1 suggestion(s)
Verified findings
blocking: [carried-forward prior finding][STILL VALID] `DocumentHistoryQuery` panics when the SDK is configured with proofs disabled
packages/rs-sdk/src/platform/query.rs (line 194)
Fetch::fetch_with_metadata_and_proof() always derives encoder settings from sdk.query_settings() before any network call, and SdkBuilder::with_proofs(false) sets settings.prove = false. In the current head this query encoder still executes unimplemented!() on that path, so DocumentHistory::fetch(...) aborts locally instead of producing a request or a typed error. That makes the public SDK API unusable for a configuration the builder explicitly exposes, and it is inconsistent with adjacent document-read paths that hard-force proved reads when their decoders require proof responses.
let _ = settings;
let prove = true;
suggestion: [new cumulative finding] Proof decoding collapses a valid empty document-history page into `None`
packages/rs-drive-proof-verifier/src/proof.rs (line 1503)
Drive::verify_document_history() always returns Some(documents) and the verified map can be empty for a successful page beyond the last revision. The ABCI query layer has an explicit test for that successful empty-page case, and DocumentHistory is a RetrievedValues<u64, Document> map rather than a proof-of-absence structure. This decoder discards that distinction by converting the map with into_option(), so proved reads return Ok(None) where unproved/server semantics are “success with an empty history page”. That makes the Rust SDK inconsistent with the server contract and with the adjacent DataContractHistory proof path, which preserves Some(empty_map).
Ok((
maybe_history.map(IndexMap::from_iter),
mtd.clone(),
proof.clone(),
))
🤖 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.
- [BLOCKING] In `packages/rs-sdk/src/platform/query.rs`:194-197: [carried-forward prior finding][STILL VALID] `DocumentHistoryQuery` panics when the SDK is configured with proofs disabled
`Fetch::fetch_with_metadata_and_proof()` always derives encoder settings from `sdk.query_settings()` before any network call, and `SdkBuilder::with_proofs(false)` sets `settings.prove = false`. In the current head this query encoder still executes `unimplemented!()` on that path, so `DocumentHistory::fetch(...)` aborts locally instead of producing a request or a typed error. That makes the public SDK API unusable for a configuration the builder explicitly exposes, and it is inconsistent with adjacent document-read paths that hard-force proved reads when their decoders require proof responses.
- [SUGGESTION] In `packages/rs-drive-proof-verifier/src/proof.rs`:1503-1505: [new cumulative finding] Proof decoding collapses a valid empty document-history page into `None`
`Drive::verify_document_history()` always returns `Some(documents)` and the verified map can be empty for a successful page beyond the last revision. The ABCI query layer has an explicit test for that successful empty-page case, and `DocumentHistory` is a `RetrievedValues<u64, Document>` map rather than a proof-of-absence structure. This decoder discards that distinction by converting the map with `into_option()`, so proved reads return `Ok(None)` where unproved/server semantics are “success with an empty history page”. That makes the Rust SDK inconsistent with the server contract and with the adjacent `DataContractHistory` proof path, which preserves `Some(empty_map)`.
2 participants
Issue being fixed or feature implemented
Adds an end-to-end public read path for document history when
documentsKeepHistoryis enabled. Today Drive stores historical revisions, but SDK users cannot retrieve them through DAPI / Evo SDK APIs.What was done?
getDocumentHistoryto DAPI proto and Rust gRPC transport.DocumentHistory.rs-sdk,wasm-sdk, andjs-evo-sdk.How Has This Been Tested?
cargo check -p dapi-grpc -p platform-version -p drive -p drive-abcicargo check -p drive-proof-verifier -p rs-dapi-client -p dash-sdk -p wasm-sdkcargo check -p drive-abcicargo test -p drive document_history -- --nocapturePATH="/opt/homebrew/opt/llvm/bin:$PATH" CC_wasm32_unknown_unknown="/opt/homebrew/opt/llvm/bin/clang" CXX_wasm32_unknown_unknown="/opt/homebrew/opt/llvm/bin/clang++" yarn workspace @dashevo/wasm-sdk buildyarn workspace @dashevo/evo-sdk buildyarn workspace @dashevo/evo-sdk test:unitBreaking Changes
None.
Checklist:
For repository code-owners and collaborators only
This pull request was created by Codex.
Summary by CodeRabbit