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

Describing presentation and clarifying its relationship to a credential. #217

Closed
wants to merge 8 commits into from

Conversation

lovesh
Copy link
Member

@lovesh lovesh commented Aug 16, 2018

Signed-off-by: lovesh harchandani lovesh.bond@gmail.com


Preview | Diff

</li>
<li>
The <a>subject</a> trusts the <a>issuer</a> to issue true (i.e., not false)
The <a>subject</a> trusts (since only the holder can verify) the <a>issuer</a> to issue true (i.e. not false)
Copy link
Contributor

Choose a reason for hiding this comment

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

add comma after i.e.

index.html Outdated
@@ -445,15 +445,17 @@ <h3>Claims</h3>

<p>
The data model for <a>claims</a> described above is powerful and can be used to
express a large variety of statements. For example, whether or not someone is
over the age of 21 may be expressed as follows:
express a large variety of statements. For example, a claim specifying the data of birth of a subject
Copy link
Contributor

Choose a reason for hiding this comment

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

data => date

index.html Outdated
</p>

<figure>
<img style="margin: auto; display: block;"
width="50%" src="diagrams/claim-example.svg">
<figcaption style="text-align: center;">
An example of a basic claim that expresses that Pat is over the age of 21.
An example of a basic claim that expresses that Pat's date of birth is 1534417926548 milliseconds from epoch.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use an ISO8601 dateTime string like other examples. Probably also just use schema.org "birthDate" property too. Also the new SVG is styled different than the other images and the subtext is wrapped oddly.

Copy link
Member Author

Choose a reason for hiding this comment

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

The dateTime string is intentionally formatted since the exact encoiding of the value is defined in a schema.

Copy link
Member Author

@lovesh lovesh Aug 22, 2018

Choose a reason for hiding this comment

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

Updated the SVG but it is still different from some other SVGs since i could not edit those SVGs. Any recommended editors?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an introductory document. I think it should be using easy to understand examples. We should avoid an arbitrary UNIX timestamp that requires an explanation in both the caption and graphic. Yes, that is a possible value, but it seems like something that could be in a more advanced example. In this case using https://schema.org/birthDate with a readable ISO8601 date makes more sense. The value could just be "1985-10-26" without the subtext and people will likely understand without introducing concepts like the epoch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msporny: do you recall what tools created these figures? (I'm guessing google docs or inkscape)
@lovesh: The figures need to all match up with the same style. There's only a few figures, but if one changes style, they all should change. In addition to style, the figure after this one is combining the previous two. So if the ageOver part is changing, it should change in both places.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, Google Docs. Specifically, Google Draw. Originals are here: https://drive.google.com/drive/u/1/folders/0BwQ7X2CScoA-emRfSmVfSU4xNTg

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidlehn Ok, i will update the birthDate to show ISO8601 format and clarify that format needs to be specified in the schema.

@lovesh lovesh changed the title Core data model Describing presentation and clarifying its relationship to a credential. Aug 22, 2018
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.

There are a number of items that are problematic with this PR. I've suggested some changes, let's get through those and see where we end up.

index.html Outdated
</li>
<li>
The <a>subject</a> trusts the <a>issuer</a> to issue true
The <a>subject</a> trusts (since only the holder can verify) the <a>issuer</a> to issue true (i.e., not false)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what "since only the holder can verify" means in this sentence.

Copy link
Member Author

Choose a reason for hiding this comment

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

We wanted to emphasize that the subject cannot verify the credential, no one apart from holder can. Subject can only verify if he is the holder

Copy link
Contributor

Choose a reason for hiding this comment

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

I reject this proposed change, because verification is nothing to do with this trust statement.
This trust statement is stating a fact, namely, that the subject trusts the issue to issue true statements about him or her. Period.
Verification is not within the scope of this sentence and is irrelevant.

</ol>

<p>
This trust model differentiates itself from other trust models by ensuring that:
This trust model differentiates itself from other trust models by ensuring that
the <a>issuer</a> does not need to trust (or even know) the <a>verifier</a>.
Copy link
Member

Choose a reason for hiding this comment

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

This text as in @dhh1128's PR, wasn't it? I feel like there are multiple sentences in this PR that were in the other PR.

