did:ion (Sidetree) resolver and client #379

clehner · 2022-01-14T15:33:33Z

Resolve did:ion(:test) DIDs via Sidetree node
Construct Sidetree operations: Create, Update, Recover, Deactivate
Construct DID from Create Operation locally
Wrap Sidetree operations into DID-method-generalized transaction data structure
Submit operations to Sidetree nodes
~~[ ] Handle response from Sidetree node~~ - Not sure what to do with the responses. Just passing back Value to in the submit_transaction function, for now.

Docs preview: https://demo.didkit.dev/2022/02/25/ssi-rustdoc/did_ion/sidetree/

sbihel

Half of the code is checking whether fields exist and/or are of the right type. It breaks my heart a little bit 😅

Don't take my comments as requests for changes. They're mostly questions, and sometimes complaints at how spaghetti dish-like the specs appear to me.

sbihel · 2022-02-15T10:27:15Z

src/did.rs

+/// A transaction for a DID method
+#[derive(Debug, Serialize, Deserialize, Builder, Clone, PartialEq)]
+#[serde(rename_all = "camelCase")]
+pub struct DIDMethodTransaction {


I don't fully understand everything behind DID transactions, but I don't know if it's a good idea to generalise them? We give up the benefits of the type system for a slight improvement in the time it takes to make a change?

Would one really use DID methods interchangeably so much that they would expect to be able to be able to make transactions generically?

Apart from DIDKit, sort of, but I think that's its role, to provide a fairly unified interface, but keep the "type purity" in ssi.

My initial thoughts for alternatives would be:

a separate trait for transactions, probably with this struct's value type being that is can be serialised; or

don't have a trait, let methods implement whatever they need (maybe some of them wouldn't have an update transaction, or instead more kinds of transactions).

Ok I see this is from the did-registrar. I still think it would be better to have a separate trait.

More info about the proposed transaction concept, in didkit-cli's readme: https://github.com/spruceid/didkit/blob/3a220b0fddfa6565a2214284430a079ac7057a10/cli/README.md#did-method-transaction
DID Method Transaction is like a Sidetree Operation Request generalized to arbitrary DID methods. The contents are DID-method-specific, except for the didMethod property which would be used by DIDKit and/or ssi's DIDMethods struct to route the operation to a DID method implementation for submission/processing.
In terms of DID Registration, the transaction represents a DID operation that is ready to be written to the DID method's verifiable data registry. I don't see this data structure as being exposed in DID Registration API; it's otherwise internal to the
implementation. It's not using DID Registration's didDocument/didDocumentOperation fields - although it might be convertible to/from them - but its signature is over a method-specific data structure. For other DID methods, the DID method transaction might contain a signed blockchain transaction (e.g. for did:ethr, did:tz, and did:btcr). (For Sidetree, the blockchain transaction is a separate step, in this implementation assumed to be done on the other side of the Sidetree REST API.)

Sidetree in https://identity.foundation/sidetree/spec/v1.0.0/#sidetree-operations requires clients to "keep the operation payload once it is submitted to a Sidetree node until it is generally available and observed" and to resubmit the same operation payload "if the submitted operation is not anchored and propagated, for whatever reason". Without the transaction concept, the operation functions here would need to block until the operation is observed, and/or maintain the operation in some external state. By returning the Sidetree Operation request as a DID method transaction to be separately submitted/published using DIDMethod::submit_transaction (didkit did-submit-tx), the operation functions stay more side-effect-free and do not risk being interrupted and losing the operation. Keeping the signed operations also allows the caller to prove their DID state with the chain of signed operations.

Note that currently this implementation does not wait for and detect that an operation has been successfully applied. So the caller must do that, resubmitting the operation if needed. This limitation could be addressed by making the submit tx function poll the resolver to detect the corresponding update - or by adding a separate function to do that.

I might need help to make the transaction into a trait. I thought of making the transaction type into an associated type on DIDMethod, but this does not work, because DIDResolver is used as a trait object, so DIDMethod cannot have generic type parameters, e.g.:

