add update for did #311

michaelneale · 2024-05-07T22:02:36Z

Overview

For bug: #310

need a way to easily modify a did or document. This uses the (I believe) idiomatic kotlin way which keeps the data objects immutable.

codecov-commenter · 2024-05-07T23:35:47Z

Codecov Report

Attention: Patch coverage is 44.82759% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 73.20%. Comparing base (9023afa) to head (616bd78).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
- Coverage   73.58%   73.20%   -0.39%     
==========================================
  Files          45       45              
  Lines        2162     2191      +29     
  Branches      402      413      +11     
==========================================
+ Hits         1591     1604      +13     
- Misses        376      383       +7     
- Partials      195      204       +9

Components	Coverage Δ
credentials	`81.27% <ø> (ø)`
crypto	`49.16% <ø> (ø)`
dids	`79.40% <44.82%> (-1.11%)`	⬇️
common	`65.21% <ø> (ø)`

jiyoonie9 · 2024-05-08T22:18:31Z

dids/src/test/kotlin/web5/sdk/dids/did/BearerDidTest.kt

+      .serviceEndpoint(listOf("https://example.com/"))
+      .build()
+
+    val newServiceList = existingBearerDid.document.service.orEmpty() + serviceToAdd


So I think with the current impl you'll add the existing service again with this line, because you add whatever is passed in options to the existing service list? did I read the impl wrong

hrm - I think that is my mistake, intention wasn't to only add but replace - let me add better tests and fix if needed before merging

jiyoonie9 · 2024-05-08T22:26:43Z

This is great thanks @michaelneale ! one thing I'm wondering is if we should just make service list mutable if that is expected usage instead of trying to preserve immutability which is typical in Kotlin.

@angiejones what other things inside a did are expected to be liable to mutation after initial creation, besides service?

michaelneale · 2024-05-09T00:50:56Z

@jiyoontbd yeah - I wasn't sure about idioms around mutability (as a rule immutable is ideal as long as DX is good enough as it eliminates a whole class of bugs).

michaelneale · 2024-05-09T04:06:32Z

PTAL @jiyoontbd again regarding your pertinent comments

jiyoonie9

One smol comment

jiyoonie9 · 2024-05-09T13:44:33Z

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

+   * publishing during the update.
+   * @return The updated [BearerDid] instance.
+   */
+  public fun update(bearerDid: BearerDid, options: CreateDidDhtOptions): BearerDid {


Thoughts on calling the option type passed in here UpdateDidDhtOptions ?

@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

Cool. If we can call .Create() with CreateOptions and .Update with UpdateOptions that would be ideal

KendallWeihe · 2024-05-11T00:48:23Z

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

+    val updatedServices = options.services ?: existingDidDocument.service
+    val updatedControllers = options.controllers ?: existingDidDocument.controller
+    val updatedAlsoKnownAses = options.alsoKnownAses ?: existingDidDocument.alsoKnownAs


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.

@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!

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:

check if service method is empty

if not empty, turn list mutable

add to list

make the list immutable again

?

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?)

yeah desired is to entirely replace it

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

KendallWeihe

one comment with respect to a bug, but I trust your judgement to merge once addressed, everything else lgtm!

angiejones · 2024-05-13T00:57:34Z

good to merge this one yet, @michaelneale?

michaelneale · 2024-05-13T01:08:20Z

@angiejones I think @jiyoontbd wanted to have a think over it and if we go this approach or move to a mutable approach or perhaps specific functions for everything we want to update (vs an Update/Create options class).

michaelneale · 2024-05-14T06:00:47Z

@jiyoontbd would you like me to (seperately) try a mutable take on this change that lets add/remove services? (and other things, perhaps) to see what it feels like?

mistermoe · 2024-05-15T14:44:21Z

hmm so thinking about this a bit:

ultimately, publishing updates to a pre-existing did:dht's did document requires re-publishing the net new did document to mainline dht effectively clobbering the existing one.

Given ^, i see where you were headed with your copy approach @michaelneale. Definitely agree that while technically correct, the api surface may have not been immediately intuitive.

If we take an update approach the question still remains of whether update takes an entirely new did document OR a series of "patches" to be applied to the pre-existing did document. applying said patches to the pre-existing did document would result in a net new did document that would then be published to mainline.

if update takes an entirely new did document then we would likely want to add methods to DidDocument itself, one of which would be copy. modify the copy, provide the modified copy to update and done.

if update takes a series of patches, we'd probably want to lean on a pre-existing standard for this e.g. json patch.

tbh, both seem a bit painful to use.

Would be helpful to think through 1 use-case for this feature to help guide the design. here's one

one of our private keys has been compromised whose public key is a verification method in our did doc. we need to replace it with a new one

^ requires:

removing the compromised verification method from the did doc
adding a new verification method to the did doc

if we take the patching approach or even a subset thereof (remove and add only) we would at least be able to provide the same functionality that create does in that it will create keys using the provided key manager for you.

update taking an entire did document would require the caller to handle key creation and removal themselves.

mistermoe · 2024-05-15T14:48:56Z

either way, if there is a need for updating did:dht did documents at the moment and absence of it is blocking someone, imo, we could go ahead and merge this PR in so that there's a way to do this (presuming that what's in this PR works!)

generally speaking, updating did docs is certainly something we'll want to consider broadly as the need will inevitably arise for us and others

michaelneale · 2024-05-16T01:22:10Z

@mistermoe no not blocking, so can iterate here. I like the update + patch approach (this one is close to that but does require you to pass in full things to replace it with).

michaelneale · 2024-05-16T09:18:17Z

here is another take: #313 - that is adding/updating/deleting just service, for comparison:

#313

angiejones · 2024-05-16T22:22:14Z

either way, if there is a need for updating did:dht did documents at the moment and absence of it is blocking someone, imo, we could go ahead and merge this PR in so that there's a way to do this (presuming that what's in this PR works!)

generally speaking, updating did docs is certainly something we'll want to consider broadly as the need will inevitably arise for us and others

It already has. fortunately it was easier for the PFI to do so in JS.

michaelneale · 2024-05-17T03:03:17Z

I think I will close this once #313 is blessed (as it is minimalist and doesn't block off any future patterns but solves the issue).

michaelneale added 2 commits May 7, 2024 15:41

test for adding it via manual way

75105db

Add copy function for updating docs and dids

80a35af

michaelneale requested review from mistermoe, nitro-neal, tomdaffurn, phoebe-lew, diehuxx, kirahsapong, jiyoonie9 and frankhinek as code owners May 7, 2024 22:02

michaelneale mentioned this pull request May 7, 2024

[Bug] Cannot update DID documents #310

Open

michaelneale added 2 commits May 8, 2024 08:28

add some docs

5dbfd5a

need to actually read and comprehend error messages

f8fc391

michaelneale added 3 commits May 8, 2024 13:27

add an update helper

1a4cfa9

line length

0ea72f3

add more idiomatic

16b09ac

jiyoonie9 reviewed May 8, 2024

View reviewed changes

michaelneale added 2 commits May 9, 2024 13:41

don't append but replace

ede0c01

adding in the docs

b602da1

jiyoonie9 approved these changes May 9, 2024

View reviewed changes

michaelneale added 2 commits May 10, 2024 10:37

use an updatedhtoptions type alias with clear docs

82dff8e

use update vs create type aliases correctly

47bf20b

jiyoonie9 approved these changes May 10, 2024

View reviewed changes

KendallWeihe reviewed May 11, 2024

View reviewed changes

KendallWeihe approved these changes May 11, 2024

View reviewed changes

clarify docs that it isn't additive

bd8d8b6

remove copy() and do it inline in update()

616bd78

michaelneale changed the title ~~add copy for did and document~~ add update for did May 13, 2024

michaelneale mentioned this pull request May 16, 2024

another take on simpler way of adding a service #313

Closed

michaelneale closed this May 17, 2024

jiyoonie9 mentioned this pull request May 17, 2024

How to add service to an existing DID (and more broadly, how to update) TBD54566975/web5-spec#157

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add update for did #311

add update for did #311

michaelneale commented May 7, 2024

codecov-commenter commented May 7, 2024 •

edited

Loading

jiyoonie9 May 8, 2024

michaelneale May 9, 2024

jiyoonie9 commented May 8, 2024

michaelneale commented May 9, 2024

michaelneale commented May 9, 2024

jiyoonie9 left a comment

jiyoonie9 May 9, 2024

michaelneale May 10, 2024

jiyoonie9 May 10, 2024

michaelneale May 10, 2024

KendallWeihe May 11, 2024 •

edited

Loading

michaelneale May 11, 2024

jiyoonie9 May 11, 2024

michaelneale May 11, 2024

KendallWeihe May 16, 2024

KendallWeihe left a comment

angiejones commented May 13, 2024

michaelneale commented May 13, 2024

michaelneale commented May 14, 2024

mistermoe commented May 15, 2024 •

edited

Loading

mistermoe commented May 15, 2024

michaelneale commented May 16, 2024

michaelneale commented May 16, 2024

angiejones commented May 16, 2024

michaelneale commented May 17, 2024

add update for did #311

add update for did #311

Conversation

michaelneale commented May 7, 2024

Overview

codecov-commenter commented May 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiyoonie9 commented May 8, 2024

michaelneale commented May 9, 2024

michaelneale commented May 9, 2024

jiyoonie9 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

KendallWeihe May 11, 2024 • 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

KendallWeihe left a comment

Choose a reason for hiding this comment

angiejones commented May 13, 2024

michaelneale commented May 13, 2024

michaelneale commented May 14, 2024

mistermoe commented May 15, 2024 • edited Loading

mistermoe commented May 15, 2024

michaelneale commented May 16, 2024

michaelneale commented May 16, 2024

angiejones commented May 16, 2024

michaelneale commented May 17, 2024

codecov-commenter commented May 7, 2024 •

edited

Loading

KendallWeihe May 11, 2024 •

edited

Loading

mistermoe commented May 15, 2024 •

edited

Loading