Remove patch. #4
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4 +/- ##
==========================================
- Coverage 71.58% 71.12% -0.47%
==========================================
Files 5 5
Lines 366 374 +8
==========================================
+ Hits 262 266 +4
- Misses 104 108 +4
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
This does make the tests pass with the fixed context vs vocab difference. The did-jwt code that the patch file was adjusting is not run at all during testing here, which can be confirmed with a few logging lines.
I'm not familiar with how jwt code works, but it looks like it's important to get the public/private differences correct here. If the new semantics are that if publicKeyJwk is present, it converts that, else the private data, the results will certainly be different depending on input. It might leak private data without that patch, depending on expectations. Can tests be added that will certainly hit the did-jwt publicKeyJwk code the patch was changing?
If it turns out those public key features were not ever needed for at least our use cases, then I assume this PR is fine for now and that limitation to address later.
| const decoded = jose.base64url.decode(methodSpecificId); | ||
| const message = new TextDecoder().decode(decoded); | ||
| const jwk = JSON.parse(message); | ||
| + const {publicKeyJwk} = jwk; |
There was a problem hiding this comment.
The jwk doesn't include a publicKeyJwk so this is a noop
| + const keyPair = await generateKeyPair(alg); | ||
| + const {publicKeyJwk} = keyPair; | ||
| + const didDocument = toDidDocument(publicKeyJwk); | ||
| + return {keyPair, didDocument}; |
There was a problem hiding this comment.
The differences here are that the toDidDocument takes in just the publicKeyJwk and the full keyPair is returned. Neither of these things is mandatory as in the former the didDocument creates fine with either private or public and in the latter the keypair is only adding more public information
It wasn't clear to me what the other parts were doing so I only pulled the one part of the patch in for now