-
Notifications
You must be signed in to change notification settings - Fork 11
v4: Add bip340 #193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v4: Add bip340 #193
Conversation
src/interfaces.ts
Outdated
| factorKey: string; | ||
| } | ||
|
|
||
| export enum SigType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use this type from @toruslabs/constants package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. will do that.
note:
just realized that we use the KeyType type from @tkey/common-types (not from @toruslabs/constants). not sure if that's in issue.
src/mpcCoreKit.ts
Outdated
| this._tssLib = options.tssLib; | ||
| this._keyType = options.tssLib.keyType as KeyType; | ||
| this._sigType = (() => { | ||
| if (!(options.tssLib as V4TSSLibType).sigType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this required to make sure that mpc core kit works with older tss libs which doesn't have sigType ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, basically, if we don't have the sigtype from the lib, then we derive it from the key type.
this is only needed if we want to maintain backwards compatibility. if don't need this, it's more straightforward.
you said we probably want backwards compatibility, but you didn't give a reason yet.
so, i think if we don't really need backwards compatibility, it would be better to drop it because it simplifies the code and makes it less error prone.
| * For signature type ed25519, consider using _UNSAFE_exportTssEd25519Seed. | ||
| */ | ||
| public async _UNSAFE_exportTssKey(): Promise<string> { | ||
| if (this.keyType !== KeyType.secp256k1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont we need to add corresponding sigType check for this as per function comments on top?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think so. exporting the private key scalar is a meaningful operation for all key types and sig types.
you are right that for ed25519 users probably want the seed in most cases. i'd still argue that providing the ability to export the scalar can still be useful (e.g., if the seed isn't available) and i don't see a reason why we should prevent it.
src/mpcCoreKit.ts
Outdated
| return (this._tssLib as V4TSSLibType).load(); | ||
| } else if ((this._tssLib as V3TSSLibType).lib) { | ||
| return (this._tssLib as V3TSSLibType).lib as DKLSWasmLib | FrostWasmLib; | ||
| return (this._tssLib as V3TSSLibType).lib as DKLSWasmLib | FrostWasmLibEd25519 | FrostWasmLibBip340; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can remove the support for older v3 tss libs in this version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy to drop the support for older versions. this will simplify some of the code as outlined above.
will add a commit that does so.
himanshuchawla009
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Adding support for bip340 signing.