Improvement ideas

[husio]

This issue is a container for ideas on how the application and the code could be improved. Not everything might be valid as I am learning the code base.

• use goimport as your code formatter 😉

• Always wrap returned errors. Doing if err != nil { return err } does not include each callers' context. Pushing errors up the stack without context makes it harder to test and debug. Additionally, a short context description makes it easier for the reader to understand the code. Example:
  

  wasmd/x/wasm/keeper/keeper.go

  Lines 334 to 338 in 93e2e66

  	if !coins.IsZero() {
  	if err := k.bank.TransferCoins(ctx, caller, contractAddress, coins); err != nil {
  	return nil, err
  	}
  	}

  
  It would be an improvement to return

   return nil, sdkerror.Wrap(err, "lock contract coins")

  Please notice that fmt.Errorf is not used, because the error handling predates fmt.Errorf and errors.Is

• Update tests so that mock objects own the data they modify. To describe the case, let's focus on a single test:
  

  wasmd/x/wasm/keeper/handler_plugin_test.go

  Lines 72 to 75 in 93e2e66

  	for name, spec := range specs {
  	t.Run(name, func(t *testing.T) {
  	*gotMsgs = make([]wasmvmtypes.CosmosMsg, 0)

  
  gotMsgs is a reference to a slice that is carried and modified by NewCapturingMessageHandler. Because it is used in all test cases, internal state of the mock requires resetting.
  I think it would be cleaner to apply two modifications to such test:
  1. make each mock own its internal state. In this case, instead of passing gotMsgs reference, let the mock own the slice and expose it via an attribute or a method.
  2. where can be done, move mock creation inside of each test case. Sharing mutable mocks breaks the islocation and tests can influence one another. When mocks are isolated, there is no need to reset them.

• Limit the use of aliases, when not used during the refactoring process.
  I only briefly know the alias functionality story in Go. Apparently in 1.9 the current implementation fianlly landed (issue and proposal).
  My understanding of alias functionality is that this functionality is meant as a temporary mean during big code base refactoring. Using an alias allows gradually introducing an otherwise breaking change.
  x/wasmd has a dedicated alias.go file. It seems like this file is a conscious decision rather than an in process refactoring state. Not sure what are consequences of this approach, maybe it is something worth looking deeper into.

[ethanfrey]

return nil, fmt.Errorf("lock contract coins: %w", err)

The comment is valid, but syntax-wise we we wrap errors (they ported much of that from weave), so it would be sdkerrors.Wrap(err, "lock contract coins") or something like that.

Whether we want to wrap on every level is a question for Alex

[alpe]

Good suggestions. Sorry for my late response

goimports

Unless we fail the build it is hard to enforce this. My IDE handles/hides imports for me. I will check if there is a way to configure this.

Always wrap returned errors.

We avoid "naked" errors mostly. Let's have an 👁️ on this and improve the code as we go. As Ethan wrote: sdkerrors.Wrap is the best practice in this project.

Update tests so that mock objects own the data they modify.

This was one of the test patterns that evolved. They are effective but I see your point that this is hard to read. 🤔 Let's find some better template as we go and discuss.

Limit the use of aliases

This is some cosmos module "standard" to provide them in the module root. This avoids some imports in the "/app" . The alias generator does not work on my box and seems not to be maintained. I had already stopped adding more to 'alias.go`. ;-)

[alpe]

PR #645 includes go imports
Added issue #654 for some developer guide doc