-
Notifications
You must be signed in to change notification settings - Fork 4
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
add check to use import flow #170
Conversation
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.
some context about why this change is needed would be helpful
src/mpcCoreKit.ts
Outdated
private get useDKG(): boolean { | ||
return this.keyType === KeyType.ed25519 && this.options.useDKG === undefined ? false : this.options.useDKG; | ||
return this.keyType === KeyType.ed25519 ? false : this.options.useDKG; |
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.
what's the reasoning for disabling dkg completely for ed25519? some customers might still want it? i would think having the option is beneficial?
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.
because there is no dkg running for ed25519, realized i should fix it on torus.js level. I will push some updates
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.
got it. but we still could have the differentiation between "user imports seed" (with export capabilities) and "user generates share, server generates share" (where key never exists in one place, even though server side share is not coming from DKG)?
src/mpcCoreKit.ts
Outdated
|
||
// import flow is used when this.options.useDKG is undefined or false | ||
private get useImportFlow(): boolean { | ||
return !this.options.useDKG; |
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 it might be a bit confusing to have two different getters for the same thing (just inverted). why not just have one and remove the other one?
src/mpcCoreKit.ts
Outdated
return this.keyType === KeyType.ed25519 ? false : this.options.useDKG; | ||
} | ||
|
||
// import flow is used when this.options.useDKG is undefined or false |
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.
if useDKG
is undefined, this errors? can useDKG
actually be undefined?
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.
if user doesnt pass anything explicity in options.useDKG thn it will be undefined.
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.
then the function signature private get useDKG(): boolean
should be adjusted
I think we should throw error if useDkg is true and keyType is ed25519 |
0cf29eb
to
1956b0a
Compare
closing it will open a new PR, for torus.js updates |
No description provided.