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: Add cheqd-sdk module #1334

Merged
merged 21 commits into from
Apr 13, 2023
Merged

Conversation

DaevMithran
Copy link
Contributor

@DaevMithran DaevMithran commented Feb 22, 2023

This PR adds cheqd-sdk module in AFJ with the following interfaces

  • CheqdDidRegistrar
  • CheqdDidResolver
  • CheqdSdkAnonCredsRegistry

Added tests for all the classes

Signed-off-by: DaevMithran <[email protected]>
@DaevMithran DaevMithran requested a review from a team as a code owner February 22, 2023 10:49
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.

Left an initial review. Really like the addition, this is great, but have some questions/concers about integrating with existing AFJ infrastructure and making sure it works in multitenancy/paralel processing cases

packages/cheqd-sdk/package.json Outdated Show resolved Hide resolved
packages/cheqd-sdk/package.json Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/anoncreds/utils/transform.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/anoncreds/utils/transform.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/ledger/CheqdSdkLedgerService.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/ledger/CheqdSdkLedgerService.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/tests/cheqd-did-registrar.e2e.test.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/tests/cheqd-did-registrar.e2e.test.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/tests/setup.ts Outdated Show resolved Hide resolved
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.

Some final changes. Have you tested these changes in React Native? We had to apply a lot of workarounds to get it to work, and we should probalby add documentation for this.

packages/cheqd-sdk/package.json Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/CheqdModuleConfig.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/dids/CheqdDidRegistrar.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/dids/CheqdDidRegistrar.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/dids/CheqdDidRegistrar.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/dids/CheqdDidRegistrar.ts Outdated Show resolved Hide resolved
packages/cheqd-sdk/src/dids/CheqdDidRegistrar.ts Outdated Show resolved Hide resolved
TimoGlastra and others added 2 commits April 5, 2023 10:27
chore: some fixes

Signed-off-by: Timo Glastra <[email protected]>
@DaevMithran DaevMithran requested a review from TimoGlastra April 5, 2023 08:03
Copy link

@lampkin-diet lampkin-diet left a comment

Choose a reason for hiding this comment

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

Great Job! A lot of stuff are done, let's maybe chat about possible changes if you want.

packages/cheqd/src/anoncreds/utils/identifiers.ts Outdated Show resolved Hide resolved
packages/cheqd/src/dids/CheqdDidResolver.ts Show resolved Hide resolved
packages/cheqd/src/dids/didCheqdUtil.ts Show resolved Hide resolved
packages/cheqd/src/dids/didCheqdUtil.ts Show resolved Hide resolved
@TimoGlastra
Copy link
Contributor

@DaevMithran Is the PR ready to be merged, or do you still need to incoroporate any of the outstanding comments?

Also, it seems DCO is failing.

@DaevMithran
Copy link
Contributor Author

DaevMithran commented Apr 10, 2023

@DaevMithran Is the PR ready to be merged, or do you still need to incoroporate any of the outstanding comments?

Also, it seems DCO is failing.

@TimoGlastra fixed the DCO, it's ready to be merged. Just one thing should we store ledger responses in cache? I haven't included that yet.

Signed-off-by: DaevMithran <[email protected]>
@TimoGlastra
Copy link
Contributor

I think we can add caching in a follow-up PR

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2023

Codecov Report

Merging #1334 (6414422) into main (4e7a380) will decrease coverage by 0.25%.
The diff coverage is 76.59%.

@@            Coverage Diff             @@
##             main    #1334      +/-   ##
==========================================
- Coverage   85.86%   85.61%   -0.25%     
==========================================
  Files         870      884      +14     
  Lines       20489    21032     +543     
  Branches     3449     3558     +109     
==========================================
+ Hits        17592    18007     +415     
- Misses       2725     2851     +126     
- Partials      172      174       +2     
Impacted Files Coverage Δ
packages/core/src/utils/TypedArrayEncoder.ts 83.33% <0.00%> (-16.67%) ⬇️
packages/cheqd/src/dids/CheqdDidResolver.ts 50.58% <50.58%> (ø)
packages/core/src/utils/uuid.ts 80.00% <66.66%> (-20.00%) ⬇️
...s/core/src/modules/dids/domain/key-type/ed25519.ts 95.45% <75.00%> (-4.55%) ⬇️
packages/cheqd/src/anoncreds/utils/transform.ts 76.27% <76.27%> (ø)
packages/cheqd/src/dids/CheqdDidRegistrar.ts 76.57% <76.57%> (ø)
...d/src/anoncreds/services/CheqdAnonCredsRegistry.ts 80.00% <80.00%> (ø)
packages/cheqd/src/ledger/CheqdLedgerService.ts 80.85% <80.85%> (ø)
packages/cheqd/src/dids/didCheqdUtil.ts 82.35% <82.35%> (ø)
packages/cheqd/src/anoncreds/utils/identifiers.ts 94.11% <94.11%> (ø)
... and 8 more

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

@DaevMithran
Copy link
Contributor Author

@TimoGlastra The tests failing are not specific to this PR, its the same one's failing in main as well

Signed-off-by: DaevMithran <[email protected]>
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.

5 participants