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

Remove msg-sending IO for issuer and holder #946

Merged
merged 40 commits into from
Sep 4, 2023
Merged

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Aug 20, 2023

Strategy

  • This PR modifies Issuer/Holder handlers (and underlying ode) to remove any message sending. Instead, this layer only produces aries messages, but caller is responsibly for making sure the messages are sent to the respective protocol counterparty.
  • This unlocks possibility to rapidly simplify test setup for issuance/verification protocol tests, as we will be no longer forced to interact with mediator and setup didcomm connection prior to testing other protocols.
  • State machines will be generally well position for further transition to type-state pattern following DID Exchange Protocol #928 pattern where transitions function include "msg to be sent out" in return type.

Changes

  • For any user of aries-vcx or components built on top, there's breaking changes in terms of intermediate states. Some state were removed. See below.
  • For any direct aries-vcx user, there's breaking changes in terms of sending messages. You have to send replies to counterparty yourself.
    • This doesn't apply to libvcx and rust-agent built on top of aries-vcx, which preserve original behaviour and assure message sending themselves.
Issuer: Removed state OfferSent
  • Remove OfferSent state, keep only OfferSet; treat OfferSet as OfferSent
  • To keep Issuer dependant works, Issuer still has its original method send_credential_offer which unlike before, does not modify state. Hence existing code should still keep working.
Issuer: Removed state CredentialSent
  • Removed state CredentialSent, introduced state CredentialSet instead
  • The semantic reason between them is self-explaining, "set" doesn't imply the message has been sent
  • Structural reason between the 2 is CredentialSet has additional field msg_issue_credential: IssueCredential to decouple construction of credential from sending issue-credential msg

Holder: Removed state RequestSent, ProposalSent

  • Removed state RequestSent, introduced state RequestSet instead
  • Removed state ProposalSent, introduced state ProposalSet instead
  • Generally changes and reasoning are analogous to what's described above for Issuer
Holder: credential-ack
  • Upon processing issue-credential, sender would previously automatically send credential-ack message, if ack was requested by the issuer. This no longer happens. Instead, after processing issue-credential msg, you should call get_final_message which can give you back aries message Option<AriesMessage> you shall send back to counterparty. The message might be credential-ack or problem-report (if processing issue-credential message failed)
Issuer & Holder: problem reports
  • For both Issuer and Holder, if a transition fails, the state machine moves to failed state like before, BUT: doesn't automatically sends the problem report. You have to check the state and send problem report yourself. You have to do that yourself.

@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2023

Codecov Report

Merging #946 (9c7b4f0) into main (429feca) will increase coverage by 0.09%.
The diff coverage is 61.58%.

@@            Coverage Diff             @@
##             main     #946      +/-   ##
==========================================
+ Coverage   39.23%   39.32%   +0.09%     
==========================================
  Files         415      414       -1     
  Lines       28867    28863       -4     
  Branches     6173     6183      +10     
==========================================
+ Hits        11325    11350      +25     
+ Misses      14202    14174      -28     
+ Partials     3340     3339       -1     
Flag Coverage Δ
unittests-aries-vcx 39.32% <61.58%> (+0.09%) ⬆️

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

Files Changed Coverage Δ
aries_vcx/src/handlers/issuance/mediated_holder.rs 29.16% <0.00%> (ø)
aries_vcx/src/handlers/issuance/mediated_issuer.rs 30.76% <0.00%> (-9.24%) ⬇️
aries_vcx/src/handlers/mod.rs 11.11% <0.00%> (+0.20%) ⬆️
...es_vcx/src/handlers/revocation_notification/mod.rs 44.44% <0.00%> (-8.89%) ⬇️
...protocols/issuance/holder/states/offer_received.rs 17.24% <ø> (-6.29%) ⬇️
...c/protocols/issuance/holder/states/proposal_set.rs 62.50% <ø> (ø)
...cx/src/protocols/issuance/issuer/states/initial.rs 100.00% <ø> (+87.50%) ⬆️
...tocols/issuance/issuer/states/proposal_received.rs 100.00% <ø> (+53.84%) ⬆️
...ocols/issuance/issuer/states/requested_received.rs 37.50% <0.00%> (ø)
...s_vcx/src/utils/mockdata/profile/mock_anoncreds.rs 0.00% <ø> (ø)
... and 13 more

... and 4 files with indirect coverage changes

@Patrik-Stas Patrik-Stas force-pushed the issuer/ioless branch 3 times, most recently from d60304a to 3577527 Compare August 20, 2023 12:45
@Patrik-Stas Patrik-Stas changed the title Make IO for issuer offer-sending opt-in Make IO for issuer and holder offer-sending opt-in Aug 20, 2023
@Patrik-Stas Patrik-Stas changed the title Make IO for issuer and holder offer-sending opt-in IO for issuer and holder as opt-in Aug 20, 2023
@Patrik-Stas Patrik-Stas marked this pull request as ready for review August 20, 2023 23:21
@Patrik-Stas Patrik-Stas force-pushed the issuer/ioless branch 2 times, most recently from 288c736 to e7e7f0c Compare August 22, 2023 12:11
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.

Seems OK to me so far. Left some comments outside of the review.

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.

Issuer: If constructing anoncreds credential / issue-credential message fails, the state machine transitions to failed Finished state, but doesn't sends the problem report. You have to check the state and send problem report yourself.

Holder: if construction anoncreds credential-request / request-credential message fails, the state machine transitions to failed Finished state, but doesn't sends the problem report. You have to check the state and send problem report yourself.

How and where is the potential problem report to be constructed? It is currently just not being sent (unless I am missing something)?

aries_vcx/src/protocols/issuance/holder/state_machine.rs Outdated Show resolved Hide resolved
aries_vcx/src/protocols/issuance/holder/state_machine.rs Outdated Show resolved Hide resolved
aries_vcx/src/handlers/mod.rs Show resolved Hide resolved
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>
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>
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>
…ential_request'; rename 'build_credential_request' to 'prepare_credential_request'

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>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
…ack whether ack was requested

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas changed the title IO for issuer and holder as opt-in Remove msg-sending IO for issuer and holder Sep 1, 2023
@Patrik-Stas Patrik-Stas merged commit 1c79a86 into main Sep 4, 2023
30 checks passed
@Patrik-Stas Patrik-Stas deleted the issuer/ioless branch September 4, 2023 08:20
bobozaur pushed a commit that referenced this pull request Sep 14, 2023
* Remove msg-sending IO for issuer and holder (#946)

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Bogdan Mircea <mirceapetrebogdan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants