-
Notifications
You must be signed in to change notification settings - Fork 10
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
add update for did #311
Changes from 9 commits
75105db
80a35af
5dbfd5a
f8fc391
1a4cfa9
0ea72f3
16b09ac
ede0c01
b602da1
82dff8e
47bf20b
bd8d8b6
616bd78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -202,6 +202,47 @@ 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 ([CreateDidDhtOptions]) to specify additional keys, services, and optional | ||
* publishing during the update. | ||
* @return The updated [BearerDid] instance. | ||
*/ | ||
public fun update(bearerDid: BearerDid, options: CreateDidDhtOptions): 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (👑 🎸) ( * @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 = existingDidDocument.copy( | ||
service = updatedServices?.toList(), | ||
controller = updatedControllers?.toList(), | ||
alsoKnownAs = updatedAlsoKnownAses?.toList(), | ||
verificationMethod = updatedVerificationMethods | ||
) | ||
|
||
val updatedBearerDid = bearerDid.copy(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. | ||
|
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.
Thoughts on calling the option type passed in here
UpdateDidDhtOptions
?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.
@jiyoontbd great idea - added a public typealias for that reason (it is the same type) but with its own docs to make semantics clear. PTAL
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.
Cool. If we can call .Create() with CreateOptions and .Update with UpdateOptions that would be ideal
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.
yep, done