feat: add indynamespace for ledger id for anoncreds

[morrieinmaas]

Signed-off-by: Moriarty [email protected]

[codecov-commenter]

Codecov Report

Merging #965 (0cc57ab) into 0.3.0-pre (5e9e0fc) will increase coverage by 0.01%.
The diff coverage is 95.91%.

@@              Coverage Diff              @@
##           0.3.0-pre     #965      +/-   ##
=============================================
+ Coverage      88.27%   88.28%   +0.01%     
=============================================
  Files            639      640       +1     
  Lines          15170    15202      +32     
  Branches        2566     2467      -99     
=============================================
+ Hits           13391    13421      +30     
- Misses          1681     1776      +95     
+ Partials          98        5      -93     

Impacted Files	Coverage Δ
...ntials/formats/indy/IndyCredentialFormatService.ts	87.43% <ø> (ø)
packages/core/src/modules/ledger/IndyPool.ts	78.57% <0.00%> (-1.92%)	⬇️
packages/core/src/modules/ledger/ledgerUtil.ts	100.00% <ø> (ø)
packages/module-tenants/src/TenantsModule.ts	100.00% <ø> (ø)
...re/src/modules/dids/methods/sov/SovDidRegistrar.ts	100.00% <100.00%> (ø)
.../repository/AnonCredsCredentialDefinitionRecord.ts	100.00% <100.00%> (+18.18%)	⬆️
...c/modules/indy/repository/AnonCredsSchemaRecord.ts	100.00% <100.00%> (+18.18%)	⬆️
packages/core/src/modules/ledger/LedgerApi.ts	97.61% <100.00%> (+0.39%)	⬆️
...e/src/modules/ledger/services/IndyLedgerService.ts	70.53% <100.00%> (+0.28%)	⬆️
packages/core/src/utils/index.ts	100.00% <100.00%> (ø)
... and 43 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[morrieinmaas]

closes #944

Note to self: update docs when all review feedback is addressed.

[TimoGlastra]

This is great @morrieinmaas! I'm not sure on the direction of adding the unqualified identifiers and methods to the repositories and records. As descried in #944 the records would be stored with the qualified indy dids that would overwrite the unqualified indy dids:

add fully qualified identifier to the anon creds (schema|credential definition) records (replace the non qualified identifier before storing and after retrieving it). For indy we use the did:indy spec identifiers to store.

• Note: Records stores fully qualified identifier as identifier.
• Need to transform unqualified to qualified before storing or querying
• Need to transform qualified to unqualified after retrieving

Was there a specific reason why you deviated from the described approach?

[berendsliedrecht]

This is great @morrieinmaas! I'm not sure on the direction of adding the unqualified identifiers and methods to the repositories and records. As descried in #944 the records would be stored with the qualified indy dids that would overwrite the unqualified indy dids:
Was there a specific reason why you deviated from the described approach?

I am partly to blame for this and I am certain that there is a better way to do it but here was my reasoning:

1. We never store the unqualified identifier, we just add id as a tag to query by credentialDefinitionId or schemaId.
2. When we have one of those identifiers we do not have the namespace and querying by qualified identifier is impossible then.
3. Converting to only support qualified identifiers would simply swap the usafe of (find|get)byunqualifiedXXXto (find|get)byqualified in the repository and instead of getting the unqualifiedIdentifier on the record you now get the qualifiedIdentifier on the record.

Swapping the identifier to qualified before storing and after retrieving I think comes with inherent issues. Every possible way of updating/adding/getting in the repository has to be checked and changed and will cause "weird" code.

I am 10000% open for better implementations. This to me however looked like the quickest way to later convert to qualifiedIdentifiers but I would not be suprised if I overcomplicated things here. :)

[TimoGlastra]

Also after thinking about it a bit more this doesn't seem to follow the indy did method specification for schema / credentials definitions ids:
https://hyperledger.github.io/indy-did-method/#did-urls-for-indy-object-identifiers

[TimoGlastra]

@blu3beri I'm not on the same page with you on this one. The goal of the AnonCreds records is to make them generic already, so we can get away from the endless unqualified legacy indy dids issues. By replacing the id before storing on the object itself, we make the id already follow the did:indy specification. The nice thing is, once we add support for did:indy it will just work and we won't have to transform the records anymore because we're already following the spec. So I would argue strongly against any tags or properties that have to do with unqualified dids.

If we have the unqualified schema/cred def id and the namespace we can transform that into the did:indy qualified shcema/cred def id and find it by the qualified id. if we have the qualified id we can reduce it to an unqualified id.

As an example the steps of creating / retrieving a schema:

1. Construct qualified indy did schema id from the input parameters:

• name: myschema, version: 1.0.0, did: 1234. pool: { id: 'test', indyNamespace: 'test' }
• qualified indy did: did:indy:test:1234/anoncreds/v0/SCHEMA/myschema/1.0.0

2. Query for existing anon creds schema record where id tag is did:indy:test:1234/anoncreds/v0/SCHEMA/myschema/1.0.0
3. a record exists:

• Transorm qualified indy did schema id into uqualified schema id 1234:2:myschema:1.0.0
• Replace id property on record schema with unqualified shcema id
• return record
  3b Record does not exist
  • Create schema using indy sdk
  • Create record, replace id with qualified indy did schema from step 1 before returning
  • Store record with qualified did
  • Return schema with unqualified did

Maybe I'm not fully understanding what you're saying, but I don't see the need for these properties?

[berendsliedrecht]

