refactor: renaming threshold in verifier_votes.rs and reducing code redundancy

[SimonRastikian]

While closing partly #3903 (verifier_votes.rs), I stumbled upon high redundancy in the tests that I couldn't help but shrink.

[claude]

Pull request overview

This PR is a test-only refactor of crates/contract/src/tee/verifier_votes.rs. The tuple-returning setup_votes helper and the threshold_params wrapper are replaced by a Voting test fixture struct with a with_threshold constructor, a cast helper that hides the vote(proposal, voter, &params).unwrap() boilerplate, and a voter(i) cloning accessor. Net effect is ~23 fewer lines and noticeably less repetition across the six existing tests; no production code and no assertion semantics change.

Changes:

• Removed threshold_params and setup_votes; inlined GovernanceThresholdParameters::new_unvalidated into the new fixture constructor.
• Added a Voting struct holding participants, params, voters and votes, with with_threshold(governance_threshold), cast(voter_idx, &proposal) and voter(idx).
• Rewrote all six tests against the fixture; expected_votes and proposal helpers untouched.

Reviewed changes

Per-file summary

File	Description
crates/contract/src/tee/verifier_votes.rs	Test module only: setup_votes/threshold_params replaced by a Voting fixture struct; six tests rewritten to use Voting::with_threshold, cast and voter. Production code unchanged.

Findings

Blocking (must fix before merge):

• crates/contract/src/tee/verifier_votes.rs:70 and :61 — the PR is titled "renaming threshold in verifier_votes.rs" and cites issue 3903, but the production code in this file is untouched and still carries exactly the terminology the issue targets. Worse, the doc comment is factually wrong, not merely ambiguous:

  • :61-62 — "Returns the winning candidate account once it crosses the signing threshold". The gate is GovernanceThresholdParameters::threshold(), which is a GovernanceThreshold — a distinct quantity, validated to be at least max(ReconstructionThreshold) in crates/contract/src/primitives/thresholds.rs:91-102. A reader who takes this doc at face value will believe verifier changes are gated on the signing/reconstruction threshold. Per the CLAUDE.md documentation-alignment rule, a doc comment describing the wrong invariant is review-blocking.
  • :70 / :82 — let protocol_threshold = threshold_parameters.threshold().value(); ... if count >= protocol_threshold. Rename to governance_threshold.
  • :4 — module doc "chosen by a threshold vote of active participants" -> "governance-threshold vote".

  Suggested: let governance_threshold = threshold_parameters.threshold().value();, and reword both doc comments to say governance threshold.

• crates/contract/src/tee/verifier_votes.rs:147 — cast panics on failure (.unwrap()) but is not must_-prefixed, which docs/engineering-standards.md §"must_ prefix for panicking test helpers" requires, so that callers can see the panic at the call site. Note the same section says helpers "whose failure could be a meaningful test outcome ... should still return Result", and vote() returning Err is SUT behavior, not test wiring. Either resolution works: rename to must_cast, or return Result<Option<AccountId>, Error> and let each test .unwrap() at the call site.

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

• crates/contract/src/tee/verifier_votes.rs:123-124 — the doc sits on Voting but describes what with_threshold builds ("3 authenticated participants ... fresh, empty pending votes"), so it paraphrases the constructor body and goes stale the moment the count or the default changes. Every test already opens with // Given 3 participants, .... Move it onto with_threshold or drop it.
• crates/contract/src/tee/verifier_votes.rs:269,275 — withdraw__should_remove_caller_vote reaches into voting.voters[0] / voting.voters[1] directly while the rest of the file goes through voting.voter(i). withdraw takes &AuthenticatedParticipantId, so &voting.voter(0) works and keeps the access pattern uniform.
• crates/contract/src/tee/verifier_votes.rs:182,199,214,238,259,281 — the // Given 3 participants, threshold 2 comments still use the bare "threshold" this PR is meant to eliminate, and are now inconsistent with the governance_threshold parameter three lines above. Cheap to fix in the same pass.
• crates/contract/src/api/tee_verifier.rs:20,134,196 — the public API doc for vote_tee_verifier_change and its test comments say "signing threshold" for this same voting path; same incorrect term as the blocking item above, and a natural inclusion here. crates/contract/src/foreign_chain_rpc.rs:169,179,212-213 has the identical protocol_threshold / "signing threshold" pattern but is genuinely out of scope for a file-scoped PR — worth a separate pass on issue 3903.

