TBD54566975 / web5-kt

Apache License 2.0
7 stars 9 forks source link

another take on simpler way of adding a service #313

Closed michaelneale closed 1 month ago

michaelneale commented 1 month ago

For bug https://github.com/TBD54566975/web5-kt/issues/310

an alternative take on https://github.com/TBD54566975/web5-kt/pull/311 which adds a service more directly without taking an update object.

codecov-commenter commented 1 month ago

Codecov Report

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

Project coverage is 73.72%. Comparing base (fb1f9aa) to head (8bff058).

Additional details and impacted files ```diff @@ 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 ``` | [Components](https://app.codecov.io/gh/TBD54566975/web5-kt/pull/313/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/313/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/313/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/313/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | `80.71% <93.33%> (+0.20%)` | :arrow_up: | | [common](https://app.codecov.io/gh/TBD54566975/web5-kt/pull/313/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TBD54566975) | `65.21% <ø> (ø)` | |
angiejones commented 1 month ago

i like this approach

michaelneale commented 1 month ago

ok, let me tighten it up to just have add - as per @jiyoontbd comment as that keeps it focussed, I like it.

michaelneale commented 1 month ago

@jiyoontbd PTAL again

jiyoontbd commented 1 month ago

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 update operation.

the reason being is that even if we have addService() implemented, this method would become tech debt quickly because it isn't considering how each did method handles updates.

the core issue here is that we're only mutating our own BearerDid abstraction by using this method. however, adding service (or updating any part of a did) should have downstream actions that we need to consider for each did method (did:dht handles updates differently than did:jwk, for example)

i've written about this a bit more as an issue in web5-spec. let me know if you have any questions! https://github.com/TBD54566975/web5-spec/issues/157

michaelneale commented 1 month ago

makes sense to me @jiyoontbd! - will close this (maybe keep the branch around temporarily just in case).