TBD54566975 / web5-kt

Apache License 2.0
7 stars 9 forks source link

add update for did #311

Closed michaelneale closed 1 month ago

michaelneale commented 2 months ago

Overview

For bug: https://github.com/TBD54566975/web5-kt/issues/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 commented 2 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/TBD54566975/web5-kt/pull/311/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | Coverage Δ | | |---|---|---| | [credentials](https://app.codecov.io/gh/TBD54566975/web5-kt/pull/311/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | `81.27% <ø> (ø)` | | | [crypto](https://app.codecov.io/gh/TBD54566975/web5-kt/pull/311/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | `49.16% <ø> (ø)` | | | [dids](https://app.codecov.io/gh/TBD54566975/web5-kt/pull/311/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | `79.40% <44.82%> (-1.11%)` | :arrow_down: | | [common](https://app.codecov.io/gh/TBD54566975/web5-kt/pull/311/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | `65.21% <ø> (ø)` | |
jiyoontbd commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

PTAL @jiyoontbd again regarding your pertinent comments

angiejones commented 1 month ago

good to merge this one yet, @michaelneale?

michaelneale commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

here is another take: https://github.com/TBD54566975/web5-kt/pull/313 - that is adding/updating/deleting just service, for comparison:

https://github.com/TBD54566975/web5-kt/pull/313

angiejones commented 1 month ago

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 commented 1 month ago

I think I will close this once https://github.com/TBD54566975/web5-kt/pull/313 is blessed (as it is minimalist and doesn't block off any future patterns but solves the issue).