feat: add Protocol enum in DomainConfig by gilcu3 · Pull Request #3085 · near/mpc

gilcu3 · 2026-04-30T09:29:57Z

Notice that this is an intermediate step for #2673 , where curve and protocol are both in DomainConfig. In the next PR that duplication will be removed

claude · 2026-04-30T09:55:58Z

Review - PR #3085: Add Protocol enum

Reviewed the full diff. This is a well-structured intermediate step for separating protocol from curve.

Potential issues to consider:

is_valid_protocol_for_purpose is defined and tested but never called in production code. The existing vote_add_domains (running.rs:171) validates is_valid_curve_for_purpose but does not validate the new protocol-purpose combination. This is safe today because add_domains (domain.rs:74) re-derives protocol from curve internally (ignoring the caller-supplied protocol field), but it means a caller can submit a DomainConfig with a mismatched protocol without error — the mismatch just gets silently overwritten. If a future PR removes that re-derivation, validation would need to be added. Worth a comment or TODO if intentional.
init_running (lib.rs:1722) uses from_raw_validated which does NOT re-derive protocol — it trusts the caller-supplied DomainConfig as-is. Unlike add_domains, there is no re-derivation safety net here. If an external caller (e.g. during contract initialization) supplies a mismatched curve/protocol pair, it would persist. Consider adding protocol-purpose validation in from_raw_validated, or at least in the init_running path.
From<Protocol> for Curve is lossy — both CaitSith and DamgardEtAl map to Secp256k1. The doc comment on the reverse direction (From<Curve> for Protocol) acknowledges this, but the From<Protocol> for Curve impl itself has no such warning. A brief doc comment there would help prevent misuse.

Everything else (Borsh migration, JSON backward compat via DomainConfigCompat, snapshot updates, test coverage) looks correct.

✅ Approved — the issues above are non-blocking given this is an intermediate step, but the init_running / from_raw_validated gap should be addressed before the protocol field becomes authoritative.

claude · 2026-04-30T11:42:35Z

Code Review

No critical issues found. This is a clean, well-structured intermediate step for adding the Protocol enum to DomainConfig.

Verified:

Borsh migration (v3_9_1_state.rs) correctly derives protocol from curve for all variants including V2Secp256k1
JSON backward compat via DomainConfigCompat handles missing protocol field gracefully
from_raw_validated now validates curve/protocol consistency (addressing the gap noted in the prior review)
add_domains validates consistency before storing
The .expect() in From<OldDomainRegistry> is acceptable per engineering standards (smart contract panic = no side-effects)
Test coverage is thorough: conversion round-trips, consistency validation, migration, protocol-purpose combinations

The observations from the prior review (lossy From<Protocol> for Curve>, is_valid_protocol_for_purpose not yet called in production) are acknowledged and appropriate for an intermediate step with TODOs in place.

✅ Approved

Copilot

Pull request overview

This PR introduces a new Protocol enum into the shared domain types and threads it through DomainConfig, contract migration code, and tests/examples so the codebase can start distinguishing protocol from curve ahead of the larger domain-separation refactor.

Changes:

Added Protocol to mpc_primitives::domain, plus conversions between Protocol and Curve.
Extended DomainConfig to carry protocol, including JSON compatibility handling and v3.9.1 state migration logic.
Updated contract/node/e2e tests, sample configs, snapshots, and docs to populate the new field.

