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

CWU - W3C Issue Credential #1061

Merged
merged 151 commits into from
Apr 29, 2021
Merged

Conversation

TimoGlastra
Copy link
Contributor

@TimoGlastra TimoGlastra commented Mar 31, 2021

  • Issue Credential v2 with Linked Data proofs
  • BBS+ and Ed25519
  • Issue, verify, derive VC
  • Create, verify VP
  • Add did:key for Ed25519 and Bls12381 keys

Most noticeable breaking changes are in the wallet. Because of the addition of new key types and did methods it had to be extended.

TimoGlastra and others added 30 commits March 17, 2021 15:17
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Karim Stekelenburg <karim@animo.id>
Signed-off-by: Karim Stekelenburg <karim@animo.id>
Signed-off-by: Karim Stekelenburg <karim@animo.id>
Signed-off-by: Karim Stekelenburg <karim@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Karim Stekelenburg <karim@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra
Copy link
Contributor Author

I see the cred ex record webhook event doesn't include the detail record. But the API does return it.

@sklump is that on purpose?

@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented Apr 28, 2021

Hmmm we can't emit webhooks for stateless records anymore. Seems like this PR that was merged yesterday changed that: https://github.com/hyperledger/aries-cloudagent-python/pull/1063/files#diff-fe738d195b46caab22aaa9e802de272264dc1f986e589aad2a2bd0965f98ac14R382

OK -- need to wait for #1123 to be resolved

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2021

Codecov Report

Attention: Patch coverage is 95.43568% with 77 lines in your changes missing coverage. Please review.

Project coverage is 98.87%. Comparing base (0b3de3f) to head (afaf1ac).
Report is 5393 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1061      +/-   ##
==========================================
- Coverage   99.20%   98.87%   -0.34%     
==========================================
  Files         383      427      +44     
  Lines       22139    24118    +1979     
==========================================
+ Hits        21964    23847    +1883     
- Misses        175      271      +96     

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra marked this pull request as ready for review April 28, 2021 14:25
@TimoGlastra
Copy link
Contributor Author

All tests are passing. Ready for review!

@swcurran
Copy link
Contributor

w00t!!!! Congrats! @ianco @andrewwhitehead -- check it out!

Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

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

Do we need to add something like this to config.py?

        extras_require={
            "indy": parse_requirements("requirements.indy.txt"),
            "bbs": parse_requirements("requirements.bbs.txt"),
            "uvloop": {"uvloop": "^=0.14.0"},
        },

Or what are the options for installing the new ursa lib?

Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra
Copy link
Contributor Author

Do we need to add something like this to config.py?

        extras_require={
            "indy": parse_requirements("requirements.indy.txt"),
            "bbs": parse_requirements("requirements.bbs.txt"),
            "uvloop": {"uvloop": "^=0.14.0"},
        },

Or what are the options for installing the new ursa lib?

Ah I was looking how that worked with the extra requirements but couldn't find anything so I assumed it would be automatically detected based on the filename. Thanks!

This explains why I couldn't get it to work in AATH :)

@ianco
Copy link
Contributor

ianco commented Apr 28, 2021

Do we need to add something like this to config.py?

        extras_require={
            "indy": parse_requirements("requirements.indy.txt"),
            "bbs": parse_requirements("requirements.bbs.txt"),
            "uvloop": {"uvloop": "^=0.14.0"},
        },

Or what are the options for installing the new ursa lib?

Ah I was looking how that worked with the extra requirements but couldn't find anything so I assumed it would be automatically detected based on the filename. Thanks!

This explains why I couldn't get it to work in AATH :)

I tried with the above update in aca-py and then installed as aries-cloudagent[indy, bbs]@git+https: ...

There are a couple of errors in AATH I think related to the cred_id_stored, specifically:

https://github.com/hyperledger/aries-agent-test-harness/blob/master/aries-test-harness/features/steps/0453-issue-credential-v2.py#L193

@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented Apr 28, 2021

Once this PR is merged we can flip the uncommented line with the commented line (at the link you posted @ianco) and it should work

Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra
Copy link
Contributor Author

@ianco Seems like we're dealing with some race conditions on the integration tests. It sporadically fails at different tests, but always with the same error. Have you seen this before?

RevocationNotSupportedError: Credential definition not found
2021-04-28T21:26:58.3297564Z >>> after_scenario activated
2021-04-28T21:26:58.3298154Z >>> shutting down active agents ...
2021-04-28T21:26:58.3298665Z     shutting down: Acme
2021-04-28T21:26:58.3319891Z �[0m�[?7h�[0;35mShutting down agent ...�[0m
2021-04-28T21:26:58.3353429Z �[0m�[0m�[?7h�[0mAries      | �[0;31maries_cloudagent.revocation.error.RevocationNotSupportedError: Credential definition not found
2021-04-28T21:26:58.3375039Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m
2021-04-28T21:26:58.3407646Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31mThe above exception was the direct cause of the following exception:
2021-04-28T21:26:58.3429055Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m
2021-04-28T21:26:58.3453138Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31mTraceback (most recent call last):
2021-04-28T21:26:58.3474689Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m  File "/home/indy/aries_cloudagent/admin/server.py", line 161, in ready_middleware
2021-04-28T21:26:58.3495719Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m    return await handler(request)
2021-04-28T21:26:58.3525780Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m  File "/home/indy/aries_cloudagent/admin/server.py", line 198, in debug_middleware
2021-04-28T21:26:58.3545123Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m    return await handler(request)
2021-04-28T21:26:58.3565162Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m  File "/home/indy/.pyenv/versions/3.6.13/lib/python3.6/site-packages/aiohttp_apispec/middlewares.py", line 45, in validation_middleware
2021-04-28T21:26:58.3585094Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m    return await handler(request)
2021-04-28T21:26:58.3603966Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m  File "/home/indy/aries_cloudagent/admin/server.py", line 321, in check_multitenant_authorization
2021-04-28T21:26:58.3633115Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m    return await handler(request)
2021-04-28T21:26:58.3652493Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m  File "/home/indy/aries_cloudagent/admin/server.py", line 365, in setup_context
2021-04-28T21:26:58.3671314Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m    return await task
2021-04-28T21:26:58.3690617Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m  File "/home/indy/aries_cloudagent/messaging/credential_definitions/routes.py", line 258, in credential_definitions_send_credential_definition
2021-04-28T21:26:58.3709163Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31m    raise web.HTTPBadRequest(reason=e.message) from e
2021-04-28T21:26:58.3728858Z �[0m�[0m�[0m�[?7h�[0mAries      | �[0;31maiohttp.web_exceptions.HTTPBadRequest: Credential definition not found

@ianco
Copy link
Contributor

ianco commented Apr 28, 2021

@TimoGlastra I haven't seen that error before. I'm able to run the integration tests for this PR locally with no issues.

@ianco
Copy link
Contributor

ianco commented Apr 28, 2021

BTW I'm also getting these two failures in AATH (with this PR plus fixing the line identified earlier):

Failing scenarios:
  features/0453-issue-credential-v2.feature:41  Issue a JSON-LD Ed25519Signature2018 credential with the Holder beginning with a proposal -- @1.1 
  features/0453-issue-credential-v2.feature:60  Issue a JSON-LD BbsBlsSignature2020 credential with the Holder beginning with a proposal -- @1.1 

(Something to do with prepare-json-ld, I'm not sure if the csv file needs updated or there is just some missing code in the test harness)

Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented Apr 29, 2021

@ianco it should work now by running the open PR in AATH (with a pull)

@ianco ianco merged commit cd4e4b0 into openwallet-foundation:main Apr 29, 2021
@sklump
Copy link
Contributor

sklump commented Apr 29, 2021

I see the cred ex record webhook event doesn't include the detail record. But the API does return it.

@sklump is that on purpose?

The detail records have webhooks of their own.

@sklump
Copy link
Contributor

sklump commented Apr 29, 2021

Yes I changed it a bit. The cred_id_stored property was stored in the cred ex record. However a cred ex can have multiple credentials stored (one per format), so I moved it to the detail record

But any cred exchange can only result in one credential issued (on one format): otherwise the state machine turns into an n-dimensional quantity.

@TimoGlastra
Copy link
Contributor Author

From the issue cred v2 RFC:

The attachment items in the messages are arrays. The arrays are provided to support the issuing of different credential formats (e.g. ZKP, JSON-LD JWT, or other) containing the same data (claims). The arrays are not to be used for issuing credentials with different claims. The formats field of each message associates each attachment with the format (and version) of the attachment.

I interpret this in the way that you could issue multiple credentials as long as they are semantically the same. seems useful IMO

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.

8 participants