Skip to content
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

feat: embedded key resolution #168

Merged
merged 18 commits into from
Dec 14, 2022
Merged

feat: embedded key resolution #168

merged 18 commits into from
Dec 14, 2022

Conversation

Gozala
Copy link
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
@@ -499,6 +504,10 @@ export interface PrincipalParser {
parse(did: UCAN.DID): Verifier
}

export interface DIDResolver {
resolveDID?: (did: UCAN.DID) => Await<Result<DIDKey, DIDResolutionError>>
Copy link
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
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
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
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.

@@ -6,6 +6,8 @@ import { varint } from 'multiformats'
describe('signing principal', () => {
const { Signer } = Lib

/** @type {Lib.Signer.EdSigner} */
Copy link
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
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?

@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