index.html Outdated
express a large variety of statements. For example, whether or not someone is
over the age of 21 may be expressed as follows:
express a large variety of statements. For example, a claim specifying the date of birth of a subject
can be used to prove that its age lies in an interval or is greater or less than a some value which is
Copy link
Member

Choose a reason for hiding this comment

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

@dhh1128, this already went into the spec, didn't it?

We also need to use simpler language here to make the concepts more accessible to readers. Suggestion:

For example, a claim related to the date of birth of a subject is useful for use cases like proving that the subject is above the legal drinking age or able to get age-related discounts at retail stores. This data model enables the holder to have the flexibility to express their date of birth directly, or indirectly (e.g., under the age of 15, over the age of 65):

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been bikeshedding this over in #213 too.

index.html Outdated
credential, as well as metadata that describes properties of the credential
itself such as: the <a>issuer</a>, the expiry time, a representative image,
etc. A <a>verifiable credential</a> is a set of claims and metadata that are
same <a>entity</a>. It includes a holder generated almost-always unique identifier
Copy link
Member

Choose a reason for hiding this comment

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

This is not accurate, we have credentials that will use the issuer supplied identifier. The previous text covered this case... it says "may include an identifier", as in it's optional. I suggest we use the original text.

Copy link
Member Author

@lovesh lovesh Sep 4, 2018

Choose a reason for hiding this comment

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

Alright

index.html Outdated
etc. A <a>verifiable credential</a> is a set of claims and metadata that are
same <a>entity</a>. It includes a holder generated almost-always unique identifier
as well as metadata that describes properties of the credential itself such as: the <a>issuer</a>,
the public key to use, the revocation mechanism to use etc. This metadata can
Copy link
Member

Choose a reason for hiding this comment

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

"the public key to use" for what? Verification? Why would it specify this. I know the answer, but a reader that knows nothing about ZKPs will be confused by this language. It's also not clear that introducing this concept this early on is going to help the reader get a good grounding in the basic concepts before moving on to ZKP concepts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

index.html Outdated
same <a>entity</a>. It includes a holder generated almost-always unique identifier
as well as metadata that describes properties of the credential itself such as: the <a>issuer</a>,
the public key to use, the revocation mechanism to use etc. This metadata can
either be signed by the issuer or not, more on that later. The credential identifier
Copy link
Member

Choose a reason for hiding this comment

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

You want to avoid statements like "more on that later" in specifications as they don't give the reader a good understanding of where to go next. You should link to a specific section in the specification if you want to refer the reader to a more detailed explanation. However, the preference is to not require linking to a more detailed explanation and establish the concept fully without needing to point elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

index.html Outdated
as well as metadata that describes properties of the credential itself such as: the <a>issuer</a>,
the public key to use, the revocation mechanism to use etc. This metadata can
either be signed by the issuer or not, more on that later. The credential identifier
is never revealed to the verifier to prevent the issuer and verifier from
Copy link
Member

Choose a reason for hiding this comment

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

This is not accurate. In some cases, the credential identifier is revealed because you are operating in a non-pseudonymous mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

index.html Outdated
either be signed by the issuer or not, more on that later. The credential identifier
is never revealed to the verifier to prevent the issuer and verifier from
colluding to correlate the holder.
A <a>verifiable credential</a> is a set of claims and meta data that are
Copy link
Member

Choose a reason for hiding this comment

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

Again, @dhh1128 -- didn't this go in?

index.html Outdated
</p>

<ul>
<li>
The <a>issuer</a> and the <a>verifier</a> do not need to trust the
Copy link
Member

Choose a reason for hiding this comment

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

This is an important part of the VC Data Model... we can't just remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added back

@dlongley
Copy link
Contributor

We need to figure out what's going on between this PR and #213 ... there's a lot of overlap. Are they meant to be based on one another?

@dlongley
Copy link
Contributor

dlongley commented Sep 4, 2018

This PR needs rebasing now that #213 has been merged.

