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.

Sign up for GitHub

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

Jump to bottom

Move DID Document proof outside metadata #728

Merged
merged 4 commits into from
Mar 18, 2022
Merged

Move DID Document proof outside metadata #728

merged 4 commits into from
Mar 18, 2022

Conversation

abdulmth
Copy link
Contributor

@abdulmth abdulmth commented Mar 17, 2022 •
edited
Loading

Description of change

Move the proof section in IotaDocument from inside its metadata to the root level.

Previous JSON:

"doc": {},
"meta": { 
  "proof": {},
  ... 
}

This PR:

"doc": {},
"meta": {},
"proof": {}

Links to any relevant issues

fixes issue #714.

Type of change

  • 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

Describe the tests that you ran to verify your changes.
Make sure to provide instructions for the maintainer as well as any relevant configurations.

@abdulmth abdulmth self-assigned this Mar 17, 2022
@abdulmth abdulmth added Wasm Related to Wasm bindings. Becomes part of the Wasm changelog 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. labels Mar 17, 2022
@abdulmth abdulmth marked this pull request as ready for review March 17, 2022 13:29
cycraig
cycraig suggested changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@cycraig cycraig left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The IOTA DID Method specification in the Wiki needs to be updated with the change, including its examples (as per the issue). Also link the issue to be closed by this PR.

@abdulmth
Copy link
Contributor Author

@cycraig yes, that's on my list, I opened this PR for review just to see if CI passes.

@abdulmth abdulmth added this to the v0.5 Features milestone Mar 17, 2022
@abdulmth abdulmth linked an issue Mar 17, 2022 that may be closed by this pull request
[Task] Move proof in IotaDocument outside of metadata #714
Closed
8 tasks
@abdulmth abdulmth requested a review from cycraig March 17, 2022 20:38
cycraig
cycraig suggested changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@cycraig cycraig 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. Only change required would be creating a DID Document with a real signature for the specification example.

bindings/wasm/src/did/wasm_document.rs Show resolved Hide resolved
documentation/docs/specs/did/iota_did_method_spec.md Outdated
Comment on lines 137 to 140
"proof": {
"type": "JcsEd25519Signature2020",
"verificationMethod": "#sign-0",
"signatureValue": "4pzMWzn19oqibHXqEdLr4EEHygs7QF2mMdvEhSMPiCVejEZGL4Vi5BGnmrJjMqKyNr6c6sSd3EXKoYxAuC2YiZNF"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please regenerate the example DID Document with a real signature, implementers may use these example as test vectors.

identity-iota-core/src/diff/diff_iota_document.rs Show resolved Hide resolved
identity-iota-core/src/diff/diff_iota_document.rs Show resolved Hide resolved
cycraig
cycraig reviewed Mar 17, 2022
View reviewed changes
Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Oh, should we remove the WasmDocument.signDocument function? Its only purpose was to enable signing other documents in Wasm since the proof location was different, it doesn't even perform any extra checks like signSelf does.

Also please check that signing and verifying documents in Wasm works as expected (i.e. something like doc2Signed = doc1.sign(doc2); doc1.verifyDocument(doc2Signed);).

@abdulmth
Copy link
Contributor Author

It's still good to keep the signDocument method since it takes a Document as parameter, which prevents the need of using fromJSON and toJSON calls if we would use signData instead.

PhilippGackstatter
PhilippGackstatter approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@PhilippGackstatter PhilippGackstatter 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!

@abdulmth abdulmth requested a review from cycraig March 18, 2022 09:13
cycraig
cycraig approved these changes Mar 18, 2022
View reviewed changes
Copy link
Contributor

@cycraig cycraig 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, thanks for the changes.

@cycraig cycraig changed the title Move proof outside metadata Move DID Document proof outside metadata Mar 18, 2022
@abdulmth abdulmth merged commit bc6e24d into dev Mar 18, 2022
@abdulmth abdulmth deleted the chore/move-proof branch March 18, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cycraig cycraig cycraig approved these changes

@PhilippGackstatter PhilippGackstatter PhilippGackstatter approved these changes

Assignees

@abdulmth abdulmth

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