aries-cloudagent-python Add support for revocable credentials in vc

May 24 '24 11:05 EmadAnwer

This line is incorrect: https://github.com/hyperledger/aries-cloudagent-python/pull/2967/files#diff-f7d9c8d89f2442a02ac7a5924be4046e195d411791e4cbfd0fd9e7e2ecb987adL557

if cred["proof"][0].get("rev_reg_id"):

You can fetch these attributes from the credential, see for example:

https://github.com/ianco/aries-cloudagent-python/commit/1903eb6d45000ff57b65a2470bcf747adc8e5a39

(Not the most elegant approach but you can use it as an example)

May 31 '24 18:05 ianco

FYI the SonarCloud errors (if you don't have access to the details) are:

Failed conditions
 [42.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=hyperledger_aries-cloudagent-python&pullRequest=2967&metric=new_coverage&view=list) (required ≥ 80%)
 [24.7% Duplication on New Code](https://sonarcloud.io/component_measures?id=hyperledger_aries-cloudagent-python&pullRequest=2967&metric=new_duplicated_lines_density&view=list) (required ≤ 3%)

The coverage is just related to the unit tests. The duplication is on the new create_credential_w3c() method which duplicates a lot of the existing create_credential() code (mostly the blocks around the revocation registry handling, which could be re-factored into helper methods).

Jun 04 '24 15:06 ianco

Thanks for reviewing, im editing it and i will push asap

Jun 04 '24 16:06 EmadAnwer

@EmadAnwer the coverage test is still failing on the VCDICredFormatHandler class (in formats/vc_di/handler.py).

There are a couple of unit tests already that are marked as "skipped" (since they were added before revocation was implemented, I suspect just copied form the indy version of the unit test) - async def test_issue_credential_revocable(self): and async def test_issue_credential_non_revocable(self): - can you take a look at those and see what it would take to make these work with your updated revocation support?

Otherwise everything in the PR looks good to me.

Jun 05 '24 14:06 ianco

@EmadAnwer the coverage test is still failing on the VCDICredFormatHandler class (in formats/vc_di/handler.py).

There are a couple of unit tests already that are marked as "skipped" (since they were added before revocation was implemented, I suspect just copied form the indy version of the unit test) - async def test_issue_credential_revocable(self): and async def test_issue_credential_non_revocable(self): - can you take a look at those and see what it would take to make these work with your updated revocation support?

Otherwise everything in the PR looks good to me.

@ianco Thanks for the review and suggestions, sure I will take a look into it and update the PR

Jun 05 '24 14:06 EmadAnwer

@EmadAnwer it looks like the most recent commit (removing the dup code) is missing the DCO signoff

Jun 06 '24 17:06 ianco