refactor: unify duplicate hash newtype macros into a single generic `HashDigest<S, N>` type in `primitives` by pbeza · Pull Request #2619 · near/mpc

pbeza · 2026-03-25T17:29:20Z

Replace three separate hash_newtype! / digest_newtype! macros (in primitives, foreign-chain-inspector, and contract/tee/measurements) with a single generic HashDigest<S, N> struct and a define_hash! macro in mpc-primitives. This eliminates code duplication across crates while supporting arbitrary byte lengths and consistent serde/borsh/schema implementations.

Addresses #2597

…HashDigest<S, N>` type in `primitives` Replace three separate `hash_newtype!` / `digest_newtype!` macros (in `primitives`, `foreign-chain-inspector`, and `contract/tee/measurements`) with a single generic `HashDigest<S, N>` struct and a `define_hash!` macro in `mpc-primitives`. This eliminates code duplication across crates while supporting arbitrary byte lengths and consistent serde/borsh/schema implementations. Addresses #2597

…hash_newtype-macros-across-primitives-and-foreign-chain-inspector

Copilot

Pull request overview

This PR consolidates multiple per-crate “hash newtype” macros into a single generic HashDigest<S, N> implementation in mpc-primitives, and migrates downstream crates to use the shared define_hash! macro.

Changes:

Introduces generic HashDigest<S, N> + exported define_hash! macro in mpc-primitives and removes the old hash_newtype! macro.
Replaces local hash-newtype macros/usages in foreign-chain-inspector, foreign-chain-rpc-interfaces, and contract/tee/measurements with mpc_primitives::define_hash!.
Updates crate dependencies accordingly (adds paste, removes now-unused hex/serde_with in some crates) and removes some unnecessary .clone() calls enabled by Copy hashes.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/primitives/src/lib.rs	Adds hidden `_macro_deps` re-exports used by `define_hash!`.
crates/primitives/src/hash.rs	Implements `HashDigest<S, N>`, `HashSpec`, and the exported `define_hash!` macro; migrates concrete hash types.
crates/primitives/Cargo.toml	Adds `paste` dependency; removes `serde_with`.
crates/mpc-attestation/src/attestation.rs	Removes redundant clones when embedding hash types (now `Copy`).
crates/foreign-chain-rpc-interfaces/src/bitcoin.rs	Replaces local hash macro with `mpc_primitives::define_hash!` for transport hashes.
crates/foreign-chain-rpc-interfaces/Cargo.toml	Drops direct `hex` dependency; adds `mpc-primitives`.
crates/foreign-chain-inspector/src/starknet.rs	Switches Starknet hash newtypes to `mpc_primitives::define_hash!`.
crates/foreign-chain-inspector/src/lib.rs	Removes the now-unused internal `hash` module.
crates/foreign-chain-inspector/src/hash.rs	Deletes the duplicate hash helper/macro implementation.
crates/foreign-chain-inspector/src/bitcoin.rs	Switches Bitcoin hash newtypes to `mpc_primitives::define_hash!`.
crates/foreign-chain-inspector/src/abstract_chain.rs	Switches abstract-chain hash newtypes to `mpc_primitives::define_hash!`.
crates/foreign-chain-inspector/Cargo.toml	Removes `hex`/`serde` from main deps; keeps serde in dev-deps for tests.
crates/contract/src/tee/proposal.rs	Removes redundant clones enabled by `Copy` hash types.
crates/contract/src/tee/measurements.rs	Replaces local 48-byte digest macro with `mpc_primitives::define_hash!`.
crates/contract/src/lib.rs	Removes redundant clones when voting with hash types (now `Copy`).
Cargo.toml	Adds workspace `paste` dependency and reformats `rustls` entry.
Cargo.lock	Updates lockfile for dependency changes (`paste`, removed `hex`/`serde_with` in some crates).

Comments suppressed due to low confidence (1)

crates/primitives/src/hash.rs:310

TestHashSpec / TestHash48Spec implement HashSpec, but HashSpec has supertraits borsh::BorshSerialize + borsh::BorshDeserialize, so these specs must also implement those borsh traits. As written, this test module will not compile. Either switch the tests back to using define_hash! (which generates the needed impls), or add minimal BorshSerialize/BorshDeserialize impls for the spec structs.

    define_hash!(TestHash, 32);
    define_hash!(TestHash48, 48);

    #[test]
    fn test_from_bytes_array() {
        let bytes = [1u8; 32];
        let hash: TestHash = bytes.into();
        assert_eq!(*hash, bytes);
    }

    #[test]

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

Copilot · 2026-03-25T17:35:51Z

crates/primitives/src/hash.rs

+
+impl<S: HashSpec<N>, const N: usize> core::fmt::Debug for HashDigest<S, N> {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "{}({})", S::NAME, self.as_hex())


Debug for HashDigest calls self.as_hex(), which allocates a String (via hex::encode) on every debug format. These hash types are logged with {:?} in contract code paths (e.g. voting), so this can add avoidable allocation/gas overhead. Consider implementing Debug without allocating (e.g., debug the raw byte array or write hex directly to the formatter without building an intermediate String).

Suggested change

write!(f, "{}({})", S::NAME, self.as_hex())

// Avoid allocating a `String` on every debug format by encoding into a

// stack buffer and writing the hex directly to the formatter.

let mut buf = [0u8; 2 * N];

hex::encode_to_slice(self.bytes, &mut buf).map_err(|_| core::fmt::Error)?;

let hex_str = core::str::from_utf8(&buf).map_err(|_| core::fmt::Error)?;

write!(f, "{}({})", S::NAME, hex_str)

Copilot · 2026-03-25T17:35:52Z

crates/primitives/src/hash.rs

+/// Marker trait binding a hash type name to a specific byte length.
+///
+/// Each concrete hash type defines a zero-sized spec struct and implements this trait
+/// for exactly one `N`. This prevents constructing a `HashDigest<S, WRONG_N>` — the
+/// compiler rejects it because the trait bound `S: HashSpec<WRONG_N>` is not satisfied.
+pub trait HashSpec<const N: usize>: borsh::BorshSerialize + borsh::BorshDeserialize {
+    const NAME: &'static str;
+}


HashSpec is defined with borsh::BorshSerialize + borsh::BorshDeserialize supertraits, which forces every crate using HashDigest/define_hash! to pull in and compile borsh even if they only need serde (e.g. RPC/interface crates). This also conflicts with the PR description/issue acceptance criteria about not forcing borsh on off-chain crates. Consider removing the borsh supertraits from HashSpec (since HashDigest’s borsh impls don’t depend on S) and/or feature-gating the borsh impls/dependency in mpc-primitives.

Copilot · 2026-03-25T17:35:52Z

crates/primitives/src/hash.rs

+macro_rules! define_hash {
+    ($(#[$meta:meta])* $name:ident, $n:literal) => {
+        $crate::_macro_deps::paste::paste! {
+            #[doc = concat!("Spec for [`", stringify!($name), "`].")]


define_hash! generates a public <Name>Spec struct for every hash type. This adds extra public types to downstream crate APIs (e.g. TransportBitcoinBlockHashSpec) that are likely not meant for consumers. Consider marking the spec struct #[doc(hidden)] (or similar) so it doesn’t pollute generated docs while still satisfying Rust privacy for the public type alias.

Suggested change

#[doc = concat!("Spec for [`", stringify!($name), "`].")]

#[doc(hidden)]

claude · 2026-03-25T17:42:14Z

Code Review

Overall: Clean deduplication refactor. The generic HashDigest<S, N> approach with HashSpec marker trait is well-structured, and making the type Copy is a nice ergonomic win. A couple of issues:

Issue 1: Test specs missing borsh impls — likely compile error

HashSpec<N> requires BorshSerialize + BorshDeserialize as supertraits:

pub trait HashSpec<const N: usize>: borsh::BorshSerialize + borsh::BorshDeserialize { ... }

But the manually-defined test specs at crates/primitives/src/hash.rs:300-309 don't implement those traits:

struct TestHashSpec;
impl HashSpec<32> for TestHashSpec {  // ← should fail: TestHashSpec doesn't impl BorshSerialize
    const NAME: &'static str = "TestHash";
}

Fix options (in order of preference):

Remove the supertraits — HashDigest's borsh impls don't delegate to the spec's borsh, so the bounds are unnecessary. Change to pub trait HashSpec<const N: usize> { ... }
Add #[derive(borsh::BorshSerialize, borsh::BorshDeserialize)] to the test specs
Use define_hash! in the test module too (it generates the borsh impls)

Issue 2: Spurious borsh bounds on `HashSpec` (related)

Even if the test code is fixed with option 2/3, the BorshSerialize + BorshDeserialize supertraits on HashSpec serve no functional purpose — the HashDigest borsh impls only serialize self.bytes, never anything on S. These bounds just leak implementation details into the public trait and force unnecessary impls on every spec. Option 1 above cleans this up.

⚠️ Issues found — the first issue likely prevents test compilation.

…neric `HashDigest<S, N>` type in `primitives`

pbeza · 2026-03-25T18:22:07Z

@gilcu3 @netrome I won’t be able to polish this PR this week, but it should be in pretty good shape for review already — would appreciate you taking a look when you get a chance. I’ll wrap it up next week once I’m back from OOO. Thanks!

…hash_newtype-macros-across-primitives-and-foreign-chain-inspector

gilcu3

Left some comments, we can continue when you are back

gilcu3 · 2026-03-26T07:26:21Z

crates/contract/tests/inprocess/attestation_submission.rs

            .iter()
-            .map(|hash| *hash.clone())
+            .map(|hash| **hash)


nit: is it possible to use into_iter here to avoid the double *?

gilcu3 · 2026-03-26T07:31:43Z

crates/primitives/src/hash.rs

+    }
+}
+
+// -- Debug -------------------------------------------------------------------


nit: lets remove these comments

gilcu3 · 2026-03-26T07:33:06Z

crates/primitives/src/hash.rs

This turned out to be much harder than I thought. When you are back, could you mention why it requires this machinery?

pbeza linked an issue Mar 25, 2026 that may be closed by this pull request

Unify duplicate hash_newtype! macros across primitives and foreign-chain-inspector #2597

Open

4 tasks

pbeza marked this pull request as ready for review March 25, 2026 17:29

Copilot AI review requested due to automatic review settings March 25, 2026 17:29

Copilot started reviewing on behalf of pbeza March 25, 2026 17:30 View session

pbeza force-pushed the 2597-unify-duplicate-hash_newtype-macros-across-primitives-and-foreign-chain-inspector branch from 227d139 to d3fada7 Compare March 25, 2026 17:34

Merge remote-tracking branch 'origin/main' into 2597-unify-duplicate-…

90f62d6

…hash_newtype-macros-across-primitives-and-foreign-chain-inspector

Copilot AI reviewed Mar 25, 2026

View reviewed changes

pbeza added 2 commits March 25, 2026 19:12

fixup! refactor: unify duplicate hash newtype macros into a single ge…

1d81511

…neric `HashDigest<S, N>` type in `primitives`

cargo insta update

0e9a54b

pbeza requested review from gilcu3 and netrome March 25, 2026 18:20

Merge remote-tracking branch 'origin/main' into 2597-unify-duplicate-…

c3dd7d4

…hash_newtype-macros-across-primitives-and-foreign-chain-inspector

pbeza requested review from anodar and kevindeforth March 25, 2026 19:30

gilcu3 reviewed Mar 26, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: unify duplicate hash newtype macros into a single generic `HashDigest<S, N>` type in `primitives`#2619

refactor: unify duplicate hash newtype macros into a single generic `HashDigest<S, N>` type in `primitives`#2619
pbeza wants to merge 5 commits intomainfrom
2597-unify-duplicate-hash_newtype-macros-across-primitives-and-foreign-chain-inspector

pbeza commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

Copilot AI Mar 25, 2026

Uh oh!

claude bot commented Mar 25, 2026

Uh oh!

pbeza commented Mar 25, 2026

Uh oh!

gilcu3 left a comment

Uh oh!

gilcu3 Mar 26, 2026

Uh oh!

gilcu3 Mar 26, 2026

Uh oh!

gilcu3 Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-        write!(f, "{}({})", S::NAME, self.as_hex())
+        // Avoid allocating a `String` on every debug format by encoding into a
+        // stack buffer and writing the hex directly to the formatter.
+        let mut buf = [0u8; 2 * N];
+        hex::encode_to_slice(self.bytes, &mut buf).map_err(|_| core::fmt::Error)?;
+        let hex_str = core::str::from_utf8(&buf).map_err(|_| core::fmt::Error)?;
+        write!(f, "{}({})", S::NAME, hex_str)

	#[doc = concat!("Spec for [`", stringify!($name), "`].")]
	#[doc(hidden)]

Conversation

pbeza commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 25, 2026

Code Review

Issue 1: Test specs missing borsh impls — likely compile error

Issue 2: Spurious borsh bounds on HashSpec (related)

Uh oh!

pbeza commented Mar 25, 2026

Uh oh!

gilcu3 left a comment

Choose a reason for hiding this comment

Uh oh!

gilcu3 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

gilcu3 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

gilcu3 Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Issue 2: Spurious borsh bounds on `HashSpec` (related)