Skip to content

Wp eddsa keygen#35

Merged
mohammadalfaiyazbitgo merged 17 commits intomasterfrom
WP-eddsa-keygen
Jun 26, 2025
Merged

Wp eddsa keygen#35
mohammadalfaiyazbitgo merged 17 commits intomasterfrom
WP-eddsa-keygen

Conversation

@alextse-bg
Copy link
Copy Markdown
Contributor

No description provided.

Comment on lines +199 to +248
'v1.mpc.eddsa.initialize': {
post: httpRoute({
method: 'POST',
path: '/{coin}/mpc/initialize',
request: httpRequest({
params: { coin: t.string },
body: MpcInitializeRequest,
}),
response: MpcInitializeResponse,
description: 'Initialize MPC for EdDSA key generation',
}),
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems duplicate

description: 'Initialize MPC for EdDSA key generation',
}),
},
'v1.key.mpc.init': {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'v1.key.mpc.init': {
'v1.mpc.key.init': {

description: 'Initialize Eddsa key generation',
}),
},
'v1.mpc.finalize': {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'v1.mpc.finalize': {
'v1.mpc.key.finalize': {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to keep it consistent with v1.multisig.sign nomenclature

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^

Comment thread src/api/enclaved/eddsaInitialize.ts Outdated
Buffer.from(keyShare.yShares[3].chaincode, 'hex'),
]).toString('hex');

const bitgoKeyShare = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const bitgoKeyShare = {
const sourceToBitGoKeyShare = {

Comment thread src/api/enclaved/eddsaInitialize.ts Outdated
Buffer.from(keyShare.yShares[2].chaincode, 'hex'),
]).toString('hex');

const counterPartyKeyShare = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const counterPartyKeyShare = {
const sourceToCounterPartyKeyShare = {

Comment thread src/api/enclaved/eddsaInitialize.ts Outdated
from: source,
to: source === 'user' ? 'backup' : 'user',
publicShare: publicKeyShare,
privateShare: gpgEncrypt(counterPartyPrivateKeyShare, counterPartyGpgPub),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure you'll have the backup's gpg key in this case? If this is dynamic, woudn't we need a 3rd round since mbe will first need backup's gpg key?

Comment thread src/api/enclaved/eddsaFinalize.ts Outdated
}

await eddsaUtils.verifyWalletSignatures(
userGpgKey.publicKey,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should use userKey instead of sourceKey, don't know if that will mess up flow

Copy link
Copy Markdown
Contributor

@pranavjain97 pranavjain97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Just requires some polishing

bitgoAddWalletNock.done();
});

// it('should generate a TSS wallet by calling the enclaved express service', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get atleast one happy path integration test in with any new feature

Comment on lines +31 to +33
const sourceIndex = source === 'user' ? 1 : 2;
const counterPartyIndex = source === 'user' ? 2 : 1;
const bitgoIndex = 3;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use KeyIndices from the sdk here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will follow up on

Comment on lines +37 to +42
const previousState = JSON.parse(
req.bitgo.decrypt({
input: encryptedData,
password: decryptedDataKey.plaintextKey,
}),
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could we type this?

Comment on lines +91 to +97
// Log the constructed keychain for verification
debugLogger('Constructed keychain:', {
sourcePrivateShare,
bitgoToSourceKeyShare,
counterPartyToSourceKeyShare,
commonKeychain: bitgoKeyChain.commonKeychain,
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this, or not log private shares

@@ -0,0 +1,150 @@
import debug from 'debug';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move these files to api/enclaved/mpc?

description: 'Generate an independent key',
}),
},
'v1.mpc.initialize': {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'v1.mpc.initialize': {
'v1.mpc.key.initialize': {

description: 'Initialize Eddsa key generation',
}),
},
'v1.mpc.finalize': {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^

'v1.mpc.initialize': {
post: httpRoute({
method: 'POST',
path: '/api/{coin}/mpc/initialize',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path: '/api/{coin}/mpc/initialize',
path: '/api/{coin}/mpc/key/initialize',

'v1.mpc.finalize': {
post: httpRoute({
method: 'POST',
path: '/api/{coin}/mpc/finalize',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path: '/api/{coin}/mpc/finalize',
path: '/api/{coin}/mpc/key/finalize',

Comment on lines +3 to +5
export interface GenerateDataKeyParams {
keyType: 'AES-256' | 'RSA-2048' | 'ECDSA-P256';
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are u sure kms supports 'RSA-2048' | 'ECDSA-P256';? i got an error when using RSA-2048

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alextse-bg can better answer this.

Copy link
Copy Markdown
Contributor

@pranavjain97 pranavjain97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do the other changes in quick follow-up

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo merged commit 144b235 into master Jun 26, 2025
3 checks passed
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo deleted the WP-eddsa-keygen branch June 26, 2025 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants