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 error handling #702

Merged
merged 35 commits into from
Dec 24, 2022
Merged

Refactor error handling #702

merged 35 commits into from
Dec 24, 2022

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Dec 20, 2022

Is addressing #582

changes:

  • Removed dead code
  • Previously there was VcxError defined in aries-vcx, this was used by both aries-vcx and libvcx. Because libvcx maps needs to map error types to u32, encoding them for FFI interface, this was part of aries-vcx - but obivously shouldn't. So to fix this:
    • aries-vcx still defined VcxError, but stripped of to minimum needed - mainly removed u32 mapping etc.
    • in libvcx was created module errors which defines LibvcxError which includes u32 mappings
    • to bridge VcxError with LibvcxError, there's mapping impl From<VcxErrorKind> for LibvcxErrorKind in mapping_from_ariesvcx.rs
  • In general, the errors.rs files has also been broken down into smaller pieces, typically into files such as errors/mapping_from_<crate> which maps errors from <crate> to domain errors of the given crate (such as mapping_from_ariesvcx.rs mentioned earlier)
  • All our crates now consistently contain module src/errors
  • Some methods in api_c contained more logic than they should - the logic has been moved to api_handle layer instead (for example the entire file api_handle/wallet.rs has been extracted from code in api_c and shifted layer down, to wallet.rs). This is good step to separate api_handle and api_c into separate crates Split libvcx crate #700
  • libvcx: change the way u32 error codes and LibvcxErrorKind are associated - there's static vector of (LibvcxErrorKind, u32) tuples. This way, adding new error code is minimal effort, as this would be the only single place where mapping is handled - as opposed to previously defined Kind->u32 and u32->Kind mapping declarations, which was quite error prone.
  • Additionally renamed all error and error kinds, so there is:
    • libvcx: ErrorLibvcx, ErrorLibvcx
    • aries-vcx: ErrorAriesVcx, ErrorKindAriesVcx
    • messages: ErrorMessages, ErrorKindMessages
    • agency client: ErrorAgencyClient, ErrorKindAgencyClient

@Patrik-Stas Patrik-Stas requested a review from a team as a code owner December 20, 2022 13:50
@Patrik-Stas Patrik-Stas changed the title Aries vcx/simplify err handling Refactor error handling Dec 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

Merging #702 (1d78ba5) into main (70ec685) will increase coverage by 0.72%.
The diff coverage is 33.02%.

@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
+ Coverage   63.43%   64.16%   +0.72%     
==========================================
  Files         234      239       +5     
  Lines       22961    22646     -315     
  Branches     5195     5085     -110     
==========================================
- Hits        14566    14531      -35     
+ Misses       4242     3972     -270     
+ Partials     4153     4143      -10     
Flag Coverage Δ
unittests-aries-vcx 64.02% <33.02%> (+0.72%) ⬆️

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

Impacted Files Coverage Δ
agency_client/src/api/agent.rs 34.78% <0.00%> (-2.43%) ⬇️
agency_client/src/api/downloaded_message.rs 43.47% <ø> (ø)
agency_client/src/api/messaging.rs 49.12% <ø> (ø)
agency_client/src/api/onboarding.rs 67.85% <ø> (ø)
agency_client/src/httpclient.rs 26.31% <ø> (ø)
agency_client/src/internal/messaging.rs 38.88% <ø> (ø)
agency_client/src/lib.rs 70.58% <ø> (ø)
agency_client/src/messages/create_key.rs 52.77% <0.00%> (ø)
agency_client/src/messages/forward.rs 46.66% <ø> (ø)
agency_client/src/messages/get_messages.rs 65.95% <ø> (ø)
... and 131 more

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

Base automatically changed from messages/simplify-crate to main December 20, 2022 22:51
@Patrik-Stas
Copy link
Contributor Author

As the previous PR got merged and squashed at merge, this created bunch of merge conflicts, I'll probably squash this entire PR after review as well, to get around it.

Signed-off-by: Patrik Stas <[email protected]>
Signed-off-by: Patrik Stas <[email protected]>
Signed-off-by: Patrik Stas <[email protected]>
Signed-off-by: Patrik Stas <[email protected]>
Signed-off-by: Patrik Stas <[email protected]>
Signed-off-by: Patrik Stas <[email protected]>
Signed-off-by: Patrik Stas <[email protected]>
@Patrik-Stas Patrik-Stas force-pushed the aries-vcx/simplify-err-handling branch from 61380f2 to 3d2ab04 Compare December 21, 2022 07:58
@Patrik-Stas Patrik-Stas linked an issue Dec 21, 2022 that may be closed by this pull request
@gmulhearn
Copy link
Contributor

Not sure if i'll have time for review tonight sorry - hopefully tomorrow. But just had an initial look - was wondering, what inspired the change from XYZError to ErrorXYZ?

My understanding was that XYZError was the norm in rust crates, (if not just calling it Error (i.e. aries_vcx::Error))

@Patrik-Stas
Copy link
Contributor Author

Patrik-Stas commented Dec 21, 2022

@gmulhearn good point! I didn't realize there was this convention but looking at bunch of crates, you are right. I'll keep the original naming style

Copy link
Contributor

@gmulhearn gmulhearn left a comment

Choose a reason for hiding this comment

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

Nice refactor! looks good, just left a few comments.

All the libvcx error handling makes me wish we had something like uniffi... ;). UniFFI lets you pass thiserror's thru the layer and they're automatically transformed into kotlin/swift exceptions and errors, maintaining the types and comments.

I'm tempted to try make a POC of uniffi with aries-vcx to demostrate

aries_vcx/src/errors/error.rs Outdated Show resolved Hide resolved
aries_vcx/src/errors/error.rs Outdated Show resolved Hide resolved
aries_vcx/src/errors/mapping_messages.rs Outdated Show resolved Hide resolved
aries_vcx/src/errors/mod.rs Show resolved Hide resolved
libvcx/src/api_lib/api_c/issuer_credential.rs Show resolved Hide resolved
libvcx/src/api_lib/api_handle/wallet.rs Outdated Show resolved Hide resolved
libvcx/src/api_lib/errors/error.rs Show resolved Hide resolved
libvcx/src/api_lib/errors/error.rs Show resolved Hide resolved
@Patrik-Stas
Copy link
Contributor Author

Patrik-Stas commented Dec 23, 2022

Nice refactor! looks good, just left a few comments.
All the libvcx error handling makes me wish we had something like uniffi... ;). UniFFI lets you pass thiserror's thru the layer and they're automatically transformed into kotlin/swift exceptions and errors, maintaining the types and comments.
I'm tempted to try make a POC of uniffi with aries-vcx to demostrate

Thanks for review, good caches! All comments addressed. (eh, I need to push that commit, will do that in an hour or two)

I'd be very cool to see uniffi in action. So far we dabbed into the napi-rs only. How would you go about working with uniffi? Would you create a crate on top of aries-vcx which would deal with uniffi? Or would you try to hook it directly into aries-vcx?

@Patrik-Stas Patrik-Stas merged commit dedb0e4 into main Dec 24, 2022
@Patrik-Stas Patrik-Stas deleted the aries-vcx/simplify-err-handling branch December 24, 2022 09:15
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.

Refactor errors
3 participants