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(rest)!: upgrade to credo 0.5 #256

Merged

Conversation

TimoGlastra
Copy link
Contributor

Updates the rest API wrapper to Credo 0.5, based on #222. It upgrade to use Aries Askar, Indy VDR and AnonCreds RS, and In addition it also adds support for multi-tenancy.

A follow up PR will add documentation, an easy to run docker image, as well as openid4vc support.

Co-authored-by: Matthew Dean [email protected]
Signed-off-by: Matthew Dean [email protected]
Signed-off-by: Timo Glastra [email protected]

mattdean-digicatapult and others added 18 commits August 21, 2023 11:24
Also siwtches the rest package to use indy-vdr and anoncreds packages

Tests are currently passing but 2 suites fail due to a cleanup issue that still needs to be resolved

There are also several TODO items that should be reviewed

Signed-off-by: Matthew Dean <[email protected]>
Signed-off-by: Matthew Dean <[email protected]>
1. refactored module instantiation to a helper function
2. configured correct types to support v1 and v2 credential and proof protocols and correct formats
3. proof controller updated to take parameters like agent
4. re-enabled oob proof api as create-request

Signed-off-by: Matthew Dean <[email protected]>
Signed-off-by: Matthew Dean <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Attention: Patch coverage is 64.54183% with 267 lines in your changes are missing coverage. Please review.

Project coverage is 62.28%. Comparing base (e5eeaf0) to head (c1f77c9).
Report is 2 commits behind head on main.

❗ Current head c1f77c9 differs from pull request most recent head 5392cb0. Consider uploading reports for the commit 5392cb0 to get more accurate results

Files Patch % Lines
...llers/didcomm/credentials/CredentialsController.ts 44.73% 55 Missing and 8 partials ⚠️
...src/controllers/didcomm/proofs/ProofsController.ts 50.00% 49 Missing and 9 partials ⚠️
...rollers/didcomm/out-of-band/OutOfBandController.ts 54.34% 40 Missing and 2 partials ⚠️
...llers/didcomm/connections/ConnectionsController.ts 38.88% 29 Missing and 4 partials ⚠️
...ackages/rest/src/controllers/did/DidsController.ts 72.58% 17 Missing ⚠️
...t/src/controllers/anoncreds/AnonCredsController.ts 79.71% 14 Missing ⚠️
packages/rest/src/authentication.ts 65.38% 9 Missing ⚠️
...ontrollers/didcomm/proofs/ProofsControllerTypes.ts 71.42% 8 Missing ⚠️
packages/rest/src/server.ts 72.72% 5 Missing and 1 partial ⚠️
.../didcomm/basic-messages/BasicMessagesController.ts 81.48% 4 Missing and 1 partial ⚠️
... and 8 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #256       +/-   ##
===========================================
- Coverage   86.78%   62.28%   -24.51%     
===========================================
  Files          24       79       +55     
  Lines        1241     1832      +591     
  Branches      270      375      +105     
===========================================
+ Hits         1077     1141       +64     
- Misses        164      635      +471     
- Partials        0       56       +56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Copy link
Contributor

@berendsliedrecht berendsliedrecht left a comment

Choose a reason for hiding this comment

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

Nothing blocking at all! Really nice changes. Looks like keeping the API up-to-date and adding new features was quite a lot for this PR, but with the current structure it seems quite easy to do, nice!

packages/rest/README.md Outdated Show resolved Hide resolved
packages/rest/src/authentication.ts Outdated Show resolved Hide resolved
packages/rest/src/cli.ts Outdated Show resolved Hide resolved
packages/rest/src/cliAgent.ts Outdated Show resolved Hide resolved

if (options.multiTenant) {
modules.tenants = new TenantsModule({
sessionLimit: Infinity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thats very scalable! wonder what happens when you exceed f32::MAX sessions :').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the session implementation is a bit broken in Credo, I want to address it for 0.6, as it counts very weird so it's not a good indication of how much resources you're using

multiTenant?: boolean
}): Promise<RestRootAgent | RestRootAgentWithTenants> => {
// FIXME: logger should not be enabled by default
const logger = new TsLogger(LogLevel.off)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be possible to set with the start script? with something like -vvv for loglevel=3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can do it for the CLI part already --log-level. I need to improve the non-cli (import rest server) api a bit. Will look at it in my next PR if okay

import type { ApiError } from '../types'

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function alternativeResponse<T>(input: T): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

T can be return type as well right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No as then the return type on the method is wrong. This is my workaround to have alternative responses in TSOA. The proper way to do it (inject error handler) doesn't allow you to set examples for alternative status codes.

This way the OpenAPI / swagger is nice, which is most important to me

packages/rest/tsconfig.build.json Outdated Show resolved Hide resolved
@@ -2,7 +2,9 @@
"extends": "../../tsconfig.build.json",
"compilerOptions": {
"outDir": "./build",
"types": []
"types": [],
"skipLibCheck": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thats already set in the abse tsconfig right?

Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra TimoGlastra mentioned this pull request Mar 27, 2024
4 tasks
@TimoGlastra TimoGlastra merged commit 3849aec into openwallet-foundation:main Mar 27, 2024
7 checks passed
@TimoGlastra TimoGlastra deleted the feat/upgrade-rest-afj-050 branch March 27, 2024 19:20
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