feat(sdk): refresh protocol version on SDK init (FFI/WASM/JS)#3902
feat(sdk): refresh protocol version on SDK init (FFI/WASM/JS)#3902Claudius-Maginificent wants to merge 2 commits into
Conversation
The SDK seeds its protocol version to the per-network min_protocol_version
floor at construction and only ratchets upward once a proven query returns.
Wire an eager, best-effort refresh_protocol_version() into every real
(network-backed) init path so fee-sensitive flows reserve against the
network's actual protocol version from the first request instead of the
seed floor.
Per init path:
FFI (packages/rs-sdk-ffi/src/sdk.rs):
- dash_sdk_create -> refresh on the real (non-mock) branch
- dash_sdk_create_extended -> refresh on the real (non-mock) branch
- dash_sdk_create_trusted -> refresh always (path is always real)
- dash_sdk_create_with_callbacks -> covered indirectly (delegates to
dash_sdk_create_extended)
- dash_sdk_create_handle_with_mock -> skipped (mock only, no network)
WASM (packages/wasm-sdk/src/sdk.rs):
- WasmSdkBuilder::build is now async and awaits a best-effort refresh
before returning; mock/test constructors are unaffected.
JS (packages/js-evo-sdk/src/sdk.ts):
- EvoSDK::connect awaits the now-async builder.build().
Failures never abort SDK creation: a private best_effort_refresh helper
(FFI) and the WASM build path log at warn and proceed with the floor
version. Pinned SDKs are unaffected — refresh_protocol_version is a no-op
when version updating is disabled.
<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds best-effort protocol version refresh at SDK initialization time across three SDK flavors. A new ChangesProtocol version refresh at SDK initialization
Sequence Diagram(s)sequenceDiagram
participant EvoSDK
participant WasmSdkBuilder
participant WasmSdk
participant refresh_protocol_version
EvoSDK->>WasmSdkBuilder: builder.build() [awaited]
WasmSdkBuilder->>WasmSdk: construct inner SDK
WasmSdk-->>WasmSdkBuilder: sdk instance
WasmSdkBuilder->>refresh_protocol_version: sdk.refresh_protocol_version().await
alt success
refresh_protocol_version-->>WasmSdkBuilder: updated version
else failure
refresh_protocol_version-->>WasmSdkBuilder: error
WasmSdkBuilder-->>WasmSdkBuilder: tracing::warn!, use floor version
end
WasmSdkBuilder-->>EvoSDK: Ok(WasmSdk)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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)
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 |
…eal bool best_effort_refresh now checks Sdk::is_mock() and skips the proven refresh on mock SDKs, so the FFI init paths no longer thread a redundant is_real flag. Behaviour is unchanged: real SDKs refresh, mocks skip. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
requested review from
QuantumExplorer,
lklimek and
shumkov
as code owners
|
✅ Review complete (commit 1742883) |
There was a problem hiding this comment.
Pull request overview
This PR makes network-backed SDK initialization eagerly attempt a protocol-version refresh so fee-sensitive flows don’t reserve fees against the seeded floor version before the first downstream proven query updates the SDK.
Changes:
- Make
WasmSdkBuilder::build()async and trigger a protocol-version refresh during build. - Add
Sdk::is_mock()and use it in FFI init to skip refresh for mock SDK instances. - Update JS
EvoSDK::connect()to await the now-async wasm builderbuild().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/wasm-sdk/src/sdk.rs | Makes wasm builder build() async and attempts protocol-version refresh during SDK construction. |
| packages/rs-sdk/src/sdk.rs | Adds Sdk::is_mock() predicate to allow init paths to self-skip refresh for mocks. |
| packages/rs-sdk-ffi/src/sdk.rs | Adds a best-effort init-time refresh helper and calls it from FFI create paths. |
| packages/js-evo-sdk/src/sdk.ts | Awaits the now-async wasm SDK builder build() in EvoSDK.connect(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sdk = self.inner.build().map_err(WasmSdkError::from)?; | ||
| if let Err(e) = sdk.refresh_protocol_version().await { | ||
| tracing::warn!( | ||
| error = %e, | ||
| "protocol version refresh failed on init; proceeding with floor version" | ||
| ); | ||
| } |
| } | ||
|
|
||
| pub fn build(self) -> Result<WasmSdk, WasmSdkError> { | ||
| pub async fn build(self) -> Result<WasmSdk, WasmSdkError> { |
| match runtime.block_on(sdk.refresh_protocol_version()) { | ||
| Ok(v) => debug!(protocol_version = v, "protocol version refreshed on init"), | ||
| Err(e) => warn!( | ||
| error = %e, | ||
| "protocol version refresh failed on init; proceeding with floor version" | ||
| ), | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/sdk.rs (1)
289-294:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the example to match async
build().Line 293 still shows synchronous usage (
const sdk = builder.build();) even thoughbuild()is now async. This example shouldawaitthe call to avoid misleading consumers.Suggested doc fix
-/// const sdk = builder.build(); +/// const sdk = await builder.build();🤖 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/wasm-sdk/src/sdk.rs` around lines 289 - 294, The documentation example for the `build()` method in WasmSdkBuilder shows synchronous usage with `const sdk = builder.build();` but the method is now async. Update the example code comment to properly `await` the `build()` call by changing it to `const sdk = await builder.build();` to accurately reflect the async nature of the method and prevent misleading consumers of the API.
🤖 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.
Outside diff comments:
In `@packages/wasm-sdk/src/sdk.rs`:
- Around line 289-294: The documentation example for the `build()` method in
WasmSdkBuilder shows synchronous usage with `const sdk = builder.build();` but
the method is now async. Update the example code comment to properly `await` the
`build()` call by changing it to `const sdk = await builder.build();` to
accurately reflect the async nature of the method and prevent misleading
consumers of the API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4c3451c-9a67-4c0a-a2e4-a34f1f23abe0
📒 Files selected for processing (4)
packages/js-evo-sdk/src/sdk.tspackages/rs-sdk-ffi/src/sdk.rspackages/rs-sdk/src/sdk.rspackages/wasm-sdk/src/sdk.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR cleanly wires best-effort refresh_protocol_version into rs-sdk-ffi, wasm-sdk, and js-evo-sdk init paths and adds Sdk::is_mock so mocks self-skip. Verified against HEAD 1742883. Two valid concerns: (1) the helper's outer Err arm is unreachable because refresh_protocol_version always returns Ok by design; (2) in dash_sdk_create_trusted the refresh runs synchronously before the trusted provider's quorum cache prefetch, which both blocks the FFI caller on a network round-trip the trusted path deliberately backgrounds AND means the proven refresh likely either falls back to lazy quorum fetching or fails proof verification on cold start. The non-trusted WASM/JS default path also cannot learn the network protocol version because WasmContext::get_quorum_public_key unconditionally errors, but this is a pre-existing limitation of non-trusted WASM mode rather than a regression introduced here.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 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.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/sdk.rs:507-535: Trusted-setup init blocks on network and refreshes before quorum cache prefetch
dash_sdk_create_trusted was deliberately structured to return quickly: provider_for_prefetch.update_quorum_caches() is spawned on the runtime (line 531) so the FFI caller is not blocked on a network round-trip. Inserting best_effort_refresh(&sdk, &runtime) at line 509 reverses this design intent in two ways. (1) It runs runtime.block_on(sdk.refresh_protocol_version()) synchronously before returning the handle, forcing the caller to wait on a proven getEpochsInfo round-trip. (2) Sdk::refresh_protocol_version issues a proven query whose verification path needs quorum public keys, but TrustedHttpContextProvider's quorum cache has not yet been warmed at this point — the prefetch is spawned afterward. The proven query will therefore either trigger an on-demand quorum fetch inline (defeating the prefetch design) or fall through refresh.rs's warn-and-keep-current-version branch, leaving the seed protocol version at the floor — exactly the symptom this PR aims to eliminate. Move the refresh into the spawned async block after update_quorum_caches().await completes, or await the quorum prefetch before calling best_effort_refresh.
| /// Best-effort protocol-version refresh on SDK init. | ||
| /// | ||
| /// Logs a warning and proceeds if the network is unreachable; never fails SDK creation. | ||
| fn best_effort_refresh(sdk: &Sdk, runtime: &BigStackRuntime) { | ||
| // Mock SDKs have no live network; refreshing only logs noise. | ||
| if sdk.is_mock() { | ||
| return; | ||
| } | ||
| match runtime.block_on(sdk.refresh_protocol_version()) { | ||
| Ok(v) => debug!(protocol_version = v, "protocol version refreshed on init"), | ||
| Err(e) => warn!( | ||
| error = %e, | ||
| "protocol version refresh failed on init; proceeding with floor version" | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: best_effort_refresh's Err arm is unreachable
Sdk::refresh_protocol_version (packages/rs-sdk/src/sdk/refresh.rs:73-104) already catches ExtendedEpochInfo::fetch_current errors, logs them at warn under target dash_sdk::protocol_version, then unconditionally returns Ok(self.protocol_version_number()). The outer Err(e) => warn!("protocol version refresh failed on init; ...") arm here (and the equivalent in packages/wasm-sdk/src/sdk.rs:317-322) can therefore never fire. The user-visible failure log on init is actually the inner 'proven protocol-version refresh failed; keeping current version' line, not the more descriptive outer message. Either drop the match in favor of letting the inner log speak for itself, or change refresh_protocol_version to surface the inner error so the documented outer warn actually runs.
source: ['claude']
Why this PR exists
Problem. A freshly built SDK seeds its protocol version to the per-network
min_protocol_versionfloor and only ratchets it upward once a proven query returns the network's real version. Until that first query lands, fee-sensitive flows (e.g. shielded pool shield/unshield/transfer/withdraw) reserve fees against the seed floor, not the network's actual protocol version.What breaks without it. Callers that issue a fee-sensitive operation as their very first action — before any other query has taught the SDK the real version — compute reservations against the floor. This PR makes every real init path eagerly learn the network version up front.
Blocking relationship. Compiles independently of #3900: the wiring calls the existing
Sdk::refresh_protocol_version()(present onv3.1-devwith the same signature), so it is based onv3.1-dev, not stacked atop #3900. It also adds a smallSdk::is_mock()predicate to rs-sdk that references only types already onv3.1-dev, so independence still holds. Semantically it pairs with #3900 — that PR collapses the floor to a flat PV10 baseline and drops the clamp; this PR makes that low floor safe by refreshing on init. The two sharers-sdk/src/sdk.rs, but edit non-overlapping regions (theis_mockinsertion sits where #3900 makes no change), so they merge cleanly. Intended to land together.Issue being fixed or feature implemented
Eagerly call the existing best-effort
refresh_protocol_version()on every network-backed SDK init path so the SDK learns the network's real protocol version at construction instead of on the first downstream query.What was done?
A best-effort refresh is wired into every real (network-backed) init path. Failures never abort SDK creation — they log at
warnand proceed with the floor version. Pinned SDKs are unaffected:refresh_protocol_version()is a no-op when version updating is disabled.Mock SDKs are skipped at the helper level. A new
Sdk::is_mock(&self) -> boolpredicate (added to rs-sdk; additive, non-breaking) letsbest_effort_refreshdetect a mock from theSdkitself and return early — so the FFI init paths call refresh unconditionally without threading a per-branch flag, and mock SDKs never issue a guaranteed-failing proven query.dash_sdk_createdapi_addresses)dash_sdk_create_extendeddash_sdk_create_trusteddash_sdk_create_with_callbacksdash_sdk_create_extendeddash_sdk_create_handle_with_mockWasmSdkBuilder::buildasync; awaits a best-effort refresh before returning (always network-real)EvoSDK::connectbuilder.build()A private
best_effort_refresh(sdk, runtime)helper centralizes the mock self-skip + FFIruntime.block_on(...)+ log-and-proceed pattern, reusing the same mechanism as the existingdash_sdk_refresh_protocol_versionFFI wrapper.How Has This Been Tested?
cargo build -p dash-sdkandcargo build -p dash-sdk --no-default-features— clean in both configs (the no-default-features build exercises the#[cfg(not(feature = "mocks"))]arm ofis_mock, sincemocksis in dash-sdk's default set).cargo check -p rs-sdk-ffi -p wasm-sdk— clean onv3.1-dev(this is what determined the base branch).cargo clippy -p dash-sdk -p rs-sdk-ffi --all-targets -- -D warningsandcargo clippy -p wasm-sdk -- -D warnings— clean.cargo fmt -p dash-sdk -p rs-sdk-ffi -p wasm-sdk -- --check— clean.WasmSdkBuilder::buildisasync,EvoSDK::connectisasyncand awaits it.cr_003,id_001,id_002,id_003,id_005; ~102s;--test-threads=1). The SDK seeded at PV10 and auto-ratcheted to the network's active PV12 on the first proven response, confirming the refresh-on-init path lands the client on the correct version.Breaking Changes
WasmSdkBuilder.build()is nowasync(returns aPromise<WasmSdk>). JS/TS callers mustawaitit. The in-repo caller (EvoSDK::connect) and the wasm-sdk tests already awaitbuild(), so no internal breakage; downstream JS consumers callingbuild()synchronously must addawait. The newSdk::is_mock()is purely additive (non-breaking).Related
version_pinned, drops the post-build clamp, and relocates therefresh_protocol_versionprimitive this PR calls. Intended to land together.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Chores