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

Modularize dependency on vdrtools/indy with option for indy-Vdr and Indy-Credx in place #648

Merged
merged 2 commits into from
Dec 5, 2022

Conversation

gmulhearn-anonyome
Copy link
Contributor

@gmulhearn-anonyome gmulhearn-anonyome commented Nov 15, 2022

Summary of Changes

  • Created Profile, BaseWallet, BaseLedger, and BaseAnoncreds traits/interfaces. Where Wallet Ledger and Anoncreds APIs signatures are based mostly on indy/vdrtools APIs for the respective sections. A Profile represents a collection of these 3 traits/dependencies bundled together - Profile is derived from ACA-py's approach.
  • Created "IndySdk" (ACA-py terms) implementations of Wallet/Ledger/Anoncreds - which mostly just calls the /indy directory functionality directly.
  • Created an IndySdkProfile implementation which bundles together those 3 Indy impls ^.
  • Created IndyVdr implementation of Ledger, which uses indy-vdr for ledger request/response formation and sending. The profile's wallet is used for signing transactions. The implementation of most functions is derived from ACA-py.
  • Created IndyCredx implementation of Anoncreds, which uses indy-credx for the formation of anoncreds related objects - e.g. creating proofs and link secrets. The profile's wallet is used for storing/searching credentials and handling master secrets. The implementation of most functions is derived from ACA-py, especially around the storage of creds as wallet records.
  • Created a ModularWalletProfile which bundles vdr Ledger and credx Anoncreds with any user provided BaseWallet (I.e. a pluggable wallet). This is similar to ACA-py's AskarProfile - which we may wish to impl in the future.
  • Updated integration tests (where possible) to dynamically pick between using an IndySdk or ModularWallet Profile (by setting the modular_deps flag)
  • Created a MockProfile which is used by unit tests. Currently MockProfile contains Mock[Ledger|Wallet|Anoncreds], all of which return the mock data which you'd expect to be returned if indy_mocks/did_mocks were enabled.
  • Add a new CI step to rerun integration tests with coverage using the 'modular_profile' flag, which will trigger ~50 int tests to run using the modularWalletProfile instead of IndySdkProfile. (note that in the future ALL tests should be integration tests should be triggered by this flag - currently only 50 due to missing implementations)

Future Work

  • IndyVdrLedger and IndyCredx implementations were made with a focus on Holder/Prover functionality. As such, some 'institutional/verifier/issuer' functionality has stubbed implementations and needs to be populated.
  • Libvcx would ideally support the ModularProfile, not just default to Indy. More thought and planning will be needed
  • May wish to move indy functions into the IndySdkWallet/Ledger/Anoncreds files directly, as the indy directory is somewhat thin now.
  • AskarWallet and AskarProfile implementation
  • May wish to consider creating interface functions for Wallet Management APIs (i.e. generic create/open/close/delete/export/etc APIs)
  • Consider a better mechanism for integration testing both versions of Profile (i.e. Modular and Indy), rather than relying on the CI running the job twice.
  • Unit testing on credx/indyvdr?
  • MockProfile may open up a good opportunity to make some mocking much easier. E.g. the injection logic could be built into the MockProfile methods rather than relying on global settings. e.g. mock_profile.set_verifier_verify_result(true), etc
  • switch all integration tests to optionally use the ModularWalletProfile
  • switch indy-credx to anoncreds-rs once it is released/stable https://github.com/hyperledger/anoncreds-rs

Concerns

  • The Credx implementation stores Link Secrets in the wallet as records with a "LINK SECRET" type. An external consumer could access these records, therefore leaking the Link Secret.

Architecture (Before (TOP) and After (BOTTOM))

VCXIndyDepsArchitecture drawio

@gmulhearn-anonyome gmulhearn-anonyome force-pushed the modular_indy_hl branch 2 times, most recently from b7aacb5 to 07c77bc Compare November 17, 2022 08:30
@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2022

Codecov Report

Merging #648 (8f9b864) into main (faca3ea) will decrease coverage by 1.82%.
The diff coverage is 63.14%.

@@            Coverage Diff             @@
##             main     #648      +/-   ##
==========================================
- Coverage   63.86%   62.04%   -1.83%     
==========================================
  Files         214      237      +23     
  Lines       21017    22873    +1856     
  Branches     4706     5145     +439     
