feat(contract): vote generics structs

[kevindeforth]

resolves #1573

The main differences to #2739 are:

• we do not keep an instance of the proposals in the contract state
• we rely on the proposal hash as an identifier (meaning, there is no ProopsalId here)
• we only have two maps instead of four and for both of these, we are now using IterableMap to keep gas costs low.

The main advantage is space saving: the contract binary size is smaller and we store less state on chain (c.f. #1674).

The main disadvantage is that viewing contract state is not human-friendly. For example, if someone queried the contract for all resharing proposals, they would no longer see a json serialization of the proposed participant set, but just a 32 byte hash.

For an example usage, c.f. #2935

[netrome]

For example, if someone queried the contract for all resharing proposals, they would no longer see a json serialization of the proposed participant set, but just a 32 byte hash.

To mitigate this, we should store and serve this information somewhere else. Either in the nodes or as a separate service. I'd picture the flow to be something like:

1. Post a proposal on-chain.
2. Post the proposal to the third-party service.
3. The third-party service verifies this proposal against the on-chain state.
4. Anyone can query the service for the proposal.

[netrome]

Skimmed this, I think I understand the idea and I believe we could simplify this even further. Curious to hear your thoughts.

[claude]

Code Review

Reviewed the generic Votes<V> system and the ThresholdParametersVotes extraction. Overall well-structured with good test coverage. A few issues:

1. Unused error variant ProposalIdDoesNotExist (minor)

VoteError::ProposalIdDoesNotExist(u64) is added in errors.rs but never used anywhere. If this is for future use, consider adding it in the PR that actually needs it to avoid dead code.

2. Typo in panic message (votes.rs ~line 303)

.expect("inconistent vote registry");
//        ^ missing 's' — should be "inconsistent"

3. Test naming convention (engineering standards)

Per CLAUDE.md and engineering-standards.md, new tests must follow <system_under_test>__should_<test_assertion>(). The new tests in votes.rs (e.g., new_is_empty, register_first_vote_creates_entry_and_returns_current_votes) and threshold_votes.rs (e.g., test_voting_and_removal) don't follow this pattern. Examples:

• new_is_empty → votes__should_be_empty_when_new
• register_first_vote_creates_entry_and_returns_current_votes → vote__should_create_entry_and_return_voter_set_on_first_vote
• test_voting_and_removal → vote__should_replace_previous_vote_for_same_participant

None of these are individually merge-blocking, but the test naming and typo should ideally be cleaned up.

✅ No critical logic, safety, or security issues found. The dual-index consistency is well-maintained with appropriate assertions, and ProposalHash being Copy keeps the remove_vote borrow pattern sound.

[claude]

PR title type suggestion: This PR appears to be refactoring vote structs to use generics, so refactor: would be more appropriate than feat:.
Suggested title: refactor(contract): make vote structs generic

[claude]

Code Review

Reviewed the full diff (both commits). No critical or merge-blocking issues found.

Verified:

• ProposalHash is Copy (via define_hash! macro), so the borrow pattern in remove_vote where we read from proposal_by_voter while mutating votes_by_proposal is sound.
• .expect() / require!() usage is acceptable — this is NEAR smart contract code (engineering standards exception Run key generation protocol via the network API. #2: panicking ensures no side-effects).
• Dual-index consistency (proposal_by_voter ↔ votes_by_proposal) is correctly maintained across all mutation paths: vote, remove_vote, remove_votes_for_proposal, retain_votes, and clear.
• Second commit addresses all nits from prior review (unused error variant removed, typo fixed, test naming updated).

✅ Approved

[claude]

PR title type suggestion: This PR restructures vote structs to be generic, which is a refactoring change rather than a new feature. Suggested title: refactor(contract): generalize vote structs

[claude]

PR title type suggestion: Making structs generic is internal code restructuring without new user-facing functionality, so the type should probably be refactor: instead of feat:.
Suggested title: refactor(contract): make vote structs generic

[Copilot]

Pull request overview

This PR introduces a generic, storage-efficient voting registry for the contract by indexing votes via proposal hashes and IterableMap, while moving the existing ThresholdParametersVotes implementation into its own module and updating internal imports accordingly.

Changes:

• Added a generic Votes<V> registry using IterableMap plus ProposalHash/VoterSet helpers in primitives/votes.rs.
• Moved the legacy ThresholdParametersVotes implementation into primitives/threshold_votes.rs.
• Updated contract state/tests and DTO mapping to import ThresholdParametersVotes from the new module.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
crates/contract/src/state/running.rs	Updates imports to use primitives::threshold_votes::ThresholdParametersVotes.
crates/contract/src/state/resharing.rs	Updates test imports for ThresholdParametersVotes new module path.
crates/contract/src/state/initializing.rs	Updates test imports for ThresholdParametersVotes new module path.
crates/contract/src/primitives/votes.rs	Replaces legacy threshold votes with generic Votes<V> + ProposalHash + VoterSet implementation and tests.
crates/contract/src/primitives/threshold_votes.rs	Adds extracted legacy ThresholdParametersVotes (BTreeMap-based) and its tests.
crates/contract/src/primitives.rs	Exposes the new threshold_votes module.
crates/contract/src/dto_mapping.rs	Updates imports for ThresholdParametersVotes to match the new module.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

PR title type suggestion: Making vote structs generic appears to be refactoring code structure rather than adding new functionality. Consider using refactor: instead of feat:.

Suggested title: refactor(contract): make vote structs generic

[pbeza]

One blocker and a couple of optional nits.

[pbeza]

V is a bit confusing here — I keep reading it as Value (like in HashMap<K, V>) or Vote (the proposal). Maybe Voter instead? Feels clearer without having to look at the code below.

[kevindeforth]

Do we have a note in a style-guide for this? @netrome mentioned in a different PR that he prefers single-letter generics.

[pbeza]

I don’t think we have. It’s your call how to name it, but I think the Rust community is generally fine with more meaningful names. Anyway, not a blocker.

[netrome]

@netrome mentioned in a different PR that he prefers single-letter generics.

Depends on the context. Would love if you could reference the PR. I would guess it was a struct like:

struct GenericThingamajig<Spam, Eggs> {
    spam: Spam,
    eggs: Eggs,
}

in which case single-letter generics definitely reads better

struct GenericThingamajig<S, E> {
    spam: S,
    eggs: E,
}

But in this case, I agree it's less obvious so it would be nice to convey some information in the generic declaration.

[kevindeforth]

Answered on slack. I am having a hard time interpolating a guideline for our style-guide based on these comments.

[pbeza]

Curious what the minimum number of proposals/votes is where it becomes worth having both proposal_by_voter and votes_by_proposal, instead of just keeping proposal_by_voter and reconstructing votes_by_proposal when needed.

[netrome]

Yeah would be fun to benchmark this just to learn more. I bet it would be pretty different when running in a smart contract on-chain vs locally.

[pbeza]

When voter re-votes for the same proposal, we don't touch storage. This is fine if V is simple, but V is generic — and BTreeSet / IterableMap always keep the original key on equality match, not the new one. So if someone in the future uses a V where Ord ignores some fields but Borsh stores them, those fields will silently freeze to the values from the first insert.

Not a real bug today (our only caller's Ord matches its bytes exactly), but the generic does not enforce this. A test showing the repro:

    /// Demonstrates that `Votes<V>` silently drops non-`Ord` fields of `V` when
    /// a voter re-votes for the same proposal. When a voter casts a vote, then
    /// casts a second vote for the same proposal with a value of `V` that
    /// compares `Ord::Equal` to the first but carries different payload bytes,
    /// the stored `V` is the first insertion's bytes. The newer payload is
    /// silently discarded because `vote()` early-returns for same-proposal
    /// re-votes without going through `remove_vote`, so the stored bytes are
    /// never refreshed
    #[test]
    #[expect(non_snake_case)]
    fn vote__silently_drops_non_ord_fields_when_revoting_same_proposal() {
        #[derive(Clone, Debug, BorshSerialize, BorshDeserialize)]
        struct PartiallyOrderedVoter {
            id: String,
            payload: u64,
        }

        impl PartialEq for PartiallyOrderedVoter {
            fn eq(&self, other: &Self) -> bool {
                self.id == other.id
            }
        }
        impl Eq for PartiallyOrderedVoter {}

        impl PartialOrd for PartiallyOrderedVoter {
            fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
                Some(self.cmp(other))
            }
        }
        impl Ord for PartiallyOrderedVoter {
            fn cmp(&self, other: &Self) -> std::cmp::Ordering {
                self.id.cmp(&other.id)
            }
        }

        // Given: voter type where `Ord` ignores `payload` but Borsh includes it
        let mut votes_registry = Votes::<PartiallyOrderedVoter>::new(
            TestStorageKey::ProposalByVoter,
            TestStorageKey::VotesByProposal,
        );

        let proposal = make_proposal_hash(1);
        let v1 = PartiallyOrderedVoter {
            id: "alice".into(),
            payload: 111,
        };
        let v2 = PartiallyOrderedVoter {
            id: "alice".into(),
            payload: 222,
        };

        assert_eq!(v1, v2);
        assert_ne!(v1.payload, v2.payload);

        // When: alice votes with v1, then re-votes the same proposal with v2
        votes_registry.vote(v1.clone(), proposal);
        votes_registry.vote(v2.clone(), proposal);

        // Then: `all()` returns v1's payload (111), not v2's (222)
        let all = votes_registry.all();
        let stored_voters = all.get(&proposal).expect("proposal should exist");
        let stored_voter = stored_voters.iter().next().expect("one voter");

        assert_eq!(stored_voter.id, "alice");
        assert_eq!(
            stored_voter.payload, 111,
            "BUG: payload should be 222 (last write), but is 111 (first write)"
        );
    }

We should probably either document it, tighten the trait bound, or always refresh the bytes on re-vote.

[kevindeforth]

That's an interesting observation.
Will change the behavior to always remove any existing vote before inserting a new one.
4b6b507

[pbeza]

It’s a bit of an exotic edge case—typically Ord takes all fields of a struct into account. I’m fine with just documenting it so it doesn’t surprise anyone using it. If you’d rather always remove the existing vote before inserting a new one, I’m good with that too.

[pbeza]

@kevindeforth I think the current implementation aligns with standard Rust behavior: https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.insert

Quote:

If the map did have this key present, the value is updated, and the old value is returned. The key is not updated, though; this matters for types that can be == without being identical.

So I don’t think anything needs to be changed here—just adding a similar comment would be nice. Sorry for the confusion.

[kevindeforth]

It's definitely constructed and I think we agree that the PartialEq in the example implementation is shady.

But I thought it was an interesting and valid concern, especially because we found some legacy code like that today (c.f. #2954 (comment)).

Also, the proposed fix is rather easy and removes a few lines of code. I don't think we need to optimize for gas here - if the caller cares, they can refrain from sending the same vote twice.

[claude]

PR title type suggestion: Making vote structs generic is typically a refactoring effort, not a new feature. Consider using refactor: instead of feat: for internal code restructuring.
Suggested title: refactor(contract): generalize vote tracking structs

[claude]

PR title type suggestion: Making structs generic is typically a refactoring effort rather than a new feature. Consider changing the type prefix to refactor: instead of feat:.
Suggested title: refactor(contract): make vote structs generic

[netrome]

Generally looks good to me. Shared some thoughts on the voting struct. I wonder if we can simplify the voter type further. I don't see any instance where this isn't either a ParticipantId or an AccountId (or AuthenticatedAccountId) - would love to hear your thoughts on this.

[netrome]

Yeah would be fun to benchmark this just to learn more. I bet it would be pretty different when running in a smart contract on-chain vs locally.

[netrome]

Nice! I'm always using #[allow(non_snake_case)] but expect is better. I'll start doing this instead.

[kevindeforth]

it's moved code, but added a broken window fix: 37df95f

edit: oops, wanted to respond to #2934 (comment)

[netrome]

@netrome mentioned in a different PR that he prefers single-letter generics.

Depends on the context. Would love if you could reference the PR. I would guess it was a struct like:

struct GenericThingamajig<Spam, Eggs> {
    spam: Spam,
    eggs: Eggs,
}

in which case single-letter generics definitely reads better

struct GenericThingamajig<S, E> {
    spam: S,
    eggs: E,
}

But in this case, I agree it's less obvious so it would be nice to convey some information in the generic declaration.

[netrome]

Now as we touched on in the meeting today, now we have the full "voter" struct in both maps, both as key and value.

In practice, do we ever want to track a complex data structure here? It feels like we just want to keep track of voter identifiers right? So we could just call this VoterId perhaps?

Or we could even simplify this further by converging on using the concrete AccountId type for all votes. While we use ParticipantId for most voting flows, I don't see why we couldn't change this to converge on AccountId everywhere.

[kevindeforth]

by converging on using the concrete AccountId type for all votes

Space-wise, ParticipantId is preferred, because it's a simple u64, whereas AccountId is a String.
We can safely use ParticipantId whenever the vote does not cross epoch boundaries (e.g. when voting for new ThresholdParameters).
But when the vote does cross epoch boundaries, we start relying on invariants, which may not be ideal (c.f. #2955).

So, for now, I wouldn't want to force AccountId here.

In practice, do we ever want to track a complex data structure here?

No, we don't plan on storing complex Voter structures and this isn't the most optimal struct to do so.

It feels like we just want to keep track of voter identifiers right? So we could just call this VoterId perhaps?

Are you suggesting to rename the generic V as VoterId?

[claude]

PR title type suggestion: This PR refactors vote structs to use generics, which is a code restructuring activity. Consider using refactor instead of feat.

Suggested title: refactor(contract): make vote structs generic