hyperledger / anoncreds-rs

anoncreds-rs
https://wiki.hyperledger.org/display/anoncreds
Apache License 2.0
74 stars 55 forks source link

feat: added support for indexing by legacy identifiers #335

Closed wadeking98 closed 5 months ago

wadeking98 commented 6 months ago

Currently different parts of the verifiable credential ecosystem are being migrated from did:sov spec to did:indy. This PR adds support to index did:sov and did:indy by a common format.

ie this pr makes it so that identifiers such as these are equal: did:indy:sovrin:F72i3Y3Q4i466efjYJYCHM/anoncreds/v0/SCHEMA/npdb/4.3.4 and F72i3Y3Q4i466efjYJYCHM:2:npdb:4.3.4

I've tested these changes in bifold and they fix the issue with the mobile verifier and accepting proofs

swcurran commented 6 months ago

I’m slightly nervous about this because the unique identifier portion of an Indy did:sov is supposed to be different from an Indy did:indyDID — the algorithm that generates the identifier when the DID is first registered is different between the two methods. E.g. the F72i3Y3Q4i466efjYJYCHM part of the identifiers are not the same with “real” did:sov and did:indy. I also see that the relatively flippant idea we accepted when creating did:indy of changing the the unique identifier algorithm because the new way was “better” was just a bad idea. We should have left the embedded identifier the same and accepted both as equivalent — as is being done here.

So in theory — this fix should not work/be needed. That said, I think what is happening is that somewhere, someone’s code is just doing an inapporpriate (but understandable) text transformation for DID Indy, and so there is no real harm in doing this. The different identifier is generated at DID creation time, and once it is created, it can be put into a DID of either form and still be found.

So — all good from my perspective. Others?

wadeking98 commented 6 months ago

If you don't think this should go in the main anoncreds package, I can just make a specific npm package for these changes that only bifold will use as a patch. I was planning on this only being a temporary fix anyway while projects transition to did:indy.

swcurran commented 6 months ago

I’m happy for it to go in — I think it makes good sense, a clever way to make this transition much easier.

My thoughts on the topic changed as I wrote my previous comment — as reflected in hesitation at the start, go for it by the end :-). I would like @TimoGlastra @berendsliedrecht @andrewwhitehead to look at the code though, and weigh in on it.

TimoGlastra commented 6 months ago

The identifier is determend based on the nym transaction right? So i think we can do the transformation two ways in this case as a did won't change:

TimoGlastra commented 6 months ago

Curious how this solves your issue in Bifold @wadeking98, as you still need to make the query logic try to fetch both identifiers new/legacy

Also i'm not sure what will happen if i put a cred with qualified identifiers in a presentation to a wallet that only supports unqualified and vice versa. It would seem to me there's more logic needed for this

wadeking98 commented 6 months ago

Hi Timo, I think I need a bit more clarification on your proposed transformation method. I don't know if we can properly convert from legacy identifiers to the new did:indy ones because I don't know which ledger a legacy identifier is coming from. I think I'd have to import some functionality from indy-vdr in order to figure that out which seems likely to become messy.

This solves my issue in bifold because after the credo update some objects were being saved using the new did:indy identifiers and then fetched using the legacy identifiers. This changes makes it so that legacy and did:indy identifiers can be used interchangeably while we're transitioning away from legacy to did:indy identifiers across the ecosystem.

genaris commented 6 months ago

The identifier is determend based on the nym transaction right? So i think we can do the transformation two ways in this case as a did won't change:

  • legacy to did:indy. If the anoncreds object was created with w legacy identifier it means the nym transaction version is 1
  • new did:indy: we can transform it to a legacy identifier as if the user has one with a legacy it works, we only need to transform did:indy dids with a legacy identifier though (but this should be detectable using a regex or something)

Wasn't this type of transformation already supported in AFJ 0.4.0? At that time, we were limited to use did:indy only for object creation, but not when it comes to storage/resolution. What has changed in Credo 0.5.0/anoncreds-rs to prevent that?

Probably a good topic to discuss in tomorrow's Credo Contributors meeting!

swcurran commented 6 months ago

Is this PR still needed? Any progress on root cause discussions?

wadeking98 commented 6 months ago

I think the credo changes should cover it, I'll just do some testing once those credo changes are available and then I'll close this if it all works out

wadeking98 commented 6 months ago

Still running into the same issue after upgrading to 0.5.3 going to do some more testing to see if I can solve it in credo

wadeking98 commented 6 months ago

@genaris @TimoGlastra cc: @andrewwhitehead I'm not quite sure what changes you would like on this PR before we consider merging. Could you please clarify?