-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added did ion deactivation endpoint. #646
base: main
Are you sure you want to change the base?
Added did ion deactivation endpoint. #646
Conversation
doc/swagger.yaml
Outdated
@@ -2927,6 +2947,53 @@ paths: | |||
summary: Updates a DID document. | |||
tags: | |||
- DecentralizedIdentityAPI | |||
/v1/dids/{method}/{id}/deactivation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be more restful as a delete to /v1/dids/{method}/{id}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a that taken by SoftDelete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, I see. awkward because we both need to handle the service's concept of deletion and the did methods
I'm inclined to merge the two...
Something lke -- if the DID method supports deactivation, the DELETE
to /v1/dids/{method}/{id}
does both delete and deactivation. If the DID method does not support deactivation, we just do a delete.
Reason being, it's more confusing to support both separately. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that, will update.
pkg/service/did/ion.go
Outdated
if storedDID.Deactivated { | ||
return storedDID, nil | ||
} | ||
storedDID.Operations = append(storedDID.Operations, deactivateRequest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems off - shouldn't the result be the processed deactivate request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which "result" are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my thought was that we should not rely on our internal source of truth, but have some operation to wait for an anchor and have that reflected as the result. but I'm thinking that's too complex. just need to make sure these properties are correct. (they look to be correct)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're eventually going to need to communicate to devs that some new state hasn't been published. That said, I agree that it's too complex for now.
I made my best to ensure that all the properties were correct.
…e' into issue_340_deactivate
Codecov Report
@@ Coverage Diff @@
## main #646 +/- ##
==========================================
+ Coverage 25.72% 26.06% +0.33%
==========================================
Files 56 57 +1
Lines 6254 6404 +150
==========================================
+ Hits 1609 1669 +60
- Misses 4369 4442 +73
- Partials 276 293 +17
|
Overview
As part of #340, this PR adds support for deactivation. See the API documentation for more details.
Description
This PR involves a couple of changes so that we return resolution results.
How Has This Been Tested?
Unit tests.