I could not execute cargo nextest or cargo check in this environment, so compilation and test results are unverified. The disjoint-field borrows in retain__should_keep_current_participants_and_drop_the_rest (voting.votes.retain(&voting.participants)) are fine by inspection, but please confirm CI is green.

⚠️ Issues found

[SimonRastikian]

@claude review

[claude]

Pull request overview

This PR completes the threshold-terminology rename for the TEE verifier voting path and collapses the repetition in the verifier_votes.rs test module. protocol_threshold becomes governance_threshold (correct: GovernanceThresholdParameters::threshold() returns a GovernanceThreshold), the "signing threshold" references on this path are corrected, and the tuple-returning setup_votes / threshold_params helpers are replaced by a Voting fixture. Production behavior is unchanged.

All blocking and non-blocking points from the previous round are addressed: the production rename and doc corrections landed, the panicking helper is now must_cast per the must_ convention, the stale Voting doc comment was dropped, voters[i] direct indexing in withdraw__should_remove_caller_vote now goes through voter(i), and the // Given ... comments were updated.

Changes:

• Renamed protocol_threshold to governance_threshold in TeeVerifierVotes::vote.
• Corrected the module doc, the vote doc, and the ABI-exposed vote_tee_verifier_change doc to say governance threshold.
• Replaced setup_votes/threshold_params with a Voting test fixture (with_threshold, must_cast, voter) and rewrote all six tests against it.

Reviewed changes

Per-file summary

File	Description
crates/contract/src/tee/verifier_votes.rs	protocol_threshold -> governance_threshold; module and vote doc comments corrected; test module rebuilt around a Voting fixture.
crates/contract/src/api/tee_verifier.rs	Doc comment on vote_tee_verifier_change and two test // Given comments say governance threshold.

Findings

Blocking (must fix before merge):

• crates/contract/src/api/tee_verifier.rs:20 — the reworded doc comment sits on a #[near]-exported entry point, so it is part of the contract ABI, and crates/contract/tests/abi.rs:45 (test_abi_has_not_changed) snapshot-asserts the entire ABI JSON including doc strings. crates/contract/tests/snapshots/abi__abi_has_not_changed.snap:2851 still carries the old text (crosses the signing threshold, the trusted verifier account is updated), so that test fails on this branch. It is not opt-in: .github/workflows/ci.yml:129 runs cargo nextest run --cargo-profile=test-release --all-features --locked --profile=ci-contract -p mpc-contract, and the test is a plain #[test] with no feature gate or #[ignore]. Per the snapshot workflow in CLAUDE.md, regenerate and commit the .snap alongside this change:

  cargo nextest run --cargo-profile=test-release --all-features test_abi_has_not_changed
  cargo insta accept
  

  This is the only ABI-visible doc touched here — the //! module doc and the TeeVerifierVotes::vote doc do not reach the ABI, so exactly one snapshot line changes.

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

• crates/contract/src/foreign_chain_rpc.rs:169,179,192,212-213 and crates/contract/src/api/foreign_chain_support.rs:133,290,604,694,960 gate on the same GovernanceThresholdParameters::threshold() while still calling it the signing threshold, and foreign_chain_rpc.rs:213 asserts outright that it is "the same one that gates threshold signatures", which is wrong. Out of scope for a file-scoped PR, but worth noting for the issue 3903 follow-up: foreign_chain_support.rs:133 and :290 are also ABI-exposed docs (snapshot lines 701 and 2902), so that pass will need the same snapshot regeneration.
• crates/contract/src/tee/verifier_votes.rs:4 — the reworded module doc line is now ~92 columns while the rest of the file wraps around 76; rustfmt.toml does not set wrap_comments, so nothing will reflow it automatically.

I have no write access to the working tree in this environment, so I could not run cargo check / cargo nextest. The two-phase borrow in voting.votes.withdraw(&voting.voter(0)) and the disjoint-field borrows in retain__should_keep_current_participants_and_drop_the_rest are fine by inspection, and no assertion semantics changed in the rewrite; please confirm CI is green after the snapshot update.

⚠️ Issues found

[gilcu3]

Thank you!

[gilcu3]

nit: that the function name is must_cast is very weird, but I think it is a misunderstanding @netrome raised in one of my PRs a few weeks ago. This function should be called something like cast_votes