refactor(contract): store parameter votes hash-only via Votes by SimonRastikian · Pull Request #4870 · near/mpc

SimonRastikian · 2026-10-20T22:55:30Z

This is part of the issue

In this PR:

GovernanceThresholdParametersVotes is a wrapper around Votes<AuthenticatedAccountId> along with ParametersVotesByVoter and ParametersVotesByProposal
process_new_parameters_proposal now returns votes.vote(...) directly

Notice we don't migrate any existing parameter votes between contract updates. We simply drop the existing votes.

…icatedAccountId>

…tes' into 2825-store-parameter-votes-hash-only

github-actions · 2026-10-20T23:01:20Z

Pull request overview

This PR finishes the hash-only vote storage for governance parameter proposals tracked in #2825. GovernanceThresholdParametersVotes becomes a newtype over the storage-backed Votes<AuthenticatedAccountId> (two new StorageKey prefixes), proposals are identified by the SHA-256 of their borsh encoding via a new ToProposalHash impl, and the "all proposed participants voted → accept and clear" decision moves from RunningContractState::process_new_parameters_proposal into GovernanceThresholdParametersVotes::vote, which now returns bool.

I verified the acceptance semantics are unchanged: every voter for a given hash was authenticated against that proposal's participant set at vote time, so count_participants(proposal.participants()) equals the raw voter count the old code compared against. Migration is unaffected because v3_17_0_state.rs shadows the old layout and constructs the new field via Default. Note: cargo test runs are denied in this review sandbox, so the review is read-only; CI should confirm the snapshot and unit tests.

Changes:

