hyperledger / aries-askar

Secure storage designed for Hyperledger Aries agents.
Apache License 2.0
58 stars 44 forks source link

Working with CapacitorJs #208

Closed Sotatek-BaoHoanga closed 3 months ago

Sotatek-BaoHoanga commented 8 months ago

Aries Askar now exclusively works with Nodejs and React Native. Is there a way to modify or integrate it to work with CapacitorJs (https://capacitorjs.com/)?

TimoGlastra commented 8 months ago

It's definitely possible to support other environments, but you'd need to implement the AriesAskar interface defined in the shared package.

We do that for Node.JS and React Native. I'm not familiar with capacitorJS so not sure what that would involve, but I suspect creating a wrapper around the c callable binary for ios/android (there's a repo that is working on kotlin and swift wrappers: https://github.com/hyperledger/aries-uniffi-wrappers)

iFergal commented 7 months ago

@TimoGlastra I've implemented most of this with @bao-sotatek for Android. The problem we're facing now is some of the AriesAskar functions do not return promises.

For Capacitor, every native call must return a promise or you'll end up with a delayed response and undefined in JS. This seems unavoidable as Capacitor uses a WebView and the calls over this boundary are always async.

Would it be possible to convert all of these functions to async (+ in NodeJS/React Native wrappers), and add the necessary awaits in aries-askar-shared and the Askar module in AFJ? We intend to make this Askar Capacitor plugin fully open source once it's out of this "PoC stage".

If you are OK with this, I can fork the libraries to do this myself, and finish the development of the plugin (in case there are other issues) and then contribute it back. I think I'd need to stay on a fork for a while anyway since we are still using @aries-framework/askar@0.4.2 with aries-askar-shared@0.1.1, but could merge in for 0.5.0.

Thanks!

TimoGlastra commented 7 months ago

@berendsliedrecht what do you think?

I quite liked the API being sync now, but I also want the interfaces to allow for other integrations besides the two we have now.

I think we should make the methods all return Promise | T, so that you can still return a non-promise. Benefit is that we don't have to add a promise wrapper around everything and the code will still run as sync in Node.JS / React Native.

berendsliedrecht commented 7 months ago

Changing the API to make it Promise<T> | T will cause a major headache for both sides as you have to be aware of the environment you are running in and handle accordingly.

For a CapacitorJS wrapper, I would suggest a new repository that could reuse the binaries from this repository, but a fully separate wrapper.

iFergal commented 7 months ago

@berendsliedrecht If I have a fully separate wrapper, then I need to re-write and maintain a new @aries-framework/askar right? (...or adapt it to accept either wrapper, but this still means adding async there)

I'm not sure I get the headache though (maybe I'm missing something) - in aries-askar-shared, you just need to get Promise<T> | T down to T by awaiting, rather than checking environment. (and do the same in @aries-framework/askar

The awkward TS part is that get methods can't be have the async keyword, but can be worked around with something like:

public get algorithm() {
  return Promise.resolve(ariesAskar.keyGetAlgorithm({ localKeyHandle: this.handle })).then(keyAlgFromString);
}

...or simply having async getAlgorithm()

berendsliedrecht commented 7 months ago

if we resolve the Promise within aries-askar-shared we would have to make every function async, correct?

public get algorithm() {
  return Promise.resolve(ariesAskar.keyGetAlgorithm({ localKeyHandle: this.handle })).then(keyAlgFromString);
}

this call we would still have to await when called. We essentially chose to write all these modules in TurboModules in React Native so that we could have a mainly sync API, and to me this (not fully of course) gets rid of those benefits.

If I have a fully separate wrapper, then I need to re-write and maintain a new @aries-framework/askar right?

It would be Android + iOS plugin for CapacitorJS and a thin JS layer on top of that which can just be an async version of the functionality in aries-askar-shared.

IMO, all wrappers (node.js, js, python, etc.) should not live in this repository anyways and at some point will be moved out as well. So I am hesitant to add any wrapper for that matter.

iFergal commented 7 months ago

Yes, when called. Fair enough, I was not aware of the reasons for wanting to stick with a sync API. I could try and dig into Capacitor to see if there are "hacks" I can do to get around this but on the surface seems like it might not be possible.

It would be Android + iOS plugin for CapacitorJS and a thin JS layer on top of that which can just be an async version of the functionality in aries-askar-shared.

Except now @aries-framework/askar expects the sync version of aries-askar-shared. So asyncs need to be added there anyway. Unless I write an async version of @aries-framework/askar, not sure exactly how much this changes to keep maintained @TimoGlastra

TimoGlastra commented 7 months ago

I do think it would be good to allow for another platform to be supported in AFJ/Credo without having to recreate the Askar Shared interfaces, as well as the @credo-ts/askar package.

The whole reason to have the shared interface is that we didn't have to recreate indy-sdk-react-native poorly adapted based on indy-sdk api.

I think the get methods can be transformed to normal methods and optionally return a promise. I agree it's not optimal, but allowing a different environment to plug into the shared interfaces like that is good for adoption of Credo.

A good chunk of the calls are already async either way.

TimoGlastra commented 7 months ago

Changing the API to make it Promise | T will cause a major headache for both sides as you have to be aware of the environment you are running in and handle accordingly

That's not true, you just have to be aware of whether the method retirns a promise. If you want easy you always await, otherwise you check the return type.

All methods in Credo that call askar are already async, so we lose nothing by awaiting each call (you can await a non-promise and it won't add runtime overhead, only if you return Promise.resolve to make the return type async)

berendsliedrecht commented 7 months ago

That's not true, you just have to be aware of whether the method retirns a promise. If you want easy you always await, otherwise you check the return type.

That's exactly my point. You can always await everything or check the value, or environment, whether it is a promise and await or not.

All methods in Credo that call askar are already async, so we lose nothing by awaiting each call (you can await a non-promise and it won't add runtime overhead, only if you return Promise.resolve to make the return type async)

I am also looking at this from other usages next to Credo.

If there really is good value for it and it will stay maintained by the contributors, I am okay with it. But since the main maintainers of the nodejs and react native wrappers are not experienced with capacitor, I will see issues come up with out-of-sync implementations. Not to mention if this work will also apply to anoncreds and Indy vdr.

iFergal commented 7 months ago

We won't be using Indy VDR, and Anoncreds is not on our roadmap at present. So just Askar, but this important for us (Cardano Foundation) so we will maintain it.

If there's a concern with non-Credo usages - a possible middle ground might be a separate async shared library interface, but allow the Credo package to work with both async and sync implementations? (since Credo calls are async anyway)

iFergal commented 3 months ago

@berendsliedrecht After a lot of consideration, our wallet has recently dropped support from DIDs to focus our efforts in the KERI ecosystem. So this issue can be closed, as we no longer rely on Credo-TS.

We still have interest in Askar (subset of it, at least, just the encryption at rest), so we may end up writing some custom wrappers. Right now, we don't need it since most of our data is held in a KERI cloud agent. Thanks.