Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Matching a Reference Value to Accepted Claims Set #107
Matching a Reference Value to Accepted Claims Set #107
Changes from 27 commits
e8ba70d
5b1fb20
c11ecb2
ad6b0cd
a8d5c59
522d7d3
b617b94
cc54304
d0140d4
204174e
d5e7b70
65415f4
b40acc7
8315842
6775eb1
29a82ad
e53ba32
cbc9511
a296701
184828d
6df72f1
10ab760
2086e9c
92f017f
65ae75b
6676808
51a51f2
5595121
3da016f
13f2368
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is not beautiful, but I am not convinced yours is better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the statements are correct. As of CDDL in the current spec, a Reference Value triple has one
environment
and corresponding onemeasurement-map
So considering this fact, I would re-word the statement asThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is reasonable to consider multiple triples having the same
environment-map
but using different triples to arrive at a set of measurements that belong to the environment. An internal representation would be described by an environment with multiple measurements.Overall, the reader is confused about what the internal representation is and how the Verifier processes it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use the word
triples
then. I was confused by the statement.I would re-phrase it then to:
Internally the Verifier can combine all the measurements belonging to the same environment together or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should have conventions for describing the internal representation without referring to specific CDDL / encodings. The challenge is doing this without being nebulous. Possibly, a strategy is to describe the internal representation by describing the CDDL that instantiated it. e.g., "...the environment (instantiated by
environment-map
)..."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this thread of comments require an issue to be raised before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. see issue #144
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need forward references to the "sub-sections below"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need forward reference to "below"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this change is not contentious, we can add the suggestion before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with me (obviously)