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

Purge vdrtools #1013

Merged
merged 22 commits into from
Oct 12, 2023
Merged

Purge vdrtools #1013

merged 22 commits into from
Oct 12, 2023

Conversation

bobozaur
Copy link
Contributor

@bobozaur bobozaur commented Oct 6, 2023

Attempts to remove all unnecessary code from libvdrtools, keeping only the wallet and the anoncreds data structures definitions (for the wallet_migrator crate to use).

Additionally, removed lint exemption and fixed lints in the crate while also removing the migration and vdrtools/vdrtools_anoncreds feature flags from aries_vcx and aries_vcx_core.

Lastly, dependencies have been updated and tweaked. Of particular notice are sqlx which was bumped to the latest version (0.7.1) and zeroize which was bumped to the latest version (1.6.0).

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2023

Codecov Report

Merging #1013 (9793048) into main (603a26c) will increase coverage by 5.46%.
The diff coverage is 34.63%.

@@            Coverage Diff             @@
##             main    #1013      +/-   ##
==========================================
+ Coverage   30.49%   35.96%   +5.46%     
==========================================
  Files         419      372      -47     
  Lines       25967    21745    -4222     
  Branches     5015     4016     -999     
==========================================
- Hits         7918     7820      -98     
+ Misses      15869    11763    -4106     
+ Partials     2180     2162      -18     
Flag Coverage Δ
unittests-aries-vcx 35.96% <34.63%> (+5.46%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
aries_vcx/src/common/anoncreds.rs 82.88% <ø> (ø)
aries_vcx/src/common/keys.rs 50.00% <ø> (ø)
aries_vcx/src/common/mod.rs 50.00% <ø> (-4.02%) ⬇️
...s_vcx/src/common/primitives/revocation_registry.rs 52.52% <ø> (ø)
aries_vcx/src/core/profile/mod.rs 0.00% <ø> (ø)
aries_vcx/src/handlers/util.rs 15.12% <ø> (-0.67%) ⬇️
aries_vcx/tests/test_credential_issuance.rs 88.95% <ø> (ø)
aries_vcx/tests/test_credential_retrieval.rs 88.38% <100.00%> (+0.80%) ⬆️
aries_vcx/tests/test_pool.rs 85.11% <ø> (ø)
aries_vcx/tests/test_proof_presentation.rs 86.36% <ø> (ø)
... and 45 more

... and 6 files with indirect coverage changes

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

Having further look, I think there's more to be cleaned up

  • I think we should be able to drop off some cargo dependencies in vdrtools directories
  • there seems to be more stuff to delete in src/domain/anoncreds
  • I'd suggest to delete vdrtool2credx migration
    • this would get our workspace ursa-free, which if I remember right, was major source of pain with askar addtion and sqlx update
  • I believe signature_serializer.rs can be dropped and also Hash in indy-utils. Perhaps there's more stuff to delete in indy-utils. Openssl dependency can be removed.
  • mac.build.sh can be deleted

@bobozaur
Copy link
Contributor Author

bobozaur commented Oct 6, 2023

* I think we should be able to drop off some cargo dependencies in vdrtools directories

Aaah right, I forgot about the dependencies! I'll work on that.

* I'd suggest to delete `vdrtool2credx` migration

@Patrik-Stas Do you mean to delete the wallet_migrator crate?

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
@bobozaur
Copy link
Contributor Author

bobozaur commented Oct 6, 2023

@Patrik-Stas I removed some more code and updated/tweaked dependencies.

For the other things:

  • I don't think deleting the migration code is something we want. Is it? Down the line I see how we might want to incorporate the data structures from libvdrtools in there for posterity, but not really remove it.
  • There's definitely code to delete from src/domain/anoncreds but it really revolves around these same data structures used by the wallet_migrator and I wouldn't really start hunting down unused methods.

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
@Patrik-Stas
Copy link
Contributor

Patrik-Stas commented Oct 6, 2023

Do you mean to delete the wallet_migrator crate?

well, not necessarily the entire crate as it could be used for credx2anoncreds-rs or something similar, but given we have released 0.59.1 which provide migration path from vdrtools to credx, and we no longer support vdrtools, I don't see reason why to keep around the vdrtool2credx migration in particular, especially if it brings in all the ursa dependencies, that's quite heavy cost to pay.

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
@bobozaur
Copy link
Contributor Author

bobozaur commented Oct 9, 2023

Do you mean to delete the wallet_migrator crate?

well, not necessarily the entire crate as it could be used for credx2anoncreds-rs or something similar, but given we have released 0.59.1 which provide migration path from vdrtools to credx, and we no longer support vdrtools, I don't see reason why to keep around the vdrtool2credx migration in particular, especially if it brings in all the ursa dependencies, that's quite heavy cost to pay.

As discussed, we'll keep the data types around for a while longer just for the wallet_migrator crate. Perhaps we'll be able to move the data structures to wallet_migrator afterwards (or at least some high-level of them, without depending on ursa) and thus get rid of more things from libvdrtools.

Patrik-Stas
Patrik-Stas previously approved these changes Oct 9, 2023
mirgee
mirgee previously approved these changes Oct 9, 2023
@Patrik-Stas
Copy link
Contributor

@gmulhearn if all looks good, feel free to merge

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
@bobozaur bobozaur dismissed stale reviews from mirgee and Patrik-Stas via ba46d1e October 11, 2023 06:44
Patrik-Stas
Patrik-Stas previously approved these changes Oct 11, 2023
modular_libs = ["dep:indy-credx"]
vdr_proxy_ledger = ["modular_libs", "dep:indy-vdr-proxy-client"]
credx = ["dep:indy-credx"]
vdr_proxy_ledger = ["credx", "dep:indy-vdr-proxy-client"]
Copy link
Contributor

Choose a reason for hiding this comment

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

general question that doesn't need to be answered here: does vdr_proxy_ledger (i.e. where VdrProxySubmitter is the core component) actually need the credx feature?

You guys know this part of the code best, but it seems like the only reason vdr_proxy_ledger needs credx is because vdr_proxy_ledger is being used to hide the VdrProxyProfile implementation, which depends on IndyCredxAnonCreds (and IndySdkWallet too actually (vdrtools_wallet?)). I wonder if logically VdrProxyProfile should be hidden by credx AND vdr_proxy_ledger rather than making vdr_proxy_ledger always depend on credx

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's a matter of perspective about whether the primary "feature" of vdr_proxy_ledger is the VdrProxySubmitter AND VdrProxyProfile, or if it's just the VdrProxySubmitter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk. @mirgee worked on this. Any insights? I just replaced the underlying implementation being used in the VdrProxyProfile since we're deprecating libvdrtools.

Copy link
Contributor

Choose a reason for hiding this comment

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

ye no worries, just a passing comment

Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Copy link
Contributor

@gmulhearn gmulhearn left a comment

Choose a reason for hiding this comment

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

nice, good stuff!

@Patrik-Stas Patrik-Stas merged commit d63effe into main Oct 12, 2023
30 checks passed
@Patrik-Stas Patrik-Stas deleted the update/purge_vdrtools branch October 12, 2023 10:55
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.

6 participants