Graphsync v2.0: bindnode conversions for vouchers

[rvagg]

Early WIP, ipldbind package in here is probably the most interesting, replacing the encoding package's functionality for vouchers.

[hannahhoward]

The goal here should be to get Vouchers and VoucherResults to be generic datamodel.Nodes or schema.TypedNode and pushing the wrapping in bindnode up to the library consumer. We already discussed but just want to serialize that direction.

[hannahhoward]

Interesting direction this is taking. I notice this deserialization of vouchers into anything but basicnode.Any out of the library. This wasn't quite how I envisioned this. I had imagined that we still pass the vouchers to the Register methods (along with type identifier) and that we call .Prototype() on those nodes and record them for later decoding. Alternatively, we could pass datamodel.NodePrototype directly to the register methods along with the TypeIdentifier -- in fact I think this would be my preference. That way, when the various validator methods get called, you'd have some guarantee that you get back a node with the prototype you setup that you can easily unwrap to a go-type. Part of the reason I support this direction is that for typed nodes, the IPLD object you get back on the other side of the network connection would behave in the same way the IPLD object you passed into data transfer on the initiating side.

Also perhaps it makes sense to seperate out registering a mapping of TypeIdentifier -> NodePrototype from TypeIdentifier -> Validator / Revalidator? I think this would make sense.

This is shaping up to be a big breaking change -- it's the right direction, but I wonder if we say #304 is the boundary for the next release. There's no network changes underlying all this, so we can always do this later right?

[codecov-commenter]

Codecov Report

Merging #305 (fffca3e) into feat/graphsync-20-minimal (8f3cfa1) will increase coverage by 6.12%.
The diff coverage is 73.36%.

@@                      Coverage Diff                      @@
##           feat/graphsync-20-minimal     #305      +/-   ##
=============================================================
+ Coverage                      66.89%   73.02%   +6.12%     
=============================================================
  Files                             24       21       -3     
  Lines                           3169     2654     -515     
=============================================================
- Hits                            2120     1938     -182     
+ Misses                           708      524     -184     
+ Partials                         341      192     -149     

Impacted Files	Coverage Δ
network/libp2p_impl.go	66.47% <0.00%> (ø)
message/message1_1prime/transfer_message.go	27.27% <27.27%> (ø)
message/message1_1prime/message.go	58.62% <58.62%> (ø)
message/message1_1prime/transfer_request.go	64.51% <64.51%> (ø)
registry/registry.go	65.21% <75.00%> (-9.79%)	⬇️
message/message1_1prime/transfer_response.go	79.59% <79.59%> (ø)
impl/restart.go	63.63% <80.00%> (+1.83%)	⬆️
impl/impl.go	65.03% <80.95%> (+0.08%)	⬆️
channels/channel_state.go	74.41% <88.88%> (-5.81%)	⬇️
channels/channels.go	72.30% <100.00%> (-0.15%)	⬇️
... and 5 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8f3cfa1...fffca3e. Read the comment docs.

[rvagg]

After discussion, we've gone with just a plain ipld.Node for now, rather than using TypeNode coupled with TypedPrototype or anything that requires validation on the IPLD side. This is left as an exercise for the consumer.

I've done some cleanup, added some more tests and fixed up the README. I hope this is good to go now. But it's not going to be a painless upgrade for downstream users who now have to deal in datatransfer.TypeIdentifier and ipld.Node pairs.

One unresolved item that may need adjusting:

type Voucher ipld.Node
type VoucherResult ipld.Node

Then the interface deals in both Voucher and VoucherResult. I did this initially because it made the refactor easier to work through, but I quite like the clarity of separating the two. But it might be a little unidiomatic so maybe we need to ditch these custom types entirely. Feedback requested!

[hannahhoward]

So this is pretty close BUT...

ipldutil usage is still off I think from what I was intending I think.

You have:

• gotten rid of RegisterVoucherResultType
• made all the register methods take a type parameter instead of a type
• made a registry of node prototype -> but keys must be go type names and explicity require the use of bind node
• still generally using any to construct nodes for vouchers though

I believe you need:

• a registry, in go-data-transfer manager, that is a mapping of TypeIdentifier -> generic node prototype
• a new function to register prototypes say `RegisterVoucherPrototype(name TypeIdentifier, prototype datamodel.NodePrototype)
• when calling functions like NodeFromDagCBOR, lot for a prototype in the registry, use that to generate a NodeBuilder for dagcbor.Decode (still feature detect and use representation prototype as needed). Probably means they are part of manager as opposed to pure functions.

This new register type would replace RegisterVoucherResultType AND the part of RegisterVoucherType that just about making a map of type identifier -> used node prototype.

[rvagg]

Some time has passed since this work was done and discussion was had but my recollection of the discussion that happened after the last comment was that we were going to try going ahead with this current approach and see how well it works downstream. We've pushed all IPLD schema work out to consumers, just passing around generic, untyped Nodes and letting the downstream users handle conversion / type checking.

I may be wrong on that recollection, it wouldn't be the first time.

Current plan is to try this out on go-fil-markets. This branch is now directly on top of current master.

[rvagg]

this is now impossible to rebase against v2, I'm going to have to rewrite this I think