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

Inject only required components, not entire profiles #873

Merged
merged 13 commits into from
Jun 14, 2023

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Jun 6, 2023

Profile abstraction has been introduced to enable smooth transition from vdrtools to modular libs (credx/anoncreds, indy-vdr, askar). Profile represents basket of trait objects to perform variety of operations.

Previously Profile was widely present in aries-vcx codebase as a convenient way to access various lower-level APIs. However this was at the price of:

  • passing references to objects down the stack, even though they were never used,
  • requiring aries-vcx consumers to build entire "Profile", even though the APIs they've consequently used only used limited number of profile's components (eg, you don't need access to ledger in order to build credential proposal, for example)
  • forcing aries-vcx to store building blocks in particular way (Profiles) - but consuming applications might want to store/handle things differntly.

This PR modifies aries-vcx and aries-vcx-core method signatures to be unaware of Profile abstraction - the APIs now only speak in terms of individual components such as dyn BaseAnoncreds, dyn AnoncredsLedgerRead - so the APIs ask only for what they really need.
Additional extra benefit is that it's now much easier to spot fishy stuff, like "oh, this method requires access to ledger? That's weird, why is that???"

@Patrik-Stas Patrik-Stas force-pushed the change/remove-profiles branch 3 times, most recently from edbc042 to 0bf1f5d Compare June 6, 2023 14:02
@Patrik-Stas Patrik-Stas force-pushed the refactor/do-not-consume-profile branch from 8f2f01e to a525a5b Compare June 6, 2023 14:14
@Patrik-Stas Patrik-Stas force-pushed the change/remove-profiles branch from 0bf1f5d to 84cbb0f Compare June 6, 2023 14:33
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Merging #873 (5d213d5) into main (f17c05a) will increase coverage by 1.02%.
The diff coverage is 83.53%.

@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
+ Coverage   48.88%   49.90%   +1.02%     
==========================================
  Files         432      432              
  Lines       34337    35058     +721     
  Branches     7612     7608       -4     
==========================================
+ Hits        16784    17497     +713     
- Misses      12277    12284       +7     
- Partials     5276     5277       +1     
Flag Coverage Δ
unittests-aries-vcx 49.87% <83.53%> (+1.02%) ⬆️

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

Impacted Files Coverage Δ
...protocols/issuance/holder/states/offer_received.rs 23.52% <0.00%> (ø)
.../protocols/issuance/holder/states/proposal_sent.rs 62.50% <0.00%> (ø)
aries_vcx/src/protocols/issuance/mod.rs 15.00% <0.00%> (+0.71%) ⬆️
aries_vcx/src/common/proofs/prover/prover.rs 45.71% <33.33%> (ø)
aries_vcx/src/handlers/out_of_band/receiver.rs 25.16% <40.00%> (-0.17%) ⬇️
aries_vcx/src/protocols/connection/invitee/mod.rs 69.44% <50.00%> (ø)
...ies_vcx/src/common/primitives/credential_schema.rs 47.28% <55.55%> (+0.83%) ⬆️
aries_vcx/src/common/ledger/transactions.rs 42.99% <55.88%> (+1.08%) ⬆️
...vcx/src/handlers/connection/mediated_connection.rs 48.49% <56.25%> (-0.37%) ⬇️
aries_vcx/src/handlers/issuance/issuer.rs 53.99% <60.00%> (+0.65%) ⬆️
... and 30 more

... and 4 files with indirect coverage changes

@Patrik-Stas Patrik-Stas force-pushed the refactor/do-not-consume-profile branch 2 times, most recently from 1e13e27 to 81f1846 Compare June 7, 2023 09:38
@Patrik-Stas Patrik-Stas force-pushed the change/remove-profiles branch from 84cbb0f to 902638d Compare June 7, 2023 09:58
@Patrik-Stas Patrik-Stas force-pushed the change/remove-profiles branch from 902638d to 68fee40 Compare June 7, 2023 11:27
Base automatically changed from refactor/do-not-consume-profile to main June 7, 2023 11:29
@Patrik-Stas Patrik-Stas force-pushed the change/remove-profiles branch 3 times, most recently from 878d741 to 166c8fd Compare June 7, 2023 14:51
@Patrik-Stas Patrik-Stas force-pushed the change/remove-profiles branch 3 times, most recently from 7a653ac to 7461831 Compare June 8, 2023 15:27
@Patrik-Stas Patrik-Stas force-pushed the change/remove-profiles branch from 7461831 to b0ce032 Compare June 8, 2023 16:40
@Patrik-Stas Patrik-Stas requested review from mirgee and gmulhearn June 8, 2023 16:42
mirgee
mirgee previously approved these changes Jun 9, 2023
@Patrik-Stas
Copy link
Contributor Author

@gmulhearn if this is fine with you, feel free to merge

Big delta but trivial changes mostly

@Patrik-Stas Patrik-Stas requested a review from bobozaur June 13, 2023 13:30
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>
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>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@bobozaur bobozaur force-pushed the change/remove-profiles branch from 02ff8d5 to 5d213d5 Compare June 13, 2023 14:57
@bobozaur bobozaur merged commit fd0b8c7 into main Jun 14, 2023
@bobozaur bobozaur deleted the change/remove-profiles branch June 14, 2023 04:18
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.

4 participants