fix: unpin the platform protocol version so clients auto-detect - #113
Draft
thepastaclaw wants to merge 4 commits into
Draft
fix: unpin the platform protocol version so clients auto-detect#113thepastaclaw wants to merge 4 commits into
thepastaclaw wants to merge 4 commits into
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>
📝 WalkthroughWalkthroughThe client no longer pins a platform protocol version. SDK instances expose their negotiated version, and example applications use it for document serialization. Read-only tests verify negotiation against testnet network status. ChangesProtocol version negotiation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant createClient
participant EvoSDK
participant DashNetwork
Application->>createClient: create SDK client
createClient->>EvoSDK: initialize without version override
EvoSDK->>DashNetwork: negotiate protocol version
DashNetwork-->>EvoSDK: return active protocol version
EvoSDK-->>Application: expose version()
Application->>EvoSDK: serialize document with negotiated 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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.Regression coverage
The pin 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.test/read-only.test.mjsnow asserts that acreateClient()client 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. It lives in the read-only suite because that is the only root tutorial job CI runs on PRs (test:setupis not wired into any workflow), and it needs no credentials or funds (the fixture is the DPNS system contract). The same suite also checks that every standalone lite page imports exactly the Evo SDK version declared by its companion app.Validation
All against Node 22.22.0, matching the
.nvmrc22.22 release line and package engine constraint.npm run lint(tsc)npm run test:read-onlynpm 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