Merging changes from anoncreds-rs branch

[andrewwhitehead]

At the moment this branch has diverged significantly from the main branch, and there are likely too many conflicts to merge it directly, so we need to look at how to merge the changes that are there in separate phases. The branch also currently adds an anoncreds module instead of renaming/reusing classes that are present in the indy module, so it would be best to avoid code duplication while merging and ensure that we maintain test coverage.

For reference, there is a diff between the two branches here: https://github.com/hyperledger/aries-cloudagent-python/compare/anoncreds-rs (select the Files Changed tab)

Suggested steps are below, to be discussed and updated:

0. Rename the indy module to anoncreds, and rename IndyHolder et al to AnonCredsHolder et al.

• Merge into main at the end of this work.
• Indy SDK may still be supported, and CredX is still supported.

0a. Consider: Remove Indy SDK support?

1. Merge (from the anoncreds-rs branch via cherry picks or copy/paste/tweak as makes sense) the generic AnonCreds resolver/registry support

• Code contained in anoncreds/base.py and anoncreds/default
• Update credential_definitions/routes and schemas/routes to use the abstract registry class (this removes the custom handling around BaseMultitenantManager in those places)
  • per tenant ledger settings (copied into multiple places)
• Update the issue-credential and present-proof indy handlers and to use the new resolver (see diff)

2. Merge the AnonCredsHolder/Issuer/Verifier classes, using IndyHolder/Issuer/Verifier as the base classes and supporting dependency injection

• This may require adding new methods to the base classes
• Tests should be gated by a new 'anoncreds-rs' flag defined in conftest.py
• Initial merge may omit revocation support
• Should include adding the anoncreds library to the images used for testing on CI

2.5. Update the routes

• Create the new /anoncreds/schema, /anoncreds/creddef, /anoncreds/revocation routes
• Change/stub the existing ones to point to the new implementations -
• drop ones that we don't want anymore -- the ones that allow the controller to manage revocation, endorser
• consider which to add back for "cleanup a mess" scenarios
  • draw on our experience -- not for backwards compatibility.

3. AnonCreds revocation support

• Includes various changes in protocols/present_proof/indy/pres_exch_handler.py for example, to handle revocation status lists instead of deltas. Maybe this can be handled more generically in the IndyVerifier base class.
• Includes updates to revocation/manager.py (as appropriate)
• Includes tails handling updates

3.5 Endorsement Updates

• Hide the endorsement from the controller and put it into the Indy registry code

4. BDD test updates

5. Data updates/migrations

[ianco]

I'd like to add a step - could be done in parallel with step 0:

0aa. Get the anon-creds branch in sync with main

This is necessary to ease the pain of combining the code from anoncreds-rds back into main - the work in steps 1-2-3 above. There are many many conflicts between the two branches, and most are due to formatting differences introduced in main (replacing flake8 with ruff) which complicate thing enormously.

The work can be broken down as:

0aa.i - branch main at commit 9fc598f and merge into anoncreds-rs (this introduces ruff but not all the re-formatted code)
0aa.ii - run the formatting in the anoncreds-rs branch to sync up the formatting between the branches
0aa.iii - merge the rest of main and hopefully there will be a manageable amount of conflicts

Then with the sync'ed up code the above steps 1-2-3 can proceed a bit more smoothly.

I took an initial crack at 0aa.i (see branch https://github.com/ianco/aries-cloudagent-python/tree/ruff_updates_pre) and there were about ~20 conflicted files to deal with, however one issue is that the new anoncreds library was introduced into the anoncreds-rs branch as a pip dependency, but the main branch was switched over to poetry.

So ... I can take a crack at updating the poetry build stuff in the branch, or - better would be if I can get some help from @dbluhm (since he did both of the original updates on both branches).

[dbluhm]

To add the anoncreds-rs library as a dependency in poetry, you can just add the following line to tool.poetry.dependencies:

anoncreds-rs = {version = "0.1.0", optional = true}

And then add anoncreds-rs to the askar extra:

askar = [
      "aries-askar",
      "indy-credx",
      "indy-vdr",
      "anoncreds-rs"
]

I would ignore any conflicts in the poetry.lock and just let poetry regenerate it after the above changes to the pyproject.toml:

poetry lock

Not sure if that was the help you were looking for. Let me know if that helps!

[ianco]

Hi @dbluhm thanks I'll give it a try and let you know if I have any questions

[ianco]

I've opened a draft PR #2571 containing the updates from main up to ruff. (not tested yet just opening up for visibility).

I did a quick test merging from the top of main and there are about 10 conflicts. I'm going to give this PR ^^^ some testing first and then try merging from the rest of main. @dbluhm @usingtechnology if you guys could take a peek to make sure I haven't mucked anything up I'd appreciate it.

[ianco]

@dbluhm on my local when I run poetry install I get this error:

Because aries-cloudagent depends on anoncreds-rs (~0.1.0) which doesn't match any versions, version solving failed.

[dbluhm]

@ianco looks like I got the name wrong; it seems to be uploaded to PyPI as anoncreds rather than anoncreds-rs

[ianco]

@ianco looks like I got the name wrong; it seems to be uploaded to PyPI as anoncreds rather than anoncreds-rs

Thanks @dbluhm !

[ianco]

I've got all the code from main in the #2751 PR into the anoncreds-rs branch. It's not "perfect" in that there are some failing tests (and probably some code I've inadvertently clobbered). Depending on what are the next steps it may be "good enough", in that it reduces the diffs between the 2 branches from over 700 files down to about ~100 files.

I'm not sure the approach recommended by @andrewwhitehead above is best, an alternate is to create net new AnonCreds holder, issuer etc classes (rather than re-naming the existing Indy classes) and then pull all the AnonCreds code over as net new (create new classes, new endpoints etc) - and add a new credential format to the V2 endpoints called anoncreds - and then once all the new AnonCreds functionality is built out, we can:

• do one release that includes both the indy and anoncreds code/endpoints
• tell everyone to switch over
• then delete all the indy code (the next release would only include anoncreds)

I think this would simplify the copying over of code into the main branch, because you're not dealing with any of the legacy Indy stuff. Also the Indy code would all continue to function as the new anoncreds is getting built out. (As an aside I think the revocation and endorser code is going to take longer to re-write than anyone expects, the devil is in the details).

[swcurran]

@ianco — thanks — good stuff! Please connect with @andrewwhitehead directly and make a call about the right thing to do and go forward. We don’t want suggestions sitting out there for anytime such that we get stalled for lack of a decision. Let’s keep moving…

Worst case — ACA-Pug is tomorrow morning at 8AM to discuss.

[swcurran]

To be clear — there is no expectation that the revocation and endorser code is going to be fast to implement and test. Rather, that we know better than the last time we worked on it what we want. There will be trouble in the details :-).

AFAIK large parts of the revocation work is done. The endorser rework is pretty much entirely to be done.

[ianco]

• Indy SDK may still be supported, and CredX is still supported.

Is the intent to support 2 flavours of --wallet-type=askar? (one with credx and one with anoncreds?)

(Or can someone clarify the relationship between the various libraries?)

@andrewwhitehead @dbluhm @swcurran

[swcurran]

This is the crux of the issue with your change to the plan towards creating a separate “AnonCreds” model without impacting the existing “Indy” model. We implicitly wind up with support for both automatically — CredX for the existing Indy, AnonCreds RS for the new AnonCreds model.

I think your question is really for you. Do you see us have a main branch that has both CredX and AnonCreds-RS imported and used beside each other? How do you see that working?

In the long term, we will only want AnonCreds-RS and we want to archive CredX. The question is the backwards compatibility as we get to that goal.

[ianco]

In the long term, we will only want AnonCreds-RS and we want to archive CredX.

So does that mean anoncred-rs replaces credx? And we run acapy with --wallet-type=askar (although once we get rid of indy-sdk this becomes unnecessary)

Related question is anyone still using aca-py with indy-sdk or have we migrated everyone to askar?

[swcurran]

Yes. AnonCreds-RS was initially a copy of CredX (from indy-shared-rs) and then evolved to remove the few “Indy-isms” that were built in. Underlying both is anoncreds-clsignatures-rs, which is has the CL Signature processing code. So it makes sense to only have one of them. The only reason to have both is for backwards compatibilty during the transition.

We can drop support for Indy SDK at any time. We’ve placed deprecation notices in the repo and in the log when you start ACA-Py using Indy SDK. If there is a reason to drop it, we can do that.

The —wallet-type=askar really means “askar+credx+indyVdr”. In theory we can break it into more startup parameters, or even (when we drop Indy SDK) keep it and ignore it (as we recently did in #2517 with some other startup flags).

[ianco]

ok thanks let me noodle this info ... I'm understanding @andrewwhitehead 's recommended approach a bit better ...

[ianco]

OK upon consideration I still think we should port over the anoncreds code as new new and not try to integrate with any existing code. The current "indy" stack supports both indy-sdk and askar (and related) libraries and I think this is introducing a lot of complecity.

I suggest we:

• copy the code from the new anoncreds package, get unit tests working etc
• copy the indyissuer/holder etc to new anoncredsissuer/holder etc (copy of classes with a new name)
• update the new anoncreds* classes to support the new anoncreds library only (remove all indy-sdk and credx references, and simplify the code to just support anoncreds, don't try to build for multiple libraries)
• where there are dependencies in other classes (e.g. ledgers) add new methods and depricate methods that will be removed

If there's a concern about keeping aca-py flexible for the future, I suggest the way to do this is via plug-ins, for example the whole anoncreds stack - if implemented as a plug-in - would be replacable.

[andrewwhitehead]

Some of my assumptions at the moment are that:

1. We want a main branch that can be immediately wrapped into a release, and isn't missing major features (like revocation).
2. The anoncreds code needs more tests, including integration tests using status lists, before we are comfortable switching to it. The library could require code updates as well.
3. By not simply implementing the AnonCreds support using dependency injection, even though the classes are essentially compatible with the current interface, we have to update the handlers to use one or the other specifically. Using DI, the choice to use one or the other would be in one place only (when setting up the context). A plugin would also conceivably solve this issue.
4. It is possible that in future we might need to switch the implementation again, and DI would make that easy. For example, we don't know exactly what it will look like code-wise to support the W3C-formatted credentials yet, but that could be implemented in a similar manner.
5. There may be an upgrade step required for existing wallets switching from Credx to Anoncreds-rs, but we haven't documented what that might look like yet.
6. Keeping this code on a separate branch is only allowing it to diverge from the main branch over time, and making the eventual merge back to main potentially more disruptive.

I agree that supporting anoncreds credentials (or any other format) through plugins would be a good path, although it also requires integration points into the exchange protocols which aren't defined yet.

[usingtechnology]

To @andrewwhitehead items 3 and 4 above, this kind of relates back to #2556. Maybe this is a good opportunity to tackle some of these classes (BaseLedger, Revocation). Right now, that's where things in anoncreds-rs branch break the existing code. We need to stop the hard dependency on an actual implementation class. Within the anoncreds-rs branch @dbluhm has some nice interface/implementation with registry and resolver and the defaults (did_indy, did_web, legacy_indy).

[andrewwhitehead]

@usingtechnology Yes once the generic AnoncredsRegistry handling is merged we should be able to switch over the resolution and publishing to use that. There's an example from that branch in issue-credential/v2_0 (can't link to the diff unfortunately):