aries-cloudagent-python

aries-cloudagent-python copied to clipboard

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

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

Why not return a DIDComm v2 problem report?

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

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

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.

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.

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.

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

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

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.

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'

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

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)

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

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

@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!

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

@mepeltier is in the process of putting together tests to pass the code coverage threshold SonarCloud is requiring.

FYI. I'm pretty sure we can actually merge this with the sonarcloud check failing. So if we think there is adequate test coverage below 80% we should be able to go ahead.

Gentle nudge for a review from @jamshale (so we don't have to keep merging upstream and resolving poetry lock conflicts :sweat_smile:). I think the 61% coverage is adequate for the nature of these changes. Uncovered code is mostly configuration only used when the experimental flag is enabled.

Day off in Canada. Will prioritize this for tomorrow morning.

I don't know where the conflicts are coming from. I can't resolve them. If they get resolved we can merge this now.

The conflicts are always occurring with the poetry lock. Let me re-lock it to "clear" the conflict (it's been happening for a while)

Or, Micah will beat me to the punch! xD Should be good to go

Quality Gate failed

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

See analysis details on SonarCloud