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

[FIX] Reduce unnecessary required deps #1217

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

gmulhearn-anonyome
Copy link
Contributor

@gmulhearn-anonyome gmulhearn-anonyome commented Jun 6, 2024

I noticed that after pulling in the newest version of aries-vcx into my project, that i started picking up vdrtools_wallet and vdrtools dependencies again.

This was mainly due to:

  • aries_vcx_anoncreds had default features of credx & vdrtools_wallet
  • aries_vcx depends on aries_vcx_anoncreds without disabling default features

there were also a few other unnecessary dependencies

solution:

  • removed default features of aries_vcx_anoncreds. other core crates like aries_vcx_wallet do not have default features; the default is that everything is disabled. I prefer it this way, as default features often get forgotten and lead to things being unnecessarily packed in
  • removed "vdrtools_wallet" & "askar_wallet" features of aries_vcx_anoncreds: it is not necessary for the crate to have these features. the crate only cares about "BaseWallet", not any specific BaseWallet implementations.
  • found an error that would happen if aries_vcx was compiled AND aries_vcx_wallet/vdrtools_wallet was NOT enabled: mapping_wallet.rs would not see the VcxWalletError::IndyApiError branch (since VcxWalletError::IndyApiError is only a branch if aries_vcx_wallet/vdrtools_wallet is enabled)
  • removed indy-vdr dependency from aries_vcx_wallet. it was unnecessary: only was used for base64. i replaced this with the normal base64 crate
  • moved did_resolver_sov to be a dev dependency of aries_vcx
  • moved aries_vcx_wallet to be a dev dep of did_resolver_sov

Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Signed-off-by: George Mulhearn <gmulhearn@anonyome.com>
Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

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

LGTM! Good reduction of unnecessary dependencies

removed default features of aries_vcx_anoncreds. other core crates like aries_vcx_wallet do not have default features; the default is that everything is disabled. I prefer it this way, as default features often get forgotten and lead to things being unnecessarily packed in

How do you feel about us recommending the Askar feature / newer Shared Components features in like the README documentation? Reason why I ask is that I can see that having defaults can lead to unnecessary packages, but I do think it's a good idea to encourage new users to use the newer packages. Also happy to chat about this on the community call if you think it's an involved discussion or related to our other discussion

@gmulhearn-anonyome gmulhearn-anonyome removed the request for review from Patrik-Stas June 6, 2024 06:28
@gmulhearn-anonyome
Copy link
Contributor Author

@JamesKEbert yea i do agree. let's discuss in the call. for consistency i removed the default here, but potentially we could add defaults to all and be more careful about our internal usage.

@gmulhearn gmulhearn merged commit d6bfca9 into hyperledger:main Jun 6, 2024
21 checks passed
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.

3 participants