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

Refactor/do not consume profile #872

Merged
merged 2 commits into from
Jun 7, 2023
Merged

Conversation

Patrik-Stas
Copy link
Contributor

No description provided.

@Patrik-Stas Patrik-Stas changed the base branch from main to feature/set-taa-in-runtime June 6, 2023 11:52
@Patrik-Stas Patrik-Stas force-pushed the refactor/do-not-consume-profile branch 2 times, most recently from 8f2f01e to a525a5b Compare June 6, 2023 14:14
Base automatically changed from feature/set-taa-in-runtime to refactor/indyvdr-remove-global-state June 6, 2023 15:21
Base automatically changed from refactor/indyvdr-remove-global-state to main June 6, 2023 15:24
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas force-pushed the refactor/do-not-consume-profile branch from 1e13e27 to 81f1846 Compare June 7, 2023 09:38
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.

lgtm, but IMO now would probably be the best time to update all "clones" of Arc<dyn Profile>. As previously the code would call Arc::clone(&profile) whenever it needed to inject ledger/anoncreds.

e.g. in holder/state_machine.rs's _store_credential:

    let ledger = Arc::clone(profile).inject_anoncreds_ledger_read();
    let anoncreds = Arc::clone(profile).inject_anoncreds();

can now just be:

    let ledger = profile.inject_anoncreds_ledger_read();
    let anoncreds = profile.inject_anoncreds();

@codecov-commenter
Copy link

codecov-commenter commented Jun 7, 2023

Codecov Report

Merging #872 (81f1846) into main (aaa1e1a) will increase coverage by 37.35%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##            main     #872       +/-   ##
==========================================
+ Coverage   8.68%   46.04%   +37.35%     
==========================================
  Files        410      432       +22     
  Lines      32812    34324     +1512     
  Branches    7211     7609      +398     
==========================================
+ Hits        2849    15803    +12954     
+ Misses     29162    13521    -15641     
- Partials     801     5000     +4199     
Flag Coverage Δ
unittests-aries-vcx 46.01% <61.11%> (+37.33%) ⬆️

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

Impacted Files Coverage Δ
...ies_vcx/src/utils/mockdata/profile/mock_profile.rs 39.13% <33.33%> (ø)
aries_vcx/src/core/profile/vdrtools_profile.rs 65.00% <66.66%> (+30.00%) ⬆️
aries_vcx/src/core/profile/modular_libs_profile.rs 76.56% <83.33%> (ø)

... and 313 files with indirect coverage changes

@gmulhearn
Copy link
Contributor

^ i think if you do a search for Arc::clone in the codebase, pretty much every Arc::clone call is to clone a profile, which is now unnecessary.

could also probably argue that the signature could/should be updated to &Arc rather than Arc. Which could allow implementations of Profile to not have to Arc:;clone before returning..

e.g. :

    fn inject_indy_ledger_read(&self) -> &Arc<dyn IndyLedgerRead>;

    fn inject_indy_ledger_write(&self) -> &Arc<dyn IndyLedgerWrite>;

    fn inject_anoncreds(&self) -> &Arc<dyn BaseAnonCreds>;

    fn inject_anoncreds_ledger_read(&self) -> &Arc<dyn AnoncredsLedgerRead>;

    fn inject_anoncreds_ledger_write(&self) -> &Arc<dyn AnoncredsLedgerWrite>;

    fn inject_wallet(&self) -> &Arc<dyn BaseWallet>;

this would have no downside, as if the consumer really does need an Arc, then can always just Arc::clone over the &Arc.

But up to you whether you want to include that in this PR

@Patrik-Stas
Copy link
Contributor Author

@gmulhearn thanks for review and comments, however I'd like to suggest to push the suggested updates further down the road as this #873 is building on top of this PR and generally eliminate profiles from non-testing portion of aries-vcx codebase. Profiles will still exist post-#873 however, so we can apply your suggestion afterwards. Right now addressing your suggestion would require many changes which would be subsequently removed by 873.

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.

lgtm!

@gmulhearn
Copy link
Contributor

yea my bad, forgot about the profile removal work coming soon. approved!

@Patrik-Stas Patrik-Stas merged commit 015e036 into main Jun 7, 2023
@Patrik-Stas Patrik-Stas deleted the refactor/do-not-consume-profile branch June 7, 2023 11:29
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