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

Profiles refactoring #785

Merged
merged 7 commits into from
Mar 29, 2023
Merged

Profiles refactoring #785

merged 7 commits into from
Mar 29, 2023

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Mar 28, 2023

  • renamed modular_wallet_profile to modular_libs_profile - it seem confusing to mention "wallet" while the profile encapsulates also credex, ledger client

  • Modified IndyVdrLedger to store require and store minimal amount of information needed - it doesn't need entire profile, it doesn't need to know about exists of Profiles, it only needs dyn BaseWallet for transaction signing

  • Similarly modified vdr-tools trait implementations to be unaware of Profile, but inject wallet_handle, pool_handle in them instead

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas requested a review from gmulhearn March 28, 2023 13:35
@Patrik-Stas Patrik-Stas changed the title Minor refactoring around profiles Profiles refactoring Mar 28, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2023

Codecov Report

Merging #785 (4789602) into main (945a67a) will decrease coverage by 0.01%.
The diff coverage is 74.33%.

@@            Coverage Diff             @@
##             main     #785      +/-   ##
==========================================
- Coverage   54.66%   54.66%   -0.01%     
==========================================
  Files         381      381              
  Lines       36858    36852       -6     
  Branches     8123     8136      +13     
==========================================
- Hits        20150    20144       -6     
- Misses      10706    10710       +4     
+ Partials     6002     5998       -4     
Flag Coverage Δ
unittests-aries-vcx 54.55% <74.33%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
aries_vcx/src/plugins/ledger/indy_vdr_ledger.rs 43.13% <33.33%> (-0.96%) ⬇️
aries_vcx/src/plugins/ledger/indy_ledger.rs 61.59% <71.05%> (-3.04%) ⬇️
aries_vcx/src/plugins/anoncreds/credx_anoncreds.rs 59.60% <75.00%> (-0.14%) ⬇️
aries_vcx/src/plugins/anoncreds/indy_anoncreds.rs 79.02% <78.94%> (+1.16%) ⬆️
aries_vcx/src/core/profile/modular_libs_profile.rs 78.94% <80.00%> (ø)
aries_vcx/src/core/profile/vdrtools_profile.rs 83.33% <83.33%> (ø)
aries_vcx/src/common/test_utils.rs 95.01% <100.00%> (ø)
...es_vcx/src/handlers/proof_presentation/verifier.rs 58.43% <100.00%> (ø)
aries_vcx/src/utils/devsetup.rs 71.20% <100.00%> (ø)
aries_vcx/tests/utils/devsetup_agent.rs 73.36% <100.00%> (ø)

... and 11 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
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 refactor, will make profiles cleaner to use!
Looks good.

If you wanna take it another step, you could now change the signature of the Profile trait params to &self rather than Arc. And then consequently, you could remove ALOT of Arc::clones of the Profile around the entire codebase.

@mirgee mirgee merged commit 9b9969b into main Mar 29, 2023
@mirgee mirgee deleted the refactor/profiles branch March 29, 2023 13:08
Andy-Waine pushed a commit to Andy-Waine/aries-vcx that referenced this pull request Apr 13, 2023
* Minor refactoring around profiles

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Inject BaseWallet instead of Profile into IndyCredxAnonCreds

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Refactor ModularLibsProfile

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* cargo fmt

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Refactor IndyProfile

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Rename IndysdkProfile to VdrtoolsProfile

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

* Fix test compile err

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

---------

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Andy Waine <88730354+Andy-Waine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants