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

add Ipld de/encoding for CACAOs #7

Merged
merged 10 commits into from
Apr 4, 2022
Merged

add Ipld de/encoding for CACAOs #7

merged 10 commits into from
Apr 4, 2022

Conversation

chunningham
Copy link
Contributor

@chunningham chunningham commented Mar 25, 2022

Implements Encode<DagCborCodec> and Decode<DagCborCodec> for Payload and CACAO, to support encoding cacaos as IPLD blocks. Also adds a test case taken from the ceramic cacao-ts tests (currently fails). Closes #4

@chunningham chunningham requested a review from clehner March 25, 2022 12:20
@clehner
Copy link
Contributor

clehner commented Mar 25, 2022

Looks good... I confirm the test fails, with the following:

thread 'tests::test_ipld' panicked at 'called `Result::unwrap()` on an `Err` value: Missing key `aud` for decoding `cacao::payload_ipld::TmpPayload`.', src/lib.rs:308:10

... which is strange because the linked test in Ceramic's implementation includes an "aud" payload value. But I didn't try to check that by decoding the binary data.

Another possible test case may be the one in the CACAO CAIP: https://github.com/ChainAgnostic/CAIPs/blob/8fdb5bfd1bdf15c9daf8aacfbcc423533764dfe9/CAIPs/caip-draft_cacao.md#test-cases

clehner added 2 commits April 1, 2022 14:14
- Add default attribute for optional fields
- Add CACAOIpld struct
Copy link
Contributor

@clehner clehner left a comment

Choose a reason for hiding this comment

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

Test decoding from CBOR passes in #12. Round-trip encode/decode succeeds in #13 (However, the serialization is different from the bytes that the Ceramic test vector uses, as some properties are in a different order; therefore different CIDs result...).

I think maybe the dependency on DagCbor in the types could be reduced by using Codec Decode + Encode instead. But to use CIDs it it looks like we have to pick a serialization. CAIP-74 suggests CBOR, and that is what Ceramic's test case uses, so I think we'll be using DagCbor anyway, although we could allow use of other serializations for the CIDs.

@chunningham
Copy link
Contributor Author

chunningham commented Apr 4, 2022

It would be ideal to have Encode<C> + Decode<C> where C: Codec without hard dependancy on one, but yeah there are optionality challenges with transliteration in IPLD (only the 3 DagX codecs would preserve any meaningful structure, so de/encode can't be so generic without a different StructuredCodec marker trait maybe). w.r.t. CIDs yeah we have to choose our default for many purposes, but I agree we should allow for other block producers to choose an alternative. CBOR was really used for convenience w.r.t. available tools + seems to be the go-to for most projects.

By rearranging the TmpPayload fields in their deterministic order, the decoding test is fixed 👍

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

Successfully merging this pull request may close these issues.

IPLD representation
2 participants