feat(drive): add document history retrieval#3725
Conversation
📝 WalkthroughWalkthroughImplements end-to-end document history: protobuf/gRPC RPC, Drive query/prove/verify implementations with version dispatch and limit validation, proof-verifier integration, and SDK bindings for Rust, WASM, and JavaScript including client stubs and tests. ChangesDocument History Query & Proof Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tsParsing error: Unexpected token : packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.tsParsing error: Unexpected token PlatformbroadcastStateTransition 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.17%
=========================================
Files 2601 2601
Lines 318220 318220
=========================================
Hits 277408 277408
Misses 40812 40812
🚀 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
| Ok(( | ||
| maybe_history.map(IndexMap::from_iter), | ||
| mtd.clone(), |
There was a problem hiding this comment.
Normalize empty histories to None.
This can return Some(IndexMap::new()) for an empty verified page, which breaks the collection semantics documented on FromProof and makes empty history behave differently from the other collection helpers in this file. Run the mapped history through into_option() before returning it.
Suggested change
- Ok((
- maybe_history.map(IndexMap::from_iter),
- mtd.clone(),
- proof.clone(),
- ))
+ let history = maybe_history
+ .map(IndexMap::from_iter)
+ .and_then(|history| history.into_option());
+
+ Ok((history, mtd.clone(), proof.clone()))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Ok(( | |
| maybe_history.map(IndexMap::from_iter), | |
| mtd.clone(), | |
| let history = maybe_history | |
| .map(IndexMap::from_iter) | |
| .and_then(|history| history.into_option()); | |
| Ok((history, mtd.clone(), proof.clone())) |
🤖 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 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.
| impl Fetch for drive_proof_verifier::types::DocumentHistory { | ||
| type Request = platform_proto::GetDocumentHistoryRequest; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Fetch trait associated type requirements:"
rg -n -C3 'pub trait Fetch|type Query:|type Request:' packages/rs-sdk/src/platform/fetch.rs
echo
echo "DocumentHistory Fetch impl:"
rg -n -C4 'impl Fetch for drive_proof_verifier::types::DocumentHistory|type Query =|type Request =' packages/rs-sdk/src/platform/fetch.rsRepository: dashpay/platform
Length of output: 7666
Add missing type Query to Fetch impl for DocumentHistory
Fetch requires both associated types; impl Fetch for drive_proof_verifier::types::DocumentHistory defines only type Request, so this impl is incomplete and won’t compile (packages/rs-sdk/src/platform/fetch.rs, lines 339-341).
💡 Proposed fix
impl Fetch for drive_proof_verifier::types::DocumentHistory {
+ type Query = platform_proto::GetDocumentHistoryRequest;
type Request = platform_proto::GetDocumentHistoryRequest;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl Fetch for drive_proof_verifier::types::DocumentHistory { | |
| type Request = platform_proto::GetDocumentHistoryRequest; | |
| } | |
| impl Fetch for drive_proof_verifier::types::DocumentHistory { | |
| type Query = platform_proto::GetDocumentHistoryRequest; | |
| type Request = platform_proto::GetDocumentHistoryRequest; | |
| } |
🤖 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-sdk/src/platform/fetch.rs` around lines 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.
| impl Query<proto::GetDocumentHistoryRequest> for DocumentHistoryQuery { | ||
| fn query(self, prove: bool) -> Result<proto::GetDocumentHistoryRequest, Error> { | ||
| 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, | ||
| document_id: self.document_id.to_vec(), | ||
| limit: self.limit, | ||
| offset: self.offset, | ||
| start_at_ms: self.start_at_ms, | ||
| prove, | ||
| }, | ||
| )), | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/rs-sdk/src/platform/query.rs"
echo "Trait contract:"
rg -n -C2 'pub trait Query<.*>|fn query\(&self, settings: &crate::platform::QuerySettings' "$FILE"
echo
echo "DocumentHistoryQuery implementation:"
rg -n -C4 'impl Query<proto::GetDocumentHistoryRequest> for DocumentHistoryQuery|fn query\(' "$FILE"Repository: dashpay/platform
Length of output: 17814
Fix Query trait impl signature for DocumentHistoryQuery.
Query<proto::GetDocumentHistoryRequest> requires fn query(&self, settings: &crate::platform::QuerySettings<'_>), but DocumentHistoryQuery implements fn query(self, prove: bool), violating the trait contract and preventing compilation.
🤖 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-sdk/src/platform/query.rs` around lines 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.
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.
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