fix: unpin the platform protocol version so clients auto-detect - #113
Conversation
PLATFORM_VERSION_OVERRIDE was pinned to 12 while testnet has been running protocol version 13 since late July. A pin is not a floor: rs-sdk's version ratchet returns early when `version_pinned` is set, so a pinned client stays on the old version silently — no error, no warning — no matter what the network reports. The pin's TODO said to remove it once dashpay/platform#3809 landed in the consumed SDK. That condition is now met, and the behavior is verified against the consumed @dashevo/evo-sdk 4.1.0 and live testnet: unpinned after connect: 12 -> after a proof-bearing read: 13 version: 12 after connect: 12 -> after a proof-bearing read: 12 version: 13 after connect: 13 -> after a proof-bearing read: 13 testnet reports protocol.drive.current = 13; the SDK's own ceiling (getLatestVersionNumber) is 13. So an unpinned client negotiates its way to the network's active version, capped at what the SDK understands. Remove the pin rather than bumping it to 13. Bumping re-arms the same trap for protocol version 14 and leaves the failure mode intact — silent, invisible in CI, and only observable by manual dispatch of the read-write suite. platformVersion.mjs existed only to hold this constant, so it goes with it. Its other consumer was `document.toJSON(PLATFORM_VERSION_OVERRIDE)` in three example apps; those now pass `sdk.version()`, which reflects the negotiated version and cannot go stale. dashmint-lite.html carried the same defect independently — pinned to 11 with a comment pointing back at this constant — so it is unpinned too. Co-Authored-By: Claude <noreply@anthropic.com>
The pin that dashpay#112 reported was invisible to CI: pull requests only run test:read-only, and that suite passed against a PV-13 testnet with the client stuck on 12 the whole time. Nothing asserted the negotiated version, so the staleness could only be found by reading the constant. Assert that a client from createClient() settles on the network's active protocol version after its first proof-bearing read. Verified both ways against live testnet: passes unpinned, fails with `{ version: 12 }` restored. The expected value is min(network active version, SDK ceiling) rather than the network version alone. When the network upgrades ahead of a released SDK the client legitimately stops at its own ceiling, and that shouldn't read as a regression. Lives in the read-only suite because that is the only job CI runs on pull requests; test:setup is not wired into any workflow. No credentials or funds required — the fixture is the DPNS system contract. Co-Authored-By: Claude <noreply@anthropic.com>
createNote and saveReview now read the negotiated protocol version off the SDK instead of a shared constant, so the hand-rolled stubs in these suites need to answer version(). Both stubs are cast (`as never` / `as unknown as DashSdk`), so the compiler couldn't flag the gap — it showed up at runtime as "sdk.version is not a function". Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 42 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change removes hardcoded platform protocol version overrides, exposes negotiated SDK versions to example applications, updates lite pages to Evo SDK 4.1.0, and adds tests and documentation for negotiation and version alignment. ChangesProtocol version negotiation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant createClient
participant EvoSDK
participant DashNetwork
participant ExampleApp
createClient->>EvoSDK: create client without version override
EvoSDK->>DashNetwork: connect and negotiate protocol version
DashNetwork-->>EvoSDK: network protocol version
EvoSDK-->>ExampleApp: negotiated SDK version
ExampleApp->>EvoSDK: serialize document with version()
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Co-Authored-By: Claude <noreply@anthropic.com>
The two guards added for dashpay#112 lived inside test/read-only.test.mjs, which otherwise only runs tutorials as subprocesses. Neither guard is a tutorial run, so they are moved to test/platform-version.test.mjs and test/lite-sdk-versions.test.mjs and `test:read-only` goes back to covering just the tutorial suite. `npm test` and `test:all` now run all three files, and the PR job in test-tutorials.yml calls `npm test` so the new guards run on pull requests.
The live negotiation test only proves the version is unpinned once a proof-bearing read has ratcheted it, so it asserts against network state rather than an invariant. test/platform-version-config.test.mjs adds an offline guard that mocks the three `*Trusted` factories and asserts createClient() passes them no arguments — a reintroduced pin fails there without needing the network, and `local` gets covered too. The live test now loops over testnet and mainnet instead of reading NETWORK, so a pin on either network shows up rather than only on whichever one the environment happened to select.
✅ Action performedReview finished.
|
![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 `@README.md`:
- Around line 111-115: Update the testing prerequisites in README.md so the .env
requirement applies only to npm run test:read-only and npm run test:read-write,
while npm test remains documented as runnable without wallet credentials.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
The prerequisite said to configure .env "before running tests", which contradicted the line two rows below calling `npm test` a safe default. Only the tutorial suites need credentials: without a PLATFORM_MNEMONIC the read-only suite skips the tutorials that require one, and the write suite cannot run at all. test/platform-version.test.mjs no longer reads process.env now that it names the networks explicitly, so its dotenv import goes too.
thephez
left a comment
thephez
left a comment
There was a problem hiding this comment.
Tests and builds working
2 participants
Closes #112.
The problem
PLATFORM_VERSION_OVERRIDEwas pinned to12while testnet has been running protocol version 13 since late July. A pin is not a floor — inrs-sdkthe version ratchet returns early whenversion_pinnedis set, so a pinned client stays on the old version silently: no error, no warning, no matter what the network reports.The constant's own TODO said to remove it once dashpay/platform#3809 landed in the consumed SDK. That condition is now met.
Why remove rather than bump to 13
Bumping re-arms the identical trap for protocol version 14 and leaves the failure mode intact — invisible, and (before this PR) unobservable in CI. Removing the pin is what the TODO prescribed, and auto-detection is verified working in the consumed release.
Measured against
@dashevo/evo-sdk@4.1.0and live testnet:sdk.version()afterconnect(){ version: 12 }{ version: 13 }Testnet reports
protocol.drive.current = 13; the SDK's own ceiling (EvoSDK.getLatestVersionNumber()) is 13. So an unpinned client negotiates up to the network's active version, capped at what the SDK understands — exactly the #3809 behavior.Changes
createClient()no longer passesversiontotestnetTrusted/mainnetTrusted/localTrusted.platformVersion.mjs/.d.mts. The module existed only to hold this constant.sdk.version()todocument.toJSON(...)instead of the constant (dashnote, dashnote-starter, dashrate). That reflects the negotiated version and cannot go stale.DashSdkin each app gainsversion(): number.dashmint-lite.htmlcarried the same defect independently — pinned to11, with a comment pointing back at this constant — so it is unpinned. DashMint, Dashnote, DashRate, and DashProof lite pages now import the same Evo SDK version as their companion apps.test:read-onlyremains tutorial-only;npm testadds separate protocol-negotiation, factory-configuration, and lite-page SDK-version checks.Regression coverage
The pin was invisible to CI before this PR: pull requests ran only
test:read-only, and that suite passed against a PV-13 testnet with the client stuck on 12 the whole time. Nothing asserted the negotiated version.The coverage is split by concern so
test/read-only.test.mjsremains strictly a tutorial suite:test/platform-version.test.mjsperforms a proof-bearing DPNS read on both testnet and mainnet, then asserts that each client settles onmin(network active version, SDK ceiling). This accommodates a network upgrading ahead of the installed SDK without hiding a stale pin. The testnet case was also verified to fail with{ version: 12 }restored.test/platform-version-config.test.mjsspies ontestnetTrusted,mainnetTrusted, andlocalTrustedand asserts that every factory is called with zero arguments. This directly catches any reintroduced{ version: ... }option even while a pin happens to equal the network's current version.test/lite-sdk-versions.test.mjschecks that every standalone lite page imports exactly the Evo SDK version declared by its companion app.npm testruns these focused checks alongside the read-only tutorials, and the PR workflow now calls that aggregate. None of the new checks needs credentials or funds; the live negotiation fixture is the DPNS system contract.Validation
All against Node 22.22.0, matching the
.nvmrc22.22 release line and package engine constraint.npm run lint(tsc)npm testnpm run test:setupprettier --check(changed files)build+test+lintbuild+test+lintbuild+lintscripts/check-shared-auth-parity.shevo-sdkimport in entry chunknode connect.mjsdrives the real path end-to-end and reportsdrive: 4.1.0on testnet.Not verified
test:read-writeis destructive and consumes testnet credits, and no fundedPLATFORM_MNEMONICwas available here, so it was not run. This PR does not claim to have settled the open question inPLATFORM_VERSION_OVERRIDEis still pinned to 12, but testnet has been running protocol version 13 since late July #112 about whether a version-12-pinned client behaved differently for state transitions — it removes the pin so the question stops mattering. Aworkflow_dispatchrun of the read-write suite would still be worth doing before this merges.Follow-ups, deliberately out of scope
react-hooks/set-state-in-effectlint errors in dashnote (LoginModal.tsx,NotesWorkspace.tsx) and a Prettier warning ondashrate/public/dashrate-lite.html.PLATFORM_VERSION_OVERRIDEis still pinned to 12, but testnet has been running protocol version 13 since late July #112 also suggests running the read-write suite on a schedule, since a version-sensitive regression is currently only observable by manual dispatch.Tracker: thepastaclaw/tracker#2594 (remains open pending review).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests