feat: add eddsa MPCv2 SMC utils#8845
Conversation
Marzooqa
left a comment
There was a problem hiding this comment.
Overall structure is correct — the 2-round key-gen flow (no round 3) is intentionally different from ECDSA because MPS EdDSA DKG only requires 2 BitGo rounds. uploadClientKeys is a faithful copy of the ECDSA version. Three issues below; one is a potential runtime failure on every call.
No tests added. The ECDSA MPCv2SMCUtils also has no unit tests, but this is still a gap — at minimum keyGenRound1BySender and keyGenRound2BySender should have state-assertion failure tests and a happy-path test using a stub sender.
| tssVersion: payload.tssVersion, | ||
| walletType: payload.walletType, | ||
| coin: payload.coin, | ||
| ovc: payload.ovc, |
There was a problem hiding this comment.
Bug: unsafe as cast hiding a type definition gap.
EddsaMPCv2KeyGenRound1Response is typed as { sessionId, bitgoMsg1 } only — walletGpgPubKeySigs is not declared on it. The cast (result as EddsaMPCv2KeyGenRound1Response & { walletGpgPubKeySigs: string }) works around this gap.
For ECDSA, MPCv2KeyGenRound1Response properly declares walletGpgPubKeySigs: NonEmptyString and is accessed directly without any cast.
Two scenarios:
- If the EdDSA
/mpc/keygen/round1API does returnwalletGpgPubKeySigs→EddsaMPCv2KeyGenRound1Responsein@bitgo/public-typesneeds to be updated to include the field, and this cast should be removed. - If the EdDSA API does not return this field →
walletGpgPubKeySigswill beundefinedat runtime. IfEddsaBitgoToOVC1Round1Response.platform.walletGpgPubKeySigsis a requiredNonEmptyString(as it is for the ECDSA equivalent),decodeOrElsewill throw"error(s) parsing response"on every call, makingkeyGenRound1BySenderpermanently broken.
Either fix the type in @bitgo/public-types or remove walletGpgPubKeySigs from the platform construction if EdDSA doesn't use it.
| backupMsg2: ovc2.ovcMsg2, | ||
| }); | ||
|
|
||
| assert.equal(sessionId, result.sessionId, 'Round 1 and round 2 session IDs do not match'); |
There was a problem hiding this comment.
Pattern inconsistency with ECDSA for sessionId in round 2.
ECDSA keyGenRound2BySender explicitly overwrites the session ID from the API result:
platform: {
...payload.platform,
sessionId: result.sessionId, // updated from result
bitgoCommitment2: ...,
}This PR asserts equality (assert.equal(sessionId, result.sessionId, ...)) and then keeps the original via ...payload.platform — the response never includes result.sessionId directly.
These two choices are consistent with each other (assert equality, then propagate original), but it's unclear if the assertion is correct. If the EdDSA round 2 API returns a different sessionId — as the ECDSA code implies is possible (since it overwrites it) — this assertion will throw unexpectedly.
Please add a comment documenting why EdDSA session IDs are stable across rounds (if that's the API contract), or switch to the ECDSA pattern and remove the assertion.
| coin: payload.coin, | ||
| ovc: payload.ovc, | ||
| platform: { | ||
| walletGpgPubKeySigs: (result as EddsaMPCv2KeyGenRound1Response & { walletGpgPubKeySigs: string }) |
There was a problem hiding this comment.
Type gap on walletGpgPubKeySigs. EddsaMPCv2KeyGenRound1Response is typed as { sessionId, bitgoMsg1 } only in @bitgo/public-types — walletGpgPubKeySigs is absent from the io-ts codec. ECDSA's MPCv2KeyGenRound1Response declares it as a required NonEmptyString and accesses it directly (result.walletGpgPubKeySigs, no cast needed).
If the EdDSA round-1 endpoint returns this field, add it to EddsaMPCv2KeyGenRound1Response in @bitgo/public-types and drop the cast. If it doesn't, walletGpgPubKeySigs will be undefined at runtime — and if EddsaBitgoToOVC1Round1Response.platform.walletGpgPubKeySigs is a required NonEmptyString (as it is in the ECDSA codec), decodeOrElse will throw "error(s) parsing response" on every call.
| const [userKeychain, backupKeychain] = await Promise.all([userKeychainPromise, backupKeychainPromise]); | ||
| return { userKeychain, backupKeychain, bitgoKeychain }; | ||
| } | ||
| } |
There was a problem hiding this comment.
No unit tests added. At minimum: (1) a state-guard failure test for each of the two assert guards (payload.state !== WaitingForBitgoRound1Data and !== WaitingForBitgoRound2Data), and (2) a happy-path test for keyGenRound1BySender and keyGenRound2BySender using a stub senderFn that returns a mock API response. ECDSA MPCv2SMCUtils also lacks tests, but that's not a reason to skip them here. Follow-up safe.
TICKET: WCI-242