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

change: simplify didcore components, did-exchange protocol #1075

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Nov 29, 2023

The changes in this PR are primarily stemming by removing generics on DidDocument and Service data structures. This change trickles-up significantly all the way to did-exchange implementation. This enable many simplifications and removal of lots of code. Some other improvements has been done on the way as well.

I recommend start looking at the PR from top down to understand public API effect of the changes.

Demo

  • aries/aries_vcx/tests/test_did_exchange.rs is the top-most level of changes

Simplified DidExchangeRequester

  • Now has single method construct_request which simple accepts a Did as input - can be any resolvable DID. This means it's also possible to establish didcomm any 2 entities which have DID - be it did:peer, did:sov or any other DID.
  • Trimmed down support for invitations - we do not support OOB invitations with service objects "embedded" into the invitation. This Aries design was objected in Out of band - proposing change ( services VS from ) aries-rfcs#802. We can implemented later if desired by any vcx users, but solution is ugly - it would be needed to convert Service object to Did Document which doesn't make sense.

These changes also lead to internals simplification where bunch of transformation code could been removed

Simplified DidExchangeResponder

  • Just like requester (invitee), DidExchangeRequester(inviter) needs to provide their pairwise PeerDid for the counterparty upon constructing Response message. This means that currently the responder always rotates their DID, regardless of what kind of DID they used in invitation. Making rotation optional will be addressed in the future.
  • Removes required argument invitation_id from receive_request method signature. Also, don't keep this information in SM state - the responder might receive request which is not constructed on basis of any invitation - but rather simply on basis of knowing responder's public DID. Invitation might not exist in a first place, so it doesn't make sense to store its ID.

DidCore

Removed generics