@blu3beri I'm not on the same page with you on this one. The goal of the AnonCreds records is to make them generic already, so we can get away from the endless unqualified legacy indy dids issues. By replacing the id before storing on the object itself, we make the id already follow the did:indy specification. The nice thing is, once we add support for did:indy it will just work and we won't have to transform the records anymore because we're already following the spec. So I would argue strongly against any tags or properties that have to do with unqualified dids.

Yeah I agree with you. I could not really find a way to retrieve a anoncredsrecord ONLY via the qualified identifier when we do not know the namespace (if this is even required). If, in every context, we are aware of the namespace, 100% remove every reference to unqualified identifiers.

If we have the unqualified schema/cred def id and the namespace we can transform that into the did:indy qualified shcema/cred def id and find it by the qualified id. if we have the qualified id we can reduce it to an unqualified id.

As an example the steps of creating / retrieving a schema:

1. Construct qualified indy did schema id from the input parameters:

Where do we get these input parameters from?

Maybe I'm not fully understanding what you're saying, but I don't see the need for these properties?

Yeah I don't think it was the best explanation, and I probably don't understand the exact situation we are in. Where we use unqualified or qualified and what we get back from the ledger.

[TimoGlastra]

Where do we get these input parameters from?

From the api, except for the pool, currently that is always the first one. Later on we can allow to specify the qualified did you'd like to use to register it and we can extract it from the qualified did

[berendsliedrecht]

From the api, except for the pool, currently that is always the first one. Later on we can allow to specify the qualified did you'd like to use to register it and we can extract it from the qualified did

Ahh okay now I think I fully understand it. I assume with first one you mean ledger in your configuration?

[morrieinmaas]

wow. thx for all the feedback guys🙏🚀. Really nice to have hit some hot topic that triggered a good discussion. I'll have a proper read through and will reply more in depth tomorrow

[morrieinmaas]

@blu3beri I'm not on the same page with you on this one. The goal of the AnonCreds records is to make them generic already, so we can get away from the endless unqualified legacy indy dids issues. By replacing the id before storing on the object itself, we make the id already follow the did:indy specification. The nice thing is, once we add support for did:indy it will just work and we won't have to transform the records anymore because we're already following the spec. So I would argue strongly against any tags or properties that have to do with unqualified dids.

If we have the unqualified schema/cred def id and the namespace we can transform that into the did:indy qualified shcema/cred def id and find it by the qualified id. if we have the qualified id we can reduce it to an unqualified id.

As an example the steps of creating / retrieving a schema:

1. Construct qualified indy did schema id from the input parameters:


* name: myschema, version: 1.0.0, did: 1234. pool: { id: 'test', indyNamespace: 'test' }

* qualified indy did: `did:indy:test:1234/anoncreds/v0/SCHEMA/myschema/1.0.0`


2. Query for existing anon creds schema record where id tag is `did:indy:test:1234/anoncreds/v0/SCHEMA/myschema/1.0.0`

3. a record exists:


* Transorm qualified indy did schema id into uqualified schema id `1234:2:myschema:1.0.0`

* Replace id property on record schema with unqualified shcema id

* return record
  3b Record does not exist
  
  * Create schema using indy sdk
  * Create record, replace id with qualified indy did schema from step 1 before returning
  * Store record with qualified did
  * Return schema with unqualified did

Maybe I'm not fully understanding what you're saying, but I don't see the need for these properties?

Ahh thx for the exmaple @TimoGlastra - I was missing some info there which is also why you correctly were puzzled about this part. I was following this instead but I get it now

[morrieinmaas]

Thanks for the feedback @TimoGlastra

[TimoGlastra]

Ah damnit, the Github UI signoff feature is broken. S

[berendsliedrecht]

Not sure why it resubmitted my previous comments...

[morrieinmaas]

Thanks for the feedback @TimoGlastra 🙏 (and sorry for this being less straight-forward than could be 😅)

[TimoGlastra]

Where does schemaSeqNo come from? I don't think this is still used?

[morrieinmaas]

this is used to create the anonCreds trunk ${did}/anoncreds/v0/CLAIM_DEF/${seqNo}/${credDef.tag} so that's needed I reckon. Maybe I'm missing sth here though?

[TimoGlastra]

I don't think the schemaSeqNo is defined anywhere? It's added to the signature of this method, but I think we shouldn't add custom properties to existing objects.

[morrieinmaas]

hmm ok fair enough. was thinking to do it this way to safe a call to getSchema but rather just have it at hand directly. Maybe you have another/better idea?

[TimoGlastra]

Where do we get the scheqSeqNo from?

[TimoGlastra]

Maybe it can be a separate property that we pass separately to this method and store separately?

[morrieinmaas]

we get it from CredentialDefinitionTemplate.schema.seqno which is the type in ledgerApi registerCredentialDefinition

[morrieinmaas]

Maybe it can be a separate property that we pass separately to this method and store separately?

yeah that might be possible. Unsure right now where to store that then tho -I'll have a think

[morrieinmaas]

@TimoGlastra this is the one bit I'm actually unsure how to do better. Happy to implement suggestions.

Otherwise all feedback addressed

[morrieinmaas]

@TimoGlastra if you have some time maybe we can work this out (together) and get this merged

[TimoGlastra]

@morrieinmaas added a PR to resolve my comment. It seems DCO is failing. Could you squash all commits into one so we can merge?

[berendsliedrecht]

@morrieinmaas added a PR to resolve my comment. It seems DCO is failing. Could you squash all commits into one so we can merge?

We can use the "set dco to pass" and squash when we merge. No need to squash before hand.

[TimoGlastra]

Yes that makes sense @blu3beri :-)

[morrieinmaas]

ALright, thx. Anything else that needs doing then?