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

Add Key value store based on a typescript port of the keyv package #1150

Merged

Conversation

nklomp
Copy link
Member

@nklomp nklomp commented Mar 22, 2023

This is a Key Value store with a typescript port of the Keyv package (https://github.com/jaredwray/keyv)., ESM compatible and should work in browser/Node/React-Native (not tested yet)

Includes TypeORM, Map based and Tiered Key Value storage adapters.

Lastly there is a separate API/Interfaces within Veramo, to not expose core data types from the package.

Tests added and code "fully" implemented.

Happy to get some feedback

@nklomp nklomp marked this pull request as ready for review April 2, 2023 19:01
Copy link
Contributor

@simonas-notcat simonas-notcat left a comment

Choose a reason for hiding this comment

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

Since this plugin is introducing top level methods, it has to have a plugin.schema.json file which is going to be used when generating OpenAPI schema for remote method execution. This means that the interface and arguments of these methods have to be serialisable.

You have to add this section to the package.json file:

  "veramo": {
    "pluginInterfaces": {
      "IKeyValueStore": "./src/key-value-types.ts"
    }
  },

and add this line to the scripts section:

"generate-plugin-schema": "node ../cli/bin/veramo.js dev generate-plugin-schema"

When you do this, you will see that this script is complaining about the types. I think the issue is that you are using <ValueType> in interface definitions. I don't think this is supported by the tools we are using for schema generation.

@nklomp
Copy link
Member Author

nklomp commented May 3, 2023

Hi @simonas-notcat Thanks for the review and sorry for not getting back sooner. It has been a bit hectic over here.

I am not sure about your plugin/schema comment. This is not a Veramo plugin to be clear. It is more like the key manager, or even better the data-store. Both which are not exposing a plugin schema.

It is not a plugin, because you might want to use one or more KV stores in your own plugins. Then you want to have typed values for your keys per KV store. You are not going to call a KV Veramo instance remotely, you would use a remote backing store, like a TypeORM DB or Mongo for instance.

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage: 85.60% and project coverage change: +3.35 🎉

Comparison is base (125bf42) 80.25% compared to head (eb0a51d) 83.60%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1150      +/-   ##
==========================================
+ Coverage   80.25%   83.60%   +3.35%     
==========================================
  Files         118      167      +49     
  Lines        4056    16989   +12933     
  Branches      875     1849     +974     
==========================================
+ Hits         3255    14204   +10949     
- Misses        798     2785    +1987     
+ Partials        3        0       -3     
Impacted Files Coverage Δ
packages/core-types/src/coreEvents.ts 100.00% <ø> (ø)
...s/credential-ld/src/module-types/jsonld/index.d.ts 0.00% <0.00%> (ø)
packages/data-store-json/src/data-store-json.ts 91.55% <ø> (+6.44%) ⬆️
...ckages/data-store-json/src/identifier/did-store.ts 92.52% <ø> (+3.16%) ⬆️
...ckages/data-store-json/src/identifier/key-store.ts 86.84% <ø> (+10.37%) ⬆️
...ata-store-json/src/identifier/private-key-store.ts 94.11% <ø> (+5.48%) ⬆️
packages/data-store-json/src/index.ts 100.00% <ø> (ø)
packages/data-store-json/src/types.ts 0.00% <ø> (ø)
packages/data-store/src/data-store-orm.ts 90.39% <ø> (+17.25%) ⬆️
packages/data-store/src/data-store.ts 97.24% <ø> (-0.26%) ⬇️
... and 76 more

... and 81 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@simonas-notcat
Copy link
Contributor

This is not a Veramo plugin to be clear.

Sorry I did not realise that. Now it makes sense. Looks good to me

@nklomp
Copy link
Member Author

nklomp commented May 10, 2023

@mirceanis Do you also want to do a review or chime in on the PR, given you had some opinions/suggestions in Discord as well?

@nklomp
Copy link
Member Author

nklomp commented May 18, 2023

@mirceanis Nudge, nudge

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks great! There are just a few nits.

packages/kv-store/src/key-value-store.ts Outdated Show resolved Hide resolved
packages/kv-store/src/key-value-store.ts Outdated Show resolved Hide resolved
packages/kv-store/src/key-value-types.ts Outdated Show resolved Hide resolved
packages/kv-store/src/key-value-store.ts Outdated Show resolved Hide resolved
@@ -31,7 +31,7 @@ function createSchema(generator: TJS.SchemaGenerator, symbol: string) {
return { components: { schemas: {} } }
}

let fixedSymbol = symbol.replace('Array<', '').replace('>', '')
let fixedSymbol = symbol.replace(/Array\<(.*)\>/gm, '$1')
Copy link
Member Author

Choose a reason for hiding this comment

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

@mirceanis Just wanted to highlight this change outside of the kv-store package. When testing a bit with the schema-generator I noticed that it mangled anything with carets in there, whilst it was only intended for Arrays.

Copy link
Member

Choose a reason for hiding this comment

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

I did notice this, thanks for adding a more elegant solution!

@nklomp
Copy link
Member Author

nklomp commented May 18, 2023

Okay comments are addressed.

2 things:

  • I took the liberty to add myself to the list of Authors
  • See the remark about the change in the CLI package

@mirceanis
Copy link
Member

  • I took the liberty to add myself to the list of Authors

That's exactly how it should be :)
We value your contributions greatly!

@mirceanis mirceanis merged commit e7138d3 into decentralized-identity:next May 19, 2023
3 checks passed
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.

None yet

3 participants