Update to IPLD Prime

[willscott]

Continuation of #30

This reverts commit 46797b1.

[aschmahmann]

Overall LGTM, just a few small nits. @warpfork you may want to take a look at this PR mostly to see if you like the emerging patterns here since they'll likely start recurring in go-ipfs.

[aschmahmann]

Why not a released version of go-ipld-prime?

[hannahhoward]

will update when we do the final merge push once the whole stack is approved

[aschmahmann]

Is this because our test objects are constructed using go-ipld-cbor instead of go-ipld-prime's dagcbor codec? Can we just drop go-ipld-cbor as a dependency here?

[warpfork]

This is probably fine and correct. The current way we handle registration of codec implementations is by side-effecting imports (the init method in the package registers a multicodec in a global table).

I'm not "thrilled" with this per se, because it's very side-effect heavy (and I'm definitely a functional programmer at heart), but it's a race-condition-free way to solve a problem, and seems to be one of the least-bad options available to us.

[aschmahmann]

good news, we can drop the blank import now since we already import dagcbor and we're importing twice now.

[willscott]

@aschmahmann - i don't see any requests here except for module versions, so can we mark this as you ready for it to merge in the merge party where we go through and merge everything while tagging versions of the deps?

[aschmahmann]

Yep, although it'd be great if this function could be rewritten to use go-ipld-prime's cbor codec so we can drop the dependency as well as the empty import. From the outside it seems like it should be pretty simple for someone familiar with the go-ipld-prime libraries (lmk if I'm wrong though).

[willscott]

until we have bind-node as an option for one-off construction in ipld-prime, i think the current setup of the test in this library is going to be more direct and simpler to understand than rewrite you suggest.

[warpfork]

This seems fine to me.

(I might nit that I don't think some of the names of functions in the fetcherhelper package auto-unpack themselves to me, but A) that's a nit, and B) that's not originated from this diff anyway.)

I don't think there's much that's useful to do about the dag-cbor dependency. If one wants any CIDs used in tests, even indirectly, one has to have some codec in the dependency tree. I think either dag-cbor and/or dag-json are generally reasonable as the "batteries included" ones.

[warpfork]

Wait, sorry, re-read Adin's comment about cbor -- it's about the old generation go-ipld-cbor repo still being in the deps. Yeah, that we can fix easily, I think:

I can't comment on it in the diff, because github UI Reasons, but looks to me like there's just two bits that need to change: two uses of cbor.WrapObject in simple/reprovide_test.go -> can probably change them to fluent.Reflect, or even just assemble the stuff manually. It's a pretty small amount of test data using this.

[hannahhoward]

@warpfork I away from using go-ipld-cbor. Interesting thing I learned about fluent.Reflect in the process -- it does not handle cid.Cid's -- which one might infer would convert to Link nodes.

@aschmahmann I removed the indirect depedencies as well. As for the tagged version of go-ipld-prime, we will probably do that once that whole stack is approved and we are merging all at once (by propogating several tagged versions).

This should be ready for rereview.

[warpfork]

Looks all good to me. 🚢

[aschmahmann]

LGTM.

Two things:

1. nit: comment above about removing the duplicated import of go-ipld-prime/dagcbor
2. We're waiting on the final merge for the merge party 🎉 where all the dependencies are updated

[warpfork]

Patched out the duplicated import. AFAICT this is now purely pending the merge party. 👍