Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: added support for indexing by legacy identifiers #335

Closed

Conversation

wadeking98
Copy link

@wadeking98 wadeking98 commented May 7, 2024

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

Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
Signed-off-by: wadeking98 <wkingnumber2@gmail.com>
@swcurran
Copy link
Member

swcurran commented May 7, 2024

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
Copy link
Author

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
Copy link
Member

swcurran commented May 7, 2024

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
Copy link
Member

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)

@TimoGlastra
Copy link
Member

TimoGlastra commented May 8, 2024

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
Copy link
Author

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
Copy link
Contributor

genaris commented May 8, 2024

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
Copy link
Member

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

@wadeking98
Copy link
Author

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
Copy link
Author

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
Copy link
Author

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants