Conversation
zahin-mohammad
left a comment
There was a problem hiding this comment.
Reviewed the EdDSA MPS implementation against the ECDSA DKLS conventions. Behavior looks correct; leaving inline notes on drifts we should close before merging. Retrofit is expected to come later — not flagged.
| ).armor(); | ||
|
|
||
| if (envRequiresBitgoPubGpgKeyConfig(this.bitgo.getEnv())) { | ||
| assert(bitgoPublicGpgKeyArmored, 'BitGo GPG public key is required'); |
There was a problem hiding this comment.
This assertion is weaker than the ECDSA equivalent in ecdsaMPCv2.ts:74 — there we call assert(isBitgoMpcPubKey(bitgoPublicGpgKey, 'mpcv2'), 'Invalid BitGo GPG public key'). Here we only assert the armored key is truthy, which will never fail because the ?? fallback always yields a key. In prod/test envs this should verify the key matches one of the known hardcoded BitGo MPC pubkeys. Please restore isBitgoMpcPubKey(bitgoPublicGpgKeyArmored, 'mpcv2') (or add an 'mpcv2-eddsa' variant if the key differs).
| enterprise: string; | ||
| originalPasscodeEncryptionCode?: string; | ||
| }): Promise<KeychainsTriplet> { | ||
| const userKey = await pgp.generateKey({ |
There was a problem hiding this comment.
ECDSA uses the shared generateGPGKeyPair('secp256k1') helper from ../../opengpgUtils. Please route through generateGPGKeyPair('ed25519') (extending it if needed) rather than inlining pgp.generateKey with a hard-coded userID and subkey layout. Keeps GPG key setup consistent across TSS flows and avoids divergence if we later add defaults (expiry, key flags, etc.).
| ).subarray(1); | ||
| const userSk = Buffer.from( | ||
| ((await userGpgKey.getEncryptionKey()).keyPacket as unknown as { privateParams: { d: Uint8Array } }).privateParams | ||
| .d |
There was a problem hiding this comment.
Reaching into keyPacket.publicParams.Q / privateParams.d with as unknown as {...} casts leaks openpgp internals into sdk-core. Compare to ECDSA, where key material stays inside DklsComms (sdk-lib-mpc). Please push these primitives down into sdk-lib-mpc — e.g. MPSComms.extractEd25519KeyPair(privateKey) returning { sk, pk } — so sdk-core only deals with Buffers. That also encapsulates the little-endian scalar reverse (easy to get wrong) in one place.
| import { MPSKeyGenSenderForEnterprise } from './eddsaMPCv2KeyGenSender'; | ||
|
|
||
| /** Round identifiers sent in the `round` field of each API request */ | ||
| const MPS_ROUND_1 = 'MPS-R1'; |
There was a problem hiding this comment.
ECDSA uses MPCv2KeyGenState from @bitgo/public-types as the typed round enum. String literals 'MPS-R1' / 'MPS-R2' are fine as a stopgap, but please add an MPSKeyGenState (or similar) enum to public-types so the server schema and SDK stay in lockstep. Server-side parser errors on typos will be much easier to catch.
|
|
||
| // #region keychain utils | ||
| async createParticipantKeychain( | ||
| participantIndex: MPCv2PartiesEnum, |
There was a problem hiding this comment.
Please type participantIndex as MPCv2PartyFromStringOrNumber for parity with EcdsaMPCv2Utils.createParticipantKeychain. The strict enum here means callers passing the string form (which the io-ts union permits in ECDSA) would need a cast. Keeping the same signature shape makes the EdDSA util a drop-in for callers already wired against ECDSA.
| function send<Req, Res>(round: string, payload: Req): Promise<Res> { | ||
| return bitgo | ||
| .post(bitgo.url('/mpc/generatekey', 2)) | ||
| .send({ enterprise, type: KeyGenTypeEnum.MPCv2, keyCurve: 'EdDSA', round, payload }) |
There was a problem hiding this comment.
Two things on this line:
keyCurve: 'EdDSA'is a hardcoded string. Please add aKeyCurveEnum(or equivalent) to@bitgo/public-typesand import it — same pattern asKeyGenTypeEnum.MPCv2.- The sender shape
{ round1, round2 }differs from ECDSA's genericKeyGenSenderForEnterprise<T>(bitgo, ent) → sendFn. Consider matching the ECDSA shape (single generic sender function) so that downstream consumers (custom senders, OVC, test harnesses) have a uniform extension point across curves.
There was a problem hiding this comment.
I'll add KeyCurveEnum from @bitgo/public-types as a follow-up here #8536
| TxRequest, | ||
| } from '../baseTypes'; | ||
|
|
||
| export { EddsaMPCv2Utils } from './eddsaMPCv2'; |
There was a problem hiding this comment.
ECDSA's index uses export * for ecdsaMPCv2 and ecdsaMPCv2KeyGenSender, which exposes the sender + type helpers. Here only the class is re-exported. Please also re-export EddsaMPCv2KeyGenSendFn, MPSKeyGenSenderForEnterprise, and any MPCv2 types so consumers don't have to import from deep paths.
zahin-mohammad
left a comment
There was a problem hiding this comment.
Test coverage notes — the happy path and one failure branch are covered, but several explicit invariants in the production code are untested. Inline suggestions below.
| }); | ||
|
|
||
| await assert.rejects(tssUtils.createKeychains({ passphrase: 'test', enterprise: enterpriseId })); | ||
| }); |
There was a problem hiding this comment.
This is the only failure-path test. Please add symmetric coverage for the other invariants asserted in eddsaMPCv2.ts:
- Round-2 invalid BitGo signature — mirrors this R1 test but targets
verifyMpsMessage(bitgoMsg2, ...)ateddsaMPCv2.ts:143. Currently uncovered. - Session ID mismatch —
assert.equal(sessionId, sessionIdRound2)ateddsaMPCv2.ts:134. Nock R2 to return a differentsessionIdand assertcreateKeychainsrejects. - commonPublicKey mismatch — asserts at
eddsaMPCv2.ts:153-154("User/Backup computed public key does not match BitGo common public key"). Nock R2 to return a mutatedcommonPublicKeyand assert rejection.
These are cheap to add and cover the core integrity checks that protect users from a malicious or buggy BitGo response.
| ((await backupPubKeyObj.getEncryptionKey()).keyPacket.publicParams as { Q: Uint8Array }).Q | ||
| ).subarray(1); | ||
| const bitgoSk = Buffer.from( | ||
| ((await bitgoPrvKeyObj.getDecryptionKeys())[0].keyPacket.privateParams as { d: Uint8Array }).d |
There was a problem hiding this comment.
This nock handler mirrors production's openpgp-internals extraction (keyPacket.privateParams.d, publicParams.Q, the .reverse() for little-endian scalar). That makes the test a change-detector for the same code path rather than an independent verification.
Once the extraction moves into sdk-lib-mpc (per the main-code review comment), please have the nock handler call the same MPSComms helper so the test exercises the helper too. Today, a bug in the extraction logic would pass tests because prod + test do it identically.
|
|
||
| constants = { | ||
| mpc: { | ||
| bitgoPublicKey: bitgoGpgKeyPair.publicKey, |
There was a problem hiding this comment.
bitgoPublicKey and bitgoMPCv2PublicKey are both set to the same generated key, so there's no coverage for the feature-flag fallback branch at eddsaMPCv2.ts:63-65 (getBitgoGpgPubkeyBasedOnFeatureFlags(...) ?? this.bitgoMPCv2PublicGpgKey). Consider adding a test where getBitgoGpgPubkeyBasedOnFeatureFlags returns undefined to confirm the fallback to bitgoMPCv2PublicGpgKey works and DKG still succeeds.
Relatedly, once the isBitgoMpcPubKey assertion is restored (see main-code review), please add a negative test where the BitGo pubkey doesn't match the hardcoded set — createKeychains should reject in prod/test envs.
| assert.ok(userKeychain); | ||
| assert.equal(bitgoKeychain.source, 'bitgo'); | ||
|
|
||
| assert.equal(userKeychain.id, nockedUserKeychain.id); |
There was a problem hiding this comment.
The createParticipantKeychain tests use 'a'.repeat(64) as a fake common keychain. Worth adding one assertion that the encrypted reducedEncryptedPrv for user/backup can be decrypted with the passphrase and round-trips to the original reduced private material — catches regressions in the browser-safe btoa(String.fromCharCode.apply(null, ...)) encoding path, which is easy to break and not otherwise exercised.
Summary
Implements EddsaMPCv2Utils in sdk-core — the SDK orchestrator for EdDSA DKG using the MPS protocol.
Changes
sdk-lib-mpc — PGP comms layer
sdk-core — Key generation
EddsaMPCv2Utilswith a 2-round createKeychains flow: generatesEd25519/X25519 GPGkeys for user and backup, runsEddsaMPSDkg.DKGfor each party, PGP-signs and exchanges messages over POST/mpc/generatekey(MPS-R1, MPS-R2), verifies BitGo's signed responses, cross-checks the derived commonPublicKey, then persists all three keychains.MPSKeyGenSenderForEnterprisesender withtype: MPCv2,keyCurve: EdDSA.bitgo — Unit tests
TICKET: WCI-5