Integrate ecdsa

[andrew-fleming]

WIP

component	repo has now	ECDSA-capable (available today, pre-release)	anticipated stable
compactc (compiler)	0.31.x line (repo compact toolchain)	0.33.0-rc.2	0.33.0
compact-runtime	0.16.0	0.18.0-rc.1	0.18.0
midnight-js-*	4.1.1	5.0.0-beta.6	5.0.0
compact-js	—	2.5.5-rc.7	~2.5.5
ledger-v9 (JS) @midnightntwrk/ledger-v9	(uses ledger-v8@8.1.0)	1.0.0-rc.3	1.0.0
onchain-runtime-v4 @midnightntwrk/onchain-runtime-v4	—	4.0.0-rc.3	4.0.0
node / ledger (on-chain)	mainnet = 1.0.x / ledger-8	node 2.0.0-rc.4 / ledger 9.1.0.0-rc.3	node 2.0.0 / ledger 9.1.0.0

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e83a949-5c95-4dbc-84b0-72789f61d858

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[HarleysCodes]

Hermes Agent Review — OpenZeppelin/compact-contracts #713

Verdict: REQUEST_CHANGES — the ECDSA wiring itself looks clean and the test scaffolding is substantial, but the PR ships against toolchain versions that aren't in the repo's compact workspace yet. Holding until that's pinned to a reproducible state.

🔴 Critical

1. Toolchain mismatch — repo can't build the PR as-described.

The PR body lists compactc 0.33.0-rc.2, compact-runtime 0.18.0-rc.1, midnight-js-* 5.0.0-beta.6, and ledger-v9 1.0.0-rc.3 as the required ECDSA-capable stack. But contracts/package.json only bumps @openzeppelin/compact-simulator to a portal: path — it doesn't pin the compiler/runtime/midnight-js versions that the contract actually needs. Without that pin, anyone cloning the PR gets the repo's published stable toolchain (compactc 0.31.x, compact-runtime 0.16.0, midnight-js 4.1.1) and the new secp256k1EcdsaVerify / keccak256 calls won't resolve.

Two options:

• (a) Add the ECDSA-capable versions as a yarn workspace override (resolutions block) so yarn install produces a buildable tree.
• (b) Document the exact compactc binary and compact-runtime source commit needed to build locally, in contracts/README.md or a top-level BUILD.md.

I'd take (a) — makes CI reproducible and removes a foot-gun for downstream consumers of @openzeppelin/compact-contracts.

⚠️ Warnings

2. SignerCommitmentInput struct changed — off-chain commitment code must change in lockstep.

The diff splits pk: Bytes<64> into pkX: Bytes<32> + pkY: Bytes<32>. Any off-chain signer that was building SignerCommitmentInput { pk, salt, domain } (or any SDK / dApp / fixture computing the commitment client-side) will now produce a different commitment hash and silently fail Signer_assertSigner. The PR updates the in-circuit docstring to point at _calculateSignerId, but I don't see a companion PR updating callers. Worth either:

• Listing known callers in the PR description (so reviewers can flag missing migrations), or
• Filing a follow-up issue for "audit and migrate off-chain commitment producers to pkX/pkY split."

💡 Suggestions

3. _calculateSignerId over Secp256k1Point — serialization determinism check.

_calculateSignerId(pubkey: Secp256k1Point, ...) feeds Secp256k1Point into a hash. Worth a one-line docstring comment confirming the Secp256k1Point → bytes serialization is canonical across SDK versions (x || y, big-endian, fixed-width). If the serialization ever changes between compact-runtime 0.18.x patch versions, every commitment changes and existing signers get locked out. A defensive comment costs nothing.

4. Duplicate-detection via != on adjacent commitments — pre-existing, but now blocking merge.

The V2 contract still has the comment: "Duplicate detection via != only works for exactly 2 signers. Production contracts with larger signer sets need a different approach." This was deferred when the code was stubbed. Now that real ECDSA is wired and this PR is the natural merge point, worth a [NICE TO HAVE] follow-up issue so the limit isn't silently inherited.

5. package.json portal path is correct, but loses the version floor.

-    "@openzeppelin/compact-simulator": "^0.2.0",
+    "@openzeppelin/compact-simulator": "portal:../../compact-tools-pub/packages/simulator",

A portal: reference resolves to whatever's at that path's package.json#version — which now isn't a fixed ^0.2.0. If compact-tools-pub advances to 0.3.0 with breaking changes, this PR's test suite will silently pick them up. Worth pinning the simulator version in the path-repo's package.json or adding a comment explaining the intentional float.

✅ Looks Good

• secp256k1EcdsaVerify(state.msgHash, signature, pubkey) replaces the stubVerifySignature cleanly — the contract signatures and comment updates are consistent across both V2 and V3.
• keccak256 swap from persistentHash is mechanical and well-documented.
• Test scaffolding is substantial: EcdsaTestUtils.ts (185 LOC), V2 test (+106/-42), V3 test (+237/-194). Real coverage for the ECDSA path, not just smoke tests.
• Simulator updates in both V2 and V3 are small and consistent.

---

Reviewed by Hermes Agent. Happy to take option (a) on item 1 myself if you'd prefer that lands in this PR rather than a follow-up — say the word and I'll push a commit.