error[E0038]: the trait `DIDResolver` cannot be made into an object --> src/ldp.rs:2085:20 | 2085 | resolver: &dyn DIDResolver, | ^^^^^^^^^^^^^^^ `DIDResolver` cannot be made into an object | note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visi t <https://doc.rust-lang.org/reference/items/traits.html#object-safety> --> src/did_resolve.rs:469:8 | 410 | pub trait DIDResolver: Sync { | ----------- this trait cannot be made into an object... ... 469 | fn to_did_method<T>(&self) -> Option<&dyn DIDMethod<Transaction = T>> { | ^^^^^^^^^^^^^ ...because method `to_did_method` has generic type parameters = help: consider moving `to_did_method` to another trait

Also I'm not sure how this would be handled in DIDMethods where there is a map of strings to DIDMethod trait objects which each might use a different transaction type, but I'm not sure how to express that:

pub struct DIDMethods<'a> { /// Collection of DID methods by method id. pub methods: HashMap<&'a str, &'a dyn DIDMethod<Transaction = ...>>, }

If the functions using the Transaction type are moved out of DIDMethod into a new trait, e.g. DIDRegistrar or DIDOperator (like how the resolve functions are in DIDResolver), I would still assume to use a conversion function like fn to_registrar<T>(&self) -> &dyn DIDRegistrar<Transaction = T> (like DIDMethod::to_resolver) but this still doesn't work with DIDMethod being object-safe, so again I'm not sure what to do.

TBH I think to_resolver is an anti-pattern. If you need both DIDMethod and DIDResolver they should be both specified or if you can't have one without the other they should be combined.

I like the idea of having multiple implementations of a DIDRegistar trait for each method for each kind of transaction. It puts the burden on the user of ssi to route the types of messages, but it probably make sense? Some DIDs can be created generated locally and/or created in a registrar -- there are so many variants that users are probably best placed to chose what to do?

sbihel · 2022-02-15T10:55:17Z

did-ion/src/sidetree.rs

+            verification_key,
+            options,
+        } = create;
+        ensure!(is_empty(&options), "Create options not supported");


Just to double check, in this kind of scenario, should it be a hard failure or just a warning?

Right now the options are method-specific, and there aren't other options to use, so I don't know exactly.
Erroring on unknown option seems to me like erring on the side of caution.
But in DID Registrar driver implementations that I looked at, there is no such requirement that all passed options are recognized. So it might be more consistent to drop this ensure - or indeed make it a warning (changing the function's return type and plumbing the warnings through to DIDKit).

It could be nice if there was some convention to indicate whether an option may be safely ignored or is required to be understood - like using the "crit" JWS header parameter.

Unlike with DID resolution input metadata / options, there isn't yet a registry of DID operation options. DID resolution options have a similar issue I think, of how to know whether an option must be understood or can be ignored. I think the tendency is to treat options as optional to implement by default. Then the caller might have to infer from the output whether the option was actually understood and processed. (vs. inferring from an error message that a given option was unsupported, which would happen with this current implementation). The current Sidetree/ION DID resolver HTTP(S) endpoint though actually fail if any query string parameters are passed, making any resolution input-metadata/options result in an error... Some of those options, like service, could be handled client-side (in ssi's dereference function).
For DID method operations besides read/resolve, one approach could be that a critical option (option that must be understood) should just have a new operation defined for it specifically. This PR doesn't add a generic mechanism for additional DID method operations, although of course DID method implementations could add their own method-specific operation implementations.

did-ion/src/sidetree.rs

sbihel · 2022-02-15T12:27:17Z

did-ion/src/lib.rs

@@ -0,0 +1,26 @@
+use anyhow::{ensure, Context, Error, Result};


I don't know how it will be in the future, but maybe it would be good to use thiserror at the top level, and just have one variant to catch all anyhow errors?

In this file anyhow is being used for the return types of functions in the implementation of the Sidetree trait defined in sidetree.rs. Should there be an associated type on the Sidetree trait for these errors? In this file it's just generate_key and validate_key implemented, but the other functions in the Sidetree trait are currently using anyhow::Result also; presumably those should be changed too, to use various thiserror Error structs, or a single Error struct for the base Sidetree implementation? I might need help with this.

