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

DID Exchange Protocol #928

Merged
merged 2 commits into from
Nov 20, 2023
Merged

DID Exchange Protocol #928

merged 2 commits into from
Nov 20, 2023

Conversation

mirgee
Copy link
Contributor

@mirgee mirgee commented Aug 10, 2023

Initial version of DID Exchange protocol implementation with support for peer DIDs (peer:did:2 for now), new DidDocument crate, and JWS signing / verification.

The primary aim is not to achieve perfection at this stage but to roll out a first iteration prototype.

Correctness has been tested (so far only) using AATH tests running against AcaPy peer:did branch (or against aries-vcx itself), where all didx tests are passing whether the requester or responder role is assumed by either side.

Additionally

@mirgee mirgee force-pushed the feature/did-exchange branch 3 times, most recently from 7cfce73 to d5efffc Compare August 11, 2023 08:26
@mirgee mirgee force-pushed the refactor/did-resolver-registry branch 2 times, most recently from d89fbf4 to a789199 Compare August 11, 2023 08:31
Base automatically changed from refactor/did-resolver-registry to main August 11, 2023 10:19
@mirgee mirgee force-pushed the feature/did-exchange branch from d5efffc to d543a35 Compare August 11, 2023 13:56
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.

had a chance to have a short look only so far, but left some comment and thouhts

@Patrik-Stas Patrik-Stas mentioned this pull request Aug 14, 2023
mirgee added a commit that referenced this pull request Aug 16, 2023
* Extract subset of changes made in didx PR

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>

* Address code review

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>

---------

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
Completed(DidExchangeServiceResponder<Completed>),
}

impl GenericDidExchange {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we split this into

GenericDidExchangeRequester
GenericDidExchangeResponder

The value proposition of the generic layer is that you don't have to care about what state the you are currently in. In current state, it technically allows you to be even more agnostic - not only in terms of states, but also in terms of roles. But in practice, that property would never be used.

If we proceed with proposed split, there's hardly any practical disadvantage for consumers, but code will be a lot simpler, as we get rid of the outer level of match statements.

And basically, you will end up with place where you actually don't have to implement handle_response for GenericDidExchangeResponder at all, because it doesn't make sense.

This will have some implications, because on libvcx layer we've previously never distinguished between Inviter/Initee, but such approach was only used for connections, not other protocols. Since we are building on greenfield here, I think we should get rid of this discrepancy.

Copy link
Contributor Author

@mirgee mirgee Aug 17, 2023

Choose a reason for hiding this comment

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

I agree in principle with it not making sense in terms of methods shared between the roles, but this is a quick-and-dirty implementation and the current approach allows for braindead simple handling on the aries-vcx-agent level which was the entire point of the enum to begin with. (I have no issues with getting rid of the discrepancy, though.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, I'd like to go ahead with that

aries_vcx/src/protocols/did_exchange/service/mod.rs Outdated Show resolved Hide resolved
aries_vcx/src/protocols/did_exchange/service/mod.rs Outdated Show resolved Hide resolved
aries_vcx/tests/test_did_exchange.rs Show resolved Hide resolved
did_doc_sov/src/service/mod.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Attention: 979 lines in your changes are missing coverage. Please review.

Comparison is base (61b7cd1) 0.05% compared to head (875519b) 0.05%.

Files Patch % Lines
...rotocols/did_exchange/state_machine/generic/mod.rs 0.00% 130 Missing ⚠️
aries_vcx/src/utils/mod.rs 0.00% 114 Missing ⚠️
...rc/protocols/did_exchange/state_machine/helpers.rs 0.00% 111 Missing ⚠️
...ge/state_machine/requester/request_sent/helpers.rs 0.00% 75 Missing ⚠️
...hange/state_machine/responder/response_sent/mod.rs 0.00% 69 Missing ⚠️
aries_vcx/tests/test_did_exchange.rs 1.56% 63 Missing ⚠️
...change/state_machine/requester/request_sent/mod.rs 0.00% 56 Missing ⚠️
aries_vcx/src/protocols/oob/mod.rs 0.00% 55 Missing ⚠️
aries_vcx/src/protocols/did_exchange/mod.rs 0.00% 40 Missing ⚠️
aries_vcx/src/protocols/connection/invitee/mod.rs 0.00% 38 Missing ⚠️
... and 39 more
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #928      +/-   ##
========================================
- Coverage   0.05%   0.05%   -0.01%     
========================================
  Files        388     471      +83     
  Lines      21076   24009    +2933     
  Branches    3871    4307     +436     
========================================
+ Hits          12      13       +1     
- Misses     21063   23995    +2932     
  Partials       1       1              
Flag Coverage Δ
unittests-aries-vcx 0.05% <0.20%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mirgee mirgee force-pushed the feature/did-exchange branch from 4a046d7 to 5ec1e5d Compare August 23, 2023 15:22
@mirgee
Copy link
Contributor Author

mirgee commented Aug 25, 2023

@gmulhearn @bobozaur I don't know why @Patrik-Stas requested a review, this PR is not ready for review.

The code is relatively independent on the rest of the codebase so there is a limited value in merging this prematurely, before the implementation is decent, signature conventions are defined, the other implementations are flashed out and tested against...

@mirgee mirgee mentioned this pull request Sep 26, 2023
2 tasks
@Patrik-Stas Patrik-Stas force-pushed the feature/did-exchange branch 3 times, most recently from a821d0e to e5eb72e Compare November 6, 2023 22:26
Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
@mirgee mirgee marked this pull request as ready for review November 20, 2023 10:27
@mirgee
Copy link
Contributor Author

mirgee commented Nov 20, 2023

We should be able to group the messages crates after this is merged

* Restore did-exchange
* Address code review

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@Patrik-Stas Patrik-Stas merged commit 35476b3 into main Nov 20, 2023
28 checks passed
@Patrik-Stas Patrik-Stas deleted the feature/did-exchange branch November 20, 2023 13:30
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.

3 participants