Add new crates for DID parsing, DID document building, and DID resolution #812

mirgee · 2023-04-24T06:10:06Z

This PR introduces new crates related to DID parsing, DID document building, and DID resolution, thus partially addressing issue #706 .

Some features in the added crates have no immediate use inside of AriesVCX, and we may consider moving some of these crates to a separate project, especially as the number of implemented resolvers begins to grow. However, their inclusion in the PR is due to the immediate need for a Sovrin DID method resolver, while striving to keep in line with following design goals:

suitability for use in AriesVCX, and
reusability in other projects, such as a driver for universal-resolver,
compliance with the relevant standards and specifications:

For example DID resolver HTTP(S) binding implementation using these crates, see here.

The following crates have been added:

did_parser

This crate is responsible for parsing DID URLs and DIDs into easily queriable components, as well as DID (URL) validation. Includes ParsedDID and ParsedDIDUrl structs, which are used heavily throughout the remaining crates.

did_doc_builder

The did_doc_builder crate allows its users to construct, serialize and deserialize DID documents and their components that comply with the DID Core specification (https://www.w3.org/TR/did-core/).

did_resolver

This crate defines traits that should be implemented by DID resolvers of specific DID methods. For now, the traits are DIDResolvable and DIDDereferenceable.

did_resolver_registry

The did_resolver_registry is a simple crate that enables the registration of multiple DID resolvers for different DID methods at runtime and subsequent resolution of DIDs for methods with a registered resolver, thus enabling support for variable number of DID methods.

did_resolver_sov

This crate provides a DID resolver for the Sovrin DID method (https://sovrin-foundation.github.io/sovrin/spec/did-method-spec-template.html), and hence is the main candidate for possible use in AriesVCX.

Please review and let me know if any further changes are required.

nicobao · 2023-04-25T00:24:19Z

Really nice!
I wonder if there is any work planned to support non-ledger methods such as did:web?
The did-web rust crate written by Spruce could be reused.

Patrik-Stas · 2023-04-27T19:22:13Z

@nicobao most definitely we'll be adding support for more methods. Thanks for heads up about the spruce repo - I wasn't aware of it and it actually looks amazing 👍 💯 - so many did methods ready to tap into at the fingertips https://github.com/spruceid/ssi

The main concern imo would be all the dependencies and the code it would bring it, many of which would likely be unnecessary. I think we could help spruceid to feature flag the dependencies, so it would be possible to bring in only the deps for desired set of did methods.

@mirgee @bobozaur @gmulhearn check it out guys :-)

did_parser/src/parsed_did.rs

did_parser/src/utils/parse.rs

aries_vcx_core/src/ledger/base_ledger.rs

aries_vcx_core/Cargo.toml

Patrik-Stas · 2023-04-28T07:38:09Z

.github/workflows/main.yml

@@ -432,6 +432,19 @@ jobs:
        run: |
          RUST_TEST_THREADS=1 cargo test --manifest-path="libvcx/Cargo.toml" -F "pool_tests";

+  test-integration-resolver:


The name is not quite accurate right now, the command also currently run unit tests too. Either update the job name, or tweak the cargo cmd

.github/workflows/main.yml

did_parser/Cargo.toml

bobozaur

I like the DID parser using ranges of the input DID string, great job!

To me it feels like a lot of places would need documentation and comments, like the structs and traits and the larger functions that are not really easy to follow. I'm honestly not able to grasp everything that's happening just from browsing through the code so I tried to start conversations where appropriate.

There are some minor things to adjust as far as idioms go, but nothing terrible. Another broader idea would be to incorporate some of the utility functions as associated methods, because it's easier to grasp what they are about if they belong to a namespace and most seem to have unique use cases.

Great job over all!

aries_vcx_core/Cargo.toml

did_doc_builder/src/error.rs

bobozaur · 2023-04-28T07:21:34Z

did_doc_builder/src/schema/did_doc.rs

+    pub fn extra(&self, key: &str) -> Option<&Value> {
+        self.extra.get(key)
+    }


This method is confusing to me. What if I wanted the whole extra field? I think it would be better to return that and consumers can lookup stuff in the map themselves.

The way this limits the use of extra is intentional - you should never need the whole extra field. Moreover, the client code needs not and should not be aware of the internal data structure used for extra fields so that changing it for another collection is not a breaking change.

Left in as is as discussed.

How about renaming it then? The method looks like a getter of the map field instead of a lookup into the map. I agree though that the inner field should not be exposed.

Apologies, forgot about the suggestion to rename. What name do you suggest?

Something like lookup_extra_field?

How about get_extra_field instead of lookup_extra_field?

I think get is implicit. Maybe just extra_field?

did_doc_builder/src/schema/did_doc.rs

did_resolver/src/traits/dereferenceable/dereferencing_error.rs

did_resolver/src/traits/resolvable/resolution_error.rs

did_resolver_registry/src/lib.rs

did_resolver/src/traits/resolvable/mod.rs

did_resolver/src/traits/dereferenceable/mod.rs

Patrik-Stas

left some comments, still continuing in review

did_doc_builder/src/schema/did_doc.rs

did_doc_builder/src/schema/service.rs

did_doc_builder/src/schema/types/multibase.rs

did_doc_builder/src/schema/types/jsonwebkey.rs

did_resolver_sov/src/resolution/utils.rs

Patrik-Stas · 2023-05-04T06:03:12Z

did_resolver_sov/src/resolution/utils.rs

+        let mut service_builder =
+            Service::builder(service_id, endpoint.endpoint.as_str().try_into()?)?;
+        for t in endpoint.types {
+            if t != DidSovServiceType::Unknown {


This is just a nice to have, and I toched on this elsewhere. Feel free to pass this, I think this could be nice good-first-issue.

If you leave this up for good-first-issue, please leave this comment un-resolved.

Although DID Core does not specify possible values of type, we could still make our code slightly developer friendlier making enum for types within our DidDocument structure. We could reuse DidSovServiceType - rename it toServiceType, move it to diddocument crate and tweak it a bit:

#[derive(Debug, Clone, Deserialize, PartialEq, Eq, Hash)] pub enum ServiceType { #[serde(rename = "endpoint")] // AIP 1.0 Endpoint, #[serde(rename = "did-communication")] // AIP 2.0 DidCommunication, #[serde(rename = "DIDComm")] // DIDComm V2 DIDComm, #[serde(other)] Unknown(value), }

Just not sure about details how to get Unknown(value) such that the value parameter would get filled in.

This would require a custom impl for Serialize/Deserialize (or re-using the MaybeKnown enum?). The #[serde(other)] attribute only works with unit variants, unfortunately.

did_resolver_sov/src/resolution/utils.rs

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

Patrik-Stas

lgtm, can we move it from draft to ready state?

Patrik-Stas

lgtm, can we move it from draft to ready state?

did_doc_builder/src/schema/did_doc.rs

bobozaur

Most of the things left are minor things or personal opinions, so feel free to merely comment or ignore them, but I think the conversion between DidUrl and Did can really be optimized to avoid double-parsing.

did_doc_builder/src/schema/types/uri.rs

did_parser/src/parsed_did.rs

bobozaur · 2023-05-08T10:11:37Z

did_parser/src/parsed_did.rs

+        Self::parse(
+            did_url
+                .did()
+                .ok_or(Self::Error::InvalidInput("No DID provided in the DID URL"))?
+                .to_string(),
+        )


Shouldn't there be a better way to convert? The DID is already parsed, so it's just a matter of whether there are path arguments and stuff like that. If the DidUrl doesn't contain any of the additional stuff that makes it a did url, then the parts could be converted to a Did, right?

Are you suggesting to expose the fields of DidUrl for this?

Through pub(crate), yeah. Why not? Or simply a constructor method could be added that's crate internal, and the from impl could be near the ParsedDidUrl type so the fields can stay private.

I think the overhead caused by this is so negligible it's not worth additional code or exposing the now private fields at all. But I will do as you wish.

The crate public constructor is probably the best way to go about it, so the fields aren't touched.

I think it's highly worth it. Why not optimize this when it's easy to do so? The difference between parsing and not parsing the string is huge in the number of executed instructions.

Again, I don't think this is worth either changing the visibility of the fields or adding additional code. Keeping a struct's trait implementations in the same module it's defined in also makes the code navigable, orderly and well-organized.

And again, a call to parse_did_method_id has absolutely negligible performance cost.

But since you think it's highly worth it, I will change it according to your preference.

bobozaur · 2023-05-08T10:14:24Z

did_resolver_sov/src/resolution/utils.rs

+        let mut service_builder =
+            Service::builder(service_id, endpoint.endpoint.as_str().try_into()?)?;
+        for t in endpoint.types {
+            if t != DidSovServiceType::Unknown {


This would require a custom impl for Serialize/Deserialize (or re-using the MaybeKnown enum?). The #[serde(other)] attribute only works with unit variants, unfortunately.

did_parser/src/parsed_did_url.rs

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

bobozaur

Just a small comment regarding the TryFrom impl between DidUrl and Did

did_parser/src/parsed_did_url.rs

bobozaur

Can we also change the parsed_did and parsed_did_url module file names, for consistency?

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

Patrik-Stas · 2023-05-09T10:36:28Z

@mirgee please rerun the CI without skip-ci tag

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

codecov-commenter · 2023-05-09T10:45:54Z

Codecov Report

Merging #812 (594e161) into main (036bbf9) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #812      +/-   ##
==========================================
+ Coverage   49.06%   49.09%   +0.03%     
==========================================
  Files         410      410              
  Lines       33501    33518      +17     
  Branches     7440     7444       +4     
==========================================
+ Hits        16436    16455      +19     
- Misses      11949    11951       +2     
+ Partials     5116     5112       -4

Flag	Coverage Δ
unittests-aries-vcx	`49.06% <ø> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
did_doc_builder/src/schema/types/mod.rs	`50.00% <ø> (ø)`
did_parser/src/did.rs	`100.00% <ø> (ø)`

... and 89 files with indirect coverage changes

bobozaur

If the CI runs, I'm okay with the PR.

mirgee added feature skip-ci labels Apr 24, 2023

mirgee linked an issue Apr 24, 2023 that may be closed by this pull request

Implement DDO Resolver #706

Closed

mirgee force-pushed the feature/did-resolver branch 6 times, most recently from f0e43ed to e3e8986 Compare April 24, 2023 07:47

mirgee changed the title ~~Add new crates for DID parsing, document building, and DID resolution~~ Add new crates for DID parsing, DID document building, and DID resolution Apr 24, 2023

mirgee force-pushed the feature/did-resolver branch 3 times, most recently from 5b457ca to 07f6ac2 Compare April 24, 2023 18:50

mirgee force-pushed the feature/did-resolver branch 2 times, most recently from d62e108 to 39b3451 Compare April 26, 2023 14:00

Patrik-Stas requested review from gmulhearn, bobozaur and Patrik-Stas April 28, 2023 06:39