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

Moves deterministic encoded CBOR language back to CBOR section #586

Closed
wants to merge 4 commits into from

Conversation

jonnycrunch
Copy link
Contributor

@jonnycrunch jonnycrunch commented Jan 28, 2021

This PR undoes the editorial changes to get us back on having the discussion regarding Deterministically Encoded CBOR back to the conversations.

The language builds on the updated CBOR spec RFC #8949


Preview | Diff

Comment on lines +3101 to +3103
To produce a deterministic canonical CBOR representation of a DID document and faciliate maximal lossless compatiblity with other
core representations via the Abstract Data Model the following constraints of a CBOR representation of a DID Document model MUST be followed:

Copy link
Member

Choose a reason for hiding this comment

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

typos and line lengths

Suggested change
To produce a deterministic canonical CBOR representation of a DID document and faciliate maximal lossless compatiblity with other
core representations via the Abstract Data Model the following constraints of a CBOR representation of a DID Document model MUST be followed:
To produce a deterministic canonical CBOR representation of a DID
document and facilitate maximal lossless compatibility with other core
representations via the Abstract Data Model the following constraints
of a CBOR representation of a DID Document model MUST be followed:
</p>

Comment on lines +3105 to +3110
<li>Property names MUST be represented as text string (major type 3) and contain only UTF-8 strings. </li>
<li>Undefined Values of Required Properties as defined in <a href="#data-model">the Data Model</a> that are absent from the CBOR representation SHOULD be labeled with Primitive type (major type 7) with value 23 (Undefined value). </li>
<li>Property names in each CBOR map MUST be unique. </li>
<li>Integer encoding MUST be as short as possible.</li>
<li>The expression of lengths in CBOR major types 2 through 5 MUST be as short as possible. </li>
<li>The keys in every map must be sorted lowest value to highest. Sorting is performed on the bytes of the representation of the keys. If two keys have different lengths, the shorter one sorts earlier. If two keys have the same length, the one with the lower value in (byte-wise) lexical order sorts earlier. </li>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li>Property names MUST be represented as text string (major type 3) and contain only UTF-8 strings. </li>
<li>Undefined Values of Required Properties as defined in <a href="#data-model">the Data Model</a> that are absent from the CBOR representation SHOULD be labeled with Primitive type (major type 7) with value 23 (Undefined value). </li>
<li>Property names in each CBOR map MUST be unique. </li>
<li>Integer encoding MUST be as short as possible.</li>
<li>The expression of lengths in CBOR major types 2 through 5 MUST be as short as possible. </li>
<li>The keys in every map must be sorted lowest value to highest. Sorting is performed on the bytes of the representation of the keys. If two keys have different lengths, the shorter one sorts earlier. If two keys have the same length, the one with the lower value in (byte-wise) lexical order sorts earlier. </li>
<li>
Property names MUST be represented as text string (major type 3) and
contain only UTF-8 strings.
</li>
<li>
Undefined Values of Required Properties as defined in <a
href="#data-model">the Data Model</a> that are absent from the CBOR
representation SHOULD be labeled with Primitive type (major type 7)
with value 23 (Undefined value).
</li>
<li>
Property names in each CBOR map MUST be unique.
</li>
<li>
Integer encoding MUST be as short as possible.
</li>
<li>
The expression of lengths in CBOR major types 2 through 5 MUST be as
short as possible.
</li>
<li>
The keys in every map must be sorted lowest value to highest. Sorting
is performed on the bytes of the representation of the keys. If two
keys have different lengths, the shorter one sorts earlier. If two
keys have the same length, the one with the lower value in (byte-wise)
lexical order sorts earlier.
</li>

line lengths

@iherman
Copy link
Member

iherman commented Jan 28, 2021

@jonnycrunch did you use a different github persona than the one you registered with the WG? Otherwise I don't know why this PR wasn't accepted.

I will set this as editorial, which will release the lock, but you should check, please. Thx

@w3cbot
Copy link

w3cbot commented Jan 28, 2021

iherman marked as non substantive for IPR from ash-nazg.

@msporny
Copy link
Member

msporny commented Jan 31, 2021

@jonnycrunch, I'm confused by this PR. The text was already moved back in this commit: 5b7c489 which was announced to the WG here: https://lists.w3.org/Archives/Member/member-did-wg/2021Jan/0002.html

Is there some difference between the text in the spec and what you're proposing in the PR?

@brentzundel
Copy link
Member

@jonnycrunch I believe the changes you are recommending here have already been made.

@msporny msporny added the do not merge Do not merge - waiting on resolution to issue label Feb 1, 2021
Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

make no mention on canonical, based on the comments from the call... prefer to not see that language / think this should be closed.

@msporny
Copy link
Member

msporny commented Feb 3, 2021

@brentzundel wrote:

@jonnycrunch I believe the changes you are recommending here have already been made.

I checked a few days ago and checked again today per @jricher's request -- the changes in this PR were merged into the specification before this PR was raised. I did a line-by-line comparison.

There are wording differences due to a refactoring of language that @peacekeeper applied to the spec wrt. ADM entry keys, etc. This PR undoes some of those changes. The group also discussed this PR during the special topic call today and decided to close it.

For those reasons, this PR is marked pending close and will be closed in 7 days or when @jonnycrunch confirms that we can close this PR, whichever event happens sooner.

@msporny msporny added the pending close Issue will be closed shortly if no objections label Feb 3, 2021
@msporny
Copy link
Member

msporny commented Feb 10, 2021

This PR is marked pending close and will be closed in 7 days or when @jonnycrunch confirms that we can close this PR, whichever event happens sooner.

7 days have elapsed since the 'pending close' tag was added with no response from @jonnycrunch, closing.

@msporny msporny closed this Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge - waiting on resolution to issue pending close Issue will be closed shortly if no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants