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

Extract IPNFT encoder into a standalone repo #837

Closed
Gozala opened this issue Nov 22, 2021 · 9 comments
Closed

Extract IPNFT encoder into a standalone repo #837

Gozala opened this issue Nov 22, 2021 · 9 comments
Assignees
Labels
kind/enhancement A net-new feature or improvement to an existing feature need/triage Needs initial labeling and prioritization

Comments

@Gozala
Copy link
Contributor

Gozala commented Nov 22, 2021

Now that we have an IPNFT spec https://github.com/nftstorage/ipnft we should extract IPNFT encoder into a standalone reference implementation library.

I think it would make sense to have a following interface for the library:

// @see https://github.com/ipld/js-car/blob/530fd31ab0b5005ec5fedd7048251b6aa501dcd4/lib/coding.ts#L4-L10
import { CarEncoder } from @ipld/car”

interface Lib {
   create<T extends TokenInput>(input:T):Promise<IPNFT<T>>
}

interface IPNFT<T extends TokenInput> {
  /**
   * CID for the token that encloses all of the files including metadata.json
   * for the stored token.
   */
  ipnft: CIDString

  /**
   * URL like `ipfs://bafy...hash/meta/data.json` for the stored token metadata.
   */
  url: EncodedURL

  /**
   * Actual token data in ERC-1155 format. It matches data passed as `token`
   * argument except Files/Blobs are substituted with corresponding `ipfs://`
   * URLs.
   */
  data: Encoded<T, [[Blob, URL]]>

  /**
   * Token data just like in `data` field except urls corresponding to
   * Files/Blobs are substituted with IPFS gateway URLs so they can be
   * embedded in browsers that do not support `ipfs://` protocol.
   */
  embed(): Encoded<T, [[Blob, URL]]>

  encodeInto(encoder:CarEncoder):Promise<void>
}
@Gozala Gozala added kind/enhancement A net-new feature or improvement to an existing feature need/triage Needs initial labeling and prioritization labels Nov 22, 2021
@Gozala
Copy link
Contributor Author

Gozala commented Nov 22, 2021

As an added requirement standalone library should ensure that input encodes to reasonable sized block as per #637, if it is not throw an error suggesting to refactor parts of JSON into separate blocks.

@Gozala
Copy link
Contributor Author

Gozala commented Nov 22, 2021

On the second thought it seems like this probably should be something like dag-ipnft, although as far as I know we do not currently have multiblock encoder interface (or do we @rvagg ?) and if so maybe we should have an interface to make IPNFT be an implementation of it.

If we had such an interface we could extend car libs to support those, removing need for encode method of above defined IPNFT

@rvagg
Copy link

rvagg commented Nov 23, 2021

No .. we don't have a multiblock encoder interface, but this smells awfully like an ADL, which we've not explored properly in JS. But I'm not sure since it seems to be very codec specific so maybe it's not quite.

I don't quite understand the spec, it says "IPNFT node" which must be an "encoded data structure"—so I gather from this that the use of "node" meant to be different to how we use it in the IPLD terms? I'm just not sure what it should mean. I think the spec is suggesting that this thing ends up being 3+ IPLD blocks - a dag-cbor root, a dag-pb directory containing at least one dag-pb asset file, and possibly more files including a metadata.json. Is that correct?

Ping @warpfork for interest.

@Gozala
Copy link
Contributor Author

Gozala commented Nov 23, 2021

No .. we don't have a multiblock encoder interface, but this smells awfully like an ADL, which we've not explored properly in JS.

Maybe if we had ADLs shipping and well supported across ecosystem when we implemented this in nft.storage we'd use them. At a time however it made sense to fuse metadata with all assets together.

But I'm not sure since it seems to be very codec specific so maybe it's not quite.

I was thinking something like our block encoder except it would produce a car bytes as opposed to block bytes. Or maybe some intermediate representation (IR) for CAR so it could be combined with other CAR IRs before encoding it all into bytes.

I don't quite understand the spec, it says "IPNFT node" which must be an "encoded data structure"—so I gather from this that the use of "node" meant to be different to how we use it in the IPLD terms?

This is a good feedback I just removed term "node" to avoid confusing, now it reads as

Every IPNFT MUST be DAG-CBOR encoded data structure which MUST have

And it is a DAG-CBOR block with links to NFT assets (which are directory with a single file in it). If this is still confusing please let me know.

@rvagg
Copy link

rvagg commented Nov 24, 2021

re "multiblock encoder" I think we're getting into tricky territory mixing this concept up with the codecs, by calling this "dag-ipfnt" it sounds like it's a thing that deals with cid:bytes:data-model but it's much more than that. Unless you can come up with an abstraction that lets that make sense with our current APIs I think we're best off leaving multi-block concepts to higher levels that are not yet built--the most mature that exists is the ADL style that's nearing usable read/write status in Go, which should probably inform our JS higher-level API development.

@Gozala
Copy link
Contributor Author

Gozala commented Nov 29, 2021

re "multiblock encoder" I think we're getting into tricky territory mixing this concept up with the codecs, by calling this "dag-ipfnt" it sounds like it's a thing that deals with cid:bytes:data-model but it's much more than that. Unless you can come up with an abstraction that lets that make sense with our current APIs I think we're best off leaving multi-block concepts to higher levels that are not yet built--the most mature that exists is the ADL style that's nearing usable read/write status in Go, which should probably inform our JS higher-level API development.

In that case, I think proposed interface is best we can do right now.

@dchoi27 dchoi27 added the need/update manually applied to any needing verbal update during meetings label Dec 10, 2021
@dchoi27
Copy link
Contributor

dchoi27 commented Dec 13, 2021

@Gozala any updates here?

@Gozala
Copy link
Contributor Author

Gozala commented Dec 13, 2021

I'll submit PR shortly

@dchoi27 dchoi27 removed the need/update manually applied to any needing verbal update during meetings label Dec 16, 2021
@Gozala
Copy link
Contributor Author

Gozala commented Jan 3, 2022

This got blocked on by ipfs/js-ipfs-unixfs#186, given that it would be really annoying to have to carry all the dependencies needed by unixfs importer.

New plan is:

  1. To propose less complex API for unixfs importer
  2. Prototype new API
    1. It does not need to re-implement importer just provide an adapter
    2. It can pull all dependencies as necessary
  3. Implement this against new importer
  4. Re-implement importer with new API

This would allow us to test our assumptions without committing to the rewrite. That way any problems / inconsistencies can be dealt with in step 2 or 3.

@dchoi27 dchoi27 added need/update manually applied to any needing verbal update during meetings and removed need/update manually applied to any needing verbal update during meetings labels Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A net-new feature or improvement to an existing feature need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

4 participants