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: indy sdk aries askar migration script #1289

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Feb 12, 2023

  • feat: setup package
  • feat: setup some base components

@berendsliedrecht berendsliedrecht changed the title feat/indy sdk aries askar migration script feat: indy sdk aries askar migration script Feb 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2023

Codecov Report

Merging #1289 (80044bb) into main (5f71dc2) will decrease coverage by 4.88%.
The diff coverage is 62.74%.

@@            Coverage Diff             @@
##             main    #1289      +/-   ##
==========================================
- Coverage   85.42%   80.55%   -4.88%     
==========================================
  Files         782      765      -17     
  Lines       19167    19038     -129     
  Branches     3116     3099      -17     
==========================================
- Hits        16374    15336    -1038     
- Misses       2786     3695     +909     
  Partials        7        7              
Impacted Files Coverage Δ
packages/indy-sdk-to-askar-migration/src/utils.ts 6.25% <6.25%> (ø)
...ar-migration/src/IndySdkToAskarMigrationUpdater.ts 67.02% <67.02%> (ø)
...gration/src/errors/IndySdkToAskarMigrationError.ts 100.00% <100.00%> (ø)
packages/indy-sdk-to-askar-migration/src/index.ts 100.00% <100.00%> (ø)

... and 86 files with indirect coverage changes

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

@berendsliedrecht berendsliedrecht force-pushed the feat/indy-sdk-aries-askar-migration-script branch from 43e45cb to fac9efb Compare March 13, 2023 13:43
Signed-off-by: blu3beri <[email protected]>
@berendsliedrecht berendsliedrecht marked this pull request as ready for review March 13, 2023 15:30
@berendsliedrecht berendsliedrecht requested a review from a team as a code owner March 13, 2023 15:30
"@aries-framework/core": "0.4.0-alpha.71",
"@aries-framework/indy-sdk": "0.4.0-alpha.71",
"@aries-framework/node": "0.4.0-alpha.71",
"@hyperledger/aries-askar-shared": "file:/Users/beri/Developer/work/hyperledger/aries-askar/wrappers/javascript/aries-askar-shared"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will replace the local files when they are released again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the reference to @aries-framework/* packages point to * instead of an specific version (0.4.0-alpha.71)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should use 0.3.3 (will be updated when released)

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any blockers for a new release? Otherwise we can cut one today

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, the. migration addition. So just releasing the wrappers would be good.

Copy link
Contributor

@genaris genaris left a comment

Choose a reason for hiding this comment

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

Didn't do a deep review yet but left a few acid remarks regarding the scope and to :clarifying it 💥

"@aries-framework/core": "0.4.0-alpha.71",
"@aries-framework/indy-sdk": "0.4.0-alpha.71",
"@aries-framework/node": "0.4.0-alpha.71",
"@hyperledger/aries-askar-shared": "file:/Users/beri/Developer/work/hyperledger/aries-askar/wrappers/javascript/aries-askar-shared"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the reference to @aries-framework/* packages point to * instead of an specific version (0.4.0-alpha.71)?

* Migration class to move a wallet form the indy-sdk structure to the new
* askar wallet structure.
*
* Right now, this is ONLY supported within React Native environments AND only sqlite.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this disclaimer is fine here but should be part of the README.

I understand the reasons about stating that it works in React Native because it assumes that no issuer or verifier side has been supported there officially, but I don't think it is accurate as we can also have a NodeJS holder-role agent that would be in the same situation.

Also, what about mediation records? Mediator configuration is not taken into account. In that case you assume only current Bifold case where a mediatorInvite is provided at startup and there is apparently no problem on creating the mediation coordination again. But that's not necessarily the case of every app using AFJ.

What about apps using generic records module? And those that have custom plug-ins that store custom records? 🤯

The latter case is maybe extreme (don't worry for us, as we are starting production using 0.4.0 right away 😎 ), but generic records and maybe proofs are used in some mobile wallets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think all the cases you described should work in Node.JS, however it's not too much work to add credential definitions (the only one missing) after which it will fully work in Node.JS. We can do that in an upcoming release I think

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 we would only need to create migration for the indy records, like Indy::Key or Indy::Credential. Every custom afj record does not need additional conversion, just moving to a different table (which is dan with the new askar ffi function).

Node.js and postgres should be trivial, without any breaking changes, to add but not a MUST required for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good to see that all records are copied (it´s not always a good idea to review a PR at midnight). So if the only missing objects in migration are the Credential Definitions I think it makes more sense to allow migration in NodeJS as not all server deployments are used to issue credentials (we have a mediator for instance, or other services that verify but not issue credentials).

packages/indy-sdk-to-askar-migration/package.json Outdated Show resolved Hide resolved
yarn.lock Outdated
@@ -10,6 +10,88 @@
"@jridgewell/gen-mapping" "^0.1.0"
"@jridgewell/trace-mapping" "^0.3.9"

"@aries-framework/[email protected]", "@aries-framework/[email protected]+a4204ef2":
Copy link
Contributor

Choose a reason for hiding this comment

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

These additions here can be avoided if using * for modules in package.json

packages/indy-sdk-to-askar-migration/README.md Outdated Show resolved Hide resolved
packages/indy-sdk-to-askar-migration/tests/migrate.test.ts Outdated Show resolved Hide resolved
Comment on lines 41 to 42
// TODO: update with an aca-py issued revokable credential
// community agent MIGHT have revocrevoc
Copy link
Contributor

Choose a reason for hiding this comment

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

community agent has a tails server set-up so that should work

packages/indy-sdk-to-askar-migration/tests/migrate.test.ts Outdated Show resolved Hide resolved
berendsliedrecht and others added 2 commits March 14, 2023 09:38
Co-authored-by: Timo Glastra <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: blu3beri <[email protected]>
@berendsliedrecht
Copy link
Contributor Author

@TimoGlastra @genaris I think I will do a release of the askar wrappers and after that it should be ready for a rereview.

packages/indy-sdk-to-askar-migration/package.json Outdated Show resolved Hide resolved

for (const row of credentials) {
this.agent.config.logger.debug(`Migrating ${row.name} to the new askar format`)
const data = JSON.parse(row.value as string) as {
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this type differ from AnonCredsCredential interface?

I see it has rev_reg and witness keys. Are those added when stored, or are those also transfered over the wire from issuer to holder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see it has rev_reg and witness keys. Are those added when stored, or are those also transfered over the wire from issuer to holder?

I am not entirely sure what you mean here.

how does this type differ from AnonCredsCredential interface?

I think I can use the AnonCredsCredential interface, this was just the object I saw after logging it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your interface contains the rev_reg and witness properties, but these are not in the AnonCredsCredential. Do you know if these properties are added when storing to the credential, or is the interface we have in AFJ incorrect (and we should add those keys to that interface)

yarn.lock Outdated Show resolved Hide resolved
@TimoGlastra TimoGlastra dismissed genaris’s stale review March 18, 2023 16:06

Initial version ready for mege

@TimoGlastra TimoGlastra enabled auto-merge (squash) March 18, 2023 16:06
@TimoGlastra TimoGlastra merged commit 4a6b99c into openwallet-foundation:main Mar 18, 2023
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