To me, it seems like the Error type can be shared between all implementations? Either it's an ssi error, or a request error, or something else that the user can't really do anything about. You can also use different error types for different methods.

Opened #392 about this.

Done in 909790b and d96ee38 (Edit: and bc52ab0, 1b30a4f); there shouldn't be any more anyhow exposed in the public APIs now - except in the error variant SidetreeError::Other.

sbihel · 2022-02-15T12:29:51Z

did-ion/src/sidetree.rs

+            .post(url)
+            .json(&op)
+            .header("Accept", "application/json")
+            .header("User-Agent", ssi::USER_AGENT)


Could maybe share the client in the struct, and maybe have a helper in ssi to make sure we always have this header?

Sharing the client would be good to allow connection reuse.
I think the User-Agent header should include the did-ion crate version also. (similar for the other DID method crates)

sbihel · 2022-02-15T12:30:08Z

did-ion/src/sidetree.rs

+            // Update operation may return empty body with 200 OK.
+            return Ok(Value::Null);
+        }
+        let bytes = resp.bytes().await.context("Unable to read HTTP response")?;


Why not use .json()?

.json() could be used. I think the current way is leftover from when I was debugging why certain results failed JSON parsing. (It was because the body was empty, now caught above). Edit: yes, the next line shows falling back to return the unparsed output in the error if parsing failed. Now that I know that the Sidetree reference implementation returns JSON on Create and an empty body for all other operations, maybe this could be safely changed. But I'm still wary that there might possibly be some useful non-JSON output returned...

clehner · 2022-02-15T21:41:23Z

src/did.rs

+    fn deactivate(&self, _deactivate: DIDDeactivate) -> AResult<DIDMethodTransaction> {
+        bail!("Deactivate operation not implemented for DID Method");
+    }
+


