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

Initial code migration from anoncreds-rs branch #2596

Merged
merged 20 commits into from
Nov 27, 2023
Merged

Initial code migration from anoncreds-rs branch #2596

merged 20 commits into from
Nov 27, 2023

Conversation

ianco
Copy link
Contributor

@ianco ianco commented Nov 6, 2023

No description provided.

@ianco ianco added the AnonCreds Ledger Agnostic AnonCreds label Nov 6, 2023
@ianco ianco self-assigned this Nov 6, 2023
@swcurran
Copy link
Contributor

swcurran commented Nov 6, 2023

@ianco FYI: all of the AATH ACA-Py to ACA-Py tests pass on for this PR.

@ianco
Copy link
Contributor Author

ianco commented Nov 6, 2023

@ianco FYI: all of the AATH ACA-Py to ACA-Py tests pass on for this PR.

Thanks for the info! This is basically a non-destructive PR so that's good news. There are still a couple of unit tests failing and I need to "hook in" the anoncreds stuff so it gets initialized on startup (and also review the unit test coverage of the new code) before this PR is finalized ...

@ianco ianco marked this pull request as ready for review November 7, 2023 20:32
@ianco
Copy link
Contributor Author

ianco commented Nov 7, 2023

I think this PR is ready to merge. It adds the new "anoncreds" code and is non-destructive (demo, unit and integration tests still run).

I still need to hook up the anoncreds code to aca-py startup and inventory the unit tests in the new anoncreds package but I think I can do that in the next PR.

@swcurran swcurran marked this pull request as draft November 7, 2023 20:37
@swcurran
Copy link
Contributor

swcurran commented Nov 7, 2023

We need to hold off on this until we get the 0.11.0 release completed, so I moved this back to Draft. Reviews are welcome though!!

@ianco
Copy link
Contributor Author

ianco commented Nov 7, 2023

We need to hold off on this until we get the 0.11.0 release completed, so I moved this back to Draft. Reviews are welcome though!!

OK sounds good. I'll keep working on this PR in the meantime ...

@ianco ianco changed the title WIP initial code migration from anoncreds-rs branch Initial code migration from anoncreds-rs branch Nov 9, 2023
@ianco
Copy link
Contributor Author

ianco commented Nov 15, 2023

Added a few comments regarding unit test scope, I think we need to add some but not go overboard. I suggest:

  • updating the routes unit tests (adding tests for the main routes)
  • adding unit tests for the main issuer, holder and verifier classes

There are already a few tests for the registries, and there are existing tests elsewhere in aca-py similar to the ones we need to add for aca-py, that whoever is going to be working on the unit tests can use as a model.

@ianco
Copy link
Contributor Author

ianco commented Nov 21, 2023

Question for @dbluhm @andrewwhitehead @swcurran ...

For the V20 credential Format enum (which is defined here: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/protocols/issue_credential/v2_0/messages/cred_format.py#L31) ...

... I had initially thought of adding a new Enum for ANONCREDS (in addition to INDY) but after some noodling in the code I now think the INDY enum should depend on which wallet type is selected (askar vs askar-anoncreds).

If askar (the existing code) (or if running wallet type indy although this is deprecated):

        INDY = FormatSpec(
            "hlindy/",
            V20CredExRecordIndy,
            DeferLoad(
                "aries_cloudagent.protocols.issue_credential.v2_0"
                ".formats.indy.handler.IndyCredFormatHandler"
            ),
        )

If askar-anoncreds:

        INDY = FormatSpec(
            "hlindy/",
            V20CredExRecordIndy,
            DeferLoad(
                "aries_cloudagent.protocols.issue_credential.v2_0"
                ".formats.anoncreds.handler.AnonCredsCredFormatHandler"
            ),
        )

@dbluhm are there differences in the attachment when using the new anoncreds library?

(PS the AnonCredsCredFormatHandler is the updated IndyCredFormatHandler from the anoncreds-rs branch)

@dbluhm
Copy link
Contributor

dbluhm commented Nov 21, 2023

We opted to continue using the existing indy format, at least initially. I think it makes sense to eventually phase out any use of "indy" as a stand in for "anoncreds" but I think that's a later problem. There are no differences in the attachment when using the anoncreds library.

@swcurran
Copy link
Contributor

The fun in this is that if the AnonCreds in W3C format comes in soon enough, perhaps we can just use W3C format for everything…

@ianco
Copy link
Contributor Author

ianco commented Nov 21, 2023

The fun in this is that if the AnonCreds in W3C format comes in soon enough, perhaps we can just use W3C format for everything…

Is this going to affect the wire protocol (agent-to-agent messages) or just the payload to the issue credential api?

@ianco
Copy link
Contributor Author

ianco commented Nov 21, 2023

We opted to continue using the existing indy format, at least initially. I think it makes sense to eventually phase out any use of "indy" as a stand in for "anoncreds" but I think that's a later problem. There are no differences in the attachment when using the anoncreds library.

This is good. The api format won't be affected by the switch from askar to askar-anoncreds then.

@ianco ianco marked this pull request as ready for review November 27, 2023 15:59
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ianco
Copy link
Contributor Author

ianco commented Nov 27, 2023

@swcurran all tests passed

Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

w00t!!

@swcurran swcurran merged commit a397e6b into openwallet-foundation:main Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AnonCreds Ledger Agnostic AnonCreds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants