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

Comments about the Specification #7

Closed
jmgurney opened this issue Feb 10, 2021 · 1 comment
Closed

Comments about the Specification #7

jmgurney opened this issue Feb 10, 2021 · 1 comment

Comments

@jmgurney
Copy link

@justindossey asked me to take a look at this, and here are a few comments. Most are minor and geared towards cleaning and clearing up the language.

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L24
when encoded -> after encoding

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L32
The term block(s) is not defined. Maybe use object or some other term. It isn't until the examples or line 78, that it's clear that a block is a key/value object or something similar.

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L32
and
https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L95
cryptographic signature is a SHA256 digest in hexadecimal form -> cryptographic signature is in hexadecimal form
You sign a hash to generate a signature, but the signature itself is not a hash.

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L36
Not sure why percent encoding is called out here. Maybe clarify that the URI is encoded per the standard using percent encoding: format are Percent Encoded per the standard.

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L89
if the colon is significant (cdc:1a9), then it should be better defined. If it isn't, then it should be clear that the value be treated as a full index, and not a two part index.

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L113
This should likely be more formally specified, such as:
To get the resulting string below 255 encoded bytes, the last Unicode code point is removed until the resulting encoding is less than or equal to 255 bytes.

This made me realize that it's likely that it should be specified that all Unicode strings are NFC normalized. For more information: https://www.unicode.org/faq/normalization.html
This will help ensure consistent encoding, and also consistent hashing as well.

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L127
This likely needs to be clarified that all Unicode characters need to be upper cased per the rules defined by Unicode, e.g. á is converted to Á.

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L165
Feels odd to not use JSON arrays, but then define the URI encoding version this way. Because otherwise the JSON parser will have to duplicate this code from the URI parser, where it could just avoid it by using JSON arrays. That would be:

[ [ 1, "PFIZER", "13a056" ], [ 2, "PFIZER", "29a063" ] ]

and per the next line (167), this is structured, so things like the producer and lot number need to have additional restrictions on them so that the above is handled properly, that is that both producer and lot number do not have space like characters (In unicode there are a LOT of space like characters that could be handled differently in this case).

https://github.com/Path-Check/paper-cred/blame/main/SPECIFICATION.md#L188
What about one dose vaccines, i.e. J&J's upcoming vaccine? the (of two) part seems problematic. Maybe a better choice would be:
0: no vaccination
1: partial/incomplete vaccination status
2: completed vaccination regiment

@justindossey
Copy link
Collaborator

Made some edits to address your comments here: https://github.com/Path-Check/paper-cred/blob/issue/1/SPECIFICATION.md

We'd been discussing deserializing the DOSEINFO structure already. I was resistant to doing so due to data size considerations, but when enough people tell me "stop doing that" I listen :)

vitorpamplona added a commit that referenced this issue Feb 11, 2021
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

2 participants