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

Remove the legacy codec system #164

Open
Stebalien opened this issue Nov 15, 2022 · 7 comments
Open

Remove the legacy codec system #164

Stebalien opened this issue Nov 15, 2022 · 7 comments

Comments

@Stebalien
Copy link
Collaborator

Serde is significantly more usable, and supporting two codec systems is very confusing and a waste of time.

@vmx
Copy link
Member

vmx commented Nov 16, 2022

Agreed. Though I'd like to get close to feature parity, so that people using the legacy codec can smoothly upgrade.

@cdata
Copy link

cdata commented Nov 28, 2022

Heya, @vmx asked me to comment on our usage of the codec approach. To be honest, my first thought was the same as yours @Stebalien but there are two things that I enjoy in the codec approach:

  • Codecs know how to extract links
  • We mostly use dag-cbor, but in some cases we also encode as raw

I would love for there to be one true way, and if serializing data as blocks was as simple as deriving Serialize/Deserialize in most cases, that would be awesome 👍

@Stebalien
Copy link
Collaborator Author

I agree that we definitely need a way to cleanly extract links. It shouldn't be too difficult to re-implement that with serde by just throwing away everything we don't care about, but the current method is more efficient.

@Frando
Copy link

Frando commented Dec 2, 2022

I recently learned about the DeserializeSeed trait. Maybe this could be used to collect links while deserializing?

@Stebalien
Copy link
Collaborator Author

That's not even necessary, really. We can just deserialize into a "links collector" that:

  1. Collects links.
  2. Ignores everything else.

@Frando
Copy link

Frando commented Dec 3, 2022

Ah right, that should be possible too. When working with many small blocks, the current API allows to collect all links into a single container (without allocating a links container for each block), that's why I thought of DeserializeSeed.

Maybe there's two features at play even?

  • Only collect links without deserializing everything
  • Deserialize into a struct and collect all links, in a single pass

The latter might not matter much in practice though.

@vmx
Copy link
Member

vmx commented Dec 5, 2022

  • Only collect links without deserializing everything

  • Deserialize into a struct and collect all links, in a single pass

The latter might not matter much in practice though.

Agreed. I'd start with the former one and add the latter only if it turns out to be useful. I prefer keeping APIs small and targeted at needs, rather then adding theoretically useful things.

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

4 participants