Reviewed changes

Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
docs/localnet/args/add_domain.json	Updates localnet example domain payloads to include `protocol`.
crates/primitives/src/domain.rs	Adds `Protocol` enum, conversions, and unit tests.
crates/node/src/tests/resharing.rs	Fills `protocol` in resharing tests.
crates/node/src/tests/onboarding.rs	Fills `protocol` in onboarding tests.
crates/node/src/tests/multidomain.rs	Fills `protocol` in multidomain tests.
crates/node/src/tests/faulty.rs	Fills `protocol` in faulty-cluster tests.
crates/node/src/tests/changing_participant_details.rs	Fills `protocol` in participant-change tests.
crates/node/src/tests/basic_cluster.rs	Fills `protocol` in basic cluster tests.
crates/node/src/key_events.rs	Fills `protocol` in key event tests.
crates/near-mpc-contract-interface/src/types/state.rs	Adds `protocol` to `DomainConfig` with JSON compat deserialization.
crates/near-mpc-contract-interface/src/lib.rs	Re-exports `Protocol` from public interface types.
crates/e2e-tests/tests/request_lifecycle.rs	Fills `protocol` in request lifecycle test config.
crates/e2e-tests/tests/request_during_resharing.rs	Fills `protocol` in resharing request tests.
crates/e2e-tests/tests/parallel_sign_calls.rs	Fills `protocol` in parallel request test config.
crates/e2e-tests/tests/key_resharing.rs	Fills `protocol` in key resharing tests.
crates/e2e-tests/tests/foreign_chain_tx_validation.rs	Fills `protocol` in foreign-tx validation setup.
crates/e2e-tests/src/cluster.rs	Adds default protocols to cluster domain defaults.
crates/devnet/src/mpc.rs	Populates `protocol` when building add-domain proposals.
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap	Updates ABI snapshot for new `protocol` field/type.
crates/contract/tests/sandbox/vote.rs	Fills `protocol` in sandbox vote tests.
crates/contract/tests/sandbox/upgrade_to_current_contract.rs	Fills `protocol` in upgrade sandbox tests.
crates/contract/tests/sandbox/update_votes_cleanup_after_resharing.rs	Fills `protocol` in vote-cleanup test data.
crates/contract/tests/sandbox/participants_gas.rs	Fills `protocol` in gas-measurement setup.
crates/contract/tests/sandbox/common.rs	Fills `protocol` across shared sandbox builders/helpers.
crates/contract/tests/inprocess/attestation_submission.rs	Fills `protocol` in in-process attestation tests.
crates/contract/src/v3_9_1_state.rs	Adds legacy-domain migration shims for old on-chain state.
crates/contract/src/state/running.rs	Updates running-state tests for the new field.
crates/contract/src/snapshots/mpc_contract__tests__mpc_contract_borsh_schema_has_not_changed.snap	Updates Borsh schema snapshot for `Protocol`.
crates/contract/src/primitives/test_utils.rs	Generates test domains with `protocol`.
crates/contract/src/primitives/domain.rs	Adds curve/protocol consistency validation and updates registry/test helpers.
crates/contract/src/lib.rs	Updates contract tests to construct domains with `protocol`.
crates/contract/src/errors.rs	Adds error variant for inconsistent curve/protocol pairs.
crates/contract/README.md	Updates contract docs/examples to include `protocol`.
crates/backup-cli/assets/contract_state.json	Updates sample backup state with `protocol`.

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

