Replace `verificationMethods` type in `did:dht` to use `VerificationMethodSpec` #146

andresuribe87 · 2023-11-21T20:48:53Z

Overview

This PR makes the CreateDid{Dht,Ion}Options consistent, and allows users to create dht dids without having to worry about the key creation for the verification methods.

Description

See the updated DidDhtTest.kt file to understand how we allow consumers to create VerificationMethods with or without a KeyManager.

How Has This Been Tested?

Simple refactor. I just moved stuff around and slightly updated tests.

References

See the original did:ion PR that allowed this in #96

…ethodSpec`.

codecov · 2023-11-21T20:51:15Z

Codecov Report

Merging #146 (04c619c) into main (8fe7e59) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 92.23%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #146   +/-   ##
=======================================
  Coverage   78.89%   78.89%           
=======================================
  Files          33       34    +1     
  Lines        1928     1933    +5     
  Branches      265      262    -3     
=======================================
+ Hits         1521     1525    +4     
- Misses        275      276    +1     
  Partials      132      132

Components	Coverage Δ
credentials	`80.33% <ø> (ø)`
crypto	`38.07% <ø> (ø)`
dids	`89.55% <92.23%> (-0.05%)`	⬇️
common	`80.44% <ø> (ø)`

dids/src/main/kotlin/web5/sdk/dids/methods/dht/DidDht.kt

dids/src/main/kotlin/web5/sdk/dids/verificationmethods/VerificationMethod.kt

mistermoe · 2023-11-22T23:46:48Z

@andresuribe87 does this change the public api surface? if so, as mentioned before, can you provide:

example of API surface prior to change
example of proposed change to API surface
rationale behind making said change

mistermoe · 2023-11-22T23:55:30Z

dids/src/main/kotlin/web5/sdk/dids/methods/dht/DidDht.kt

+  public val verificationMethodsToAdd: Iterable<VerificationMethodSpec> = emptyList(),
+  public val servicesToAdd: Iterable<Service> = emptyList(),


toAdd seems redundant here given that these are create options.

This was toAdd in CreateDidIonOptions already. Happy to change, but I think that should be part of a separate PR. Mind creating an issue?

i can create a separate issue for changing DidIon. DidDht was already just services and verificationMethods. We should keep it that way. no point in changing code to match what's going to be immediately changed

Ah, I see what you mean. Fixed in c5b2eac

And renamed to the existing didion to b24b9fd

mistermoe · 2023-11-22T23:57:53Z

dids/src/main/kotlin/web5/sdk/dids/verificationmethods/VerificationMethod.kt

+ * The [id] property cannot be over 50 chars and must only use characters from the Base64URL character set.
+ */
+public class JsonWebKey2020VerificationMethod(
+  public val id: String,


i think this should be optional not required. default to jwk thumbprint if not provided

This was required before. What's the motivation behind not requiring it?

Regardless, I think that should be part of a separate PR.

Elaborating further, I don't think we should be adding for a couple of reasons. The id here is the verification method ID. It's not the id for the key. The key id for JWK is contained inside the publicKeyJwk parameter.

andresuribe87 · 2023-11-27T20:20:47Z

@andresuribe87 does this change the public api surface? if so, as mentioned before, can you provide:

example of API surface prior to change

example of proposed change to API surface

rationale behind making said change

The API surface before wasn't consistent across different implementations. This PR isn't introducing a new API, it's just standardizing. As such, I didn't consider this as needing to go through the normal process of proposing new public APIs. The existing API that this PR is moving the DHT implementation towards was discussed before doing merging in the DID ION implementation PR (#96). This PR is purposefully not proposing changing the existing public API used by Did Ion.

Just for clarity, I'm including answers here:

API before:

      val manager = InMemoryKeyManager()

      val otherKey = manager.generatePrivateKey(JWSAlgorithm.ES256K, Curve.SECP256K1)
      val publicKeyJwk = manager.getPublicKey(otherKey).toPublicJWK()
      val verificationMethodsToAdd: Iterable<Pair<JWK, Array<PublicKeyPurpose>>> = listOf(
        Pair(publicKeyJwk, arrayOf(PublicKeyPurpose.AUTHENTICATION, PublicKeyPurpose.ASSERTION_METHOD))
      )

      val serviceToAdd =
        Service.builder()
          .id(URI("test-service"))
          .type("HubService")
          .serviceEndpoint("https://example.com/service)")
          .build()

      val opts = CreateDidDhtOptions(
        verificationMethods = verificationMethodsToAdd, services = listOf(serviceToAdd), publish = false
      )
      val did = DidDht.create(manager, opts)

API after

      val manager = InMemoryKeyManager()

      val otherKey = manager.generatePrivateKey(JWSAlgorithm.ES256K, Curve.SECP256K1)
      val publicKeyJwk = manager.getPublicKey(otherKey).toPublicJWK()
      val verificationMethodsToAdd = listOf(
        JsonWebKey2020VerificationMethod(
          id = publicKeyJwk.keyID,
          publicKeyJwk = publicKeyJwk,
          relationships = listOf(PublicKeyPurpose.AUTHENTICATION, PublicKeyPurpose.ASSERTION_METHOD)
        ),
        VerificationMethodCreationParams(
          algorithm = JWSAlgorithm.EdDSA,
          curve = Curve.Ed25519,
          relationships = listOf(PublicKeyPurpose.ASSERTION_METHOD, PublicKeyPurpose.CAPABILITY_INVOCATION)
        ),
      )

      val serviceToAdd =
        Service.builder()
          .id(URI("test-service"))
          .type("HubService")
          .serviceEndpoint("https://example.com/service)")
          .build()

      val opts = CreateDidDhtOptions(
        verificationMethodsToAdd = verificationMethodsToAdd, servicesToAdd = listOf(serviceToAdd), publish = false
      )
      val did = DidDht.create(manager, opts)

Rationale

As explained in the PR description, CreateDidIonOptions already had a public API for adding verification methods and services to a did. This PR is making CreateDidDhtOptions use the same style, so it's consistent across different did methods.

andresuribe87 · 2023-12-05T17:58:20Z

@mistermoe let me know if I have addressed your concerns.

decentralgabe · 2023-12-13T16:52:04Z

dids/src/main/kotlin/web5/sdk/dids/methods/dht/DidDht.kt

@@ -77,14 +79,14 @@ private class DidDhtApiImpl(configuration: DidDhtConfiguration) : DidDhtApi(conf

 /**
 * Specifies options for creating a new "did:dht" Decentralized Identifier (DID).
- * @property verificationMethods A list of [JWK]s to add to the DID Document mapped to their purposes
- * as verification methods.
+ * @property verificationMethods List of specs that will be added to the DID ION document. It's important to note


Suggested change

* @property verificationMethods List of specs that will be added to the DID ION document. It's important to note

* @property verificationMethods List of verification methods that will be added to the DID DHT document. It's important to note

decentralgabe · 2023-12-13T16:53:17Z

dids/src/test/kotlin/web5/sdk/dids/methods/dht/DidDhtTest.kt

-      val verificationMethodsToAdd: Iterable<Pair<JWK, Array<PublicKeyPurpose>>> = listOf(
-        Pair(publicKeyJwk, arrayOf(PublicKeyPurpose.AUTHENTICATION, PublicKeyPurpose.ASSERTION_METHOD))
+      val verificationMethodsToAdd = listOf(
+        JsonWebKey2020VerificationMethod(


noting that we should change this to just JsonWebKey TBD54566975/did-dht#60 TBD54566975/web5-spec#73

mistermoe · 2023-12-18T06:58:32Z

dids/src/test/kotlin/web5/sdk/dids/methods/dht/DidDhtTest.kt

+        DidDht.create(
+          manager,
+          CreateDidDhtOptions(
+            verificationMethods = listOf(
+              JsonWebKey2020VerificationMethod(
+                id = "0",
+                publicKeyJwk = manager.getPublicKey(
+                  manager.generatePrivateKey(
+                    JWSAlgorithm.EdDSA,
+                    Curve.Ed25519
+                  )
+                ).toPublicJWK(),
+                relationships = listOf(PublicKeyPurpose.AUTHENTICATION)
+              )
+            ),
+            publish = false
+          )
+        )


i'm just not sure anyone would be able to figure out how to do this

@mistermoe is there a concrete suggestion you have in mind?

This was my best approach towards having options such that:

The options between ion and Dht are consistent.

there is a way for consumers of the library to provide the JWK keys they want to be used within the did document.

there is a way for consumers to have the key creation handled for them.

Note that this had been discussed beforehand, when we added options to did ion. It made sense to me back then, and still make sense. So I'd be curious to understand further what differences you see between ion and dht.

andresuribe87 added 2 commits November 21, 2023 15:05

Replace verificationMethods type in did:dht to use `VerificationM…

