hyperledger
Remove in-memory wallet type and flatten wallet abstractions
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
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.
+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.
I don't use, or really understand the need for the in-memory wallet so I'm all for removing the complexity.
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.
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?
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?
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.
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.
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-anoncredsis 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.
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.
I'm going to work on this when I have time. Anything that removes unneeded complexity is worth the effort imo.
Going to close this. Wallet types aren't flattened yet but the in_memory wallet has been removed.