==========================================
+ Hits        13423    14192     +769     
- Misses       3843     4551     +708     
- Partials     3751     4130     +379     
Flag Coverage Δ
unittests-aries-vcx 61.91% <63.00%> (-1.76%) ⬇️

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

Impacted Files Coverage Δ
agency_client/src/error.rs 17.07% <ø> (+1.16%) ⬆️
agency_client/src/lib.rs 64.70% <ø> (ø)
aries_vcx/src/common/credentials/encoding.rs 72.22% <ø> (ø)
aries_vcx/src/common/mod.rs 0.00% <0.00%> (ø)
...es_vcx/src/common/proofs/proof_request_internal.rs 24.00% <ø> (ø)
aries_vcx/src/error.rs 16.09% <0.00%> (-1.01%) ⬇️
aries_vcx/src/indy/ledger/pool.rs 50.57% <0.00%> (ø)
aries_vcx/src/indy/primitives/mod.rs 57.14% <ø> (-22.30%) ⬇️
...ies_vcx/src/indy/primitives/revocation_registry.rs 65.49% <0.00%> (+8.43%) ⬆️
aries_vcx/src/indy/proofs/prover/prover.rs 51.54% <0.00%> (-7.33%) ⬇️
... and 118 more

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

@gmulhearn-anonyome gmulhearn-anonyome force-pushed the modular_indy_hl branch 5 times, most recently from cda099f to 3082c4d Compare November 29, 2022 01:23
Copy link
Contributor Author

@gmulhearn-anonyome gmulhearn-anonyome left a comment

Choose a reason for hiding this comment

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

issue: test-android-build failure in cargo test is not triggering a failure of the pipeline

.vscode/settings.json Outdated Show resolved Hide resolved
aries_vcx/Cargo.toml Outdated Show resolved Hide resolved
@gmulhearn-anonyome gmulhearn-anonyome changed the title Draft: Modularize dependency on vdrtools/indy Modularize dependency on vdrtools/indy Nov 30, 2022
@gmulhearn-anonyome gmulhearn-anonyome changed the title Modularize dependency on vdrtools/indy Modularize dependency on vdrtools/indy with option for indy-Vdr and Indy-Credx in place Nov 30, 2022
@gmulhearn-anonyome gmulhearn-anonyome marked this pull request as ready for review November 30, 2022 09:24
@gmulhearn-anonyome gmulhearn-anonyome requested a review from a team as a code owner November 30, 2022 09:24
aries_vcx/Cargo.toml Outdated Show resolved Hide resolved
@Patrik-Stas
Copy link
Contributor

  1. looks good to me, I've verified if some of the older PRs have been correctly merged/rebase into this and didn't find an issue
  2. please attach the diagram you were presenting on the aries-vcx call, it could help someone scanning through these changes
  3. please do, hopefully last, rebase, and let's get this merged 🚀

@gmulhearn-anonyome
Copy link
Contributor Author

Hi @guijd3p, cc @Patrik-Stas , i've rebased your most recent changes for supporting legacy vs non-legacy service endpoint writing and resolving.

I've had to move a few things around, you may want to double check that no regressions have been made - your tests still pass however.

Changes include:

  • moving the new write_endpoint, write_endpoint_legacy and get_service out of indy and into common/ledger/transactions.
  • Changing these methods to use a profile instead of indy handles.
  • Changing these methods to use the profile.ledger for adding and getting attributes, rather than depending on indy methods.
  • Added a 'clear attributes' step to your 2 tests - I was running into issues where running test_add_get_service_public before test_add_get_service would cause test_add_get_service to fail - as it would resolve the non-legacy service rather than legacy service (which the test is intended to target)

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

I've squashed commits on this branch for sake of the DCO - rebasing was a struggle.

However commit history can be viewed here: https://github.com/anonyome/aries-vcx/tree/modular_indy

Patrik-Stas
Patrik-Stas previously approved these changes Dec 2, 2022
@Patrik-Stas
Copy link
Contributor

I'll include this #683 in one more release 0.49.0 and then this PR will go to 0.50.0 today or over the weekend.
Nothing else to be merged in the meantime.

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
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