67f1079

…ethodSpec`.

i have no idea how to simplify

afcf469

andresuribe87 requested review from mistermoe, nitro-neal, tomdaffurn and phoebe-lew as code owners November 21, 2023 20:48

decentralgabe reviewed Nov 22, 2023

View reviewed changes

dids/src/main/kotlin/web5/sdk/dids/methods/dht/DidDht.kt Show resolved Hide resolved

decentralgabe reviewed Nov 22, 2023

View reviewed changes

dids/src/main/kotlin/web5/sdk/dids/verificationmethods/VerificationMethod.kt Show resolved Hide resolved

mistermoe reviewed Nov 22, 2023

View reviewed changes

andresuribe87 added 2 commits November 27, 2023 15:40

Validate that id isn't 0.

f6c9b50

Merge branch 'main' into add_load_to_dht

585d9e7

andresuribe87 requested review from mistermoe and decentralgabe November 27, 2023 20:42

andresuribe87 mentioned this pull request Nov 27, 2023

I think we should axe all usage of this in favor of JWK #149

Closed

andresuribe87 added 2 commits November 27, 2023 17:36

Rename to existing.

c5b2eac

Rename DidIon to drop toAdd

b24b9fd

decentralgabe approved these changes Nov 28, 2023

View reviewed changes

Merge branch 'main' into add_load_to_dht

04c619c

decentralgabe reviewed Dec 13, 2023

View reviewed changes

decentralgabe approved these changes Dec 13, 2023

View reviewed changes

mistermoe reviewed Dec 18, 2023

View reviewed changes

decentralgabe requested review from diehuxx, kirahsapong and jiyoontbd as code owners August 13, 2024 04:25

decentralgabe requested a review from frankhinek as a code owner August 13, 2024 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `verificationMethods` type in `did:dht` to use `VerificationMethodSpec` #146

Replace `verificationMethods` type in `did:dht` to use `VerificationMethodSpec` #146

andresuribe87 commented Nov 21, 2023

codecov bot commented Nov 21, 2023 •

edited

Loading

mistermoe commented Nov 22, 2023 •

edited

Loading

mistermoe Nov 22, 2023

andresuribe87 Nov 27, 2023

mistermoe Nov 27, 2023

andresuribe87 Nov 27, 2023

andresuribe87 Nov 27, 2023

mistermoe Nov 22, 2023

andresuribe87 Nov 27, 2023

andresuribe87 Dec 5, 2023

andresuribe87 commented Nov 27, 2023

andresuribe87 commented Dec 5, 2023

decentralgabe Dec 13, 2023

decentralgabe Dec 13, 2023

mistermoe Dec 18, 2023

andresuribe87 Dec 18, 2023 •

edited

Loading

		public val verificationMethodsToAdd: Iterable<VerificationMethodSpec> = emptyList(),
		public val servicesToAdd: Iterable<Service> = emptyList(),

	* @property verificationMethods List of specs that will be added to the DID ION document. It's important to note
	* @property verificationMethods List of verification methods that will be added to the DID DHT document. It's important to note

Replace verificationMethods type in did:dht to use VerificationMethodSpec #146

Are you sure you want to change the base?

Replace verificationMethods type in did:dht to use VerificationMethodSpec #146

Conversation

andresuribe87 commented Nov 21, 2023

Overview

Description

How Has This Been Tested?

References

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

mistermoe commented Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresuribe87 commented Nov 27, 2023

API before:

API after

Rationale

andresuribe87 commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresuribe87 Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Replace `verificationMethods` type in `did:dht` to use `VerificationMethodSpec` #146

Replace `verificationMethods` type in `did:dht` to use `VerificationMethodSpec` #146

codecov bot commented Nov 21, 2023 •

edited

Loading

mistermoe commented Nov 22, 2023 •

edited

Loading

andresuribe87 Dec 18, 2023 •

edited

Loading