Skip to content

refactor(contract): factor out shared gen_authenticated_participants helper - #3654

Merged
pbeza merged 4 commits into
mainfrom
3650-testcontract-factor-out-shared-participant-authentication-setup-helper
Jun 23, 2026
Merged

refactor(contract): factor out shared gen_authenticated_participants helper#3654
pbeza merged 4 commits into
mainfrom
3650-testcontract-factor-out-shared-participant-authentication-setup-helper

Conversation

@pbeza

@pbeza pbeza commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Closes #3650

Copilot AI review requested due to automatic review settings June 23, 2026 12:12
@pbeza pbeza linked an issue Jun 23, 2026 that may be closed by this pull request
4 tasks
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR refactors code to use a shared helper, so the type prefix should be refactor: instead of test:.

Suggested title: refactor(contract): factor out shared gen_authenticated_participants helper

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

Pull request overview

Factors out the per-test setup helpers that were duplicated across foreign_chain_rpc.rs, domain.rs, and verifier_votes.rs into a single shared gen_authenticated_participants helper in crates/contract/src/primitives/test_utils.rs. Pure test-side refactor — no production code touched, no behavior changes beyond consolidation.

Changes:

  • New gen_authenticated_participants(n) in test_utils.rs that builds n participants and pre-authenticates each before any storage-backed state is written (the ordering invariant from the original foreign_chain_rpc.rs helper is preserved in the doc comment).
  • Removes the three local helpers (setup/setup_participants) and their now-redundant VMContextBuilder / testing_env! imports.
  • Each call site swaps to gen_authenticated_participants(...).

Reviewed changes

Per-file summary
File Description
crates/contract/src/primitives/test_utils.rs Adds the shared helper, plus AuthenticatedParticipantId and near_sdk::{test_utils::VMContextBuilder, testing_env} imports.
crates/contract/src/foreign_chain_rpc.rs Drops local setup, swaps every call site to the shared helper, drops now-unused imports.
crates/contract/src/primitives/domain.rs Drops local setup_participants, swaps every call site to the shared helper, drops now-unused imports.
crates/contract/src/tee/verifier_votes.rs Drops local setup, setup_votes uses the shared helper.

Findings

No blocking issues.

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

  • crates/contract/src/primitives/domain.rs:498 — Subtle behavior change worth flagging: the removed setup_participants previously did participants.insert(account_id, info).unwrap() (panic on duplicate); the new helper delegates to gen_participants, which silently swallows insert errors with let _ = participants.insert(...) at test_utils.rs:145. With 12-char random lowercase account IDs the collision probability is effectively zero in practice, so this is fine, but the inconsistency between gen_participants (silent) and the rest of the codebase (unwrap) is a latent footgun. Consider hardening gen_participants itself to .expect("test setup: duplicate participant account id") in a follow-up — out of scope for this PR.
  • crates/contract/src/tee/tee_state.rs:1505,1549 — Two tests still inline the same VMContextBuilder + testing_env! + AuthenticatedParticipantId::new pattern, but only authenticate a subset of participants (2 of 5, 1 of 3). They don't fit gen_authenticated_participants's all-or-nothing shape, so leaving them out is reasonable; flagging only so future readers know it was a conscious scope choice.
  • crates/contract/src/primitives/test_utils.rs:150-153 — Nice doc comment that captures the why (the testing_env! ordering invariant). No change requested — calling it out as a good example for similar future helpers.

✅ Approved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces duplication across mpc-contract unit tests by extracting a shared helper that generates a participant set and pre-authenticates each participant, then updates several test modules to use it.

Changes:

  • Added gen_authenticated_participants(n) to the shared contract test utilities.
  • Replaced locally-defined “setup/authenticate participants” helpers in multiple test modules with the shared helper.
  • Simplified test-only imports now that per-module authentication loops are removed.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
crates/contract/src/tee/verifier_votes.rs Replaces local participant-auth setup with shared gen_authenticated_participants.
crates/contract/src/primitives/test_utils.rs Introduces gen_authenticated_participants helper for reuse across tests.
crates/contract/src/primitives/domain.rs Removes duplicated test setup helper; uses shared authenticated-participant generator.
crates/contract/src/foreign_chain_rpc.rs Removes duplicated test setup helper; uses shared authenticated-participant generator.

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

Comment thread crates/contract/src/primitives/test_utils.rs Outdated
@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR modifies source code files, not test files only. Since you're factoring out a shared helper across multiple source modules, the type prefix should probably be refactor: instead of test:.

Suggested title: refactor(contract): factor out shared gen_authenticated_participants helper

@pbeza pbeza changed the title test(contract): factor out shared gen_authenticated_participants helper refactor(contract): factor out shared gen_authenticated_participants helper Jun 23, 2026

@gilcu3 gilcu3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@pbeza
pbeza enabled auto-merge June 23, 2026 13:45

@netrome netrome left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pbeza
pbeza added this pull request to the merge queue Jun 23, 2026
Merged via the queue into main with commit affdfce Jun 23, 2026
15 checks passed
@pbeza
pbeza deleted the 3650-testcontract-factor-out-shared-participant-authentication-setup-helper branch June 23, 2026 17:10
nocktoshi pushed a commit to nocktoshi/mpc that referenced this pull request Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(contract): factor out shared participant-authentication setup helper

4 participants