feat: Cred Issuance V2 APIs

[gmulhearn-anonyome]

Overview

v1: https://github.com/hyperledger/aries-rfcs/blob/main/features/0036-issue-credential/README.md

v2: https://github.com/hyperledger/aries-rfcs/blob/main/features/0453-issue-credential-v2/README.md

v2 of issue-credential makes the protocol flow work for 'any' credential format (anoncreds, json-ld w3c, etc), whereas v1 has a lot of fields which are specific to anoncreds.

they do this by hiding away credential-format-specific data into attachments, and then we have a formats field in the messages to specify the format being used.

the v2 specific has tables of 'format registries' which contain links to the expected attachment payloads for different formats (anoncreds, w3c etc).

To cater for this generic format concept in our state machines, I've tried to use generics HolderCredentialIssuanceFormat & IssuerCredentialIssuanceFormat, which are traits that are responsible for creating the attachment payloads (and returning metadata) for a particular format. I've created an Anoncreds impl of these traits for proof of concept purposes. These anoncreds impls reuse a lot of existing infrastructure from our v1 anoncreds state machines.

The idea is that in the future aries-vcx could include more formats, or consumers can go ahead and create their own format impls outside of aries-vcx. This way, aries-vcx does not have a hard requirement to keep up with the (potentially) growing list of formats in the registry, as consumers can create their own in the meantime.

This potential for growth and supporting generic formats is why I've gone with generics instead of defined enum variants for different formats; i think enum variants would lock consumers into only using a set of formats that we support.

Note that HolderV2 and IssuerV2 assume that you are using a single format family (anoncreds, json-ld, etc) thru out the whole protocol, and they also assume that you are using single attachments per message. The spec implies that you can have multiple attachments, potentially with different formats, within single messages, however i am yet to see a use-case for this or much discussion about it. For comparison, aca-py seems to assume single attachments and of the same format thru out the protocol. All in all, this confusion around what is 'potentially possible' with this spec lead me just to implement HolderV2 and IssuerV2 under those assumptions. Perhaps if it does become a use case, we'd need to make a breaking change to these typestate machines, however for now i'd like to keep it simple; i can't even begin to imagine how you'd generically deal with all the possible permutations when using multi attachments and multi formats...

While the above is true, i believe we agreed that single format is acceptable for now until we have a real use case. discussion here

Open Qs

• is the way I'm handling recoverable errors acceptable and what we were thinking?
• are we happy with me bringing in mockall for unit test? IMO we should be leveraging it thru out our tests, especially tests involving Profile trait components

TODO:

• Manually test against ACApy issue v2
• Finish unit tests with mocks
• More system tests for more anoncreds variety
• Remove the mock jsonld format handler
• Remove acapy testing related assets
• more commenting
• couple of small in-line TODOs
• make lint more happy :)

Future work

some items which i'd like to leave for the future;

• Serde of the state machines
• 'Generic' Holder / Issuers
• 'processing' simplified layer

[codecov-commenter]

Codecov Report

Merging #987 (dbce3a7) into main (b2e87eb) will decrease coverage by 5.38%.
The diff coverage is 45.42%.

❗ Current head dbce3a7 differs from pull request most recent head 83ce114. Consider uploading reports for the commit 83ce114 to get more accurate results

@@            Coverage Diff             @@
##             main     #987      +/-   ##
==========================================
- Coverage   35.97%   30.59%   -5.38%     
==========================================
  Files         372      436      +64     
  Lines       21745    27787    +6042     
  Branches     4014     5376    +1362     
==========================================
+ Hits         7822     8502     +680     
- Misses      11763    16989    +5226     
- Partials     2160     2296     +136     

