Add indy-vdr-proxy profile

[mirgee]

Adds a new profile VdrProxyProfile which uses a running instance of indy-vdr-proxy (through indy-vdr-proxy-client) to read and write transactions instead of communicating with the ledger directly.

This may be useful in situations where

• a service is limited in its access to the public internet for security reasons or
• the client wants quick and easy access to a ledger without necessarily having to obtain the genesis file and set up and maintain their own ledger connection.

The VdrProxyProfile profile reuses existing IndyVdrLedger implementation of the BaseLedger trait and request submitting is injected through an implementation of the RequestSubmitter trait.

[Patrik-Stas]

Very cool 👍 but I've noticed you perhaps by mistake created PR against your own repo, rather against hyperledger origin mirgee/indy-vdr#1

[mirgee]

This is on purpose, I first want to make the maintainers are actually interested in the client before making a PR.

[Patrik-Stas]

Have you let them know somehow?

[mirgee]

In the end decided to open a PR against the Hyperledger repo. But IMO merging it should not be in any way a precondition for merging this PR.

[Patrik-Stas]

agreed

[gmulhearn-anonyome]

I agree, but it would be good to make a comment above this dependency; stating the changes you made and also that it is future TODO to monitor your indy-vdr PR and revert to using the main branch if/when your indy-vdr PR is merged

[mirgee]

I would assume forked dependencies are implicitly suspicious and to be avoided and that the changes made are easily traceable; but added the comment for clarity.

[codecov-commenter]

Codecov Report

Merging #837 (e84da36) into main (3072394) will decrease coverage by 40.31%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #837       +/-   ##
==========================================
- Coverage   49.01%   8.70%   -40.31%     
==========================================
  Files         414     408        -6     
  Lines       33704   32870      -834     
  Branches     7458    7199      -259     
==========================================
- Hits        16519    2862    -13657     
- Misses      12064   29197    +17133     
+ Partials     5121     811     -4310     

Flag	Coverage Δ
unittests-aries-vcx	8.70% <0.00%> (-40.28%)	⬇️

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

Impacted Files	Coverage Δ
aries_vcx/src/common/keys.rs	0.00% <ø> (-55.72%)	⬇️
aries_vcx/src/common/ledger/transactions.rs	6.19% <0.00%> (-35.92%)	⬇️
...vcx/src/common/primitives/credential_definition.rs	5.85% <0.00%> (-48.65%)	⬇️
aries_vcx/src/common/primitives/mod.rs	0.00% <0.00%> (-79.02%)	⬇️
aries_vcx/src/utils/devsetup.rs	30.93% <ø> (-36.79%)	⬇️
aries_vcx/tests/test_creds_proofs.rs	0.09% <ø> (-87.21%)	⬇️
aries_vcx/tests/utils/devsetup_agent.rs	0.00% <ø> (-72.24%)	⬇️

... and 298 files with indirect coverage changes

[Patrik-Stas]

Currently we have 3 different ledger errors in aries-vcx-core

   #[error("Connection to Pool Ledger.")]
    PoolLedgerConnect,
    #[error("Ledger rejected submitted request.")]
    InvalidLedgerResponse,
    #[error("Ledger item not found.")]
    LedgerItemNotFound,

when using VdrProxy ledger trait implementation, all errors are squashed into AriesVcxCoreErrorKind::InvalidLedgerResponse, - that's fine with the first two, but remapping LedgerItemNotFound into InvalidLedgerResponse can cause some troubles. For example function _try_get_cred_def_from_ledger is handling LedgerItemNotFound in very particular way.

It would be good if we could on level of indy-vdr-proxy-client distinquish and return "item not not found on ledger" vs "other issue with ledger response" errors, and that would enable us propagate more granular handling on aries-vcx level as well

[Patrik-Stas]

Just realized this is actually a problem of wider scope - vdr_ledger submitter, eg previously indy_vdr_ledger.rs also behaves differently

let send_req_result: VdrSendRequestResult = recv
            .await
            .map_err(|e| AriesVcxCoreError::from_msg(AriesVcxCoreErrorKind::InvalidState, e))?;
        let (result, _) = send_req_result?;

It actually returns even different error.

I think you can still line up the error handling as suggested in previous comment, but I think what needs to happen in the end is:

• take out the ledger trait as separate crate
• the trait needs to return custom error types - for example ErrorPoolLedgerConnect, ErrorInvalidLedgerResponse, ErrorLedgerItemNotFound
• Add some documentation to traits specifying what error needs to be returned in what case, ideally covered with some tests against the trait interface which any implementor of the trait could reuse (not sure how this would be practically done)

This should solve the deviations in error handling of the trait implementation, which now returns AriesVcxCoreError - way too much freedom for trait implementors.

[Patrik-Stas]

left some comments

[Patrik-Stas]

Per our discussion, I'll conclude things we discussed privately, for transparency and other reviewers

• there's lots of manual parsing going on here, because the indy-vdr doesn't contain model of ledger responses - the assumption is that indy-vdr has been typically used via FFI so far, so there wasn't value for implementors to maintained detailed types for ledger responses
• there are indy ledger responses in vdrtools under libvdrtools/src/domain/ledger/*.rs - we could reuse that, either pull it out from vdrtool as a small crate within aries-vcx repo, or even better make contribution to indy-vdr where we would map the ledger responses to those types.

But that would be scope for another PR(s)

[gmulhearn-anonyome]

code-coverage-aries-vcx-modular-dependencies-integration-tests is failing. I suspect because alot of SetupProfile::run_indy code has been changed to SetupProfile::run. where previously, SetupProfile::run_indy was being used to force modular_libs tests to avoid calling onto un-implemented modular_libs code (e.g. credx issuer functionality).

Would be ideal if code-coverage-aries-vcx-modular-dependencies-integration-tests can continue to run and pass. I'm not entirely sure how you could approach this to handle all 3 testing cases (vdrtools, modular_libs, vdr-proxy)..

[Patrik-Stas]

almost there

[gmulhearn-anonyome]

Looks really good, splitting the logic into a request_submitter was a good move.

Just noticed a couple of things:

1. vdr_reader.rs in did_resolver_sov may need to be updated to use the new request_submitter. It currently fails to build with cargo build --features modular_libs --no-default-features.
2. The vdr_proxy_profile technically depends on the vdrtools dependency/feature-flag. I think this is ok, but I wonder if the vdr_proxy_profile would therefore need to be to hidden behind the vdrtools flag as well? e.g. aries_vcx currently fails to build with cargo build --features vdr_proxy_ledger --no-default-features. But maybe it's that's ok and you can ignore this comment, but something to think about. I've been using the check-aries-vcx-feature-variants CI job to check that features can survive/compile on their own, if you think it's appropriate, you could add the vdr_proxy_ledger in there.

Also had that other comment about commenting on your usage of indy-vdr fork.

Feel free to merge after you've addressed/considered my review! thnx

[gmulhearn-anonyome]

I agree, but it would be good to make a comment above this dependency; stating the changes you made and also that it is future TODO to monitor your indy-vdr PR and revert to using the main branch if/when your indy-vdr PR is merged

[mirgee]

@gmulhearn-anonyome Good catch. Updated vdr_reader, temporarily made vdr_proxy_ledger dependent on vdrtools feature flag (until IndyCredxAnonCreds is fully implemented - we want to test issuer side with indy-vdr-proxy as well), and added a check under check-aries-vcx-feature-variants CI job.

[gmulhearn]

LGTM assuming CI still good