openwallet-foundation / acapy

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
408 stars 512 forks source link

Remove in-memory wallet type and flatten wallet abstractions #3243

Open dbluhm opened 2 weeks ago

dbluhm commented 2 weeks ago

ACA-Py started off built on the Indy SDK and then later transitioned to Aries Askar for the wallet. An in-memory wallet has also been supported for some time.

We recently dropped the Indy SDK, leaving just the in-memory wallet and Askar.

What do we think of dropping the in-memory wallet and leaning into Askar? Do we find value in continuing to leave the interface abstract?

It would take some effort but I think we would see a fairly significant simplification if we just committed to Askar.

@swcurran @ianco @PatStLouis @jamshale @TelegramSam

WadeBarnes commented 2 weeks ago

I'm all for simplification. I like the idea of dropping the in-memory wallet, I've actually seen it cause trouble for newcomers.

Askar itself is an interface/abstraction to secure storage. It makes sense to me to view and rely on it as such. Allowing the abstractions to be removed from ACA-Py to simplify the interface and integration.

PatStLouis commented 2 weeks ago

+1 for me, there are cases I found where both wallet types don't behave the same when instantiating a BaseWallet and it adds extra work to support both when adding new features. With this we could potentially drop other packages such as PyNaCl and have askar cover their features for cryptographic purposes.

jamshale commented 2 weeks ago

I don't use, or really understand the need for the in-memory wallet so I'm all for removing the complexity.

ianco commented 2 weeks ago

I think some tests use an in-memory wallet (where a wallet is needed) however if so they should be able to use sqlite, so I agree let's get rid of the in-memory wallet.

swcurran commented 2 weeks ago

Sounds good. Do we need to deprecate or just remove? Is it worth replacing it with SQLite In Memory? I had thought that was what the in-memory used — didn’t realize it was a separate implementation. Would Ask ar need to be updated for that?

PatStLouis commented 2 weeks ago

Most likely follow a deprecation path as we've seen with some of the routes / protocols.

What is the difference between askar and askar-anoncreds? could we see some alignment eventually? Ideally we can get rid of the wallet-type entirely and just use askar?

jamshale commented 2 weeks ago

askar-anoncreds is used to load different contexts. It decides whether to load the anoncreds api and modules. There's also some logic in credential and presentation handling that checks the wallet type. It still uses askar but some of the models stored in the wallet are different.

Eventually we'd want to only have the askar-anoncreds wallet type but that seems a long way off if it ever happens. It probably could have been done a different way than using wallet type but it works fine, other than being a bit confusing.

jamshale commented 2 weeks ago

I don't see why we can't just remove it?

dbluhm commented 2 weeks ago

I don't see why we can't just remove it?

Agreed; by definition, it can't be used in an environment that has any longevity to it. At most, it's used in testing. We can add a way for an in-memory sqlite db with Askar to be used instead if that wallet-type is selected or something.

Is it worth replacing it with SQLite In Memory? I had thought that was what the in-memory used — didn’t realize it was a separate implementation. Would Ask ar need to be updated for that?

Right, the Askar wallet does support doing an in-mem sqlite DB. You have to know the magic string to use in the wallet config, though.

dbluhm commented 2 weeks ago

What is the difference between askar and askar-anoncreds? could we see some alignment eventually? Ideally we can get rid of the wallet-type entirely and just use askar?

askar-anoncreds is used to load different contexts. It decides whether to load the anoncreds api and modules. There's also some logic in credential and presentation handling that checks the wallet type. It still uses askar but some of the models stored in the wallet are different.

We can persist the idea of selecting a wallet-type even while collapsing some of the abstractions in the background. The askar-anoncreds type is still an Askar wallet, of course, so it will behave the same the vast majority of the time.

dbluhm commented 2 weeks ago

Okay, I think we have a consensus. I'll reword the issue title to reflect that. I'll see where this fits into other planned and ongoing refactors.

jamshale commented 6 days ago

I'm going to work on this when I have time. Anything that removes unneeded complexity is worth the effort imo.