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

Proposal: Make Link and Block interfaces compatible #227

Open
Gozala opened this issue Dec 5, 2022 · 5 comments
Open

Proposal: Make Link and Block interfaces compatible #227

Gozala opened this issue Dec 5, 2022 · 5 comments
Assignees

Comments

@Gozala
Copy link
Contributor

Gozala commented Dec 5, 2022

As things stand today it is impossible to satisfy both Link and Block interfaces with a same instance, because both they collide on bytes field:

export interface Block<
T = unknown,
C extends number = number,
A extends number = number,
V extends Version = 1
> {
bytes: ByteView<T>
cid: Link<T, C, A, V>
}

export interface Link<
Data extends unknown = unknown,
Format extends number = number,
Alg extends number = number,
V extends Version = 1
> extends Phantom<Data> {
readonly version: V
readonly code: Format
readonly multihash: MultihashDigest<Alg>
readonly byteOffset: number
readonly byteLength: number
readonly bytes: ByteView<Link<Data, Format, Alg, V>>
equals: (other: unknown) => other is Link<Data, Format, Alg, Version>
toString: <Prefix extends string>(base?: MultibaseEncoder<Prefix>) => ToString<Link<Data, Format, Alg, Version>, Prefix>
toJSON: () => { version: V, code: Format, hash: Uint8Array }
link: () => Link<Data, Format, Alg, V>
toV1: () => Link<Data, Format, Alg, 1>
}

This is unfortunate because we're finding ourselves in a position where we would like to satisfy both interfaces along with following DAG interface:

interface DAG {
   links(): IterableIterator<Link|Block>
}

More broadly speaking we would like an ability to give a link with it's data attached (when we have it).

@Gozala
Copy link
Contributor Author

Gozala commented Dec 5, 2022

I think possible solutions here are:

  1. Rename bytes in either or both to have something less generic & non colliding.
  2. Drop bytes from Link and come up with something else for link['/'] === link.bytes
    • Perhaps link['/'] instanceof Uin8Array is good enough & is also backwards compatible.
  3. Drop bytes from Block in favor toBytes(): Uint8Array.
    • Perhaps we can drop cid in favor of link(): Link while we're at it.

@rvagg
Copy link
Member

rvagg commented Dec 6, 2022

@Gozala I agree that this is worth fixing, but I can't bring myself to prefer a solution. Both of these things have .bytes and both feel like they should be named that way. Since Block class doesn't get much use currently, at least in comparison to CID (and therefore Link), any breakage there is going to be much lower impact. So a toBytes() or getBytes() might make sense. I'll leave it to you to be creative but I'm happy to move forward with this because what you're wanting to do with your DAG iterator makes a lot of sense and you already flagged that during the Link work and I thought it was a great idea.

@Gozala
Copy link
Contributor Author

Gozala commented Dec 7, 2022

My current plan is to idea that motivated and make more informed and proposal based on that experience.

I thought starting this thread in parallel was a good idea to gather input from collaborators. I've been also trying to get some ideas from Node interface but so far I have failed to borrow much. But here is some rough plan:

  1. I need separate Link, Block and Node (which I previously called DAG) interfaces.
  2. In most codecs I use I already have write function that returns Block as opposed to just bytes, I would like to upgrade it to return Link & Block & Node instead.
  3. Bunch of our code works with higher level abstractions however, I call them views because they provide domain specific view of the Block.
    • We often find ourselves needing to go from View layer to block & links layer, I hope that our views could extend Link, Block and Node interfaces to address this.
  4. We don't depend on any outside code that expects Blocks so we can prototype Link & Block compat simply by changing our Block definition and learn from that experience.
  5. We could also evaluate if this hybrid constructs recognized as links by codecs are good idea or a bad one.

@Gozala
Copy link
Contributor Author

Gozala commented Dec 7, 2022

Personally I'm leaning towards option 2, which I'd re-frame as:

  1. Deprecate bytes field in CID class
  2. Drop bytes field in Link interface (it's new enough to not cause much problem I think)
  3. Change CID-ness check to link['/'] instanceof Uin8Array
    • I highly doubt false positives, but if it turns out to be problem we could offer some opt-out field

Note that if bytes field is simply deprecated and not used by codecs Link & Block could put block bytes in bytes and have no issues.

@rvagg
Copy link
Member

rvagg commented Jan 2, 2023

Just looking through my ipld js folders and I'm seeing cid.bytes show up pretty frequently—although of course they're mainly low-level things which don't necessarily reflect how end-users are going to be touching these things. But all the codecs use it, js-car uses it, anything that needs to do something encody with a CID. So it's going to be fairly disruptive to migrate away from it.

Do you have a proposal for a new interface for CID that gives access to the bytes? Am I going to have to cid['/'] to get access to this? That's not a great option, it makes the code more opaque. Are we going to have to go back to cid.buffer?

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