fix: unqualified anoncreds revocation notification support #2240
Conversation
|
Signed-off-by: Ariel Gentile <[email protected]> fix: data type Signed-off-by: Ariel Gentile <[email protected]>
genaris
force-pushed
the
fix/unqualified-anoncreds-revocation-notification
branch
from
39598b8 to
c7bbdd4
Compare
Signed-off-by: Ariel Gentile <[email protected]>
|
This is how the changes look like right now for a 0.5.13 credo: https://github.com/openwallet-foundation/credo-ts/compare/v0.5.x...genaris:fix/v0.5x-unqualified-anoncreds-revocation-support?expand=1 |
Signed-off-by: Ariel Gentile <[email protected]>
|
|
||
| // Find credential exchange record associated with this credential | ||
| credentialExchangeRecord = | ||
| (await this.credentialRepository.getAll(agentContext)).find((record) => |
There was a problem hiding this comment.
Is there no way to find the credential exchange record associated with a credential record?
This seems inefficient to run for every query ... :/
There was a problem hiding this comment.
Yes, that's why I tried to avoid running it in cases not affected by this problem (e.g. using qualified identifiers).
Probably we have some other tricks to do, but I don't think in practice it will be so critical, since this code runs on holder side and I don't expect users to hold thousands of credentials using unqualified identifiers.
Something not done in this PR (at the moment) are changes on AnonCredsCredentialFormatService: we can also add the anonCredsUnqualified* tags to CredentialExchangeRecords. This would at least prevent this code from running in 0.5.14 agents.
|
@bryce-mcmath Were you able to test this? I was wondering if this could still get merged into the patch as it is the last thing allowing this scenario to work and do our migrations incrementally. I don't have a good understanding of credo storage, but, I agree that the performance doesn't seem to be much of an issue if it only does this when unqualified id's are present and is only on the holder side. |
It's already in our patches in the Testflight BC Wallet if you want to double check anything in particular. Seems to be working well so far |
|
OK, great. Thank you. |
|
@TimoGlastra since this fix can be useful for some users and appearently works well for their use case, do you think we can re-target this PR to 0.5.x branch so it can be included in the next patch release for them? I can create a different one for |
| credentialExchangeRecord = | ||
| (await this.credentialRepository.getAll(agentContext)).find((record) => |
There was a problem hiding this comment.
I think we should limit the calling of this to:
- filter by state (credential received and completed)
- only if the incoming revocation id is unqualified
This is a fix focused mainly for 0.5.x deployments to address the issue #2206.
The proposed solution here is to search for W3C Credential Records matching the revocation notification only in case we don't find any matching Credential Exchange record. The opposite (search first for W3C Credential records) would work, but force us to get all Credential Exchange records and filter them (i.e. no optimized query using tags), since
RevocationNotificationReceivedEventrequires to send the associated CredentialExchangeRecord.I think for 0.6 we could aim to update this API to focus on W3CCredentialRecord directly, meaning that we will need to do some updates to the data model (e.g. W3CCredentialRecord will need to hold revocation info and connectionId).
@JoLuPuma @jamshale @barnabef @zoblazo I didn't test it deeply yet but, if you have the chance of patching your 0.5.13 with this and test it, please let me know to make sure this does the trick.