fix(wasm-sdk): protocol upgrade state misreported vote count as activation height#3979
Conversation
…ation height ProtocolVersionVoteCount::fetch_many returns a map of protocol version to vote count, but getProtocolVersionUpgradeState assigned the count to activationHeight, left voteCount null, and reported thresholdReached whenever any vote existed. Populate voteCount from the fetched map, drop the activationHeight and isThresholdReached fields (neither is present in or derivable from this response), take currentProtocolVersion from the response metadata instead of the SDK's own configured version, and pick the future version with the most votes as nextProtocolVersion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ProtocolVersionUpgradeStateWasm struct is simplified to drop activation_height and is_threshold_reached fields, changing vote_count to Option. Callers now derive current version from fetch metadata and use a new next_version_upgrade helper. Unit tests are updated accordingly. ChangesProtocol version upgrade state simplification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WasmSdk
participant ProtocolVersionVoteCount
participant next_version_upgrade
participant ProtocolVersionUpgradeStateWasm
WasmSdk->>ProtocolVersionVoteCount: fetch_many_with_metadata
ProtocolVersionVoteCount-->>WasmSdk: metadata + upgrade_result
WasmSdk->>next_version_upgrade: upgrade_result, current_version
next_version_upgrade-->>WasmSdk: next_version, vote_count
WasmSdk->>ProtocolVersionUpgradeStateWasm: new(current_version, next_version, vote_count)
Related issues: None specified. Related PRs: None specified. Suggested labels: wasm-sdk, breaking-change Suggested reviewers: None specified. 🐰 A vote is cast, a height set free, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
✅ Review complete (commit c96f199) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/wasm-sdk/src/queries/protocol.rs (1)
64-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake equal-vote selection deterministic.
ProtocolVersionUpgradesis built from a hash-map-likeIntMap, so equal vote counts can resolve in arbitrary order. If the contract is “highest votes, then highest version,” key on(votes, version)here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/wasm-sdk/src/queries/protocol.rs` around lines 64 - 70, The equal-vote upgrade selection in ProtocolVersionUpgrades is nondeterministic because max_by_key currently compares only vote counts from the upgrades iterator. Update the selection logic in the ProtocolVersionUpgrades handling so that it keys on both votes and version, choosing the highest vote count and then the highest version as the tie-breaker. Keep the change localized to the method/closure that filters upgrades and returns the selected version/votes pair.
🤖 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.
Nitpick comments:
In `@packages/wasm-sdk/src/queries/protocol.rs`:
- Around line 64-70: The equal-vote upgrade selection in ProtocolVersionUpgrades
is nondeterministic because max_by_key currently compares only vote counts from
the upgrades iterator. Update the selection logic in the ProtocolVersionUpgrades
handling so that it keys on both votes and version, choosing the highest vote
count and then the highest version as the tie-breaker. Keep the change localized
to the method/closure that filters upgrades and returns the selected
version/votes pair.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62fa8cf2-3da0-46c9-928f-3bb117c1bb95
📒 Files selected for processing (2)
packages/wasm-sdk/src/queries/protocol.rspackages/wasm-sdk/tests/unit/conversion-simple-types.spec.ts
…sion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the CodeRabbit nitpick in 1894166: 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Focused, correct bug fix for the wasm-sdk protocol-upgrade-state binding: vote counts are now read from the map values, unrepresentable fields are dropped, and currentProtocolVersion comes from ResponseMetadata. No consensus, security, or FFI concerns. Only in-scope observation is that the new next_version_upgrade selection helper has no direct unit test.
🟡 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/wasm-sdk/src/queries/protocol.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/protocol.rs:60-72: Add a direct unit test for `next_version_upgrade`
`next_version_upgrade` centralizes the non-trivial selection logic used by both `getProtocolVersionUpgradeState` and its `WithProofInfo` variant — skip `None` entries, exclude versions `<= current_version`, and pick the max-vote future version. The new `conversion-simple-types.spec.ts` cases only round-trip the WASM struct's JSON/Object shape with a single-candidate fixture; they never construct a `ProtocolVersionUpgrades` map with multiple future versions or a mix of `None`/`Some` entries. Given the original bug was precisely a misinterpretation of this map (activation height read as vote count, single vote read as threshold reached), a `#[cfg(test)]` covering (a) empty upgrades, (b) only-`None` values, (c) all entries `<= current_version`, and (d) max-vote selection among multiple future versions would cheaply protect against a regression in the exact spot most likely to break again.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #3979 remains a narrow, correct wasm-sdk fix for getProtocolVersionUpgradeState. The latest delta (a8557e5..1894166) makes the next_version_upgrade tie-breaker deterministic via max_by_key(|&(version, votes)| (votes, version)) and updates the docstring — this cleanly resolves a prior low-confidence tie-break concern. No new correctness, consensus, security, or FFI issues in the delta. Carried forward: the prior test-coverage suggestion (prior-1) for next_version_upgrade remains valid — no #[cfg(test)] was added, and the new tie-break behavior is not exercised by any test.
🟡 1 suggestion(s)
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [SUGGESTION] (deduped existing open thread)
packages/wasm-sdk/src/queries/protocol.rs:61-73: Add a direct unit test fornext_version_upgrade— Carried forward from the prior review (prior-1) and sharpened by this delta.next_version_upgradecentralizes the selection logic used by bothgetProtocolVersionUpgradeStateand itsWithProofInfovariant: skipNoneentries, exclude versions<= current_version, pick the max-vote future v...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-sdk/src/queries/protocol.rs`:
- [SUGGESTION] packages/wasm-sdk/src/queries/protocol.rs:61-73: Add a direct unit test for `next_version_upgrade`
Carried forward from the prior review (prior-1) and sharpened by this delta. `next_version_upgrade` centralizes the selection logic used by both `getProtocolVersionUpgradeState` and its `WithProofInfo` variant: skip `None` entries, exclude versions `<= current_version`, pick the max-vote future version, and — new in this delta — break ties by highest version via the tuple key `(votes, version)`. The existing `conversion-simple-types.spec.ts` only round-trips the WASM struct's JSON/Object shape with a single-candidate fixture; it never constructs a multi-entry `ProtocolVersionUpgrades` map, so it cannot catch regressions in the selection or tie-break logic. Given the original bug was a misinterpretation of this same map, a small `#[cfg(test)]` covering (a) empty upgrades, (b) only-`None` values, (c) all entries `<= current_version`, (d) max-vote selection among multiple future versions, and (e) tie-break resolving to the higher version would cheaply protect the exact spot that has now been modified twice.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (1894166..c96f199) is test-only: adds a #[cfg(test)] module in packages/wasm-sdk/src/queries/protocol.rs covering next_version_upgrade with five cases (empty map, all-None votes, versions <= current excluded, max-vote selection, equal-vote tie-break to highest version). This directly resolves the sole carried-forward prior finding (prior-1). No new correctness, consensus, security, or FFI issues in the delta or cumulative PR.
3 participants
Issue being fixed or feature implemented
getProtocolVersionUpgradeState(and itsWithProofInfovariant) misinterpreted the result ofProtocolVersionVoteCount::fetch_many. The returnedIndexMap<u32, u64>maps protocol version → vote count (pub type ProtocolVersionVoteCount = u64), but the binding assigned the count toactivationHeight, leftvoteCountas aNoneTODO, and setisThresholdReachedwhenever any vote existed. Verified live on mainnet:activationHeightreturned 132 — exactly the number of PV12 votes — withthresholdReached: truefrom a single vote onward.currentProtocolVersionwas also taken from the SDK's own configured version (self.version()), which can be pinned or stale, rather than from chain state.What was done?
voteCount(nowOption<u64>) from the fetched map.activationHeightandisThresholdReachedfromProtocolVersionUpgradeState: the activation height is not present in this response, and the vote threshold cannot be derived without the active evonode count, for which the WASM SDK has no proven query. Reporting nothing is better than reporting wrong data.currentProtocolVersionfrom the response metadata'sprotocol_version(chain state at response time); the plain variant switched tofetch_many_with_metadata, which costs no extra network round trip.nextProtocolVersionas the future version with the most votes (shared helper for both variants) instead of the first map entry.Not affected:
rs-sdk-ffi's equivalent binding already interprets the map correctly as vote counts, and thejs-evo-sdkprotocol facade is a pass-through whose types come from the generated.d.ts.How Has This Been Tested?
cargo check/cargo clippy -p wasm-sdk --target wasm32-unknown-unknownclean.Breaking Changes
None consensus-wise. SDK API surface:
ProtocolVersionUpgradeState.activationHeightand.isThresholdReachedare removed, andvoteCountchanges from always-nullto a populatedbigint— pre-release v4.0 API cleanup. Consumers reading the removed fields now getundefinedinstead of a wrong number.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit