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

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

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented May 18, 2022

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: ipld/go-ipld-prime#415, which itself builds on ipld/go-ipld-prime#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: ipld/go-ipld-prime#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
Copy link
Collaborator

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:

  • the GO types in the structure were already CBOR-Gen compatible. 414 enables the types that are CBOR-gen comptabile but not standard data types (big.Int/tokenAmount, Address, Signature) to work with bindnode.
  • the selector has gone from cbg.Deferred to shared.SerializedNode. This one's a bit confusing since it was never really fully compatible with CBOR-gen, and we were dealing with that by just not deserializing it till later using CBG.deferred. A datamodel.Node is actually the proper type, but this has the problem that it's NOT cbor-gen compatible. So 415 enables us to wrap datamodel.Node inside of shared.SerializedNode which implements the methods for CBOR-gen comptability. Under the hood, shared.SerializedNode uses CBG.deferred in its deserialization step. I wouldn't neccesarily say we're "working with cbor-gen's deferred types" -- in this case we didn't really want to defer deserialization -- it's just that actually, we couldn't get to datamodel.Node in a single step -- which now we can sort of, which is actually better :)

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

@hannahhoward
Copy link
Collaborator

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
Copy link
Member Author

rvagg commented May 20, 2022

renamed SerializedNode & added additional comments

@rvagg
Copy link
Member Author

rvagg commented Jun 2, 2022

Updated to current filecoin-project/go-data-transfer#325

@hannahhoward hannahhoward marked this pull request as ready for review June 2, 2022 23:50
@rvagg rvagg changed the base branch from remove-old-types to feat/datatransfer-v2-integration June 3, 2022 00:59
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THis is looking pretty great. I need to once over again, and I think we should really figure out how to remove deduplication around bindnode embedded schemas between this and go-data-transfer, but otherwise pretty good to go.

I'm fine with merging into the base branch if you want to address any issues in a separate ticket.

retrievalmarket/impl/client.go Outdated Show resolved Hide resolved
retrievalmarket/impl/dtutils/dtutils.go Show resolved Hide resolved
retrievalmarket/impl/ipld_compat_test.go Show resolved Hide resolved
retrievalmarket/impl/providerstates/provider_states.go Outdated Show resolved Hide resolved
retrievalmarket/types.go Outdated Show resolved Hide resolved
retrievalmarket/types.go Outdated Show resolved Hide resolved
retrievalmarket/types.go Outdated Show resolved Hide resolved
shared/ipldutils.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

❗ 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           

@rvagg rvagg force-pushed the rvagg/bindnode branch 2 times, most recently from 655f4f6 to 43e2645 Compare June 3, 2022 10:04
@rvagg rvagg force-pushed the rvagg/bindnode branch 6 times, most recently from 7765cd2 to 3376176 Compare June 10, 2022 06:40
@hannahhoward hannahhoward merged commit 727a2b1 into feat/datatransfer-v2-integration Jul 28, 2022
hannahhoward added a commit that referenced this pull request Oct 12, 2022
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps
hannahhoward added a commit that referenced this pull request Oct 12, 2022
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps
hannahhoward added a commit that referenced this pull request Oct 12, 2022
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps
hannahhoward added a commit that referenced this pull request Oct 14, 2022
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps
hannahhoward added a commit that referenced this pull request Nov 17, 2022
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps
hannahhoward added a commit that referenced this pull request Jan 7, 2023
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps
hannahhoward added a commit that referenced this pull request Feb 11, 2023
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps

Update retrievalmarket/types.go

Update storagemarket/types.go

chore(deps): update to rc candidate

chore(dtutils): update for new data transfer configuration interface
hannahhoward added a commit that referenced this pull request Feb 11, 2023
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps

Update retrievalmarket/types.go

Update storagemarket/types.go

chore(deps): update to rc candidate

chore(dtutils): update for new data transfer configuration interface
hannahhoward added a commit that referenced this pull request Feb 18, 2023
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps

Update retrievalmarket/types.go

Update storagemarket/types.go

chore(deps): update to rc candidate

chore(dtutils): update for new data transfer configuration interface

chore(deps): update to rc4
hannahhoward added a commit that referenced this pull request Feb 18, 2023
Voucher refactor integration (#707)

* refactor(storagemarket): update storagemarket

* refactor(retrievalmarket): use new voucher system

refactor for predictability and correctness using new voucher system

* chore(deps): update go-data-transfer v2

* style(lint): prep for pr

* docs(retrievalmarket): add comments

* chore(deps): update go-statemachine

* style(imports): fix imports

chore(retrievalmarket): remove old types (#712)

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

* feat(ipld): new data-transfer ipld vouchers + bindnode

* feat(ipld): simplify ipldutils API

* feat(ipld): use new bindnode registry in go-ipld-prime

Ref: ipld/go-ipld-prime#437

feat(deps): update data transfer 61f0756c

feat(deps): update data transfer and other deps

update to master data transfer with libp2p v0.22.0 plus associated other deps

Update retrievalmarket/types.go

Update storagemarket/types.go

chore(deps): update to rc candidate

chore(dtutils): update for new data transfer configuration interface

chore(deps): update to rc4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants