refactor(sdk): per-network protocol-version floor + version_pinned unification#3900
refactor(sdk): per-network protocol-version floor + version_pinned unification#3900Claudius-Maginificent wants to merge 17 commits into
Conversation
…e initial protocol version floor Replace the per-network min_protocol_version (mainnet 11, testnet/devnet/regtest 12) and its build()/post-refresh clamps with a single cross-network initial protocol version floor of 11 (DEFAULT_INITIAL_PROTOCOL_VERSION = PROTOCOL_VERSION_11). The initial version is now the ratchet floor; refresh_protocol_version() still ratchets up as the network's proven version is observed. Deliberate trade-off: non-mainnet networks (live on PV 12) now seed at 11 and rely on the proven-query refresh to climb to 12; until a refresh succeeds they sit one version low. Also resolves the v3.1-dev CI failure in dash-sdk's sdk_builder_default_seeds_atomic_to_floor caused by the #3809/#3886/#3893 protocol-version interaction. Co-Authored-By: Claude Opus 4.8 (1M context) <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:
📝 WalkthroughWalkthroughThe PR refactors protocol-version floor logic by converting ChangesProtocol-Version Floor Rework
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
…inline refresh, drop redundant runtime clamp Addresses both PR #3900 review comments. Part A: restore the per-network `const fn min_protocol_version` (mainnet → 11; testnet/devnet/regtest → 12) as the single source of the build() floor, replacing `DEFAULT_INITIAL_PROTOCOL_VERSION`. build() now floors via `version.protocol_version.max(min_protocol_version(self.network))`. The `DEFAULT_INITIAL_PROTOCOL_VERSION` const is removed; no `MIN_PROTOCOL_VERSION` const is introduced. All references (code, tests, docs) repoint to `min_protocol_version(...)` or the right per-network value. Part B: inline `refresh_protocol_version` into an `impl Sdk` block in sdk.rs (~10 lines) and delete `packages/rs-sdk/src/sdk/refresh.rs` plus its `mod refresh;` declaration. `ExtendedEpochInfo::fetch_current` already ratchets the version internally via the proven-metadata path, so the post-refresh runtime clamp is removed as redundant: build() floors per-network and the ratchet is monotonic-up; a network switch is a fresh build(), not a runtime mutation. Tests reverted to per-network reality: testnet/devnet/regtest default builder boots at 12 directly; mainnet seeds at 11. The relocated refresh tests exercise the real `refresh_protocol_version` end to end — testnet (live at 12 == latest) confirms its floor through the proven query; the generic ratchet test still proves an upward 10→12 climb. No under-shoot trade-off — per-network floors are restored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ Review complete (commit ec3d164) |
There was a problem hiding this comment.
Pull request overview
This PR aims to restore correct SDK protocol-version boot behavior by reintroducing a per-network minimum protocol-version floor (instead of a single cross-network default), and by simplifying protocol-version refresh to use the existing proven-query ratchet path.
Changes:
- Replaces the single initial protocol-version constant with a
min_protocol_version(network)floor used duringSdkBuilder::build(). - Inlines
Sdk::refresh_protocol_versionintosdk.rsand deletes the dedicatedsdk/refresh.rsmodule. - Updates fetch/encoder tests and shared test helpers to assert the new “boot at per-network floor” behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/rs-sdk/src/sdk.rs | Implements protocol-version floor + refresh changes; updates builder seeding and related tests. |
| packages/rs-sdk/src/sdk/refresh.rs | Removes the old refresh module (refresh logic moved into sdk.rs). |
| packages/rs-sdk/tests/fetch/document_query_v0_v1.rs | Updates tests to assert default builder seeds to the mainnet floor instead of the removed constant. |
| packages/rs-sdk/tests/fetch/common.rs | Updates documentation/comments in shared test helpers to describe per-network floor boot behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 62-64: The min_protocol_version function ignores its network
parameter (indicated by the underscore prefix) and always returns
PROTOCOL_VERSION_10, but the tests expect per-network values: Mainnet should
return PROTOCOL_VERSION_11 and Testnet/Devnet/Regtest should return
PROTOCOL_VERSION_12. Remove the underscore prefix from the network parameter and
implement a match statement to return the appropriate protocol version based on
the network variant, returning PROTOCOL_VERSION_11 for Mainnet and
PROTOCOL_VERSION_12 for the other network types.
🪄 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: 093f5a31-599c-4e7b-b043-4896611f61e9
📒 Files selected for processing (4)
packages/rs-sdk/src/sdk.rspackages/rs-sdk/src/sdk/refresh.rspackages/rs-sdk/tests/fetch/common.rspackages/rs-sdk/tests/fetch/document_query_v0_v1.rs
💤 Files with no reviewable changes (1)
- packages/rs-sdk/src/sdk/refresh.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
All six agents independently flagged the same blocking issue: min_protocol_version at packages/rs-sdk/src/sdk.rs:62-64 ignores its Network argument and returns PROTOCOL_VERSION_10 for every network. Verified against the source — this directly contradicts the new in-file tests (test_min_protocol_version_mapping, test_testnet_default_builder_boots_at_per_network_floor, test_testnet_refresh_confirms_floor_via_proven_query) and reintroduces (and widens) the fee-under-reservation window the PR description claims to close. The accompanying doc comments still describe a per-network floor that no longer exists.
🔴 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] packages/rs-sdk/src/sdk.rs:62-64: min_protocol_version returns v10 for every network — defeats the PR's purpose and breaks the new tests
`min_protocol_version` discards its `Network` argument and unconditionally returns `dpp::version::v10::PROTOCOL_VERSION_10`, with a `TODO` to set real per-network floors later. This is the helper `SdkBuilder::build()` uses to seed `initial_protocol_version`, so every unpinned SDK now boots at v10 — mainnet boots below its v11 live floor, and testnet/devnet/regtest boot two versions below their v12 live floor.
Concrete consequences in this PR:
1. The three new tests added in the same module fail at runtime:
- `test_min_protocol_version_mapping` (sdk.rs:1922) asserts Mainnet → `PROTOCOL_VERSION_11` and Testnet/Devnet/Regtest → `PROTOCOL_VERSION_12`.
- `test_testnet_default_builder_boots_at_per_network_floor` (sdk.rs:1944) asserts the testnet SDK boots at `PROTOCOL_VERSION_12`.
- `test_testnet_refresh_confirms_floor_via_proven_query` (sdk.rs:1967) asserts `dpp::version::LATEST_VERSION == floor`, which is false when `floor == 10`.
2. The PR description's stated regression fix is undone. The PR explains that an unpinned SDK on testnet/devnet/regtest was 'booting one protocol version low (at 11 instead of those networks' live 12)' and that fee-sensitive flows (shielded shield/unshield/transfer/withdraw) size their reserve off `Sdk::version()`. With the floor pinned at v10 for every network — and the previous runtime `fetch_max` clamp in `refresh_protocol_version` removed in this PR — the under-shoot window is strictly wider than before this PR.
3. The surrounding doc comments (sdk.rs:56–61 and elsewhere) still describe a per-network hard lower bound and claim 'testnet boots directly at 12', which no longer matches the implementation.
Either restore the per-network mapping the rest of the PR (description, tests, doc comments) was built around, or — if a single uniform floor is intentional — revise the PR title, description, tests, and re-evaluate the fee-under-reservation risk for non-mainnet networks before landing.
The pin concept was named two inconsistent, opposite-polarity ways: `SdkBuilder.version_explicit` (true = pinned) and `Sdk.auto_detect_protocol_version` (true = auto-detect, i.e. NOT pinned), wired together as `auto_detect_protocol_version: !self.version_explicit`. Collapse both onto a single name and polarity: `version_pinned` (true = version is pinned, auto-detection disabled). The builder field is a straight rename; the Sdk field inverts, so every read/assert flips: - `maybe_update_protocol_version`: `if !auto_detect` -> `if version_pinned` - `refresh_protocol_version`: `if auto_detect` -> `if !version_pinned` - `build()` wiring drops the negation: `version_pinned: self.version_pinned` No behavioural change. fmt + clippy (-D warnings, --all-features) clean; 147 dash-sdk lib tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
|
✅ 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: "152b8b74e130dd3c7cee1e0d04f5e32707d038f622a1a73213616b4d08db220e"
)Xcode manual integration:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior finding from 8b03937 is STILL VALID at head 42f75e0: min_protocol_version at packages/rs-sdk/src/sdk.rs:62-64 still ignores its Network argument and returns PROTOCOL_VERSION_10 for every network, directly contradicting the PR's own tests (test_min_protocol_version_mapping asserts v11/v12) and the PR's stated goal of booting mainnet at 11 and testnet/devnet/regtest at 12. The latest-push delta (rename of auto_detect_protocol_version to version_pinned and boolean polarity inversion) is mechanically clean and introduces no new latest-delta findings. Carried-forward: 1 blocking. New: 0.
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
…ersion-floor' into refactor/sdk-collapse-protocol-version-floor
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward prior finding from 42f75e0 is STILL VALID at 21ea0d9: min_protocol_version at sdk.rs:64-66 still returns PROTOCOL_VERSION_10 unconditionally with a TODO, while the PR description and title explicitly promise per-network floors (Mainnet→v11, Testnet/Devnet/Regtest→v12). The PR's central stated goal therefore is not implemented; unpinned testnet/devnet/regtest SDKs boot two versions below the live network protocol until the first proven metadata refresh, re-opening the fee-under-reservation window the PR was written to close. A secondary issue from the latest delta: build() no longer clamps caller-supplied with_initial_version against min_protocol_version, so once the per-network mapping is corrected, a sub-floor initial seed would still slip through (with_version pins remain intentionally exempt).
🔴 1 blocking | 🟡 1 suggestion(s)
🤖 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] packages/rs-sdk/src/sdk.rs:64-66: min_protocol_version still returns v10 for every network — PR's stated goal is not implemented
Carried-forward prior finding, STILL VALID at this head. `min_protocol_version` ignores its `_network` argument and unconditionally returns `dpp::version::v10::PROTOCOL_VERSION_10` with a TODO deferring the real per-network mapping. The PR description explicitly commits Mainnet→`PROTOCOL_VERSION_11` and Testnet/Devnet/Regtest→`PROTOCOL_VERSION_12`, and frames the whole change as closing a fee-under-reservation window for shielded flows that size reserves from `Sdk::version()` before the first proven response.
Consequences observable at this SHA:
- `SdkBuilder::build()` (sdk.rs:1081-1084) seeds every unpinned network at v10. Testnet/devnet/regtest under-shoot the live floor by two versions instead of one, which is strictly worse than the cross-network constant the PR claims to have replaced.
- `test_testnet_default_builder_boots_at_per_network_floor` (sdk.rs:1885-1897) asserts `sdk.protocol_version_number() == min_protocol_version(Network::Testnet)`. Because both sides resolve through the same function, the assertion is tautological and passes against any uniform floor; it does NOT verify the PR's stated invariant.
- The PR description claims a `test_min_protocol_version_mapping` test guards the mapping, but no such test exists in this checkout.
Until the per-network mapping is implemented and guarded by a literal-value test (e.g. `assert_eq!(min_protocol_version(Network::Testnet), PROTOCOL_VERSION_12)`), the surrounding refactor (refresh inlining, clamp removal, field rename) is cosmetic and the original regression remains.
- [SUGGESTION] packages/rs-sdk/src/sdk.rs:1081-1084: build() does not floor an explicit with_initial_version against the per-network minimum
The PR description states: "`build()` floors per-network. `initial_protocol_version = self.version.protocol_version.max(min_protocol_version(self.network))`." The actual code uses `self.version.unwrap_or_else(...)`, so a caller invoking `with_initial_version(pv)` where `pv.protocol_version < min_protocol_version(self.network)` is NOT lifted to the floor. `with_version` pins remain intentionally exempt (sdk.rs:949-950, guarded by the pin-below-floor test), but `with_initial_version` is documented as a seed, not a pin — its docstring says auto-detect already starts at the per-network floor, so an explicit seed below the floor contradicts the documented contract. Once the blocking finding above is fixed, this gap is what allows a caller to re-open the under-reservation window. Pair the floor mapping fix with a `max()` clamp here.
Claudius-Maginificent
changed the title
The PV-floor change on this branch flattened min_protocol_version() to PROTOCOL_VERSION_10 for all networks and removed floor-clamping of pinned versions. Two test suites still encoded the old expectations: - rs-sdk sdk_builder_default_seeds_atomic_to_floor asserted the hardcoded PROTOCOL_VERSION_11 constant; now asserts PROTOCOL_VERSION_10. - wasm-sdk builder chaining tests expected withVersion(1) to be raised to the network floor; pinned versions are now used as-is, so the assertions move from greaterThan(1) to equal(1). <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>
Claudius-Maginificent
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-forward verification: prior finding #1 (flat v10 floor) and #2 (with_initial_version not floored) are both classified per the current PR contract — #1 is INTENTIONALLY DEFERRED (TODO at sdk.rs:65 explicitly tracks restoration; docs frame v10 as a seed for auto-detect) and #2 is OUTDATED (with the lowest known PV as the floor and with_initial_version documented as a deliberate auto-detect seed, no clamp is required). One new cumulative finding remains: the PR description promises eager refresh_protocol_version on every real init path (native, FFI dash_sdk_create*, WASM constructor), but the code at f7d9154 does not wire it into any of those paths — only the explicit caller-driven dash_sdk_refresh_protocol_version FFI symbol and Swift wrapper invoke it. Because this PR simultaneously lowers the mainnet floor from v11 to v10, the gap the PR claims to close is in fact widened for shielded fee-sensitive flows that compute reserves from Sdk::version() before the first proven response ratchets the atomic upward. The latest-push delta itself is test-only and reconciles assertions to the new flat floor and the no-clamp behavior; those test changes are correct.
🔴 1 blocking
1 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/src/sdk.rs`:
- [BLOCKING] packages/rs-sdk/src/sdk.rs:1075-1199: Real SDK init paths return before the proven refresh runs, contradicting the PR's stated safety story
The PR description justifies collapsing `min_protocol_version` to a flat `PROTOCOL_VERSION_10` (which is below mainnet's live `PROTOCOL_VERSION_12`) by stating that `refresh_protocol_version()` is invoked eagerly on every real init path — native, the FFI `dash_sdk_create*` entry points (via the FFI runtime), and the WASM constructor — so the version is correct before the first real query. That wiring is not present at this head:
- `SdkBuilder::build()` (sdk.rs:1075-1199) is synchronous; it seeds the atomic with `min_protocol_version` (v10) and returns without invoking refresh.
- `dash_sdk_create`, `dash_sdk_create_extended`, `dash_sdk_create_trusted`, and `dash_sdk_create_with_callbacks` in `packages/rs-sdk-ffi/src/sdk.rs` all return immediately after `builder.build()` without `runtime.block_on(wrapper.sdk.refresh_protocol_version())`. Only the standalone, caller-driven `dash_sdk_refresh_protocol_version` symbol calls it.
- `WasmSdkBuilder::build()` (packages/wasm-sdk/src/sdk.rs:315) just calls `self.inner.build()` and returns the wrapper.
Auto-detect via `verify_response_metadata` → `maybe_update_protocol_version` still ratchets the version after the first proven response, but it cannot help any code path that reads `Sdk::version()` before that first response — which is exactly the pre-query window the PR cites for shielded shield/unshield/transfer/withdraw fee reserves. Before this PR, mainnet seeded at v11, so the gap to a v12 network was one version; with v10 as the new floor and no eager refresh wired, the gap is two versions on first use.
Either wire `refresh_protocol_version` into the real init paths as the description promises (e.g. an async `build_and_refresh` on `SdkBuilder` plus `runtime.block_on(refresh)` after `builder.build()` in each `dash_sdk_create*` and an `await` in `WasmSdkBuilder::build`), or restore a higher per-network floor (the prior behavior) and amend the description to make clear that eager refresh on init is the caller's responsibility on the FFI/WASM paths. Shipping a v10 floor with the eager-refresh story un-wired is the worst combination for the fee-reserve regression the PR is meant to address.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3900 +/- ##
=============================================
+ Coverage 72.78% 87.23% +14.44%
=============================================
Files 22 2643 +2621
Lines 3054 328910 +325856
=============================================
+ Hits 2223 286918 +284695
- Misses 831 41992 +41161
🚀 New features to boost your workflow:
|
…e sdk.rs comments The initial-version-11 bump (0771206) updated the unit test but left document_query_v0_v1::sdk_builder_default_seeds_atomic_to_floor asserting PROTOCOL_VERSION_10 against an 11 floor. Assert the real floor instead and clear comments left stale by the floor/rename/refresh-relocation refactor. 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
Latest push (0771206 'chore: initial version 11') bumps min_protocol_version from PV10 to PV11 in sdk.rs:65, but the integration test sdk_builder_default_seeds_atomic_to_floor in tests/fetch/document_query_v0_v1.rs:229 still asserts PROTOCOL_VERSION_10 — CI will fail deterministically. The PR description also still describes the floor as PV10, so code, test, and description are three-way out of sync. Prior finding about real init paths not running refresh_protocol_version is INTENTIONALLY DEFERRED per the updated PR body, which scopes refresh-on-init wiring to #3902 and keeps native rs-sdk caller-driven.
🔴 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/tests/fetch/document_query_v0_v1.rs`:
- [BLOCKING] packages/rs-sdk/tests/fetch/document_query_v0_v1.rs:226-230: Floor bumped to PV11 but integration test still asserts PV10 — CI will fail
Commit 077120681 raised `min_protocol_version()` in `packages/rs-sdk/src/sdk.rs:65` from `PROTOCOL_VERSION_10` to `PROTOCOL_VERSION_11`. `SdkBuilder::new_mock()` defaults to `Network::Mainnet` and `build()` (sdk.rs:1081-1084, 1178) seeds the atomic from `min_protocol_version(self.network)`, so `sdk_default.version().protocol_version` is now 11. But this test still hard-codes the assertion to `dpp::version::v10::PROTOCOL_VERSION_10`, so the assertion will fail every run under `cargo test -p dash-sdk --features mocks`.
The in-crate refresh test `test_refresh_ratchets_up_via_proven_query` was updated in this same delta to use `super::min_protocol_version(Network::Mainnet)` (floor-agnostic), but the equivalent external test here was missed. The comment immediately above the assertion (lines 222-225) explicitly says "We assert against the actual floor constant rather than a hardcoded integer so the test survives future floor bumps" — yet the assertion below it does the opposite by binding to the `PROTOCOL_VERSION_10` symbol.
Note that the PR description still describes the floor as `PROTOCOL_VERSION_10` and lists this exact test as already reconciled to `== 10`; that text is stale relative to the head being reviewed. The PR author should either (a) revert the floor bump in sdk.rs:65 back to PV10 and keep the test as-is to match the stated contract, or (b) keep the bump, update the test to PV11, and reconcile the PR description. Because `min_protocol_version` is private to `sdk.rs`, the cleanest in-scope fix if keeping PV11 is to assert against `dpp::version::v11::PROTOCOL_VERSION_11`. A more durable fix (worth a follow-up) would expose the floor via a `pub` accessor so `tests/` files can track future floor bumps automatically.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior PV10/PV11 test-assertion finding is FIXED at head 1976b6b. No new blocking issues in the 0771206..1976b6b delta. Remaining items are documentation staleness — several doc-comments and one test name still describe the protocol-version floor as 'per-network' even though min_protocol_version now returns a flat PV11 baseline — plus a minor API hygiene nit that refresh_protocol_version returns a Result that is always Ok.
💬 2 nitpick(s)
min_protocol_version returns the network-specific minimum (Mainnet -> v11, Testnet/Devnet/Regtest -> v12) instead of a single flat v11 baseline, so an unpinned SDK on a v12 network no longer seeds below the network's live minimum. Kept as a const fn; condensed the rustdoc to the floor-not-a-pin rationale. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of the rs-sdk protocol-version floor/pinning/refresh refactor at 02b63c5. The per-network floor is correctly restored (Mainnet→PV11, Testnet/Devnet/Regtest→PV12), version_pinned polarity is consistent, the proven-only refresh path is sound, and tests cover floor seeding, sub-floor pin preservation, upward ratchet, and non-fatal refresh failure. Remaining items are documentation/API-shape nitpicks around the newly public surface and the always-Ok refresh signature.
💬 4 nitpick(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
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)
374-376:⚠️ Potential issue | 🔴 CriticalCall
fetch_current_with_metadata_and_proof()explicitly to honor refresh's proven-only contract.The documentation states refresh_protocol_version issues a "proven" query, but the implementation calls
ExtendedEpochInfo::fetch_current(), which routes through the genericfetch_with_metadata()→fetch_with_metadata_and_proof()path that honorssdk.query_settings(). This means refresh can consume unproven metadata when the SDK is configured withwith_proofs(false). Either callfetch_current_with_metadata_and_proof()directly to enforce proven-only semantics, or update the documentation to reflect that refresh follows the SDK's proof setting.🤖 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/rs-sdk/src/sdk.rs` around lines 374 - 376, In the refresh_protocol_version method, replace the call to ExtendedEpochInfo::fetch_current() with ExtendedEpochInfo::fetch_current_with_metadata_and_proof() to explicitly enforce the proven-only contract that the documentation claims for refresh operations, ensuring that refresh always uses proven metadata regardless of the SDK's general query_settings configuration.
🧹 Nitpick comments (1)
packages/rs-sdk/src/sdk.rs (1)
1088-1188: ⚡ Quick winRun rustfmt on the builder construction block.
This changed block has several non-rustfmt forms (
let sdk=,Sdk{,inner:SdkInstance, extra spaces in calls). Please format before merge.As per coding guidelines, Rust files should be formatted with
cargo fmt --all, follow rustfmt defaults, and use 4-space indentation.🤖 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/rs-sdk/src/sdk.rs` around lines 1088 - 1188, Run cargo fmt on the SDK builder construction block in packages/rs-sdk/src/sdk.rs to fix formatting inconsistencies. The block contains several non-rustfmt formatting issues in the Sdk struct initialization (lines 1088-1188), including missing spaces around operators in variable assignments (let sdk=), improper spacing in struct field initialization (Sdk{, inner:SdkInstance), and inconsistent spacing in method calls. Execute cargo fmt --all to automatically reformat this section to comply with rustfmt defaults and 4-space indentation standards.Source: Coding guidelines
🤖 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/rs-sdk/src/sdk.rs`:
- Around line 374-376: In the refresh_protocol_version method, replace the call
to ExtendedEpochInfo::fetch_current() with
ExtendedEpochInfo::fetch_current_with_metadata_and_proof() to explicitly enforce
the proven-only contract that the documentation claims for refresh operations,
ensuring that refresh always uses proven metadata regardless of the SDK's
general query_settings configuration.
---
Nitpick comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 1088-1188: Run cargo fmt on the SDK builder construction block in
packages/rs-sdk/src/sdk.rs to fix formatting inconsistencies. The block contains
several non-rustfmt formatting issues in the Sdk struct initialization (lines
1088-1188), including missing spaces around operators in variable assignments
(let sdk=), improper spacing in struct field initialization (Sdk{,
inner:SdkInstance), and inconsistent spacing in method calls. Execute cargo fmt
--all to automatically reformat this section to comply with rustfmt defaults and
4-space indentation standards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cf04fbc-1d25-4ef4-bb0b-61768eb95e08
📒 Files selected for processing (2)
packages/rs-sdk/src/sdk.rspackages/rs-sdk/tests/fetch/document_query_v0_v1.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/tests/fetch/document_query_v0_v1.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative reconciliation against the prior reviewed commit 02b63c5. The PR's behavioral core — version_pinned unification, per-network seed via min_protocol_version, no construction-time clamp, and the inlined refresh_protocol_version ratchet — is sound and well-tested. All five prior nitpicks remain unaddressed in the new HEAD (23832dc); they are carried forward verbatim per verifier policy. One additional nit (truncated test rustdoc) is added. No blocking issues; review action is COMMENT.
💬 2 nitpick(s)
1 additional finding(s) omitted (not in diff).
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative reconciliation against prior reviewed commit 23832dc. The 23832dc..2dc43e1 delta is dominated by a v3.1-dev merge bringing in unrelated work (swift-sdk, rs-sdk-ffi signer preflight, KeyDisableGate) that does not touch the PR-owned protocol-version surface. All six prior nitpicks were re-verified against the current HEAD and remain present verbatim; none are addressed. The behavioral core (version_pinned unification, per-network seed via min_protocol_version, removal of the construction-time clamp, inlined best-effort refresh) remains sound and well-tested. No blocking issues — review action COMMENT.
💬 3 nitpick(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verifier review of PR #3900 at 5b0f41d. Latest delta (single rustdoc fix on test_explicit_pin_below_floor_is_preserved) resolves prior finding F6. Four prior nitpicks on rustdoc accuracy after with_initial_version became public and the construction-time clamp was removed remain STILL VALID and are carried forward. One new codex-rust-quality finding (refresh_protocol_version may panic under proofs=false via ExtendedEpochInfo::fetch_current) is preserved as a suggestion. F5 (collapse three non-mainnet match arms) is dropped as out-of-scope style polish. No blocking issues; PR core (version_pinned unification, removed post-build clamp, per-network floor restoration) is coherent and well-tested.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
3 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/rs-sdk/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk/src/sdk.rs:375-384: refresh_protocol_version may panic when SDK is built with proofs disabled
codex-rust-quality flagged that `ExtendedEpochInfo::fetch_current(self)` flows into `LimitQuery<EpochQuery>::query`, which on proofs-disabled SDKs hits `unimplemented!("queries without proofs are not supported yet")`. The refresh path documents itself as best-effort and non-fatal, so a panic on a supported builder configuration would violate that contract. Verify the proofs=false path: if it does panic, gate it behind an early return that either no-ops (preserving the stored version) or returns a typed error via the existing Result channel.
…oc for no-clamp seeding min_protocol_version no longer enforces a runtime lower bound: with_initial_version is now pub and build() applies no construction-time clamp, so an unpinned SDK can be seeded below the per-network value. Reword both doc blocks to describe the floor as a default seed only, flag the sub-floor seeding hazard on with_initial_version, and point callers needing eager discovery at refresh_protocol_version. 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
Latest delta (rustdoc-only on min_protocol_version, with_initial_version, and Sdk::version) resolves the prior floor/seed precision findings (prior-3, prior-4). Three carried-forward prior findings remain valid against head 0d7697b: refresh_protocol_version still panics via unimplemented! when the SDK is built with proofs disabled, its Result<u32, Error> shape still never produces Err, and Sdk::version()'s rustdoc still omits the now-public with_initial_version seed path. One new related nit: parse_proof_with_metadata_and_proof's bootstrap-limit rustdoc still references latest() as the seed, which no longer matches the per-network floor seed used in practice.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review against head 54b8cc7. Latest delta (0d7697b..54b8cc7) is documentation-only (Swift SwiftExampleApp TEST_PLAN.md removing TOK-17) and does not touch the rs-sdk protocol-version refactor. All four prior findings remain valid against the current sdk.rs and are carried forward unchanged; no new in-scope issues were introduced.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
…isabled SDK refresh_protocol_version is documented non-fatal but reached an unimplemented!() epoch query on a proofs-disabled SDK, aborting the process before the Result could carry the error. Early-return the current version when proofs are off so the contract holds. Also: document the with_initial_version seed path in Sdk::version(), correct the parse_proof_with_metadata_and_proof bootstrap doc to name the min_protocol_version seed instead of latest(), and drop the ephemeral CMT-008 review-ID from a fetch.rs comment (keeping #3711). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental review at 33a69c6 reconciles four prior findings: the proofs-disabled refresh panic is fixed via the early-return guard, the Sdk::version() rustdoc now references with_initial_version, and the parse_proof_with_metadata_and_proof bootstrap doc is reframed around the per-network floor seed. The single carried-forward concern is API-shape: refresh_protocol_version still returns Result<u32, Error> while every branch (proofs-off, pinned, fetch error) returns Ok. New verified concerns are a missing test for the new proofs-off no-op branch and minor doc gaps (the parse_proof bootstrap doc still omits the with_initial_version seed path, and several public rustdoc links target the now-module-private min_protocol_version).
🟡 1 suggestion(s) | 💬 2 nitpick(s)
1 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/rs-sdk/src/sdk.rs`:
- [SUGGESTION] packages/rs-sdk/src/sdk.rs:380-395: New proofs-disabled refresh branch has no test
The `!self.prove()` early-return at lines 381–383 is the regression-fix the latest delta adds to prevent `refresh_protocol_version` from panicking on a `with_proofs(false)` SDK. The two other branches of this method each have a dedicated unit test in this file (`test_refresh_leaves_pinned_sdk_unchanged` for the pinned arm and `test_refresh_query_unavailable_keeps_current_version` for the fetch-error arm), but the proofs-disabled arm — the exact configuration this commit is in the tree to protect — is not asserted. Adding a tokio test that builds `SdkBuilder::new_mock().with_proofs(false).build()`, calls `refresh_protocol_version()`, and asserts the seeded version is returned without recording any mock expectation would pin the documented contract ("On a proofs-disabled SDK … this is a no-op") so a future refactor cannot silently re-introduce the panic by inverting or removing the guard.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review for the rs-sdk protocol-version refactor at ec3d164. The latest delta (33a69c6..ec3d164) is a v3.1-dev merge bringing only Swift example-app work (PR #3926); no new Rust changes were introduced. All four prior in-scope findings remain unresolved in the current head and are carried forward unchanged. None are blocking — the only behavioral one (Result with no Err path) is a public-API shape concern best fixed now before #3902 wires this signature into FFI/WASM. Overflow: none (4 of 10 budget used).
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Why this PR exists
SdkBuilder::version_explicittrue = pinned,Sdk::auto_detect_protocol_versiontrue = not pinned), andrefresh_protocol_versionlived in its own module behind a redundant post-build clamp that silently rewrote a caller's pinned version.with_version()did not actually honour the version you pinned — it bumped a sub-floor pin up to the floor behind your back. Cleaning this up is the groundwork for refreshing the protocol version on SDK init.version_pinnedflag introduced here. The two are intended to land together. (Nativers-sdkinit is wired by neither PR —build()stays synchronous; native callers can callrefresh_protocol_version()themselves or rely on the lazy first-query ratchet. Tracked as follow-up.)What was done
min_protocol_versionis per-network (restored in02b63c5f0a). It is aconst fnreturning the network-specific minimum — Mainnet → v11; Testnet / Devnet / Regtest → v12 — and is the per-network lower bound an unpinned SDK seeds from inbuild(). An earlier iteration of this PR had collapsed it to a single flat v11 baseline; that has been reverted so an unpinned SDK on a v12 network no longer seeds below the network's live minimum. The rustdoc was condensed to the floor-not-a-pin rationale.build()deliberately does not apply.max(min_protocol_version(network))— the clamp is removed and stays removed (out of scope for the floor restoration). So a pinned-below version is preserved exactly as given (doc: "the pinned version is used as-is; it is not clamped"), while an unpinned build seeds from the per-network floor and ratchets upward monotonically via auto-detect.version_pinned. Collapses the opposite-polarity pair (version_explicit+auto_detect_protocol_version) into one consistently-polarised flag (true= pinned, auto-detect off). The builder'sversionfield also becameOption<&'static PlatformVersion>(defaulting toNoneinstead of a seeded constant). Internal rename — 21 references, none of the old names remain.refresh_protocol_versionis inlined intosdk.rsand the dedicatedsdk/refresh.rsmodule is deleted (−105 lines). It has no production caller in this PR — the init call sites live in feat(sdk): refresh protocol version on SDK init (FFI/WASM/JS) #3902; three#[cfg(test)]unit tests exercise it here.Testing
cargo build -p dash-sdk,cargo fmt -p dash-sdk --check,cargo clippy -p dash-sdk --all-targets --features mocks— clean (zero warnings).cargo test -p dash-sdk --features mocks --lib— 143 passed;cargo test -p dash-sdk --features mocks --test main fetch::document_query_v0_v1— 10 passed. The floor-targeted unit tests assert againstmin_protocol_version(network)directly (floor-agnostic, survive future floor changes);sdk_builder_default_seeds_atomic_to_floorboots a default (Mainnet) SDK and still expectsPROTOCOL_VERSION_11.CI reconciliation
wasm-sdkWasmSdkBuilderbuilder-chaining tests (withVersion()/ multiple-method chaining) —greaterThan(1)→equal(1), because a pinned version is no longer clamped to the floor (depends on the removed clamp, not on the floor value).tests/fetch/common.rsand thesdk_builder_default_seeds_atomic_to_floorcomment — doc-comment wording aligned with the per-network floor semantics (no logic change).Breaking changes
DEFAULT_INITIAL_PROTOCOL_VERSIONconstant (waspub). External code referencing it will no longer compile.SdkBuilder::with_initial_versionfrompub(crate)topub(additive).with_version()now honours the version exactly as given, including a pin below the floor. (The per-network seed values themselves are unchanged: Mainnet → v11, others → v12.)with_version()signature is unchanged;version_pinnedandmin_protocol_versionremain internal.Checklist
cargo build/cargo fmt --check/cargo clippy --all-targets --features mocks).dash-sdklib: 143;document_query_v0_v1: 10).Related
refresh_protocol_versioninto the SDK init paths (FFI / WASM / JS). Provides the production behaviour that eagerly refreshes the version on init; builds on the unifiedversion_pinnedflag and the co-located refresh primitive from this PR; intended to land together.🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Refactor