perf(sdk): cache contracts in JS SDK #2978
Conversation
📝 WalkthroughWalkthroughAdds contract caching and cache-management APIs across Rust and WASM SDK layers; refactors document/token flows to use cached contract retrieval (get_or_fetch_contract) instead of direct network fetches; trusted context creation now stores contexts in global caches. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WasmSdk
participant Cache as Contract Cache
participant Network as RPC / Platform
Client->>WasmSdk: get_or_fetch_contract(contract_id)
WasmSdk->>Cache: get_cached_contract(contract_id)
alt cached
Cache-->>WasmSdk: Some(contract)
WasmSdk-->>Client: return contract
else not cached
WasmSdk->>Network: fetch DataContract(contract_id)
Network-->>WasmSdk: DataContract
WasmSdk->>Cache: cache_contract(DataContract)
Cache-->>WasmSdk: stored
WasmSdk-->>Client: return contract
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-sdk-trusted-context-provider/src/provider.rs (1)
61-66: Contract cache is currently unbounded (HashMap) — likely memory growth risk.
Becauseknown_contractsis now used as a general cache by the WASM SDK, consider adding an eviction policy (e.g., LRU) or a hard cap, otherwise this can grow for the lifetime of the process. Also consider handlingMutexpoisoning more gracefully thanunwrap()for these public methods.Also applies to: 201-220
packages/wasm-sdk/src/queries/document.rs (1)
122-175: Perf gap: list queries bypass caching withDocumentQuery::new_with_data_contract_id.
This method performs an uncached network fetch viaDataContract::fetch(). Refactorbuild_documents_queryto usesdk.get_or_fetch_contract(contract_id)followed byDocumentQuery::new(contract, &document_type_name)sogetDocuments/getDocumentsWithProofInfobenefit from contract caching (already used elsewhere in this file).
🤖 Fix all issues with AI agents
In @packages/wasm-sdk/src/queries/token.rs:
- Around line 928-930: Before calling get_or_fetch_contract(contract_id) avoid
the unnecessary network fetch by first checking the per-network trusted context
initialization (use the existing trusted-context accessor/flag on self, e.g.,
check self.trusted_contexts or the ensure/initialized helper for the current
network). If the trusted context is not present/initialized, return the
appropriate error early instead of proceeding to Step 2; apply the same change
to the other block referenced (lines ~942–981) so both code paths validate
trusted context first and only call get_or_fetch_contract when the trusted
context is ready.
In @packages/wasm-sdk/src/sdk.rs:
- Around line 121-207: cache_contract/get_cached_contract/remove_cached_contract
only consult the global *_TRUSTED_CONTEXT singletons, so when an SDK is created
via new_mainnet_trusted/new_testnet_trusted/new_local_trusted (without
prefetch_trusted_quorums_*) caching is a no-op and get_or_fetch_contract will
repeatedly refetch; fix by making these cache methods consult the SDK instance's
trusted context/provider first (e.g., use a self.trusted_context or a method on
the SDK instance) and fall back to the globals, or alternatively ensure
new_*_trusted stores the created context into the corresponding global; update
cache_contract, get_cached_contract, remove_cached_contract (and any callers
like get_or_fetch_contract) to use the instance-scoped context before using
*_TRUSTED_CONTEXT, and keep prefetch_trusted_quorums_* behavior unchanged.
In @packages/wasm-sdk/src/state_transitions/tokens/mod.rs:
- Around line 163-165: Several places create intentionally unused bindings like
_data_contract = self.get_or_fetch_contract(contract_id).await?; which is less
idiomatic; replace those with a throwaway assignment using let _ =
self.get_or_fetch_contract(contract_id).await?; (i.e., use let _ = ...) for each
occurrence that currently binds an unused variable (examples: the _data_contract
uses in the tokens module and the other similar unused bindings at the other
ranges). Update each call site that invokes get_or_fetch_contract(...) (and any
other functions where the result is intentionally unused) to use let _ =
<call>.await?; so the intent is clear and the compiler warnings are avoided.
🧹 Nitpick comments (1)
packages/wasm-sdk/src/sdk.rs (1)
121-207: Reduce global mutex hold time around inner cache operations.
Consider cloning theWasmTrustedContextout of theMutex<Option<_>>and dropping the guard before callingget_known_contract/add_known_contract/remove_known_contractto minimize nested-lock scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/rs-sdk-trusted-context-provider/src/provider.rspackages/wasm-sdk/src/context_provider.rspackages/wasm-sdk/src/queries/document.rspackages/wasm-sdk/src/queries/token.rspackages/wasm-sdk/src/sdk.rspackages/wasm-sdk/src/state_transitions/documents/mod.rspackages/wasm-sdk/src/state_transitions/tokens/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/wasm-sdk/src/context_provider.rspackages/rs-sdk-trusted-context-provider/src/provider.rspackages/wasm-sdk/src/state_transitions/tokens/mod.rspackages/wasm-sdk/src/queries/token.rspackages/wasm-sdk/src/state_transitions/documents/mod.rspackages/wasm-sdk/src/sdk.rspackages/wasm-sdk/src/queries/document.rs
🧠 Learnings (8)
📚 Learning: 2024-10-03T11:51:06.980Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-03T11:51:06.980Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/wasm-sdk/src/state_transitions/tokens/mod.rspackages/wasm-sdk/src/state_transitions/documents/mod.rs
📚 Learning: 2025-11-25T13:10:38.019Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: packages/swift-sdk/SwiftExampleApp/CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:38.019Z
Learning: Applies to packages/swift-sdk/SwiftExampleApp/**/{LocalDataContractsView,DataContractParser}.swift : Use LocalDataContractsView to load data contracts from network and rely on DataContractParser to automatically parse tokens and document types
Applied to files:
packages/wasm-sdk/src/queries/token.rs
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/AI_REFERENCE.md:771-783
Timestamp: 2025-07-28T20:00:08.502Z
Learning: In packages/wasm-sdk/AI_REFERENCE.md, the documentation correctly shows the actual SDK method signatures (including identityCreate and identityTopUp with their full parameter lists), which may differ from the UI inputs shown in fixed_definitions.json. The UI may collect fewer parameters from users while handling additional requirements internally.
Applied to files:
packages/wasm-sdk/src/state_transitions/documents/mod.rspackages/wasm-sdk/src/sdk.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/wasm-sdk/src/state_transitions/documents/mod.rspackages/wasm-sdk/src/queries/document.rs
📚 Learning: 2025-07-28T20:04:48.458Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/index.html:4360-4416
Timestamp: 2025-07-28T20:04:48.458Z
Learning: In packages/wasm-sdk, the wallet helper `derive_key_from_seed_with_path` (Rust function in src/wallet/key_derivation.rs) is synchronous; its JS wrapper returns a value immediately, so `await` is unnecessary.
Applied to files:
packages/wasm-sdk/src/state_transitions/documents/mod.rs
📚 Learning: 2025-07-28T20:00:24.323Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/docs.html:2359-2383
Timestamp: 2025-07-28T20:00:24.323Z
Learning: In packages/wasm-sdk/docs.html, QuantumExplorer confirmed that placeholder private keys in documentation examples are acceptable as they are not real keys, though field name accuracy for the SDK API should still be maintained.
Applied to files:
packages/wasm-sdk/src/state_transitions/documents/mod.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use WASM bindings to connect Rust and JavaScript code
Applied to files:
packages/wasm-sdk/src/state_transitions/documents/mod.rs
📚 Learning: 2025-08-14T15:03:56.681Z
Learnt from: thephez
Repo: dashpay/platform PR: 2726
File: packages/wasm-sdk/check_documentation.py:69-76
Timestamp: 2025-08-14T15:03:56.681Z
Learning: In packages/wasm-sdk/api-definitions.json, the structure is nested by categories: { "queries": { "categoryName": { "label": "...", "queries": { "actualQueryName": {...} } } } }. The check_documentation.py script correctly iterates over categories and then accesses the nested 'queries'/'transitions' objects within each category to collect the actual query/transition names.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
🧬 Code graph analysis (4)
packages/wasm-sdk/src/context_provider.rs (1)
packages/rs-sdk-trusted-context-provider/src/provider.rs (2)
get_known_contract(210-213)remove_known_contract(217-220)
packages/rs-sdk-trusted-context-provider/src/provider.rs (1)
packages/wasm-sdk/src/context_provider.rs (2)
get_known_contract(186-188)remove_known_contract(192-194)
packages/wasm-sdk/src/sdk.rs (1)
packages/wasm-sdk/src/error.rs (1)
not_found(83-85)
packages/wasm-sdk/src/queries/document.rs (2)
packages/wasm-sdk/src/error.rs (1)
not_found(83-85)packages/js-evo-sdk/src/documents/facade.ts (1)
query(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust crates security audit
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (4)
packages/wasm-sdk/src/context_provider.rs (1)
184-194: Straight, safe delegation for contract cache accessors.
These wrappers look correct and keep the cache API surface consistent across the WASM boundary.packages/wasm-sdk/src/sdk.rs (1)
210-223: PublicremoveCachedContractexport: ensure docs / API definitions are updated.
Given the repo’s documentation validation tooling, please confirm this new exported method is reflected wherever WASM SDK APIs are enumerated.packages/wasm-sdk/src/state_transitions/documents/mod.rs (1)
7-8: Consistent switch toget_or_fetch_contractacross document transitions.
This aligns document flows with the new cache-aware contract retrieval path.Also applies to: 238-240, 570-572, 882-884, 1016-1018, 1146-1148, 1320-1322
packages/wasm-sdk/src/queries/document.rs (1)
416-427: Good cache-aware refactor for single-document queries.
The new flow (cached contract → validate doc type →DocumentQuery::new) looks correct.Also applies to: 461-472
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/wasm-sdk/src/queries/document.rs (1)
122-140:build_documents_queryis not using contract cache, inconsistent with other document methods.The
get_documentandget_document_with_proof_infomethods now useget_or_fetch_contractfor cached retrieval, butbuild_documents_query(used byget_documentsandget_documents_with_proof_info) still callsDocumentQuery::new_with_data_contract_idwhich fetches from the network without using the cache.Consider refactoring to use the cache-first approach here as well:
♻️ Suggested refactor to use cached contract retrieval
async fn build_documents_query( sdk: &WasmSdk, input: DocumentsQueryInput, ) -> Result<DocumentQuery, WasmSdkError> { let DocumentsQueryInput { data_contract_id, document_type_name, where_clauses, order_by, limit, start_after, start_at, } = input; let contract_id: Identifier = data_contract_id.into(); - let mut query = - DocumentQuery::new_with_data_contract_id(sdk.as_ref(), contract_id, &document_type_name) - .await?; + // Fetch the data contract (using cache) + let data_contract = sdk.get_or_fetch_contract(contract_id).await?; + + let mut query = DocumentQuery::new(data_contract, &document_type_name)?; query.limit = limit.unwrap_or(100);
🧹 Nitpick comments (1)
packages/wasm-sdk/src/sdk.rs (1)
173-193: Minor: Potential duplicate fetches for concurrent requests.There's a small race window between checking the cache (line 180) and caching the fetched result (line 190). If two concurrent calls request the same contract, both may fetch from the network before either caches. This is a minor performance concern—not a correctness issue—since caching is idempotent.
If this becomes problematic under load, consider using a concurrent map with entry API or a per-contract lock. However, for typical usage patterns, this implementation should be fine.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/wasm-sdk/src/queries/document.rspackages/wasm-sdk/src/sdk.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/wasm-sdk/src/queries/document.rspackages/wasm-sdk/src/sdk.rs
🧠 Learnings (3)
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
📚 Learning: 2025-08-14T15:03:56.681Z
Learnt from: thephez
Repo: dashpay/platform PR: 2726
File: packages/wasm-sdk/check_documentation.py:69-76
Timestamp: 2025-08-14T15:03:56.681Z
Learning: In packages/wasm-sdk/api-definitions.json, the structure is nested by categories: { "queries": { "categoryName": { "label": "...", "queries": { "actualQueryName": {...} } } } }. The check_documentation.py script correctly iterates over categories and then accesses the nested 'queries'/'transitions' objects within each category to collect the actual query/transition names.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/AI_REFERENCE.md:771-783
Timestamp: 2025-07-28T20:00:08.502Z
Learning: In packages/wasm-sdk/AI_REFERENCE.md, the documentation correctly shows the actual SDK method signatures (including identityCreate and identityTopUp with their full parameter lists), which may differ from the UI inputs shown in fixed_definitions.json. The UI may collect fewer parameters from users while handling additional requirements internally.
Applied to files:
packages/wasm-sdk/src/queries/document.rspackages/wasm-sdk/src/sdk.rs
🧬 Code graph analysis (2)
packages/wasm-sdk/src/queries/document.rs (2)
packages/wasm-sdk/src/error.rs (1)
not_found(83-85)packages/js-evo-sdk/src/documents/facade.ts (1)
query(13-16)
packages/wasm-sdk/src/sdk.rs (1)
packages/wasm-sdk/src/error.rs (1)
not_found(83-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Rust crates security audit
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (6)
packages/wasm-sdk/src/queries/document.rs (2)
416-426: LGTM!The cache-first retrieval pattern is well implemented. The validation step for document type existence before query construction is a good addition that provides a clearer error message.
460-470: LGTM!Consistent with the
get_documentimplementation, properly using cached contract retrieval and document type validation.packages/wasm-sdk/src/sdk.rs (4)
122-147: LGTM!Clean implementation that properly handles network-specific caching. Returning
Arc<DataContract>avoids unnecessary clones when reading from cache.
149-169: LGTM!Gracefully handles cases where the trusted context is not initialized by silently skipping the cache operation. This is appropriate since not all SDK configurations use trusted contexts.
195-225: LGTM!Consistent pattern with
get_cached_contract. The boolean return value is useful for callers to know if a contract was actually removed.
228-241: LGTM!Clean WASM binding with good documentation. The JSDoc-style comment clearly explains the purpose and return value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/wasm-sdk/src/queries/token.rs (1)
958-982: Consider potential TOCTOU window for context validity.The unwraps on lines 963-965, 970-972, and 977-979 rely on the earlier
is_some()check at lines 917-921. While the comment correctly notes the context was verified, there's a theoretical time-of-check-to-time-of-use window if another thread could clear the context between check and use.Given that the codebase only sets contexts (never clears them during normal operation), this is low risk. However, for extra safety, consider using
if let Some(ctx) = guard.as_ref()pattern here as well, which would be more defensive.♻️ Optional defensive pattern
match network { Network::Dash => { let guard = MAINNET_TRUSTED_CONTEXT.lock().unwrap(); - guard - .as_ref() - .unwrap() - .add_known_token_configuration(token_id, token_configuration); + if let Some(ctx) = guard.as_ref() { + ctx.add_known_token_configuration(token_id, token_configuration); + } } Network::Testnet => { let guard = TESTNET_TRUSTED_CONTEXT.lock().unwrap(); - guard - .as_ref() - .unwrap() - .add_known_token_configuration(token_id, token_configuration); + if let Some(ctx) = guard.as_ref() { + ctx.add_known_token_configuration(token_id, token_configuration); + } } Network::Regtest => { let guard = LOCAL_TRUSTED_CONTEXT.lock().unwrap(); - guard - .as_ref() - .unwrap() - .add_known_token_configuration(token_id, token_configuration); + if let Some(ctx) = guard.as_ref() { + ctx.add_known_token_configuration(token_id, token_configuration); + } } - _ => unreachable!(), // Already checked above + _ => {} // Already handled by early return }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/wasm-sdk/src/queries/token.rspackages/wasm-sdk/src/sdk.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/wasm-sdk/src/queries/token.rspackages/wasm-sdk/src/sdk.rs
🧠 Learnings (1)
📚 Learning: 2025-02-10T11:26:36.709Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2405
File: packages/wasm-sdk/src/verify.rs:26-68
Timestamp: 2025-02-10T11:26:36.709Z
Learning: In the wasm-sdk package, empty vectors and placeholder values are intentionally used in verification functions during the proof-of-concept stage to ensure proper compilation and type checking.
Applied to files:
packages/wasm-sdk/src/queries/token.rs
🧬 Code graph analysis (2)
packages/wasm-sdk/src/queries/token.rs (2)
packages/wasm-sdk/src/sdk.rs (1)
network(110-112)packages/wasm-sdk/src/error.rs (1)
generic(71-73)
packages/wasm-sdk/src/sdk.rs (2)
packages/wasm-sdk/src/context_provider.rs (3)
new_local(130-132)new_mainnet(102-114)new_testnet(116-128)packages/wasm-dpp2/src/identifier.rs (4)
from(23-25)from(29-31)from(35-37)from(41-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build JS packages / Build JS
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (10)
packages/wasm-sdk/src/queries/token.rs (2)
915-929: LGTM - Precondition check ensures context availability.The early validation that the trusted context is initialized before performing network fetches is a good defensive pattern. This ensures the caching infrastructure is available before attempting operations that depend on it.
944-945: Good use of the centralized caching mechanism.Replacing direct
DataContract::fetchwithget_or_fetch_contractproperly leverages the new cache-first approach, reducing redundant network calls.packages/wasm-sdk/src/sdk.rs (8)
120-147: LGTM - Clean cache retrieval implementation.The
get_cached_contractmethod properly handles all network variants and gracefully returnsNonewhen the context isn't initialized or the contract isn't cached.
149-169: LGTM - Appropriate silent caching behavior.The
cache_contractmethod correctly usesif let Some(ref context)to silently skip caching when the trusted context isn't available. This is the right approach for a cache-write operation.
171-193: LGTM - Standard cache-aside pattern implemented correctly.The
get_or_fetch_contractmethod follows the proper cache-aside pattern:
- Check cache first
- Fetch from network on cache miss
- Cache the result for future use
- Return the contract
The cloning at line 181 and 190 is necessary to satisfy ownership requirements.
195-225: LGTM - Cache invalidation support.The
remove_cached_contractmethod provides a way to force fresh fetches when needed, returning a boolean to indicate whether a contract was actually removed.
228-241: LGTM - Clean WASM binding.The WASM binding correctly converts the
IdentifierWasmto the internalIdentifiertype and delegates to the internal implementation.
466-477: LGTM - Get-or-create pattern ensures context availability.The updated
new_local_trustedproperly implements the get-or-create pattern: if a cached context exists, clone and reuse it; otherwise create a new one and store it for future use. This ensures the context is always available for subsequent caching operations.
493-506: LGTM - Consistent pattern for mainnet.The
new_mainnet_trustedfollows the same get-or-create pattern as local, ensuring consistency across network variants.
538-551: LGTM - Consistent pattern for testnet.The
new_testnet_trustedcompletes the consistent get-or-create pattern across all three network types (local, mainnet, testnet).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/wasm-sdk/src/sdk.rs (2)
418-473: Unnecessary cloning ofaddress_listin each match arm.The
address_list.clone()on lines 420, 438, and 456 is redundant since each arm consumes the value once and the match is exhaustive. You can useaddress_listdirectly.♻️ Proposed fix
Network::Dash => { - let address_list = address_list.clone(); - // Use the cached trusted context if available for mainnet let context = { // ... }?; SdkBuilder::new(address_list) .with_network(network) .with_context_provider(context) } Network::Testnet => { - let address_list = address_list.clone(); - // Use the cached trusted context if available for testnet // ... + SdkBuilder::new(address_list) + .with_network(network) + .with_context_provider(context) } Network::Regtest => { - let address_list = address_list.clone(); - // ... + SdkBuilder::new(address_list) + .with_network(network) + .with_context_provider(context) }Note: Since the match is exhaustive (with unreachable for
_), only the first arm that actually executes will consumeaddress_list, so cloning is not needed.
119-147: Consider extracting the network-matching pattern into a helper.There are 5 methods with nearly identical match arms over
Networkto access the appropriate trusted context global (add_contract_to_context_cache,get_cached_contract,cache_contract,remove_cached_contract). A helper method could reduce duplication:fn with_trusted_context<F, R>(&self, f: F) -> Option<R> where F: FnOnce(&WasmTrustedContext) -> R, { let guard = match self.network() { Network::Testnet => TESTNET_TRUSTED_CONTEXT.lock().unwrap(), Network::Dash => MAINNET_TRUSTED_CONTEXT.lock().unwrap(), Network::Regtest => LOCAL_TRUSTED_CONTEXT.lock().unwrap(), _ => return None, }; guard.as_ref().map(f) }This would simplify each method to a single call.
Also applies to: 163-212, 238-268
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/wasm-sdk/src/queries/document.rspackages/wasm-sdk/src/sdk.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/wasm-sdk/src/queries/document.rspackages/wasm-sdk/src/sdk.rs
🧠 Learnings (5)
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
📚 Learning: 2025-08-14T15:03:56.681Z
Learnt from: thephez
Repo: dashpay/platform PR: 2726
File: packages/wasm-sdk/check_documentation.py:69-76
Timestamp: 2025-08-14T15:03:56.681Z
Learning: In packages/wasm-sdk/api-definitions.json, the structure is nested by categories: { "queries": { "categoryName": { "label": "...", "queries": { "actualQueryName": {...} } } } }. The check_documentation.py script correctly iterates over categories and then accesses the nested 'queries'/'transitions' objects within each category to collect the actual query/transition names.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
📚 Learning: 2025-09-03T14:41:16.196Z
Learnt from: thephez
Repo: dashpay/platform PR: 2754
File: packages/wasm-sdk/AI_REFERENCE.md:766-766
Timestamp: 2025-09-03T14:41:16.196Z
Learning: In packages/wasm-sdk/, the AI_REFERENCE.md file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in AI_REFERENCE.md, as manual changes to AI_REFERENCE.md would be overwritten during regeneration.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
📚 Learning: 2025-09-03T14:42:29.958Z
Learnt from: thephez
Repo: dashpay/platform PR: 2754
File: packages/wasm-sdk/docs.html:1970-1971
Timestamp: 2025-09-03T14:42:29.958Z
Learning: In packages/wasm-sdk/, the docs.html file is auto-generated from api-definitions.json. Any documentation fixes should be made in api-definitions.json rather than directly in docs.html, as manual changes to docs.html would be overwritten during regeneration.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/AI_REFERENCE.md:771-783
Timestamp: 2025-07-28T20:00:08.502Z
Learning: In packages/wasm-sdk/AI_REFERENCE.md, the documentation correctly shows the actual SDK method signatures (including identityCreate and identityTopUp with their full parameter lists), which may differ from the UI inputs shown in fixed_definitions.json. The UI may collect fewer parameters from users while handling additional requirements internally.
Applied to files:
packages/wasm-sdk/src/queries/document.rs
🧬 Code graph analysis (2)
packages/wasm-sdk/src/queries/document.rs (3)
packages/js-evo-sdk/src/documents/facade.ts (1)
query(12-15)packages/js-evo-sdk/tests/unit/facades/documents.spec.mjs (2)
query(47-53)query(61-64)packages/wasm-sdk/src/error.rs (1)
not_found(83-85)
packages/wasm-sdk/src/sdk.rs (2)
packages/wasm-sdk/src/error.rs (1)
not_found(83-85)packages/wasm-sdk/src/context_provider.rs (2)
new_mainnet(102-114)new_testnet(116-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Rust crates security audit
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (9)
packages/wasm-sdk/src/sdk.rs (6)
163-190: LGTM! Cache retrieval implementation is correct.The per-network lock acquisition and optional context access pattern is appropriate. The graceful fallback to
Nonefor unsupported networks is consistent with the rest of the caching layer.
192-212: LGTM! Cache storage implementation is correct.Consistent pattern with
get_cached_contract. Silent no-op for missing context or unsupported networks is acceptable behavior.
214-236: LGTM! Cache-first fetch pattern is well-implemented.The flow correctly:
- Checks cache first to avoid network calls
- Fetches from network only when necessary
- Caches the result for future use
- Returns appropriate error for missing contracts
238-269: LGTM! Cache removal implementation is correct.Returns
falsewhen context is absent or contract wasn't cached, which is sensible behavior for the JS API.
271-284: LGTM! WASM binding correctly delegates to internal implementation.
509-520: LGTM! Store-and-reuse pattern for trusted contexts.The builder methods now correctly:
- Check if context already exists
- Create and store a new one if absent
- Return the stored/existing context
This ensures subsequent SDK builds reuse the same context and its cached contracts.
Also applies to: 537-549, 582-594
packages/wasm-sdk/src/queries/document.rs (3)
138-141: LGTM! Cache-backed contract fetch for document queries.The refactored flow correctly:
- Fetches the contract via cache (
get_or_fetch_contract)- Creates the query with the already-resolved contract
- Eliminates the redundant network fetch that occurred previously
417-426: LGTM! Proper validation before query construction.Good practice to validate that the document type exists in the contract before constructing the query. This provides clearer error messages to the caller.
461-470: LGTM! Consistent pattern withget_document.The proof-returning variant follows the same cache-first approach and validation pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/wasm-sdk/src/state_transitions/token.rs:
- Line 260: The call to get_or_fetch_contract(contract_id) can cause redundant
concurrent network fetches for the same contract_id; introduce request
deduplication by tracking in-flight fetches and awaiting a single shared future
per contract_id (e.g., an in_progress map from contract_id -> shared/oneshot
receiver or a per-key Mutex/RwLock) so that concurrent callers for the same
contract_id wait on the already-started fetch instead of issuing new network
requests; update get_or_fetch_contract to check the cache, then consult the
in_progress structure to either await an existing future or insert and perform
the fetch, and ensure removal/notification on completion or error.
🧹 Nitpick comments (1)
packages/wasm-sdk/src/state_transitions/token.rs (1)
296-301: Consider reusing the already-extracted settings.The
settingsvariable extracted at line 263 could be reused here instead of callingextract_settings_from_optionsagain. This pattern repeats in all 10 token operations.♻️ Suggested simplification
// Add user fee increase from settings - let user_fee_increase = - get_user_fee_increase(extract_settings_from_options(&options_value)?.as_ref()); + let user_fee_increase = get_user_fee_increase(settings.as_ref()); if user_fee_increase > 0 { builder = builder.with_user_fee_increase(user_fee_increase); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/wasm-sdk/src/state_transitions/document.rspackages/wasm-sdk/src/state_transitions/token.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Rust code must passcargo clippy --workspacelinter checks
Rust code must be formatted usingcargo fmt --all
**/*.rs: Use 4-space indent for Rust files
Follow rustfmt defaults and keep code clippy-clean for Rust modules
Use snake_case for Rust module names
Use PascalCase for Rust type names
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/wasm-sdk/src/state_transitions/token.rspackages/wasm-sdk/src/state_transitions/document.rs
🧠 Learnings (9)
📚 Learning: 2024-10-09T00:22:57.778Z
Learnt from: shumkov
Repo: dashpay/platform PR: 2201
File: packages/rs-platform-version/src/version/v2.rs:1186-1188
Timestamp: 2024-10-09T00:22:57.778Z
Learning: In the `IdentityTransitionVersions` structure within `packages/rs-platform-version/src/version/v2.rs`, the field `credit_withdrawal` does not need the `identity_` prefix since it is already encompassed within identity state transitions.
Applied to files:
packages/wasm-sdk/src/state_transitions/token.rspackages/wasm-sdk/src/state_transitions/document.rs
📚 Learning: 2025-01-20T16:20:59.791Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2432
File: packages/rs-dpp/src/data_contract/associated_token/token_configuration/v0/mod.rs:222-225
Timestamp: 2025-01-20T16:20:59.791Z
Learning: In the Dash Platform codebase, TokenAmount from crate::balances::credits is compatible with u64 when used for token base supply.
Applied to files:
packages/wasm-sdk/src/state_transitions/token.rs
📚 Learning: 2025-11-25T13:10:23.481Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:23.481Z
Learning: Use state transitions for updates in documents and data contracts
Applied to files:
packages/wasm-sdk/src/state_transitions/document.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
Repo: dashpay/platform PR: 2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/wasm-sdk/src/state_transitions/document.rs
📚 Learning: 2025-10-01T08:37:32.168Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2790
File: packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs:65-0
Timestamp: 2025-10-01T08:37:32.168Z
Learning: In purchase document transitions (packages/rs-drive/src/drive/document/index_uniqueness/validate_document_purchase_transition_action_uniqueness/v1/mod.rs), the `changed_data_values` field in `UniquenessOfDataRequestUpdateType::ChangedDocument` should be set to an empty BTreeSet (`Cow::Owned(BTreeSet::new())`) because purchase transitions do not modify document data properties (like "price"), only ownership and transfer metadata. An empty set signals the uniqueness validation logic to skip checking unique indexes on data properties.
Applied to files:
packages/wasm-sdk/src/state_transitions/document.rs
📚 Learning: 2025-11-25T13:10:38.019Z
Learnt from: CR
Repo: dashpay/platform PR: 0
File: packages/swift-sdk/SwiftExampleApp/CLAUDE.md:0-0
Timestamp: 2025-11-25T13:10:38.019Z
Learning: Applies to packages/swift-sdk/SwiftExampleApp/**/{LocalDataContractsView,DataContractParser}.swift : Use LocalDataContractsView to load data contracts from network and rely on DataContractParser to automatically parse tokens and document types
Applied to files:
packages/wasm-sdk/src/state_transitions/document.rs
📚 Learning: 2025-07-28T20:00:08.502Z
Learnt from: QuantumExplorer
Repo: dashpay/platform PR: 2711
File: packages/wasm-sdk/AI_REFERENCE.md:771-783
Timestamp: 2025-07-28T20:00:08.502Z
Learning: In packages/wasm-sdk/AI_REFERENCE.md, the documentation correctly shows the actual SDK method signatures (including identityCreate and identityTopUp with their full parameter lists), which may differ from the UI inputs shown in fixed_definitions.json. The UI may collect fewer parameters from users while handling additional requirements internally.
Applied to files:
packages/wasm-sdk/src/state_transitions/document.rs
📚 Learning: 2025-09-02T13:30:17.703Z
Learnt from: thephez
Repo: dashpay/platform PR: 2739
File: packages/wasm-sdk/test/ui-automation/tests/state-transitions.spec.js:1-171
Timestamp: 2025-09-02T13:30:17.703Z
Learning: In packages/wasm-sdk/index.html, state transition definitions are loaded dynamically from api-definitions.json via the loadApiDefinitions() function that fetches './api-definitions.json'. The UI doesn't have hardcoded transition definitions - instead it populates categories, types, inputs, and labels from this JSON configuration file at runtime.
Applied to files:
packages/wasm-sdk/src/state_transitions/document.rs
📚 Learning: 2025-09-02T13:30:17.703Z
Learnt from: thephez
Repo: dashpay/platform PR: 2739
File: packages/wasm-sdk/test/ui-automation/tests/state-transitions.spec.js:1-171
Timestamp: 2025-09-02T13:30:17.703Z
Learning: In packages/wasm-sdk/index.html, state transition definitions are loaded dynamically from api-definitions.json rather than being hardcoded in the HTML file. The UI loads transition categories, types, inputs, and labels from this JSON configuration file.
Applied to files:
packages/wasm-sdk/src/state_transitions/document.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci, SDK_TEST_DATA=true
) / Build Drive image - GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (7)
packages/wasm-sdk/src/state_transitions/token.rs (1)
517-517: Consistent caching pattern across all token operations.All remaining token operations (burn, transfer, freeze, unfreeze, destroy_frozen, emergency_action, claim, set_price, direct_purchase) correctly use the same
get_or_fetch_contractpattern. The implementation is uniform and error handling via?is appropriate.Also applies to: 775-775, 1009-1009, 1245-1245, 1462-1462, 1683-1683, 1916-1916, 2176-2176, 2420-2420
packages/wasm-sdk/src/state_transitions/document.rs (6)
238-239: LGTM!Consistent with the caching pattern introduced in
document_create.
380-381: LGTM!Caching pattern applied consistently. The
Arc::new(data_contract)wrap on line 388 is appropriate for the builder.
502-503: LGTM!Consistent caching pattern applied.
627-628: LGTM!Consistent caching pattern applied.
744-745: LGTM!Consistent caching pattern applied across all six document state transition operations.
133-134: LGTM! Consistent caching pattern applied across document operations.The switch to
get_or_fetch_contractis a clean improvement that centralizes contract retrieval and caching. The method properly handles the "not found" case by explicitly returningWasmSdkError::not_found("Data contract not found")when the contract doesn't exist, maintaining the same error handling semantics as the previousDataContract::fetch(...).await?.ok_or_else(...)pattern.
|
|
||
| // Fetch and cache the data contract | ||
| let data_contract = self.fetch_and_cache_token_contract(contract_id).await?; | ||
| let data_contract = self.get_or_fetch_contract(contract_id).await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'get_or_fetch_contract' packages/wasm-sdk/src/ -A 15Repository: dashpay/platform
Length of output: 30517
🏁 Script executed:
# Also search for the async function definition specifically
ast-grep --pattern $'async fn get_or_fetch_contract($_, $_) -> $_ {
$$$
}'Repository: dashpay/platform
Length of output: 42
🏁 Script executed:
# Check for synchronization primitives (Mutex, RwLock, Arc) in sdk.rs
rg -n 'Mutex|RwLock|Arc|tokio::sync' packages/wasm-sdk/src/sdk.rs | head -30Repository: dashpay/platform
Length of output: 927
Consider adding synchronization to prevent redundant concurrent fetches for the same contract.
The current get_or_fetch_contract implementation has a time-of-check-time-of-use race condition: multiple concurrent calls with the same contract_id can both miss the cache and independently fetch from the network. This creates the thundering herd scenario you identified. While the caching pattern itself is sound, concurrent calls to the same contract ID should be serialized or deduplicated to avoid redundant network requests.
🤖 Prompt for AI Agents
In @packages/wasm-sdk/src/state_transitions/token.rs at line 260, The call to
get_or_fetch_contract(contract_id) can cause redundant concurrent network
fetches for the same contract_id; introduce request deduplication by tracking
in-flight fetches and awaiting a single shared future per contract_id (e.g., an
in_progress map from contract_id -> shared/oneshot receiver or a per-key
Mutex/RwLock) so that concurrent callers for the same contract_id wait on the
already-started fetch instead of issuing new network requests; update
get_or_fetch_contract to check the cache, then consult the in_progress structure
to either await an existing future or insert and perform the fetch, and ensure
removal/notification on completion or error.
| dash_sdk::dpp::dashcore::Network::Testnet => { | ||
| let guard = TESTNET_TRUSTED_CONTEXT.lock().unwrap(); | ||
| guard | ||
| .as_ref() | ||
| .and_then(|ctx| ctx.get_known_contract(contract_id)) | ||
| } | ||
| dash_sdk::dpp::dashcore::Network::Dash => { | ||
| let guard = MAINNET_TRUSTED_CONTEXT.lock().unwrap(); | ||
| guard | ||
| .as_ref() | ||
| .and_then(|ctx| ctx.get_known_contract(contract_id)) | ||
| } | ||
| dash_sdk::dpp::dashcore::Network::Regtest => { | ||
| let guard = LOCAL_TRUSTED_CONTEXT.lock().unwrap(); | ||
| guard | ||
| .as_ref() | ||
| .and_then(|ctx| ctx.get_known_contract(contract_id)) | ||
| } |
There was a problem hiding this comment.
Shouldn't we have a method for get_trusted_context, and then just do
self.get_trusted_contest.lock().unwrap(); guard .as_ref() .and_then(|ctx| ctx.get_known_contract(contract_id))
There was a problem hiding this comment.
I have a task to get rid of these static variables for context. They create a lot of problems in wasm especailly when you are running multiple instances of SDK. Context can be just an instance what you pass into sdk. But this is topic for another PR
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
Issue being fixed or feature implemented
JS SDK methods are fetching contracts without using cache and sometimes multiple times.
What was done?
How Has This Been Tested?
With existing tests
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.