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

draft: DidDocument integration #864

Closed
wants to merge 1 commit into from
Closed

draft: DidDocument integration #864

wants to merge 1 commit into from

Conversation

mirgee
Copy link
Contributor

@mirgee mirgee commented May 30, 2023

Replaces the use of

  • diddoc_legacy::aries::diddoc::AriesDidDoc with did_doc::schema::did_doc::DidDocument, and
  • diddoc_legacy::aries::service::AriesService with did_doc::schema::service::Service
    across the aries-vcx codebase.

Integration with mediated connection is skipped and diddoc_legacy will be removed along with mediated_connection.

The method specific fields of the service struct are now set via a type associated with the DidResolvable trait, where only fields specific to the sovrin method are used in the codebase for now. This choice impacts the current implementation of the resolver registry, which needs to be reconsidered.

Moreover, currently, the service type is set manually during the construction of the DidDocument, and the accept field is not set at all. However, as per the Sovrin DID method specification, the value of those two fields is implied by the composition of / fields included in the service. This suggests that potentially we might want to wrap the DidDocumentBuilder in some kind of DidDocumentBuilderSov responsible for this method-specific logic in the future.

This would be also useful because the extra fields are defined in did_resolver_sov. In some use-cases, the entire resolver is imported only for the extra fields needed in order to name the DidDocument or Service type.

The DID parser fails to parse any did which is not fully qualified. This is desirable in cases where fully qualified DID is required as it forces early failure. But in situations where the method is implied, an unqualified DID may also be valid and usable. Therefore, perhaps a separate type should be created to be used in these cases.

As it stands, this integration is a breaking change as it changes the format of the did_doc field in the serialized connection state machines in both non-backwards-compatible and non-forwards-compatible way (e.g. some fields optional in the legacy format are now required and publicKey used in the legacy format is not a valid DDO field - it can probably be mapped to another valid field when deserializing, but currently isn't).

@mirgee mirgee linked an issue May 30, 2023 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 30, 2023

Codecov Report

Merging #864 (b72de4b) into main (1222ef7) will decrease coverage by 1.01%.
The diff coverage is 61.46%.

@@            Coverage Diff             @@
##             main     #864      +/-   ##
==========================================
- Coverage   48.96%   47.96%   -1.01%     
==========================================
  Files         429      462      +33     
  Lines       34225    35414    +1189     
  Branches     7594     7749     +155     
==========================================
+ Hits        16759    16985     +226     
- Misses      12208    13139     +931     
- Partials     5258     5290      +32     
Flag Coverage Δ
unittests-aries-vcx 47.92% <61.46%> (-1.02%) ⬇️

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

Impacted Files Coverage Δ
aries_vcx/src/errors/error.rs 32.43% <ø> (ø)
aries_vcx/src/errors/mapping_diddoc.rs 0.00% <0.00%> (ø)
aries_vcx/src/errors/mapping_others.rs 5.33% <0.00%> (-0.31%) ⬇️
aries_vcx/src/protocols/connection/trait_bounds.rs 0.00% <0.00%> (ø)
...ocols/mediated_connection/invitee/state_machine.rs 51.26% <0.00%> (-0.73%) ⬇️
...ocols/mediated_connection/inviter/state_machine.rs 60.08% <0.00%> (-1.29%) ⬇️
aries_vcx/src/utils/constants.rs 100.00% <ø> (ø)
aries_vcx/tests/test_connection.rs 72.39% <0.00%> (+0.90%) ⬆️
aries_vcx_core/src/errors/error.rs 43.24% <ø> (ø)
aries_vcx_core/src/indy/wallet.rs 37.65% <ø> (ø)
... and 44 more

... and 32 files with indirect coverage changes

@mirgee mirgee force-pushed the integration/did-doc branch 6 times, most recently from 32e32ff to b4452fe Compare May 31, 2023 12:30
{
controller: '2RjtVytftf9Psbh3E8jqyq',
id: '1',
Copy link
Contributor Author

@mirgee mirgee May 31, 2023

Choose a reason for hiding this comment

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

The problem here is 1 is not a valid DidUrl, so the deserialization fails with this value. The correct value here would be did:sov:2RjtVytftf9Psbh3E8jqyq#1.

@@ -62,7 +63,7 @@ export const ARIES_CONNECTION_REQUEST = {
{
id: 'did:example:123456789abcdefghi;indy',
priority: 0,
recipientKeys: ['2RjtVytftf9Psbh3E8jqyq#1'],
Copy link
Contributor Author

@mirgee mirgee May 31, 2023

Choose a reason for hiding this comment

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

Currently, after converting from the new to legacy DDO format, resolving key by reference fails. Since this is a field specific to the Sovrin DID method, the dereferencing should probably be responsibility of ExtraFieldsSov and is not implemented currently.

@mirgee
Copy link
Contributor Author

mirgee commented Jun 1, 2023

Postponed until

  • mediated connection is removed,
  • libvdrtools is removed,
  • DidExchange protocol is implemented,
  • Sovrin-specific wrapper for DidDocument is implemented (incl. dereferencing), and potentially
  • fully qualified DIDs are used across the codebase in backwards-compatible manner.

Signed-off-by: Miroslav Kovar <miroslav.kovar@absa.africa>
@Patrik-Stas Patrik-Stas changed the title WIP: DidDocument integration Draft: DidDocument integration Nov 26, 2023
@Patrik-Stas Patrik-Stas changed the title Draft: DidDocument integration draft: DidDocument integration Nov 26, 2023
@mirgee mirgee closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate DID resolver
2 participants