DID ION Update, Recover, and Deactivate Operations #77

andresuribe87 · 2023-10-11T00:04:42Z

Overview

Fixes #1 by implementing did ion updates.

Includes the updates of PR #88, which fixes #3 and #2.

Description

After this PR, all ion type of updates will be supported.

How Has This Been Tested?

Unit test that mimics https://github.com/decentralized-identity/ion-sdk/blob/main/tests/IonRequest.spec.ts#L38 as much as possible.

codecov-commenter · 2023-10-11T00:08:06Z

Codecov Report

Merging #77 (02ea4b3) into main (de73ad8) will increase coverage by 6.40%.
The diff coverage is 94.09%.

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   63.17%   69.58%   +6.40%     
==========================================
  Files          21       21              
  Lines         964     1200     +236     
  Branches      123      136      +13     
==========================================
+ Hits          609      835     +226     
  Misses        294      294              
- Partials       61       71      +10

Components	Coverage Δ
credentials	`65.76% <50.00%> (-0.68%)`	⬇️
crypto	`33.33% <50.00%> (-0.25%)`	⬇️
dids	`92.81% <95.68%> (+3.73%)`	⬆️
common	`60.83% <ø> (ø)`

crypto/src/main/kotlin/web5/sdk/crypto/Crypto.kt

crypto/src/main/kotlin/web5/sdk/crypto/KeyManager.kt

dids/src/main/kotlin/web5/sdk/dids/DIDIonManager.kt

nitro-neal

Looks good, just make sure the sign is working the same way as #63

andresuribe87

With the latest changes from the base branch now

crypto/src/main/kotlin/web5/sdk/crypto/Crypto.kt

crypto/src/main/kotlin/web5/sdk/crypto/KeyManager.kt

bradleydwyer · 2023-10-11T08:27:11Z

dids/src/main/kotlin/web5/sdk/dids/DIDIonManager.kt

+public class UpdateDidIonOptions(
+  public val didString: String,
+  public val updateKeyAlias: String,
+  public val servicesToAdd: Iterable<Service>? = null,


Curious as to why the adds are more generic than the remove (Iterable vs Set). Is order important for add operations?

bradleydwyer · 2023-10-11T08:53:13Z

dids/src/main/kotlin/ion/model/Models.kt

+  JsonSubTypes.Type(ReplaceAction::class, name = "replace"),
+  JsonSubTypes.Type(RemoveServicesAction::class, name = "remove-services"),
+  JsonSubTypes.Type(AddPublicKeysAction::class, name = "add-public-keys"),
+  JsonSubTypes.Type(RemovePublicKeysAction::class, name = "remove-public-keys"),
 )
 public sealed class PatchAction


Curious why the sealed class route vs a marker interface?

bradleydwyer · 2023-10-11T08:54:51Z

dids/src/main/kotlin/ion/model/Models.kt