index.html Outdated
express a large variety of statements. For example, a claim specifying the date of birth of a subject
can be used to prove that its age lies in an interval or is greater or less than a some value which is
needed in use cases like proving drinking age or teenager discount or elderly discount;
the holder has the flexibility to use the claim in any way that is applicable to that claim:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point of this statement (before the edit) was to indicate that an unbounded number of claims can be made using this model. It was not about how, for a given claim, additional statements can be proved. In fact, that is not a property of the model at all, but a property of a certain technology that can be used on top of the model (the model enables such technologies but does not provide the tech itself, it's just a model).

Therefore, we should change this section back to what it said before -- and then move these new edits just below the old text and talk about this additional powerful feature that is also compatible with (builds on top of) the model. So something like:

Additionally, selective disclosure schemes may also use claims expressed
in this model to prove additional statements about those claims. For example,
a claim specifying the date of birth of a subject can be used to prove that its
age lies...

index.html Outdated
set of claims and meta data that are tamper-evident and that cryptographically
prove who issued it.
The credential identifier should not be revealed to the verifier if the holder
wants to avoid coorelation. This metadata may be signed by the issuer.
Copy link
Contributor

Choose a reason for hiding this comment

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

coorelation => correlation

I'm not sure if this is best said this way. It suggests only one of the two anti-correlation mechanisms and may fool the reader into thinking that they won't be correlated if they merely omit a credential identifier. Perhaps:

Credential identifiers can be used for correlation. A holder that wants to help
minimize correlation should consider using a selective disclosure scheme that
does not reveal the credential identifier and/or a terms of use policy requiring
the verifier not to correlate their information with any third parties.

It seems like this sort of statement should be in the privacy section, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc: @msporny ... thoughts on where this statement should go?

@burnburn
Copy link
Contributor

burnburn commented Oct 18, 2018

What is the status of this PR? Hard to tell from the spate of commits and outstanding reviews. A summary would be nice in case we are able to discuss at TPAC.

@brentzundel
Copy link
Member

Discussed at TPAC.
General consensus for the changes.
Will be ready for review after modifying one of the images.

@msporny
Copy link
Member

msporny commented Oct 30, 2018

@msporny to review and pull in after Nov 1st.

@msporny
Copy link
Member

msporny commented Nov 1, 2018

LGTM, modulo some minor editorial nits that will be caused by other PRs. I'll clean those up in another pass. Please resolve conflicts and we'll pull this in.

@msporny
Copy link
Member

msporny commented Nov 7, 2018

I'll try to do the rebase/merge ... it'll result in this branch being closed w/o merge, but a new PR will take it's place that will merge the changes in.

@brentzundel
Copy link
Member

If you've got time for a quick rebase tutorial, I'm happy to attempt it 😄
On the non-editorial view, github only says "This branch has no conflicts with the base branch"
There isn't any indication that rebasing is necessary.

@msporny
Copy link
Member

msporny commented Nov 7, 2018

If you've got time for a quick rebase tutorial, I'm happy to attempt it 

The only one I know involves eating glass and walking on nails... it's always a painful process... but if you want to give it a shot, try this one:

git checkout gh-pages
git pull
git checkout anoncred-3
git rebase gh-pages

There will be a number of conflicts that you will have to merge... there may be some squashing you need to do via git rebase -i

On the non-editorial view, github only says "This branch has no conflicts with the base branch"
There isn't any indication that rebasing is necessary.

Strange... it says this for me:

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

@brentzundel
Copy link
Member

Does it need to be rebased before merging? I thought there was a "Squash and merge" option.

lovesh and others added 8 commits November 7, 2018 14:37
Signed-off-by: lovesh harchandani <lovesh.bond@gmail.com>
Signed-off-by: lovesh harchandani <lovesh.bond@gmail.com>
Signed-off-by: lovesh harchandani <lovesh.bond@gmail.com>
Signed-off-by: Brent <brent.zundel@gmail.com>
Signed-off-by: Brent <brent.zundel@gmail.com>
Signed-off-by: Brent <brent.zundel@gmail.com>
Signed-off-by: Brent <brent.zundel@gmail.com>
Signed-off-by: Brent <brent.zundel@gmail.com>
@brentzundel
Copy link
Member

attempted rebasing (yum broken glass). Let me know if there is more to do.

@davidlehn
Copy link
Contributor

That rebase left a bunch of garbage in the PR. Some of those commits should probably just be backed out and a rebase done again. All that .idea junk needs to be removed from the history. And the current diff has lots of merge conflict markers in it. Hard to tell which changes are intended. This may require a git expert to get involved.

@msporny
Copy link
Member

msporny commented Nov 9, 2018

Merged via #271 ... closing.

@msporny msporny closed this Nov 9, 2018
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

7 participants