approach to constants about go-multihash HOT 6 OPEN

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.

from go-multihash.

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

from go-multihash.

(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.

from go-multihash.

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 🤷.

from go-multihash.

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?

from go-multihash.

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

from go-multihash.