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 base wallet type must be new wallet type restriction #3542

Merged
merged 4 commits into from
Feb 26, 2025

Conversation

jamshale
Copy link
Contributor

This allows a askar-anoncreds type subwallet to be created when the base admin wallet remains askar type. As discussed briefly in #3513, the base wallet isn't required to be anoncreds because it never creates anoncreds objects.

The one reason why this would be useful is that it would allow the old askar wallet type context (indy) to not be loaded once all subwallets were askar-anoncreds. However, there is other less confusing ways to do this other than have this extra step required to create anoncreds wallets.

jamshale and others added 3 commits February 24, 2025 22:51
Signed-off-by: jamshale <jamiehalebc@gmail.com>
Signed-off-by: jamshale <jamiehalebc@gmail.com>
@jamshale jamshale requested a review from ff137 February 25, 2025 17:43
Copy link
Contributor

@ff137 ff137 left a comment

Choose a reason for hiding this comment

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

I'm happy with the change --

Haven't heard any comments about why not to change it, so maybe the proponents for keeping it missed the other discussion, and the PR can be open a bit longer for comment before change is applied.

If there aren't any advocates for keeping the existing behaviour, I guess it's good to go


To try steelman a reason for keeping it, it might be to prevent untested scenarios, since there are different combinations for base wallet and tenant wallet types, which may not be covered in e2e tests. So maybe the web exception is to guard against that, until tests are in place.

So maybe this should only be removed when there are tests involving base-wallet=askar & tenant-wallet=askar-anoncreds, and vice versa. But, either way, the restriction may as well be removed, and then test for and fix the known errors that come up after the fact. Up to the maintainers.

(again, I'm giving the perspective of someone who doesn't know the design decision to configure "anoncreds" functionality as a "wallet-type" to begin with - so I'd like to raise the potential for refactoring that - maybe making it a different, extra setting - but I don't know enough about the reasons for the decision. So I'm just thinking out loud, as an outside contributor. Main point I wanted to raise is more testing is always a good thing -- and clearing technical debt sometimes beats new features)

@jamshale
Copy link
Contributor Author

One idea for forcing it to be the same type was that for multitenancy we need to load both anoncreds and indy modules. But if all subwallets and the base-wallet were anoncreds and you couldn't create any askar type wallets you'd be able to only the anoncreds modules and the api would be a subset of what it is now with both versions of the create schema/cred_def/revocation objects.

@jamshale jamshale merged commit 6b228d7 into openwallet-foundation:main Feb 26, 2025
11 checks passed
ff137 pushed a commit to didx-xyz/acapy that referenced this pull request Feb 27, 2025
…et-foundation#3542)

* Remove base wallet type must be new wallet type restriction

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Remove invalid test

Signed-off-by: jamshale <jamiehalebc@gmail.com>

---------

Signed-off-by: jamshale <jamiehalebc@gmail.com>
ff137 pushed a commit to didx-xyz/acapy that referenced this pull request Feb 27, 2025
…et-foundation#3542)

* Remove base wallet type must be new wallet type restriction

Signed-off-by: jamshale <jamiehalebc@gmail.com>

* Remove invalid test

Signed-off-by: jamshale <jamiehalebc@gmail.com>

---------

Signed-off-by: jamshale <jamiehalebc@gmail.com>
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

Successfully merging this pull request may close these issues.

2 participants