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

Restructure Core Properties section #345

Merged
merged 18 commits into from
Aug 3, 2020

Conversation

rhiaro
Copy link
Member

@rhiaro rhiaro commented Jul 12, 2020

Adjusts the nesting a bit and some of the subheadings for readability.

Adds a mini TOC to the beginning so that people looking for actual property names can find them quickly (as it's not always obvious from the subheadings in the actual TOC).

Adds dfn elements to all the properties, and corresponding internal refs. Also includes some CSS so links in code elements keep the same appearance.

Removes the publicKey property, and integrates remaining text into the Verification Methods section. Adds a subsection for key types and formats. I'd appreciate careful review of this in particular to make sure I didn't mix up wording around keys vs verification methods. I also add explicit values for type for each of the keys (where known; one is missing). I'm aware that support for some of these key types is still up for discussion, I just kept the list the same for now. Altering what appears in the table should be orthogonal.

There's a bit from the old Public Key section about embedding vs referencing public keys, with a little algorithm about how to find the key data in a DID doc. I updated to the language for this to apply to verification methods rather than only public keys. But I'm not sure if this is accurate. Possibly this whole bit is outdated and ought to be removed - advice appreciated.

Replaces publicKey with verificationMethod in all examples that use it.


Preview | Diff

@msporny
Copy link
Member

msporny commented Jul 28, 2020

There are still merge conflicts here, waiting for those to be resolved.

@rhiaro rhiaro force-pushed the rhiaro-restructure-core-properties branch from a807e46 to 1396daa Compare August 2, 2020 07:27
@rhiaro rhiaro force-pushed the rhiaro-restructure-core-properties branch from 1396daa to 38c28cd Compare August 2, 2020 07:32
Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

Largely editorial, I believe the shuffling of the normative text still reflects the consensus position of the group (which is subject to change in the following weeks, maybe). Given the size of the PR, we should merge it in quickly so that we can rebase others that have been waiting on this one to go in.

LGTM.

@msporny
Copy link
Member

msporny commented Aug 2, 2020

Need a review as quickly as you can get to it, please -- @dlongley @OR13 @kdenhartog @peacekeeper.

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Approving, but I left a couple of comments to be considered.

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
</li>
<li>
not use the <a>DID Document</a> at all to decide this, but have rules that are
"built into" the method.
</li>
</ul>

<p>
Each <a>DID method</a> MUST define how authorization is implemented, including
Copy link
Contributor

@Fak3 Fak3 Aug 3, 2020

Choose a reason for hiding this comment

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

Suggest more precise language here:

MUST define how authorization of operations performed on DID Document is implemented

Partially resolves #363

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @Fak3, could you note that proposed change in the related issue if not already? I'd like to note that this PR only moves text around and does not change anything (except to consolidate given new structure). Proposes for changes to text affected by this PR should be made in related issues or new PRs once this is merged.

@msporny
Copy link
Member

msporny commented Aug 3, 2020

Multiple reviews, changes requested and made, no objections, largely editorial, merging.

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

Successfully merging this pull request may close these issues.

None yet

4 participants