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

Feature/msg builders #972

Merged
merged 31 commits into from
Sep 13, 2023
Merged

Feature/msg builders #972

merged 31 commits into from
Sep 13, 2023

Conversation

bobozaur
Copy link
Contributor

@bobozaur bobozaur commented Sep 8, 2023

This PR introduces message builders through the typed-builder crate. The builder are based on the typestate pattern, but the crate is (at least advertised) to be fully compatible/replaceable by the derive-builder trait, which is using runtime construction of the type and allows for more flexibility (like passing builders around).

The typestate vs runtime construction decision will essentially depend on whether consumers want/need to build messages themselves and how convoluted these processes usually get. We don't really have to decide now, but it's a good thing to consider in the future.

For consistency reasons even some single fielded protocol specific messages contents implement builders, so that it is convenient, uniform and easy to know how one would go about composing a message.

There are some inner types which also implement the builders, yet for some types it is indeed quite the overkill (like CredentialPreview). I might revisit such types and just implement plain old new() constructors.

Another change that this PR brings is a uniformization of the Invitation message from the Connection protocol, so that the same decorators are used regardless of invitation type. The rationale is that it's much easier to reason with the invitation this way and since the protocol is old and won't be suffering changes it should be fine to simply deal with it like this.

Also, this addresses #949 .

@bobozaur bobozaur force-pushed the feature/msg_builders branch 2 times, most recently from f891840 to 4fc7631 Compare September 8, 2023 21:33
@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2023

Codecov Report

Merging #972 (66eaf07) into main (7558c3d) will decrease coverage by 0.54%.
The diff coverage is 42.74%.

@@            Coverage Diff             @@
##             main     #972      +/-   ##
==========================================
- Coverage   29.76%   29.23%   -0.54%     
==========================================
  Files         411      414       +3     
  Lines       26070    26246     +176     
  Branches     5315     5365      +50     
==========================================
- Hits         7760     7672      -88     
- Misses      16001    16263     +262     
- Partials     2309     2311       +2     
Flag Coverage Δ
unittests-aries-vcx 29.23% <42.74%> (-0.54%) ⬇️

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

Files Changed Coverage Δ
...vcx/src/handlers/connection/mediated_connection.rs 0.00% <0.00%> (ø)
aries_vcx/src/handlers/discovery/mod.rs 0.00% <ø> (ø)
aries_vcx/src/handlers/issuance/holder.rs 42.13% <0.00%> (-0.54%) ⬇️
aries_vcx/src/handlers/out_of_band/receiver.rs 0.00% <0.00%> (ø)
...ries_vcx/src/handlers/proof_presentation/prover.rs 38.59% <0.00%> (+2.53%) ⬆️
...es_vcx/src/handlers/proof_presentation/verifier.rs 39.37% <0.00%> (+0.11%) ⬆️
aries_vcx/src/protocols/connection/generic/mod.rs 9.30% <0.00%> (-0.30%) ⬇️
aries_vcx/src/protocols/connection/mod.rs 4.54% <0.00%> (-0.07%) ⬇️
...ocols/issuance/issuer/states/requested_received.rs 37.50% <ø> (ø)
...ocols/mediated_connection/invitee/state_machine.rs 0.00% <0.00%> (ø)
... and 67 more

... and 5 files with indirect coverage changes

Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Signed-off-by: Bogdan Mircea <[email protected]>
Patrik-Stas
Patrik-Stas previously approved these changes Sep 12, 2023
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.

This is great! Looks a lot cleaner, much easier to read. Very nice 👍

mirgee
mirgee previously approved these changes Sep 13, 2023
Copy link
Contributor

@mirgee mirgee left a comment

Choose a reason for hiding this comment

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

Love it! Makes message construction a breeze - so much easier on the eyes, the programmer is easily guided by the code completion now and I like that it is guaranteed that all mandatory fields are set at compile time 👍

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.

Very epic! Cannot wait to use this

@Patrik-Stas Patrik-Stas merged commit f4c8642 into main Sep 13, 2023
30 checks passed
@Patrik-Stas Patrik-Stas deleted the feature/msg_builders branch September 13, 2023 10:22
bobozaur added a commit that referenced this pull request Sep 14, 2023
* Added builders for messages crate

---------

Signed-off-by: Bogdan Mircea <[email protected]>
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.

5 participants