@@ -151,6 +169,11 @@ public data class OperationSuffixDataObject(
 */
 public typealias Commitment = String

+/**


Nit: IMHO these comments are super obvious. I don't think they add value here and make the code less readable.

bradleydwyer · 2023-10-11T10:15:34Z

dids/src/main/kotlin/web5/sdk/dids/DIDIonManager.kt

+  public val servicesToAdd: Iterable<Service>? = null,
+  public val idsOfServicesToRemove: Set<String>? = null,
+  public val publicKeysToAdd: Iterable<PublicKey>? = null,
+  public val idsOfPublicKeysToRemove: Set<String>? = null,


I think it might be preferable to just create empty collections by default rather than cater for the possibility of a null collection

bradleydwyer · 2023-10-11T10:16:39Z

dids/src/main/kotlin/web5/sdk/dids/DIDIonManager.kt

+ * @param publicKeysToAdd PublicKeys to add to the DID document.
+ * @param idsOfPublicKeysToRemove Keys to remove from the DID document.
+ */
+public class UpdateDidIonOptions(


A possible alternative to this impl might be

public data class UpdateDidIonOptions( val didString: String, val updateKeyAlias: String, val servicesToAdd: List<Service> = emptyList(), val idsOfServicesToRemove: List<String> = emptyList(), val publicKeysToAdd: List<PublicKey> = emptyList(), val idsOfPublicKeysToRemove: List<String> = emptyList() ) { internal fun toPatches(): List<PatchAction> { fun <T> MutableList<PatchAction>.addIfNotEmpty(list: List<T>, action: (List<T>) -> PatchAction) { list.takeIf { it.isNotEmpty() }?.let { this.add(action(it)) } } return buildList { addIfNotEmpty(servicesToAdd, ::AddServicesAction) addIfNotEmpty(idsOfServicesToRemove, ::RemoveServicesAction) addIfNotEmpty(publicKeysToAdd, ::AddPublicKeysAction) addIfNotEmpty(idsOfPublicKeysToRemove, ::RemovePublicKeysAction) } } }

For a rough and untested stab at it. I think this covers the comments below.

bradleydwyer · 2023-10-11T10:27:11Z

dids/src/main/kotlin/web5/sdk/dids/DIDIonManager.kt

+        updateKeyAlias = newUpdateKeyAlias,
+      )
+    }
+    throw Exception("received error response '$opBody'")


RuntimeException or even better a custom runtime exception type.

bradleydwyer · 2023-10-11T10:44:30Z

dids/src/main/kotlin/ion/model/Models.kt

This file is an interop issue with Java. The declared package doesn't match the file location

andresuribe87 · 2023-10-16T00:28:10Z

@bradleydwyer you made comments on a very old commit. Can you take another stab at it, please?

bradleydwyer · 2023-10-16T00:42:37Z

dids/src/main/kotlin/web5/sdk/dids/DidIonManager.kt

+public class UpdateDidIonOptions(
+  public val didString: String,
+  public val updateKeyAlias: String,
+  public val servicesToAdd: Iterable<Service>? = null,


I think it might be preferable to just create empty collections by default rather than cater for the possibility of a null collection (#77 (comment))

bradleydwyer · 2023-10-16T00:43:45Z

dids/src/main/kotlin/web5/sdk/dids/DidIonManager.kt

+ * @param publicKeysToAdd PublicKeys to add to the DID document.
+ * @param idsOfPublicKeysToRemove Keys to remove from the DID document.
+ */
+public class UpdateDidIonOptions(


A possible alternative to this impl might be

public data class UpdateDidIonOptions( val didString: String, val updateKeyAlias: String, val servicesToAdd: List<Service> = emptyList(), val idsOfServicesToRemove: List<String> = emptyList(), val publicKeysToAdd: List<PublicKey> = emptyList(), val idsOfPublicKeysToRemove: List<String> = emptyList() ) { internal fun toPatches(): List<PatchAction> { fun <T> MutableList<PatchAction>.addIfNotEmpty(list: List<T>, action: (List<T>) -> PatchAction) { list.takeIf { it.isNotEmpty() }?.let { this.add(action(it)) } } return buildList { addIfNotEmpty(servicesToAdd, ::AddServicesAction) addIfNotEmpty(idsOfServicesToRemove, ::RemoveServicesAction) addIfNotEmpty(publicKeysToAdd, ::AddPublicKeysAction) addIfNotEmpty(idsOfPublicKeysToRemove, ::RemovePublicKeysAction) } } }

For a rough and untested stab at it. I think this covers the comments below.

(#77 (comment))

I like this, thanks! Adopted! Just made a tiny change to accept iterables everywhere.

bradleydwyer · 2023-10-16T00:44:37Z

dids/src/main/kotlin/web5/sdk/dids/DidIonManager.kt

+public class UpdateDidIonOptions(
+  public val didString: String,
+  public val updateKeyAlias: String,
+  public val servicesToAdd: Iterable<Service>? = null,


Curious as to why the adds are more generic than the remove (Iterable vs Set). Is order important for add operations?

(#77 (comment))

When I looked at the spec it didn't seem like the order was important (or that each type of patch was required to be present)

And in addition to https://github.com/TBD54566975/web5-kt/pull/77/files#r1359995250 was confused at marshalling everything into a List at the end - why not just be a List from the start?

The spec says, for the add operation:

In the case a service entry already exists for the given id specified within an add-services Patch Action, the implementation MUST overwrite the existing entry entirely with the incoming patch. To construct an add-services patch, compose an object as follows:

So my understanding is that order is important. Am I reading this correctly?

Re:

was confused at marshalling everything into a List at the end - why not just be a List from the start?

Great question. UpdateIonOptions is part of the public API. As such, my intention was to adopt Postel's law: "Be liberal in what you accept, and conservative in what you send.". By having Iterable, clients can have any sort of collection before the pass in the data.

I went ahead and updated the classes inside Models to follow this pattern as well.

bradleydwyer · 2023-10-16T00:45:22Z

dids/src/main/kotlin/web5/sdk/dids/ion/model/Models.kt

+/**
+ * Type alias for reveal value.
+ */
+public typealias Reveal = String


Nit: IMHO these comments are super obvious. I don't think they add value here and make the code less readable. This is also a general comment on things like @param kdoc that adds no detail beyond what is already obvious from the associate type and param name.

(#77 (comment))

Agreed. That said, with the changes to this class, the comments have become more useful.

bradleydwyer · 2023-10-16T00:45:52Z

dids/src/main/kotlin/web5/sdk/dids/ion/model/Models.kt

+  JsonSubTypes.Type(ReplaceAction::class, name = "replace"),
+  JsonSubTypes.Type(RemoveServicesAction::class, name = "remove-services"),
+  JsonSubTypes.Type(AddPublicKeysAction::class, name = "add-public-keys"),
+  JsonSubTypes.Type(RemovePublicKeysAction::class, name = "remove-public-keys"),
 )
 public sealed class PatchAction


Curious why the sealed class route vs a marker interface?

(#77 (comment))

No reason - changed in the base PR in 5392a4e

bradleydwyer · 2023-10-16T00:58:40Z

@bradleydwyer you made comments on a very old commit. Can you take another stab at it, please?

@andresuribe87 They were made 5 days ago, but held off hitting submit on the review to avoid distracting from deadline. Have duplicated the comment on the current HEAD with pointers back.

bradleydwyer · 2023-10-16T01:26:36Z

dids/src/main/kotlin/web5/sdk/dids/ion/model/Models.kt

@@ -151,6 +169,11 @@ public data class OperationSuffixDataObject(
 */
 public typealias Commitment = String


Declared but not used in places like

public class MetadataMethod( public val published: Boolean, public val recoveryCommitment: String, public val updateCommitment: String, )

which I presume it should be?

Absolutely! I massaged this a bit and now all instances are using the Commitment and Reveal classes.

bradleydwyer · 2023-10-16T01:27:07Z

dids/src/main/kotlin/web5/sdk/dids/ion/model/Models.kt

+public data class SidetreeUpdateOperation(
+  public val type: String,
+  public val didSuffix: String,
+  public val revealValue: String,


use the Reveal typealias?

…r_op

Support DID Ion Recovery and Deactivate

mistermoe · 2023-10-24T22:02:29Z

crypto/src/main/kotlin/web5/sdk/crypto/InMemoryKeyManager.kt

-    require(jwk.isPrivate) {
-      "Importing a non-private key is not permitted"
-    }


what's the use-case driving the ability to import a public key?

I wanted to use the same test vectors from the ion-sdk repository. The update request tests uses the public key of the next update, but not the private key.

See https://github.com/TBD54566975/web5-kt/blob/ad6a91e1a8cac29c752156116fdf253395c5bf7c/dids/src/test/kotlin/web5/sdk/dids/DidIonTest.kt#L292 for where it's used.

And see https://github.com/decentralized-identity/ion-sdk/blob/main/tests/IonRequest.spec.ts#L48 for the original test.

mistermoe · 2023-10-24T23:30:33Z

Took some time to use the current ION implementation last night beyond the default scenario.

There's quite a bit of optionality that ends up creating a strange usage pattern where the caller effectively ends up having to use the keyManger outside ion to create keys for or associated to an ion did. I think it'd make more sense for the optionality to look something like this:

package web5.sdk.dids
import web5.sdk.crypto.InMemoryKeyManager

public fun main() {
  val km = InMemoryKeyManager()
  val opts = CreateDidIonOptions(
    services = listOf(
      Service(...)
    ),
    verificationMethods = listOf(
      IonVerificationMethod(
        alg: JWSAlgorithm.ES256K,
        relationships = listOf(VerificationRelationships.Authentication, VerificationRelationships.Assertion)
      )
    )
  )

  val did = DidIon.create(km)
}

other thoughts:

found the name DidIonManager to be confusing. instinctively assuming that it managed multiple ion dids. or was persistently stateful in some way. personally prefer DidIon
DidIonHandle was also a bit confusing. the general pattern that seemed to stick was the way in which DidKey is implemented
would be nice to use okhttp instead of ktor so that there's some congruency across kotlin repos. tbh though i haven't dug into kotlin multiplatform enough to know whether ktor actually helps here or not in a way that okhttp doesn't.

I think we can merge this PR into main and then address the above as a whole in a separate PR. thoughts?

dids/src/main/kotlin/web5/sdk/dids/ion/model/Models.kt

credentials/src/main/kotlin/web5/sdk/credentials/VerifiableCredential.kt

tomdaffurn · 2023-10-25T00:46:46Z

credentials/src/main/kotlin/web5/sdk/credentials/VerifiableCredential.kt

+      didResolutionResult.didDocument.assertionMethodVerificationMethodsDereferenced
+
+    require(!assertionMethods.isNullOrEmpty()) {
+      throw SignatureException("no assertion methods found in did document")


Suggested change

throw SignatureException("no assertion methods found in did document")

throw SignatureException("No assertion methods found in DID document")

Co-authored-by: Tom Daffurn <[email protected]>

andresuribe87 · 2023-10-25T02:45:03Z

thanks for the feedback @mistermoe
Will merge and we can address in a separate PR. Some thoughts for now below

Took some time to use the current ION implementation last night beyond the default scenario.

There's quite a bit of optionality that ends up creating a strange usage pattern where the caller effectively ends up having to use the keyManger outside ion to create keys for or associated to an ion did. I think it'd make more sense for the optionality to look something like this:
package web5.sdk.dids
import web5.sdk.crypto.InMemoryKeyManager

public fun main() {
  val km = InMemoryKeyManager()
  val opts = CreateDidIonOptions(
    services = listOf(
      Service(...)
    ),
    verificationMethods = listOf(
      IonVerificationMethod(
        alg: JWSAlgorithm.ES256K,
        relationships = listOf(VerificationRelationships.Authentication, VerificationRelationships.Assertion)
      )
    )
  )

  val did = DidIon.create(km)
}

I agree that there is too much optionality. That said, I'm not a fan of digressing from the VerificationMethod data model described in the spec. Let's chat more about it in the followup PR.

other thoughts:

found the name DidIonManager to be confusing. instinctively assuming that it managed multiple ion dids. or was persistently stateful in some way. personally prefer DidIon

Curious what made you think there wasn't any state within the manager?

It is stateful (the httpclient and the ionHost are part of the state that's kept).
It also has the ability to manage multiple dids (you can update any did).

My take on DidIon is that it doesn't tell me much about which actions I might call from the object. It sounds like you're proposing to use it like a namespace, rather than an actual object?

I originally thought to use DidIonFactory, but decided against it as a manager can update\recover\deactivate. See additional comments into the next point.

DidIonHandle was also a bit confusing. the general pattern that seemed to stick was the way in which DidKey is implemented

I actually found DidKey pattern to be confusing :P ; my mental model is that a Did object should follow what's defined as a DID in the spec). As such, a DID isn't aware of it's context, nor does it know what private keys it has access to. It's simply an identifier.

would be nice to use okhttp instead of ktor so that there's some congruency across kotlin repos. tbh though i haven't dug into kotlin multiplatform enough to know whether ktor actually helps here or not in a way that okhttp doesn't.

No preference whatsoever. I did an example PR in #86 showcasing what it would look like to support any http framework, which I discourage as it seems like overkill.

I think we can merge this PR into main and then address the above as a whole in a separate PR. thoughts?

andresuribe87 added 2 commits October 10, 2023 17:31

Matching all the functionality in TS

d967931

Fine I'm not crazy

5da6cb2

andresuribe87 requested review from mistermoe, nitro-neal, tomdaffurn and phoebe-lew as code owners October 11, 2023 00:04

bradleydwyer self-requested a review October 11, 2023 08:02

nitro-neal reviewed Oct 11, 2023

View reviewed changes

crypto/src/main/kotlin/web5/sdk/crypto/Crypto.kt Outdated Show resolved Hide resolved

nitro-neal reviewed Oct 11, 2023

View reviewed changes

crypto/src/main/kotlin/web5/sdk/crypto/KeyManager.kt Outdated Show resolved Hide resolved

nitro-neal reviewed Oct 11, 2023

View reviewed changes

dids/src/main/kotlin/web5/sdk/dids/DIDIonManager.kt Outdated Show resolved Hide resolved

nitro-neal approved these changes Oct 11, 2023

View reviewed changes

andresuribe87 added 3 commits October 13, 2023 12:54

Merge branch 'did_ion_create' into did_update_op

f2f34cf

Post merge fixes.

30d9767

Back to using less

2e7df3d

andresuribe87 commented Oct 13, 2023

View reviewed changes

crypto/src/main/kotlin/web5/sdk/crypto/Crypto.kt Outdated Show resolved Hide resolved

crypto/src/main/kotlin/web5/sdk/crypto/KeyManager.kt Outdated Show resolved Hide resolved

andresuribe87 added 4 commits October 13, 2023 17:18

recover operation with test

ef0242a

Merge branch 'did_ion_create' into did_update_op

7d8a3c7

tiny merge fix

22c5545

Merge branch 'did_update_op' into did_recover_op

20d3858

bradleydwyer reviewed Oct 15, 2023

View reviewed changes

bradleydwyer reviewed Oct 16, 2023

View reviewed changes

andresuribe87 added 12 commits October 16, 2023 16:52

Use empty lists

115ab15

Prefer iterables

2fc4b71

Merge branch 'did_update_op' into did_recover_op

52ee52c

Post merge

f15f69f

Post merge

60b3841

Merge remote-tracking branch 'origin/secp256k1-low-s' into did_recove…

47f95c5

…r_op

Consistent interface plus examples.

ede85ff

Documentation

1e42181

Fix le warn

6cf9d99

Better names

9dd1714

Assertion method should be the default

5fe23d0

Some fixes after testing

0d9c571

andresuribe87 mentioned this pull request Oct 17, 2023

Support DID Ion Recovery and Deactivate #88

Merged

andresuribe87 and others added 5 commits October 23, 2023 12:25

Merge branch 'main' into did_update_op

f7403a2

Merge branch 'main' into did_update_op

5ec1c9c

Added more validation and tests

ad6a91e

Merge branch 'did_update_op' into did_recover_op

9bdc032

Merge pull request #88 from TBD54566975/did_recover_op

1d0e5ce

Support DID Ion Recovery and Deactivate

andresuribe87 changed the title ~~DID ION Update Operation~~ DID ION Update, Recover, and Deactivate Operations Oct 24, 2023

phoebe-lew approved these changes Oct 24, 2023

View reviewed changes

mistermoe reviewed Oct 24, 2023

View reviewed changes

tomdaffurn approved these changes Oct 25, 2023

View reviewed changes

tomdaffurn mentioned this pull request Oct 25, 2023

Fix null pointer in VC verify #94

Merged

andresuribe87 and others added 3 commits October 24, 2023 22:12

Apply suggestions from code review

ceb31a1

Co-authored-by: Tom Daffurn <[email protected]>

Merge branch 'main' into did_update_op

22a2d06

Do also authentication by default.

02ea4b3

andresuribe87 merged commit 38dfacb into main Oct 25, 2023
4 checks passed

andresuribe87 deleted the did_update_op branch October 25, 2023 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DID ION Update, Recover, and Deactivate Operations #77

DID ION Update, Recover, and Deactivate Operations #77

andresuribe87 commented Oct 11, 2023 •

edited

Loading

codecov-commenter commented Oct 11, 2023 •

edited by codecov bot

Loading

nitro-neal left a comment

andresuribe87 left a comment

bradleydwyer Oct 11, 2023

bradleydwyer Oct 11, 2023

bradleydwyer Oct 11, 2023

bradleydwyer Oct 11, 2023

mistermoe Oct 24, 2023

bradleydwyer Oct 11, 2023

bradleydwyer Oct 11, 2023

bradleydwyer Oct 11, 2023

andresuribe87 commented Oct 16, 2023

bradleydwyer Oct 16, 2023

andresuribe87 Oct 16, 2023

bradleydwyer Oct 16, 2023

andresuribe87 Oct 16, 2023

bradleydwyer Oct 16, 2023

bradleydwyer Oct 16, 2023

bradleydwyer Oct 16, 2023

andresuribe87 Oct 16, 2023

bradleydwyer Oct 16, 2023 •

edited

Loading

andresuribe87 Oct 16, 2023

bradleydwyer Oct 16, 2023

andresuribe87 Oct 16, 2023

bradleydwyer commented Oct 16, 2023 •

edited

Loading

bradleydwyer Oct 16, 2023

andresuribe87 Oct 16, 2023

bradleydwyer Oct 16, 2023

andresuribe87 Oct 16, 2023

mistermoe Oct 24, 2023

andresuribe87 Oct 25, 2023

mistermoe commented Oct 24, 2023 •

edited

Loading

tomdaffurn Oct 25, 2023

andresuribe87 commented Oct 25, 2023

		@@ -151,6 +169,11 @@ public data class OperationSuffixDataObject(
		*/
		public typealias Commitment = String

	throw SignatureException("no assertion methods found in did document")
	throw SignatureException("No assertion methods found in DID document")

DID ION Update, Recover, and Deactivate Operations #77

DID ION Update, Recover, and Deactivate Operations #77

Conversation

andresuribe87 commented Oct 11, 2023 • edited Loading

Overview

Description

How Has This Been Tested?

codecov-commenter commented Oct 11, 2023 • edited by codecov bot Loading

Codecov Report

nitro-neal left a comment

Choose a reason for hiding this comment

andresuribe87 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresuribe87 commented Oct 16, 2023

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

bradleydwyer Oct 16, 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

bradleydwyer commented Oct 16, 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

mistermoe commented Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

andresuribe87 commented Oct 25, 2023

andresuribe87 commented Oct 11, 2023 •

edited

Loading

codecov-commenter commented Oct 11, 2023 •

edited by codecov bot

Loading

bradleydwyer Oct 16, 2023 •

edited

Loading

bradleydwyer commented Oct 16, 2023 •

edited

Loading

mistermoe commented Oct 24, 2023 •

edited

Loading