Skip to content

Conversation

judithmh
Copy link

As a group of university students we implemented the protocols issue_credential and present_proof for DIDComm v2. The encryption is currently still the same as for DIDComm v1.
 In addition to the tests of v3, the new endpoints can be tested using a DIDComm v1 connection for now. As soon as there is a decision on how to implement/extend the existing connection for DIDComm v2, the intention is to implement it and use its information regarding the DIDComm version for choosing the encryption type (and in case of DIDComm v2 to use the encryption which is already implemented in the askar wallet).

Signed-off-by: judithmh <40442827+judithmh@users.noreply.github.com>
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2022

Codecov Report

Merging #2019 (ccd6990) into main (8a0d7cb) will increase coverage by 0.11%.
The diff coverage is 93.09%.

❗ Current head ccd6990 differs from pull request most recent head e99fd6d. Consider uploading reports for the commit e99fd6d to get more accurate results

@@            Coverage Diff             @@
##             main    #2019      +/-   ##
==========================================
+ Coverage   93.50%   93.61%   +0.11%     
==========================================
  Files         539      592      +53     
  Lines       34666    38965    +4299     
==========================================
+ Hits        32413    36478    +4065     
- Misses       2253     2487     +234     

@hkny
Copy link

hkny commented Nov 18, 2022

@swcurran I would like to talk about this PR during the next Acapug call to give an update on where we are on DIDComm v2 implementation on Aca-Py. This PR covers the message structure required by DIDComm v2 (according to WACI Issue Cred and Present Proof v3). It still uses the old envelope.

@swcurran
Copy link
Contributor

Sounds good. I assume you are involved in the Aries DIDComm V2 working group, and this will be raised/discussed there? I'm thinking from there we can discuss at an ACA-Pug meeting?

@2byrds
Copy link

2byrds commented Nov 21, 2022

Good discussion with @hkny at the first Aries-DIDCommv2-WG meeting today. It would be very interesting to canvas each agent implementation to see how they did/will handle this kind of transition.

Signed-off-by: judithmh <40442827+judithmh@users.noreply.github.com>
Signed-off-by: judithmh <40442827+judithmh@users.noreply.github.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

This is great!!!

}


class LDProofCredFormatHandler(V30CredFormatHandler):
Copy link
Contributor

Choose a reason for hiding this comment

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

The formats are identical between DIDComm v2 and v3. I think we should try to reuse the handlers as much as possible to remove overhead. If something changes about the format, it needs to be updated in 2 places (3 now for indy)

Copy link
Contributor

Choose a reason for hiding this comment

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

In AFJ we have the following model:

  1. Credentials Api provides interface to issue credentials. You provide a protocol version which indicates the protocol version, but the api is the same otherwise for all versions
  2. Credential Services are implemented for the different protocol version. The v1 protocol version has a hard depenency on the indy format and will throw an error if you try to issue a format that is other than indy.
  3. Credential Format Service provide the format specific implementation. This can be used by the credential services of different versions. The indy credential format service is currently being used by the v1 and v2 credential service and once the v3 credential service is added it will also leverage the indy format. This means the indy format is reused as much as possible and we get consistent behaviour whether you're using v1, v2 or v3.

Not sure if this is the approach ACA-Py wants to take, but I'm a bit worried about copying everything over when a new version is added instead of creating reusable abstract classes and interfaces.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you very much for your comment @TimoGlastra.

We went for the approach of creating the v3 folder after the decision in this meeting (the relevant minutes of the recording of the meeting are 15:45-21:40):

https://wiki.hyperledger.org/display/ARIES/2022-04-19+Aries+Cloud+Agent+-+Python+Users+Group+Community+Meeting?preview=/62249226/62249966/20220419%20ACA-Pug%20Community%20Call%20Recording.mp4

Maybe someone of the ACA-Py community could also have a look at the PR because I am not sure how to proceed now (next step would be implementing Out-of-Band for DIDComm v2).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the discussion here is about the present proof v2 vs the present proof v3 implementation. These protocols are indeed different and I see the need for a separate implementation. However the attachment formats are separate RFCs 0592, 0593 & 0510.

The attachments have been extracted out of the issue credential / present proof protocols, with one of the reasons being that they could be versioned separately (i.e. a change to the issue credential protocol, doesn't require the attachment to be changed, and vice versa). Thus, the implementation for the format whether you're using v2 or v3 of the protocol is exactly the same. If a new format version is introduced it will require a new format implementation, but both v2 and v3 of the protocol could leverage that new version format.

For me it's on the same level that we don't have a separate implementation of the connection protocol whether we're using issue credential or present proof protocol. Connections are a separate RFC with a separate implementation that can be reused by other parts of ACA-Py. I see the same for attachments formats. You implement them once and they can be reused by different credential protocol versions.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@andrewwhitehead
Copy link
Contributor

Looking at the current state of the issue-credential 3.0 protocol, it seems like the spec is lacking behind the recommended usage at the moment. The WACI-DIDComm spec is using a credential manifest attachment rather than attaching a credential preview to the offer, and there are some DIDComm v1 artifacts as well. I think we need to wait for that spec to be updated.

https://identity.foundation/waci-didcomm/#issuance-2
https://github.com/decentralized-identity/waci-didcomm/tree/main/issue_credential

@dbluhm
Copy link
Contributor

dbluhm commented Jul 11, 2023

There are a few challenges I see with this PR:

  • There is a lot of copy-paste-edited code; to make future maintenance of these protocols feasible, we really need a common base
  • DIDComm v2 structures are adopted but no change in wire format is implemented
  • The set of changes is massive; I would have liked to be able to chime in on decisions made a bit at a time (i.e. first a PR for DIDComm v2 message models, then a PR with attachment structures updated for DIDComm v2, then a PR refactoring ICv2 components to be more reusable from an ICv3 implementation, etc.)

Unfortunately, I'm not sure I see a path forward for this PR as it stands. I acknowledge the significant efforts that went into it -- thank you @judithmh for your contributions.

Thoughts, @swcurran @TimoGlastra @usingtechnology?

@swcurran swcurran closed this Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants