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

Define serializations for Numbers, Dates, other properties #361

Closed
jricher opened this issue Jul 28, 2020 · 19 comments
Closed

Define serializations for Numbers, Dates, other properties #361

jricher opened this issue Jul 28, 2020 · 19 comments
Assignees

Comments

@jricher
Copy link
Contributor

jricher commented Jul 28, 2020

Infra doesn't define serializations for things like numbers and dates, so we should do that for metadata properties within the spec. Infra gives us strings and we can define how the string is parsed. This can be a simple set of recommendations like "use a decimal integer for integers" and "use an ISO date in XXX format".

These need to be recommendations that property definitions can refer to. Properties MUST define how they're parsed, though, so we need something to anchor on.

@OR13
Copy link
Contributor

OR13 commented Jul 28, 2020

@jricher
Copy link
Contributor Author

jricher commented Jul 29, 2020

JCS is probably not the best approach to handling this, but the points in the above are sound.

@jricher jricher removed their assignment Jul 29, 2020
@OR13
Copy link
Contributor

OR13 commented Jul 29, 2020

Is there some better way of defining how the JSON representations we discuss in the spec handle dates and numbers?... clearly we are having trouble using Infra to do that.

I'd love to have a way to clearer about what we mean when we say JSON.... otherwise the did spec would seem to just be ignoring these issues with JSON.... and we use JSON a lot :) JSON-LD is JSON.

@jricher
Copy link
Contributor Author

jricher commented Jul 30, 2020

This isn't about JSON, this is about strings within Infra.

JSON serialization of values is its own other thing entirely.

@msporny
Copy link
Member

msporny commented Jul 30, 2020

the did spec would seem to just be ignoring these issues with JSON

Which version of JSON subset do you mean?

https://tools.ietf.org/html/rfc7159
https://tools.ietf.org/html/rfc7493

:)

... and then there are issues wrt. converting to/from CBOR.

In any case, the thing Justin is asking for is hopefully a little simpler than solving all of the issues w/ converting from JSON-to-JSON across different architectures... 53-bit (Javascript) vs 64-bit (Python) environments, for example.

More of the conversation is happening over here (where we're talking about just numbers... not Dates):

whatwg/infra#87

@OR13
Copy link
Contributor

OR13 commented Aug 3, 2020

Either way, DID Core should not use a term like "JSON" or "INFRA" without warning about these issues.

@iherman
Copy link
Member

iherman commented Sep 8, 2020

This is also related to #379

@jonnycrunch
Copy link
Contributor

similar concern for ordered map that I brought up before. https://infra.spec.whatwg.org/#maps defines ordered map, but doesn't specify HOW it is ordered to make it deterministic. I, myself, like the option with json<-->dagCBOR that is in https://tools.ietf.org/html/rfc7049.

@jonnycrunch
Copy link
Contributor

copied over from issue #435,

I've finally unraveled the problems regarding representing numbers in the Abstract Data Model and we will need to discuss this more. While we currently don't have properties in the DID document that rely on numbers, we haven't yet gotten to consensus regarding how to unambiguously represent them in the abstract data model and this will lead to errors in implementations especially across core representations.

In particular, JavaScript number system treats all numbers as floating point, which may result in silent loss of precision in decoding integers with more than 53 significant bits.

In comparison, a CBOR-based core representation of the DID document could represent floating-point numbers in any of the three formats (half-precision, single-precision, and double-precision).

In the CBOR representation section I attempted to constrain the model to only double-precision even for integral values. Despite this we still have a disconnect with the CBOR able to represent 64 bits of significance compared to JSON's 53 bits.

@msporny
Copy link
Member

msporny commented Nov 3, 2020

Yes, what @jonnycrunch says above has always been the issue with Numbers (and is why INFRA struggles with putting anything specific in there).

All that said, we have serializations for Datetimes in the spec already, but they need to be pulled out into a Datetime that the entire spec can use. The same for Numbers. These are easy enough to do and I'll take an action to write the spec text that builds on top of INFRA to do that.

@msporny msporny added pre-cr-p1 ready for pr Issue is ready for a PR labels Nov 3, 2020
@rhiaro rhiaro added pr exists There is an open PR to address this issue and removed ready for pr Issue is ready for a PR labels Nov 16, 2020
@rhiaro
Copy link
Member

rhiaro commented Nov 16, 2020

#455 cover this

@rhiaro
Copy link
Member

rhiaro commented Nov 23, 2020

#455 was merged, okay to close? needs more about serializations, not pending close yet, sorry!

@rhiaro rhiaro added pending close Issue will be closed shortly if no objections ready for pr Issue is ready for a PR and removed pr exists There is an open PR to address this issue pending close Issue will be closed shortly if no objections labels Nov 23, 2020
@msporny
Copy link
Member

msporny commented Nov 24, 2020

This will be addressed by PR #455, #469 (and similar PRs for each representation section). We can close this issue when all of those PRs have been merged.

@jonnycrunch
Copy link
Contributor

are we going to handle bigInt and bigFloat for numbers?

@jonnycrunch
Copy link
Contributor

are per my understanding from today's call, the answer is no, there aren't many use cases that require support for this. In certain methods that use large EC numbers they are represented as base64 and would translate into a byte array in CBOR.

@dlongley
Copy link
Contributor

dlongley commented Dec 8, 2020

Do we need a distinction on datetimes that covers the level of time precision (for example, seconds vs. milliseconds)? It may be challenging/not possible to parse to the ADM and then produce a representation out of it without losing the ability to verify digital signatures if the precision isn't known. People will have to create workarounds for these cases or just avoid the ADM if it cannot losslessly keep track of the level of precision used.

It may be that those DID methods that are looking to sign DID Documents should only sign wrappers/envelopes containing DID Documents instead of the DID Documents themselves. Other options include handling additional types via the registries -- so I don't think this is a blocker, but noting it here as people interested in methods such as did:web may care.

@msporny msporny removed the ready for pr Issue is ready for a PR label Dec 14, 2020
@msporny
Copy link
Member

msporny commented Dec 14, 2020

Do we need a distinction on datetimes that covers the level of time precision (for example, seconds vs. milliseconds)?

Yes, the other thing that's missing is forcing that GMT is used in serializations via 'Z' trailer. I suggest we DO NOT serialize datetimes with millisecond precision by default (but do not block other parameters that want to use millisecond precision). This should be raised in another issue.

@msporny
Copy link
Member

msporny commented Dec 14, 2020

@dlongley's concern now being tracker in issue #498.

Someone needs to make sure we define data types and serializations for metadata properties, which is being tracked in issue #499.

@msporny
Copy link
Member

msporny commented Dec 14, 2020

PRs #455 and #469 have been merged, we are more clear on how representations should serialize and deserialize all properties in the data model now. Closing.

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

No branches or pull requests

7 participants