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

Remove generics in CoreDocument, VerificationMethod, Service, DIDUrl and LinkedDomainService #1110

Merged
merged 62 commits into from
Feb 2, 2023

Conversation

olivereanderson
Copy link
Contributor

@olivereanderson olivereanderson commented Jan 30, 2023

Description of change

This PR simplifies the library by removing the Document trait as well as all generics on CoreDocument, Service,VerificationMethod, DomainLinkageService and DIDUrl. As this is a somewhat radical change the reviewers are encouraged to carefully read the justification below and judge if it is sufficient.

This PR additionally upgrades the wasm-bindgen dependency to version 0. 2.84 as we need the following change. As a consequence of that I think we can remove bindings/build/lint::lintPtrNullWithoutFree, but I will leave that for another PR.

Why?

At the time the generic DID parameter was introduced it brought a lot of value as we could remove several casts that required opting out of the safe subset of Rust. The problem with
generics in the DID document however is that one cannot directly put documents not parameterized with the same types in a container (such as a Vec or slice). We tried to work around this problem with a Document trait and a closely related object safe ValidatorDocument trait, but when designing the new Resolver it was hard to work with those traits. More specifically I found it almost impossible to implement the Document trait for something like

enum DocumentType {SomeDIDType(CoreDocument<SomeDIDType>), OtherDIDType(CoreDocument<OtherDIDType>)}

or more realistically enum DocumentType {Iota(IotaDocument), Core(CoreDocument)}. This led to the introduction of the AbstractThreadSafeValidatorDocument that both has a terrible name and is a very leaky abstraction. This is very much the sort of thing we do not want users to have to deal with.

One possible way of discarding the AbstractThreadSafeValidatorDocument would be to make the Resolver only resolve CoreDocument (in which case all document types would have to implement Into<CoreDocument>). With that approach one would have to use a specific client directly if one wishes to preserve method specific information when resolving. After discussing with the team we arrived at the conclusion that we would prefer not to do that.

Another possibility would be to focus more on promoting the object safe ValidatorDocument trait, but that trait is fairly abstract as is now and such an approach would need more careful consideration. Furthermore it is likely that we want verification functionality to not be a direct concern of the document, but rather another structure such as a JwtVerifier where custom cryptographic algorithms can be registered. In other words this seems to be a case where it is a good idea to separate data from functionality.

The third approach which is what this PR does is to just remove the generics from CoreDocument altogether. I have yet to see the rewards of having type enforced method specific DID Urls for a document's verification methods or services. In fact most of the time you would want them to have the same DID component (not just method) as that of the document which is something we cannot enforce in the type system, and there could also be niche cases where you would even want the URLs to refer to other DID methods which the spec allows (this is perhaps more so the case for controllers). If I am overlooking something I would of course be happy to strongly consider closing this PR.

What about the non-DID specific parameters in VerificationMethod, Service, etc.

In practice we have so far found that they are either not used, or a (JSON) Object representation is enough. If we should later become aware of (or get requests for supporting) use-cases requiring the ability to inject more specific types into these structures, we can consider re-adding those (non-DID specific) parameters in an almost non-breaking manner (in the case when the same parameters can be used across different document types only though).

Also note that the generic parameter T in the properties field in the current CoreDocument<D,T,U,V> can be avoided by instead introducing a wrapper

#[Derive(Serialize, Deserialize)]
struct MyDocument {
    #[serde(flatten)]
    doc: CoreDocument,
    #[serde(flatten)]
    my_additional_properties: MyProperties 
} 

What about identifiers one would expect to be method specific?

A good example of such an identifier is the identifier of a DID document published with the IOTA UTXO method specification. Even though represented internally using a CoreDID, one will still receive a &IotaDID when calling IotaDocument::id. Like before this is not expressible in the safe subset of Rust (yet), but this time we wrap this functionality in a safe interface generated by the ref-cast crate. A crate created by dtolnay with more than five million downloads. Unfortunately the #![forbid(unsafe_code)] lint has to be removed from the identity_iota_core crate to accommodate for this as the macro from ref-cast generates an unsafe code block (while checking its soundness).

Similarly one would expect the controller(s) of an IotaDocument to be IOTA DIDs. To accommodate for this the IotaDocument::controller method now returns an iterator over the controller(s) and this uses the same trick as mentioned in the previous paragraph to obtain IotaDID references (in the bindings there is no change as we need to clone anyway).

