Skip to content
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

Data integrity routes #3261

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

PatStLouis
Copy link
Contributor

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

Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Copy link
Member

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

Quick pass, I'll have to come back for more review.

Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
Signed-off-by: PatStLouis <[email protected]>
@PatStLouis
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@PatStLouis
Copy link
Contributor Author

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
Copy link
Contributor

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 :/ hyperledger/aries-agent-test-harness#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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@PatStLouis
Copy link
Contributor Author

PatStLouis commented Sep 30, 2024

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

Copy link

sonarcloud bot commented Sep 30, 2024

@PatStLouis
Copy link
Contributor Author

Commented on the aath PR about the prompt toolkit version:
hyperledger/aries-agent-test-harness#873 (comment)

@jamshale
Copy link
Contributor

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
Copy link
Contributor Author

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

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.

3 participants