-
Notifications
You must be signed in to change notification settings - Fork 515
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
feat: Integrate AnonCreds with W3C VCDI Format Support in ACA-Py #2861
Conversation
f6340c3
to
04c4891
Compare
Does this replace the other PR? (In which case we can close it) Also please make sure to include in the description of this PR a ... description of what the PR includes (and if there is any work still in progress) to help us to know what to review. Thanks |
Thank you for your comments. Indeed, this replaces the previous PR and will close that one. I will update the detaild description as soon as I complete the modification and testing. |
Looks good @sarthakvijayvergiya make sure as you wrap up to check all the comments in #2830 as well |
yes, closed the other. this one is not fully ready for review yet, @sarthakvijayvergiya however can you keep the description up to date on what parts are complete? |
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. I added some comments - there are some cleanup tasks I think we need before we can merge the PR, but also a bunch of stuff we can cleanup later (which I think we should add the TODO
comments now so we don't lose track of them).
The demo "issue credential" works with the vc_di
format but the proof request fails (not sure why I haven't looked into it yet).
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Outdated
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Outdated
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/models/detail/vc_di.py
Outdated
Show resolved
Hide resolved
Also ... I think for the demo we should add an option where the user can change Faber's credential_type "on the fly" (i.e. specify on startup but then change while faber is running). Eventually we need to test issuing a credential using "vc_di" and then testing a "legacy" proof request, or issuing a "legacy" credential and then requesting a "vc_di" proof. Again not necessary for this PR ... |
Co-authored-by: Orjiene Kenechukwu <[email protected]> Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
d9e9ce4
to
6a9ee84
Compare
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
I'm still doing some testing, but overall looks good! Just a few minor comments ... A couple of things we need:
For the last one, it should be a simple matter of adding a new scenario to
|
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/handler.py
Outdated
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/models/cred.py
Outdated
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/models/cred_request.py
Outdated
Show resolved
Hide resolved
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/tests/test_handler.py
Outdated
Show resolved
Hide resolved
I would really like to see at least an interactive run of using Credo-TS as a holder, and using ACA-Py as an issuer and verifier. Crucial for the CWU is to see the interop. Thoughts on that? We can engage the devs from Animo in helping with that. |
Could we do this using AATH? (Or AMTH?) |
AATH would be awesome. I don’t know the effort, but we would need someone that knows AATH to help out. The scope is too much for this work, unfortunately. My big concern is the current state of the Credo Backchannel. It needs work. |
52ec3c4
to
bac56be
Compare
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
bac56be
to
8198f47
Compare
I ran a quick test of the alice/faber demo and it failed. I did the Faber: Alice: Then connected and tried to issue a credential:
Tagging @swcurran and @dbluhm because it looks like we're using a |
... also when I run the integration tests they all report as "pass" but when I look through the logs there are errors:
|
Running the alice/faber demo with the
|
I created issue #2923 for this — I think it is an urgent one for 0.12.1. An qualified DID is being used by Alice, being sent in the |
I suspect this is just an issue with the new vc_di but I will do a bit of digging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a couple of issues to resolve. The validation for the "entropy" should be straightforward. Not sure exactly what is happening with the integration tests.
aries_cloudagent/protocols/issue_credential/v2_0/formats/vc_di/models/cred_request.py
Outdated
Show resolved
Hide resolved
@@ -28,6 +28,7 @@ Feature: RFC 0453 Aries agent issue credential | |||
Examples: | |||
| Acme_capabilities | Bob_capabilities | Schema_name | Credential_data | Acme_extra | Bob_extra | | |||
| --public-did --wallet-type askar-anoncreds | --wallet-type askar-anoncreds | driverslicense | Data_DL_NormalizedValues | | | | |||
| --public-did --wallet-type askar-anoncreds --cred-type vc_di | --wallet-type askar-anoncreds | driverslicense | Data_DL_NormalizedValues | | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These integration tests need to work with the sample data provided, which (in this test) is in this file: https://github.com/hyperledger/aries-cloudagent-python/blob/main/demo/features/data/cred_data_schema_driverslicense.json
The data is loaded and the passed through to the integration test. The data is loaded here: https://github.com/hyperledger/aries-cloudagent-python/blob/main/demo/bdd_support/agent_backchannel_client.py#L205
... and the code for the integration test step is here:
... which calls the faber agent to issue the credential.
Somewhere along the line a value is getting dropped (see the other comment re integration test errors).
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianco Do we need to do anything here? I checked the status of integration tests and it works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm taking a look at the integration tests now ...
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
@ianco when you take a look at this, my thought is if the code is correct and passing the current integration tests, we should merge this PR before it gets more outdated, and start a new one for any additional integration tests and also the one for the DIF format (see questions there) Let me know if you think differently. |
One question - if in the DIF pr they are calling it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment re calling the process
method on the legacy credential.
To duplicate, run the alice/faber demo (commands are in a previous comment somewhere), issue a credential, change cred type to indy
(option "1a") and then ask for a proof. The proof should validate.
Yes once this is working we should merge, and then we can deal with outstanding functionality in separate PR's. |
Signed-off-by: Sarthak Vijayvergiya <[email protected]>
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tagging @jamshale for a review.
|
||
## Indicio Developer Demo | ||
|
||
Minimal Aca-Py demo that can be used by developers to isolat and test features: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small spelling mistake on islolat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch we can fix in the next PR tho ...
try: | ||
secret = await self.get_master_secret() | ||
cred_w3c = W3cCredential.load(credential_data) | ||
await asyncio.get_event_loop().run_in_executor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused what this is doing? If it is processing a cred_w3c credential wouldn't it have, and use, a return value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We receive a W3C credential, but then we have to convert to an Indy credential to store. So, we have to process the W3C credential before we can convert to Indy format, but we don't store the W3C version of the credential explicitely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny spelling mistake that might as well get fixed, and one question.
The code is clean and looks well tested!
Summary
Pull request introduces comprehensive support for AnonCreds within the W3C VCDI formats. The changes include the addition of new models, methods, and protocols that enable the Aries Cloud Agent Python to issue, hold, and verify credentials in alignment with W3C's VCDI standards.
Changes Overview
Detailed Changes
Additional Notes
Link to the detailed documentation and specifications: W3C VCDI Integration