Note that there is (at least to the best of my knowledge) no unchecked way to mutate the identifier of an IotaDocument (or its controllers) after construction in the public API hence
handing out IotaDID references to said identifiers should be sound, but I am very much aware that this approach creates more opportunities for bugs to be introduced.

How do these changes affect the PresentationValidator?

The API stays pretty much the same as before except all the ValidatorDocument trait bounds are now replaced with AsRef<CoreDocument> which should be simpler for Rust developers to understand.

Short summary

The advantages are:

  • Simpler structs (less generics).
  • Simpler trait bounds (AsRef<CoreDocument> instead of ValidatorDocument).
  • Less manual monomorphization in the bindings.
  • The Resolver can now easily be configured to return an enum that both wraps IotaDocument and satisfies AsRef<CoreDocument>, hence AbstractThreadSafeValidatorDocument can be removed. I have some ideas for how to generalize the Resolver in the bindings, but that is for another PR.
  • With the ideas we have for JWT verification we probably want to keep reading key material from DID documents, but it is likely better for the verification functionality/logic to reside elsewhere.
  • More methods available directly on the CoreDocument / IotaDocument without needing to import the Document trait (more suggestions from IDEs).
  • Less work for users to integrate their custom DID document types with the functionality of our library (provided that the types we provide are sufficient for their use-cases). All that is needed is to implement AsRef<CoreDocument>.

The disadvantages are:

  • Increased chances of bugs in/with IotaDID.
  • Potentially leads to more runtime checks (But I struggle to come up with a use-case where you extract a verification method or service from an already resolved document and need the DIDUrl of said service or method to be of a certain specialized type for further processing).
  • The AsRef<CoreDocument> trait bound forces implementers to build their types using
    CoreDocument. This is something a trait abstraction might be able to avoid, but as it is now the Document trait still pretty much enforces the use of some types from our library.

Open Questions

  • Do we want to merge this?
  • Should we also remove the DID trait and instead bake its methods directly in to CoreDID and IotaDID so users don't need to import it (the trait is no longer really necessary for polymorphism).
  • Is there enough value in returning &IotaDID from IotaDocument::id (and an iterator of these in the case of controller), or would it also be OK to just return &CoreDID and we can worry less about accidental bugs breaking user's expectations?

Links to any relevant issues

Sub-task of #1103 .

Type of change

Add an x to the boxes that are relevant to your changes.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

Runs under cargo test.

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@olivereanderson olivereanderson self-assigned this Jan 30, 2023
@olivereanderson olivereanderson added the Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog label Jan 30, 2023
Copy link
Contributor

@abdulmth abdulmth left a comment

Choose a reason for hiding this comment

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

Looks good to me, I agree with Philipp that this PR is an improvement and I like how simpler everything became. I added minor comments, mostly nits.

identity_credential/src/revocation/document_ext.rs Outdated Show resolved Hide resolved
identity_credential/src/revocation/document_ext.rs Outdated Show resolved Hide resolved
identity_iota_core/src/document/iota_document.rs Outdated Show resolved Hide resolved
identity_iota_core/src/document/iota_document.rs Outdated Show resolved Hide resolved
bindings/wasm/examples/src/0_basic/0_create_did.ts Outdated Show resolved Hide resolved
Oliver E. Anderson and others added 7 commits February 2, 2023 09:05
Co-authored-by: Abdulrahim Al Methiab <31316147+abdulmth@users.noreply.github.com>
Co-authored-by: Abdulrahim Al Methiab <31316147+abdulmth@users.noreply.github.com>
Co-authored-by: Abdulrahim Al Methiab <31316147+abdulmth@users.noreply.github.com>
Co-authored-by: Abdulrahim Al Methiab <31316147+abdulmth@users.noreply.github.com>
@olivereanderson olivereanderson merged commit a58e7b8 into main Feb 2, 2023
@olivereanderson olivereanderson deleted the chore/remove-coredocument-generics branch February 2, 2023 20:17
@PhilippGackstatter PhilippGackstatter changed the title Remove generics in CoreDocument, VerificationMethod, Service, DIDUrl and LinkedDomainService. Remove generics in CoreDocument, VerificationMethod, Service, DIDUrl and LinkedDomainService Aug 15, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
Development

Successfully merging this pull request may close these issues.

3 participants