Flag	Coverage Δ
unittests-aries-vcx	30.59% <45.42%> (-5.38%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...protocols/issuance/holder/states/offer_received.rs	17.85% <100.00%> (ø)
...c/protocols/issuance/holder/states/proposal_set.rs	62.50% <100.00%> (ø)
...rc/protocols/issuance/holder/states/request_set.rs	77.27% <100.00%> (ø)
...protocols/issuance/issuer/states/credential_set.rs	30.00% <100.00%> (ø)
.../src/protocols/issuance/issuer/states/offer_set.rs	93.10% <100.00%> (ø)
...tocols/issuance/issuer/states/proposal_received.rs	100.00% <100.00%> (ø)
...ocols/issuance/issuer/states/requested_received.rs	37.50% <100.00%> (ø)
aries_vcx/src/utils/openssl.rs	37.93% <ø> (ø)
aries_vcx/tests/utils/scenarios/data.rs	96.49% <100.00%> (ø)
...lds/protocols/cred_issuance/v1/issue_credential.rs	25.00% <ø> (ø)
... and 44 more

... and 185 files with indirect coverage changes

[Patrik-Stas]

The idea is that in the future aries-vcx could include more formats, or consumers can go ahead and create their own format impls outside of aries-vcx. This way, aries-vcx does not have a hard requirement to keep up with the (potentially) growing list of formats in the registry, as consumers can create their own in the meantime.

I like this extensibility property a lot

[gmulhearn]

HolderV2 with HyperledgerIndyHolderCredentialIssuanceFormat works with aca-py for receiving anoncreds via issue-credential-v2; just manually tested it

[Patrik-Stas]

Left review, but nevertheless this is awesome job. 👍

Also note my review is partial, I don't feel like I yet went through it thoroughly enough. Will continue tomorrow.

[Patrik-Stas]

in did-exchange, we adopted approach where we don't store entire aries messages, but rather return tuple of (new_state, response_message).
While the new_state could potentially be capable of regenerating response_message - though I don't think we do that in did_exchange implementation

[gmulhearn-anonyome]

kk, latest commit converts to this approach, let me know what you think. Would be helpful to me if we could go over the reason why again though..

I originally thought it was bcus we were switching to the mentality of "our state machines should only store the information required to keep transitioning", but that is not necessarily true, because i'm still storing other convenience items in the state machines: e.g. CredentialReceived state is also storing stored_credential_metadata, which is just a convenience for the state machine consumer so that they can do a holder.get_stored_credential_metadata() and see information about the credential they just stored.

If we were to go all the way with the "our state machines should only store the information required to keep transitioning" mentality, then stored_credential_metadata would be removed from CredentialReceived state. and instead it would be returned from the HolderV2::receive_credential API in a tuple like we do with messages.

Same story with the issuer's CredentialPrepared state, which stores credential_metadata, allowing them to do a issuer.get_credential_creation_metadata() and see information such as the cred-rev-id of the created credential. To remove credential_metadata from the state, then the IssuerV2::prepare_credential API would need to return this metadata AND the credential message:

pub async fn prepare_credential(self,...) -> <(IssuerV2<CredPrepared>, IssueCredentialV2Message, T::CreatedCredentialMetadata)>

So i guess it'd be good if we made a decision/statement about how far we want to lean into the "our state machines should only store the information required to keep transitioning" mentality

[Patrik-Stas]

The complete state contains radically different data than CredentialReceived, but in practice they are kinda-sorta the same state, obviously this is just marking that we have sent the final ack. Which makes CredentialReceived against kind of ephemeral. Which makes me (again) wonder if the 2 states shall not be merged, as typically you would:

• receive issue-credential message (being in OfferPrepared stated)
• ephemerally transition to CredentialReceived state, as most likely right after, you would send Ack and transition to Complete

But if the 2 states remain separate, I would expect them to hold the same data.

[gmulhearn-anonyome]

Part of the reason i have them separate, is because i started off implementing issue-credential-v2 as the V2.2 impl (i.e. what is seen on main branch of the RFC: https://github.com/hyperledger/aries-rfcs/blob/main/features/0453-issue-credential-v2/README.md). This is bcus in V2.1+ of the issue-credential-v2 protocol, the CredentialReceived state is not necessarily the end, if there are MULTIPLE credential being offered, then the holder can reply to an issue-credential message with ANOTHER request-credential message (i.e. transitioning from CredentialReceived -> RequestPrepared in our state machine).

So i kept them as seperate to support that transition logic. however since then i reverted to just the plain old 2.0 implementation (no support for multi credential issuance). Now the Completed state is essentially just a "Ack optionally prepared" state. I like having them seperated personally.. but we can discuss further if you prefer them merged.

But if the 2 states remain separate, I would expect them to hold the same data.

i.e. the Completed state should have a

    stored_credential_metadata: T::StoredCredentialMetadata,

field?

[bobozaur]

I'm leaving this as a general comment as it impacts a big part of the code.

The formats thing feels weird to me for a number of reasons. I've went through RFCs and specifications these past couple of days and thought about this quite a bit thinking of maybe proposing some alternative implementation, but I think that might actually bias things now so instead I'd like to propose an open discussion about the architecture of this.

My two cents are:

• Probably the most important thing is that some of these formats are not even mature/used anywhere yet (like the identity foundation one). I don't even know why they included them.
• In the same note, I tend to believe more and more that these formats are tightly connected to the way a credential gets processed. For the hlindy formats, it's through credx. For the anoncreds one, it would be through anoncreds-rs. For the JSON-LD stuff, it seems W3C talks about some controllers and stuff, which poses a different processing mechanism. The dif one has no such thing yet (lol).
• There are multiple formats that a credential can be provided in, which is why these attachments are represented as arrays.

So what this boils down to in my mind is that a particular agent will support one or more formats based on what implementations of credential processing it supports. While there might be some wiggle room for a compatibility layer between credential formats, especially between hlindy and anoncreds, I don't think we should rely on that (as it seems like they do not either).

So, in that vein, an agent that only works with credx will then look for the hlindy credential format and process that in a well defined data structure from credx. One that implements anoncreds-rs will do the same, but for the anoncreds format. One that supports both could technically choose the first one that matches a supported format or have a preferred format that it searches for first.

I believe the current implementation couples these formats with the protocol way more than it should. The formats are strictly used for the attachments, which get ultimately processed through a Wallet and Anoncreds implementation. The protocol specification does not directly care about the format.

I think that all the formats should be used for are taking an Attachment, converting it to the data structure used by the Anoncreds implementation, use a Wallet maybe to do some stuff with the cred, and move on. Or, conversely, take some expected data structure and ultimately provide an Attachment, which is what the protocol and the messages work with.

Right now the format traits are encapsulating the creation, processing, storing of credential (and they would probably contain the retrieval too when the PresentProofV2 protocol is introduced). That's too much, if you ask me. Instead, I think the format should strictly work with the Attachment type (either producing or processing one).

In fact, given that the Anoncreds implementation actually "dictates" the format you're looking for, the whole formatting trait might not even be needed. Anyway, I thought about some specifics in terms of implementation too but I'm wondering what everyone is thinking in terms of this and we can discuss approaches afterwards.

@Patrik-Stas @mirgee @gmulhearn ?

[gmulhearn-anonyome]

@bobozaur thanks for spending time reviewing and thinking, always appreciated.

---

• In the same note, I tend to believe more and more that these formats are tightly connected to the way a credential gets processed. For the hlindy formats, it's through credx. For the anoncreds one, it would be through anoncreds-rs. For the JSON-LD stuff, it seems W3C talks about some controllers and stuff, which poses a different processing mechanism. The dif one has no such thing yet (lol).

I guess that is true for hlindy & anoncreds being tethered to indy-sdk/credx & anoncreds-rs. But also not 100% true, i believe credx consumers would also be able to handle anoncreds format if they wanted; they just might need to handle the data with a bit more care (e.g. master_secret -> link_secret).. e.g. internally i use anoncreds-rs as a Holder in my stack, but i can handle the hlindy format just fine (which is something i want since most big agents haven't moved onto anoncreds format yet) (i see you mention this later on, but i think it's worth noting).

But if you're saying that these formats are tied to the library/sdk they are using for processing, i don't think this statement is true for ld-proof VCs. There are lotsss of ld-proof VC libraries out there, recently i was experimenting with a new typescript one called onyx, however there are plenty of others.
JSON-LD has been around for awhile, ld-proof VCs is just an application of that. So there are lots of different ways an agent could implement support for that format.

So what this boils down to in my mind is that a particular agent will support one or more formats based on what implementations of credential processing it supports. While there might be some wiggle room for a compatibility layer between credential formats, especially between hlindy and anoncreds, I don't think we should rely on that (as it seems like they do not either).
So, in that vein, an agent that only works with credx will then look for the hlindy credential format and process that in a well defined data structure from credx. One that implements anoncreds-rs will do the same, but for the anoncreds format. One that supports both could technically choose the first one that matches a supported format or have a preferred format that it searches for first.

I agree for hlindy and anoncreds, however i think it's oversimplifying to suggest that the format is somewhat abstract and doesn't really mean anything other than just being an artifact of the underlying implementation you're using (e.g. indy-sdk/credx, anoncreds-rs, or some ld-proof SDK).

There are business reasons why an issuer/holder would choose ld-proofs or anoncreds on a case-by-case basis. There is a bit of divide in decentralized identity between these formats, which comes from the differing pros and cons. W3C VCs are standardized (it's w3c), but arguably aren't fully there with privacy features. Anoncreds are great with privacy, but are not as standardized or recognised as w3c VCs.

So i think the choice often comes down to business reasons. For instance, some companies may be forced via regulation to choose w3c or anoncreds. it's not neccessarily just "use the first format handler that matches". So with that, alot of the time "have a preferred format that it searches for first" might be the way it goes.

which get ultimately processed through a Wallet and Anoncreds implementation.

in the case of anoncreds/hlindy format, yea. Not other formats (just stating this for this thread record)

and they would probably contain the retrieval too when the PresentProofV2 protocol is introduced

yea correct, this is the path i was heading

In fact, given that the Anoncreds implementation actually "dictates" the format you're looking for, the whole formatting trait might not even be needed.

Just want to be careful here again, Anoncreds implementations will likely be capable of handling anoncreds & hlindy; they are almost identical. Just want to make sure we don't lock in the aries_vcx Anoncreds trait to "only support support the hlindy format"

---

Anyway, i think all of those are just small comments/corrections on your comment, i think overall i agree with your sentiment, but still unsure if their is a reasonable solution.

If i'm understanding correctly, you'd essentially prefer that the attachment processing/creating be done outside of the HolderV2/IssuerV2 state machine? In general, i was trying to have it so that the HolderV2 and IssuerV2 flow (when using hlindy/anoncreds format) feels almost the same as the original Holder and Issuer flows in aries_vcx. I.e. the existing state machines do all the format processing and creating inside them, abstracted from the user, same with Prover/Verifier. But you're saying that this might not be the right approach here?

I would like to see what implementation you had in mind, i guess i'm imagining a similar impl, but the format handler/s are separated from the state machine. and the consumer code would bounce between the state machine and their selected format handler. And maybe the state machine would have helpers to advice the consumers on what formats can be used. If this is the direction you are thinking, do you think that may be asking the consumer to do too much? particularly because they will have to be storing/maintaining format data/metadata outside of the state machine (such as the credential request metadata a holder gets after creating the anoncreds request). My current state machine impl (+ the hlindy format impls) does all of that for the consumer and holds onto all their protocol data in one place, which i thought was convenient and matched the simplicity of other state machines. Additional, i believe what you're proposing would make the state machines extremely trivial, as they'd pretty much just be responsible for putting attachments into messages, matching thids and marking a state, not that that is a bad thing.

Let's discuss further in today's vcx meeting if you're keen

[Patrik-Stas]

Leaving what I have so far, but there's probably space for more review. But let's keep the conversation ping-ponging and see where we get

I would also like to propose following to make the reviewing for me and other easier. How about we actually take out all state machines from here, and have that in separate PR. We can focus on the underlying infra here, then retrofit state machines layer. Given my understanding that you don't use state machines, this might be better for you too?

[Patrik-Stas]

Perhaps HLIndyIssuerFormat::<MockAnoncreds>::get_offer_attachment_format(); could be used internaly by create_offer_attachment_content and return that in the tuple (of 3 elements)?

So we would have

        let (attachment_data, offer_metadata, attachment_format) =
            HLIndyIssuerFormat::create_offer_attachment_content(&HyperledgerIndyCreateOfferInput {
                anoncreds,
                cred_def_id: cred_def.get_cred_def_id(),
            })
            .await
            .unwrap();
            

However then 3 elements are quite growing over the head, perhaps instead of returning tuple of 3, we could return some struct.

[Patrik-Stas]

I see this MaybeKnown thing leaking out to many places, because it's part of underlying AttachmentFormatSpecifier, eg

pub struct AttachmentFormatSpecifier<F> {
    pub attach_id: String,
    pub format: MaybeKnown<F>,
}

I don't know what&why MaybeKnown serves, but the bottom line I can say here is that I find it weird that this function create_offer_message_from_attachments takes something wrapped in MaybeKnown.

I think what we could possibly do is modify create_attachments_and_formats by removing its T generics, and in all upstream signatures work with OfferCredentialAttachmentFormatType rather than MaybeKnown<OfferCredentialAttachmentFormatType>. Instead create_attachments_and_formats can wrap it into MaybeKnown at the last moment.

[Patrik-Stas]

I am reviewing this now mainly by observing the DX (Developer Experience) working with this. On lines 100-134 there's 2 things happening:

1. Constructing preview - quite simple
2. Constructing attachments_format_and_data - this takes 3 sub-steps
   i. prepare correct attachment format string
   ii. build a matching attachment content, which should be in matching the string from step 2.
   iii. put stuff in vector attachments_format_and_data
3. Building the offer message create_offer_message_from_attachments

• My previous comment was already talking into removing the step 2.i

• I am thinking we could trim off 2.iii too, by making some assumptions about our aries-vcx use-cases: if we only allow issuers to submit 1 credential at the time, then bundling things up into vector can happen inside create_offer_message_from_attachments and tweak it to accept attachment_format, attachment_data instead of current, more general, vec of tuples of these pairs.

  I am not convinced the multi-credential support will maintain support, and if yes, we can worry about it once we needed. For now trying to support all of the protocol features seems like YAGNI.

[Patrik-Stas]

This is basically more abstract builder. Nothing wrong with that, just makes me wonder if we'd eventually create new, more abstract builder layer on top of of messages itself. I guess time will show, I don't see problem with the placement of this currently.

[Patrik-Stas]

Out of PR scope, just a general remark, more toward messages crate. I've seen thins kind of pattern before; especially accessing thid or pthid is quite common operation, but it's quite cumbersome.

I think messages crate should probably include this sort of functionality.

Feel free to click this off as resolved, but also happy to hear your thoughts on the matter. I'll create github issue to track this

[Patrik-Stas]

I believe I touched on this lightly when we voice called. I think in this attachment layer, it would be for best to decouple from concept of aries messages. And I think it's possible at least in this case (don't yet know about other methods around here). The only reason you are taking arg of aries message type OfferCredentialV2 is cause you need to scan it for the right type. The entire function can be taken out of here, if you just make OfferCredentialAttachmentFormatType as input argument.

Not precisely sure of upstream effects, ?perhaps? something similar could be done with with extract_offer_details which is calling this, but then I am not sure right now, how would that impact DX in test

let offer_details =
            HLIndyHolderFormat::<MockLedger, MockAnoncreds>::extract_offer_details(&offer_msg)
                .unwrap();
        assert_eq!(offer_details.schema_id, cred_def.get_schema_id());
        assert_eq!(offer_details.cred_def_id, cred_def.get_cred_def_id());

[Patrik-Stas]

analogically applicable my previous comment

Perhaps HLIndyIssuerFormat::::get_offer_attachment_format(); could be used internaly by create_offer_attachment_content and return that in the tuple (of 3 elements)?