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

Split IndyVdrLedger and IndySdkLedger #862

Merged
merged 3 commits into from
May 26, 2023
Merged

Conversation

mirgee
Copy link
Contributor

@mirgee mirgee commented May 24, 2023

Splits

  • IndyVdrLedger -> IndyVdrLedgerRead & IndyVdrLedgerWrite, and
  • IndySdkLedger -> IndySdkLedgerRead & IndySdkLedgerWrite

This allows each implementation to use only what it needs (e.g. IndyVdrLedgerRead variant doesn't need RequestSigner, IndyVdrLedgerWrite doesn't need ResponseCacher and ResponseParser). Moreover, since both are initialized separately, a IndyLedgerRead implementation can be used to initialize a IndyLedgerWrite implementation.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

Merging #862 (497df30) into main (9f2e922) will decrease coverage by 0.69%.
The diff coverage is 37.27%.

@@            Coverage Diff             @@
##             main     #862      +/-   ##
==========================================
- Coverage   48.88%   48.20%   -0.69%     
==========================================
  Files         428      428              
  Lines       34134    34103      -31     
  Branches     7517     7502      -15     
==========================================
- Hits        16687    16439     -248     
- Misses      12294    12549     +255     
+ Partials     5153     5115      -38     
Flag Coverage Δ
unittests-aries-vcx 48.17% <37.27%> (-0.68%) ⬇️

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

Impacted Files Coverage Δ
...ries_vcx/src/utils/mockdata/profile/mock_ledger.rs 29.33% <0.00%> (ø)
aries_vcx_core/src/ledger/indy_vdr_ledger.rs 30.43% <24.69%> (-0.31%) ⬇️
aries_vcx_core/src/ledger/indy_ledger.rs 63.77% <44.44%> (-4.26%) ⬇️
aries_vcx/src/core/profile/modular_libs_profile.rs 82.00% <100.00%> (+1.14%) ⬆️
aries_vcx/src/core/profile/vdrtools_profile.rs 74.28% <100.00%> (-8.07%) ⬇️

... and 29 files with indirect coverage changes

@mirgee mirgee marked this pull request as ready for review May 24, 2023 14:06
@mirgee mirgee force-pushed the refactor/split-base-ledger branch from 763ae60 to 2804b55 Compare May 24, 2023 14:37
@mirgee mirgee force-pushed the refactor/split-indyvdr branch from 1a4e623 to d51c5f8 Compare May 24, 2023 14:39
@@ -6,22 +6,37 @@ use crate::{indy, PoolHandle, WalletHandle};
use super::base_ledger::{AnoncredsLedgerRead, AnoncredsLedgerWrite, IndyLedgerRead, IndyLedgerWrite};

#[derive(Debug)]
pub struct IndySdkLedger {
pub struct IndySdkLedgerRead {
indy_wallet_handle: WalletHandle,
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps can be removed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately get_ledger_txn still needs WalletHandle to sign the request if submitter_did is passed in - but perhaps we can omit this argument and not sign the request as with the other getters. However, I don't think there is much worth in optimizing IndySdkLedger as it is soon-to-be-deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not worth it doing it here, but how about taking a generic implementing BaseWallet along with the submitter_did? Something like Option(&str, &T) where T: BaseWallet. Shouldn't that remove the need to store the wallet handle while also making it easier to work with that scenario since the arguments are either provided together or not at all?

Patrik-Stas
Patrik-Stas previously approved these changes May 24, 2023
@Patrik-Stas Patrik-Stas requested a review from bobozaur May 24, 2023 15:16
Copy link
Contributor

@bobozaur bobozaur left a comment

Choose a reason for hiding this comment

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

I merely have an observation. Otherwise LGTM.

Base automatically changed from refactor/split-base-ledger to main May 25, 2023 10:55
mirgee added 3 commits May 25, 2023 13:07
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
@mirgee mirgee force-pushed the refactor/split-indyvdr branch from 4bee597 to 497df30 Compare May 25, 2023 11:07
@mirgee mirgee requested review from bobozaur and Patrik-Stas May 25, 2023 11:07
@Patrik-Stas Patrik-Stas merged commit a6ad85a into main May 26, 2023
@Patrik-Stas Patrik-Stas deleted the refactor/split-indyvdr branch May 26, 2023 10:48
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