feature/SDK-7 #25
feature/SDK-7 #25
Conversation
|
|
||
| export interface ICreateIdentifierOpts {} | ||
|
|
||
| export enum KeyType { |
There was a problem hiding this comment.
Don't introduce new key type enums. We already have TKeyType. If you need to restrict the current Enum, then use a Pick<>
There was a problem hiding this comment.
TKeyType is an UnionType and Pick<> doesn't work with it and according what I've read so far, it doesn't work either with enums. In addition enums are completely different from UnionTypes and the process to extract a subset of keys from them is completely different. 😕 In the end I just had to use Extract<>.
| options?: IKeyOpts | ||
| options?: { | ||
| methodSpecificId?: string // method specific id for import | ||
| secp256k1?: IKeyOpts | VerificationMethod |
There was a problem hiding this comment.
I rather see the word Key in there. So secp256k1Key. Also what is the reason for the union. The VerificiationMethod interface extends IKey, which makes the name poor BTW (should be changed). You could just as well have added the purposes property as an optional prop to the IKey interface
There was a problem hiding this comment.
For the sake of consistency I just copied snippets from did-provider-ion. I cannot add anything to the IKey interface because it comes from @veramo/core, but I could make IKeyOpts extend it directly and keeping the required privateKeyHex property.
| } | ||
|
|
||
| private assertedPurposes = (args: { keyType: KeyType; purposes: EbsiPublicKeyPurpose[] }) => { | ||
| if (args.purposes) { |
There was a problem hiding this comment.
Since purposes is required this expression would also be favored over the ones below and as it returns immediately, the rest of the function does nothing. IMO purposes should be optional. Also IMO the construct is a bit odd anyway. For EBSI you cannot say that for instance a Secp256k1 key can be used for an assertionMethod; so why would you allow that anyway. IMO this function should check whether the provided purposes are allowed for the specific key. And seperately a method should check when no purposes have been given (so undefined), then it should set the defaults.
There was a problem hiding this comment.
It makes sense, broke it in two methods as suggested
| throw Error(`Type ${options.type} not supported`) | ||
|
|
||
| if (args?.keyOpts && 'purposes' in args.keyOpts) { | ||
| keyManagerImportArgs.meta = { purposes: [...args.keyOpts.purposes] } |
There was a problem hiding this comment.
See remark about purposes. IMO there should always be purposes, as otherwise the keys have no meaning in the context of the DID
There was a problem hiding this comment.
Refactored the method and added the purpose checks as requested in one of the previous remarks
| } | ||
| } | ||
|
|
||
| export interface ICreateIdentifierOpts {} |
There was a problem hiding this comment.
What is this doing here? Should not be needed. Or at least properly list the actual options available
There was a problem hiding this comment.
Removed unused type
| throw new Error(`Unsupported key type: ${key.type}`) | ||
| } | ||
| } | ||
| return this.setDefaultPurposes({ key, type }) |
There was a problem hiding this comment.
Hmzz this is rather ugly. An assert method setting defaults instead of sticking to assertions. Either rename the method slightly or adjust the implementation
There was a problem hiding this comment.
Totally ugly 😞. Refactored: Separated assertion from setting defaults
| @@ -43,5 +41,19 @@ export interface ICreateIdentifierArgs { | |||
| kms?: string | |||
| alias?: string | |||
| type?: EbsiDidSpecInfo | |||
| options?: IKeyOpts | |||
| options?: { | |||
| methodSpecificId?: string // method specific id for import | |||
There was a problem hiding this comment.
Is this actually used? If so I am assuming for the r1 key and not the k1 key
There was a problem hiding this comment.
This is existing code, it's an id that gets asserted and appended to did:ebsi. It seems to be allowing an external did to be imported. Not attached to any key. Removed.
No description provided.