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

Fix substantive issues related to Representations intro section. #644

Merged
merged 3 commits into from
Feb 17, 2021

Conversation

msporny
Copy link
Member

@msporny msporny commented Feb 15, 2021

This PR cleans up the introductory portion of the Representations section by 1) providing a non-normative background to what representation/production/consumption is, 2) groups the normative statements into 3 groups -- all representations, all conforming producers, and all conforming consumers, 3) downgrades a MAY statement that wasn't easily testable but keeps the language around to make the intent clear. I tried to keep the essence of the section intact, but did end up modifying a few normative statements... so this PR makes substantive changes while trying to preserve what the section was doing.


Preview | Diff

@msporny msporny added the substantive This is a substantive change to the specification. label Feb 15, 2021
Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

I'm mostly fine with this (minus one small change in the beginning).

I still think it's super confusing to talk about "representation-specific entries" in a data model that's supposed to be abstract. But I like how this PR makes the section easier to read and understand.

Not sure what you meant by "downgrades a MAY statement that wasn't easily testable", I couldn't find that.

index.html Outdated
Comment on lines 2476 to 2477
The rules in this section apply to all implementations seeking to be compatible
with independent implementations of the specification. Deployments of this
Copy link
Contributor

Choose a reason for hiding this comment

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

This language sounds funny to me, not sure exactly what it means :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was language that @jricher created, I believe... I think what he's saying is "If you are writing an implementation, and you want to be compatible with other implementations, these rules apply to you."

