-
Notifications
You must be signed in to change notification settings - Fork 37
rework credential and batch credential endpoint #293
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
Conversation
|
The example in the "Batch Credential Request" section seems to be broken (in the proofs array). There are too many curly braces. The indentation does not seem to be correct either. |
|
The square brackets are also not set correctly. The closing brackets are missing several times. |
POST /batch_credential HTTP/1.1
Host: [server.example.com](http://server.example.com/)
Content-Type: application/json
Authorization: BEARER czZCaGRSa3F0MzpnWDFmQmF0M2JW
{
"credential_requests":[
{
"format":"jwt_vc_json",
"credential_definition":{
"type":[
"VerifiableCredential",
"UniversityDegreeCredential"
]
},
"proofs":[
{
"credential_identifier":"BiologyEngineeringDegree-2023",
"keys":[
{
"proof_type":"jwt",
"jwt":"eyJ0eXAiOiJvcGVuaWQ0dmNpL...Lb9zioZoipdP-jvh1WlA"
},
{
"proof_type":"jwt",
"jwt":"eyJraWQiOiJkaWQ6ZXhhbXBsZ...KPxgihac0aW9EkL1nOzM"
}
]
},
{
"credential_identifier":"CivilEngineeringDegree-2023",
"keys":[
{
"proof_type":"jwt",
"jwt":"eyJraWQiOiJkaWQ6ZXhhbXBsZ...KPxgihac0aW9EkL1nOzM"
}
]
},
{
"credential_identifier":"ElectricalEngineeringDegree-2023",
"keys":[
{
"proof_type":"jwt",
"jwt":"eyJraWQiOiJkaWQ6ZXhhbXBsZ...KPxgihac0aW9EkL1nOzM"
}
]
}
]
}
]
} |
|
I've corrected the JSON structure. The example that you posted is substantially different though. |
|
if a mechanism to better issue multiple copies of the same credential dataset is added to credential endpoint in a backward compatible manner, I am ok having only one endpoint doing it. though we do need to discuss what it means on the mandatory to implement requirements - 1) we can scrap anything around MTI requirements, 2) say particular request (one credential or copies of the credential or multiple credential) is mandatory, 3) everything in credential endpoint is mandatory. |
|
Following up the discussions from DCP Workshop on 19th of April, there are different options how to continue, so therefore please provide feedback/preferences. Please use emoji reaction of your choice to communicate your preferences. |
|
A - make a breaking change to the Batch Credential Endpoint (current proposal - only proof[]) |
|
B - make the Batch Credential Request polymorphic (backwards compatible - proof and proofs[] are allowed) |
|
C - make the Credential and Batch Credential Endpoint polymorphic (backwards compatible - proof and proofs[] are allowed) |
|
We should add in this PR, credential issuer metadata parameter that the issuer uses to indicate how many credential copies it expects. |
|
FWIW I would be in favor of the credential endpoint having a mechanism to issue multiple copies of the same credential dataset (and would be fine with the batch endpoint going away). |
|
credential endpoint with proofs[] issuance of multiple credential istances has been tested successfully at IDunion hackathon at 23/24 of April between 4 participants! |
|
WG call: tobias, Giuseppe, Christian (maybe Brian) to re-review. will merge once there are 4 approvals - hopefully within the next 1-2 days. |
|
will continue the discussion on merging batch/credential endpoints in this issue: #18 |
|
Approved on the proviso that my above minor suggestions are incorperated. |
Co-authored-by: Tobias Looker <tobias.looker@mattr.global>
peppelinux
left a comment
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 can say that I can approve it to not block the world.
I would say that this specification is getting too much complex with mixed conditions. Something I would avoid.
|
@peppelinux the plan is to merge batch credential endpoint and credential endpoint once we merge this PR, but we need to merge this PR to start simplifying things. |
Co-authored-by: Giuseppe De Marco <demarcog83@gmail.com>
|
@peppelinux changes applied |
|
Go! |
c2bo
left a comment
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.
looks good apart from the very minor json error 👍
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
torsten confirmed he is ok with the current PR
Closes #93