Similar to https://github.com/spruceid/ssi/pull/379/files#r806777763 about the Sidetree trait..
I since learned that anyhow is not recommended for APIs; maybe these should be changed back to using ssi::error::Error or something? Error associated type seems problematic for object-safety (also discussed in https://github.com/spruceid/ssi/pull/379/files#r807305656 about DID method transactions).

Opened #392

Wouldn't it be preferable to do the switch now to avoid breaking the API in the future? Not that it's a lot of work to adapt code for consumers.

The code could simply be

#[derive(Error, Debug)] pub enum MyError { #[error(transparent)] Other(#[from] anyhow::Error), // source and Display delegate to anyhow::Error }

Trying that... It seems that bail! and ensure! cannot be used then.

Done in a7879ce

sbihel

I have a few comment/questions for future improvements but I guess it can be merged as is.

Clippy has a few errors/warnings.
Maybe DIDMethodError::NotImplemented should actually be a panic, with unimplemented! or todo!.
If DIDMethodError::OptionNotSupported means that not all methods support all operations, maybe that means the trait should be split, to get compile time guarantees.
cargo build in did-ion doesn't work for me. We probably should try to have every crate in the workspace compile separately with default features.

- Use reqwest. - Keep hyper as dev-dependency for testing server. - Remove hyper-tls as unused.

- Don't use anyhow in did module - Use Map for DID method options, for simpler checking

- Add SidetreeError and JWSDecodeVerifyError - Remove anyhow from sidetree API

- Add PublicKeyJwkFromJWKError - Add JWKFromPublicKeyJwkError - Remove anyhow::Error from JWSDecodeVerifyError

clehner · 2022-03-01T16:07:37Z

Rebased and squashed some commits (1b30a4f → cd6a760).

@sbihel
Opened issues regarding your comment: #396, #398, #399.
I'm hesitant about adding panics, but see the point about compile guarantees. I'm wondering how this would work with DIDKit where there is a set (map) of DID method implementations (DID_METHODS implementing ssi::did::DIDMethods), and some commands require a DID method operation. e.g. if there is a DeactivatableDIDMethod trait, would there be another map for DID methods implementing that trait? In spruceid/didkit#255 there has to be a way to go from a DID for which deactivation is requested, to the implementation of the deactivate functionality. If the operation fails, it can return an error, like how resolving an invalid DID returns an invalidDid error (although errors for operations other than read are not as well defined).

clehner mentioned this pull request Jan 21, 2022

did:ion and DID operations spruceid/didkit#255

Merged

8 tasks

clehner changed the title ~~did:ion resolver and client~~ did:ion (Sidetree) resolver and client Feb 9, 2022

clehner mentioned this pull request Feb 14, 2022

Allow non-normalized ES256K #389

Merged

clehner force-pushed the feat/did-ion branch from 7a2f694 to 0cc6689 Compare February 14, 2022 20:29

sbihel reviewed Feb 15, 2022

View reviewed changes

clehner commented Feb 15, 2022

View reviewed changes

clehner marked this pull request as ready for review February 15, 2022 21:44

clehner mentioned this pull request Feb 23, 2022

Remove anyhow from public API #392

Closed

clehner requested a review from sbihel February 25, 2022 22:17

sbihel approved these changes Feb 28, 2022

View reviewed changes

clehner added 2 commits March 1, 2022 10:18

Relax DIDMethod: DIDResolver requirement

8fff89a

Fix WASM compilation with http-did feature

c9bc80e

- Use reqwest. - Keep hyper as dev-dependency for testing server. - Remove hyper-tls as unused.

This was referenced Mar 1, 2022

Workspace crates should build with default features #396

Open

Traits for DID method operations #398

Open

clehner added 7 commits March 1, 2022 10:46

Add DID operations

685046a

Add DID method transactions

afb1fcd

Add did:ion and Sidetree

dcb203f

Add DIDMethodError

a206815

- Don't use anyhow in did module - Use Map for DID method options, for simpler checking

Use error types in sidetree

369e067

- Add SidetreeError and JWSDecodeVerifyError - Remove anyhow from sidetree API

More Sidetree rustdocs

992dd34

Use error type for PublicKeyJwk JWK conversion

cd6a760

- Add PublicKeyJwkFromJWKError - Add JWKFromPublicKeyJwkError - Remove anyhow::Error from JWSDecodeVerifyError

clehner force-pushed the feat/did-ion branch from 1b30a4f to cd6a760 Compare March 1, 2022 15:47

clehner merged commit cd6a760 into main Mar 1, 2022

clehner deleted the feat/did-ion branch March 1, 2022 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

did:ion (Sidetree) resolver and client #379

did:ion (Sidetree) resolver and client #379

clehner commented Jan 14, 2022 •

edited

Loading

sbihel left a comment

sbihel Feb 15, 2022

sbihel Feb 15, 2022

clehner Feb 15, 2022

sbihel Feb 16, 2022

sbihel Feb 15, 2022

clehner Feb 15, 2022

sbihel Feb 15, 2022

clehner Feb 15, 2022

sbihel Feb 16, 2022

clehner Feb 23, 2022

clehner Feb 25, 2022 •

edited

Loading

sbihel Feb 15, 2022

clehner Feb 15, 2022

sbihel Feb 15, 2022

clehner Feb 15, 2022 •

edited

Loading

clehner Feb 15, 2022

clehner Feb 23, 2022

sbihel Feb 24, 2022

clehner Feb 25, 2022

clehner Feb 25, 2022

sbihel left a comment

clehner commented Mar 1, 2022

		@@ -0,0 +1,26 @@
		use anyhow::{ensure, Context, Error, Result};

did:ion (Sidetree) resolver and client #379

did:ion (Sidetree) resolver and client #379

Conversation

clehner commented Jan 14, 2022 • edited Loading

sbihel left a 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

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

clehner Feb 25, 2022 • 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

clehner Feb 15, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbihel left a comment

Choose a reason for hiding this comment

clehner commented Mar 1, 2022

clehner commented Jan 14, 2022 •

edited

Loading

clehner Feb 25, 2022 •

edited

Loading

clehner Feb 15, 2022 •

edited

Loading