Make Codecs composable

[expede]

@vmx I'm happy to update the serde_ipld_* instances (which are all currently broken in the test suite), but I wanted to at least propose this change in case there was some reason to not do it.

Note that the tests will currently fail because they pull in downstream libs (e.g. serde_ipld_dagcbor) which would need to be updated, so there's a bit of a chicken and egg situation.

Rationale

From the docs:

Each IPLD codec implementation should implement this Codec trait. This way codecs can be more easily exchanged or combined. [emphasis mine]

I've been through this exact design loop in one of my own library modules that plugged holes in libipld in many of the same ways that ipld-core does. Essentially, if the goal is to make codecs composable (e.g. "I support both DAG-CBOR and DAG-JSON and want to model that as 'a codec'"), then const CODE doesn't let these stack because it requires runtime information.

Proposal

I propose (per this PR) that Codec uses a fn instead of a const to make this dynamic.

// AFAIK this is impossible to write a `Codec` instance for with `const CODE`
#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
pub enum TotalCodec {
    /// DAG-CBOR
    #[default]
    DagCbor,

    /// DAG-JSON
    DagJson,
}

// Proposed
impl Codec for TotalCodec {
    fn to_code(&self) -> u64 {
      self.to_code()
    }

    // ...
}

It's entirely possible that I'm interpreting this line incorrectly, but I've run into this in one of my libraries that I'm switching from libipld to ipld-core ahead of IPFS Camp.

Another Option

Changing from const CODE: u64 to fn code(&self) -> u64 may have a (very minor) performance implication (assuming that the compiler can't elide this on simple instances). Another option would be to have something like the following...

// Pseudocode

trait HasStaticCode {
    const CODE: u64

    // ...decode, encode, etc
}

impl<T: HasStaticCode> Codec for T {
    fn to_code(&self) -> u64 {
      Self::CODE
    }

    // ...decode, encode, etc
}

...which gives you the ability to restrict to the static variant, but this feels like an over optimisation to me.

from_code

I also have uses for a from_code as well. Right now I'm hacking it in with TryFrom<u64>, but u64 and a codec are isomorphic. I see no reason to not put it right on Codec, but you will know much more about the overall design philosophy for ipld-core :)

[expede]

from_code

(I ended up also adding the one-liner for this)

[vmx]

Generally spoken, the design goal of this library is that it works for real world use cases.

I haven't thought about the use case of having an enum of codecs, but I can see the point of it. To me this change looks good and I agree that the performance hit is negligible.

@Stebalien do you have any thoughts on this?

[Stebalien]

Yeah, this makes a lot of sense.

[Stebalien]

nit: I'd just call this code().

[Stebalien]

I assume these now need to take &self?

[Stebalien]

Basically, this is going to become a "by value" instead of a "by type" trait.

[Stebalien]

I'm surprised this was necessary.

[expede]

It's probably not; it's having a hard time picking up the codec in the doctests and I'm troubleshooting. Because this spans multiple repos I need to push to git, so I've flipped the PR to Draft

[expede]

this spans multiple repos

Multiple repos with a dependency cycle, even (on the dev-dependencies)

[expede]

Actually, I know that it's not required because in the library that I'm actually working on I now actually need fewer type annotations. For example:

//Before
let bytes = <DagCborCodec as Codec<Ipld>>::encode_to_vec(dag).unwrap();

// After
let bytes = DagCborCodec.encode_to_vec(dag).unwrap();