The main goal of the PR was simplification of DidCore components - mainly removing generic type parameters from DidDocument<E> and Service<E>. This lead to many changes, but in scope of DidCore components:

  • Instance of DidDocument can represent any valid Did Document with any extra fields. Instance of Service can represent any Service object, with any extra fields.
  • In practice, in didcomm we usually use certain extra fields on Service object - such as routingKeys, recipientKeys, accept etc. Given the "extra" typing we dropped, the current typing design has following considerations:
    1. When resolving did document, we don't know what extra field we might find. We don't know what resolver will return for a particular DID. We don't know what fields the applications build on top of did_doc crate wants to support, work with. Hence it's up to applications to decide either work with DidDocument/Service as their are, or convert these to custom types in runtime, if typed environment for extra fields is needed.
    2. We also provide fully typed version of common didcomm Services - such as struct ServiceDidCommV1, struct ServiceDidCommV2. These tend to be useful when the user wants to construct particular service type themselves. Equally, as mentioned in the previous point i., user can also choose to convert Serviceto more explicit types such asServiceDidCommV1upon resolution. However, in practical terms so far inaries_vcx`, this was not really needed.

New features

  • Implement new util methods on DidDocument such as get_service_of_type, get_key_agreement_of_type to support data lookups within did documents.

Refactoring

  • Remove custom url wrapper for url::Url crate in did_doc crate
  • Refactoring around did:peer:2 de/abbreviation

Error handling

  • Refactoring of errors to reduce number of variants, for example DidDocumentBuilderError now has only single KeyDecodingError variant, which itself hold further more details about particular decoding issue
  • Introduce new error types in did_doc crate, such as JsonWebKeyError, MultibaseWrapperError, KeyDecodingError and others to avoid misusing DidDocumentBuilderError in contexts where we are not in fact using did document builder.

Other changes

Testing

Logging

  • Derive Display for more structs

Refactoring

  • aries-vcx-agent did-exchange service is generating invitation with did:peer as value of services, rather than inlining service object.
  • Make DidPeer<Numalgo2> --- DDO user friendlier. Instead of calling "util" function resolve_numalgo2(did PublicKeyEncoding::Base58) this is now done did.to_did_doc(PublicKeyEncoding::Base58)
  • Didcomm v1 refactoring (EncryptionEnvelope)
    • added new constructor which takes in 2 DidDocument structures. Noteworthy is that key_agreement is used for encryption rather than recipientKeys "extra field". The thing is, peer:did:2 doesn't even define how recipientKeys should be abbreviated, I believe it assumes it wouldn't be used anymore.

Fix

  • did:peer:3 validation regex did not allow DIDs which contains Service only. But all fields in Did Document are optional - except id, did:peer:3.S.* is valid DID too.

Note

@Patrik-Stas Patrik-Stas changed the title change: simplify didcore components, did-exchange protocol draft: change: simplify didcore components, did-exchange protocol Nov 29, 2023
@Patrik-Stas Patrik-Stas force-pushed the refactor/encryption-envelope branch from 476d5e8 to 53f3a26 Compare November 29, 2023 19:47
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from 1caf5e1 to 33e34cd Compare November 29, 2023 21:59
Base automatically changed from refactor/encryption-envelope to main November 30, 2023 07:55
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from 33e34cd to 6442acc Compare November 30, 2023 15:10
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

Attention: Patch coverage is 0% with 1125 lines in your changes are missing coverage. Please review.

Project coverage is 0.05%. Comparing base (74ab042) to head (053055e).

Files Patch % Lines
did_core/did_doc/src/schema/legacy.rs 0.00% 104 Missing ⚠️
...peer_did/numalgos/numalgo2/service_abbreviation.rs 0.00% 95 Missing ⚠️
aries/aries_vcx/tests/test_did_exchange.rs 0.00% 93 Missing ⚠️
did_core/did_doc/src/schema/service/mod.rs 0.00% 72 Missing ⚠️
...rc/protocols/did_exchange/state_machine/helpers.rs 0.00% 67 Missing ⚠️
...core/did_doc/src/schema/service/typed/didcommv1.rs 0.00% 60 Missing ⚠️
aries/aries_vcx/src/utils/didcomm_utils.rs 0.00% 53 Missing ⚠️
...did_peer/src/peer_did/numalgos/numalgo2/helpers.rs 0.00% 47 Missing ⚠️
did_core/did_doc/src/schema/service/typed/mod.rs 0.00% 43 Missing ⚠️
aries/aries_vcx/src/protocols/did_exchange/mod.rs 0.00% 41 Missing ⚠️
... and 46 more
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #1075    +/-   ##
======================================
  Coverage   0.05%   0.05%            
======================================
  Files        473     476     +3     
  Lines      24239   24137   -102     
  Branches    4492    4498     +6     
======================================
  Hits          13      13            
+ Misses     24226   24124   -102     
Flag Coverage Δ
unittests-aries-vcx 0.05% <0.00%> (+<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.

@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from d15cdcb to 3e42245 Compare December 5, 2023 14:14
@Patrik-Stas Patrik-Stas changed the base branch from main to refactor/encryption-envelope-2 December 5, 2023 14:15
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from f9f8e2c to daff7fb Compare December 7, 2023 11:45
@Patrik-Stas Patrik-Stas force-pushed the refactor/encryption-envelope-2 branch from 5fc1905 to a00160a Compare December 7, 2023 14:24
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch 2 times, most recently from 784828a to dc17346 Compare December 7, 2023 21:33
Base automatically changed from refactor/encryption-envelope-2 to main December 8, 2023 08:28
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from 8fdd9c6 to f1af5c5 Compare December 15, 2023 15:50
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch 3 times, most recently from d0fdfcd to 7d5a064 Compare January 8, 2024 09:02
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch 2 times, most recently from 354362b to 4a3a300 Compare January 15, 2024 10:00
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from 5dcd014 to e0c163e Compare January 18, 2024 13:59
@Patrik-Stas Patrik-Stas changed the title draft: change: simplify didcore components, did-exchange protocol change: simplify didcore components, did-exchange protocol Jan 18, 2024
@Patrik-Stas Patrik-Stas marked this pull request as ready for review January 18, 2024 17:33
@Patrik-Stas Patrik-Stas requested a review from mirgee January 18, 2024 17:34
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from e0c163e to 79fadb5 Compare January 31, 2024 11:21
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch 2 times, most recently from 87a5de5 to 43be694 Compare February 10, 2024 00:01
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from 43be694 to ea718f8 Compare March 1, 2024 05:52
@Patrik-Stas Patrik-Stas changed the base branch from main to improvements/aries-agent March 1, 2024 05:52
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from ff3bc02 to 4e99c35 Compare March 8, 2024 08:09
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch 10 times, most recently from 87e13ab to 560a143 Compare March 14, 2024 14:19
@Patrik-Stas Patrik-Stas changed the base branch from improvements/aries-agent to main March 14, 2024 14:22
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch 2 times, most recently from 69d80d3 to 80dc4d7 Compare March 14, 2024 14:30
@Patrik-Stas Patrik-Stas force-pushed the diddoc/remove-generics branch from 80dc4d7 to 053055e Compare March 15, 2024 01:05
@Patrik-Stas Patrik-Stas requested a review from gmulhearn March 15, 2024 04:03
@Patrik-Stas
Copy link
Contributor Author

aries-vcx -- aries-vcx AATH did-exchange tests passing
image
image

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.

Skimmed thru aries changes. nice stuff

@Patrik-Stas Patrik-Stas merged commit 910a630 into main Mar 15, 2024
25 checks passed
@Patrik-Stas Patrik-Stas deleted the diddoc/remove-generics branch March 15, 2024 04:40
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