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: move ledger to a separate crate #1172

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Conversation

xprazak2
Copy link
Contributor

@xprazak2 xprazak2 commented Apr 4, 2024

Extracts ledger from aries_vcx_core and moves it to a separate crate.

@xprazak2 xprazak2 force-pushed the ledger-split-cp branch 2 times, most recently from 7a34c50 to 95a16e5 Compare April 4, 2024 10:41
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 129 lines in your changes are missing coverage. Please review.

Project coverage is 0.05%. Comparing base (d0ad7a6) to head (9c938e3).

Files Patch % Lines
aries/aries_vcx_ledger/src/errors/error.rs 0.00% 25 Missing ⚠️
...ies/aries_vcx_ledger/src/ledger/indy_vdr_ledger.rs 0.00% 22 Missing ⚠️
aries/aries_vcx/src/errors/mapping_ledger.rs 0.00% 17 Missing ⚠️
...ies/aries_vcx_ledger/src/errors/mapping_indyvdr.rs 0.00% 12 Missing ⚠️
aries/misc/test_utils/src/mockdata/mock_ledger.rs 0.00% 11 Missing ⚠️
...vcx/src/common/primitives/credential_definition.rs 0.00% 7 Missing ⚠️
...edger/src/errors/mapping_ledger_response_parser.rs 0.00% 6 Missing ⚠️
...ries/aries_vcx_ledger/src/errors/mapping_others.rs 0.00% 6 Missing ⚠️
aries/aries_vcx_ledger/src/ledger/common.rs 0.00% 5 Missing ⚠️
...es_vcx/src/common/proofs/prover/prover_internal.rs 0.00% 3 Missing ⚠️
... and 8 more
Additional details and impacted files
@@           Coverage Diff            @@
##            main   #1172      +/-   ##
========================================
- Coverage   0.05%   0.05%   -0.01%     
========================================
  Files        487     491       +4     
  Lines      23966   24002      +36     
  Branches    4409    4416       +7     
========================================
  Hits          13      13              
- Misses     23953   23989      +36     
Flag Coverage Δ
unittests-aries-vcx 0.05% <0.00%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xprazak2 xprazak2 force-pushed the ledger-split-cp branch 10 times, most recently from b7ec668 to 7ced0f0 Compare April 4, 2024 12:41
Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>
@@ -0,0 +1,49 @@
use indy_vdr::{
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this is reason why indy_vdr was introduced as dependency - but aries_vcx shouldn't know anything about the underlying ledger implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the mapping for VcxLedgerError in the file below suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is the reason why indy_vdr is now a dependency - it was moved from aries_vcx_core. There used to be VdrError -> AriesVcxCoreErrr -> AriesVcxError conversion path but because aries_vcx_core is going away, we need to change to a direct VdrError -> AriesVcxError mapping. Alternatively, we could create an error variant in VcxLedgerError for each VdrError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mapping from VcxLedgerError currently does not suffice, because VcxLedgerError does not create its own variant for each VdrError but rather wrapps it

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets do the changes necessary to shield off vcx from indy-vdr, be it creating/adjusting VcxLedgerError. There's not much code relying on specific ledger error kinds (afaik the only place where we directly deal with them is when we are creating credential definition, and we have some specific behaviour based on whether the same cred-definition already exists/not exist on ledger.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can merge after this is sorted


# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[features]
vdr_proxy_ledger = ["dep:indy-vdr-proxy-client"]
Copy link
Contributor

@Patrik-Stas Patrik-Stas Apr 4, 2024

Choose a reason for hiding this comment

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

Not actionable in this PR:

It would be ideal if we use vdr_proxy_ledger, we don't actually compile and bring in indy-vdr itself (as it brings/requires all the zmq stuff with it).

I suppose the reason why this can't be done now (having 2 separate feature vdr_proxy_ledger and vdr_zmq_ledger) is because even when you use proxy implementations, I guess we are still relying on some types defined indy indy-vdr (which carries the zmq stuff).

If you can confirm my suspition, there's potential refactoring task:
Refactor indy-vdr library such that it's broken down into

  • types crates
  • logic

So that we can use the types, given that's necessary regardless of what transport we use, but not necessary require zmq library installed on the system if the user only intends to use proxy implementation.

Copy link
Contributor

@Patrik-Stas Patrik-Stas left a comment

Choose a reason for hiding this comment

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

Left few comments, given these changes aries_vcx_core could be instead renamed to aries_vcx_anoncreds to keep consistent naming with these other extracted crates.

@xprazak2
Copy link
Contributor Author

xprazak2 commented Apr 4, 2024

Left few comments, given these changes aries_vcx_core could be instead renamed to aries_vcx_anoncreds to keep consistent naming with these other extracted crates.

That will be my next step.

Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>
@Patrik-Stas Patrik-Stas merged commit d0772a8 into main Apr 5, 2024
26 checks passed
@Patrik-Stas Patrik-Stas deleted the ledger-split-cp branch April 5, 2024 12:32
GHkrishna pushed a commit to GHkrishna/aries-vcx that referenced this pull request Apr 16, 2024
* refactor: move ledger to a separate crate

Signed-off-by: Ondrej Prazak <ondrej.prazak@absa.africa>
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