feat(ipld): vouchers as plain ipld.Node

[rvagg]

Yet another take at this; previous at #305 which is impossible to rebase now and is probably not correct anyway.

This isn't done, tests aren't passing yet; still some problems in here that need to be figured out but I'll tackle it again next week.

[rvagg]

I managed to fix some of the integration test failures, but there's a few remaining I haven't figured out yet.

The problem I was having is in needing to have two things for a voucher now instead of one - you need the voucher (ipld.Node) and the voucher type (datamodel.TypeIdentifier). So there's some places where formerly the voucher is just passed around but I've neglected to pass through the type as well. Fixing the ones I could find helped with most of the failures. I don't know if the rest are related.

[rvagg]

Hah! I figured it out, got to green. And it was related to voucher types.

The API is now more verbose because of the voucher:voucherType pairs everywhere. The main places where this becomes painful from the outside is the additional arguments to OpenPullDataChannel() and OpenPushDataChannel(). In those two calls you need to provide a voucher, voucherType and voucherResultType. The two types are cached on the internal channel and are used for future interactions. Then there's another change in acceptRequest() that also sets up a new channel. It gets the voucher and voucherType from the incoming message and the voucherResultType from the validatorFunc() call. Like everywhere else, ValidationResults need to provide the pair, this time a VoucherResult and a VoucherResultType.

An alternative might be to make a new type to deal with this verbosity and use it everywhere:

type Voucher {
  Data ipld.Node
  Type datatransfer.TypeIdentifier
}

Aside from the verbosity, I don't really know if this API makes sense in all places it gets used:

1. Do we have this information available at all of these places? Is the voucher type always a known quantity?
2. Is it true that both voucherType and voucherResultType are stable across the lifetime of a channel? Does a channel only have one type of each, or can they vary? The code being changed relied on getting the Type() off the vouchers, but it also seemed to be able to make an assumption that the first voucher's Type() is reflective of all of the vouchers in that channel.

[hannahhoward]

My initial gut reaction is we should keep the type near the voucher, so I like the proposed type. As I read the current code, there is a bit of loss of type information as the current voucher interface always has the Type() method, and the Type is encoded into the serialized data. My biggest immediate concern is we are changing the serialized data for transfers in a way we need we need to write migrations for -- it's find to press forward, but the symmetry between the current EncodedVoucher and your proposed Voucher struct suggests we're on the right track.

Vouchers are not consistent across a whole transfer. For example, retrievals use both DealProposal and DealPayment as voucher types.

[hannahhoward]

Some initial comments. I definitely think the struct w/ a type is a good idea.

[hannahhoward]

What is going on with all this code at this point? It seems like a bunch of complexity maybe designed to support actually picking bindnode schemas when deserializing data transfer vouchers, but it's only used for the actual data transfer message formats. Do we need it? does this actually make the message code easier to read?

[rvagg]

TODO - it's copypasta from go-fil-markets code currently which is evolving to deal with message complexity there. But it's probably way too much here.

My thought was to extract this to a separate package but let's discuss this. The idea was that we could hide the schemas within the type itself so you just need to pass around type pointers and you don't need to pre-register for prototypes, it's done lazily and cached...

Mostly I'd really like to be using similar patterns across both codebases for this bindnode work, somehow, if possible.

[codecov-commenter]

Codecov Report

Merging #325 (05233b7) into v2 (5253bfe) will increase coverage by 5.57%.
The diff coverage is 62.56%.

@@            Coverage Diff             @@
##               v2     #325      +/-   ##
==========================================
+ Coverage   64.99%   70.56%   +5.57%     
==========================================
  Files          30       21       -9     
  Lines        3528     2762     -766     
==========================================
- Hits         2293     1949     -344     
+ Misses        865      591     -274     
+ Partials      370      222     -148     

Impacted Files	Coverage Δ
transport/graphsync/graphsync.go	76.79% <ø> (ø)
message/message1_1prime/transfer_message.go	22.22% <33.33%> (+2.22%)	⬆️
message/message1_1prime/message.go	50.00% <34.14%> (+1.74%)	⬆️
message/message1_1prime/transfer_request.go	59.42% <38.88%> (-9.55%)	⬇️
message/message1_1prime/transfer_response.go	78.43% <45.45%> (-8.24%)	⬇️
registry/registry.go	65.21% <57.14%> (-9.79%)	⬇️
impl/restart.go	56.32% <64.70%> (-1.82%)	⬇️
channels/channel_state.go	64.19% <68.57%> (-11.13%)	⬇️
impl/receiving_requests.go	67.70% <87.09%> (+0.80%)	⬆️
channels/channels.go	70.79% <100.00%> (-0.64%)	⬇️
... and 6 more

[rvagg]

OK, this is getting closer I think. datatransfer.TypedVoucher is a tuple that binds a Node and a TypeIdentifier together and is now required through most of the API. There's some internal places where a Node will be by itself - they're decoupled inside messages (we could couple them there too, but it's not so important right now) and in validators - because you register a validator for a type so you probably don't need to see that type again, right?

Channels should now be able to support multiple voucher types (although this still isn't tested in here, but will be by go-fil-markets); and the serialization of the statemachine should be the same as before, I've reverted all changes to the channels/internal package.

[hannahhoward]

This is honestly looking really good. I'd like to go ahead and merge to v2 and you can cleanup in subsequent PRs.

[hannahhoward]

Why would someone pass a VoucherResult that wasn't nil but had a nil voucher? I guess that's a possibility and perhaps best to be defense against code supplied outside the library.

[hannahhoward]

I think this is ultimately the correct change and something we should have done a while ago. That said, keep in mind this will bubble up a lot of api changes. We had better factor in integration with Lotus + Filclient + Boost at a minimum in our timelines