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

[Task] Implement metadata structures #486

Closed
8 tasks
cycraig opened this issue Nov 8, 2021 · 8 comments · Fixed by #540
Closed
8 tasks

[Task] Implement metadata structures #486

cycraig opened this issue Nov 8, 2021 · 8 comments · Fixed by #540
Assignees
Labels
Enhancement New feature or improvement to an existing feature

Comments

@cycraig
Copy link
Contributor

cycraig commented Nov 8, 2021

Description

Refactor IotaDocument and move the following fields to a new DocumentMetadata structure as per https://www.w3.org/TR/did-core/#did-document-metadata:

  • previousMessageId
  • created
  • updated
  • proof (will need to sign the DID Document and its metadata)
  • versionId (new)
  • deactivated (new) - optional if we don't want to implement deactivation yet

Both IotaDocument and the DocumentMetadata will be published to the Tangle together, not just the document.

Additionally, a ResolutionMetadata structure should be implemented as per https://www.w3.org/TR/did-core/#did-resolution-metadata which holds:

  • messageId (split this into integrationMessageId and diffMessageId)
  • contentType (as per specification)
  • error (as per specification)

Some metadata structures exist in identity-did already but they are mostly unused.

The spec-compliant resolver has been pushed back for now, so ResolutionMetadata will be included in a later issue.

Motivation

This change is necessary:

Resources

To-do list

  • Implement IotaDocumentMetadata
  • Implement IotaResolutionMetadata

Change checklist

  • The feature or fix is implemented in Rust and across all bindings whereas possible.
  • The feature or fix has sufficient testing coverage
  • All tests and examples build and run locally as expected
  • Every piece of code has been document according to the documentation guidelines.
  • If conceptual documentation (mdbook) and examples highlighting the feature exist, they are properly updated.
  • If the feature is not currently documented, a documentation task Issue has been opened to address this.
@cycraig cycraig self-assigned this Nov 8, 2021
@cycraig cycraig added the Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog label Nov 9, 2021
@cycraig
Copy link
Contributor Author

cycraig commented Nov 15, 2021

Current

metadata0 drawio

In the current codebase, the metadata fields (created, updated, previousMessageId, proof) are stored directly in the document as part of the properties. The properties field catches all other JSON fields not hard-coded in the core DID specification, and any user-defined fields on the document.

The messageId field is on the IotaDocument itself and points to the last integration chain message or is MessageId::null if merged with a diff chain update. This is why issue #361 exists and the reason to add the unambiguous previousIntegrationMessageId and previousDiffMessageId fields.

Note that we can deviate from the DID specification w.r.t. the document and resolution metadata representations as long as we can convert/re-arrange fields during the spec-compliant resolution. The on-Tangle representation can also differ if we convert it back during resolution, so there's room to play with the data layouts based on what makes sense for the API.

Option 1

metadata1 drawio

Introduces two new structs:

  • IotaDocumentMetadata: holds all the metadata fields moved out of properties.
  • ResolvedIotaDocument (name TBD): holds the document, its metadata, and the fields obtained during resolution (messageId, previousIntegrationMessageId, previousDiffMessageId, and versionId).

Note that versionId in this case is assumed to be deterministic based on the number of integration chain and diff chain messages. E.g. 3.4 could mean the resolved document is the third integration chain message and had four diff chain updates merged in. Versioning DID documents could be descoped from this issue entirely.

Option 2

metadata2 drawio

Same as Option 1 except it introduces:

  • IotaIdentity (name TBD): convenience wrapper for IotaDocument and IotaDocumentMetadata. This is mainly for API ergonomics since operations like signing (creating a proof) and publishing need both the document and its metadata.

Option 3

metadata3 drawio

Same as Option 2 except the resolution fields are split out of ResolvedIotaIdentity into their own struct, IotaResolutionMetadata.

Option 4

metadata4 drawio

This tries to keep the metadata fields as close to the specification as possible, notably that the versionId field is part of IotaDocumentMetadata rather than IotaResolutionMetadata. This is possible if we do not derive the version from the integration and diff chains but instead let the user set it manually.

This option has the disadvantage of not having a wrapping struct for the (IotaDocument, IotaDocumentMetadata) combination needed for signing and publishing.

