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

Ledger response parser #851

Merged
merged 4 commits into from
May 19, 2023
Merged

Ledger response parser #851

merged 4 commits into from
May 19, 2023

Conversation

mirgee
Copy link
Contributor

@mirgee mirgee commented May 17, 2023

Extracts ledger response parsing capability into the indy-ledger-response-parser crate which

  • returns types from indy-data-types (reexported by indy-vdr) and
  • defines its own types (taken from libvdrtools) for ledger responses.

This crate is further integrated into IndyVdrLedger implementation to replace the current provisional constructions.

@mirgee mirgee added the skip-ci label May 17, 2023
@mirgee mirgee force-pushed the feature/ledger-indy branch 7 times, most recently from 2d4a86c to a48207f Compare May 17, 2023 11:55
@mirgee mirgee marked this pull request as ready for review May 17, 2023 11:56
Base automatically changed from feature/vdr-proxy-client to main May 17, 2023 12:01
@mirgee mirgee force-pushed the feature/ledger-indy branch 3 times, most recently from 72d824a to 49281f7 Compare May 17, 2023 12:52
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
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.

reviewed partially, will continue tonight/tomorrow

aries_vcx_core/src/errors/mapping_vdrtools.rs Show resolved Hide resolved
aries_vcx_core/src/errors/mod.rs Show resolved Hide resolved
aries_vcx_core/src/ledger/indy_vdr_ledger.rs Show resolved Hide resolved
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 some comments but nothing big, this is really great improvement

aries_vcx_core/src/ledger/indy_vdr_ledger.rs Show resolved Hide resolved
libvdrtools/indy-api-types/Cargo.toml Show resolved Hide resolved
libvdrtools/indy-api-types/Cargo.toml Show resolved Hide resolved
Patrik-Stas
Patrik-Stas previously approved these changes May 18, 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.

Looks good, but I think there are some improvements that can be made, especially regarding parse_message.

indy_ledger_response_parser/src/lib.rs Outdated Show resolved Hide resolved
indy_ledger_response_parser/src/lib.rs Outdated Show resolved Hide resolved
indy_ledger_response_parser/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2023

Codecov Report

Merging #851 (dcd220f) into main (0ef8430) will decrease coverage by 0.16%.
The diff coverage is 27.97%.

@@            Coverage Diff             @@
##             main     #851      +/-   ##
==========================================
- Coverage   48.95%   48.79%   -0.16%     
==========================================
  Files         415      423       +8     
  Lines       33784    34027     +243     
  Branches     7493     7510      +17     
==========================================
+ Hits        16539    16605      +66     
- Misses      12107    12285     +178     
+ Partials     5138     5137       -1     
Flag Coverage Δ
unittests-aries-vcx 48.76% <27.97%> (-0.16%) ⬇️

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

Impacted Files Coverage Δ
indy_ledger_response_parser/src/domain/attrib.rs 0.00% <0.00%> (ø)
indy_ledger_response_parser/src/domain/did.rs 0.00% <0.00%> (ø)
indy_ledger_response_parser/src/domain/rev_reg.rs 8.57% <8.57%> (ø)
indy_ledger_response_parser/src/domain/schema.rs 15.00% <15.00%> (ø)
indy_ledger_response_parser/src/domain/cred_def.rs 15.78% <15.78%> (ø)
indy_ledger_response_parser/src/domain/response.rs 25.00% <25.00%> (ø)
...y_ledger_response_parser/src/domain/rev_reg_def.rs 33.33% <33.33%> (ø)
aries_vcx_core/src/ledger/indy_vdr_ledger.rs 29.25% <38.70%> (+1.03%) ⬆️
indy_ledger_response_parser/src/lib.rs 40.32% <40.32%> (ø)
aries_vcx_core/src/errors/mapping_vdrtools.rs 32.35% <50.00%> (ø)
... and 1 more

... and 5 files with indirect coverage changes

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.

Sorry about the mistake on the suggestion. This adapts your changes further to get everything a bit more streamlined. Thanks for considering my suggestions nonetheless!

indy_ledger_response_parser/src/domain/response.rs Outdated Show resolved Hide resolved
indy_ledger_response_parser/src/lib.rs Outdated Show resolved Hide resolved
indy_ledger_response_parser/src/lib.rs Outdated Show resolved Hide resolved
@mirgee mirgee requested a review from bobozaur May 19, 2023 08:35
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.

Modified the last suggestion for the Message<T>, and also wondering about this Vec in ReplyV1. I'm curious of what your thoughts are on this.

indy_ledger_response_parser/src/domain/response.rs Outdated Show resolved Hide resolved
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
@mirgee mirgee requested a review from bobozaur May 19, 2023 10:25
bobozaur
bobozaur previously approved these changes May 19, 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.

LGTM

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
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.

LGTM

@Patrik-Stas Patrik-Stas merged commit e8233f4 into main May 19, 2023
41 checks passed
@Patrik-Stas Patrik-Stas deleted the feature/ledger-indy branch May 19, 2023 11:58
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.

None yet

4 participants