DIDComm V2 Initial Implementation

[TheTechmage]

The purpose of this PR is to add basic DIDComm V2 support to ACA-Py. It is our intention to add support gradually with small PRs, rather than one massive PR. To that end, at present, here's what we've added:

• Added --experimental-didcomm-v2 flag to enable the DIDComm V2 code
• Added the ability to decrypt/unpack V2 messages
• When a message is received, we respond back with a DIDComm V1 problem report (there-by, providing a foundation to rely upon when adding protocols) and pack it the same way that we'd pack a DIDComm V2 message

[dbluhm]

@frostyfrog Some minor conflicts in this PR after #2892 was merged

[dbluhm]

Why not return a DIDComm v2 problem report?

[TheTechmage]

I hadn't created a base message class for V2 problem reports to be based off of. My focus was on getting a response back that could be decoded on the other end. So I just used what was already available

[dbluhm]

Some of these comments are requested changes. Others are notes for other reviewers to pay attention to. Others still are open questions. Probably not all of them need to be answered before I would feel good about merging this PR.

That being said, I had a hand in these changes so I defer to other ACA-Py contributors for a full review.

Nice work to this point, @frostyfrog and @mepeltier!

[dbluhm]

Calling out a limitation: experimental v2 support will only work using did:peer:2 right now.

Should we implement it for did:peer:4 as well at this stage?

[mepeltier]

Given how early we are in the process, I don't think its necessary, but at the same time, it might be worth getting it in before our changes are more solidified. It'll also probably make it easier to switch off of did:peer:2, which is a good thing imo

[TheTechmage]

I do believe that it's not limited to did:peer:2. Other did:peer methods should be supported as well. Once I have something better to key off of for outbound, things like did:web should work (in theory). Though you are right in that all did methods, aside from did:peer:2, are currently untested

[dbluhm]

Calling out a fundamental paradigm shift starting here for ACA-Py: to work with DCv2, these changes start using full DID URL references to verification methods to identify keys instead of the base58 encoding of the public key.

And a limitation: There is only one kid associated with a key. Does it make sense for a key to be looked up by multiple different key ids?

[dbluhm]

Now that I think on it, the way we implemented the interface, it doesn't actually preclude the ability to associate a key with multiple kids. But the implementation of the interface does impose this limitation.

[dbluhm]

Calling out another start of a paradigm shift: sender and receiver are identified by DIDs instead of by base58 encoded public keys.

Should the sender/receiver be a kid rather than a DID? Technically, as seen in the unpack result metadata, it is fundamentally a kid that the message is sent to and received by. Perhaps it would be better to prefer keeping that specificity around until it is no longer needed.

[dbluhm]

Should the pack method of DMP that we're using here support multiple recipients?

[TheTechmage]

It probably should. Another problem that I ran into is that it implicitly forward wraps the message, even if we are responding on the same connection. Something I planned to look at later

[dbluhm]

Most of the feedback so far has been from those who have already had a hand in the PR; it would be great to get more feedback from outside that group.

The ability to assign a kid to a key has been identified as something we would like to see before we lose some of the old JSON-LD endpoints. This PR adds that ability incidentally as this same support is required for efficient key lookup when using kids in DIDComm v2. If we end up having issues that prevent this PR from going forward, we should consider breaking that component out.

cc @PatStLouis

[jamshale]

I can't add any good comments about the approach other than it like it's been done well as a non intrusive way to add the changes and the code generally looks really good. This isn't a strong area for me.

We will need to fix the unit testing and add some coverage. Also, a couple integration tests would be good to have setup before we got this merged into v1.0.0. Could probably be done as another task if there's been good manual testing.

[TheTechmage]

I can't add any good comments about the approach other than it like it's been done well as a non intrusive way to add the changes and the code generally looks really good. This isn't a strong area for me.

Thank you for your feedback! I was worried that we weren't getting any eyes on the changes

We will need to fix the unit testing and add some coverage. Also, a couple integration tests would be good to have setup before we got this merged into v1.0.0. Could probably be done as another task if there's been good manual testing.