Unresolved questions:

  • How does this refactor affect diff updates? Does the diff contain both metadata and document diffs separately or can they be part of the same nested structure? Do any of the options affect or restrict diff updates differently?

@olivereanderson
Copy link
Contributor

If keeping the versionId field in IotaDocumentMetadata does not cause trouble, then I would prefer to go with Option 4 as it is closer to the spec than any of the others. In comparison with Option 3 one does not loose very much ergonomics due to Rust's destructuring capabilities, and one could also add a method to ResolvedIotaDocument that groups the IotaDocumentMetadata with the IotaDocumentMetadata.

@cycraig
Copy link
Contributor Author

cycraig commented Nov 15, 2021

I agree that Option 4 looks most similar to the DID specification but the spec also notes that, with reference to resolve and resolveRepresentation: These functions each return multiple values, and no limitations are placed on how these values are returned together. Link: https://www.w3.org/TR/did-core/#did-resolution-metadata (only affects ResolvedIotaDocument).

Note that theIotaResolutionMetadata struct still does not match the specification since it does not include a contentType or error field, but I think it's fine to deviate here for ergonomics and then return a spec-compliant resolutionMetadata struct if needed for a spec-compliant resolver. Unless we want to rename IotaResolutionMetadata to something else to avoid confusion?

I think I'm most in favour of Option 3 due to the ergonomics of grouping the IotaDocument together with the IotaDocumentMetadata in IotaIdentity? On reflection I don't think separating them would be too bad though (i.e. Option 4). Note that any alternative involving tuples does not translate to wasm_bindgen at all.

@PhilippGackstatter
Copy link
Contributor

Options

I think having a wrapper for document and metadata would be nice to have, for the account at least. That wrapper would be what's cached in storage. Also, for allowing low-level access, it'd be nice to return that wrapper to users, so they can modify document and metadata at the same time.

At least in the account, I don't think the ResolvedIotaDocument wrapper is really needed. During synchronization, the resolution metadata would be used to update the local chain state (previous int and diff message ids) and then discarded. Not sure if users would care about that metadata either?

So I think I'm slightly in favor of Option 3 over 4, but as long as there is a wrapper for doc + metadata, I think it's fine either way.

Naming

The IotaIdentity type might clash with a (potential!) future rename of the account to Identity or even IotaIdentity. Does it make sense to have an IotaDocument without the metadata? Maybe internally, but then that internal type could be named accordingly ProtoIotaDocument or whatever, and eventually combined with metadata to produce a proper IotaDocument.

Diff Updates

As noted in #462, the identity_iota::did::doc::properties::Properties type doesn't implement Diff. It would be good for the new metadata types to implement Diff.

If we end up separating document and metadata, then it might be more consistent to keep them separated in the diff message, so they can be deserialized independently. Not a strong opinion.

Wasm

Regarding Wasm bindings: Would it not be possible to return a Rust tuple as an array? I think that's what is typically done for tuple-like return values in JS. I imagine it to be unergonomic to code, though.

It also seems fine to have tuple return types in Rust but introduce a wrapper for bindings only.

@cycraig
Copy link
Contributor Author

cycraig commented Nov 23, 2021

At least in the account, I don't think the ResolvedIotaDocument wrapper is really needed. During synchronization, the resolution metadata would be used to update the local chain state (previous int and diff message ids) and then discarded. Not sure if users would care about that metadata either?

I think the Account could potentially store the ResolvedIotaDocument (or IntegrationMessage below) instead of the chain state? Low-level API users need the information in the wrapper (integration_message_id and diff_message_id) to perform manual updates that extend a chain. So they need to access it somehow and the alternative of returning a tuple with the resolution metadata does not work for the wasm bindings.

Edit: the version is potentially quite important.

So I think I'm slightly in favor of Option 3 over 4, but as long as there is a wrapper for doc + metadata, I think it's fine either way.

I agree. A wrapper also appears necessary since several operations, particularly signing and verification, have to operate over both the document and its metadata now.

The IotaIdentity type might clash with a (potential!) future rename of the account to Identity or even IotaIdentity. Does it make sense to have an IotaDocument without the metadata?

I've settled on IotaMetaDocument (MetaDocument is already in identity-did) instead of IotaIdentity. I had not considered renaming the inner IotaDocument but would avoid doing so since it matches the concept of a DID Document in the DID specification.

Regarding Wasm bindings: Would it not be possible to return a Rust tuple as an array?

It's very awkward since by default a js_sys::Array is of type Array<any> in JavaScript/TypeScript. We would lose typing information since the elements in the array are different and hence have to be any. Basically I don't think this is a viable option for the wasm bindings. A struct wrapping the tuple elements (even if just for the Wasm bindings) is what we used previously before I refactored them out for parity.

Option 5

After more consideration and exploratory code, this is what I'm leaning towards. The separation of the resolution metadata fields seemed unnecessary and just added yet another struct to deal with, hence they are just part of the outer wrapper in this option.

This is similar to Option 2 except:

  • the metadata field names have been corrected
  • the outer wrapper is IntegrationMessage (open to suggestions since it could be merged with one or more diffs too)
  • the inner wrapper is IotaMetaDocument

metadata6 drawio

@cycraig
Copy link
Contributor Author

cycraig commented Dec 8, 2021

Option 6

I'm thinking about scrapping the code so far and going with a simpler design where the metadata lives directly in the IotaDocument.

This would be similar to how the metadata fields exist in the document properties currently, except they would be moved to alongside the CoreDocument instead of inside it (see the Current section in the first comment).

metadata7 drawio

Why? Simplicity and ergonomics.

  • One rarely wants to deal with a IotaMetaDocument in Option 5 instead of the IotaDocument itself, which contains all the interesting things, and with IotaTangleDocument there are two wrapping layers before I can get to what I actually want to use. There is also a somewhat awkward separation of things like document signature and verification functions between IotaMetaDocument and IotaDocument with some duplication and overlap.
  • One always has to keep the IotaDocument along with its metadata (and message ids in IotaTangleDocument) anyway in order to publish updates.
  • Avoids some more confusion with terms like IotaTangleDocument vs IotaMetaDocument vs IotaDocument vs CoreDocument vs DID Document in documentation.

The IotaTangleDocument and IotaDocumentMetadata still take some responsibilities and methods away from IotaDocument itself, so overall the code would be somewhat cleaner.

Disadvantages

  • Option 6 makes separating a document from its metadata more difficult, but does not necessarily violate the DID resolution specification as it states: the resolve function returns the DID document in its abstract form (a map). We could easily return a CoreDocument in that case with the IotaDocumentMetadata (or generic equivalent), and expose functions to try convert the combination back into an IotaDocument. Internally we don't need to make a distinction.

It is unclear how this impacts generalising the code for any future method specifications, although that's unclear right now regardless.

Please criticise this or raise any issues if you think this will lead to more problems or violate the DID core specification.

@JelleMillenaar
Copy link
Collaborator

I guess my biggest question is why we need an IotaTangleDocument, why can't the fields messageId, diffMessageId and versionId be moved into IotaDocumentMetadata, because that is what they are, right? Not according to the standard, but if we ever make a standard compliant resolver, it can just filter those out again. Is it because they are unsigned and derived instead of immutable properties from the Tangle?

@cycraig
Copy link
Contributor Author

cycraig commented Dec 9, 2021

Is it because they are unsigned and derived instead of immutable properties from the Tangle?

Yes.

Those fields technically can be moved but that causes issues with serialization (we can't and shouldn't store those fields on the Tangle because they don't exist when publishing and shouldn't be signed).

There needs to be a distinction between what fields are set and stored versus derived from the Tangle messages.

For instance we currently have this problem with IotaDocument::message_id, where we can't serialize it without losing message_id (previous problem when trying to transmit and display it as JSON in Javascript), but we can't include message_id during serialization without breaking things like signatures. The extra layer of IotaTangleDocument solves this problem and (hopefully) makes it clear that those fields are derived during resolution and not stored on the Tangle.

@cycraig cycraig mentioned this issue Dec 9, 2021
10 tasks
@cycraig cycraig linked a pull request Dec 9, 2021 that will close this issue
10 tasks
@cycraig cycraig added Enhancement New feature or improvement to an existing feature and removed Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog labels Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants