fix(sdk): deserialization error due to outdated contract cache#3052
Open
fix(sdk): deserialization error due to outdated contract cache#3052
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence DiagramsequenceDiagram
participant Client as SDK Consumer
participant SDK as Sdk
participant Cache as ContextProvider<br/>(Cache)
participant Network as Network/DAPI
participant Parser as FromProof<br/>Parser
Client->>SDK: fetch_document(query with old_contract)
SDK->>Cache: get cached old_contract
Cache-->>SDK: returns old_contract
SDK->>Network: fetch with old_contract
Network-->>Parser: send proof
Parser->>Parser: attempt parse with old_contract
Parser-->>SDK: ProtocolError(CorruptedSerialization)
rect rgba(255, 100, 100, 0.5)
Note over SDK: Detect deserialization error
SDK->>Network: refetch_contract_for_query()
Network-->>SDK: returns fresh_contract
SDK->>Cache: update_data_contract(fresh_contract)
Cache->>Cache: store in contract cache
end
rect rgba(100, 200, 100, 0.5)
Note over SDK: Retry with fresh contract
SDK->>SDK: clone_with_contract(fresh_contract)
SDK->>Network: fetch_document(query with fresh_contract)
Network-->>Parser: send proof
Parser->>Parser: parse with fresh_contract
Parser-->>SDK: Success(Document)
end
SDK-->>Client: Document
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
✅ gRPC Query Coverage Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/rs-sdk/tests/fetch/mock_document_contract_refresh.rs`:
- Around line 114-235: Rename the three tests so their function names start with
"should_" per guidelines: change test_fetch_document_retries_on_stale_contract
to should_fetch_document_retries_on_stale_contract,
test_fetch_many_documents_retries_on_stale_contract to
should_fetch_many_documents_retries_on_stale_contract, and
test_fetch_document_does_not_retry_on_other_errors to
should_fetch_document_does_not_retry_on_other_errors; update the async fn
identifiers accordingly wherever they are referenced (e.g., in this file) so the
test runner and imports continue to work.
Comment on lines
+114
to
+235
| /// Test: Document::fetch retries once when the first attempt fails with a deserialization error. | ||
| /// | ||
| /// Setup: | ||
| /// 1. Create old and new versions of a data contract | ||
| /// 2. Build a DocumentQuery using the OLD contract | ||
| /// 3. Mock the document query with old contract to return a CorruptedSerialization error | ||
| /// 4. Mock DataContract::fetch to return the NEW contract | ||
| /// 5. Mock the document query with new contract to return the expected document | ||
| /// 6. Verify that Document::fetch succeeds (retry worked) | ||
| #[tokio::test] | ||
| async fn test_fetch_document_retries_on_stale_contract() { | ||
| let mut sdk = Sdk::new_mock(); | ||
|
|
||
| let (old_contract, new_contract, new_doc_type) = make_old_and_new_contracts(); | ||
|
|
||
| let expected_document = new_doc_type | ||
| .random_document(None, sdk.version()) | ||
| .expect("create document"); | ||
| let document_id = expected_document.id(); | ||
|
|
||
| // Build a query using the old contract (simulates stale cache) | ||
| let old_query = DocumentQuery::new(old_contract.clone(), "document_type_name") | ||
| .expect("create document query with old contract") | ||
| .with_document_id(&document_id); | ||
|
|
||
| // 1) Old query should fail with a deserialization error | ||
| sdk.mock() | ||
| .expect_fetch_proof_error::<Document, _>(old_query.clone()) | ||
| .await | ||
| .expect("set error expectation"); | ||
|
|
||
| // 2) DataContract::fetch should return the new contract (refetch) | ||
| sdk.mock() | ||
| .expect_fetch(new_contract.id(), Some(new_contract.clone())) | ||
| .await | ||
| .expect("set contract refetch expectation"); | ||
|
|
||
| // 3) New query (with fresh contract) should succeed | ||
| let new_query = old_query.clone_with_contract(std::sync::Arc::new(new_contract)); | ||
| sdk.mock() | ||
| .expect_fetch(new_query, Some(expected_document.clone())) | ||
| .await | ||
| .expect("set document fetch expectation with new contract"); | ||
|
|
||
| // Execute: the retry logic should handle the error and succeed | ||
| let result = Document::fetch(&sdk, old_query) | ||
| .await | ||
| .expect("fetch should succeed after retry"); | ||
|
|
||
| let fetched = result.expect("document should be returned"); | ||
| assert_eq!(fetched.id(), expected_document.id()); | ||
| } | ||
|
|
||
| /// Test: Document::fetch_many retries once when the first attempt fails with a deserialization error. | ||
| #[tokio::test] | ||
| async fn test_fetch_many_documents_retries_on_stale_contract() { | ||
| let mut sdk = Sdk::new_mock(); | ||
|
|
||
| let (old_contract, new_contract, new_doc_type) = make_old_and_new_contracts(); | ||
|
|
||
| let expected_document = new_doc_type | ||
| .random_document(None, sdk.version()) | ||
| .expect("create document"); | ||
|
|
||
| let expected_documents = | ||
| Documents::from([(expected_document.id(), Some(expected_document.clone()))]); | ||
|
|
||
| // Build a query using the old contract (simulates stale cache) | ||
| let old_query = DocumentQuery::new(old_contract.clone(), "document_type_name") | ||
| .expect("create document query with old contract"); | ||
|
|
||
| // 1) Old query should fail with a deserialization error | ||
| // We use expect_fetch_proof_error with Document since DocumentQuery is the | ||
| // request type for both Fetch and FetchMany | ||
| sdk.mock() | ||
| .expect_fetch_proof_error::<Document, _>(old_query.clone()) | ||
| .await | ||
| .expect("set error expectation"); | ||
|
|
||
| // 2) DataContract::fetch should return the new contract (refetch) | ||
| sdk.mock() | ||
| .expect_fetch(new_contract.id(), Some(new_contract.clone())) | ||
| .await | ||
| .expect("set contract refetch expectation"); | ||
|
|
||
| // 3) New query (with fresh contract) should succeed | ||
| let new_query = old_query.clone_with_contract(std::sync::Arc::new(new_contract)); | ||
| sdk.mock() | ||
| .expect_fetch_many(new_query, Some(expected_documents.clone())) | ||
| .await | ||
| .expect("set document fetch_many expectation with new contract"); | ||
|
|
||
| // Execute: the retry logic should handle the error and succeed | ||
| let result = Document::fetch_many(&sdk, old_query) | ||
| .await | ||
| .expect("fetch_many should succeed after retry"); | ||
|
|
||
| assert_eq!(result.len(), 1); | ||
| let (doc_id, doc) = result.into_iter().next().unwrap(); | ||
| assert_eq!(doc_id, expected_document.id()); | ||
| assert!(doc.is_some()); | ||
| } | ||
|
|
||
| /// Test: When the error is NOT a deserialization error, no retry happens and the error propagates. | ||
| #[tokio::test] | ||
| async fn test_fetch_document_does_not_retry_on_other_errors() { | ||
| let sdk = Sdk::new_mock(); | ||
|
|
||
| let doc_type = mock_document_type(); | ||
| let contract = mock_data_contract(Some(&doc_type)); | ||
|
|
||
| // Build a query but don't set any expectations — the mock will have no matching | ||
| // response for execute, which should result in an error that is NOT a deserialization error. | ||
| let query = DocumentQuery::new(contract, doc_type.name()).expect("create document query"); | ||
|
|
||
| let result = Document::fetch(&sdk, query).await; | ||
|
|
||
| // Should fail — non-deserialization errors propagate without retry | ||
| assert!( | ||
| result.is_err(), | ||
| "expected error when no mock expectations are set" | ||
| ); |
Contributor
There was a problem hiding this comment.
Rename new tests to start with should_… for guideline compliance.
✏️ Suggested renames
-async fn test_fetch_document_retries_on_stale_contract() {
+async fn should_retry_document_fetch_on_stale_contract() {
-async fn test_fetch_many_documents_retries_on_stale_contract() {
+async fn should_retry_fetch_many_documents_on_stale_contract() {
-async fn test_fetch_document_does_not_retry_on_other_errors() {
+async fn should_not_retry_document_fetch_on_other_errors() {As per coding guidelines: **/tests/**/*.{js,jsx,ts,tsx,rs}: Name tests descriptively, starting with 'should …'.
🤖 Prompt for AI Agents
In `@packages/rs-sdk/tests/fetch/mock_document_contract_refresh.rs` around lines
114 - 235, Rename the three tests so their function names start with "should_"
per guidelines: change test_fetch_document_retries_on_stale_contract to
should_fetch_document_retries_on_stale_contract,
test_fetch_many_documents_retries_on_stale_contract to
should_fetch_many_documents_retries_on_stale_contract, and
test_fetch_document_does_not_retry_on_other_errors to
should_fetch_document_does_not_retry_on_other_errors; update the async fn
identifiers accordingly wherever they are referenced (e.g., in this file) so the
test runner and imports continue to work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
When a data contract is updated on the network, the SDK may hold a cached (old) version of the contract. Documents serialized with the new schema
fail to deserialize against the old cached contract, producing a
CorruptedSerializationerror that propagates to the caller with no recovery.What was done?
ProtocolErrorindrive-proof-verifierinstead of stringifying it, enabling pattern matching on specific error variantsdownstream.
Document::fetchandDocument::fetch_many: onCorruptedSerializationerror, the SDK refetches the contract from thenetwork, updates the context provider cache, and retries with the fresh contract.
update_data_contractmethod to theContextProvidertrait so the cache can be updated after a successful refetch.clone_with_contracttoDocumentQueryto rebuild the query with a fresh contract.How Has This Been Tested?
packages/rs-sdk/tests/fetch/mock_document_contract_refresh.rs:test_fetch_document_retries_on_stale_contract— verifiesDocument::fetchretries with a fresh contracttest_fetch_many_documents_retries_on_stale_contract— verifiesDocument::fetch_manyretries with a fresh contracttest_fetch_document_does_not_retry_on_other_errors— verifies non-deserialization errors propagate without retryis_document_deserialization_error()inpackages/rs-sdk/src/platform/fetch.rscargo clippyandcargo fmtclean on all affected cratesBreaking Changes
drive_proof_verifier::Error::ProtocolErrorchanged from a struct variant{ error: String }to a tuple variant(dpp::ProtocolError). Codematching on this variant needs updating.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes