another take on simpler way of adding a service #313
Conversation
michaelneale
requested review from
mistermoe,
nitro-neal,
tomdaffurn,
phoebe-lew,
diehuxx,
kirahsapong,
jiyoonie9 and
frankhinek
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #313 +/- ##
==========================================
+ Coverage 73.58% 73.72% +0.13%
==========================================
Files 45 45
Lines 2162 2177 +15
Branches 402 403 +1
==========================================
+ Hits 1591 1605 +14
Misses 376 376
- Partials 195 196 +1
|
There was a problem hiding this comment.
i'm generally more in favor of this PR than #311 , with a couple caveats:
the original issue #310 is wanting a DX that allows a service to be added and not a whole new list of services to be provided to be replaced. whether under the hood a whole new did doc is created should be irrelevant to the client, imo.
in this PR, i'd also like to see removal of deleteService() and clearService() as that was not in the scope of #310 , and according to #311 (comment) we have a bit more thinking to do wrt how to treat update() as a whole.
| * @return A new `BearerDid` instance with the updated DID Document. | ||
| */ | ||
| public fun deleteService(serviceId: String): BearerDid { | ||
| val updatedServices = document.service?.filter { it.id != serviceId } ?: emptyList() |
There was a problem hiding this comment.
if i don't find the serviceId, i nuke the whole list of services?
There was a problem hiding this comment.
nvm, i'm blind. if service is null then i get an empty list
|
i like this approach |
|
ok, let me tighten it up to just have add - as per @jiyoontbd comment as that keeps it focussed, I like it. |
|
@jiyoontbd PTAL again |
| /** | ||
| * Adds a new service to the DID Document and returns a new `BearerDid` instance with the updated document. | ||
| * | ||
| * @param service The service to add to the DID Document. | ||
| * @return A new `BearerDid` instance with the updated DID Document. | ||
| */ | ||
| public fun addService(service: Service): BearerDid { | ||
| val updatedServices = document.service?.toMutableList() ?: mutableListOf() | ||
| updatedServices.add(service) | ||
| val updatedDocument = createUpdatedDocument(updatedServices) | ||
| return BearerDid(uri, did, keyManager, updatedDocument) | ||
| } |
There was a problem hiding this comment.
that would only work if the service was mutable (like it is in other places) - otherwise currently, has to return a new did (with it added to the doc) OR you need to return a new doc and then update the doc in the did (which returns a new did) if sticking with immutable approach in kotlin (correct me if wrong).
There was a problem hiding this comment.
ie, you would do blah.document.addService()... but it wouldn't change document in place but return a new document as List is immutable (can extend to other things that may want to mutate).
There was a problem hiding this comment.
so - we could change them to be mutable lists?
|
hey @michaelneale ! ok, so after some discussion, i'd like to propose that we close this PR for now, and address the issue #310 once we hold some design discussions on how exactly one performs a DID the reason being is that even if we have the core issue here is that we're only mutating our own i've written about this a bit more as an issue in web5-spec. let me know if you have any questions! decentralized-identity/web5-spec#157 |
|
makes sense to me @jiyoontbd! - will close this (maybe keep the branch around temporarily just in case). |
For bug #310
an alternative take on #311 which adds a service more directly without taking an update object.