-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
Encode integrity check as multihash #549
Encode integrity check as multihash #549
Conversation
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 think this mainly needs an encoding test, but otherwise looks great to me
1b497de
to
26e2600
Compare
I am weakly opposed to this, since this will require special support for the cbor library used by each implementation. Moreover we rarely encode non-resolved expressions, so the gains look small to me. |
I am weakly opposed to this, since this will require special support for the cbor library used by each implementation
What kind of "special support" do you mean? This only uses one of the CBOR "major types", the most fundamental building blocks of CBOR, and one Dhall already relies on for bignums.
|
I am also not feeling great about this, for the same reasons as @Nadrieril:
|
doesn't even mention "multihash"
multihash has nothing to do with CBOR and if that's adding confusion we can remove mention of it from the spec entirely. This change only moves from a CBOR array of two CBOR unicode strings to a single CBOR bytestring.
|
some CBOR libraries don't easily give access to the "building blocks" of CBOR and basically only support encoding a JSON value (the [Clojure one](https://github.com/greglook/clj-cbor)
Reading the source there it looks like byestrings are mapped to https://clojuredocs.org/clojure.core/bytes
I will look at rust and (since it came up) javascript CBOR libraries soon for this feature also.
|
browser side CBOR library for JavaScript supports bytestring as Uint8Array: https://github.com/paroga/cbor-js/blob/master/cbor.js#L158 Rust serde_cbor implements for at least |
So from reading through this it seems like the only sticky issue is the Clojure @f-f: Would it be possible to open an issue against them to support encoding Clojure byte arrays as CBOR byte arrays? For me it seems reasonable to expect a language to (A) support byte arrays and (B) have a CBOR library that supports encoding those byte arrays as CBOR byte arrays. |
@Gabriel439 sorry for the delay on this. As @singpolyma figured I was just confused by the mention of "multihash" (which is independent from the CBOR encoding, while I thought it was a CBOR feature). |
Using a byte string with internal label as much more compact than storing base16 as unicode. Re-using the multihash spec so we don't have to invent our own but using such a small subset of it that implementors do not need to be familiar with multihash at all to implement this. This change does not affect semantic hashes, since semantic hashes are computed on fully resolved expressions. Closes dhall-lang#548
26e2600
to
45d4c10
Compare
I've pushed updates to the tests based on my branch implementation in dhall-ruby |
PR #549 adopted the use of multihash for binary encoding of semantic hashes; this commit extends that to the filenames of cache files on disk.
PR #549 adopted the use of multihash for binary encoding of semantic hashes; this commit extends that to the filenames of cache files on disk.
Using a byte string with internal label as much more compact than
storing base16 as unicode. Re-using the multihash spec so we don't have
to invent our own but using such a small subset of it that implementors
do not need to be familiar with multihash at all to implement this.
This change does not affect semantic hashes, since semantic hashes are
computed on fully resolved expressions.
Closes #548