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(ledger): smart schema and credential definition registration #900

Conversation

morrieinmaas
Copy link
Contributor

Signed-off-by: Moriarty [email protected]

closes #859

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2022

Codecov Report

Merging #900 (f64b016) into 0.3.0-pre (310185b) will increase coverage by 0.12%.
The diff coverage is 89.01%.

@@              Coverage Diff              @@
##           0.3.0-pre     #900      +/-   ##
=============================================
+ Coverage      86.97%   87.09%   +0.12%     
=============================================
  Files            532      536       +4     
  Lines          12965    13050      +85     
  Branches        2229     2244      +15     
=============================================
+ Hits           11276    11366      +90     
+ Misses          1593     1588       -5     
  Partials          96       96              
Impacted Files Coverage Δ
.../repository/AnonCredsCredentialDefinitionRecord.ts 81.81% <81.81%> (ø)
...c/modules/indy/repository/AnonCredsSchemaRecord.ts 81.81% <81.81%> (ø)
...ository/AnonCredsCredentialDefinitionRepository.ts 85.71% <85.71%> (ø)
...dules/indy/repository/AnonCredsSchemaRepository.ts 85.71% <85.71%> (ø)
packages/core/src/modules/ledger/LedgerModule.ts 97.26% <94.59%> (+16.30%) ⬆️
packages/core/src/modules/ledger/ledgerUtil.ts 100.00% <100.00%> (ø)
...e/src/modules/ledger/services/IndyLedgerService.ts 69.89% <0.00%> (+2.04%) ⬆️
packages/core/src/error/IndySdkError.ts 100.00% <0.00%> (+60.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 310185b...f64b016. Read the comment docs.

@morrieinmaas morrieinmaas changed the title Initial commit with dummy structures to be filled Smart schema and credential definition registration Jun 23, 2022
@morrieinmaas
Copy link
Contributor Author

Still need to add tests for new functionality. Old stuff works

@morrieinmaas morrieinmaas force-pushed the feat/smart-schema-and-creds branch from 78c3fc7 to df4f9fd Compare June 23, 2022 14:21
@morrieinmaas morrieinmaas changed the title Smart schema and credential definition registration feat: smart schema and credential definition registration Jun 23, 2022
@morrieinmaas morrieinmaas force-pushed the feat/smart-schema-and-creds branch from df4f9fd to 81e39fd Compare June 23, 2022 14:24
@morrieinmaas morrieinmaas changed the title feat: smart schema and credential definition registration feat: Smart schema and credential definition registration Jun 23, 2022
@morrieinmaas morrieinmaas changed the title feat: Smart schema and credential definition registration feat: Smart schema and credential definition registration Jun 23, 2022
Moriarty added 6 commits June 24, 2022 15:01
todo:
* move this logic to ledger module

Signed-off-by: Moriarty <[email protected]>
…chema-and-creds

todo:
* move findBy methods to ledgerModule and out of the anonCreds impls

Signed-off-by: Moriarty <[email protected]>
@morrieinmaas
Copy link
Contributor Author

Note to self @morrieinmaas & @blu3beri & @TimoGlastra still need to add tests.

@morrieinmaas morrieinmaas force-pushed the feat/smart-schema-and-creds branch from fe0c8d9 to e463253 Compare July 4, 2022 12:00
@morrieinmaas
Copy link
Contributor Author

@TimoGlastra @blu3beri would be great to get som efeedback before I add the tests - thanks in advance

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great @morrieinmaas! Left some nits but mostly around naming. Overall implementation looks good.

packages/core/src/modules/ledger/LedgerModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/ledger/LedgerModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/ledger/LedgerModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/ledger/LedgerModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/ledger/LedgerModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/ledger/LedgerModule.ts Outdated Show resolved Hide resolved
packages/core/src/modules/ledger/LedgerModule.ts Outdated Show resolved Hide resolved
@morrieinmaas morrieinmaas marked this pull request as ready for review July 5, 2022 15:03
@morrieinmaas morrieinmaas requested a review from a team as a code owner July 5, 2022 15:03
@morrieinmaas morrieinmaas force-pushed the feat/smart-schema-and-creds branch from 50a99d1 to ee7389d Compare July 6, 2022 08:25
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me @morrieinmaas. I think integrating the get schema/cred def and adding methods to get all created schema/cred def records would be a nice addition, but something we can add in a future pr without breaking changes.

As discussed yesterday only thing we should add is storing the associated ledger in the record. I'll try to add some more details later today on possible approaches

I'm going to Request Changes, mostly to prevent it from accidently being merged, but current code is approved

@morrieinmaas
Copy link
Contributor Author

@TimoGlastra Thanks for the feedback. Should be addressed.

@morrieinmaas morrieinmaas requested a review from TimoGlastra July 6, 2022 12:39
@TimoGlastra
Copy link
Contributor

TimoGlastra commented Jul 12, 2022

  • add indyNamespace to the indy ledgers configuration (need to document this: https://hyperledger.github.io/indy-did-method/#indy-did-method-identifiers)
  • 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
  • Need to return the fully qualified id somehow from the service (either by adding qualifiedId return value, or overriding the id before returning and the setting it as the unqualified again after storing it).
  • Ledger module construct fully qualified string/name. Check with that against record store

@morrieinmaas morrieinmaas force-pushed the feat/smart-schema-and-creds branch from 2332b3b to 231e681 Compare July 13, 2022 14:50
FIXME: test deriveProof -> should derive proof successfully fails
due to inability to convert key in node-bbs-signatures library

Signed-off-by: Moriarty <[email protected]>
@morrieinmaas
Copy link
Contributor Author

@TimoGlastra - changes addressed

@TimoGlastra TimoGlastra changed the title feat: Smart schema and credential definition registration feat(ledger): smart schema and credential definition registration Jul 14, 2022
@morrieinmaas
Copy link
Contributor Author

@TimoGlastra can this be merged then?

@TimoGlastra TimoGlastra merged commit df1ed92 into openwallet-foundation:0.3.0-pre Jul 14, 2022
genaris pushed a commit to 2060-io/aries-framework-javascript that referenced this pull request Sep 16, 2022
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.

Smart schema and credential definition registration
4 participants