feat(contract): introduce generic voting structs by kevindeforth · Pull Request #2739 · near/mpc

kevindeforth · 2026-04-07T11:48:51Z

resolves #1573

claude · 2026-04-08T10:13:59Z

Code Review: feat(contract): introduce generic voting structs

Reviewed the full diff: new Votes<V, P> generic struct with ProposalRegistry and VoteRegistry, plus extraction of ThresholdParametersVotes into its own module.

No critical issues found. The implementation is solid:

Dual-index consistency is correctly maintained in both registries (voter→proposal and proposal→voters)
Orphan proposal cleanup on vote switches works correctly
Operations are properly idempotent where documented
ProposalId overflow is handled with a panic
Comprehensive test coverage across all modules

Minor nits (non-blocking):

Typos in doc comments: votes.rs:245 has "enw" and "Prpoosals", votes.rs:267 has "expect do exist"

✅ Approved

netrome

Just started looking. Noticed a lot of trait bounds on structs, which should be avoided. I'll continue reviewing tomorrow.

netrome

Nice stuff overall, only a minor hard blocker about toe TODO in threshold votes - it's unclear to me if this is something intended for this PR that was overlooked or a planned follow-up. If it's the latter we should reference the issue in the TODO.

netrome · 2026-04-10T07:11:32Z

+    votes_by_voter: BTreeMap<V, ProposalId>,
+    votes_by_proposal: BTreeMap<ProposalId, VoterSet<V>>,


No strong opinion but I'm curious of the choice of BTreeMap here, when we use HashMap and IterableMap in the proposal registry. They all seem very similar to me so I'd expect us to be able to converge to a single map type.

These votes will have at most num_voters entries each and contain pretty small data (type u64 and AccountId, both of which are rather small-ish).

This is fine until we exceed 100 participants, which I don't think will happen anytime soon.

After 1c306a8, we no longer use HashMap. Instead, we have the following BTreeMaps:

Voter <-> ProposalId, containing at most num_voters entries

ProposalId <-> VoterSet<Voter>. The map contains at most num_voters entries and its values VoterSets are disjoint, with their union containing at most num_voters elements.

ProposalHash <-> ProposalId, containing at most num_voters entries, likely much fewer.

And we have one IterableMap: proposal_by_id: IterableMap<ProposalId, (ProposalHash, Proposal)>.

While proposal_by_id also contains less than num_voters entries and thus, is still below the threshold for which native collection types are recommended (taking this guide as reference), we prefer an IterableMap here, because Proposal can be potentially quite large.
An IterableMap ensures we only deserialize the proposal when truly needed.

kevindeforth · 2026-04-16T11:50:46Z

note: this file was votes.rs before and was just renamed. This is the diff to the file currently on main:

< use super::thresholds::ThresholdParameters; < use super::{key_state::AuthenticatedAccountId, participants::Participants}; --- > use crate::primitives::thresholds::ThresholdParameters; > use crate::primitives::{key_state::AuthenticatedAccountId, participants::Participants}; 7a8,9 > // TODO(#2825): Replace with Votes<AuthenticatedAccountId, ThresholdParameters> from generic_votes.rs > // once this type is moved out of RunningContractState (which requires Clone + PartialEq + JSON).

DSharifi

Thanks, for this change. It looks quite good, but I haven't fully understood why some of the APIs are designed as they are, but I assume there's a good reason for it.

I think it would be easier to understand the API if you could migrate one of the existing voting patterns to use these new structs as a PR stacked on top of this one.

DSharifi · 2026-04-16T12:22:47Z

+    P: ProposalBounds,
+{
+    id_by_proposal: BTreeMap<ProposalHash, ProposalId>,
+    proposals_by_id: IterableMap<ProposalId, (ProposalHash, P)>,


note for myself:
is ProposalHash used here?

DSharifi · 2026-04-16T12:42:09Z

+    }
+
+    /// Returns the registry in a form fit for json serialization.
+    pub(super) fn all(&self) -> BTreeMap<ProposalId, (ProposalHash, P)> {


Note:
This function is currently dead code (only used in tests)

DSharifi · 2026-04-16T12:45:06Z

+    pub(super) fn next(&self) -> Self {
+        let Some(next) = self.0.checked_add(1) else {
+            near_sdk::env::panic_str("overflow in proposal id")
+        };
+        ProposalId(next)


Wonder if a mutable increment would be nicer to have, so we don't have to increment and assign.

DSharifi · 2026-04-16T12:55:31Z

+    // counts all the votes for which `predicate` returns true
+    pub fn count_for(&self, predicate: impl Fn(&V) -> bool) -> usize {


How do you envision this being used?

Currently I only see it used in the unit tests.

Some of our current voting structs are only comparing the number of collected votes to the required threshold for accepting a proposal. The issue with this is that we risk counting votes of former participants, who have since left the network. We rely on votes getting cleaned up after each resharing.

Now, the downside of course is that calling this method is more expensive than simply comparing two integers. But I have not yet seen any indication that this would result in intolerable gas costs.

kevindeforth · 2026-04-16T15:19:49Z

Thanks, for this change. It looks quite good, but I haven't fully understood why some of the APIs are designed as they are, but I assume there's a good reason for it.

I think it would be easier to understand the API if you could migrate one of the existing voting patterns to use these new structs as a PR stacked on top of this one.

Thanks for the review! Regarding usage of this API, please take a look at #2930.

kevindeforth · 2026-04-17T02:13:57Z

Putting this on hold for now.

Using the hash allows us to save some code bytes and simplify the structs.
The main idea is to not store the Proposal itself, but only the Hash of the proposal.

Take a look at this comment: #2930 (comment).

I will open another PR with the simplified voting structs such that we can compare.

c.f. #2934

kevindeforth · 2026-04-20T06:35:01Z

closing this in favor of #2934

kevindeforth force-pushed the kd/1573-mpc-contract-introduce-generic-voting-struct branch 2 times, most recently from 5bf1d50 to b6fec1b Compare April 8, 2026 09:48

kevindeforth marked this pull request as ready for review April 8, 2026 10:12

netrome reviewed Apr 9, 2026

View reviewed changes

Comment thread crates/contract/src/primitives/votes/types.rs

Comment thread crates/contract/src/primitives/votes/votes_registry.rs

Comment thread crates/contract/src/primitives/votes/proposal_registry.rs

Comment thread crates/contract/src/primitives/votes.rs

netrome requested changes Apr 10, 2026

View reviewed changes

kevindeforth force-pushed the kd/1573-mpc-contract-introduce-generic-voting-struct branch from 1c4ae34 to 632b1ea Compare April 16, 2026 11:47

kevindeforth commented Apr 16, 2026

View reviewed changes

DSharifi self-requested a review April 16, 2026 12:21

DSharifi reviewed Apr 16, 2026

View reviewed changes

kevindeforth force-pushed the kd/1573-mpc-contract-introduce-generic-voting-struct branch from 1c306a8 to 143e074 Compare April 16, 2026 14:26

kevindeforth mentioned this pull request Apr 16, 2026

refactor: replace tee launcher hash votes with generics #2930

Closed

kevindeforth and others added 6 commits April 16, 2026 18:30

rename votes to threshold_votes and introduce generic voting structs

bc6b41c

fix typos

bcbea9f

some nits

ecbee82

use ProposalHash, instead of hash map

63bfbe4

improve expect message and comments

8100660

add debug assert

6a579df

kevindeforth force-pushed the kd/1573-mpc-contract-introduce-generic-voting-struct branch from 0efb84e to 6a579df Compare April 16, 2026 15:30

kevindeforth marked this pull request as draft April 17, 2026 02:14

kevindeforth mentioned this pull request Apr 17, 2026

feat(contract): vote generics structs #2934

Merged

kevindeforth closed this Apr 20, 2026

kevindeforth deleted the kd/1573-mpc-contract-introduce-generic-voting-struct branch April 20, 2026 09:01

		votes_by_voter: BTreeMap<V, ProposalId>,
		votes_by_proposal: BTreeMap<ProposalId, VoterSet<V>>,

		// counts all the votes for which `predicate` returns true
		pub fn count_for(&self, predicate: impl Fn(&V) -> bool) -> usize {

Conversation

kevindeforth commented Apr 7, 2026

Uh oh!

claude Bot commented Apr 8, 2026

Uh oh!

netrome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

netrome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindeforth Apr 16, 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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DSharifi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevindeforth commented Apr 16, 2026

Uh oh!

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

Uh oh!

Uh oh!

kevindeforth commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevindeforth Apr 16, 2026 •

edited

Loading

kevindeforth commented Apr 17, 2026 •

edited

Loading