feat(sdk): auto-detect protocol version from network response metadata#3483
feat(sdk): auto-detect protocol version from network response metadata#3483
Conversation
Store protocol version as Arc<AtomicU32> on Sdk (shared between clones). On each gRPC response, compare metadata.protocol_version against stored value and atomically update if newer and known to this binary. Unknown versions are logged at WARN and ignored. Downgrades and zero values are silently skipped. Closes #3410 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nc tests Cover the two remaining test cases for protocol version auto-detection: concurrent thread races converge to highest version, and PlatformVersionCurrentVersion::get_current() is synced after update. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Relax test_global_dpp_version_synced_after_update assertion to account for concurrent test execution — global PlatformVersion is process-wide, so parallel tests may push it higher. Assert >= instead of ==. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSdk now tracks protocol version as a shared Changes
Sequence DiagramsequenceDiagram
participant Client as Client/App
participant SDK as Sdk
participant Meta as verify_response_metadata()
participant PV as PlatformVersion
participant Global as PlatformVersion::current
Client->>SDK: response with metadata (protocol_version)
SDK->>Meta: verify_response_metadata(metadata)
Meta->>PV: PlatformVersion::get(metadata.protocol_version)
alt valid & higher
PV-->>Meta: &'static PlatformVersion
Meta->>SDK: maybe_update_protocol_version(new_u32)
activate SDK
loop CAS loop
SDK->>SDK: load current_u32
alt new_u32 > current_u32
SDK->>SDK: atomic_compare_exchange(current_u32, new_u32)
else
SDK-->>SDK: stop (no update)
end
end
SDK->>Global: PlatformVersion::set_current(new_u32)
deactivate SDK
else invalid/zero/lower
Meta-->>SDK: ignore update
end
Client->>SDK: version() / protocol_version_number()
SDK-->>Client: resolved PlatformVersion or u32
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
Review GateCommit:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 285-309: The CAS loop can win but then race when calling
PlatformVersion::set_current(received_version), possibly overwriting a higher
version set by another thread; after the CAS success in the loop (where
protocol_version.compare_exchange_weak returns Ok(_)), only call
PlatformVersion::set_current(received_version) if PlatformVersion::get_current()
< received_version to avoid downgrades, and add a brief comment by the
protocol_version compare_exchange_weak block explaining the use of relaxed
ordering here and whether it should match the SeqCst success ordering used in
verify_metadata_height (or change the success ordering to SeqCst if stronger
ordering is required).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a98f844-d431-4f7f-a1ef-0be6a496c982
📒 Files selected for processing (1)
packages/rs-sdk/src/sdk.rs
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "f980728866198d8d690e992397cd5f3952c05780e0e001e8b2d189ec637bdeb2"
)Xcode manual integration:
|
SdkBuilder::with_version() now sets auto_detect_protocol_version=false, preventing the SDK from overriding a user-pinned version with network metadata. Default behavior (no with_version() call) still auto-detects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/sdk.rs (1)
1213-1224: Tests assume protocol versions 1, 2, 3 exist.The helper and tests directly use version numbers 1, 2, 3 assuming they're valid
PlatformVersionvalues. If these versions are ever removed from the platform version registry, these tests would fail with potentially confusing errors (theversion()fallback tolatest()would cause assertion mismatches).Consider using
PlatformVersion::first()andPlatformVersion::latest()for boundary cases, or adding a brief comment noting this assumption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/sdk.rs` around lines 1213 - 1224, The helper mock_sdk_with_auto_detect currently hardcodes numeric protocol versions (e.g., 1,2,3) which can break if registry changes; update tests to use PlatformVersion::first() and PlatformVersion::latest() (or other PlatformVersion accessors) when setting sdk.protocol_version.store(...) via SdkBuilder::new_mock() so boundaries are robust, or at minimum add a comment in mock_sdk_with_auto_detect documenting the assumption that numeric versions map to existing PlatformVersion entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 1213-1224: The helper mock_sdk_with_auto_detect currently
hardcodes numeric protocol versions (e.g., 1,2,3) which can break if registry
changes; update tests to use PlatformVersion::first() and
PlatformVersion::latest() (or other PlatformVersion accessors) when setting
sdk.protocol_version.store(...) via SdkBuilder::new_mock() so boundaries are
robust, or at minimum add a comment in mock_sdk_with_auto_detect documenting the
assumption that numeric versions map to existing PlatformVersion entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c0112be-34e5-4de5-8d29-cb028461c3e6
📒 Files selected for processing (1)
packages/rs-sdk/src/sdk.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I validated this PR directly from the checked-out diff because five reviewer lanes and the verifier ACP lane all stalled during startup. The explicit-version pinning behavior in this head looks intentional, so I dropped the weaker API-compatibility concern from the one completed agent lane. The real remaining issue is that the default auto-detect path still seeds itself with PlatformVersion::latest() and only accepts increases, which means it can never learn that the connected network is running an older protocol version.
Reviewed commit: 2624d3a
🔴 1 blocking
🤖 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/src/sdk.rs`:
- [BLOCKING] lines 968-969: Default-built SDK can never auto-detect an older network protocol version
Auto-detect is enabled by default, but the runtime still seeds `protocol_version` from `PlatformVersion::latest()` here (and likewise in the mock builder at line 1036). `maybe_update_protocol_version()` then returns early for every `received_version <= current` (lines 276-280). So a normal SDK created without `with_version()` starts at the newest protocol this binary knows about and will ignore metadata from any older-but-still-supported network forever. That defeats the point of #3410 / this PR family: when the network has not activated the latest protocol yet, the SDK keeps serializing/parsing with the too-new version instead of converging to the network version. The new tests miss this because they mutate the atomic to `1` via a helper before calling `verify_response_metadata()`, so they never exercise the real default-builder path.
…loop Replace compare_exchange_weak loop with a single fetch_max atomic operation. Versions are monotonically increasing, so fetch_max is sufficient and much simpler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The global CURRENT_PLATFORM_VERSION (process-wide RwLock) is not read by any SDK runtime code path — all SDK code passes &PlatformVersion explicitly. Removing set_current() makes multi-SDK scenarios clean: each instance tracks its own version independently. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… network version Default-built SDK was seeded at PlatformVersion::latest(), so it could never detect an older network version. Now seeds at 0 (uninitialized) when auto-detect is enabled. version() returns latest() as fallback until the first network response arrives and sets the real version. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The received_version == 0 early return already guarantees received_version >= 1, so received_version <= 0 can never be true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The auto-detection machinery is directionally good, but the current PR still has one concrete regression and one important bootstrap limitation. The regression is definite and reproducible: removing the SDK's global PlatformVersion::set_current() initialization breaks existing mock/dump consumers that still call PlatformVersion::get_current(). Separately, the first proof-based request on a fresh auto-detect SDK still parses with the latest() fallback before response metadata can teach the SDK the real network version.
Reviewed commit: b01969d
🔴 1 blocking | 🟡 1 suggestion(s)
2 additional findings
🔴 blocking: Removing global platform-version initialization breaks existing mock and dump paths
packages/rs-sdk/src/sdk.rs (lines 923-927)
SdkBuilder::build() no longer initializes the process-wide current platform version, but code that is still live in this tree continues to depend on that global being set. This is easy to reproduce on the checked-out head: cargo test -p dash-sdk --test main fetch::mock_fetch::test_mock_fetch_data_contract -- --exact --nocapture now fails with CurrentProtocolVersionNotInitialized from packages/rs-sdk/tests/fetch/common.rs:33, because mock_document_type() still unwraps PlatformVersion::get_current(). The same root cause also affects the dump path: GrpcContextProvider::dump_data_contract() serializes a fetched contract with serde_json::to_vec(data_contract), and impl Serialize for DataContract still calls PlatformVersion::get_current(). So dropping the only initialization site is not just dead-code cleanup — it is a behavior regression in existing supported SDK workflows that must be fixed before merge.
🟡 suggestion: Auto-detect still cannot learn the network version until after the first proof parse succeeds
packages/rs-sdk/src/sdk.rs (lines 326-342)
The new auto-detect path updates protocol_version only inside verify_response_metadata(), but parse_proof_with_metadata_and_proof() calls O::maybe_from_proof_with_metadata(..., self.version(), ...) first and only processes metadata.protocol_version afterward. On a fresh SDK, version() still falls back to PlatformVersion::latest(), so the first proof-verified response from an older network is decoded and verified with the fallback version before the SDK learns the real one. If proof interpretation differs across versions, that first request can fail before verify_response_metadata() runs, leaving the SDK stuck at the fallback. This is an inherited limitation rather than a new regression, but it means the PR does not fully deliver the "correctly handles older protocol versions" behavior its description promises. A follow-up should either bootstrap the version before version-sensitive proof parsing or narrow the docs/tests to make the one-request lag explicit.
🤖 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/src/sdk.rs`:
- [BLOCKING] lines 923-927: Removing global platform-version initialization breaks existing mock and dump paths
`SdkBuilder::build()` no longer initializes the process-wide current platform version, but code that is still live in this tree continues to depend on that global being set. This is easy to reproduce on the checked-out head: `cargo test -p dash-sdk --test main fetch::mock_fetch::test_mock_fetch_data_contract -- --exact --nocapture` now fails with `CurrentProtocolVersionNotInitialized` from `packages/rs-sdk/tests/fetch/common.rs:33`, because `mock_document_type()` still unwraps `PlatformVersion::get_current()`. The same root cause also affects the dump path: `GrpcContextProvider::dump_data_contract()` serializes a fetched contract with `serde_json::to_vec(data_contract)`, and `impl Serialize for DataContract` still calls `PlatformVersion::get_current()`. So dropping the only initialization site is not just dead-code cleanup — it is a behavior regression in existing supported SDK workflows that must be fixed before merge.
- [SUGGESTION] lines 326-342: Auto-detect still cannot learn the network version until after the first proof parse succeeds
The new auto-detect path updates `protocol_version` only inside `verify_response_metadata()`, but `parse_proof_with_metadata_and_proof()` calls `O::maybe_from_proof_with_metadata(..., self.version(), ...)` first and only processes `metadata.protocol_version` afterward. On a fresh SDK, `version()` still falls back to `PlatformVersion::latest()`, so the first proof-verified response from an older network is decoded and verified with the fallback version before the SDK learns the real one. If proof interpretation differs across versions, that first request can fail before `verify_response_metadata()` runs, leaving the SDK stuck at the fallback. This is an inherited limitation rather than a new regression, but it means the PR does not fully deliver the "correctly handles older protocol versions" behavior its description promises. A follow-up should either bootstrap the version before version-sensitive proof parsing or narrow the docs/tests to make the one-request lag explicit.
…ocument first-request version-lag `SdkBuilder::build()` no longer calls `PlatformVersion::set_current()`, so `mock_document_type()` was panicking on `get_current().unwrap()`. Switch to `PlatformVersion::latest()` directly (consistent with `mock_data_contract()`). Also documents the known bootstrap limitation in `parse_proof_with_metadata_and_proof`: the first request on a fresh auto-detect SDK parses proofs with `PlatformVersion::latest()` fallback before the real network version is learned from response metadata. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressing the two findings from the latest review (commit 🔴 BLOCKING —
🟡 SUGGESTION — auto-detect learns version after first proof parse Acknowledged as a known bootstrap limitation. Added a doc comment to 🤖 Co-authored by Claudius the Magnificent AI Agent |
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/rs-sdk/src/sdk.rs (1)
945-973:⚠️ Potential issue | 🟠 MajorDon't remove process-wide version initialization until the remaining
get_current()serde path is migrated.
build()no longer initializes the global platform version, butpackages/rs-dpp/src/data_contract/v0/serialization/mod.rs:1-20still deserializes viaPlatformVersion::get_current(), and that API explicitly errors when nothing has calledset_current()yet (packages/rs-dpp/src/version/mod.rs:16-46). Any SDK flow that still reaches that deserializer will now fail withCurrentProtocolVersionNotInitialized.Also applies to: 1039-1043
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/sdk.rs` around lines 945 - 973, The SDK build removed process-wide platform version initialization which breaks deserializers that call PlatformVersion::get_current() and expect set_current() to have been called; restore a temporary initialization by calling PlatformVersion::set_current(self.version.protocol_version) from the Sdk build path (when self.version_explicit is true, matching how protocol_version/auto_detect_protocol_version are set) so code paths hitting the deserializer won't trigger CurrentProtocolVersionNotInitialized; apply the same fix in the other build branch mentioned (the similar block around the other address-handling branch).
🧹 Nitpick comments (1)
packages/rs-sdk/tests/fetch/common.rs (1)
26-33: Pass the helper's platform version in instead of hardcodinglatest().The fetch tests in
packages/rs-sdk/tests/fetch/mock_fetch.rs:111-149andpackages/rs-sdk/tests/fetch/mock_fetch_many.rs:17-60pairmock_document_type()withsdk.version()when generating documents. Freezing this helper atPlatformVersion::latest()makes it drift as soon as a test useswith_version()or an SDK that has already auto-detected an older protocol. Taking&PlatformVersionor&Sdkhere would keep the helper aligned with the version under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/tests/fetch/common.rs` around lines 26 - 33, The helper mock_document_type() currently hardcodes PlatformVersion::latest(); change its signature to accept a &PlatformVersion (or &Sdk) and use that passed-in version instead of calling PlatformVersion::latest() so the generated DocumentType aligns with the test's SDK/version; update all call sites (e.g., in tests at mock_fetch.rs and mock_fetch_many.rs where mock_document_type() is paired with sdk.version() or with_version()) to pass sdk.version() or the appropriate &PlatformVersion into mock_document_type().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 945-973: The SDK build removed process-wide platform version
initialization which breaks deserializers that call
PlatformVersion::get_current() and expect set_current() to have been called;
restore a temporary initialization by calling
PlatformVersion::set_current(self.version.protocol_version) from the Sdk build
path (when self.version_explicit is true, matching how
protocol_version/auto_detect_protocol_version are set) so code paths hitting the
deserializer won't trigger CurrentProtocolVersionNotInitialized; apply the same
fix in the other build branch mentioned (the similar block around the other
address-handling branch).
---
Nitpick comments:
In `@packages/rs-sdk/tests/fetch/common.rs`:
- Around line 26-33: The helper mock_document_type() currently hardcodes
PlatformVersion::latest(); change its signature to accept a &PlatformVersion (or
&Sdk) and use that passed-in version instead of calling
PlatformVersion::latest() so the generated DocumentType aligns with the test's
SDK/version; update all call sites (e.g., in tests at mock_fetch.rs and
mock_fetch_many.rs where mock_document_type() is paired with sdk.version() or
with_version()) to pass sdk.version() or the appropriate &PlatformVersion into
mock_document_type().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e74100ba-c8e6-4787-aa2e-008ca1fcf56f
📒 Files selected for processing (2)
packages/rs-sdk/src/sdk.rspackages/rs-sdk/tests/fetch/common.rs
Removing the set_current() call broke DPP deserialization paths that read the process-wide current platform version via get_current() (e.g. DataContract serde, document query mock data loading). Restore it at the top of build() so the global is always initialized before any serialization or mock expectation loading occurs. In explicit-version mode this sets the pinned version; in auto-detect mode it sets PlatformVersion::latest() — the same fallback Sdk::version() returns before the first network response arrives. Fixes: fetch::document tests failing with "current platform version not initialized" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…erde impls Replace all `get_current()` calls in DataContract/DataContractV0/DataContractV1 serde impls with `get_version_or_current_or_latest(None)`, which falls back to `PlatformVersion::latest()` when no global version is set. This removes the dependency on `set_current()` being called before serde operations, allowing the SDK to drop that call from `SdkBuilder::build()`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3483 +/- ##
============================================
+ Coverage 84.79% 84.83% +0.03%
============================================
Files 2476 2476
Lines 267709 267733 +24
============================================
+ Hits 227000 227123 +123
+ Misses 40709 40610 -99
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds per-SDK protocol version tracking that auto-detects the network protocol version from response metadata, removing reliance on the process-wide DPP “current platform version” global during (de)serialization.
Changes:
rs-sdk: Store a per-instance (shared-between-clones)AtomicU32protocol version, update it fromResponseMetadata.protocol_version, and makeSdk::version()derive from it (with an auto-detect disable switch whenwith_version()is used).rs-dpp: Update DataContract serde paths to usePlatformVersion::get_version_or_current_or_latest(None)instead of requiringPlatformVersion::get_current().- Tests: Update fetch test helpers to avoid
set_current()and add SDK unit tests covering version update behavior (including concurrency).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/rs-sdk/tests/fetch/common.rs | Test helper now uses PlatformVersion::latest() directly instead of relying on a global current version. |
| packages/rs-sdk/src/sdk.rs | Implements per-instance protocol version auto-detection, updates metadata verification flow, and adds unit tests for version update behavior. |
| packages/rs-dpp/src/data_contract/v1/serialization/mod.rs | Switches DataContractV1 deserialization to version-or-current-or-latest lookup. |
| packages/rs-dpp/src/data_contract/v0/serialization/mod.rs | Switches DataContractV0 deserialization to version-or-current-or-latest lookup. |
| packages/rs-dpp/src/data_contract/conversion/serde/mod.rs | Switches DataContract serde to version-or-current-or-latest lookup for both serialize/deserialize. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let current_version = PlatformVersion::get_version_or_current_or_latest(None) | ||
| .map_err(|e| serde::ser::Error::custom(e.to_string()))?; |
There was a problem hiding this comment.
In Serialize, mapping the platform-version resolution error to e.to_string() drops useful context about what failed (platform version resolution vs contract conversion). Consider wrapping the error with a brief prefix so debugging serde failures is easier.
| let current_version = PlatformVersion::get_version_or_current_or_latest(None) | |
| .map_err(|e| serde::ser::Error::custom(e.to_string()))?; | |
| let current_version = PlatformVersion::get_version_or_current_or_latest(None).map_err( | |
| |e| { | |
| serde::ser::Error::custom(format!( | |
| "failed to resolve platform version for data contract serialization: {}", | |
| e | |
| )) | |
| }, | |
| )?; |
| let current_version = PlatformVersion::get_version_or_current_or_latest(None) | ||
| .map_err(|e| serde::de::Error::custom(e.to_string()))?; |
There was a problem hiding this comment.
In Deserialize, the new serde::de::Error::custom(e.to_string()) loses the prior contextual message about failing to resolve the platform version. Please add a short context prefix (e.g., "failed to resolve platform version for DataContract deserialization") to make serde errors more diagnosable.
| let current_version = PlatformVersion::get_version_or_current_or_latest(None) | |
| .map_err(|e| serde::de::Error::custom(e.to_string()))?; | |
| let current_version = PlatformVersion::get_version_or_current_or_latest(None).map_err( | |
| |e| { | |
| serde::de::Error::custom(format!( | |
| "failed to resolve platform version for DataContract deserialization: {}", | |
| e | |
| )) | |
| }, | |
| )?; |
| verify_metadata_time(metadata, now, time_tolerance)?; | ||
| }; | ||
|
|
||
| self.maybe_update_protocol_version(metadata.protocol_version); |
There was a problem hiding this comment.
Calling maybe_update_protocol_version here means verify_response_metadata mutates SDK state based on the response. Please update the doc comment on verify_response_metadata to explicitly mention this side effect (and that it’s disabled when with_version() is used), since the name currently implies verification-only behavior.
| PlatformVersion::get(v).unwrap_or_else(|_| PlatformVersion::latest()) | ||
| } | ||
|
|
||
| /// Return the raw protocol version number currently used by this SDK. |
There was a problem hiding this comment.
protocol_version_number() can return 0 when auto-detect is enabled and no network response has been processed yet (uninitialized sentinel). The current doc comment says it returns the version "currently used" which is ambiguous—please document the 0/uninitialized behavior so downstream code doesn’t treat 0 as a real protocol version.
| /// Return the raw protocol version number currently used by this SDK. | |
| /// Return the raw stored protocol version number for this SDK. | |
| /// | |
| /// When auto-detection is enabled (default) and no network response has been | |
| /// processed yet, this returns `0`. In that case, `0` is an uninitialized | |
| /// sentinel meaning the protocol version has not been detected yet, not a real | |
| /// protocol version. | |
| /// | |
| /// When pinned via [`SdkBuilder::with_version()`], this always returns the | |
| /// pinned protocol version number. | |
| /// | |
| /// Call [`Self::version`] if you need a [`PlatformVersion`] value with the SDK's | |
| /// fallback behavior applied. |
| /// When auto-detection is enabled (default), returns [`PlatformVersion::latest()`] | ||
| /// until the first network response is received, then tracks the network's version. | ||
| /// When pinned via [`SdkBuilder::with_version()`], always returns the pinned version. | ||
| pub fn version<'v>(&self) -> &'v PlatformVersion { |
There was a problem hiding this comment.
The signature pub fn version<'v>(&self) -> &'v PlatformVersion uses an unconstrained lifetime parameter, which is unusual and makes it look like the returned reference can outlive &self arbitrarily. Since this function always returns a &'static PlatformVersion (from PlatformVersion::get / latest), consider changing the signature to -> &'static PlatformVersion (or simply -> &PlatformVersion with elided lifetime) to make the API clearer.
| pub fn version<'v>(&self) -> &'v PlatformVersion { | |
| pub fn version(&self) -> &'static PlatformVersion { |
…ContractV1 Exercise the serde Deserialize impls (which call get_version_or_current_or_latest) via serde_json roundtrip, increasing patch coverage for the DPP serde change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The auto-detect protocol version feature introduced in #3483 updated the cached version only *after* a proof-backed response was successfully parsed. On a fresh auto-detect SDK the first parse therefore used PlatformVersion::latest() as a fallback, so on an older network whose proof interpretation differs from latest() the very first request could fail before the SDK ever got a chance to learn the correct version. Close that bootstrap hole by running a one-shot unproved RPC (CurrentQuorumsInfo) the first time parse_proof_with_metadata_and_proof is invoked, reading metadata.protocol_version from the response, and updating the SDK's cached version before the proof parse runs. A tokio::sync::OnceCell guarantees the bootstrap RPC runs at most once per SDK (and its clones) even under concurrent first calls. Skipped for pinned SDKs (with_version), mock SDKs, and any SDK that has already seen a response. If the bootstrap RPC itself fails the SDK logs a warning and falls back to the previous latest() behaviour so partially-reachable networks still function. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Auto-detect the protocol version from network response metadata so the SDK stays in sync with the actual network version without requiring manual configuration.
Changes
Sdk: addsAtomicU32 protocol_versionfield (per-instance, not process-wide);version()reads it;maybe_update_protocol_version()updates it viafetch_maxon every responseSdkBuilder:with_version()pins the version and disables auto-detect; without it, starts atlatest()and updates on first network responseverify_response_metadata(): callsmaybe_update_protocol_version()after proof verificationrs-dppDataContract serde: replaced allPlatformVersion::get_current()calls withget_version_or_current_or_latest(None)— falls back tolatest()when no global is set, removing the dependency onset_current()being called before deserializationSdkBuilder::build(): removedPlatformVersion::set_current()— DPP serde no longer requires itKnown limitation
The very first request uses
PlatformVersion::latest()as the version for proof parsing. The network version is learned from the response metadata of that same request, so it cannot be applied until the second request. This is documented inparse_proof_with_metadata_and_proof().Test plan
test_explicit_version_disables_auto_detect,test_default_sdk_auto_detects_version,test_auto_detect_disabled_when_version_explicitly_setset_current()being called (DataContract serde usesget_version_or_current_or_latest)🤖 Co-authored by Claudius the Magnificent AI Agent