[TECHDEBT] Persistence code-review follow (tend TODOs added via a PR rather than a code review)

[Olshansk]

Objective

Tend to and prioritize all of the issues introduced in #171.

Origin Document

1. There was a major PR that touched most configurations here: [Configuration] Genesis and Node Configurations (Issue#154) #166
2. In order to avoid time on conflict resolution after major code reviews, the code review was the in the form of TODO being left throughout the code here: [Configuration] PR166 Code Review (Issue#154 Follow ups) #171
3. After [Core] Minimize shared module #163 is complete, this [Configuration] PR166 Code Review (Issue#154 Follow ups) #171

Goals / Deliverables

• Remove all TODO(andrew) left throughout the codebase. At the time of creating this issue, the list is available below. Please note that some of these TODOs may seem like duplicates while others are left in one place but should be tended to elsewhere.

./consensus/module.go:	// TODO(andrew): Explain (or remove) why have an explicit `MaxBlockBytes` if we are already storing a reference to `consCfg` above?
./consensus/module.go:	// TODO(andrew): This needs to be updated every time the utility module changes this value. It can be accessed via the "application specific bus" (mimicking the intermodule interface in ABCI)
./shared/types/error.go:// TODO(andrew): move errors to respective modules; pocket/issues/163
./shared/types/error.go:// TODO(andrew): consolidate errors into one file after recovery; pocket/issues/163
./shared/types/genesis/proto/account.proto:// TODO(andrew): move to p2p module; pocket/issues;163
./shared/types/genesis/proto/actor.proto:// TODO(andrew): move to utility module; pocket/issues/163
./shared/types/genesis/proto/actor.proto:  // TODO(andrew): Rename `generic_param` to `actor_specific_param`
./shared/types/genesis/doc/CHANGELOG.md:// TODO(andrew): Use backticks around variable types to improve readability (e.g. see the first couple of examples)
./shared/types/block.go:// TODO(andrew): move block to consensus module; pocket/issues/163
./shared/tests/utility_module/transaction_test.go:// TODO(andrew): Fix this test once txIndexer is implemented by postgres context
./shared/tests/utility_module/module_test.go:// TODO(andrew): Take in `t` and fail the test if there's an error
./shared/tests/utility_module/module_test.go:	// TODO(andrew): Move the number of actors into local constants
./shared/tests/utility_module/module_test.go:	// TODO(andrew): Check for error
./shared/tests/utility_module/gov_test.go:// TODO(andrew): Remove the use of `require.True` and `require.False` in all cases.
./shared/tests/utility_module/gov_test.go:	// TODO(andrew): Use require.Equal and avoid the use of formatted strings. Ditto elsewhere.
./shared/tests/utility_module/gov_test.go:// TODO(andrew): Use reflection to iterate over all the params and test them.
./shared/tests/utility_module/account_test.go:// TODO(andrew): Remove all `require.True` in test files unless we're checking the value of a boolean
./shared/tests/utility_module/account_test.go:// TODO(andrew): Remove all `fmt.Sprintf(`
./shared/modules/persistence_module.go:// TODO(andrew): convert address and public key to string not bytes; pocket/issues/163
./shared/modules/persistence_module.go:	// TODO(andrew): remove address from pool; pocket/issues/163
./persistence/test/service_node_test.go:	// TODO(andrew): Remove all `typesGenesis` imports and use `genesis` instead.
./persistence/test/service_node_test.go:	// TODO(andrew): Use `require.NoError`
./persistence/test/application_test.go:	// TODO(andrew): Avoid the use of `log.Fatal(fmt.Sprintf`
./persistence/test/application_test.go:	// TODO(andrew): Use `require.NoError` instead of `log.Fatal` in tests`
./persistence/test/fisherman_test.go:	// TODO(andrew): Rename `addrBz` to `fishAddrBz` so tests are easier to read and understand. Ditto in all other locations.
./persistence/test/fisherman_test.go:	// TODO(andrew): Replace `fisherman` with `fishermans` for grammatic correctness
./persistence/test/validator_test.go:	// TODO(andrew): In order to make the tests clearer (here and elsewhere), use `validatorAddrBz` instead of `addrBz`
./persistence/test/generic_test.go:		// TODO(andrew): Be consistent with `GenericParam` and `ActorSpecificParam` throughout the codebase; preferably the latter.
./persistence/test/account_test.go:	// TODO(andrew): Find all places where we import genesis twice (like below) and update the imports appropriately.
./persistence/test/account_test.go:	// TODO(andrew): All `log.Fatal` calls should be converted to `require.NoError` calls.
./persistence/test/account_test.go:// TODO(andrew): consolidate newTestAccount and newTestPool into one function
./persistence/test/setup_test.go:// TODO(andrew): Take in `t testing.T` as a parameter and error if there's an issue
./persistence/test/setup_test.go:			// TODO(andrew): Use `require.Contains` instead
./persistence/account.go:// TODO(andrew): remove address param
./persistence/genesis.go:// TODO(andrew): Use `log.Fatalf` instead of `log.Fatal(fmt.Sprintf())`.
./persistence/genesis.go:// TODO(andrew): generalize with the `actors interface`
./persistence/genesis.go:	// TODO(andrew): genericize the genesis population logic for actors; pocket/issues/163
./persistence/genesis.go:// TODO(andrew): Consolidate with `GetAllAccounts`
./persistence/module.go:		DeferrableMode: pgx.Deferrable, // TODO(andrew): Research if this should be `Deferrable`
./persistence/module.go:		DeferrableMode: pgx.NotDeferrable, // TODO(andrew): Research if this should be `Deferrable`
./build/config/main.go:// TODO(andrew): Add a make target to help trigger this from cmdline
./utility/types/actor.go:// TODO(andrew): Moving this into a proto file enum (impacts everything) and making them `int32` by default
./utility/types/actor.go:// TODO(andrew): consolidate with genesis and `types/block.go`
./utility/actor.go:// TODO(andrew): Make sure the `er` value in all the functions here is used. E.g. It is not used in `GetMinimumPauseBlocks`.
./utility/actor.go:// TODO(andrew): Remove code that is unnecessarily repeated in this file. E.g. The number of times `store.GetHeight()` can be reduced in the entire file.
./utility/block.go:// TODO(andrew): consolidate with `utility/types/actor.go`

• Tend to these TODOs in small scoped PRs by module. There is no definitive rule of "only one module at a time" or "only one change at a time" but decision is left to the discretion of the author. Some PRs may be dependant on one another while others may be completely parallelizable. Module owners:
  • @andrewnguyen22 - Utility and Shared
  • @Olshansk - Consensus and Persistence
  • @deblasis - RPC and P2P
• Make PRs for small fixes that can be reviewed and merged immediately. Medium fixes can be tackled at the author's discretion or scoped and prioritized. Large fixes should be drafted as issues and prioritized by urgency to decide if we tackle them immediately after this tech debt iteration or at a later date

Please reach out to @Olshansk with any need for support or questions

Maybe goals

• Bonus points to addressing TODO(drewsky) items

Non goals

• Adding any new functionality

Testing

• All tests: make test_all
• LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md
  Remove

---

Creator: @Olshansk
Co-Owners: @andrewnguyen22

[andrewnguyen22]

@Olshansk This is in progress, but I'm not sure what this is exactly. Can we downscope or clarify or close?

[Olshansk]

@Olshansk This is in progress, but I'm not sure what this is exactly. Can we downscope or clarify or close?

Originally what happened was:

1. We merged in [Configuration] Genesis and Node Configurations (Issue#154) #166 without review
2. [Configuration] PR166 Code Review (Issue#154 Follow ups) #171 were TODOs (rather than GitHub comments) to get (1) in faster
3. [TECHDEBT] Persistence code-review follow (tend TODOs added via a PR rather than a code review) #172 was meant to tackle (2)

Since we're scoping down the iterative tasks though, the next steps are:

1. I change this to "Rescope"
2. Next week, I will review what's already done and "low hanging fruit" and create tickets for the larger items

[jessicadaugherty]

Rescoped by modules. Closing this issue.