Skip to content

feat: embedded key resolution#168

Merged
Gozala merged 18 commits intomainfrom
feat/did-mailto
Dec 14, 2022
Merged

feat: embedded key resolution#168
Gozala merged 18 commits intomainfrom
feat/did-mailto

Conversation

@Gozala
Copy link
Copy Markdown
Collaborator

@Gozala Gozala commented Dec 8, 2022

Implements embedded key resolution non did:key principals as per https://github.com/web3-storage/specs/blob/main/w3-account.md#update

@Gozala Gozala marked this pull request as ready for review December 12, 2022 09:30
@Gozala Gozala requested review from gobengo and hugomrdias December 12, 2022 09:31
Comment thread packages/interface/src/capability.ts
Comment thread packages/interface/src/lib.ts Outdated
}

export interface DIDResolver {
resolveDID?: (did: UCAN.DID) => Await<Result<DIDKey, DIDResolutionError>>
Copy link
Copy Markdown
Contributor

@gobengo gobengo Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the long term, I bristle a bit seeing the only thing that a UCAN.DID can resolve to is a single DIDKey.

What if we had

resolveDID<DidMethod>?: (did: UCAN.DID<DidMethod>) => Await<Result<DIDResolution<DidMethod>, DIDResolutionError>>

and

type SingleKeypairDidDocument<ID extends UCAN.DID> = {
  id: ID,
  /** https://www.w3.org/TR/did-core/#also-known-as */
  alsoKnownAs: DIDKey
}
type DidResolution<ID> =
| KeyAliasDidDocument<ID>

I like that return type because it's a subset of actual did doc type

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively could have resolveDID -> resolveDIDKey.

I like that it gets nowhere near 'did resolution' nor reinforces the (IMO footgun) assumption that a DID will always cleanly resolve to a single default keypair.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right resolveDID is misleading name, this is not a function to resolve a DID document but rather to resolve a key. I'll rename both interface and a function.

Comment thread packages/principal/test/ed25519.spec.js Outdated
describe('signing principal', () => {
const { Signer } = Lib

/** @type {Lib.Signer.EdSigner} */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this type assertion doin anything? I'm not sure what line it asserts about given the newline before and after

export const access = async (
invocation,
{ canIssue = isSelfIssued, principal, resolve = unavailable, capability }
export const access = async (invocation, { capability, ...config }) =>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a human readable js doc explaining when/why to use this could be helpful. In general, I like short names like these packages use, but we also frequently re-use those names across modules/files, and there are a lot of objects whose property names come from the intersection of several types, and those properties sometimes come from exports like this via a glob import.

wdyt of jsdoc human readable explanation here?
wdyt of something like an eslint rule that requires human readable explainers for exported functions?

Comment thread packages/validator/src/lib.js
Comment thread packages/validator/test/mailto.spec.js
@gobengo gobengo self-requested a review December 13, 2022 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants