feat: support multicurve #304
Conversation
packages/core/src/metadata.ts
Outdated
| secp256k1: TssMetadata; | ||
| ed25519: TssMetadata; |
There was a problem hiding this comment.
Make this a Record of KeyType and TssMetadata instead of fixing the entries here?
tssKeyType from json is undefined
| serverEncs: EncryptedMessage[]; | ||
| }; | ||
|
|
||
| export interface ITssMetadata { |
There was a problem hiding this comment.
I think it would be good if we can provide some comments for the interface fields
| scopedStore, | ||
| nonce, | ||
| tss, | ||
| version, |
There was a problem hiding this comment.
Should we validate the value of version here, in case user provides values rather than "1.0.0" or "0.0.1"?
packages/core/src/metadata.ts
Outdated
| if (tss[tsstag]) { | ||
| metadata.tss[tsstag] = {}; | ||
| Object.keys(tss[tsstag]).forEach((tssKeyType) => { | ||
| metadata.tss[tsstag][tssKeyType] = TssMetadata.fromJSON(tss[tsstag][tssKeyType]); |
There was a problem hiding this comment.
Should we do further validations here too?
The value tss is any as specified in type StringifiedType.
There was a problem hiding this comment.
We should check whether tssKeyType is a valid key type, right?
There was a problem hiding this comment.
Not possible, as legacy tkey tss metadata might not have tssKeyType defined
It is possible for new format.
will try to add keytype for new format
There was a problem hiding this comment.
Added checking of instance of object and check for tssKeyTypes
| tssNonces, | ||
| factorPubs, | ||
| factorEncs, | ||
| } = value; |
There was a problem hiding this comment.
We should do some validations here, too.
The input is { [key: string] : any } type
matthiasgeihs
left a comment
matthiasgeihs
left a comment
There was a problem hiding this comment.
this is a big PR with a lot of changes.
many well done.
however, there is some things that look unfinished.
there is a lot of seemingly leftover commented out code and todo comments.
moreover, the package.json points to a local torus.js dependency.
these things need to be tidied up before this can be merged.
there is also valuable comments by others that need to be addressed.
PS:
for the future, i'd suggest keeping a PR in draft as long as local dependencies are used, so that it's clear that this is not ready to be merged.
|
|
||
| static fromJSON(value: StringifiedType): AuthMetadata { | ||
| const { data, sig } = value; | ||
| // need to inject legacyMetadata flag |
There was a problem hiding this comment.
is this comment a TODO? or can be removed?
There was a problem hiding this comment.
not a todo.
It remind that for tkeyTss deserialization, legacyMetadata Flag needed to be injected as we are trying to force upgrade the metadata format to v1.0.0 from v0.0.1 when the legacyMetadataFlag is false
There was a problem hiding this comment.
not sure what this means. where do you inject the flag?
There was a problem hiding this comment.
Feat/multicurve tag
add tests failing reason remove `secp256k1` comment fix typo
remove not needed check
remove unsued comment or code
serialize metadata.tss
fix: add stringified value check
No description provided.