Issue with revoking AnonCreds credentials

[bryce-mcmath]

After upgrading from AFJ 0.4.2 to Credo 0.5.2 revocation seems to have stopped working with the credentials we use (anoncreds). The failure happens in @credo-ts/packages/core/src/modules/credentials/protocol/revocation-notification/services/RevocationNotificationService.ts in processRevocationNotification method when this.credentialRepository.getSingleByQuery is called. That method is throwing an error because findSingleByQuery is returning null in @credo-ts/packages/core/src/storage/Repository.ts ie. it didn't find any records.

Here is one of the sample queries that is getting this error, it looks normal to me but let me know if you notice anything wrong.

{
  "anonCredsCredentialRevocationId": "3",
  "anonCredsRevocationRegistryId": "J6LCm5Edi9Mi3ASZCqNC1A:4:J6LCm5Edi9Mi3ASZCqNC1A:3:CL:109799:dev-attestation:CL_ACCUM:a76612f2-d80e-4bfb-8739-0c468c728d6a",
  "connectionId": "aa602260-631f-40f3-8442-2194da060c1a"
}

Might be worth mentioning that this still happens if the credentials are new, ie. they were both issued and then revoked after the upgrade to 0.5.2. (not just for old credentials from before 0.5.2 getting revoked)

Relevant dep versions (this occurs in Bifold / BC Wallet):

"@credo-ts/anoncreds": "0.5.2",
"@credo-ts/askar": "0.5.2",
"@credo-ts/core": "0.5.2",
"@credo-ts/drpc": "0.5.2",
"@credo-ts/indy-sdk-to-askar-migration": "0.5.2",
"@credo-ts/indy-vdr": "0.5.2",
"@credo-ts/push-notifications": "0.7.0",
"@credo-ts/question-answer": "0.5.2",
"@credo-ts/react-hooks": "^0.6.0",
"@credo-ts/react-native": "0.5.2",
"@hyperledger/anoncreds-react-native": "0.2.1",
"@hyperledger/aries-askar-react-native": "0.2.1",
"@hyperledger/indy-vdr-react-native": "0.2.2",

[genaris]

I think this is something related to the usage of legacy indy identifiers vs did:indy identifiers. In 0.5.x there were some changes in the handling of Indy identifiers, and now they are stored in their fully-qualified, did:indy form. As a result, if you are using former legacy identifiers to query for them, you won't be able to find them.

For instance, I'm using Credo 0.5.3 and when I receive the credential from BCGov Wallet Showcase demo, whose revocation rev_reg_id field is "QEquAHkM35w4XVT3Ku5yat:4:QEquAHkM35w4XVT3Ku5yat:3:CL:567943:student_card:CL_ACCUM:b62ed306-45ff-4951-a1a2-52ade0a312f3". But if I inspect the stored CredentialExchangeRecord I see:

"_tags":{
"anonCredsCredentialRevocationId":"3",
"anonCredsRevocationRegistryId":"did:indy:bcovrin:test:QEquAHkM35w4XVT3Ku5yat/anoncreds/v0/REV_REG_DEF/567943/student_card/b62ed306-45ff-4951-a1a2-52ade0a312f3",
"connectionId":"7a272d39-8453-45d1-b81a-7cdea81b66a3",
"credentialIds":["c94d5ae0-b9e4-4ef9-a1c8-d311f0f7ed7a"],
"role":"holder",
"state":"done",
"threadId":"46514894-0c01-4d72-a44b-a0bc0dcd2c95"
}

So I guess what's happening here is that your notification sender is using the legacy revocation registry id and Credo only looks-up for the new one. Maybe this is related to the issue hyperledger/anoncreds-rs#335 wants to solve (which also was raised when dealing with legacy credentials with Credo 0.5).

In my opinion we should fix these problems in Credo, by either:

• Adding tags to CredentialExchangeRecord that also include legacy identifiers (as it used to be before), so we can efficiently search for one or another
• Doing a double query: first by the legacy one and then by the did:indy one. This will not be that efficient because we'll need two queries, but will work without any data migration and make it easier to deprecate when we decide to do so

[bryce-mcmath]

Thank you for your prompt response!

Doing a double query: first by the legacy one and then by the did:indy one. This will not be that efficient because we'll need two queries, but will work without any data migration and make it easier to deprecate when we decide to do so

How much less efficient are we talking? Would it effectively take twice as long (in worse case) for record lookup if we went with this strategy?

I'm not a Credo maintainer so I'm not sure if my opinion is needed here, but if the above assumption is true that would significantly impact performance in projects like Bifold.

[TimoGlastra]

Doing a double query: first by the legacy one and then by the did:indy one. This will not be that efficient because we'll need two queries, but will work without any data migration and make it easier to deprecate when we decide to do so

I think we can do it with a single query using an OR statement (that's also what we do in certain other cases)

[genaris]

I think we can do it with a single query using an OR statement (that's also what we do in certain other cases)

You are right, I think that's the way to go.

[bryce-mcmath]

I love it. Would that be an easy change for you guys to make or would you prefer the Bifold team take a swing at it first?