multiformats/go-multihash

approach to constants

Opened this issue · 6 comments

warpfork commented

We need to talk about how this library approaches constants.

Currently:

  • There are a bunch of exported caps-lock constant names in the root multihash package, mapping to mulithash indicator codes.
    • Mostly these are caps-lock and s/-/_/g of the names in the multiformats table...
    • But there are also some others, which point to other sibling constants, and are commented as "DEPRECATED" (...what's the point?)
    • There are some other consts that are referring to ranges rather than specific indicator codes (e.g. for the blakes)
    • There are many multihash indicator code numbers which have a function for them made available by the library, but don't have constants (again, the blakes)
  • There's a Codes = map[uint64]string.
    • But it's only good for the things this library knows about (which is not 100% in sync with the table)
  • There's a Names = map[string]uint64, which is the dual of Codes.

... so, there are numerous consistency issues with the current status quo, and it's very manual to update.

And we have a recently born go-multicodec repo, which contains a bunch of constants!

  • it's automatically generated from the csv table! Super nice!
  • contains no other functionality but the constants. Trivial dependency weight (outside of adding a line item to the go.mod file).
  • (one tangent: I wonder if we should rename this repo to go-multiformat. There was pretty resounding agreement that the word multicodec may be confusingly used to describe too many things the last time the IPLD team talked about this. But I also don't feel the need to hold up any other discussion this / if we do engage this, let's spawn another issue for that conversation.)

And, these questions are all raised afresh by #136 -- wherein we've gotta use something for constants in various places; and the revisitation of the registration features means it's a very good time to check in on where we expect any of these mappings to string names to be handled.

Okay, that's all the background. What shall we do? Discuss.

warpfork commented

I think it's probably reasonable to switch over to using go-multicodec already. Having something auto-generated is very nice.

On the other hand, it's another repo and another line item in go.mod.

If we do switch over to using go-multicodec, a few subquestions would remain:

  • go-multicodec offers a type Code uint64 typedef. Should we start using that type name everywhere instead of bare uint64?
  • go-multicodec contains all the indicator codes -- not just the ones that indicate hashes! Should we... try to do something about that, to attempt to make things carry more meaningful suggestive power?
    • (n.b. I think there's a limit to how much compile-time checking we can have, or even want, here. Too much compile time checking inhibits forward compatibility. But no hinting at all about which numbers are known to be sane to use as multihash indicators is also unfortunate.)
  • would we actually remove the current constants? (I vote yes, because migrating one constant to another is just easy peasy, and marking a ton of things "deprecated" and never actually removing them has more negative value in confusion produced than positive value for migration effort avoidance.)
  • see question in initial post about whether go-multicodec repo name might itself be confusing -- if we ever want to address that, best to do it immediately before spreading more usage of it.
willscott commented

would strongly suggest not having #136 block on this. I think we should spend our time on

  1. choosing a preferred exported view of constants
  2. migrating consumer implementations to that exported view
  3. deprecating other exported constants
    but that solving this doesn't need to block the the current refactor, because while it's a bit ugly to still have a few exports it helps provide backwards compatibility to many consumers that are just using current constants from the library, and isn't overly harmful with decent documentation
Stebalien commented

(one tangent: I wonder if we should rename this repo to go-multiformat. There was pretty resounding agreement that the word multicodec may be confusingly used to describe too many things the last time the IPLD team talked about this. But I also don't feel the need to hold up any other discussion this / if we do engage this, let's spawn another issue for that conversation.)

Strong disagreement. These are called multicodecs and the only confusion in IPLD is that IPLD also has "codecs" (which should probably be called formats". We could call them something else (multicodes?) but it's likely too late to change them and go-multiformats would imply that this library contains all the multiformats projects.

aschmahmann commented

would strongly suggest not having #136 block on this.

I think there's a minimal subset of this issue that needs to be resolved before closing #136, or at least before doing a release of go-multihash containing #136.

For example, if we're not going to be putting DefaultLengths for hash functions into the code table (which seems like a thing we shouldn't need to do as the spec behind the hash function already conveys that information) then the Register function needs to be augmented in #136 or we'll have to perform another breaking change. Similarly, we need some direction of what's going to happen with the Codes and Names maps. For example, if the plan is to not include names in the Register function, but also keep those maps around in the short term then Register should probably return an error (or panic I guess) if the name is undefined.

go-multicodec contains all the indicator codes -- not just the ones that indicate hashes! Should we... try to do something about that, to attempt to make things carry more meaningful suggestive power?

Probably, it seems to me at least somewhat desirable to make more information from the code table easier for developers to access than searching through a CSV (e.g. through their development tools). Thankfully we already have the Tag field in the CSV the can be used to autogenerate things here. An example codegen scheme might look like:

Input: (Name, Tag, Code, Description) ->

package {Tag}

type Code uint64 // hopefully we'll never have to exceed this since it's technically a varint 😄

// {Name} has description: {Description}
const {Name} Code = {Code}

Note: we could also duplicate the code in a generic list of all codes and/or supply maps for convenience of anyone who wants to iterate through the codes.

would we actually remove the current constants?

I have no particularly strong preferences here. I get not wanting to break things when there's a totally plausible alternative where things don't break for users (e.g. just marking multicode constants as deprecated and forwarding them to the correct package). However, if a plausible and easy enough to use alternative is available pushing people in the right direction might be worthwhile. I also want to point out that breaking things might be a good idea in the event people were for some reason relying on bad behaviors (e.g. assuming that all valid multihash codes MUST be in the Names/Codes maps and that therefore failing a map check means the code number isn't a hash - like if someone manually made a copy of the ValidCode function).

about whether go-multicodec repo name might itself be confusing -- if we ever want to address that, best to do it immediately before spreading more usage of it.

Maybe, although no strong preferences from me at the moment on the naming. I get @vmx's previous suggestion of referring to items in the table as "multicodes". I think this brings us to then describing a particular code as an "x multicode", where x is effectively the tag in the multicode table (e.g. namespace, multihash, ipld, etc.). I'm not sure if those tags are totally accurate (e.g. are they "hash" codes or "multihash" codes, should the CID codes be tagged the same way as the IPLD codec codes, etc.), but perhaps it's a start 🤷.

Stebalien commented

For example, if we're not going to be putting DefaultLengths for hash functions into the code table (which seems like a thing we shouldn't need to do as the spec behind the hash function already conveys that information) then the Register function needs to be augmented in #136 or we'll have to perform another breaking change. Similarly, we need some direction of what's going to happen with the Codes and Names maps. For example, if the plan is to not include names in the Register function, but also keep those maps around in the short term then Register should probably return an error (or panic I guess) if the name is undefined.

I believe Hash.Size() covers this, right?

aschmahmann commented

@Stebalien Ya, I think you're right. Will comment in the PR