I do agree that the sentence is hard to read and that we don't really need it. Let's see if @jricher has alternative language, and if not, I can come up with something (or just remove the language, since it's fairly obvious that the rules are there in order to achieve interoperability).

@msporny msporny force-pushed the msporny-cr-representations-intro branch from 0b2c29c to 1a2b6e1 Compare February 16, 2021 15:53
@msporny msporny force-pushed the msporny-cr-representations-intro branch from 1a2b6e1 to d299524 Compare February 16, 2021 15:53
@msporny
Copy link
Member Author

msporny commented Feb 16, 2021

I'm holding this PR until we can resolve @peacekeeper's ADM serialization concern in #644 (comment)

serializations as long as the <a>consumption</a> process back into the <a
href="#data-model">data model</a> is lossless. For example, some CBOR-based
<a>representations</a> express <a>datetime</a> values using integers to
represent the number of seconds since the Unix epoch.
Copy link
Contributor

@jonnycrunch jonnycrunch Feb 16, 2021

Choose a reason for hiding this comment

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

some CBOR-based representations express datetime values using integers

I don't believe this is correct. We constrained the CBOR datetime to major type 3, which is string. I think this type of extensibility could be made in the DID-Spec Registries, not in Core.

Copy link
Member Author

@msporny msporny Feb 17, 2021

Choose a reason for hiding this comment

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

The decision for the Vanilla CBOR datetime does not have anything to do with other CBOR-based media types. The CBOR-LD encoding of datetime uses an integer, which is why this clarification exists. Note the use of the word "some", not "all".

@iherman
Copy link
Member

iherman commented Feb 16, 2021

This issue was discussed in a meeting.

  • No actions or resolutions
View the transcript 6.1. resolution testing
See github pull request #644.
Manu Sporny: one issue with the resolution section is that it is abstract so we are still trying to figure out how to make it concrete enough to test
… one approach is to say that the metadata structure that represents the abstract data model can be serialized so that it can then be translated into various representations
… in order to do that we are suggesting a change to the abstract data model
… up to this point it has only been about DID documents
… but we are now considering making the abstract data model can represent other data structures that may be needed to create a specific representation
… and then you can use the representation production rules in order to create a concrete representation
… Markus and Justin have expressed some reservations about this
… so we are open to discussing it
Markus Sabadello: I understand the motivation to make things more testable…
… but the intention was to make the DID resolution section abstract because concrete resolution protocols were out of scope
… so it can be confusing to say that the abstract data model can be used for other things besides DID documents
… so Markus would prefer that the abstract data model stays focused on DID documents and not include metadata structures
… since metadata structures are “really something else”
Brent Zundel: We have repeatedly used “testable” to be machine testable. However “testable” does not have to mean machine-testable.
Justin Richer: What concerns me about this proposal is that it is “leaking requirements” of the test suite into the specification
… not all of the requirements of the test suite need to be represented in the spec
… for example, in the resolution spec we agreed to define “contracts”
… that does not mean that the core specification needs to go into this level of detailing
… and it does not mean the spec needs to change its nature to fit the needs of an implementation
… this is dangerous and backwards
Markus Sabadello: In the section on metadata structures, we defined them the same way as the abstract data model where we use Infra and data maps
… so we do have examples for how this abstract data structure that could be represented in JSON
… so we could add some examples that show how the abstract data model can be made machine testable
Brent Zundel: Roughly the two approaches are either we need to describe how to make concrete the abstract parts of the spec in order to make them machine-testable…
… or we alter the test suite in order to machine-test those statements
Manu Sporny: The issue we are in is that the group defined resolution in the abstract…
… but when the language was written for the resolution section, we used a whole bunch of normative MUST statements
… then we decided that we should have machine-testing of normative MUST statements
… so now we have a resolution section that needs testing
… so why don’t we focus on the actual language of the PRs
… there are two proposals: make the normative statements testable OR
… put the logic for testing into the test suite and say that is good enough
… we need more input from the WG about which direction we should go
… and see what specific language we can agree on
… for example, Markus suggests that we can provide examples of how the abstract data model can be serialized
Brent Zundel: Let’s move in the direction of looking at the PR text
Orie Steele: where is the list of normative requirements.
Jonathan Holt: Adding this detail this late in the game is not a good idea - it can be handled in the DID Spec Registries
… I added an example for CBOR that uses date time stamps using vanilla CBOR
… so I added that detail to DID Spec Registries instead
Markus Sabadello: We should really look at the concrete impacts. I think this is a big change.
… I understand the motivations, but I consider the metadata to be very different than the DID document data
… so keeping the abstract data model focused on the DID document is more precise
… it is not helpful to use the abstract data model for metadata
… we defined the abstract data model for DID documents, not for other metadata
Orie Steele: why is the type of did document an infra map… if its not an infra map?
Markus Sabadello: the metadata has some similarities, but it will be handled differently than the DID document data
Justin Richer: +1 to markus_sabadello
Markus Sabadello: my proposal is to expand the examples of what the metadata would look like in JSON
… and then make it testable in the test suite
Manu Sporny: I would like people to look at the text that is under debate. WG members should read the PR.
… I disagree with what the abstract data model is supposed to do.
… it should apply to everything in the specification
… so from my perspective, the ADM was supposed to be for any data structure specified in DID Core
… for those who are saying that the ADM should not apply, I’d like to know why we are “creating this other world” that needs to be respecified in the same way we define the ADM
… but we may just want to move on to other PRs at this point as we need the rest of the WG to engage on this issue
… but not deciding about this issue is going to slow down finishing this section
Justin Richer: to me, the ADM was always meant to be only for DID Documents. that we use the same definitions for other data structures don’t mean they’re the same
Brent Zundel: Note that the special topic call today will be a continuation of this discussion
Ivan Herman: Taking it to a higher level: originally the goal of resolution was defining a contract
… regardless of whether INFRA can be used to define these contracts, we must ultimately make these contracts machine-testable
… so it has come up several times in the past few weeks which relies on human reports of implementability
Justin Richer: +1 to ivan
Ivan Herman: and whether going to one extreme is the basis of the disagreement
Manu Sporny: I worry that we are speaking out of both sides of our mouth
… we should either agree that we’re not doing machine testability of the resolution section
Justin Richer: -1
Amy Guy: +1 DID resolution in a Note
Orie Steele: -1 to removing resolution… its 99% of what the spec is about…
Justin Richer: ORie: that’s why it got added in a year ago, it doesn’t make sense to talk about DIDs and Documents w/o talking about what’s in between
Drummond Reed: It feels like if we lock in on what the requirements in the spec are for the contract
… and if we say the contract is abstract, then maybe the way to test it can be described in the test suite
Justin Richer: +1 to drummond
Ivan Herman: +1 to what Drummond said - in addition, the presentation of things whereby having a normative statement must be backed up my machine-testing is not a requirement
… it is okay to have a normative statement that is not directly machine-testable
… we should be more liberal in that respect
Joe Andrieu: I think the interpreted rule that all normative statements much be machine-testable is too restrictive
… the definition of a contract is that it will say what you must do, and whether or not it is machine-testable is a different issue
Daniel Buchner: I strongly agree with Joe
Daniel Buchner: Folks, JOE AND I STRONGLY AGREE
Daniel Buchner: this means it is the right thing, obviously
Markus Sabadello: Based on what others are saying here, I want to ask: is it acceptable to have normative statements in the spec that are not directly machine testable without additional information…
… but that additional information is readily available in some additional documents
Brent Zundel: +1 to Markus
Markus Sabadello: for example in other protocol specification documents in the W3C Credentials Community Group or at the Decentralized Identity Foundation
Manu Sporny: I’m confused now as to whether we are testing DID resolution or not
… some are staying that we shouldn’t test it at all, vs. testing with some additional info that’s not in the spec
… I have been operating under the assumption that because there are so many normative statements in the resolution section, that we should make it all machine testable
Ivan Herman: First, to answer to Markus, I’m not sure I fully understand the question
… the implementation of the resolution contract should not depend on another spec or document
… the #1 question is whether the contract is fulfilled or not
… to reply to Manu, my view is that we have a contract definition. We can define “tests” that, for specific data, this is how the contract can be fulfilled and this is the response it should return
… but whether or not that is machine-testable is not required
… if it is machine-testable, that is good, but not required
Daniel Buchner: There are 3 classes of testability. 1) 100% testable via a local DID document. Fully deterministic.
… 2) the class of untestable language, such as “you must not use data describing a human”
Justin Richer: +1 to the coupling idea, that’s what I’ve been saying all along
Markus Sabadello: Can’t we use this in the test suite? https://w3c-ccg.github.io/did-resolution/#did-resolution-result
Orie Steele: I spent some time trying to implement the abstract data model this weekend and I’m starting to agree with some of what Justin and Markus are saying
… there is a class of tests that are not valuable to use as spec authors
… there probably needs to be a way to test what is in the spec without talking about a specific implementation
… this is challenging because it requires some concrete implementation of the ADM in the test suite
… anyone who says that we should not test testable statements as long as there is some way to test them is harming the spec
… the ADM is testable, but what’s hard is that the language around the ADM how to interpret it
… for example, if production works directly from the ADM, or does it take additional arguments
… so we have a problem here. There are people talking past each other.
… the solution is to gain clarity around the function signatures for the processes defined in the spec
Markus Sabadello: +1 to Orie that production is too vague right now
Orie Steele: then writing tests will be much easier
Jonathan Holt: Orie: +1, that was extremely well stated!
Daniel Buchner: We should test certain things with a default set of functions that implementers can put their own function bodies into them
Manu Sporny: Yes, +1 to Orie, that’s the problem.
Daniel Buchner: for ex: you can test against in-mem default values, but allow devs to replace the function with their own DB fetch function to get the same values
Daniel Buchner: that’s still a deterministic test

@msporny
Copy link
Member Author

msporny commented Feb 17, 2021

Substantive, multiple reviews, changes requested and made, objection raised by @peacekeeper and address via spec text change, no objections remain, merging.

@msporny msporny merged commit b83ff9f into main Feb 17, 2021
@msporny msporny deleted the msporny-cr-representations-intro branch February 21, 2021 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
substantive This is a substantive change to the specification.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants