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

add update for did #311

Closed
wants to merge 13 commits into from
64 changes: 64 additions & 0 deletions dids/src/main/kotlin/web5/sdk/dids/methods/dht/DidDht.kt
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,22 @@ public class CreateDidDhtOptions(
public val alsoKnownAses: Iterable<String>? = null,
) : CreateDidOptions

/**
* Specifies options for updating a "did:dht" Decentralized Identifier (DID).
* Any options not specified will retain the value from the DID being updated, otherwise
* the values provided will be set in the DID document.
* For example if you wanted to update the services, pass in the full list of `services` you
* would like the updated DID Document to contain.
*
* @property verificationMethods A list of [Jwk]s to add to the DID Document mapped to their purposes
* as verification methods, and an optional controller for the verification method.
* @property services A list of [Service]s that will be set in the DID Document.
* @property publish Whether to publish the DID Document to the DHT after creation.
* @property controllers A list of controller DIDs that will be set in the DID Document.
* @property alsoKnownAses A list of also known as identifiers to be set in the DID Document.
*/
public typealias UpdateDidDhtOptions = CreateDidDhtOptions

private const val PROPERTY_SEPARATOR = ";"
private const val ARRAY_SEPARATOR = ","
private val logger = KotlinLogging.logger {}
Expand Down Expand Up @@ -202,6 +218,54 @@ public sealed class DidDhtApi(configuration: DidDhtConfiguration) {
return BearerDid(didUri, did, keyManager, didDocument)
}

/**
* Updates an existing "did:dht" DID by applying the provided changes to the DID Document.
*
* @param bearerDid The existing "did:dht" DID to update.
* @param options Optional parameters ([UpdateDidDhtOptions]) to specify keys, services, and optional
* publishing during the update.
* @return The updated [BearerDid] instance.
*/
public fun update(bearerDid: BearerDid, options: UpdateDidDhtOptions): BearerDid {
val existingDidDocument = bearerDid.document

val updatedServices = options.services ?: existingDidDocument.service
val updatedControllers = options.controllers ?: existingDidDocument.controller
val updatedAlsoKnownAses = options.alsoKnownAses ?: existingDidDocument.alsoKnownAs
Comment on lines +232 to +234
Copy link
Contributor

@KendallWeihe KendallWeihe May 11, 2024

Choose a reason for hiding this comment

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

I think you might have a bug here with how you're using the Elvis operator (👑 🎸) (?:). The UpdateDidDhtOptions has docs:

 * @property services A list of [Service]s to add to the DID Document.
// ...
 * @property controllers A list of controller DIDs to add to the DID Document.
 * @property alsoKnownAses A list of also known as identifiers to add to the DID Document.

As in, those values are additive, but the Elvis operator is a null coalescing operation, so imagine the existingDidDocument.service has service A, and then options.services has service B & C, then this would update the did:dht's DID Document to now have only services B & C (and not A), but the expected behavior according to the doc comments would be to have services A, B & C.

Copy link
Author

Choose a reason for hiding this comment

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

@KendallWeihe ah - that is a bug in the docs, it isn't adding now, but replacing (ie code is right, comments are not). will fix. thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is the desired behavior to replace completely or add a new service to an existing list?

If it's the former, is this because you are prioritizing immutability?

I am not certain that going with former approach is a good dx. If I just want to add a service, shouldn't the update method do the work internally of:

  1. check if service method is empty
  2. if not empty, turn list mutable
  3. add to list
  4. make the list immutable again

?

Copy link
Author

Choose a reason for hiding this comment

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

yeah desired is to entirely replace it (at least as it stands currently) which preserves immutability but keeps it relatively simple (but alternative means having add/remove etc things explicitly in there for things that are lists, perhaps add/remove for each thing you may want to change and separate methods?)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah desired is to entirely replace it

I agree with this approach, if only for the sake of pragmatism ("crawl before walk")


val updatedVerificationMethods = existingDidDocument.verificationMethod?.toMutableList() ?: mutableListOf()
options.verificationMethods?.forEach { (publicKey, purposes, controller) ->
val verificationMethod = VerificationMethod.Builder()
.id("${existingDidDocument.id}#${publicKey.kid ?: publicKey.computeThumbprint()}")
.type("JsonWebKey")
.controller(controller ?: existingDidDocument.id)
.publicKeyJwk(publicKey)
.build()
updatedVerificationMethods.add(verificationMethod)
}

val updatedDidDocument = DidDocument(
existingDidDocument.id,
existingDidDocument.context,
alsoKnownAs = updatedAlsoKnownAses?.toList(),
controller = updatedControllers?.toList(),
verificationMethod = updatedVerificationMethods,
service = updatedServices?.toList(),
assertionMethod = existingDidDocument.assertionMethod,
authentication = existingDidDocument.authentication,
keyAgreement = existingDidDocument.keyAgreement,
capabilityDelegation = existingDidDocument.capabilityDelegation,
capabilityInvocation = existingDidDocument.capabilityInvocation
)

val updatedBearerDid = BearerDid(bearerDid.uri, bearerDid.did, bearerDid.keyManager, document = updatedDidDocument)

if (options.publish) {
publish(bearerDid.keyManager, updatedDidDocument)
}

return updatedBearerDid
}
/**
* Resolves a "did:dht" DID into a [DidResolutionResult], which contains the DID Document and possible related
* metadata.
Expand Down
28 changes: 28 additions & 0 deletions dids/src/test/kotlin/web5/sdk/dids/did/BearerDidTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,41 @@ import org.mockito.kotlin.spy
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever
import web5.sdk.crypto.InMemoryKeyManager
import web5.sdk.dids.didcore.DidDocument
import web5.sdk.dids.didcore.Service
import web5.sdk.dids.methods.dht.CreateDidDhtOptions
import web5.sdk.dids.methods.dht.DidDht
import web5.sdk.dids.methods.dht.UpdateDidDhtOptions
import web5.sdk.dids.methods.jwk.DidJwk
import kotlin.test.Test
import kotlin.test.assertContains
import kotlin.test.assertEquals
import kotlin.test.assertNotNull

class BearerDidTest {

@Test
fun `update service endpoint of existing did`() {
val keyManager = InMemoryKeyManager()
var myBearerDid = DidDht.create(keyManager, CreateDidDhtOptions(publish = true))

val existingBearerDid: BearerDid = myBearerDid

val serviceToUpdate = Service.Builder()
.id("pfi")
.type("PFI")
.serviceEndpoint(listOf("https://example.com/"))
.build()

val updatedBearerDid = DidDht.update(existingBearerDid,
UpdateDidDhtOptions(services = listOf(serviceToUpdate)))

DidDht.publish(updatedBearerDid.keyManager, updatedBearerDid.document)

assertEquals(1, updatedBearerDid.document.service?.size)
assertEquals(serviceToUpdate, updatedBearerDid.document.service?.first())
}

@Test
fun `getSigner should return a signer and verification method`() {
val keyManager = spy(InMemoryKeyManager())
Expand Down
Loading