Tests/mpc dkg#99
Conversation
mohammadalfaiyazbitgo
force-pushed
the
tests/mpc-dkg
branch
from
b9545d0 to
aa18717
Compare
mohammadalfaiyazbitgo
requested review from
Copilot and
pranavjain97
and removed request for
pranavjain97
mohammadalfaiyazbitgo
force-pushed
the
tests/mpc-dkg
branch
2 times, most recently
from
4999da5 to
5b3e4d5
Compare
| debugLogger('MPC Finalize request received'); | ||
| debugLogger('Request inputs:', { | ||
| source: req.decoded.source, | ||
| coin: req.decoded.coin, | ||
| encryptedData: req.decoded.encryptedData, | ||
| encryptedDataKey: req.decoded.encryptedDataKey, | ||
| bitgoKeyChain: req.decoded.bitgoKeyChain, | ||
| counterPartyGpgPub: req.decoded.counterPartyGpgPub, | ||
| counterPartyKeyShare: req.decoded.counterPartyKeyShare, | ||
| bitgoKeyChainKeyShares: req.decoded.bitgoKeyChain.keyShares.map((keyShare: any) => | ||
| JSON.stringify(keyShare), | ||
| ), | ||
| }); | ||
|
|
| coin: coinInstance, | ||
| verifySignatures: [ | ||
| source === 'user' ? sourceGpgPub : counterPartyGpgPub, | ||
| source === 'user' ? counterPartyGpgPub : sourceGpgPub, |
There was a problem hiding this comment.
did u mean source === 'backup here?
There was a problem hiding this comment.
no,
async verifyWalletSignatures(
userGpgPub: string,
backupGpgPub: string,
bitgoKeychain: Keychain,
decryptedShare: string,
verifierIndex: 1 | 2,
useHardcodedBitGoKeys?: {
env: EnvironmentName;
pubKeyType: 'nitro' | 'onprem';
}
): Promise<void> {
if the source is 'user' then we have to provide the source gpg key for userGpgKey, and vice versa.
| sourceIndex, | ||
| { | ||
| env: req.bitgo.env, | ||
| pubKeyType: 'onprem', |
There was a problem hiding this comment.
we were fetching constants from bitgo in the sdk method for this check, i added some changes to the SDK to use the hard coded ones if these options were given. BitGo/BitGoJS@520d70d
| debugLogger('MPC Initialize request received'); | ||
| debugLogger('Request inputs:', { source: req.decoded.source }); |
There was a problem hiding this comment.
is this helpful? also, we use logger.debug for all the logs
| ): Promise<EDDSA.KeyCombine> { | ||
| const eddsa = await bitgoSdk.Eddsa.initialize(); | ||
| return eddsa.keyCombine(...params); | ||
| } |
There was a problem hiding this comment.
why do we need a helper just for this? It might be clearer to do this at the caller
There was a problem hiding this comment.
this was because getting a mock to work on this method was next to impossible so i removed it out and mocked it.
mohammadalfaiyazbitgo
force-pushed
the
tests/mpc-dkg
branch
from
5b3e4d5 to
4ecfe36
Compare
mohammadalfaiyazbitgo
force-pushed
the
tests/mpc-dkg
branch
2 times, most recently
from
e2502dd to
5f2b647
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR implements MPC (Multi-Party Computation) Distributed Key Generation (DKG) functionality with comprehensive test coverage. The changes include adding error stack traces to response handling, refactoring MPC finalize operations to use utility functions, and introducing extensive test suites for both MPC initialization and finalization workflows.
- Enhances error debugging by including stack traces in error responses
- Refactors MPC finalize operations to use centralized utility functions for better maintainability
- Adds comprehensive test coverage for MPC initialization and finalization workflows
- Updates package dependencies to latest beta versions
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/shared/responseHandler.ts | Adds stack trace information to error responses for better debugging |
| src/api/advancedWalletManager/utils.ts | Introduces utility functions for EDDSA operations and wallet signature verification |
| src/api/advancedWalletManager/mpcInitialize.ts | Improves error handling by using specific BadRequestError |
| src/api/advancedWalletManager/mpcFinalize.ts | Refactors to use utility functions and improves parameter structure |
| src/advancedWalletManager/routers/advancedWalletManagerApiSpec.ts | Removes redundant error handling now managed by responseHandler |
| src/tests/api/master/recoveryWalletMpcV2.test.ts | Updates chain ID in test expectations |
| src/tests/api/advancedWalletManager/mpcInitialize.test.ts | Adds comprehensive test suite for MPC initialization |
| src/tests/api/advancedWalletManager/mpcFinalize.test.ts | Adds comprehensive test suite for MPC finalization |
| package.json | Updates BitGo SDK dependencies to latest beta versions |
| masterBitgoExpress.json | Updates API schema to include stack trace in error responses |
| const combinedKey = await eddsaKeyCombine([ | ||
| sourcePrivateShare, | ||
| [bitgoToSourceYShare, counterPartyToSourceYShare], | ||
| ]); |
There was a problem hiding this comment.
The parameters passed to eddsaKeyCombine don't match the function signature. The function expects Parameters<Eddsa['keyCombine']> but is receiving an array with sourcePrivateShare and an array of yShares. This should be sourcePrivateShare as first parameter and the array of yShares as second parameter, not wrapped in an outer array.
| const combinedKey = await eddsaKeyCombine([ | |
| sourcePrivateShare, | |
| [bitgoToSourceYShare, counterPartyToSourceYShare], | |
| ]); | |
| const combinedKey = await eddsaKeyCombine( | |
| sourcePrivateShare, | |
| [bitgoToSourceYShare, counterPartyToSourceYShare], | |
| ); |
mohammadalfaiyazbitgo
force-pushed
the
tests/mpc-dkg
branch
3 times, most recently
from
086ee67 to
24bb820
Compare
pranavjain97
left a comment
pranavjain97
left a comment
There was a problem hiding this comment.
lgtm; just fix CI and double check the copilot comment
mohammadalfaiyazbitgo
force-pushed
the
tests/mpc-dkg
branch
from
24bb820 to
5519d8c
Compare
- added tests for mpc finalize - added tests for mpc initalize Ticket: WP-5338
mohammadalfaiyazbitgo
force-pushed
the
tests/mpc-dkg
branch
from
5519d8c to
bd3baa1
Compare
No description provided.