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

keys in did documents should list controller instead of owner #33

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

ugoamanoh
Copy link
Contributor

What's Here

This resolves the issue #29

  • Updated the owner key in DIDDocument to controller as stated in the DID spec
  • Updated existing tests

Testing

Rund the entire test suite using the command ./gradlew test cC --no-parallel

@codecov
Copy link

codecov bot commented Nov 5, 2019

Codecov Report

Merging #33 into develop will increase coverage by 0.69%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop      #33      +/-   ##
=============================================
+ Coverage      72.69%   73.38%   +0.69%     
- Complexity       181      182       +1     
=============================================
  Files             24       24              
  Lines            868      868              
  Branches         145      145              
=============================================
+ Hits             631      637       +6     
+ Misses           159      152       -7     
- Partials          78       79       +1
Impacted Files Coverage Δ Complexity Δ
.../main/java/me/uport/sdk/ethrdid/EthrDIDResolver.kt 81.76% <100%> (ø) 21 <0> (ø) ⬇️
...n/java/me/uport/sdk/jwt/test/EthrDIDTestHelpers.kt 94.73% <100%> (ø) 1 <0> (ø) ⬇️
...ava/me/uport/sdk/uportdid/UportIdentityDocument.kt 72.09% <100%> (ø) 4 <0> (ø) ⬇️
...main/java/me/uport/sdk/universaldid/DIDDocument.kt 64.1% <100%> (ø) 0 <0> (ø) ⬇️
jwt/src/main/java/me/uport/sdk/jwt/JWTTools.kt 85.02% <0%> (+3.59%) 59% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc507c2...982704c. Read the comment docs.

@mirceanis mirceanis self-requested a review November 5, 2019 12:58
Copy link
Collaborator

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

This is a good start towards adhering to the latest W3C definitions but there are also existing DIDs out there that would fail with this proposed version.

Real-world existing web dids will crash with this change:

@Test
fun `can resolve real web did`() {
    val doc = WebDidResolver().resolve("did:web:uport.me")
}

Since the existing testing data was also changed, the current tests don't reflect real world behavior.
We should have a test to support parsing of older-style DIDs. This test should hardcode an old format DID doc and verify that it is resolved properly.

This problem will manifest for web-did and uport-did documents since they are stored somewhere and not generated at runtime like ethr-did

@mirceanis
Copy link
Collaborator

Adapting to legacy DID doc formats is tricky without support from the serialization lib.

This PR should make it easier to completely cover the above cases:
Kotlin/kotlinx.serialization#597

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants