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

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

Open
dbluhm opened this issue Sep 19, 2024 · 12 comments
Open

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

dbluhm opened this issue Sep 19, 2024 · 12 comments
Assignees

Comments

@dbluhm
Copy link
Contributor

dbluhm commented Sep 19, 2024

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
Copy link
Contributor

WadeBarnes commented Sep 19, 2024

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
Copy link
Contributor

+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
Copy link
Contributor

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

@ianco
Copy link
Contributor

ianco commented Sep 19, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@dbluhm
Copy link
Contributor Author

dbluhm commented Sep 19, 2024

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
Copy link
Contributor Author

dbluhm commented Sep 19, 2024

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
Copy link
Contributor Author

dbluhm commented Sep 19, 2024

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.

@dbluhm dbluhm changed the title Thoughts on reducing some abstraction around the wallet interface Remove in-memory wallet type and flatten wallet abstractions Sep 19, 2024
@jamshale jamshale self-assigned this Oct 3, 2024
@jamshale
Copy link
Contributor

jamshale commented Oct 3, 2024

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

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

No branches or pull requests

6 participants