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 Node.js and React Native wrapper #58

Merged
merged 6 commits into from Jan 13, 2023

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented Jan 12, 2023

closes #54
closes #28

  • Moved the wrapper which was complete but not merged yet for indy-shared-rs.
  • Renamed all instances of indy-credx to anoncreds
  • Interesting thing, I used the casing like Anoncreds and not AnonCreds like in AFJ. @TimoGlastra any issues with this?
  • Node.js tests are all succeeding next to one, which is on hold for a change in the FFI with regards to the new revocation status list.
  • React Native is renamed, but it does need some work with the ffi changes we made.
  • CI/CD will come once this is merged to keep this pr ""small"".

Signed-off-by: blu3beri <blu3beri@proton.me>
Signed-off-by: blu3beri <blu3beri@proton.me>
Signed-off-by: blu3beri <blu3beri@proton.me>
Signed-off-by: blu3beri <blu3beri@proton.me>
Signed-off-by: blu3beri <blu3beri@proton.me>
Signed-off-by: blu3beri <blu3beri@proton.me>
@berendsliedrecht berendsliedrecht changed the title Add Node.js and React Native wrapper. Add Node.js and React Native wrapper Jan 12, 2023
@TimoGlastra
Copy link
Member

Interesting thing, I used the casing like Anoncreds and not AnonCreds like in AFJ. @TimoGlastra any issues with this?

If there's a good reason for it, no issue with it. But consistency is always good I think

@@ -0,0 +1,26 @@
# anoncreds-nodejs
Copy link
Member

Choose a reason for hiding this comment

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

Will you do the renaming of the folders (to anoncreds-nodejs etc..) in a follow up PR?

We should make sure that change is also made in the askar repo

import { registerAnoncreds } from 'anoncreds-shared'
import { AnoncredsNodeJS } from 'anoncreds-nodejs'

registerAnoncreds({ anoncreds: AnoncredsNodeJS })
Copy link
Member

Choose a reason for hiding this comment

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

We should probalby update this to work the same as the indy vdr registration (for another PR, just so we don't forgot)

Comment on lines +55 to +56
// @ts-ignore
nativeAnoncreds.anoncreds_create_schema(name, version, issuerId, attributeNames, ret)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this ts-ignored? Is it temporary?

@@ -0,0 +1,674 @@
/* eslint-disable @typescript-eslint/ban-ts-comment */
Copy link
Member

Choose a reason for hiding this comment

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

:( temporary?

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 I believe there were some issues with ref-napi giving some "funky" types. Will do a cleanup PR after this one.


const attributeNames = StringListStruct({
count: Object.keys(options.attributeRawValues).length,
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Won't comment on the ts-ignores anymore. If temporary okay, but we should really avoid them. If there's a typing issue we should add a method that handles that issue and reuse that everywhere.

Comment on lines +62 to +73
anoncreds_create_credential_request: [
FFI_ERRORCODE,
[
FFI_STRING,
FFI_OBJECT_HANDLE,
FFI_OBJECT_HANDLE,
FFI_STRING,
FFI_OBJECT_HANDLE,
FFI_OBJECT_HANDLE_PTR,
FFI_OBJECT_HANDLE_PTR,
],
],
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a lot of work, but if we were to assign custom types based on the ffi types , it would be much easier which parameters should be passed.

E.g. let's say we have a method that takes a credentialDefinitionId. Currently it would be typed as:

  anoncreds_create_credential_request: [
    FFI_ERRORCODE,
    [
      FFI_STRING,
    ],
  ],

However we could also write it like this:

const FFI_CREDENTIAL_DEFINITION_ID = FFI_STRING

  anoncreds_create_credential_request: [
    FFI_ERRORCODE,
    [
      FFI_CREDENTIAL_DEFINITION_ID,
    ],
  ],

Just a thought, maybe impractical

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 I think I had that for a bit, but keeping that across all the shared components was a huuuuge amount of effort. It makes sense a lot for readability, but it will only be needed in the NodeJsAnoncreds package, so it was not worth the effort me.

@berendsliedrecht
Copy link
Contributor Author

I will merge this for now. All the issues you mentioned are noted as issues and will be resolved incrementally. This was supposed to be just a move, but I wanted to see if I could get the tests working as well.

@berendsliedrecht berendsliedrecht merged commit c757334 into hyperledger:main Jan 13, 2023
@berendsliedrecht berendsliedrecht deleted the move-js-wrapper branch January 13, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants