feat: add internal did:web resolution #190

enmand · 2024-05-09T17:01:31Z

Remove dependency on the Spruce crates for did:web resolution, and implement did:web resolution internally in the dids crate.

Adds a test related to #39, but needs a trusted host for a real-resolution test for a path-based DID (does TBD have one already?)

Resolves #42.

Note: e830f1d makes the VerificationMessage optional to match the current set of properties in DID-Core -- the web5-spec disagrees with this, and I'm not sure who's right -- but the example hosted DID (https://www.tbd.website/.well-known/did.json) doesn't include a VerificationMap. The current implementation maps this to an empty Vec<VerificationMethod>, this PR changes that to an Option<Vec<VerificationMap>>.

kirahsapong · 2024-05-13T17:25:56Z

crates/dids/src/method/web/resolver.rs

+    use super::*;
+
+    #[tokio::test]
+    async fn resolution_success() {


Awesome work! Would we be able to add another test for resolving web dids with a port?

Yep, I can certainly do that!

enmand · 2024-05-20T23:58:59Z

crates/dids/src/document.rs

-    #[serde(rename = "verificationMethod")]
-    pub verification_method: Vec<VerificationMethod>,
+    #[serde(rename = "verificationMethod", skip_serializing_if = "Option::is_none")]
+    pub verification_method: Option<Vec<VerificationMethod>>,


How do you all feel about this change? 6a54d27 has some reasoning -- did-core and the web5 spec disagree so I'm not sure who's right. This example DID document on https://tbd.website/.well-known/did.json doesn't have a VerificationMethod which is what sparked this change.

Maybe it's worth a separate PR for discussion aside from the did:web resolution PR?

web5-spec for reference

you're correct - per current web5-spec, verificationMethod list requires at least 1 VM in it. here is some discussion where we noted it would be a departure, but we didn't explicitly record the reasons why.

@decentralgabe would it make sense to add to our web5-spec the reasoning for this departure from did-core spec?

yes, though I am wondering whether this guidance should still apply to the rust code.
originally it was to support a consistent feature set across languages that handled optional/polyglot type properties consistency
however, if rust is able to directly support the did core spec then maybe our guidance is superfluous

suffice to say - in this case I would be fine to follow DID core and ignore our spec - but open to counter points

Okay cool this is a great matter to work through. I'm in favor of enabling support for the DID Core spec over our web5-spec here in Rust. However, I propose we don't implement that change here, because there are more implications which need attending to. Primarily, the ones which come to mind are:

did:jwk requires exactly one verification method, and so we should add error cases

did:dht requires at least one verification method, and so we should add error cases

we'll need to also account for this change in the APID and consider the downstream implications for Kotlin & Swift target languages

@enmand could we parse these changes out of this PR, and then I stubbed in #231 for subsequent work

Sounds good! I've opened #233 with the changes to enable optional verification methods and rebased it out of this PR.

KendallWeihe · 2024-05-29T12:13:28Z

I see review has been requested, nice! This may sit for a bit as I'll be AFK for the next week, but others may get to it in the mean time. Nevertheless, we'll get this reviewed sooner or later! Thanks 🙌

enmand · 2024-06-05T18:54:17Z

Looks like this PR is out-of-date with the recent crate changes. I should have time later this week to rebase it.

enmand · 2024-06-10T02:24:40Z

Rebased on the changes for the crate re-organization. Let me know if you want me to separate the VerificationMethod stuff into a separate PR.

crates/web5/src/dids/document.rs

Remove dependency on the Spruce crates for did:web resolution Resolves TBD54566975#42 Adds a test related to TBD54566975#39, but needs a trusted host for a real-resolution test for a path-based DID.

crates/web5/src/dids/methods/web/resolver.rs

KendallWeihe · 2024-06-13T13:14:23Z

crates/web5/src/dids/methods/web/resolver.rs

+// PORT_SEP is the : character that separates the domain from the port in a URI.
+const PORT_SEP: &str = "%3A";
+
+const USER_AGENT: &str = concat!(env!("CARGO_PKG_NAME"), "/", env!("CARGO_PKG_VERSION"));


This is interesting, @enmand can you help me understand your reasoning for including setting the User-Agent header? I can come up with a few reasons for and against, but I'm slightly skeptical and default to not implementing it unless absolutely necessary.

Also I wonder, do you know how this would impact browser environments given we were to run this in a browser via a WASM binding? It's not a huge deal in this moment because we're deprioritizing WASM in this moment, so no need to spend a bunch of time investigating, but again my skepticism would lead me to conclude we're better off leaving this out until we have clarity on the broad array of cross-platform implications.

Mostly because it's a way for the did:web server to understand what versions/libraries it's dealing with, though given it's not a service-to-service environment and there are many callers maybe it's not as important. I'm not strong on keeping it or removing it -- I only added it because it was a natural thing for me to include, but also happy to not have it included.

Re. WASM environments -- that's a good questions. I'll implement it in a browser/WASM environment soon for the DWN project I'm working on, and if this is included I can report back (otherwise, I'll probably have other places where I make reqwest requests in the DWN so I can try it there). One challenge with WASM in general is networking, but WASIX and other modern preview specs include some networking (including HTTP stubbed by fetch, iirc)

I'm wrangling around code into the APID in this PR. I'm going to go ahead and merge this code and copy everything over into the temporary apid module, but I'm going to remove this feature. It's probably something we want, but I want to approach these matters conservatively because of downstream impacts to cross-language binding. I appreciate the thought! And I suspect we'll add it back at some point once we fully comprehend the implications.

KendallWeihe · 2024-06-13T13:21:16Z

crates/web5/src/dids/document.rs

@@ -10,7 +10,7 @@ pub struct Document {
    pub controller: Option<Vec<String>>,
    #[serde(rename = "alsoKnownAs", skip_serializing_if = "Option::is_none")]
    pub also_known_as: Option<Vec<String>>,
-    #[serde(rename = "verificationMethod")]
+    #[serde(rename = "verificationMethod", default = "Vec::new")]


Clever. This may have implications but I'm fine with moving forward with it.

I added it because the example did:web (https://tbd.website/.well-known/did.json) was failing to Deserialize, complaining about verification_methods being missing -- though maybe there's another way (aside from #233)?

We should update that DID because it's not really adding any value. I'm not sure how it can be used in its current state.

Co-authored-by: Kendall Weihe <[email protected]>

KendallWeihe

Really fantastic work ✅

kirahsapong reviewed May 13, 2024

View reviewed changes

enmand force-pushed the chore/did-web-impl branch 5 times, most recently from 5357418 to c76f6c6 Compare May 20, 2024 23:55

enmand commented May 20, 2024

View reviewed changes

enmand marked this pull request as ready for review May 24, 2024 02:03

enmand requested review from KendallWeihe, nitro-neal, shamilovtim, diehuxx, decentralgabe and frankhinek as code owners May 24, 2024 02:03

enmand force-pushed the chore/did-web-impl branch from c76f6c6 to 4bc9f42 Compare June 10, 2024 02:20

KendallWeihe reviewed Jun 12, 2024

View reviewed changes

crates/web5/src/dids/document.rs Outdated Show resolved Hide resolved

KendallWeihe mentioned this pull request Jun 12, 2024

Consider enabling optional verification methods #231

Open

enmand mentioned this pull request Jun 13, 2024

chore: enable optional verification_method for DID Document #233

Open

enmand added 4 commits June 12, 2024 22:55

feat: add internal did:web resolution

0e64772

Remove dependency on the Spruce crates for did:web resolution Resolves TBD54566975#42 Adds a test related to TBD54566975#39, but needs a trusted host for a real-resolution test for a path-based DID.

test: test for did-web parsing for id with encoded port

8177110

chore: move logic for resolution into static Resolver fn

777bab2

fix: default to empty vec for missing VerificationMethods

633df6b

enmand force-pushed the chore/did-web-impl branch from b7d8fc0 to 633df6b Compare June 13, 2024 02:08

KendallWeihe reviewed Jun 13, 2024

View reviewed changes

crates/web5/src/dids/methods/web/resolver.rs Outdated Show resolved Hide resolved

KendallWeihe reviewed Jun 13, 2024

View reviewed changes

fix: type in crates/web5/src/dids/methods/web/resolver.rs

69ccca3

Co-authored-by: Kendall Weihe <[email protected]>

KendallWeihe mentioned this pull request Jun 14, 2024

Implement partial APID 0.1.0 with UDL bindings #234

Merged

KendallWeihe approved these changes Jun 17, 2024

View reviewed changes

KendallWeihe merged commit abe6e72 into TBD54566975:main Jun 17, 2024
6 checks passed

KendallWeihe added a commit that referenced this pull request Jun 17, 2024

Bind latest did:web resolve from #190

fa13331

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add internal did:web resolution #190

feat: add internal did:web resolution #190

enmand commented May 9, 2024 •

edited

Loading

kirahsapong May 13, 2024

enmand May 16, 2024

enmand May 20, 2024

jiyoonie9 May 29, 2024

decentralgabe May 29, 2024 •

edited

Loading

KendallWeihe Jun 12, 2024

enmand Jun 13, 2024

KendallWeihe commented May 29, 2024

enmand commented Jun 5, 2024

enmand commented Jun 10, 2024

KendallWeihe Jun 13, 2024

enmand Jun 13, 2024

KendallWeihe Jun 17, 2024

KendallWeihe Jun 13, 2024

enmand Jun 13, 2024

decentralgabe Jun 13, 2024

KendallWeihe left a comment

feat: add internal did:web resolution #190

feat: add internal did:web resolution #190

Conversation

enmand commented May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentralgabe May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KendallWeihe commented May 29, 2024

enmand commented Jun 5, 2024

enmand commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KendallWeihe left a comment

Choose a reason for hiding this comment

enmand commented May 9, 2024 •

edited

Loading

decentralgabe May 29, 2024 •

edited

Loading