-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Added encrypted StatusList APIs + enhanced type safety #350
Conversation
Eengineer1
commented
Aug 21, 2023
- Adds encrypted / unencrypted status list distinction.
- Enriched unencrypted.
- Overall encrypted implementation.
- Enhanced check APIs.
- Performant search APIs.
- Adds intermittent payment layer on demand.
- Enhances type safety.
- Monkey patches on globalThis replace issue.
- Relevant documentation.
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.
Really good PR overall and a delight to read through/review!
I did make some changes:
- As discussed, I removed
encrypted
from the top-level response. Please cross-check I got it removed in all places (pretty sure I have, but worth a second pair of eyes). - I have made some changes by leveraging schema reuse a lot more widely within the Credential Status API Swagger docs. This makes it shorter and less verbose (especially large, repeated example blocks). I didn't do this for any of the APIs as it might make the PR harder to read/review if there are non Credential Status APIs to review, but probably worth a pass by Abdulla from a similar lens after this is done.
Next steps:
- The only minor comment is the
/check
API, which asks forstatusPurpose
as a query parameter. This is redundant, since based on the DID, the status list name etc we already search/look it up to determine if it's got payment conditions or not. The input provided instatusPurpose
therefore is already present in the resource metadata and the resource body underresourceType
, in the body, and again in the metadata in body. So IMO this should be removed since user input is not required to determine the status purpose. - For the
/update
actions, thestatusAction
should match the resource type. For example, if the chosen action isrevoke
, but theresourceType
/type
within the resource provided isStatusList2021Suspension
, then this action should fail. Now, I haven't checked did-provider-cheqd to see if it's enforced there instead, but if not, we should. Regardless of did-provider-cheqd, not sure whether we want to catch/handle the same case here (e.g., did-provider-cheqd might already reject, but is there a relevant error message surfaced to the user?)
Hey @ankurdotb I'm not sure this is entirely correct. In my opinion the
|
Underlying search operations are based on user input + validated against the desired action, therefore On underlying did-provider-cheqd functionality, it's deterministically action based + takes relevant information from the credential body, namely the credential status index section, which acts as the single source of truth. Cross-purpose validation handles edge cases. Having that in mind, both So I don't think we should remove either. |
I've enhanced the runtime removal of underlying I'd scope further conversations to relevant changes / technical points required + take additional discussions elsewhere, if no objections. |
Swagger reusability type additions make sense + thanks! |
## [2.9.0-develop.4](2.9.0-develop.3...2.9.0-develop.4) (2023-08-25) ### Features * Added encrypted StatusList APIs + enhanced type safety ([#350](#350)) ([951e2b4](951e2b4))
🎉 This PR is included in version 2.9.0-develop.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
## [2.9.0](2.8.0...2.9.0) (2023-08-29) ### Features * Added encrypted StatusList APIs + enhanced type safety ([#350](#350)) ([951e2b4](951e2b4)) * Change from `did/:did` to `did/search/:didUrl` endpoint and support query parameters for DID endpoint in Credential Service [DEV-3097] ([#331](#331)) ([4e34b76](4e34b76)) ### Bug Fixes * Credential Service Staging - Resolve issue with page not loading and broken login [DEV-3162] ([#358](#358)) ([419d2ec](419d2ec)) * Fix problem with agent initialize [DEV-3146] ([#346](#346)) ([45e1074](45e1074)) * Validate did access on operation ([#351](#351)) ([8bbfce1](8bbfce1))
🎉 This PR is included in version 2.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |