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 libvcx in 2 crates, update diagrams #759

Merged
merged 7 commits into from
Feb 28, 2023
Merged

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Feb 21, 2023

Split libvcx in 2 crates:

  • libvcx_core - the handle layer of libvcx extracted as separate crate
  • libvcx - this represents the same thing as libvcx used to be. Built on top of libvcx_core. This crate is now deprecated.

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2023

Codecov Report

Merging #759 (7ace634) into main (e9efacb) will increase coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #759      +/-   ##
==========================================
+ Coverage   54.40%   54.55%   +0.15%     
==========================================
  Files         381      380       -1     
  Lines       36766    36673      -93     
  Branches     8092     8065      -27     
==========================================
+ Hits        20002    20007       +5     
+ Misses      10807    10714      -93     
+ Partials     5957     5952       -5     
Flag Coverage Δ
unittests-aries-vcx 54.45% <ø> (+0.15%) ⬆️

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

Impacted Files Coverage Δ
agency_client/src/lib.rs 64.70% <0.00%> (-5.89%) ⬇️
wrappers/vcx-napi-rs/src/error.rs
...vcx/src/protocols/issuance/issuer/state_machine.rs 71.20% <0.00%> (+0.11%) ⬆️
libvdrtools/src/services/pool/pool.rs 47.67% <0.00%> (+0.19%) ⬆️
libvdrtools/src/services/pool/mod.rs 52.47% <0.00%> (+0.38%) ⬆️
libvdrtools/src/services/pool/request_handler.rs 45.27% <0.00%> (+0.50%) ⬆️
libvdrtools/src/controllers/non_secrets.rs 48.52% <0.00%> (+0.98%) ⬆️
aries_vcx/src/plugins/wallet/indy_wallet.rs 55.81% <0.00%> (+1.16%) ⬆️
aries_vcx/src/plugins/wallet/base_wallet.rs 57.14% <0.00%> (+14.28%) ⬆️

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

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>
@Patrik-Stas Patrik-Stas force-pushed the refactor/libvcx-split branch from 7fb7b6f to cb0408d Compare February 23, 2023 18:07
@Patrik-Stas Patrik-Stas changed the title Split libvcx in 2 crates Split libvcx in 2 crates, update diagrams Feb 23, 2023
bobozaur
bobozaur previously approved these changes Feb 24, 2023
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.

Apart from the tests thing (which would probably be handled separately), LGTM.

Comment on lines +341 to +345
#[cfg(feature = "test_utils")]
pub mod tests_utils {
pub const BAD_CREDENTIAL_OFFER: &str = r#"{"version": "0.1","to_did": "LtMgSjtFcyPwenK9SHCyb8","from_did": "LtMgSjtFcyPwenK9SHCyb8","claim": {"account_num": ["8BEaoLf8TBmK4BUyX8WWnA"],"name_on_account": ["Alice"]},"schema_seq_no": 48,"issuer_did": "Pd4fnFtRBcMKRVC2go5w3j","claim_name": "Account Certificate","claim_id": "3675417066","msg_ref_id": "ymy5nth"}"#;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not in the scope of this PR, as this spans across most the crates in the repository, but we should clear out these test-only stuff from the actual code.

Instead, we could provide something like common.rs (or common as a dir) in the integration tests folder where we can declare all the tests related stuff. Then mod common and use common::* would allow us to use all of these in integration tests without having to intertwine these things with the code.

Alternatively, if these need to be available for unit/doc tests, we could have separate crates for them directly. It would make the implementation crates cleaner and smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the test_utils under the /src are used for unit tests - though I am actually leaning to keep this as is, I am not strongly opinionated. I find it nice that the unit test utils for some domain are in respective files and right next to the tests which are using them the most.

We can discuss on other channels further

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas changed the base branch from main to ci/fix-docker-build February 24, 2023 13:34
Base automatically changed from ci/fix-docker-build to main February 24, 2023 15:47
@Patrik-Stas Patrik-Stas merged commit 22b6f47 into main Feb 28, 2023
@Patrik-Stas Patrik-Stas deleted the refactor/libvcx-split branch February 28, 2023 21:39
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