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

Implement a publicMethodFor() helper function. #38

Merged
merged 6 commits into from
Apr 9, 2021

Conversation

dmitrizagidulin
Copy link
Contributor

This PR adds a convenience function that can be used in conjunction with a .get() to start with a did:key DID, and end up with a key for a specific purpose.

For example,

// Start with the DID
const didDocument = await didKeyDriver.get({did});
// This lets you use `publicMethodFor()` to get a key for a specific purpose
const keyAgreementData = didKeyDriver.publicMethodFor({
  didDocument, purpose: 'keyAgreement'
});
const assertionMethodData = didKeyDriver.publicMethodFor({
  didDocument, purpose: 'assertionMethod'
});
// If you're using a `crypto-ld` driver harness, you can create key instances
// which allow you to get access to a `verify()` function.
const assertionMethodPublicKey = await cryptoLd.from(assertionMethodData);
const {verify} = assertionMethodPublicKey.verifier();

This addition matches the convenience methodFor function that gets returned with generate(). The main difference is -- since it's returned by generate(), methodFor has access to the private key material (and so, can be used for decryption, signing, etc).
Whereas publicMethodFor only has access to the public key material (extracted from the DID Document), and is useful by consumers that need those keys for verification and encryption (to a known DID recipient).

Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

I have no idea how useful this is to devs (not having contact with this code in a while). I'm presuming people are needing this ... in which case I approve.

lib/DidKeyDriver.js Show resolved Hide resolved
test/driver.spec.js Show resolved Hide resolved
'zB12NYF8RrR3h41TDCTJojY59usg3mbtbjnFs7Eud1Y6u');
});

it('should return undefined if key is not found for purpose', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not return a NotFoundError? Otherwise the consumer of this API has the null check the response right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing an error seemed too drastic of a response (since not having a key in a did document for a given purpose is a normal condition). I could add that, if you feel strongly about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think an error would be better. I'm having difficulty imagining how returning undefined does not lead to an error anyhow. Likely the typical cannot access foo of undefined error.

Did you have some specific use pattern in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattcollier Np; changed to throw error when not found. Thanks!

Copy link
Contributor

@mattcollier mattcollier left a comment

Choose a reason for hiding this comment

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

Question about return value of publicMethodFor

Copy link
Contributor

@mattcollier mattcollier left a comment

Choose a reason for hiding this comment

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

Approving, just a few more nits.

error = e;
}

expect(error).to.be.exist;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(error).to.be.exist;
expect(error).to.exist;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops.


expect(error).to.be.exist;
expect(error.message).to
.match(/No verification method found for purpose/);
Copy link
Contributor

Choose a reason for hiding this comment

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

can avoid regex with .to.contain()

Comment on lines 83 to 85
* @returns {object} Returns the public key object (obtained from the DID
* Document), without a `@context`. Returns undefined if no key is found
* for that purpose.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs updating now.

@dmitrizagidulin
Copy link
Contributor Author

@mattcollier Thanks for the catch; all updated.

@dmitrizagidulin dmitrizagidulin merged commit f42d788 into v1.x Apr 9, 2021
@dmitrizagidulin dmitrizagidulin deleted the public-method-for branch April 9, 2021 16:32
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.

None yet

3 participants