SDK-1.0.0-alpha #2347
SDK-1.0.0-alpha #2347
Conversation
|
Still need to fix the tests |
|
|
||
| const { | ||
| didDocumentMetadata: { canonicalId, deactivated }, | ||
| } = await DidResolver.resolve(lightDid, {}); |
There was a problem hiding this comment.
Isn’t the second parameter optional? I feel that it has to be.
There was a problem hiding this comment.
I also think it's weird that it's not optional, especially when it's called ResolutionOptions
There was a problem hiding this comment.
Let’s apply slight pressure to someone
There was a problem hiding this comment.
@rflechtner we are applying slight pressure to you 🙂
There was a problem hiding this comment.
That is an interesting one! I agree this is massively annoying and I'm inclined to make it optional - especially bc it is only required on the typing, but the actual implementation handles omitting it just fine. A quick look at the typings explains what feels like an oversight (Antonio built this, so I went to a similar discovery process):
resolve: (
/**
* This is the DID to resolve.
* This input is REQUIRED and the value MUST be a conformant DID as defined in 3.1 DID Syntax.
*/
did: Did,
/**
* A metadata structure containing properties defined in 7.1.1 DID Resolution Options.
* This input is REQUIRED, but the structure MAY be empty.
*/
resolutionOptions: ResolutionOptions
) => Promise<ResolutionResult>
Meaning: it is a specification thing, which is why I am unsure what to do about it. I don't see any harm in making it optional really, but for some reason it's explicitly required even if empty...
There was a problem hiding this comment.
Okay, my opinion about this specification authors has sunk a little more, nothing to see here
There was a problem hiding this comment.
I think we don't need to be sticklers about it really, I'll put it in my list of improvements
There was a problem hiding this comment.
Not an oversight, but compliance to the resolution spec. We could make it optional, I agree.
| import { HexString } from '@polkadot/util/types'; | ||
|
|
||
| import { DAppName } from '../../dApps/AccessChannels/DAppName'; | ||
| import { Origin } from '../../dApps/AccessChannels/Origin'; | ||
|
|
||
| export type CreateDidInput = DAppName & { | ||
| submitter: KiltAddress; | ||
| pendingDidUri?: DidUri; | ||
| pendingDidUri?: Did; |
There was a problem hiding this comment.
Would changing pendingDidUrl to pendingDid following the new naming convention of the SDK break anything?
There was a problem hiding this comment.
Unless it's also changed in all the apps that use this API
| }; | ||
|
|
||
| export type SignDidOriginInput = SignDidInput & Origin; | ||
|
|
||
| export interface SignDidOutput { | ||
| signature: HexString; | ||
| didKeyUri: DidResourceUri; | ||
| didKeyUri: DidUrl; |
There was a problem hiding this comment.
Just a heads-up, mind that the new definition of DidUrl can include optional query parameters, so something like did:kilt:4....?param=value#keyId would now be a valid URL. They can of course simply be ignored, but I wanted to raise awareness that that's also possible now.
| ); | ||
| const dAppEncryptionDidKey = contentStream as VerificationMethod | undefined; | ||
|
|
||
| if (dAppEncryptionDidKey?.type !== 'Multikey') { |
There was a problem hiding this comment.
@arty-name what would the convenience method do?
|
|
||
| const { | ||
| didDocumentMetadata: { canonicalId, deactivated }, | ||
| } = await DidResolver.resolve(lightDid, {}); |
There was a problem hiding this comment.
Not an oversight, but compliance to the resolution spec. We could make it optional, I agree.
No description provided.