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

Add version_id support #21

Merged
merged 42 commits into from
Jan 11, 2024
Merged

Conversation

tsachiherman
Copy link
Contributor

@tsachiherman tsachiherman commented Jan 2, 2024

What ?

This PR add the support for version_id as part of the resolver implementation.

More details

The DID document specifications is described https://github.com/decentralized-identity/ethr-did-resolver/blob/master/doc/did-method-spec.md.
The changes here convert the return type from DidDocument into a DidResolutionResult.
The difference is this:

pub struct DidResolutionResult {
    #[serde(
        default,
        rename = "didDocumentMetadata",
        skip_serializing_if = "Option::is_none"
    )]
    pub metadata: Option<DidDocumentMetadata>,
    #[serde(
        rename = "didResolutionMetadata",
        skip_serializing_if = "Option::is_none"
    )]
    pub resolution_metadata: Option<DidResolutionMetadata>,
    #[serde(rename = "didDocument")]
    pub document: DidDocument,
}

So that a DidResolutionResult provides additional information regarding the document. In particular, it provides versioning information.

To take advantage of that, the resolver also support requesting a particular version. That version can be provided as the second argument :

> curl -H "Content-Type: application/json" -d '{"id":1, "jsonrpc":"2.0", "method": "did_resolveDid", "params": ["0x423B6F365C14F4b233928088FF937e01c143A328","4930857"]}' http://localhost:9944 | jq

result by

{
  "jsonrpc": "2.0",
  "result": {
    "didDocumentMetadata": {
      "deactivated": false,
      "versionId": 4930857,
      "updated": "2023-12-21T15:43:00Z",
      "nextVersionId": 4930891,
      "nextUpdate": "2023-12-21T15:50:48Z"
    },
    "didResolutionMetadata": {
      "contentType": "application/did+ld+json"
    },
    "didDocument": {
      "@context": [
        "https://www.w3.org/ns/did/v1",
        "https://w3id.org/security/suites/ed25519-2020/v2"
      ],
      "id": "did:ethr:0x423b6f365c14f4b233928088ff937e01c143a328",
      "verificationMethod": [
        {
          "id": "did:ethr:0x423b6f365c14f4b233928088ff937e01c143a328#controller",
          "controller": "did:ethr:0x423b6f365c14f4b233928088ff937e01c143a328",
          "type": "EcdsaSecp256k1VerificationKey2019",
          "blockchainAccountId": "0x423b6f365c14f4b233928088ff937e01c143a328"
        }
      ]
    }
  },
  "id": 1
}

@tsachiherman tsachiherman self-assigned this Jan 2, 2024
@tsachiherman tsachiherman linked an issue Jan 2, 2024 that may be closed by this pull request
@tsachiherman tsachiherman added the enhancement New feature or request label Jan 2, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2024

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (749f7c1) 91.79% compared to head (a94ecaf) 91.25%.

Files Patch % Lines
src/lib.rs 0.00% 13 Missing ⚠️
src/resolver.rs 94.94% 5 Missing ⚠️
src/types/ethr.rs 60.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
- Coverage   91.79%   91.25%   -0.54%     
==========================================
  Files          11       11              
  Lines        1194     1292      +98     
==========================================
+ Hits         1096     1179      +83     
- Misses         98      113      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tsachiherman tsachiherman marked this pull request as ready for review January 8, 2024 21:31
Copy link
Contributor

@insipx insipx 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, but you got a bunch of clippy suggestions you should go through,

cargo clippy

src/lib.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jac18281828 jac18281828 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. One note about process, you should be able to directly create branches in the xmtp/didethresolver repo and PR from there. Let me know if GitHub permissions forced you to fork the repo. Your GitHub user may not be in the right group. IT should be able to fix that.

@tsachiherman
Copy link
Contributor Author

Looks good to me. One note about process, you should be able to directly create branches in the xmtp/didethresolver repo and PR from there. Let me know if GitHub permissions forced you to fork the repo. Your GitHub user may not be in the right group. IT should be able to fix that.

That was done very much on purpose, so that we won't "spam" the public repository with intermediate branches. Creating shared branches are good practice when we have multiple individuals contributing to the same branch, but aren't really designed for scalability. ( i.e. it would add the role of "cleanup officer" ).

That's the reason why most large organizations that promotes external contributions would allow PR to be created only from a fork.

src/resolver.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
src/resolver.rs Outdated Show resolved Hide resolved
}
Ok(())
Ok(deactivated)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this result. Is deactivated successful or failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's neither. The deactivation is separate from the error.

Copy link
Contributor

@jac18281828 jac18281828 left a comment

Choose a reason for hiding this comment

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

Generally looking good to me! I made one note but thanks for cleaning up the warnings!

Copy link
Contributor

@insipx insipx left a comment

Choose a reason for hiding this comment

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

much cleaner and nice job!

@tsachiherman tsachiherman merged commit a7a8510 into xmtp:main Jan 11, 2024
2 checks passed
@tsachiherman tsachiherman deleted the tsachi/add_version_id branch January 11, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Handle version_id during building of DID Document
5 participants