feat(bindnode): add a BindnodeRegistry utility

[rvagg]

For centralising bindnode setup - register a type & schema once and not have to
worry about the TypedPrototype or direct bindnode calls after that. It also
caches bindnode Options that may go along with the type.

This could have been a global registry, but there exists a possibility of the
same type wanting to be used by different users in the same application
instance but with different schemas and/or Options. So instead, the question of
it being a global is left to the user.

---

This is the core of work I've been distilling in go-fil-markets and go-data-transfer and is an evolution of something that's currently in go-graphsync. There's really not a lot to this, it just simplifies the setup management, and when we have Options involved too it makes it a lot easier than needing them whenever you need to do a Wrap().

This could also set up for more efficiencies as detailed in #416.

TODO: Tests!

[willscott]

do you have an example of using this?

[rvagg]

do you have an example of using this?

Why yes I do: filecoin-project/go-fil-markets#713

That's where I evolved it out of. Look in the two types.go files for the setup of it - declaration as a global var BindnodeRegistry for both storagemarket and retrievalmarket (separately), and registry of the types using an init(). retrievalmarket/types.go has the most interesting types because they need converters too.

[hannahhoward]

I wonder whether it would make better sense to index on a string type name than a reflect type?

I think that would have the effect of making the TypeFrom commands simpler but the TypeTo commands would require an extra parameter. I've never been a huge fan of maps indexed on reflect.Type

[hannahhoward]

there are a LOT of panics in this function and I wonder if it should just return error and let the caller panic if it wants.

[rvagg]

Yeah .. there's also a lot of panics inside bindnode.Prototype() that's called from in here - if we error out of this call, do we also recover and return those as errors too? I'm not a big fan of all this panic stuff but I get it that err handling in Go still sucks terribly, and all of the panics here and inside bindnode are all things you're going to hit as a programmer error or a setup error, which should happen very early in your application and you really shouldn't be letting continue in production.

[rvagg]

I wonder whether it would make better sense to index on a string type name than a reflect type?

The reason for using reflect.Type is to get maximal uniqueness for the type concerned. There is a type.PkgPath() API in reflect that gets us as close as possible to that in string form, but once we're dealing with that then we're already at reflect.Type.

I think that would have the effect of making the TypeFrom commands simpler but the TypeTo commands would require an extra parameter. I've never been a huge fan of maps indexed on reflect.Type

If you mean that the user should supply the name rather than it being generated, then I'm not sure it makes anything simpler other than swapping out some calls from a pointer (usually null or empty object pointer) to a string, but it front-loads that string to the Register setup, where you need to supply the type:name tuple rather than having it generated.

But if you're suggesting that the user should be able to register with (*Address)(nil) (or &Address{}) but supply "Address" as an argument then we're at the uniqueness problem. What if there are two Address types in operation, PkgPath() is supposed to deal with that but does that mean we're asking the user to use "github.com/filecoin-project/go-state-types/address/Address" (or wherever it is)?

I'm not sure that strings help us much here but reflect.Type is simple enough internally that it should be safe to index. (Also mentioned by Dan here as a suggestion for internal caching that's not too dissimilar to what's going on here). Unless I'm misunderstanding the suggestion?

[rvagg]

@hannahhoward @willscott maybe some high-level feedback would be helpful here:

1. Is this API broadly comfortable, or is it awkward in any way that could be solved by pushing it around or just not having this abstraction at all.
2. Does it belong in go-ipld-prime, or should I make a github.com/ipld/go-bindnode-utils package to shunt this too. There's something not quite go-ipld-prime about it, although it's got a roughly similar feel to some of the things in the /storage/ sub-packages - but they are true sub-packages with their own go.mods.

I'm fairly happy with it as it is, I think it could be improved and I see some avenues to improve internal performance based on the external API, but I iterated through many variations of this API to boil it down to this which is useful across the 3 main uses of bindnode we have in the data transfer stack so far. But I don't have high confidence in merging this into go-ipld-prime as a great fit. Adding EXPERIMENTAL will probably help with that though.

[rvagg]

• RegisterType() now returns an error and additionally captures local bindnode panics and turns them into errors too.
• Added tests for full coverage (minus a couple of error conditions)