Data integrity routes - Githubissues

PatStLouis commented 1 month ago

This supersedes #3181 with a better architecture based on the MultikeyManager class.

@jamshale @dbluhm this is mostly the same code as the other pr so review should be light.

I would like to get this in the up comming release if possible.

Adds support for

eddsa-jcs-2022 cryptosuite
Data integrity proof issuance/verification
Data integrity proof set issuance/verification

Here's the tracelog of reviews from the previous PR:

@dbluhm said:

Nitpick but I think I'd rather have these be closer to the request handlers that use them rather than being in a separate file.

The reason I had separated them is because the wallet routes was a very bloated file. In this PR I went with the plugin model for data integrity which is a lot more manageable. The schemas are now closer to the request handlers.

@dbluhm said:

When would we look up this context by the string data-integrity-v2? I get the feeling it might be simpler to just have DIV2_CONTEXT = "...".

This has been addressed by removing linked data support for the time being, therefore this URI is no longer needed for validation. We will review / re haul LD support when implementing eddsa-edfc-2022, it's due for improvement.

Other notable improvements: The crypto suite has now been split into functions, each reflecting a specific algorithm form the spec (can almost be followed line by line). Links are in the doc string.

It now lives at the /vc/di/* endpoints, since this is the vc-data-integrity specification.

Future improvements:

Add proof chains
Add other cryptosuite support

PatStLouis commented 1 month ago

@dbluhm @jamshale I could use a hand here, I re-based on main and now 2 tests are failing which seem unrelated to the work in this PR. I had to re-lock the poetry file since I'm adding a new dependency canonicaljson.

PatStLouis commented 1 month ago

I just ran the test locally and I don't get a single failure.

PatStLouis commented 1 month ago

The error I'm getting is

aries_cloudagent/protocols/issue_credential/v2_0/tests/test_routes.py::TestV20CredRoutes::test_credential_exchange_send_bound_offer_linked_data_error - TypeError: argument of type 'LinkedDataProofException' is not iterable

Can someone shed some light as in how my PR is affecting the LinkedDataProofException type?

jamshale commented 1 month ago

I'm not working today and can have a closer look tomorrow. For the unit tests it looks like there's an error. I'm not sure why it wouldn't happen locally. But there's also a warning and the the github action fails on most warnings. There's something going on here.

For the interop integration tests the AATH just upgraded promp_toolkit and it's conflicting with the version in acapy :/ https://github.com/hyperledger/aries-agent-test-harness/pull/873. Apparently both projects use it. We'll either have to upgradeit in aca-py or revert it in AATH for this workflow to work.

jamshale commented 1 month ago

I noticed you did a general update to the poetry.lock file. Probably using poetry install. Sometimes this can have unintended consequences as it will upgrade all libraries. Not sure if it is a problem here but when adding a library I'd suggest using poetry lock --no-update instead to only add the one library.

PatStLouis commented 1 month ago

@jamshale thanks for the pointer, I bumped the prompt-toolkit version to match the version required by AATH (~=3.0.48), and it's causing even more problems since there's a lot of breaking changes. I think reverting would be preferable.

PatStLouis commented 1 month ago

I copied the lock file from main and applied a poetry lock --no-update as suggested.

PatStLouis commented 1 month ago

@jamshale using --no-update fixed the issue, thank you

PatStLouis commented 1 month ago

Commented on the aath PR about the prompt toolkit version: https://github.com/hyperledger/aries-agent-test-harness/pull/873#issuecomment-2384272893

jamshale commented 1 month ago

Ya. I think I looked at prompt-toolkit a while ago and decided to stay on major version 2. I think reverting it in AATH would be the best. I'll talk to Stephen about it tomorrow.

PatStLouis commented 1 month ago

@dbluhm all tests are now passing except the BDD which are currently broken due to a version upgrade of a package in aath. If I could have a final review before merging this would be appreciated.

jamshale commented 1 month ago

There's a few minor issues reported by sonarcloud. I think most of them can be ignored but the redundant exception class could easily be fixed. https://sonarcloud.io/project/issues?id=hyperledger_aries-cloudagent-python&pullRequest=3261&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true

PatStLouis commented 1 month ago

@dbluhm @jamshale can I get a thumbs up so we can merge this? thank you

PatStLouis commented 1 month ago

@jamshale I applied a couple of the fixes suggested by sonarcloud

PatStLouis commented 1 month ago

Agreed and I think the responses from the api calls are a lot more intuitive.