add update for did

[michaelneale]

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]

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]

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

[michaelneale]

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]

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]

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

PTAL @jiyoontbd again regarding your pertinent comments

[jiyoonie9]

One smol comment

[jiyoonie9]

Thoughts on calling the option type passed in here UpdateDidDhtOptions ?

[michaelneale]

@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

[jiyoonie9]

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

[michaelneale]

yep, done

[KendallWeihe]

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.

[michaelneale]

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

[jiyoonie9]

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

?

[michaelneale]

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

[KendallWeihe]

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]

good to merge this one yet, @michaelneale?

[michaelneale]

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

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

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:

1. removing the compromised verification method from the did doc
2. 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]

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]

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

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

#313

[angiejones]

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]

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