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!: Upgrade did-resolver to v3 #151

Merged
merged 3 commits into from
Mar 9, 2021
Merged

feat!: Upgrade did-resolver to v3 #151

merged 3 commits into from
Mar 9, 2021

Conversation

oed
Copy link
Contributor

@oed oed commented Mar 5, 2021

Replaces #150
Fixes #144

Edit:
I think this would be seen as a breaking release so I flagged it as such.

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Thank you for the help maintaining this library!

The PR generally looks good, but I have some questions that I'd like to get answered before the merge.

src/JWT.ts Show resolved Hide resolved
if (typeof key === 'string') {
return didDoc.publicKey.find((pk) => pk.id === key)
return didDocument.verificationMethod.find((pk) => pk.id === key)
Copy link
Member

Choose a reason for hiding this comment

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

Some resolvers may still be using the deprecated publicKey member of the DIDDocument.
I think the 2 arrays should be merged, to support these resolvers.
Perhaps something like this would do the trick:

Suggested change
return didDocument.verificationMethod.find((pk) => pk.id === key)
return ([
...(didDocument.publicKey),
...(didDocument.verificationMethod)
].find((pk) => pk.id === key)

I see that a similar approach is used in JWT.ts#resolveAuthenticator() but that one has some extra checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. I believe I did this elsewhere in the code.

if (pk.publicKeyBase58) {
return base58ToBytes(pk.publicKeyBase58)
} else if (pk.publicKeyBase64) {
} else if (isLegacyVerMethod(pk) && pk.publicKeyBase64) {
Copy link
Member

Choose a reason for hiding this comment

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

is this extra check necessary?
Perhaps it can be simplified to something like:

Suggested change
} else if (isLegacyVerMethod(pk) && pk.publicKeyBase64) {
} else if ((<LegacyVerificationMethod>pk).publicKeyBase64) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, ty

src/JWT.ts Outdated
const authenticators: PublicKey[] = publicKeysToCheck.filter(({ type }) =>
types.find((supported) => supported === type)
const authenticators: VerificationMethod[] = publicKeysToCheck.filter(({ type }) =>
types.indexOf(type) !== -1
Copy link
Member

Choose a reason for hiding this comment

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

Why is the filter for supported types removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's actually the same logic. This change is not needed, can change it back.

src/JWT.ts Show resolved Hide resolved
src/JWT.ts Show resolved Hide resolved
@oed
Copy link
Contributor Author

oed commented Mar 8, 2021

Pushed some edits to address your comments @mirceanis

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the contribution!

src/JWT.ts Show resolved Hide resolved
@mirceanis mirceanis merged commit e02f56b into decentralized-identity:master Mar 9, 2021
uport-automation-bot pushed a commit that referenced this pull request Mar 9, 2021
# [5.0.0](4.9.0...5.0.0) (2021-03-09)

### Features

* upgrade did-resolver to v3 ([#151](#151)) ([e02f56b](e02f56b))

### BREAKING CHANGES

* The `Resolver` used during verification is expected to conform to the latest spec.
@uport-automation-bot
Copy link
Collaborator

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Comply with the did-resolution spec
3 participants