GovernanceThresholdParametersVotes wraps Votes<AuthenticatedAccountId> with a manual Default bound to the new ParametersVotesByVoter/ParametersVotesByProposal storage keys
ToProposalHash (Borsh + Sha256) implemented for ProposedGovernanceThresholdParameters
vote now returns bool and clears all pending votes on acceptance; process_new_parameters_proposal forwards that result
Borsh schema snapshot updated; ProposedGovernanceThresholdParameters no longer appears in persisted state
TODO(#2825) comments resolved; new unit test for the accept-and-clear path

Reviewed changes

Per-file summary

File	Description
crates/contract/src/primitives/threshold_votes.rs	Newtype over `Votes`, `ToProposalHash` impl, `vote` returns `bool` and clears on acceptance, test updates plus one new test
crates/contract/src/state/running.rs	Delegates accept/clear decision to `votes.vote`
crates/contract/src/storage_keys.rs	Appends two new storage key variants
crates/contract/src/lib.rs	Replaces TODO with rationale doc comment on `parameters_votes`
crates/contract/src/dto_mapping.rs	Replaces TODO with a comment stating pending votes can no longer be reported
crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap	Schema snapshot reflects the new tuple-struct layout

Findings

Blocking (must fix before merge):

crates/near-mpc-contract-interface/src/types/state.rs:168 — Doc drift on a public API. The DTO GovernanceThresholdParametersVotes is still documented as "Votes for threshold parameter changes.", but after this PR the contract never populates it: crates/contract/src/dto_mapping.rs:967 always returns an empty map. The base branch carried a TODO(#2825) for this; this PR drops the TODO and makes the empty report permanent, so state() consumers (operator tooling, backup-cli) will read {} as "no pending votes" with nothing telling them otherwise. Either (a) document on the DTO that the contract no longer reports pending parameter votes and the field is kept only for wire compatibility, with a TODO(#issue) tracking its replacement, or (b) report the hash-keyed tally now, mirroring how MeasurementVotes/LauncherHashVotes are mapped from Votes::all() at crates/contract/src/dto_mapping.rs:819. Option (b) is a few lines since the new type already wraps Votes<AuthenticatedAccountId>.

Non-blocking (nits, follow-ups, suggestions):

crates/contract/src/primitives/threshold_votes.rs:230 — The new test's name promises that acceptance clears all pending votes, but the assertion only checks the accepted proposal's tally is zero. That does not distinguish self.clear() from "remove votes for this hash". Suggest casting a vote for a second, distinct proposal in // Given and asserting votes.is_empty() in // Then.
crates/contract/src/primitives/threshold_votes.rs:15 — "Proposals are stored hash-only: every vote carries the full proposal." reads as self-contradictory on first pass. Suggest: "Only proposal hashes are stored; callers pass the full proposal with each vote so acceptance can be checked against its participant set."

⚠️ Issues found

kevindeforth · 2026-10-21T13:12:49Z

+            .expect("usize should never fail to convert to u64 on wasm32");
+        let accepted = n_votes == n_proposed;
+        if accepted {
+            self.clear();


What is the reason for moving this here? I think this should be handled by the caller (like we currently do).
If this is the right place, then the function name should be reflecting that we not only vote, but also clear in case of success.

kevindeforth · 2026-10-21T13:18:44Z

            keyset: (&self.keyset).into_dto_type(),
            parameters: (&self.parameters).into_dto_type(),
-            // TODO(#2825): reporting dropped ahead of the switch to hash-only votes.
+            // Pending votes are stored hash-only, so their proposals can't be reported.


This comment doesn't add much value, please remove it.

kevindeforth · 2026-10-21T13:19:42Z

+/// Tracks votes for proposed threshold parameters; each account can maintain one vote.
+/// Only proposal hashes are stored; callers pass the full proposal with each vote so
+/// acceptance can be checked against its participant set.


nit: Can you make this comment more concise?

kevindeforth · 2026-10-21T13:25:21Z

+    /// Pending votes for new parameters; storage-backed, so kept outside [`ProtocolContractState`].
+    /// Emptied on every protocol state transition except a resharing cancellation.


nit: This comment risks getting stale. Please avoid paraphrasing the code.

kevindeforth · 2026-10-21T13:26:25Z


-/// Votes for threshold parameter changes.
+/// Votes for threshold parameter changes. Always empty: the contract stores only proposal
+/// hashes and no longer reports pending votes. Kept for wire compatibility.


I have two questions:

What depends on it, i.e. why do we need to keep it and can't remove it?

How can we view current votes

edit: ah, I think this gets changed in #4872

SimonRastikian added 2 commits October 21, 2026 00:20

refactor(contract): store parameter votes hash-only via Votes<Authent…

61501b3

…icatedAccountId>

Merge branch '2825-replace-GovernanceThresholdParametersVotes-with-Vo…

461681a

…tes' into 2825-store-parameter-votes-hash-only

SimonRastikian added this pull request to stack #4871 October 20, 2026 22:55

SimonRastikian self-assigned this Oct 20, 2026

fix(contract): address review feedback on hash-only parameter votes

e429828

github-actions Bot mentioned this pull request Oct 20, 2026

feat(contract): report pending parameter votes by proposal hash in state() #4872

Draft

kevindeforth reviewed Oct 21, 2026

View reviewed changes

Navigation Menu

Repository navigation

refactor(contract): store parameter votes hash-only via Votes - #4870

refactor(contract): store parameter votes hash-only via Votes#4870
SimonRastikian wants to merge 3 commits into
2825-replace-GovernanceThresholdParametersVotes-with-Votesfrom
2825-store-parameter-votes-hash-only

SimonRastikian commented Oct 20, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 20, 2026

Uh oh!

kevindeforth Oct 21, 2026

Uh oh!

kevindeforth Oct 21, 2026

Uh oh!

kevindeforth Oct 21, 2026

Uh oh!

kevindeforth Oct 21, 2026

Uh oh!

kevindeforth Oct 21, 2026 •

edited

Loading

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		/// Pending votes for new parameters; storage-backed, so kept outside [`ProtocolContractState`].
		/// Emptied on every protocol state transition except a resharing cancellation.

Conversation

SimonRastikian commented Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 20, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

kevindeforth Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

kevindeforth Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

kevindeforth Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

kevindeforth Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

kevindeforth Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SimonRastikian commented Oct 20, 2026 •

edited

Loading

kevindeforth Oct 21, 2026 •

edited

Loading