feat: add anoncreds package

[karimStekelenburg]

This PR adds a package that contains a set of interfaces that define unified APIs for AnonCreds related logic.

The goal of adding this package is that we can have two separate implementations, one for indy-sdk and another one for the anoncreds-rs Rust library that is currently being developed.

Note: the interfaces in this package contain a lot of todo comments and any statements. The idea is to update these during the implementation of the first package.

Signed-off-by: Karim Stekelenburg [email protected]

[swcurran]

Just as a matter of interest, why the "-" in "anon-creds"?

[TimoGlastra]

Just as a matter of interest, why the "-" in "anon-creds"?

Agree, I think we should just call it anoncreds

[TimoGlastra]

Overall looks good, I'll make some amendments on the interfaces with the missing types. Can't comment on the file, but internal.ts is an empty file. I think we can remove it for now?

For others reviewing this PR. Some notes/todos that we would like input on:

• Do we want to follow data model terms (credDefId) or AFJ terms (credentialDefinitionId) in the interfaces. E.g. for the createSchema method it should always return the data model. However when we call createOffer we need to pass a cred def id. This is not part of the data model, so in this case should we use credentialDefinitionId?

[berendsliedrecht]

Left a couple of comments, mainly some cleanup and consistency.

[berendsliedrecht]

Just as a matter of interest, why the "-" in "anon-creds"?

I think the reason was as it is AnonCreds, which is in pascal case. If you transform pascal case to kebab case, which is what we use for packages and folders, you get anon-creds.

I prefer simply anoncreds, but I think that this is the reason for that choice.

[karimStekelenburg]

The reasoning behind the "-" is simple. AnonCreds is an acronym for Anonymous Credentials, so separating those words made sense to me.

I'm happy to change it though, consistency is more important.

@swcurran any ideas on what the rationale is behind not separating those words is? I've been wondering the same thing about the "cloudagent" in Aries Cloudagent Python for years 😅.

[swcurran]

Naming is fun!

AnonCreds is the term that has been used in the community for year, and the project at Hyperledger is "AnonCreds", so I think if you are using it in code, it should be "anoncreds". And yes, AnonCreds was originally short for "Anonymous Credentials", but the name has a life of its own, beyond just an acronym. I've never seen it hyphenated before, hence my comment.

Cloudagent was just what was settled on, before all the rest of the "Aries things" were called Frameworks. Had we named it a few months later, it probably would have been called "Aries Framework Python". The cloudagent was to indicate that it was not a suitable candidate for mobile deployment. In other words, nothing special -- just a name that someone came up with and the rest of us thought was less terrible than the other options we had come up with.

[karimStekelenburg]

@TimoGlastra, agree with your points. Maybe I should have made it a draft as it's not finalised yet. I just copied what you defined and planned on updating it during the indy-sdk implementation.

I assumed you had a purpose in mind for internals.ts, that is why I kept it.

I thought the idea was to merge this package first, and then merge the indy-sdk implementation once finished. However, now I'm thinking about it that, I don't see a reason to really.

If you agree, I will make this PR a draft for now and start adding the indy-sdk implementation to it.

I can finalise the of the interfaces while implementing them for indy-sdk and merge them in all together. That way we keep main clean of unused, semi-implemented packages.

I guess the PPV2 situation scared me into shrinking down PRs a bit too much 😅

[TimoGlastra]

However, now I'm thinking about it that, I don't see a reason to really.

I think smaller PRs is a good reason! If we remove the API part and clean it up I'm for merging this first. It's a private package for now. That'll also allow e.g. cheqd work to already start

[TimoGlastra]

Overall looks good. I think all metadata values can be removed except for the seqNo value.

However I think that should be typed in the indy-sdk implementation, not in the generic AnonCreds interfaces? Also could we rename it to indyTransactionSeqNo or something to make it very focused on indy credentials?

Not sure if the metadata approach is worth it for just this one value

[codecov-commenter]

Codecov Report

Merging #1118 (cae255f) into main (5f98fed) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1118   +/-   ##
=======================================
  Coverage   88.82%   88.82%           
=======================================
  Files         707      707           
  Lines       16593    16593           
  Branches     2804     2804           
=======================================
  Hits        14739    14739           
  Misses       1744     1744           
  Partials      110      110           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[TimoGlastra]

Awesome! Let's get this merged and we can iterate over it as the spec changes

Comments resolved