Rework on ACA-Py to achieve ledger agnosticism

[pasquale95]

This pull request contains all the rework that we, as IBM Research, have done on aries-cloudagent-python (also known as ACA-Py) with a view to achieve ledger agnosticism. The biggest problem we found out in ACA-Py is that, once the agent is started, the ledger sdk to use is strictly bounded with the --wallet-type parameter. More specifically, by choosing a specific wallet (e.g. --wallet-type indy) we are bounded to use IndySdkLedger as class implementation of the abstract class BaseLedger. With this approach we need to create a new wallet whenever we want to support a new ledger:

We believe that such behaviour goes against the goal of being ledger agnostic and here we propose our solution.

In our implementation the user can select what ledger to use by adding a new command-line parameter named --wallet-ledger. In this way we can add new ledger implementations without being forced to add a new wallet:

More in details:

• each ledger class implementation (called also as ledger adapter class) of the abstract class BaseLedger has a BACKEND_NAME parameter which is used as the ledger class identifier. Such identifier can be indicated with the --wallet-ledger command-line parameter to choose what ledger adapter to use when running the agent;
• we implemented a class named LedgerProvider in charge of choosing what ledger class to use to interact with the ledger based on both the parameters --wallet-type and --wallet-ledger. The choice of the ledger is still bounded to the wallet the user chooses since each wallet can be free to specify what ledgers to interact with. The list of ledgers supported by a specific wallet is defined directly inside the LedgerProvider class as WALLET_SUPPORTED_LEDGERS;
• we made a rework on the interactions between the agent and the tails server to allow revocation in a ledger-agnostic way. Specifically we added the field ledger_type into the request payload which goes from the agent to the tails server and which allows the latter to correctly setup his connection with the ledger. To have a ledger-agnostic implementation of the tails-server capable of parsing correctly this new request layout we recommend to use the implementation available here.

More details can be found at the LedgerAgnosticism document.

[swcurran]

@pasquale95 -- not sure if you want to deal with these now or after review and discussion, but there are issues raised in the Circle CI tests and the Black Code Formatter.

You can ignore the SonarCloud analysis as it failed to run, so produced no results based on your code. We're still trying to figure out how to get that to work properly.

[burdettadam]

I like the ledger provider in this PR and the features it provides. I think we should consider removing the centralized ssi ledger as a default ledger. I would rather see the centralized ssi ledger as a plugin. I did have the thought that we could possibly simplify and speed up tests that depend on containerized ledgers if we used a ledger without consensus like the centralized ssi ledger. So I do like the centralized ssi ledger and recognize its utility, but I think we should merge the provider interface before considering the centralized ssi ledger as a default. Other than that, I really like this PR, good work!

[pasquale95]

I agree @burdettadam, consider that we left the CentralizedSdkLedger code inside the PR as a toy example of what we could achieve with such architecture and with the only purpose to let the community practically test the functionalities we provided, but if the PR would be accepted we plan to remove the centralized ledger before the merge.

[codecov-commenter]

Codecov Report

Merging #1931 (3b6f8d2) into main (98f5537) will decrease coverage by 0.58%. The diff coverage is 30.52%.

@@            Coverage Diff             @@
##             main    #1931      +/-   ##
==========================================
- Coverage   93.59%   93.01%   -0.59%     
==========================================
  Files         539      541       +2     
  Lines       34279    34597     +318     
==========================================
+ Hits        32084    32179      +95     
- Misses       2195     2418     +223     

[pasquale95]

@swcurran @andrewwhitehead I improved the code and the PR now seems passing all checks. As discussed with @burdettadam , if the PR will be merged I'll remove the CentralizedSdkLedger code before since it's not production code, but only a toy example.

[swcurran]

I've activated the additional checks to be run -- sorry I was slow about that. I see that Black is failing, but that should be an easy fix.

I definitely want to have others weigh in on this before we merge. Timo and team have been working in Aries Framework JavaScript on a ledger-agnostic implementation and so they have some ideas. They have also proposed some changes to CredX (AnonCreds implementation) to make it a bit easier at the Aries level, allthough most of the hard work is here.

Those that have experience with AnonCreds on other than Indy, please take a look and add your expertise!

[TimoGlastra]

The overall architecture looks good to me! Pasquale and I have had a chat on both of our approaches. Our approach would take it a step further and introduce more api changes, breaking changes and would be more high-effort, but would allow for multiple ledgers to be registered at the same time, and also follow the AnonCreds specification with unique identifiers.

That's one downside of this approach still, e.g if you use the centralized ledger implementation, you need to provide the client ip where the AnonCreds object are hosted to the agent on startup. The identifiers still use the indy legacy identifiers in transit, meaning the other agent must be configured exactly like this agent for it to work (same like we have with the multi-ledger approach now). The approach the AnonCreds specification is that each id now identifies exactly where you can find the specific object, allowing agents to support multiple VDR simultaneiously and switch based on the format of the identifier (like we have with different did methods). The benefit of this appraoch is that it works with the current indy-sdk and indy-credx implementations. This is at least how I understand the current code, is that correct @pasquale95?

So to sum up (based on my current understanding, please correct me if wrong @pasquale95), this does allow to use AnonCreds with different ledgers, but I do think there's a few things to consider before we merge (and I'll let the maintainers decide if that's what we want):

• The identifiers still use the legacy indy AnonCreds identifiers, meaning it'll be even more ambiguous where the anoncreds objects are hosten by just looking at an identifier
• Because it uses legacy identifiers, it means this doesn't follow changes being made to the AnonCreds specification and wouldn't support AnonCreds with did:indy and e.g. did:cheqd
• It is not possible to register multiple anoncreds resources at the same time, you have to pick on startup. This is probably more fine for issuer agents, but I think as verifier/holder agent it is a desired features to be able to work with multiple anoncreds object methods at the same time.
• The ledger api is kept as is. This is great for backwards compatability, but the current ledger interface is heavily inspired by the features of the indy ledger. As we can see with the resolver and registrar features that have been added to ACA-Py, they have their own api and you implement them for each did method. So instead of one ledger class that has a registerNym (indy specific) and a register schema, etc... We should implement an IndyDidRegistar to register dids, IndyDidResovler to resolve dids and an IndyAnonCredsResource to read/write anoncresd objects.

[swcurran]

Thanks for the work @pasquale95 -- awesome!! Thanks for your thoughts, @TimoGlastra.

I agree with Timo that we have to do this work in the spirit of where we are going, not based on what we have. The format of the identifiers is (I think) a big deal. Like DIDs, we have to relax the format of the identifiers so that the identifier content is defined by where the object is published, and we can find/resolve where the object is published from the identifier. In other words, we have to make it so we have a solid "registrar/resolver" architecture that is consistent across all Aries (and beyond) implementations. That is where the AnonCreds specification is going.

I also like what I am hearing here and in the discussion about DIDs where we don't rely on startup parameters exclusively, but we have a registry that can be dynamically maintained as to what DIDs we resolve and where objects (DIDs, AnonCreds objects) are written. I think that needs to be thought through and applied to this change.

Huge progress!

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[dbluhm]

Having recently contributed significantly to the Ledger Agnostic AnonCreds effort in ACA-Py, I wanted to give a brief update on my thoughts on this PR and the changes contained in it.

There has been excellent discussion above from @TimoGlastra and others on the pros and cons of this approach; this work has been a catalyst for discussion for how Ledger Agnostic AnonCreds should look. In the end, it was determined that in order to truly de-indy-ify AnonCreds, a more generic interface would be required. This has been the foundation of the AnonCreds specification, the anoncreds-rs library, and the work on ACA-Py now contained in https://github.com/hyperledger/aries-cloudagent-python/tree/anoncreds-rs.

As a result of going in a different direction, involving more dramatic changes to ACA-Py and AnonCreds itself, this PR is no longer applicable and future efforts should go into the generic interface implemented in the anoncreds-rs branch.

That being said, I think there are some really good ideas in this PR, particularly around the handling of Indy ledgers -- which is still something we'll have to worry about with the generic anoncreds interface for the Legacy Indy and did:indy backends. Thank you for your contributions and patience, @pasquale95!