gilcu3 · 2026-05-04T08:37:09Z

        let mut new_registry = self.clone();
        for domain in domains {
-            let new_domain_id = new_registry.add_domain(domain.curve, domain.purpose);
+            validate_curve_protocol_consistency(&domain)?;


Makes sense, but better to do it inside the curve protocol consistency

gilcu3 · 2026-05-04T08:36:06Z

+#[serde(from = "DomainConfigCompat")]
 pub struct DomainConfig {
    pub id: DomainId,
    pub curve: Curve,
+    pub protocol: Protocol,
    pub purpose: DomainPurpose,
 }

+// `Deserialize` is implemented manually via this compat struct so that JSON
+// emitted by older contracts (pre-`protocol` field) still deserializes:
+// the missing `protocol` is inferred from `curve`.
+#[derive(Deserialize)]
+struct DomainConfigCompat {


I think this is fine, as the compact struct is just transitional, to be removed in the next release

…m-to-mpc-primitives

pbeza · 2026-05-04T09:21:59Z

+/// Validates that `domain.curve` matches the curve derived from
+/// `domain.protocol`. Once dispatch is rewired to read `protocol` directly,
+/// every entry point must reject inconsistent pairs so that stored state is
+/// authoritative.
+///
+/// The legacy `Curve::V2Secp256k1` value is accepted as a stand-in for
+/// `(Secp256k1, DamgardEtAl)`.
+// TODO(#2442): drop the V2Secp256k1 carve-out once that variant is removed
+// from `Curve` in the next PR.
+pub fn validate_curve_protocol_consistency(domain: &DomainConfig) -> Result<(), Error> {
+    let expected = Curve::from(domain.protocol);
+    let consistent = domain.curve == expected
+        || (domain.curve == Curve::V2Secp256k1 && domain.protocol == Protocol::DamgardEtAl);
+    if !consistent {
+        return Err(DomainError::InconsistentCurveProtocol {
+            curve: domain.curve,
+            protocol: domain.protocol,
+            expected,
+        }
+        .into());
+    }
+    Ok(())
+}
+


Small suggestion, not blocking — I’m not too familiar with this part of the code, but could we drop curve from DomainConfig entirely and just keep protocol, exposing curve as a derived accessor like domain.curve()?

IIUC, every protocol already maps to exactly one curve, so validate_curve_protocol_consistency and InconsistentCurveProtocol would just go away (and so would is_valid_curve_for_purpose).

I’m just trying to figure out a more idiomatic way to avoid extra runtime checks for config consistency.

Yeah I think those would go away according to the design.

Also related, I will change this a bit to address this comment

kevindeforth

Thank you!

…m-to-mpc-primitives

gilcu3 force-pushed the 3081-add-protocol-enum-to-mpc-primitives branch from 934c7c3 to 76dac2d Compare April 30, 2026 09:47

gilcu3 marked this pull request as ready for review April 30, 2026 09:54

Base automatically changed from 3083-remove-pre-390-serde-aliases-scheme-ed25519 to main April 30, 2026 10:10

gilcu3 force-pushed the 3081-add-protocol-enum-to-mpc-primitives branch from 76dac2d to dffe2e9 Compare April 30, 2026 10:12

gilcu3 linked an issue Apr 30, 2026 that may be closed by this pull request

Add Protocol enum to mpc-primitives #3081

Closed

gilcu3 force-pushed the 3081-add-protocol-enum-to-mpc-primitives branch 3 times, most recently from 0cfc4e1 to 46f9e95 Compare April 30, 2026 11:29

gilcu3 marked this pull request as draft April 30, 2026 11:29

feat: add Protocol enum

1cccaff

gilcu3 force-pushed the 3081-add-protocol-enum-to-mpc-primitives branch from 46f9e95 to 1cccaff Compare April 30, 2026 11:36

gilcu3 marked this pull request as ready for review April 30, 2026 11:41

fix: add new parameter in assets/docs

21cef4f

gilcu3 changed the title ~~feat: add Protocol enum~~ feat: add Protocol enum in DomainConfig Apr 30, 2026

gilcu3 mentioned this pull request May 4, 2026

Add ReconstructionThreshold, update DistributedKeyConfig #2663

Open

pbeza requested a review from Copilot May 4, 2026 08:30

Copilot started reviewing on behalf of pbeza May 4, 2026 08:31 View session

Copilot AI reviewed May 4, 2026

View reviewed changes

gilcu3 added 2 commits May 4, 2026 11:26

fix: address copilot's review

f4262bd

Merge remote-tracking branch 'origin/main' into 3081-add-protocol-enu…

82b6d91

…m-to-mpc-primitives

pbeza previously approved these changes May 4, 2026

View reviewed changes

gilcu3 dismissed pbeza’s stale review via 82b6d91 May 4, 2026 09:35

pbeza previously approved these changes May 4, 2026

View reviewed changes

gilcu3 dismissed pbeza’s stale review via 224a5f6 May 4, 2026 10:39

fix: adapt test to new checks

b383968

gilcu3 force-pushed the 3081-add-protocol-enum-to-mpc-primitives branch from 224a5f6 to b383968 Compare May 4, 2026 10:41

pbeza previously approved these changes May 4, 2026

View reviewed changes

kevindeforth previously approved these changes May 4, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into 3081-add-protocol-enu…

5e5aa7d

…m-to-mpc-primitives

gilcu3 dismissed stale reviews from kevindeforth and pbeza via 5e5aa7d May 4, 2026 15:19

gilcu3 enabled auto-merge May 4, 2026 15:20

pbeza approved these changes May 4, 2026

View reviewed changes

kevindeforth approved these changes May 4, 2026

View reviewed changes

gilcu3 added this pull request to the merge queue May 4, 2026

Merged via the queue into main with commit 8665fdb May 4, 2026
13 checks passed

gilcu3 deleted the 3081-add-protocol-enum-to-mpc-primitives branch May 4, 2026 17:38

This was referenced May 4, 2026

Improve Claude PR review thoroughness in CI #3116

Closed

refactor: delete v2secp256k1 and add Protocol enum #2673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add Protocol enum in DomainConfig#3085

feat: add Protocol enum in DomainConfig#3085
gilcu3 merged 6 commits into
mainfrom
3081-add-protocol-enum-to-mpc-primitives

gilcu3 commented Apr 30, 2026 •

edited

Loading

Uh oh!

claude Bot commented Apr 30, 2026

Uh oh!

claude Bot commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

gilcu3 May 4, 2026

Uh oh!

gilcu3 May 4, 2026

Uh oh!

pbeza May 4, 2026 •

edited

Loading

Uh oh!

gilcu3 May 4, 2026

Uh oh!

gilcu3 May 4, 2026

Uh oh!

kevindeforth left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

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

Uh oh!

Uh oh!

claude Bot commented Apr 30, 2026

Review - PR #3085: Add Protocol enum

Uh oh!

claude Bot commented Apr 30, 2026

Code Review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

gilcu3 May 4, 2026

Choose a reason for hiding this comment

Uh oh!

gilcu3 May 4, 2026

Choose a reason for hiding this comment

Uh oh!

pbeza May 4, 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!

gilcu3 May 4, 2026

Choose a reason for hiding this comment

Uh oh!

gilcu3 May 4, 2026

Choose a reason for hiding this comment

Uh oh!

kevindeforth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gilcu3 commented Apr 30, 2026 •

edited

Loading

pbeza May 4, 2026 •

edited

Loading