refactor(sdk): get rid of static trusted contexts#3043
Conversation
✅ gRPC Query Coverage Report |
📝 WalkthroughWalkthroughSwitches from global cached trusted contexts to per-instance trusted contexts: adds WasmTrustedContext prefetch factories, embeds optional trusted_context into WasmSdk and WasmSdkBuilder, replaces trusted builder variants with withTrustedContext(context), and updates SDK logic and tests to prefetch and inject contexts before building. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test / App
participant JS as WasmTrustedContext (JS/WASM)
participant Builder as WasmSdkBuilder
participant SDK as WasmSdk
Test->>JS: prefetchLocal()
JS-->>Test: WasmTrustedContext (includes discovered_addresses)
Test->>Builder: local()
Test->>Builder: withTrustedContext(context)
Builder->>Builder: configure inner SdkBuilder + trusted_context
Test->>Builder: build()
Builder-->>SDK: WasmSdk (sdk + trusted_context)
Test->>SDK: use SDK methods (reads trusted_context when needed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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
🤖 Fix all issues with AI agents
In `@packages/js-evo-sdk/src/sdk.ts`:
- Around line 93-105: The code destructures a trusted flag from this.options but
always prefetched a trusted context in connect(); update connect() to honor the
trusted option by only calling
wasm.WasmTrustedContext.prefetchMainnet()/prefetchTestnet()/prefetchLocal() when
the instance's trusted property is true (reference the trusted field set in the
constructor and the connect() function), or alternatively remove the trusted
option and associated helpers (testnetTrusted/mainnetTrusted/localTrusted) from
EvoSDKOptions and factories if you decide trusted contexts are mandatory; modify
only the connect() prefetch branch or the options/factory definitions
accordingly so the trusted flag is either used or eliminated consistently.
🧹 Nitpick comments (4)
packages/js-evo-sdk/tests/unit/facades/identities.spec.ts (2)
79-84: Minor: Inconsistent indentation in multiline stub setup.The object properties inside
.resolves({...})are not indented consistently with similar patterns elsewhere in this file (e.g., lines 101-106). The properties should be indented one level relative to the opening brace.🔧 Suggested fix for consistent indentation
getIdentityContractNonceWithProofInfoStub = this.sinon .stub(wasmSdk, 'getIdentityContractNonceWithProofInfo').resolves({ - data: BigInt(0), - proof: {}, - metadata: {}, - }); + data: BigInt(0), + proof: {}, + metadata: {}, + });
130-135: Minor: Same indentation inconsistency as above.🔧 Suggested fix for consistent indentation
getIdentityTokenBalancesWithProofInfoStub = this.sinon .stub(wasmSdk, 'getIdentityTokenBalancesWithProofInfo').resolves({ - data: new Map(), - proof: {}, - metadata: {}, - }); + data: new Map(), + proof: {}, + metadata: {}, + });packages/wasm-sdk/tests/unit/builder.spec.ts (1)
9-29: Consider adding test coverage forwithTrustedContext()method.The static methods are well tested, but the new
withTrustedContext()instance method introduced by this PR is only implicitly covered via functional tests. Adding a unit test would provide explicit validation of the API surface.🧪 Suggested test addition
it('should expose withTrustedContext method on builder instance', () => { const builder = sdk.WasmSdkBuilder.local(); expect(builder.withTrustedContext).to.be.a('function'); });packages/wasm-sdk/src/context_provider.rs (1)
106-157: Consider extracting shared prefetch logic into a helper method.
prefetch_mainnetandprefetch_testnethave nearly identical implementations, differing only in theNetworkparameter. A private helper could reduce duplication:♻️ Optional refactor
+ async fn prefetch_for_network( + network: dash_sdk::dpp::dashcore::Network, + ) -> Result<WasmTrustedContext, WasmSdkError> { + let inner = rs_sdk_trusted_context_provider::TrustedHttpContextProvider::new( + network, + None, + std::num::NonZeroUsize::new(100).unwrap(), + ) + .map_err(|e| WasmSdkError::generic(format!("Failed to create context provider: {}", e)))? + .with_refetch_if_not_found(false); + + let inner = Arc::new(inner); + + inner + .update_quorum_caches() + .await + .map_err(|e| WasmSdkError::generic(format!("Failed to prefetch quorums: {}", e)))?; + + let discovered_addresses = Self::fetch_addresses_from(&inner).await?; + + Ok(WasmTrustedContext { + inner, + discovered_addresses, + }) + } + #[wasm_bindgen(js_name = "prefetchMainnet")] pub async fn prefetch_mainnet() -> Result<WasmTrustedContext, WasmSdkError> { - // ... current implementation + Self::prefetch_for_network(dash_sdk::dpp::dashcore::Network::Dash).await }That said, keeping them explicit is also reasonable for JS-exported factory methods where clarity is valued.
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 (3)
packages/wasm-sdk/src/dpns.rs (1)
458-507:⚠️ Potential issue | 🟠 MajorReturn
undefinedto match the new JS-facing type.You updated the unchecked return types to
... | undefined, but the absence paths still returnJsValue::NULL. This contradicts the declared contract and can break consumers that rely onundefinedchecks. ReturnJsValue::UNDEFINEDin both functions.✅ Proposed fix
@@ - } else { - JsValue::NULL - }; + } else { + JsValue::UNDEFINED + }; @@ - let username = if usernames.length() > 0 { - usernames.get(0) - } else { - JsValue::NULL - }; + let username = if usernames.length() > 0 { + usernames.get(0) + } else { + JsValue::UNDEFINED + };Also applies to: 559-578
packages/wasm-sdk/src/queries/token.rs (1)
635-637:⚠️ Potential issue | 🟠 MajorReturn value inconsistent with declared TypeScript type.
The
unchecked_return_typeon line 612 declaresTokenTotalSupply | undefined, but the implementation returnsJsValue::NULLwhen no data is found. This creates a type mismatch for TypeScript consumers.Other methods in this file correctly use
JsValue::UNDEFINED(e.g., lines 822 and 868).🐛 Proposed fix
let data = supply_result .map(|supply| JsValue::from(TokenTotalSupplyWasm::new(supply.token_supply as u64))) - .unwrap_or(JsValue::NULL); + .unwrap_or(JsValue::UNDEFINED);packages/wasm-sdk/src/queries/identity.rs (1)
340-343:⚠️ Potential issue | 🟠 MajorSystematic mismatch between declared TypeScript types and returned values.
The
unchecked_return_typeannotations across this file were updated to useundefined(e.g.,Identity | undefinedon line 327), but the implementations still returnJsValue::NULL. This pattern repeats in multiple methods:
Method Type Annotation Line Returns NULL at Line get_identity_with_proof_info327 342 get_identity_nonce_with_proof_info555 573 get_identity_contract_nonce_with_proof_info605 632 get_identities_balances659 681 get_identity_balance_with_proof_info913 932 get_identities_balances_with_proof_info942 971 get_identity_balance_and_revision_with_proof_info985 1006 get_identity_by_public_key_hash_with_proof_info1016 1039 TypeScript consumers relying on these types will expect
undefinedbut receivenull, which can cause subtle bugs (e.g.,=== undefinedchecks will fail).🐛 Proposed fix (example for get_identity_with_proof_info)
let data: JsValue = match identity { Some(identity) => IdentityWasm::from(identity).into(), - None => JsValue::NULL, + None => JsValue::UNDEFINED, };Apply the same change to all affected methods listed above.
Replace 6 Lazy<Mutex<Option<T>>> statics with instance-owned state. Introduce WasmTrustedQuorumCache as an explicit JS-exported object for prefetching and sharing trusted quorum data. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
f93f3d8 to
7368973
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/js-evo-sdk/src/sdk.ts`:
- Around line 93-122: The connect logic attaches a trusted context
unconditionally which causes wasm.WasmSdkBuilder.withTrustedContext(context) to
override caller-provided addresses from
wasm.WasmSdkBuilder.withAddresses(addresses, network); change the flow in the
connect method so that if options.addresses is provided (addresses &&
addresses.length > 0) you do NOT prefetch/attach the trusted context (skip
calling wasm.WasmTrustedContext.prefetch* and builder.withTrustedContext),
otherwise keep the existing prefetch and withTrustedContext behavior; reference
the symbols WasmTrustedContext.prefetchMainnet/prefetchTestnet/prefetchLocal,
WasmSdkBuilder.withAddresses, and withTrustedContext to locate and update the
code.
Remove `bincode` from simple-signer (used via dpp re-export, not directly) and `once_cell` from platform-version (not used at all). https://claude.ai/code/session_01GzTJ1YK7pskXNs2ipADL9X
github-project-automation
Bot
moved this from In review / testing
to Done
in Platform team
3 participants
Issue being fixed or feature implemented
The wasm-sdk used 6
Lazy<Mutex<Option<T>>>static globals for managing trusted context and discovered masternode addresses. This shared mutable state made the SDK harder to reason about, prevented multiple SDK instances with different configurations, and coupled context lifetime to the process rather than the SDK instance.What was done?
Replaced all 6 static globals with instance-owned state by introducing
WasmTrustedContextas a JS-exported object with async prefetch factories:Rust changes:
WasmTrustedContextnow holdsArc<TrustedHttpContextProvider>+Vec<Address>withprefetchMainnet(),prefetchTestnet(),prefetchLocal(),prefetchLocalWithUrl()async factory methodsWasmSdkBuildergainedwithTrustedContext(context)that sets the context provider and replaces addresses with discovered onesWasmSdkis now a named struct withtrusted_context: Option<WasmTrustedContext>for contract/token cache accessprefetchTrustedQuorums*static methods fromWasmSdk*Trusted()builder methods (replaced bybuilder.local().withTrustedContext(context))Lazy<Mutex<Option<T>>>statics and theironce_cell/std::sync::Muteximportswith_address_list()toSdkBuilderin rs-sdkprefetch_token_configurationinqueries/token.rsto useself.trusted_contextdirectlyJS changes:
EvoSDK.connect()updated to prefetchWasmTrustedContextand pass it viawithTrustedContext()New JS API:
How Has This Been Tested?
cargo fmt --all— cleancargo clippy -p wasm-sdk -p dash-sdk— clean./build.sh) — successBreaking Changes
WasmSdk.prefetchTrustedQuorumsMainnet/Testnet/Local()removed — useWasmTrustedContext.prefetchMainnet/Testnet/Local()insteadWasmSdkBuilder.mainnetTrusted/testnetTrusted/localTrusted()removed — useWasmSdkBuilder.mainnet/testnet/local().withTrustedContext(context)insteadWasmSdkBuilder.withAddresses()no longer accepts an optional cache parameterChecklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests