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

consider using dag-json instead of json.stringify #73

Open
Gozala opened this issue Apr 9, 2022 · 4 comments
Open

consider using dag-json instead of json.stringify #73

Gozala opened this issue Apr 9, 2022 · 4 comments

Comments

@Gozala
Copy link

Gozala commented Apr 9, 2022

dag-ucan had been updated to use dag-cbor as it's primary representation (which is more compact and hash consistent). However while all UCANs in primary representation can be formatted into a valid JWT UCAN string and be parsed / validated by this library (and I have tests in place to ensure) not all UCANs can be represented in that representation, because key ordering and white-spacing is not preserved. For that reason dag-ucan has secondary representation which is basically RAW bytes of JWT string. This allows dag-ucan to use optimal representation on all UCANs it produces but still interop with UCANs that can't be represented that way.

Unfortunately UCANs produced by this library can't be represented by primary representation, I am guessing due to key ordering (DAG-CBOR orders those alphabetically for hash consistency). For that reason I would like propose adopting dag-json in place of JSON.stringify which adds same key ordering as dag-cbor. If adopted tokens issued by this library could be parsed into primary and more compact representation in dag-ucan. In addition it would provide few additional benefits:

  1. IPLD linking support (see https://ipld.io/specs/codecs/dag-json/spec/#link-kind)
  2. Native binary support (see https://ipld.io/specs/codecs/dag-json/spec/#bytes-kind)

Caveats

  1. dag-ucan also lower cases all cans since according to spec they are case insensitive and normalizing would provide better hash consistency. In other words this library would have to do the same for it's tokens to be parsed into compact representation.
    https://github.com/ipld/js-dag-ucan/blob/7bacaf91a90f511ec70d1a4f51ed923947185399/src/parser.js#L104-L120

  2. dag-ucan omits fct field if it an empty array https://github.com/ipld/js-dag-ucan/blob/main/src/formatter.js#L58

@expede
Copy link
Member

expede commented Apr 16, 2022

Caveats

Given that these are both spec-complaint, I don't think that either of these would be an issue 👍

@expede
Copy link
Member

expede commented Apr 16, 2022

Pulling the conversation here from Discord, @matheus23 mentioned that he was in favour there. I'm just the maintainer of the spec and not this specific library, but also wanted to throw my support behind using dag-json.

I had heard elsewhere that dag-jose has some deficiencies, but

Unfortunately UCANs produced by this library can't be represented by primary representation, I am guessing due to key ordering (DAG-CBOR orders those alphabetically for hash consistency)

This is surprising! If they have a fallback to raw strings, then ones referenced from that format should still maintain that ordering, no? UCANs generated initially from the CBOR<>JSON codec should still be valid if they're signed after serialization.

@Gozala Random idea that I'll move to the spec: would it be helpful to have an optional flag in the header that says "this is in a lexicographical sorted compact representation", to help you switch on that information?

UPDATE: created discussion thread on the spec ucan-wg/spec#60

@Gozala
Copy link
Author

Gozala commented Apr 17, 2022

This is surprising! If they have a fallback to raw strings, then ones referenced from that format should still maintain that ordering, no? UCANs generated initially from the CBOR<>JSON codec should still be valid if they're signed after serialization.

Maybe I'm misunderstanding your comment, but when parsing UCAN issued by this library it ends up in secondary representation which is basically raw string. If proposed changes were implemented UCAN issued by this library could be parsed into primary representation.

In other words UCANs issued by both libraries are going to be valid, it's just UCANs issued by this library can't be encoded into more compact representation.

@Gozala
Copy link
Author

Gozala commented Apr 17, 2022

@Gozala Random idea that I'll move to the spec: would it be helpful to have an optional flag in the header that says "this is in a lexicographical sorted compact representation", to help you switch on that information?

UPDATE: created discussion thread on the spec ucan-wg/spec#60

Responded there. tldr: I'm not sure it would make a difference

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