Issue Credential v2 issue tracker

[TimoGlastra]

This issue tracks all outstanding tasks for ICv2:

• Unresolved comments from the ICv2 PR: feat: issue credential v2 #649 (review)
• Offer credential interface takes an optional credentialRecordId. It shouldn't take this as when you offer a credential there is no credential record id yet.
• Accept proposal takes the following properties that are not required
  • protocolVersion
  • connectionId
  • credentialFormats (accept should just take over the values from the proposal for the request -- same as before this PR)
• ProtocolVersion shouldn't be optional in proposeCredential method
• The credential message builder still contains checks for attachments from the format service being undefined (e.g. offersAttach === undefined). We can remove them as the types are not optional anymore. This will reduce the complexity of the code.
• The ICv2 tests use waitForProofRecord incorrectly in some places, meaning it can lead to race conditions. As the event listener only starts when you call waitForCredentialRecord, there's a good chance the event will already be emitted. So instead it should be used like this (or use the subject approach as is done in some of the other tests):

// start listener, but do not await promise yet!
    const faberProofRecordPromise = waitForProofRecord(faberAgent, {
      threadId: aliceProofRecord.threadId,
      state: ProofState.PresentationReceived,
    })

// do the action that will trigger the event
aliceProofRecord = await aliceAgent.proofs.acceptRequest(acceptPresentationOptions)

// wait for the event
proofRecord = await faberProofRecordPromise

• accepting a proposal should take the credential definition id from the proposal to send a credential offer (if availalbe on the proposal). This functionality was there for v1 (and still is: https://github.com/hyperledger/aries-framework-javascript/blob/main/packages/core/src/modules/credentials/protocol/v1/V1CredentialService.ts#L204-L205), but itsn't present in v2. We should probably extract this from the proposal in the indy credential format service so we can reuse it across the v1 and v2 implementation. Currently throws an error that it is missing the credential definition id. It also means the user has to retrieve the message and extract the credential definition from the proposal themselves.
• Same for the attributes, it shouldn't be required to pass the attributes when accepting a proposal as we can take them from the proposal (but should be optional for the case where there was no proposal preview and thus no attributes)
• The proposal data attachment for indy is put in the message as follows:

{
    schemaIssuerDid: '3msFQcrmEPJz5XetTn8bJB',
    issuerDid: '3msFQcrmEPJz5XetTn8bJB',
    schemaName: 'Schema_DriversLicense_v2',
    credentialDefinitionId: '3msFQcrmEPJz5XetTn8bJB:3:CL:130369:8129',
    schemaVersion: '1.0.1',
    schemaId: '3msFQcrmEPJz5XetTn8bJB:2:Schema_DriversLicense_v2:1.0.1'
  }

while the actual fields should be snake_case. I think this is related to the comment I made here: #649 (comment). We should make sure to transform the class instance to json before putting it inside the attachment. This is quite important and completely breaks interop with ACA-Py

• revocationRegistryId and revocationId are not stored in the credential exchange record anymore. I think some parts of the recent revocation notification have been undone now. There is a separate issue here to integrate icv2 with revocation notification and I think we can cover it in that PR (Integrate revocation notification with issue credential v2 #737)
• check whether the indy key is present in v1 credential service. We should probably also check that other formats are not supported. Maybe something like this (taken from review comment from present proof pr):

if (!proofFormats.indy || Object.keys(proofFormats).length !== 1) {
  throw new AriesFrameworkError("Only indy proof format is supported for present proof protocol v1")
}

• connections should be retrieved on the module layer and passed to the service layer, not retrieved on the service layer.
  • connections are now retrieved multiple times
  • this makes the services easier
  • it's the same for v1 and v2

[TimoGlastra]

@NB-MikeRichardson using this to track everything related to issue credential v2. Please make a separate PR per item so the code review will be easier to do.

[TimoGlastra]

Branch I'm working on to integrate ICv2 into AATH: https://github.com/TimoGlastra/aries-agent-test-harness/tree/feat/afj-icv2

I've got it working between AFJ <-> AFJ, but as listed in the items above there are some issues with interop.

I've also created a PR that integrates the breaking changes introduced by the v2 merge and it seems we're still fully interoperable with ACA-Py for ICv1. Nice!