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 3 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>
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 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 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 6b3e934) |
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']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The cumulative PR cleanly wires a best-effort proven protocol-version refresh into FFI/WASM/JS init paths and adds the supporting Sdk::is_mock() predicate. One new blocking issue: making WasmSdkBuilder::build async breaks three pre-existing Rust unit tests in the same file that still call .build().expect(...) synchronously — cargo check -p wasm-sdk --tests fails with three E0599 errors. Two prior nitpicks (unreachable outer Err arm, default WASM builders unable to actually verify the refresh) and the prior suggestion about trusted setup losing its previously non-blocking startup remain valid against HEAD and are carried forward.
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
4 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/wasm-sdk/src/sdk.rs`:
- [BLOCKING] packages/wasm-sdk/src/sdk.rs:542-617: Async `WasmSdkBuilder::build` breaks in-crate unit tests that still call `.build().expect(...)` synchronously
This PR flips `WasmSdkBuilder::build` from sync to `async`, so it now returns `impl Future<Output = Result<WasmSdk, WasmSdkError>>`. Three pre-existing `#[test]` functions in the same file were not updated and still call `.build().expect("...")`:
- `with_trusted_context_preserves_user_addresses` (line 550-551)
- `with_trusted_context_replaces_preset_addresses` (line 590-591)
- `with_trusted_context_no_discovered_keeps_existing_addresses` (line 615-616)
Verified locally on HEAD (6b3e934b): `cargo check -p wasm-sdk --tests` fails with three `error[E0599]: no method named 'expect' found for opaque type 'impl Future<Output = ...>'`. The author's earlier `cargo check -p wasm-sdk` and `cargo build -p wasm-sdk` passed because they only build the library target; CI / `cargo test -p wasm-sdk` / `cargo clippy --all-targets` will fail after merge. The tests landed on the branch via the merge from `v3.1-dev`, which is why they were not visible in the initial async-build commit.
Fix: switch each affected test to an async test attribute (`#[tokio::test]`) and `await` the build call before `.expect(...)`, e.g. `.build().await.expect("...")`. Pinning a protocol version (`.with_version(PlatformVersion::latest().protocol_version)`) is optional and would short-circuit the refresh round-trip for purely structural address-list assertions.
- [SUGGESTION] packages/wasm-sdk/src/sdk.rs:336-348: Default (non-trusted) WASM/JS builds cannot actually refresh the protocol version
`WasmSdkBuilder::{new_mainnet,new_testnet,new_local,new_devnet,new_with_addresses}` install `WasmContext` as the context provider unless the caller later attaches a `WasmTrustedContext` via `withTrustedContext`. `WasmContext::get_quorum_public_key` unconditionally returns an error (`context_provider.rs:31-41`), so the proven `getEpochsInfo` query that `refresh_protocol_version` issues cannot verify quorum signatures and will always fail. The new `build()` then just logs `"protocol version refresh failed on init; proceeding with floor version"` and returns — so default WASM/JS builds advertise refresh-on-init but never actually leave the floor version. Either document that this code path is a no-op without `withTrustedContext`, or short-circuit the refresh (and skip the warning) when a non-trusted `WasmContext` is in use so the log is not misleading.
In `packages/rs-sdk-ffi/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/sdk.rs:507-535: Trusted-setup init now blocks on a network round-trip before the previously-async quorum prefetch
`dash_sdk_create_trusted` was deliberately structured so that `provider_for_prefetch.update_quorum_caches()` runs on `runtime.spawn(async move { … })` (line 515-535), letting the FFI caller get its SDK handle back without waiting on a network round-trip. Inserting `best_effort_refresh(&sdk, &runtime)` at line 509 — which calls `runtime.block_on(sdk.refresh_protocol_version())` — undoes that property: trusted-setup init now synchronously waits for a proven `getEpochsInfo` query against the seeded address list before returning, bounded only by the SDK's default request timeouts (multi-second on unreachable evonodes). Embedders that previously got a fast handle now see init latency climb on flaky networks, with no inner deadline available to opt out.
Consider either (a) wrapping the refresh in `tokio::time::timeout(<small>, …)` so a misconfigured or unreachable network degrades within a known budget, or (b) spawning the refresh on the runtime so it runs alongside the existing quorum prefetch instead of in front of it (mirroring the original non-blocking design). Either path preserves the proven ratchet's correctness while keeping trusted-setup init fast.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3902 +/- ##
=============================================
- Coverage 72.78% 52.54% -20.25%
=============================================
Files 22 11 -11
Lines 3054 1707 -1347
=============================================
- Hits 2223 897 -1326
+ Misses 831 810 -21
🚀 New features to boost your workflow:
|
|
This solution is not correct. Depending on configuration, proof verification can require SPV to be synced. This means code added in this PR will always fail. Closing. |
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