refactor(contract): move parameter votes from RunningContractState to MpcContract by SimonRastikian · Pull Request #4869 · near/mpc

SimonRastikian · 2026-10-20T22:13:28Z

This is part of the issue that allows replacing unnecessary old voting structures for different functions on the smart contract, with one single generic struct: Votes. The replacement could mean modifying an existing struct to some form of a wrapper for Votes.

The current struct that we are targeting in this issue is GovernanceThresholdParametersVotes.
Recall: Votes<V> struct contains two information: which participants has voted for a certain proposalhash, and which proposalHashes have been voted for by a certain participant. We thus no more focus on the proposals per-se but on their hashes.

In this PR:

The vote are moved from of RunningContractState to MpcContract.parameters_votes (see parameter_votes variable)
Votes are cleared when a proposal is accepted, when adding a domains, when resharing completes (not on cancellation), and on the TEE kickout. This step is important as moving votes outside RunningContractState removes a reset that happens automatically (and that needs to be restored)
Test coverage for the clear

The next final PR should deal with transforming GovernanceThresholdParametersVotes into a wrapper for Votes and hashing ProposedGovernanceThresholdParameters

… MpcContract

…shadow

github-actions · 2026-10-20T22:22:15Z

Pull request overview

Moves pending vote_new_parameters votes out of RunningContractState into a new MpcContract.parameters_votes field, threading a &mut GovernanceThresholdParametersVotes through the state-machine voting methods. Because the votes no longer get reset for free when a fresh RunningContractState is built, the PR adds explicit clear() calls at each transition (proposal accepted, add-domains, resharing complete, TEE kickout), extends the 3.17.0 migration shadow to decode the old RunningContractState layout, and stops reporting pending votes in the state() view. This is a stepping stone to replacing the bespoke vote type with the generic Votes (#2825).

Changes:

MpcContract gains parameters_votes; RunningContractState loses it (borsh schema snapshot updated).
ProtocolContractState/RunningContractState/ResharingContractState::vote_new_parameters and process_new_parameters_proposal take the vote store as a parameter; the store is cleared on acceptance.
Explicit clears in vote_add_domains, vote_reshared, and the verify_tee kickout path; new unit test for the add-domains case.
v3_17_0_state.rs shadows the old ProtocolContractState/RunningContractState layouts; migration drops pending parameter votes.
dto_mapping.rs no longer maps votes; state() returns an empty parameters_votes.
GovernanceThresholdParametersVotes drops Clone/PartialEq/Eq, gains clear() and a test-only is_empty().

Reviewed changes

Per-file summary

File	Description
crates/contract/src/lib.rs	Adds `parameters_votes: GovernanceThresholdParametersVotes` to `MpcContract` with a doc comment on its lifecycle
crates/contract/src/state/running.rs	Removes the field from `RunningContractState`; `vote_new_parameters` / `process_new_parameters_proposal` take `&mut votes`; clears on acceptance; tests updated
crates/contract/src/state.rs	Threads `votes` through `ProtocolContractState::vote_new_parameters`
crates/contract/src/state/resharing.rs	Threads `votes` into the re-proposal path; tests updated; reset assertion removed
crates/contract/src/state/initializing.rs	Reset assertion removed from keygen-complete test
crates/contract/src/api/governance.rs	Passes `&mut self.parameters_votes`; clears on the add-domains transition; new `vote_add_domains__should_clear_pending_parameter_votes_on_transition` test
crates/contract/src/api/key_events.rs	Clears votes when resharing completes
crates/contract/src/api/attestation.rs	Clears votes on the TEE kickout transition
crates/contract/src/api/lifecycle.rs, crates/contract/src/api/test_utils.rs	Initialise the new field in `init` / `init_running` / test constructor
crates/contract/src/primitives/threshold_votes.rs	Drops derives, adds `clear()` and `#[cfg(test)] is_empty()`
crates/contract/src/dto_mapping.rs	Removes the votes→DTO impl; Running DTO always carries an empty `parameters_votes`
crates/contract/src/v3_17_0_state.rs	Adds `OldGovernanceThresholdParametersVotes`, `OldRunningContractState`, `OldProtocolContractState` (no `Resharing` variant); migration discards pending votes
crates/contract/src/snapshots/…borsh_schema…snap	Schema snapshot moved the field from `RunningContractState` to `MpcContract`
crates/contract/src/state/test_utils.rs	`gen_resharing_state` threads a local vote store
crates/node/src/indexer/fake.rs	Drops the removed field from a struct literal

Findings

Blocking (must fix before merge):

crates/contract/src/api/key_events.rs:110 and crates/contract/src/api/attestation.rs:347 — the two most consequential new clear() calls have no test; reverting either leaves the suite green. The PR also deleted the assertions that previously pinned the reset-on-transition invariant (crates/contract/src/state/initializing.rs:306, crates/contract/src/state/resharing.rs:357), so net coverage of that invariant went down. The vote_reshared path matters most: GovernanceThresholdParametersVotes::vote counts every stored vote for a proposal regardless of current membership (crates/contract/src/primitives/threshold_votes.rs:47), and votes cast during Resharing target prospective epoch P+1, which is exactly the prospective epoch the post-resharing Running state accepts. Without the clear, a failed re-proposal's votes would silently carry into the next epoch's tally. Suggested tests: (a) a contract-level test that seeds a vote while Resharing, drives MpcContract::vote_reshared to completion, and asserts contract.parameters_votes.is_empty(); (b) seed a pending vote in test_verify_tee_triggers_resharing_and_kickout_on_expired_attestation (attestation.rs:762) before verify_tee() and assert it is cleared. No contract-level test currently calls MpcContract::vote_reshared at all.
crates/contract/src/dto_mapping.rs:967 — the comment "Pending votes are stored hash-only, so their proposals can't be reported" is false in this PR: the store still holds full ProposedGovernanceThresholdParameters per voter (threshold_votes.rs:12). It describes the planned follow-up, not this code, and will mislead the next reader. Either keep reporting the votes (they are reachable from MpcContract::state via self.parameters_votes), or reword honestly, e.g. // TODO(#2825): reporting dropped ahead of the switch to hash-only votes.
crates/contract/src/state/resharing.rs:24 — doc drift: "For tracking the votes we also use the same tracking structure in the running state" no longer holds; votes now live on MpcContract. Per AGENTS.md, stale doc comments on reshaped code are review-blocking. Suggest: "Re-proposal votes are tallied in MpcContract::parameters_votes, not here."

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

crates/contract/src/lib.rs:70 — "kept outside ProtocolContractState so they can be storage-backed" explains a future design; here it is still an inline BTreeMap. Phrase it as a TODO(#2825) so it reads as intent rather than a description of the current code.
crates/contract/src/v3_17_0_state.rs:108 — the old and new vote layouts are byte-identical (BTreeMap<AuthenticatedAccountId, ProposedGovernanceThresholdParameters>; per_domain_thresholds predates 3.17.0 via feat: adding resharing per-domain #3326), so the pending votes could be carried into crate::MpcContract.parameters_votes for free instead of dropped. Dropping is consistent with the OldTeeState precedent, but operators mid-vote at upgrade time will have to re-vote, which should be noted in the release notes. Also note the sandbox upgrade tests do not exercise the non-empty path: execute_key_generation_and_add_random_state (crates/contract/tests/sandbox/common.rs:586) casts one vote, but the subsequent keygen on the 3.17.0 binary resets it before the upgrade, so the new OldGovernanceThresholdParametersVotes decode of a populated map is untested.
crates/contract/src/v3_17_0_state.rs:122 — omitting Resharing means a contract mid-resharing fails with a borsh "unexpected variant" error, falls through to the current-layout parse (which also fails), and panics with "could not deserialize contract state" instead of the explicit "Contract must be in running state when migrating." Same outcome, worse diagnostics. Shadowing Resharing(OldResharingContractState) is five fields and keeps the explicit check reachable.
crates/near-mpc-contract-interface/src/types/state.rs:249 — the public DTO field parameters_votes is now always empty from this contract version onward, with no signal to SDK consumers. Add a doc comment (or #[deprecated]) saying the contract no longer reports pending parameter votes, or plan its removal in the follow-up. No in-repo consumer reads it, so nothing breaks today.

⚠️ Issues found

…tes-with-Votes

…posal

kevindeforth

Thank you, just some nits.
I think this is good. I suspect we could simplify the code a bit, but that will become easier once we already use the generic votes struct, so it's a bit of a chicken-and-egg problem.

kevindeforth · 2026-10-22T07:41:28Z

@claude review

github-actions · 2026-10-22T07:49:58Z

Pull request overview

Moves the pending vote_new_parameters votes out of RunningContractState into a new MpcContract.parameters_votes field and threads a &mut GovernanceThresholdParametersVotes through the Running/Resharing voting methods. Since the votes no longer reset for free whenever a fresh RunningContractState is built, the PR restores that reset explicitly: on proposal acceptance (inside process_new_parameters_proposal), on the add-domains transition, on resharing completion, and on the TEE kickout. The 3.17.0 migration shadow now decodes the old ProtocolContractState layout (all four variants) and discards pending votes, and state() reports an empty vote map pending the switch to hash-only Votes (#2825).

This revision addresses every blocking point from the earlier review: contract-level tests now cover the vote_reshared and verify_tee clears plus the re-proposal path while Resharing, the misleading dto_mapping.rs comment is a TODO(#2825), the resharing.rs doc no longer claims votes live in the running state, and Resharing is shadowed so the "must be in running state" panic stays reachable.

Changes:

MpcContract gains parameters_votes; RunningContractState loses it (borsh schema snapshot updated; DTO/ABI unchanged).
ProtocolContractState / RunningContractState / ResharingContractState::vote_new_parameters and process_new_parameters_proposal take the vote store; process_new_parameters_proposal is now &self and clears the store on acceptance.
Explicit clear() in vote_add_domains, vote_reshared, and the verify_tee kickout path, each with a unit test; a new test records a re-proposal vote while Resharing.
v3_17_0_state.rs shadows OldGovernanceThresholdParametersVotes, OldRunningContractState, OldResharingContractState, OldProtocolContractState; migration drops pending votes.
GovernanceThresholdParametersVotes drops Clone/PartialEq/Eq, gains clear() and a #[cfg(test)] is_empty().

Reviewed changes

Per-file summary

File	Description
crates/contract/src/lib.rs	Adds `parameters_votes` to `MpcContract` with a lifecycle doc comment and `TODO(#2825)`
crates/contract/src/state/running.rs	Removes the field; `vote_new_parameters` / `process_new_parameters_proposal` take `votes`; clears on acceptance; tests thread a local store
crates/contract/src/state/resharing.rs	Doc updated; `vote_new_parameters` threads `votes` into the re-proposal path; tests updated
crates/contract/src/state.rs	Threads `votes` through `ProtocolContractState::vote_new_parameters`
crates/contract/src/state/initializing.rs	Drops the now-impossible `parameters_votes == default` assertion
crates/contract/src/api/governance.rs	Passes `&mut self.parameters_votes`; clears on add-domains; new tests for add-domains clear, re-proposal vote while Resharing, and reshare-completion clear
crates/contract/src/api/key_events.rs	Clears votes when resharing completes
crates/contract/src/api/attestation.rs	Clears votes on the TEE kickout transition; kickout test seeds a vote and asserts it is cleared
crates/contract/src/api/lifecycle.rs, api/test_utils.rs	Initialise the new field in `init`, `init_running`, and the test constructor
crates/contract/src/primitives/threshold_votes.rs	Drops derives; adds `clear()` and test-only `is_empty()`
crates/contract/src/dto_mapping.rs	Removes the votes→DTO impl; Running DTO carries an empty `parameters_votes` with a `TODO(#2825)`
crates/contract/src/v3_17_0_state.rs	Shadows the 3.17.0 `ProtocolContractState` tree; `Running` migrates with votes dropped, other variants hit the explicit panic
crates/contract/src/snapshots/…borsh_schema…snap	Field moved from `RunningContractState` to `MpcContract`
crates/contract/src/state/test_utils.rs	`gen_resharing_state` threads a local vote store
crates/node/src/indexer/fake.rs	Drops the removed field from the contract-type struct literal

Findings

I traced every transition against the pre-PR behaviour: Running→Resharing (vote or kickout), Running→Initializing, Initializing→Running (keygen done or cancelled; nothing can vote in Initializing, so the store is already empty), Resharing→Resharing (re-proposal), Resharing→Running (complete or cancelled). The only path that keeps votes is resharing cancellation, which matches the old code where previous_running_state.parameters_votes was restored as-is, and the epoch those votes target (prospective + 1) is exactly what the restored Running state expects. The unconditional clear() in verify_tee is correct for both arms of transition_to_resharing_no_checks (the no-domains arm also bumps the epoch in place). The borsh shadow mirrors main's existing assumption that ProtocolContractState was layout-stable since 3.17.0, so no new migration risk is introduced.

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

crates/contract/src/state/resharing.rs:57 — ResharingContractState::vote_new_parameters no longer mutates self now that process_new_parameters_proposal is &self and the vote store is passed in; it can take &self. Small, but it makes the "this state is read-only during a re-proposal vote" property visible in the signature.
CI — the red "Extra CI checks" job runs only cargo-deny. This PR changes no Cargo.toml or Cargo.lock, so the failure is almost certainly an advisory that also affects main; I could not confirm that from this environment, so please check it is pre-existing before merging.

✅ Approved

…ernance tests

…rameters

kevindeforth

Thank you!

note: please wait with merging, I thing it would be best to merge the first 3 PRs in the stack at the same time

kevindeforth · 2026-10-22T11:25:01Z

@@ -107,6 +104,7 @@ impl RunningContractState {
        &mut self,


does this still need to be &mut?

Yes because the inner functions transition_to_resharing_no_checks needs it

netrome

Thanks, looks good to me. A few minor questions.

netrome · 2026-10-22T17:10:55Z

+/// Shadow of the `3.17.0` `GovernanceThresholdParametersVotes` layout.
+#[derive(Debug, BorshSerialize, BorshDeserialize)]
+struct OldGovernanceThresholdParametersVotes {
+    proposal_by_account: BTreeMap<AuthenticatedAccountId, ProposedGovernanceThresholdParameters>,
+}


Why do we shadow this type here rather than reusing the existing type? They seem identical.

netrome · 2026-10-22T17:13:12Z

            tee_verifier_account_id: old.tee_verifier_account_id,
            tee_verifier_votes: old.tee_verifier_votes,
            available_attestation_grants: old.available_attestation_grants,
+            parameters_votes: GovernanceThresholdParametersVotes::default(),


Why not preserve the votes through the migration here?

Note: I see this simplifies the next PR quite a bit: #4870 (review)

netrome · 2026-10-22T17:15:36Z

+        &self,
        prospective_epoch_id: EpochId,
        proposal: &ProposedGovernanceThresholdParameters,
+        votes: &mut GovernanceThresholdParametersVotes,


I'm not too fond of taking a mutable reference here but it seems like the least bad option without making any other major changes.

SimonRastikian added 2 commits October 20, 2026 22:45

refactor(contract): move parameter votes from RunningContractState to…

6a1579a

… MpcContract

fix(contract): don't box InitializingContractState in 3.17 migration …

0365e39

…shadow

SimonRastikian added 4 commits October 21, 2026 00:25

Merge branch 'main' into 2825-replace-GovernanceThresholdParametersVo…

6fb287f

…tes-with-Votes

test(contract): pass the votes registry to process_new_parameters_pro…

c8f90c7

…posal

refactor(contract): address review feedback on the parameter-votes move

52df55f

test(contract): split the re-proposal vote and reshare-clear cases

e1837be

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

SimonRastikian self-assigned this Oct 20, 2026

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

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

Draft

gilcu3 mentioned this pull request Oct 21, 2026

Bump webpki-roots and webpki-root-certs for the emSign advisories #4875

Closed

kevindeforth reviewed Oct 22, 2026

View reviewed changes

Comment thread crates/contract/src/api/governance.rs

Comment thread crates/contract/src/api/governance.rs Outdated

SimonRastikian added 2 commits October 22, 2026 13:01

test(contract): use the Environment helper for caller contexts in gov…

cb00e0d

…ernance tests

refactor(contract): take &self in ResharingContractState::vote_new_pa…

6bac975

…rameters

kevindeforth approved these changes Oct 22, 2026

View reviewed changes

netrome approved these changes Oct 22, 2026

View reviewed changes

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevindeforth commented Oct 22, 2026

Uh oh!

github-actions Bot commented Oct 22, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

kevindeforth left a comment • 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!

kevindeforth Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

SimonRastikian Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

netrome left a comment

Choose a reason for hiding this comment

Uh oh!

netrome Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

netrome Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

netrome Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

netrome Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SimonRastikian commented Oct 20, 2026 •

edited

Loading

kevindeforth left a comment •

edited

Loading