A much smaller set of newly enabled linters (#1127)

* A much smaller set of newly enabled linters * Apply suggestions from code review Co-authored-by: Aleksandr Bezobchuk <[email protected]> * Update Make lint, disable gofumpt and godot until future * changelog Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: ValarDragon <[email protected]>
osmosis-labs · Mar 23, 2022 · e90fc2d · e90fc2d
1 parent d143d83
commit e90fc2d
Show file tree

Hide file tree

Showing 226 changed files with 1,070 additions and 937 deletions.
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -22,7 +22,7 @@ jobs:
       - uses: golangci/golangci-lint-action@v2
         with:
           # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
-          version: v1.39
+          version: v1.45
           args: --timeout 10m
           github-token: ${{ secrets.github_token }}
         if: "env.GIT_DIFF != ''"
diff --git a/.golangci.yml b/.golangci.yml
@@ -5,22 +5,95 @@ run:
 
 linters:
   disable-all: true
+  # Enable specific linter
+  # https://golangci-lint.run/usage/linters/#enabled-by-default-linters
   enable:
+    - asciicheck
+    - bidichk
+    - bodyclose
+    - contextcheck
+    #    - cyclop
+#    - deadcode <- re-enable in another pr
+    - depguard
+    #    - dogsled <- disabled because we would like to participate in the iditerod
+    # - dupl <- really just catches cli stub text
+    - durationcheck
     - errcheck
+    - errname
+#    - errorlint <- later
+#    - exhaustive <- too picky
+    #    - exhaustivestruct <- disabled because we don't want to use every element of every struct
+    - exportloopref
+    #    - forbidigo <- forbids fmt.Print* by default
+    - forcetypeassert
+    #    - funlen <- some of our functions are longer than this by necessity
+    #    - gci <- questionable utility
+    #    - gochecknoglobals <- disabled because we want some globals
+    #    - gochecknoinits <- disabled because we want to use init()
+    #    - gocognit <- disabled for same reason as gocyclo
+    - goconst
+    # - gocritic <- re enable later to catch capitalized local variables
+    #    - gocyclo <- disabled because it won't work with how we cosmos
+    # - godot
+    #    - godox <- detects TODO/BUG/FIXME and we may wantnthis later but isn't appropriate now
+    #    - goerr113 <- disabled due to lack of comprehension
     - gofmt
-    # - goimports
-    # - golint
-    - maligned
+    # - gofumpt
+    - goheader
+    - goimports
+    #    - gomoddirectives <- disables replaces
+    - gomodguard
+    - goprintffuncname
+#    - gosec <- triggers too much for this round and should be re-enabled later
+#    - gosimple <- later
+    - govet
+    #   - ifshort <- seems to be of questionable value
+    - importas
+    - ineffassign
+    #    - ireturn <- disabled because we want to return interfaces
+    #    - lll <- disabled as it is a bit much.  Maybe we have a desired limit?
+    - makezero
+    - misspell
+    - nakedret
+#    - nestif <- ?
+    - nilerr
+    - nilnil
+    #    - nlreturn <- disabled as it doesn't auto-fix something simple and doesn't add anything useful
+    - noctx
+    #    - nolintlint <- disabled because we use nolint in some places
+    - paralleltest
+    # - prealloc <- disabled because it makes simple code complicated
+#    - predeclared <- later
+    - promlinter
+#    - revive <- temporarily disabled while jacob figures out how to make poolId not trigger it.
+    - rowserrcheck
+    - sqlclosecheck
+#    - staticcheck <- later
+#    - structcheck <- later
+#    - stylecheck <- enable later, atriggers on underscores in variable names and on poolId
+    #    - tagliatelle <- disabled for defying cosmos idiom
+    - tenv
+    - testpackage
+#    - thelper <- later
+    - tparallel
+    - typecheck
+    - unconvert
+#    - unparam <- looks for unused parameters (enable later)
+#    - unused <- looks for unused variables (enable later)
+#    - varcheck <- later
+    #    - varnamelen <- disabled because defies idiom
+    #    - wastedassign
+    - whitespace
+#    - wrapcheck
+#    - wsl
 
 issues:
   exclude-rules:
     - path: bench_test\.go
       linters:
         - errcheck
+    - path: client/docs
+      linters:
+        - all
   max-issues-per-linter: 10000
   max-same-issues: 10000
-
-linters-settings:
-  maligned:
-    # print struct with more effective memory layout or not, false by default
-    suggest-new: true
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -52,6 +52,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * [#1095](https://github.com/osmosis-labs/osmosis/pull/1095) Fix authz being unable to use lockup & superfluid types.
 * [#1105](https://github.com/osmosis-labs/osmosis/pull/1105) Add GitHub Actions to automatically push the osmosis Docker image
 * [#1114](https://github.com/osmosis-labs/osmosis/pull/1114) Improve CI: remove duplicate runs of test worflow
+* [#1127](https://github.com/osmosis-labs/osmosis/pull/1127) Stricter Linting:  bump golangci-lint version and enable additional linters.
 
 ### SDK fork updates
 

diff --git a/Makefile b/Makefile
@@ -231,7 +231,7 @@ benchmark:
 ###############################################################################
 
 lint:
-	golangci-lint run --disable-all -E errcheck
+	golangci-lint run
 	find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" | xargs gofmt -d -s
 
 format:

diff --git a/app/app.go b/app/app.go
@@ -1,32 +1,32 @@
 package app
 
 import (
-	// Imports from the Go Standard Library
+	// Imports from the Go Standard Library.
 	"fmt"
 	"io"
 	"net/http"
 	"os"
 	"path/filepath"
 	"strings"
 
-	// HTTP Router
+	// HTTP Router.
 	"github.com/gorilla/mux"
 
-	// Used to serve OpenAPI information
+	// Used to serve OpenAPI information.
 	"github.com/rakyll/statik/fs"
 
-	// A CLI helper
+	// A CLI helper.
 	"github.com/spf13/cast"
 
-	// Imports from Tendermint, Osmosis' consensus protocol
+	// Imports from Tendermint, Osmosis' consensus protocol.
 	abci "github.com/tendermint/tendermint/abci/types"
 	tmjson "github.com/tendermint/tendermint/libs/json"
 	"github.com/tendermint/tendermint/libs/log"
 	tmos "github.com/tendermint/tendermint/libs/os"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 	dbm "github.com/tendermint/tm-db"
 
-	// Utilities from the Cosmos-SDK other than Cosmos modules
+	// Utilities from the Cosmos-SDK other than Cosmos modules.
 	"github.com/cosmos/cosmos-sdk/baseapp"
 	"github.com/cosmos/cosmos-sdk/client"
 	"github.com/cosmos/cosmos-sdk/client/grpc/tmservice"
@@ -51,36 +51,36 @@ import (
 	authrest "github.com/cosmos/cosmos-sdk/x/auth/client/rest"
 	authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
 
-	// Capability: allows developers to atomically define what a module can and cannot do
+	// Capability: allows developers to atomically define what a module can and cannot do.
 	capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
 
 	// Crisis: Halting the blockchain under certain circumstances (e.g. if an invariant is broken).
 	"github.com/cosmos/cosmos-sdk/x/crisis"
 
 	// Evidence handling for double signing, misbehaviour, etc.
 
-	// Params: Parameters that are always available
+	// Params: Parameters that are always available.
 	paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"
 
 	// Upgrade:  Software upgrades handling and coordination.
 	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 
-	// IBC Transfer: Defines the "transfer" IBC port
+	// IBC Transfer: Defines the "transfer" IBC port.
 	transfer "github.com/cosmos/ibc-go/v2/modules/apps/transfer"
 
-	// Osmosis application prarmeters
+	// Osmosis application prarmeters.
 	appparams "github.com/osmosis-labs/osmosis/v7/app/params"
 
-	// Upgrades from earlier versions of Osmosis
+	// Upgrades from earlier versions of Osmosis.
 	v4 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v4"
 	v5 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v5"
 	v7 "github.com/osmosis-labs/osmosis/v7/app/upgrades/v7"
 	_ "github.com/osmosis-labs/osmosis/v7/client/docs/statik"
 
-	// Superfluid: Allows users to stake gamm (bonded liquidity)
+	// Superfluid: Allows users to stake gamm (bonded liquidity).
 	superfluidtypes "github.com/osmosis-labs/osmosis/v7/x/superfluid/types"
 
-	// Wasm: Allows Osmosis to interact with web assembly smart contracts
+	// Wasm: Allows Osmosis to interact with web assembly smart contracts.
 	"github.com/CosmWasm/wasmd/x/wasm"
 )
 
@@ -95,7 +95,7 @@ var (
 	// https://github.com/CosmWasm/wasmd/blob/02a54d33ff2c064f3539ae12d75d027d9c665f05/x/wasm/internal/types/proposal.go#L28-L34
 	EnableSpecificWasmProposals = ""
 
-	// use this for clarity in argument list
+	// use this for clarity in argument list.
 	EmptyWasmOpts []wasm.Option
 )
 
@@ -117,18 +117,18 @@ func GetWasmEnabledProposals() []wasm.ProposalType {
 }
 
 var (
-	// DefaultNodeHome default home directories for the application daemon
+	// DefaultNodeHome default home directories for the application daemon.
 	DefaultNodeHome string
 
 	// ModuleBasics defines the module BasicManager is in charge of setting up basic,
 	// non-dependant module elements, such as codec registration
 	// and genesis verification.
 	ModuleBasics = module.NewBasicManager(appModuleBasics...)
 
-	// module account permissions
+	// module account permissions.
 	maccPerms = moduleAaccountPermissions
 
-	// module accounts that are allowed to receive tokens
+	// module accounts that are allowed to receive tokens.
 	allowedReceivingModAcc = map[string]bool{}
 )
 
@@ -188,7 +188,6 @@ func NewOsmosisApp(
 	wasmOpts []wasm.Option,
 	baseAppOptions ...func(*baseapp.BaseApp),
 ) *OsmosisApp {
-
 	appCodec := encodingConfig.Marshaler
 	cdc := encodingConfig.Amino
 	interfaceRegistry := encodingConfig.InterfaceRegistry
@@ -235,7 +234,7 @@ func NewOsmosisApp(
 
 	// NOTE: we may consider parsing `appOpts` inside module constructors. For the moment
 	// we prefer to be more strict in what arguments the modules expect.
-	var skipGenesisInvariants = cast.ToBool(appOpts.Get(crisis.FlagSkipGenesisInvariants))
+	skipGenesisInvariants := cast.ToBool(appOpts.Get(crisis.FlagSkipGenesisInvariants))
 
 	// NOTE: All module / keeper changes should happen prior to this module.NewManager line being called.
 	// However in the event any changes do need to happen after this call, ensure that that keeper
@@ -327,27 +326,27 @@ func NewOsmosisApp(
 
 // MakeCodecs constructs the *std.Codec and *codec.LegacyAmino instances used by
 // simapp. It is useful for tests and clients who do not want to construct the
-// full simapp
+// full simapp.
 func MakeCodecs() (codec.Codec, *codec.LegacyAmino) {
 	config := MakeEncodingConfig()
 	return config.Marshaler, config.Amino
 }
 
-// Name returns the name of the App
+// Name returns the name of the App.
 func (app *OsmosisApp) Name() string { return app.BaseApp.Name() }
 
-// BeginBlocker application updates every begin block
+// BeginBlocker application updates every begin block.
 func (app *OsmosisApp) BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock) abci.ResponseBeginBlock {
 	BeginBlockForks(ctx, app)
 	return app.mm.BeginBlock(ctx, req)
 }
 
-// EndBlocker application updates every end block
+// EndBlocker application updates every end block.
 func (app *OsmosisApp) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
 	return app.mm.EndBlock(ctx, req)
 }
 
-// InitChainer application update at chain initialization
+// InitChainer application update at chain initialization.
 func (app *OsmosisApp) InitChainer(ctx sdk.Context, req abci.RequestInitChain) abci.ResponseInitChain {
 	var genesisState GenesisState
 	if err := tmjson.Unmarshal(req.AppStateBytes, &genesisState); err != nil {
@@ -359,7 +358,7 @@ func (app *OsmosisApp) InitChainer(ctx sdk.Context, req abci.RequestInitChain) a
 	return app.mm.InitGenesis(ctx, app.appCodec, genesisState)
 }
 
-// LoadHeight loads a particular height
+// LoadHeight loads a particular height.
 func (app *OsmosisApp) LoadHeight(height int64) error {
 	return app.LoadVersion(height)
 }
@@ -380,7 +379,7 @@ func (app *OsmosisApp) AppCodec() codec.Codec {
 	return app.appCodec
 }
 
-// InterfaceRegistry returns Osmosis' InterfaceRegistry
+// InterfaceRegistry returns Osmosis' InterfaceRegistry.
 func (app *OsmosisApp) InterfaceRegistry() types.InterfaceRegistry {
 	return app.interfaceRegistry
 }
@@ -414,7 +413,7 @@ func (app *OsmosisApp) GetSubspace(moduleName string) paramstypes.Subspace {
 	return subspace
 }
 
-// SimulationManager implements the SimulationApp interface
+// SimulationManager implements the SimulationApp interface.
 func (app *OsmosisApp) SimulationManager() *module.SimulationManager {
 	return app.sm
 }
@@ -495,7 +494,7 @@ func (app *OsmosisApp) setupUpgradeHandlers() {
 			app.AccountKeeper))
 }
 
-// RegisterSwaggerAPI registers swagger route with API Server
+// RegisterSwaggerAPI registers swagger route with API Server.
 func RegisterSwaggerAPI(ctx client.Context, rtr *mux.Router) {
 	statikFS, err := fs.New()
 	if err != nil {
@@ -507,7 +506,7 @@ func RegisterSwaggerAPI(ctx client.Context, rtr *mux.Router) {
 	rtr.PathPrefix("/swagger/").Handler(staticServer)
 }
 
-// GetMaccPerms returns a copy of the module account permissions
+// GetMaccPerms returns a copy of the module account permissions.
 func GetMaccPerms() map[string][]string {
 	dupMaccPerms := make(map[string][]string)
 	for k, v := range maccPerms {

diff --git a/app/apptesting/test_suite.go b/app/apptesting/test_suite.go
@@ -66,7 +66,7 @@ func (keeperTestHelper *KeeperTestHelper) SetupValidator(bondStatus stakingtypes
 }
 
 func (keeperTestHelper *KeeperTestHelper) BeginNewBlock(executeNextEpoch bool) {
-	valAddr := []byte(":^) at this distribution workaround")
+	valAddr := []byte(":^) at this distribution workaround") //nolint
 	validators := keeperTestHelper.App.StakingKeeper.GetAllValidators(keeperTestHelper.Ctx)
 	if len(validators) >= 1 {
 		valAddrFancy, err := validators[0].GetConsAddr()
@@ -91,9 +91,11 @@ func (keeperTestHelper *KeeperTestHelper) BeginNewBlock(executeNextEpoch bool) {
 	newCtx := keeperTestHelper.Ctx.WithBlockTime(newBlockTime).WithBlockHeight(keeperTestHelper.Ctx.BlockHeight() + 1)
 	keeperTestHelper.Ctx = newCtx
 	lastCommitInfo := abci.LastCommitInfo{
-		Votes: []abci.VoteInfo{{
-			Validator:       abci.Validator{Address: valAddr, Power: 1000},
-			SignedLastBlock: true},
+		Votes: []abci.VoteInfo{
+			{
+				Validator:       abci.Validator{Address: valAddr, Power: 1000},
+				SignedLastBlock: true,
+			},
 		},
 	}
 	reqBeginBlock := abci.RequestBeginBlock{Header: header, LastCommitInfo: lastCommitInfo}
@@ -129,10 +131,9 @@ func (keeperTestHelper *KeeperTestHelper) SetupGammPoolsWithBondDenomMultiplier(
 	})
 
 	bondDenom := keeperTestHelper.App.StakingKeeper.BondDenom(keeperTestHelper.Ctx)
-	//TODO: use sdk crypto instead of tendermint to generate address
+	// TODO: use sdk crypto instead of tendermint to generate address
 	acc1 := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())
 
-	//fund account with pool creation fee
 	poolCreationFee := keeperTestHelper.App.GAMMKeeper.GetParams(keeperTestHelper.Ctx)
 	err := simapp.FundAccount(keeperTestHelper.App.BankKeeper, keeperTestHelper.Ctx, acc1, poolCreationFee.PoolCreationFee)
 	keeperTestHelper.Require().NoError(err)
@@ -180,7 +181,7 @@ func (keeperTestHelper *KeeperTestHelper) SetupGammPoolsWithBondDenomMultiplier(
 }
 
 // SwapAndSetSpotPrice runs a swap to set Spot price of a pool using arbitrary values
-// returns spot price after the arbitrary swap
+// returns spot price after the arbitrary swap.
 func (keeperTestHelper *KeeperTestHelper) SwapAndSetSpotPrice(poolId uint64, fromAsset sdk.Coin, toAsset sdk.Coin) sdk.Dec {
 	// create a dummy account
 	acc1 := sdk.AccAddress(ed25519.GenPrivKey().PubKey().Address().Bytes())

diff --git a/app/encoding.go b/app/encoding.go
@@ -5,7 +5,7 @@ import (
 	"github.com/osmosis-labs/osmosis/v7/app/params"
 )
 
-// MakeEncodingConfig creates an EncodingConfig for testing
+// MakeEncodingConfig creates an EncodingConfig for testing.
 func MakeEncodingConfig() params.EncodingConfig {
 	encodingConfig := params.MakeEncodingConfig()
 	std.RegisterLegacyAminoCodec(encodingConfig.Amino)