[EPIC] Coding style/standards type conversions.

[joroshiba]

Problem

There is a lack of consistency between how conversions on types are being defined across the repo. It not being all aligned on the same standard easily can lead to duplication, and this has already happened: with Block there are conversions in the conv/abci to convert to and from ABCI types. But when the hash of the block is calculated, there is also a recreated conversion.

Marshaling to binary occurs in a separate file, and within this file an ABCI type converted to proto whereas our block from ABCI types exist in an entirely different module. Meanwhile state.go has a function NewFromGenesisDoc defined on the types module. A call to types.NewFromGenesisDoc (link) leaves me asking "new what" without analyzing surrounding code.

In order to add a field to 2 different types (and conversion between 3) I had to change conversions between types in 5 different files. I think I found them all, but it's hard to know with certainty. I know I missed the marshaling changes needed initially only based on manual testing. So my high-level thought there is that all conversions between types should follow the same pattern, finding the code should be easy, so no one recreates the code, or misses important places to change code. Implementation-wise, I think there is room for standards discussion to be had on how that should look.

Standards Opinion

I would argue that the most common pattern in imported libraries I've used is for the conversions to exist within the same module and file as their defined type, and to exist as methods on the type. Some related prior art: Geth Blocks, Tendermint Blocks.

I'd go a step further and suggest that it's not absurd if there are many types (ie not a single types.go file) that each exported type has its own file by default. As shown in the above links there is prior art on Block and Header being in the same file, having colocated types when they are so highly connected can be fine. Generally from a code path/search perspective though it's nice to be able to find a file based on the type name.

Note I'm using standards in quotes because I really think these are just guidelines that could be used for making modifications to codebase to improve it, and as a guideline for any nits in code review process. It's really just a shitty style guide defini

Types/Type Conversion Style Guide Proposal

1. Methods on a core type (within the types module) should be defined in the file which defines the type.
2. Unless there is significant enough logical overlap there should be one exported type, matching file name, per file in the types directory.
3. Whether or not functions are exported is up to programming need. All naming forms below use unexported capitalization, this isn't required.
4. Conversions between types where both are fully owned types should be of the form to{TypeName} and exist on the type being converted format.
5. When converting from an imported type either:

• Define a method on the local type, which modifies an instance of the type called from{ImportedType}
• Create a wrapped type and follow all local type standards for it.
• Define a function inside the local type file of the form new{Type}From{ImportedType} which returns a new instance of the type

6. When converting between imported types either:

• Define one or both of the types as wrapped types and follow local typing standards on the wrapped type
• Define a function in the conv module that returns a new object of the form new{ImportedType1}From{ImportedType2}

With this "standard", in my example PR for type conversion purposes, I should have ONLY had to touch either the blocks.go or a (nonexistent) header.go file, since all functions were practical functions on the Header type (1 file not 5). The conv/abci module would be able to be removed, and its functions moved to the file of their correct. The conv module would continue to exist but if the imported type starts to get many functions created to interact with it, would be appropriate to migrate to a wrapped type.