Conversation
- Add signTxRequest, signTxRequestForMessage, and signRequestBase to EddsaMPCv2Utils implementing the full online DSG protocol: WASM round 0, API rounds 1-3, and sendTxRequest - Verify PGP signatures on BitGo round1Output and round2Output - Use pickBitgoPubGpgKeyForSigning with isEddsaMpcv2=true for ed25519 key - Use decodeWithCodec for type-safe parsing of signature share responses - Add comprehensive signTxRequest unit tests covering: - tx signing (all 3 rounds + send) - message signing (all 3 rounds + send) - 429 rate-limit retry handling (up to 3 retries) - rejection after 4+ consecutive 429 errors - rejection on wrong round1Output type Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> TICKET: WCI-154
zahin-mohammad
left a comment
There was a problem hiding this comment.
Reviewed against base #8687. Built locally — tsc --noEmit clean in sdk-core, signTxRequest.ts test passes (5/5).
The orchestration faithfully mirrors ECDSA's EcdsaMPCv2Utils.signRequestBase and the protocol structure looks right: WASM rounds 0/1/2 interleaved with API rounds 1/2/3, no client-side verification on round 3 (WP finalises). Nothing here freezes public API surface — the two new public methods (signTxRequest, signTxRequestForMessage) match the existing ITSSUtils/ECDSA shape, and signRequestBase is private. So everything below is follow-up safe.
Most significant finding is the missing mpcv2PartyId plumbing — backup signing is implicitly unsupported here even though the helpers from #8687 already accept the party id. ECDSA supports it; this is a behavioral gap vs the parallel path.
See inline comments.
| } | ||
|
|
||
| // ── WASM Round 0 ────────────────────────────────────────────────────────── | ||
| const userDsg = new EddsaMPSDsg.DSG(MPCv2PartiesEnum.USER); |
There was a problem hiding this comment.
Backup-signer support gap. params.mpcv2PartyId (declared on TSSParamsWithPrv for exactly this purpose) is silently dropped. ECDSA's signRequestBase plumbs it through to both DSG init and the share helpers (see ecdsaMPCv2.ts:784, 792, 827, 845, 876, 893); here the user is hardcoded to MPCv2PartiesEnum.USER.
The helpers from #8687 already accept partyId: 0 | 1, so plumbing is mechanical:
const partyId = params.mpcv2PartyId ?? 0;
const userParty = partyId === 0 ? MPCv2PartiesEnum.USER : MPCv2PartiesEnum.BACKUP;
const userDsg = new EddsaMPSDsg.DSG(userParty);
userDsg.initDsg(userKeyShare, bufferContent, derivationPath, MPCv2PartiesEnum.BITGO);
// pass partyId through each getEddsaSignatureShareRoundN callFollow-up safe — but worth filing as a Linear ticket so it doesn't get lost. Without it, EdDSA MPCv2 backup-signer flow can't go through this path.
| * API Round 1 → send userMsg1 / recv bitgoMsg1 | ||
| * WASM Round 1 → handleIncomingMessages([userMsg1, bitgoMsg1]) (produces userMsg2) | ||
| * API Round 2 → send userMsg2 / recv bitgoMsg2 | ||
| * WASM Round 2 → handleIncomingMessagexs([userMsg2, bitgoMsg2]) (produces userMsg3) |
There was a problem hiding this comment.
Typo: handleIncomingMessagexs → handleIncomingMessages.
| txOrMessageToSign = unsignedTx.signableHex; | ||
| derivationPath = unsignedTx.derivationPath; | ||
| bufferContent = Buffer.from(txOrMessageToSign, 'hex'); | ||
| assert(txOrMessageToSign, 'Missing signableHex in unsignedTx'); |
There was a problem hiding this comment.
Assert ordering — the assert runs after Buffer.from(txOrMessageToSign, 'hex'). If signableHex is undefined, Buffer.from(undefined, 'hex') throws a TypeError first and the helpful assertion message never surfaces. Same pattern at L388-389 for the message branch. Suggest moving the assert above the Buffer.from call:
txOrMessageToSign = unsignedTx.signableHex;
assert(txOrMessageToSign, 'Missing signableHex in unsignedTx');
bufferContent = Buffer.from(txOrMessageToSign, 'hex');| ); | ||
|
|
||
| // ── WASM Round 1 ────────────────────────────────────────────────────────── | ||
| const [userMsg2] = userDsg.handleIncomingMessages([userMsg1, bitgoDeserializedMsg1]); |
There was a problem hiding this comment.
Defensive: handleIncomingMessages returns DeserializedMessages (an array). If WASM ever returns [] or an unexpected length, userMsg2 becomes undefined and the next getEddsaSignatureShareRound2(undefined, ...) crashes with a confusing PGP error. A targeted assert(userMsg2, 'WASM round 1 produced no message') after the destructure would surface the real failure. Same applies to userMsg3 at L478.
|
|
||
| const parsedBitGoToUserSigShareRoundTwo = decodeWithCodec( | ||
| EddsaMPCv2SignatureShareRound2Output, | ||
| JSON.parse(txRequestSignatureShares[txRequestSignatureShares.length - 1].share), |
There was a problem hiding this comment.
Fragile share lookup: txRequestSignatureShares[length - 1] assumes BitGo's response share is always last. ECDSA does the same thing so it's not a regression, but it would be more robust to filter explicitly:
const bitgoShare = txRequestSignatureShares.find(
(s) => s.from === SignatureShareType.BITGO && s.to === SignatureShareType.USER
);
assert(bitgoShare, 'Missing BitGo round-2 share');Follow-up safe; flagging here so the pattern doesn't propagate further.
| } | ||
|
|
||
| /** | ||
| * Signs the message associated with the transaction request. |
There was a problem hiding this comment.
Note (not blocking, pre-existing pattern): signTxRequestForMessage accepts a TSSParamsForMessageWithPrv which includes messageRaw: string and bufferToSign: Buffer, but signRequestBase ignores both — it derives bufferContent from txRequest.messages[0].messageEncoded. ECDSA does the same thing so this PR isn't introducing the issue, but it's a quiet footgun: a caller who passes bufferToSign thinking that's what gets signed will be confused when actual signing uses the txRequest's messageEncoded. Worth filing as a separate cleanup ticket against the shared TSSParamsForMessageWithPrv contract.
| .should.be.rejectedWith(/Unexpected signature share response/); | ||
| }); | ||
|
|
||
| it('successfully signs a txRequest for an mps hot wallet after receiving multiple 429 errors', async function () { |
There was a problem hiding this comment.
Coverage gap: rate-limit retries are tested for round 2 only. Rounds 1 and 3 use the same sendSignatureShareV2 retry path but aren't exercised. A parameterised helper that injects 429s on each round in turn would be cheap to add.
| nockPromises[0].isDone().should.be.true(); | ||
| nockPromises[1].isDone().should.be.true(); | ||
| nockPromises[2].isDone().should.be.true(); | ||
| nockPromises[3].isDone().should.be.true(); |
There was a problem hiding this comment.
Coverage gaps that pair with the mpcv2PartyId finding on the impl side:
- Tests only cover party 0 (USER); BACKUP path is untested.
- No round-2 tampered-signature negative test (round 1 negative is at L198, but round 2's
verifyBitGoEddsaMessageRound2could swallow tampering with no test catching it). - No test for malformed/missing round-3 server response — currently any failure there surfaces as a confusing
sendTxRequesterror rather than something localised to round 3.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
TICKET: WCI-154