-
Notifications
You must be signed in to change notification settings - Fork 54
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
Update DID DHT Key Type to EdDSA #653
Conversation
🦋 Changeset detectedLatest commit: 62a0a73 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
specifier: 1.0.0 | ||
version: 1.0.0 | ||
specifier: 1.0.1 | ||
version: link:../common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this fine?
TBDocs Report ✅ No errors or warnings @web5/api
@web5/crypto
@web5/crypto-aws-kms
@web5/dids
@web5/credentials
TBDocs Report Updated at 2024-06-11T22:50:23Z |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #653 +/- ##
==========================================
+ Coverage 90.81% 90.90% +0.09%
==========================================
Files 119 119
Lines 30101 30106 +5
Branches 2243 2249 +6
==========================================
+ Hits 27337 27369 +32
+ Misses 2729 2702 -27
Partials 35 35
|
|
||
it('successful verify with did:dht', async () => { | ||
const hexString = | ||
'0ab2b3386e22595e1271e7ef67fda70c37acf7d28b8c884a6fdcbb0ea739f341' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of opaque inputs to tests. Can you update and enable vector tests accordingly for the js implementation and call out in #638 that the kid changes must be reverted when 638 is fixed.
I think they should cover this change. Let me know if I am not making sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test tests the Jwt.sign / verify with did dht, (the reason we had to change the spec a bit in the first place)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, assuming the main intent of this test is to test sign and verify, can we avoid using the opaque hex string in the test since it is besides the point?
I have turned the test vectors back on, they can pass by either:
if(dnsRecordId === 'k0') {
publicKey.kid = '0';
} Not sure which way is right at this point! but feel free to let me know which way we want to go |
I have updated, added this line:
And chnaged the test vectors to have kid as 0 again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
This bug fixes this issue and changes the index 0 key type from
Ed25519
toEdDSA
(pending change coming to did-dht.com)Here was the issue that was happening which this pr fixes:
When creating a portable did with a jwk like this:
(notice Alg is EdDSA)
the resolver gives:
(notice alg is Ed25519)
The solution was to change the actual did dht spec so that this does not happen and allows use to verify jwts with did dht