[BWC/BWS] Add TSS to BWC & BWS #3895
Conversation
| const { value, algo } = params; | ||
| switch (String(algo).toUpperCase()) { | ||
| switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
The optional chaining after toUpperCase is extraneous
There was a problem hiding this comment.
Actually, it's not because algo could be a non-string if being called from JS (read: no type checking) as is the case in tests.
| const { value, algo } = params; | ||
| switch (String(algo).toUpperCase()) { | ||
| switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
extraneous optional chaining after toUpperCase
| switch (String(params?.algo).toUpperCase()) { | ||
| #getPrivKey(params: { algo?: KeyAlgorithm; } = {}) { | ||
| const { algo } = params; | ||
| switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
extraneous optional chaining after upperCase()
| switch (String(params?.algo).toUpperCase()) { | ||
| #getPrivKeyEncrypted(params: { algo?: KeyAlgorithm; } = {}) { | ||
| const { algo } = params; | ||
| switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
extraneous optional chaining after upperCase()
| switch (String(params?.algo).toUpperCase()) { | ||
| #getFingerprint(params: { algo?: KeyAlgorithm; } = {}) { | ||
| const { algo } = params; | ||
| switch (algo?.toUpperCase?.()) { |
There was a problem hiding this comment.
extraneous optional chaining after upperCase()
| supportStaffWalletId: any; | ||
| timeout: any; | ||
| r: Request; | ||
| credentials: CredT; |
There was a problem hiding this comment.
This could lead to type errors since it's initialized as null.
| @@ -59,13 +68,13 @@ export class Request { | |||
| useSession?: boolean | |||
| ) { | |||
| if (this.credentials) { | |||
| headers['x-identity'] = this.credentials.copayerId; | |||
| headers['x-identity'] = (this.credentials as Credentials).copayerId; | |||
There was a problem hiding this comment.
Should we be ensuring that this.credentials is a Credentials, and not some other 'CredT'?
| /** | ||
| * Threshold Signature Scheme (TSS) client class | ||
| * @param {ITssKeyGenConstructorParams} params Constructor parameters | ||
| * @param {EventEmitterOptions} eventOpts Options object for EventEmitter |
| $.checkArgument(params.key instanceof Key, 'key must be an instance of Key'); | ||
|
|
||
| this.#request = new Request(params.baseUrl, { | ||
| r: params.request, // For testing only |
| const prevRound = thisRound - 1; // Get previous round's messages | ||
| const { body } = await this.#request.get(`/v1/tss/keygen/${this.id}/${prevRound}`) as RequestResponse; | ||
|
|
||
| if (body.messages?.length === this.n - 1) { |
There was a problem hiding this comment.
Please add a clarifying comment above this conditional
| }, | ||
| metadata: { | ||
| id: string; | ||
| m: number; |
There was a problem hiding this comment.
n is number of participants, and m is number of required signers - right? Brief inline documentation in this file would be good, especially for ambiguous fields
There was a problem hiding this comment.
Same comment in TssKeyGen class below
There was a problem hiding this comment.
This is documented elsewhere (e.g. on input params) and m of n is the generally understood terminology for signers of participants (https://github.com/bitcoin/bips/blob/master/bip-0129.mediawiki#coordinator-1 see first bullet under Coordinator)
| }): string { | ||
| const { partyId, partyPubKey, opts } = params; | ||
| const extra = params.extra || ''; | ||
| const data = [this.id, partyId, this.m, this.n, extra].join(':'); |
There was a problem hiding this comment.
If you use the default value of extra (or if it's undefined), then you end up with a ':' at the end of your string.
const myParams = {
id: '123',
partyId: '456',
m: 3,
n: 5
};
const myStr = [myParams.id, myParams.partyId, myParams.m, myParams.n, myParams.extra].join(':');
const myStr2 = [myParams.id, myParams.partyId, myParams.m, myParams.n, ''].join(':');
console.log(myStr);
console.log(myStr2);
If that's not the intended behavior -
Suggestion:
const myJoinCodeArr = [this.id, partyId, this.m, this.n];
if (params.extra) { myJoinCodeArr.push(params.extra };
const code = myJoinCodeArr.join(':');
| privateKey: this.#requestPrivateKey, | ||
| opts | ||
| }); | ||
| return code.toString(opts?.encoding || 'hex'); |
There was a problem hiding this comment.
If opts.encoding, then is it ensured to be in your list of provided values? Would validation be useful here?
const encodingSupported = ['hex', 'base64', 'utf8', 'binary'].includes(opts?.encoding);
const encoding = encodingSupported ? opts.encoding : 'hex';
| $.checkArgument(code, 'Missing required param: code'); | ||
| $.checkArgument(typeof code === 'string' || Buffer.isBuffer(code), '`code` must be a string or buffer'); | ||
|
|
||
| code = Buffer.isBuffer(code) ? code : Buffer.from(code, opts?.encoding || 'hex'); |
There was a problem hiding this comment.
see note in createJoinCode comment above
| this.partyId = parseInt(partyId); | ||
|
|
||
| const msg = await keygen.initJoin(); | ||
| password = password || extra; |
There was a problem hiding this comment.
What should happen if password isn't provided in opts, and there's no 'more'?
It will be an empty string on the reassign - is that alright?
|
|
||
| /** | ||
| * Export the session for storage | ||
| * @returns {string} Session string |
There was a problem hiding this comment.
The string format would be nice: 'id:partyId:m:n:keygenSession'
And in the restoreSession below it.
| /** | ||
| * Threshold Signature Scheme (TSS) client class | ||
| * @param {ITssConstructorParams} params Constructor parameters | ||
| * @param {EventEmitterOptions} eventOpts Options object for EventEmitter |
| const prevRound = thisRound - 1; // Get previous round's messages | ||
| const { body } = await this.#request.get(`/v1/tss/sign/${this.id}/${prevRound}`) as RequestResponse; | ||
|
|
||
| if (body.messages?.length === this.#tssKey.metadata.m - 1) { |
There was a problem hiding this comment.
A comment would be really useful here
| If the DKG ceremony is executed asynchronously (meaning all parties are not required to be online at the same time during the DKG), the `keyGen.export()` function should be used between each step to export the local session state. The session state should be securely stored and used with `KeyGen.restore()` for following steps. | ||
|
|
||
| ### Flow Diagram | ||
|  |
There was a problem hiding this comment.
Broken link. You could change this to DKG-flow.png & it would show up in an md preview as the image.
| If the DSG ceremony is executed asynchronously (meaning all signing parties are not required to be online at the same time during the DSG), the `sign.export()` function should be used between each step to export the local session state. The session state should be securely stored and used with `Sign.restore()` for following steps. | ||
|
|
||
| ### Flow Diagram | ||
|  |
There was a problem hiding this comment.
DSG Flow. And also, as above, you can reference the DSG-flow.png from the markdown file to embed the image into it.
| <summary style="font-size:17px"><span style="font-weight:bold">POST</span> /v1/tss/keygen/:id/store</summary> | ||
|
|
||
|
|
||
| ### Request body: |
There was a problem hiding this comment.
Can't see the request body on markdown preview
| @@ -71,7 +75,7 @@ export class ExpressApp { | |||
| const POST_LIMIT_LARGE = 2 * 1024 * 1024; // Max POST 2 MB | |||
|
|
|||
| this.app.use((req, res, next) => { | |||
| if (req.path.includes('/txproposals')) { | |||
| if (req.path.includes('/txproposals') || req.path.includes('/tss/')) { | |||
There was a problem hiding this comment.
Checking: is the difference in form intended - note the trailing slash for tss
| x.copayers = _.map(obj.copayers, copayer => { | ||
| return Copayer.fromObj(copayer); | ||
| }); | ||
| x.copayers = obj.copayers?.map(copayer => Copayer.fromObj(copayer)); |
There was a problem hiding this comment.
If obj.copayers is undefined, x.copayers will be undefined.
To get array behavior, consider:
x.copayers = (obj.copayers || []).map(...) or use ternary
| } | ||
|
|
||
| async processMessage(params: { id: string; message: ITssKeyMessageObject; n?: string | number; password?: string; copayerId: string; }) { | ||
| const { id, message, n, password, copayerId } = params; |
There was a problem hiding this comment.
There's TS problem here. Here, n is typed as string | number, and no longer considered to potentially be undefined.
The documentation for this method, in the md file in this PR, also considers it optional. _initSession expects a defined value.
Invalid n gets handled in _initSession - do you want it to make it that far?
| } | ||
|
|
||
| const otherPartyMsgs = session.rounds[round].filter(m => m.fromPartyId != partyId); | ||
| if (otherPartyMsgs.length === session.n - 1) { |
There was a problem hiding this comment.
A comment as to why it's all or nothing for otherPartyMsgs length would be useful
| throw Errors.TSS_GENERIC_ERROR.withMessage('Invalid n provided: ' + n); | ||
| } | ||
|
|
||
| let passwordHash: string = null; |
There was a problem hiding this comment.
Could you make the assignment stricter by setting as an empty string, or defining the type as string | null?
Also, consider declaring as const and using ternary operator on password.
| throw Errors.TSS_INVALID_MESSAGE.withMessage('Invalid message provided'); | ||
| } | ||
|
|
||
| const isParticipant = !!session.participants.find(p => p.copayerId === copayerId && p.partyId === message.partyId); |
There was a problem hiding this comment.
Array.prototype.some is semantically more appropriate
|
|
||
| let result = false; | ||
| while (!result) { | ||
| result = await this._pushMessage({ id, session, message, storage }); |
There was a problem hiding this comment.
A comment ensuring a guarantee that we'll break out of this loop would be nice, and provide context to the work at large
This PR:
Note: I tried to be careful NOT to import the TSS classes into any existing files. The reason for this is so that it doesn't break any frontend webpack/vite/etc compilations with the underlying TSS WASM dependencies...which might be a pain to configure.