Skip to content
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: Support DID accreditations [DEV-4415] #601

Open
wants to merge 32 commits into
base: develop
Choose a base branch
from

Conversation

DaevMithran
Copy link
Contributor

@DaevMithran DaevMithran commented Sep 23, 2024

  • accredit/issue api
  • accredit/verify api
  • Add relevant tests

@DaevMithran DaevMithran changed the title feat: Support DID accreditations feat: Support DID accreditations [DEV-4415] Sep 23, 2024
@ankurdotb
Copy link
Contributor

Task linked: DEV-4415 Start on implementations

@DaevMithran DaevMithran marked this pull request as ready for review September 25, 2024 06:57
@DaevMithran DaevMithran force-pushed the accreditation branch 5 times, most recently from c8adad8 to bb4d8c5 Compare September 25, 2024 10:24
@DaevMithran DaevMithran force-pushed the accreditation branch 2 times, most recently from 6d5caae to 14377ca Compare September 30, 2024 11:05
@DaevMithran DaevMithran force-pushed the accreditation branch 4 times, most recently from 0958a9b to c96b540 Compare September 30, 2024 12:13
eventTracker.emit('track', trackInfo);

return response.status(StatusCodes.OK).json({
didUrls: [
Copy link
Contributor Author

@DaevMithran DaevMithran Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be helpful to receive suggestions on the response schema

Currently it is

  • didUrls: [ didUrlwithPathparam, didUrlwithQueryParam ]
  • accreditation: the vc jwt

Copy link
Contributor

@ankurdotb ankurdotb Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the query param version, is the name of the accreditation escaped correctly in case it has characters like spaces? Does it even allow spaces?

Yes @ankurdotb spaces are allowed, the query params are parsed by the express router

What happens if the credential issuance part is successful, but creating the resource fails?

We return a 500

public async verify(request: Request, response: Response) {
// Extract did from params
let { verifyStatus = false, allowDeactivatedDid = false } = request.query as VerifyCredentialRequestQuery;
const { accreditation, policies, subjectDid } = request.body;
Copy link
Contributor Author

@DaevMithran DaevMithran Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here accreditation in request body is a didUrl.

Can I just receive it as did, resourceName, resourceType or resourceId separately?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you deconstruct a DID URL to those parts? We do something similar on Creds API, IIRC.

src/app.ts Outdated Show resolved Hide resolved
src/app.ts Outdated Show resolved Hide resolved
src/controllers/api/accreditation.ts Show resolved Hide resolved
src/controllers/api/accreditation.ts Outdated Show resolved Hide resolved
eventTracker.emit('track', trackInfo);

return response.status(StatusCodes.OK).json({
didUrls: [
Copy link
Contributor

@ankurdotb ankurdotb Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the query param version, is the name of the accreditation escaped correctly in case it has characters like spaces? Does it even allow spaces?

Yes @ankurdotb spaces are allowed, the query params are parsed by the express router

What happens if the credential issuance part is successful, but creating the resource fails?

We return a 500

Comment on lines +448 to +452
* connector:
* type: string
* enum:
* - verida
* - resource
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verida won't be an allowed connector for this, will it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankurdotb we still haven't removed verida connector from our code entirely, we just disabled it. Should I remove it?

* - issuerDid
* - subjectDid
* - schemas
* example:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad example?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test being skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to debug this in a separate PR. JSONLD issuance using Jsonwebkey 2020 is failing

Comment on lines +11 to +13
"accreditationName": "accreditAccreditation",
"parentAccreditation": "did:cheqd:testnet:5RpEg66jhhbmASWPXJRWrA?resourceName=authorizeAccreditation&resourceType=VerifiableAuthorisationForTrustChain",
"rootAuthorization": "did:cheqd:testnet:5RpEg66jhhbmASWPXJRWrA?resourceName=authorizeAccreditation&resourceType=VerifiableAuthorisationForTrustChain"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like same DID as parent and root. Can we do at least two different levels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankurdotb it is done it the attestation test. At one point there should be a single level when the parent is root

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants