filecoin-project / go-fil-markets

Shared Implementation of Storage and Retrieval Markets for Filecoin Node Implementations
Other
78 stars 59 forks source link

feat(ipld): bindnode support for all voucher types #713

Closed rvagg closed 2 years ago

rvagg commented 2 years ago

PR against #712 but will be against #707 when that one gets merged.

This depends on a the branch of go-ipld-prime that's in here: https://github.com/ipld/go-ipld-prime/pull/415, which itself builds on https://github.com/ipld/go-ipld-prime/pull/414 (I'll merge them into one soon).

Aim

We want to be able to use datamodel.Node vouchers from go-data-transfer, where we're stripping out cbor-gen and it now has a unified go-ipld-prime usage. But we encounter lots of cbor-gen types here that we have to write translation layers to get to if we were to use bindnode or any other go-ipld-prime Node interface (codegen, basicnode, qp?).

This highlights a problem with bindnode - in typical use you end up needing to write a translation layer between two Go types to get to the form your application wants. We found that in graphsync and it's even true in go-ipld-prime itself (as can be seen here: https://github.com/ipld/go-ipld-prime/pull/350 which needs to convert operationsRaw to Operation).

Wot?

So 414 and 415 are experiments in exposing extension points in bindnode that give you the ability to convert to and from custom types that don't have a natural translation between Go types and data model kinds. These end up mapping almost exactly to the Go types that have hand-rolled MarshalCBOR and UnmarshalCBOR in the Lotus codebase (a surprising number of them).

So now, we get to say how a type translates from a custom Go type to an commonly understood one one (e.g. Address -> []byte), or even a custom Go type to a datamodel.Node in the case of an Any. We also get to do this from outside of the type, so we don't need to go digging around in go-state-types, go-address, etc. to add methods to the types we care about—we declare them when we interact with bindnode.

The Any case (to and from datamodel.Node) lets us even work with cbor-gen's Deferred types .. sort of. At least we don't need very heavy modifications to make it work.

So this PR includes minimal adjustment of the voucher types, the exception being the Deferred for Selector which we box in a type that can speak with both cbor-gen and bindnode. And we get to use the same types for both cbor-gen and bindnode. It also includes a helper package that improves usability of bindnode for a library like this where you have multiple types you care about—such as allowing the schema to hang off the type itself with a BindnodeSchema() method.

Tests

The main work in this PR are the tests that compare the round-trip serialisation and deserialisation of types through both cbor-gen and bindnode and demonstrate that they are correct and comparable.

hannahhoward commented 2 years ago

Ok, this took me a long time to understand but as I see it, the end goal here is that, in the case of the retrieval deal proposal, as an example, is now both a CBOR-GEN compatible type AND a bindnode compatible type.

The reasoning is:

I've looked at all three PRs now, and my general input is: go forth and implement!

hannahhoward commented 2 years ago

I will say: shared.SerializedNode is not my favorite name as I read that as "stored in serialized form" which is not true -- it is in fact already deserialzied into a datamodel.Node, unlike cbg.Deferred. I would call it "shared.CborGenIPLDNode" or something like that I think.

rvagg commented 2 years ago

renamed SerializedNode & added additional comments

rvagg commented 2 years ago

Updated to current https://github.com/filecoin-project/go-data-transfer/pull/325

codecov-commenter commented 2 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (feat/datatransfer-v2-integration@b93af0a). Click here to learn what that means. The diff coverage is n/a.

Impacted file tree graph

@@                         Coverage Diff                         @@
##             feat/datatransfer-v2-integration     #713   +/-   ##
===================================================================
  Coverage                                    ?   58.73%           
===================================================================
  Files                                       ?       62           
  Lines                                       ?     5224           
  Branches                                    ?        0           
===================================================================
  Hits                                        ?     3068           
  Misses                                      ?     1815           
  Partials                                    ?      341