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

Did dht test vectors #181

Merged
merged 41 commits into from
Jan 19, 2024
Merged

Did dht test vectors #181

merged 41 commits into from
Jan 19, 2024

Conversation

andresuribe87
Copy link
Contributor

@andresuribe87 andresuribe87 commented Dec 28, 2023

Overview

Adds test vectors for did:dht for the create and resolve functions. The motivation is to ensure interoperability across implementations of the standard. This PR is related to TBD54566975/web5-spec#11, and will form the basis to fix the issue.

Description

This PR includes the following changes:

  • Adds typed exceptions to ExceptionDeclarations.kt.
  • Adds test vector files for create and resolve.
  • Modifies the implementation of DidDht so that tests pass.
  • Adds a README for the test-vectors added.

Once this is accepted, the test vectors will be promoted to the sdk-development repository.

Note

The test vectors were based off of https://did-dht.com/#test-vectors, and adjusted to match our test-vector structure.

How Has This Been Tested?

Unit tests

return try {
resolveInternal(did)
} catch (e: Exception) {
println(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with a logger.

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Merging #181 (02932be) into main (e822caf) will increase coverage by 0.03%.
The diff coverage is 85.18%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
+ Coverage   80.15%   80.18%   +0.03%     
==========================================
  Files          37       37              
  Lines        2116     2150      +34     
  Branches      297      299       +2     
==========================================
+ Hits         1696     1724      +28     
- Misses        283      288       +5     
- Partials      137      138       +1     
Components Coverage Δ
credentials 81.38% <ø> (ø)
crypto 38.07% <ø> (ø)
dids 90.34% <85.18%> (-0.25%) ⬇️
common 80.55% <ø> (ø)

@andresuribe87 andresuribe87 changed the base branch from did_web_test_vectors to main January 16, 2024 16:17
@andresuribe87 andresuribe87 enabled auto-merge (squash) January 16, 2024 21:55
@frankhinek
Copy link
Contributor

I could be misreading the Kotlin (KT newb 🖐️)...

It seems that the create operation test vector provides an identity key as input:

"identityPublicJwk": {
"kty": "OKP",
"crv": "Ed25519",
"x": "YCcHYL2sYNPDlKaALcEmll2HHyT968M4UWbr-9CFGWE",
"alg": "EdDSA",
"kid": "0"
}

But the DidDht implementation doesn't support specifying a pre-existing identity key and mocks the key generation so that the test vector passes:

val identityKeyId = keyManager.import(listOf(vector.input.identityPublicJwk!!)).first()
doReturn(identityKeyId).whenever(keyManager).generatePrivateKey(JWSAlgorithm.EdDSA, Curve.Ed25519)

Had a number of long discussions with Moe and Adam this week on how to approach DID key management across all of our SDKs. It seems there's an opportunity to continue to get feedback from our partners and tweak how create(), load(), etc. work. But that should probably wait for another day and not block this PR.

@andresuribe87
Copy link
Contributor Author

I could be misreading the Kotlin (KT newb 🖐️)...

It seems that the create operation test vector provides an identity key as input:

"identityPublicJwk": {
"kty": "OKP",
"crv": "Ed25519",
"x": "YCcHYL2sYNPDlKaALcEmll2HHyT968M4UWbr-9CFGWE",
"alg": "EdDSA",
"kid": "0"
}

But the DidDht implementation doesn't support specifying a pre-existing identity key and mocks the key generation so that the test vector passes:

val identityKeyId = keyManager.import(listOf(vector.input.identityPublicJwk!!)).first()
doReturn(identityKeyId).whenever(keyManager).generatePrivateKey(JWSAlgorithm.EdDSA, Curve.Ed25519)

Had a number of long discussions with Moe and Adam this week on how to approach DID key management across all of our SDKs. It seems there's an opportunity to continue to get feedback from our partners and tweak how create(), load(), etc. work. But that should probably wait for another day and not block this PR.

Your reading is correct. The dht identity key cannot be passed in.

There were also other long discussions about the same topic during the ION implementation. The decision at that point in time is that the sdk should allow both paradigms: manually specifying the keys and also automatically creating them. That's the design that motivated

public interface VerificationMethodSpec

I agree that such a discussion shouldn't block this.

Copy link
Contributor

@phoebe-lew phoebe-lew left a comment

Choose a reason for hiding this comment

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

@frankhinek might be good for us to schedule time to discuss key management across the SDK?

private val mapper = jacksonObjectMapper()

class Web5TestVectorsDidDhtTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

need to remove Test

Copy link
Contributor

Choose a reason for hiding this comment

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

should just be Web5TestVectorsDidDht

@andresuribe87 andresuribe87 merged commit f0ddfb1 into main Jan 19, 2024
6 of 8 checks passed
@andresuribe87 andresuribe87 deleted the did_dht_test_vectors branch January 19, 2024 16:45
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

4 participants