In regards to testing, I used the docker-compose within this repo of mine. It uses the same underlying didcomm-messaging library to send and receive DIDComm V2 messages and has been used in conjunction with the DIDComm Demo hosted by the DIF. The compose setup has allowed us to quickly iterate upon changes and verify whether or not they work. I'm not quite sure how ACA-Py's unit tests are structured at the affected layers of the stack, but the test script itself is small and easy enough to follow and will hopefully help serve as the basis for the unit tests. I hope that clears up any concerns.

The new code should be gated under the experimental flag and the changes were designed to allow for a series of many PRs, rather than one large PR with everything done all at once. This being the first PR of many.

[jamshale]

The new code should be gated under the experimental flag and the changes were designed to allow for a series of many PRs, rather than one large PR with everything done all at once. This being the first PR of many.

Sounds good. We'll need to at least get the unit tests passing and then we can consider getting this merged as a experimental feature.

[jamshale]

I think this is getting pretty close. I'm not opposed to merging it with the unit testing and integration testing coming later.

Sonarcloud flagged a few minor things that should be addressed. https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&pullRequest=2959&id=hyperledger_aries-cloudagent-python

[mepeltier]

I'm happy to tackle the first few issues that SonarCloud brought up, but I'm not sure about the last one: https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&pullRequest=2959&id=hyperledger_aries-cloudagent-python&open=AY_jne_SO865kDwTsb7n&tab=code
It feels odd to me to define a constant for error text. I'm willing to be convinced otherwise, but it feels like it's not really an issue?

I'm also not sure what's going on with the integration tests, I'll have to take a look at those

[jamshale]

It feels odd to me to define a constant for error text. I'm willing to be convinced otherwise, but it feels like it's not really an issue?

I'm not really concerned about these. We might have to change sonarcloud settings to get this passing. We haven't really done this anywhere in the project. However, when we have the same error message in many places we should have a constant for it. It's just good practice to not need to find and replace all if we ever wanted to change it.

[PatStLouis]

I finally got to give this a look.

@frostyfrog when building the image and running with the flag --experimental-didcomm-v2 I get the error ModuleNotFoundError: No module named 'didcomm_messaging'. I can see it's in the poetry lock file so it should be installed, any idea why I get this behavior? Am I missing something?

To build the image I checked out the feat/didcommv2-proof-of-concept branch.

Full error track:

agent-1  | 2024-06-09 16:38:14,894 aries_cloudagent.commands.start ERROR Exception during startup:
agent-1  | Traceback (most recent call last):
agent-1  |   File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/commands/start.py", line 72, in init
agent-1  |     await startup
agent-1  |   File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/commands/start.py", line 28, in start_app
agent-1  |     await conductor.setup()
agent-1  |   File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/conductor.py", line 127, in setup
agent-1  |     context = await self.context_builder.build_context()
agent-1  |   File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/config/default_context.py", line 69, in build_context
agent-1  |     from didcomm_messaging import (
agent-1  | ModuleNotFoundError: No module named 'didcomm_messaging'

[TheTechmage]

Did you install it with the didcommv2 extra (similar to installing with Askari)? This is how it's done for my test docker container: https://github.com/frostyfrog/didcomm-v2-test-util/blob/main/Dockerfile.acapy

[mepeltier]

Yeah, you'll need to run something like poetry install --all-extras or poetry install --extras "didcommv2 askar" (I think the askar extra is needed for this branch, but I'm not actually 100% sure if that's the case)

[TheTechmage]

Wait, @mepeltier, wasn't your custom import error supposed to trigger to talk about the extra, or am I thinking of something else?

[PatStLouis]

@frostyfrog I'm using the dockerfile from the project, I'll give these solutions a try.

[TheTechmage]

@PatStLouis looking at that Dockerfile, it looks like there's a build argument called: ARG acapy_reqs=[askar,bbs]. If this is changed to include there DIDComm extra (I think it was called didcommv2. It was mentioned earlier), then you should be golden!

[TheTechmage]

It was discussed in Today's ACA-Pug call to add the didcommv2 extra to the docker file, so I'm doing that now.

[sonarcloud]

Quality Gate failed

Failed conditions
30.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud