[Linting] Enforce 120 max lines, and import order with custom linting

.github/workflows/linter.yml

@@ -0,0 +1,84 @@
+# The reason the linting step has been moved to its own file is due to this
+# aside in the golangci-lint action repo underneath the first code block:
+# Ref: https://github.com/golangci/golangci-lint-action#how-to-use
+# tl;dr: "We recommend running this action in a job separate from other jobs
+# (go test, etc) because different jobs run in parallel."
+
+Name: Linter
+
+on:
+  push:
+    branches: ["main"]
+  pull_request:
+
+concurrency:
+  group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }}
+  cancel-in-progress: true
+
+# Ref: https://github.com/golangci/golangci-lint-action#comments-and-annotations
+permissions:
+  contents: read
+  pull-requests: read
+  checks: write
+
+jobs:
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      # Firstly setup the repo to a point where golangci-lint is able to lint
+      # properly without getting typeerror's from revive everywhere.
+      - name: install ignite
+        # TODO_TECHDEBT: upgrade to the latest Ignite (the latest at the moment of creating a note is 0.28). Need to downgrade to fix CI pipelines. Might be done in scope of #240.
+        run: |
+          # curl https://get.ignite.com/cli! | bash
+          wget https://github.com/ignite/cli/releases/download/v0.27.2/ignite_0.27.2_linux_amd64.tar.gz
+          tar -xzf ignite_0.27.2_linux_amd64.tar.gz
+          sudo mv ignite /usr/local/bin/ignite
+          ignite version
+
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: "0" # Per https://github.com/ignite/cli/issues/1674#issuecomment-1144619147
+
+      - name: Set up Go
+        uses: actions/setup-go@v4
+        with:
+          go-version: "1.20.10"
+          cache: false # Per https://github.com/golangci/golangci-lint-action#how-to-use
+
+      - name: Install CI dependencies
+        run: make install_ci_deps
+
+      - name: Generate protobufs
+        run: make proto_regen
+
+      # Fetch a list of all modified files in this PR.
+      - name: Get Modified Files
+        run: git diff --name-only ${{ github.event.before }} ${{ github.sha }} > modified_files.txt
+
+      # Filter the list of files modified in this PR to exclude those that have
+      # ignite scaffold comments in their import block, this is because gci
+      # (golangci-lint's import formatter) doesn't support this kind of filtering
+      # and removes all comments from import blocks.
+      - name: Filter For Lintable Files
+        run: go run ./tools/scripts/scaffold_filter/main.go $(cat modified_files.txt) > files_to_lint.txt
+
+      # Run golangci-lint on the filtered files from this PR - allowing gci to
+      # catch any improperly formatted import blocks in this PR.
+      - name: Run golangci-lint (filtered for scaffold comments)
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: latest
+          args: --timeout=15m --config=.golangci.yml $(cat files_to_lint.txt)
+          only-new-issues: true
+
+      # Do a second lint pass without import checking (after filtered lint) to
+      # catcy any linter errors in files that were excluded in the first pass.
+      # This step disables gci checks, to check for all other linter errors,
+      # on every file in the PR, without filtering.
+      - name: Run golangci-lint (without import checking)
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: latest
+          args: --timeout=15m --config=.golangci.yml --disable gci $(cat modified_files.txt)
+          only-new-issues: true

.github/workflows/run-tests.yml

@@ -1,4 +1,4 @@
-name: Run tests
+Name: Run tests
 
 on:
   push:
@@ -44,8 +44,11 @@ jobs:
       - name: Generate mocks
         run: make go_mockgen
 
-      - name: Run golangci-lint
-        run: make go_lint
+      - name: Fix imports
+        run: make gci
+
+      - name: Format the repo
+        run: make gofumpt
 
       - name: Test
         run: make go_test

.golangci.yml

@@ -1,17 +1,39 @@
-linters-settings:
-  govet:
-    check-shadowing: true
+run:
+  timeout: 15m
+  build-tags:
+    - e2e
+    - test
+    - integration
 
 # TODO_TECHDEBT: Enable each linter listed, 1 by 1, fixing issues as they appear.
 # Don't forget to delete the `disable-all: true` line as well.
 linters:
   disable-all: true
   enable:
-#    - govet
-#    - revive
-#    - errcheck
-#    - unused
-    - goimports
+    #    - govet
+    #    - revive
+    #    - errcheck
+    #    - unused
+    - gofumpt
+    - lll
+    - gci
+
+linters-settings:
+  govet:
+    check-shadowing: true
+  lll:
+    line-length: 120
+    tab-width: 4
+  gci:
+    sections:
+      - standard # std lib
+      - default # external
+      - prefix(github.com/pokt-network/poktroll) # local imports
+    skip-generated: true
+    custom-order: true
+  skip-files:
+    - "*.pb.go"
+    - "*/vendor/*"
 
 issues:
   exclude-use-default: true
@@ -27,30 +49,54 @@ issues:
     # empty import block containing a comment used by ignite CLI.
     - path: ^x/.+/genesis\.go$
       linters:
-        - goimports
+        - gci
     # Exclude cosmos-sdk module module.go files as they are generated with unused
     # parameters and unchecked errors.
     - path: ^x/.+/module\.go$
       linters:
+        - lll
+        - gci
         - revive
         - errcheck
     # Exclude cosmos-sdk module tx.go files as they are generated with unused
     # constants.
     - path: ^x/.+/cli/tx\.go$
       linters:
+        - gci
+        - lll
         - unused
     # Exclude simulation code as it's generated with lots of unused parameters.
     - path: .*/simulation/.*|_simulation\.go$
       linters:
+        - gci
+        - lll
         - revive
     # Exclude cosmos-sdk module codec files as they are scaffolded with a unused
-    # paramerters and a comment used by ignite CLI.
+    # parameters and a comment used by ignite CLI.
     - path: ^x/.+/codec.go$
       linters:
+        - lll
         - revive
     - path: _test\.go$
       linters:
         - errcheck
+    # Exclude line length check for errors.go files as they are easier to read
+    # as as single line.
+    - path: errors\.go$
+      linters:
+        - lll
+    # Exclude app module code as it's generated with lots with long lines and
+    # improperly formatted imports.
+    - path: ^app
+      linters:
+        - lll
+        - gci
+    # Exclude cmd directory code as it's generated with lots with long lines and
+    # improperly formatted imports.
+    - path: ^cmd/poc
+      linters:
+        - lll
+        - gci
   # TODO_IMPROVE: see https://golangci-lint.run/usage/configuration/#issues-configuration
-  #new: true,
+  new: true,
   #fix: true,

Makefile

@@ -14,7 +14,8 @@ POCKET_ADDR_PREFIX = pokt
 install_ci_deps: ## Installs `mockgen`
 	go install "github.com/golang/mock/[email protected]" && mockgen --version
 	go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest && golangci-lint --version
-	go install golang.org/x/tools/cmd/goimports@latest
+	go install github.com/daixiang0/gci@latest
+	go install mvdan.cc/gofumpt@latest
 
 ########################
 ### Makefile Helpers ###
@@ -71,6 +72,26 @@ check_godoc:
 	fi; \
 	}
 
+.PHONY: check_gci
+# Internal helper target - check if gci is installed
+check_gci:
+	{ \
+	if ( ! ( command -v gci >/dev/null )); then \
+		echo "Seems like you don't have gci installed. Make sure you install it via 'go install github.com/daixiang0/gci@latest' before continuing"; \
+		exit 1; \
+	fi; \
+	}
+
+.PHONY: check_gofumpt
+# Internal helper target - check if gofumpt is installed
+check_gofumpt:
+	{ \
+	if ( ! ( command -v gci >/dev/null )); then \
+		echo "Seems like you don't have gci installed. Make sure you install it via 'go install mvdan.cc/gofumpt@latest' before continuing"; \
+		exit 1; \
+	fi; \
+	}
+
 .PHONY: check_npm
 # Internal helper target - check if npm is installed
 check_npm:
@@ -150,10 +171,15 @@ localnet_regenesis: ## Regenerate the localnet genesis file
 
 .PHONY: go_lint
 go_lint: ## Run all go linters
-	golangci-lint run --timeout 5m --build-tags test
+	golangci-lint run
+
+.PHONY: gci
+gci: check_gci ## Run gci (import ordering) on all applicable files, this writes changes in place
+	go run ./tools/scripts/gci
 
-go_imports: check_go_version ## Run goimports on all go files
-	go run ./tools/scripts/goimports
+.PHONY: gofumpt
+gofumpt: check_gofumpt ## Run gofumpt (stricter gofmt) on all applicable files, this writes changes in place
+	go run ./tools/scripts/gofumpt
 
 #############
 ### Tests ###

app/simulation_test.go

@@ -60,7 +60,9 @@ func fauxMerkleModeOpt(bapp *baseapp.BaseApp) {
 // Running using starport command:
 // `starport chain simulate -v --numBlocks 200 --blockSize 50`
 // Running as go benchmark test:
-// `go test -benchmem -run=^$ -bench ^BenchmarkSimulation ./app -NumBlocks=200 -BlockSize 50 -Commit=true -Verbose=true -Enabled=true`
+//
+//	$ go test -benchmem -run=^$ -bench ^BenchmarkSimulation ./app -NumBlocks=200 \
+//		-BlockSize 50 -Commit=true -Verbose=true -Enabled=true
 func BenchmarkSimulation(b *testing.B) {
 	simcli.FlagSeedValue = time.Now().Unix()
 	simcli.FlagVerboseValue = true
@@ -209,7 +211,8 @@ func TestAppStateDeterminism(t *testing.T) {
 			if j != 0 {
 				require.Equal(
 					t, string(appHashList[0]), string(appHashList[j]),
-					"non-determinism in seed %d: %d/%d, attempt: %d/%d\n", config.Seed, i+1, numSeeds, j+1, numTimesToRunPerSeed,
+					"non-determinism in seed %d: %d/%d, attempt: %d/%d\n",
+					config.Seed, i+1, numSeeds, j+1, numTimesToRunPerSeed,
 				)
 			}
 		}
@@ -344,7 +347,8 @@ func TestAppImportExport(t *testing.T) {
 			bApp.GetKey(stakingtypes.StoreKey), newApp.GetKey(stakingtypes.StoreKey),
 			[][]byte{
 				stakingtypes.UnbondingQueueKey, stakingtypes.RedelegationQueueKey, stakingtypes.ValidatorQueueKey,
-				stakingtypes.HistoricalInfoKey, stakingtypes.UnbondingIDKey, stakingtypes.UnbondingIndexKey, stakingtypes.UnbondingTypeKey, stakingtypes.ValidatorUpdatesKey,
+				stakingtypes.HistoricalInfoKey, stakingtypes.UnbondingIDKey, stakingtypes.UnbondingIndexKey,
+				stakingtypes.UnbondingTypeKey, stakingtypes.ValidatorUpdatesKey,
 			},
 		}, // ordering may change but it doesn't matter
 		{bApp.GetKey(slashingtypes.StoreKey), newApp.GetKey(slashingtypes.StoreKey), [][]byte{}},
@@ -355,7 +359,9 @@ func TestAppImportExport(t *testing.T) {
 		{bApp.GetKey(govtypes.StoreKey), newApp.GetKey(govtypes.StoreKey), [][]byte{}},
 		{bApp.GetKey(evidencetypes.StoreKey), newApp.GetKey(evidencetypes.StoreKey), [][]byte{}},
 		{bApp.GetKey(capabilitytypes.StoreKey), newApp.GetKey(capabilitytypes.StoreKey), [][]byte{}},
-		{bApp.GetKey(authzkeeper.StoreKey), newApp.GetKey(authzkeeper.StoreKey), [][]byte{authzkeeper.GrantKey, authzkeeper.GrantQueuePrefix}},
+		{bApp.GetKey(authzkeeper.StoreKey), newApp.GetKey(authzkeeper.StoreKey), [][]byte{
+			authzkeeper.GrantKey, authzkeeper.GrantQueuePrefix,
+		}},
 	}
 
 	for _, skp := range storeKeysPrefixes {
@@ -366,7 +372,9 @@ func TestAppImportExport(t *testing.T) {
 		require.Equal(t, len(failedKVAs), len(failedKVBs), "unequal sets of key-values to compare")
 
 		fmt.Printf("compared %d different key/value pairs between %s and %s\n", len(failedKVAs), skp.A, skp.B)
-		require.Equal(t, 0, len(failedKVAs), simtestutil.GetSimulationLog(skp.A.Name(), bApp.SimulationManager().StoreDecoders, failedKVAs, failedKVBs))
+		require.Equal(t, 0, len(failedKVAs), simtestutil.GetSimulationLog(
+			skp.A.Name(), bApp.SimulationManager().StoreDecoders, failedKVAs, failedKVBs),
+		)
 	}
 }
 

cmd/pocketd/cmd/config.go

@@ -8,8 +8,9 @@ import (
 
 // InitSDKConfig initializes the SDK's config with the appropriate parameters
 // and prefixes so everything is named appropriately for Pocket Network.
-// TODO_DISCUSS: Exporting publically for testing purposes only.
-// Consider adding a helper per the discussion here: https://github.com/pokt-network/poktroll/pull/59#discussion_r1357816798
+// TODO_DISCUSS: Exporting publicly for testing purposes only.
+// Consider adding a helper per the discussion here:
+// https://github.com/pokt-network/poktroll/pull/59#discussion_r1357816798
 func InitSDKConfig() {
 	// Set prefixes
 	accountPubKeyPrefix := app.AccountAddressPrefix + "pub"

cmd/pocketd/cmd/genaccounts.go

@@ -181,11 +181,20 @@ contain valid denominations. Accounts may optionally be supplied with vesting pa
 		},
 	}
 
-	cmd.Flags().String(flags.FlagKeyringBackend, flags.DefaultKeyringBackend, "Select keyring's backend (os|file|kwallet|pass|test)")
-	cmd.Flags().String(flags.FlagHome, defaultNodeHome, "The application home directory")
-	cmd.Flags().String(flagVestingAmt, "", "amount of coins for vesting accounts")
-	cmd.Flags().Int64(flagVestingStart, 0, "schedule start time (unix epoch) for vesting accounts")
-	cmd.Flags().Int64(flagVestingEnd, 0, "schedule end time (unix epoch) for vesting accounts")
+	cmd.Flags().
+		String(
+			flags.FlagKeyringBackend,
+			flags.DefaultKeyringBackend,
+			"Select keyring's backend (os|file|kwallet|pass|test)",
+		)
+	cmd.Flags().
+		String(flags.FlagHome, defaultNodeHome, "The application home directory")
+	cmd.Flags().
+		String(flagVestingAmt, "", "amount of coins for vesting accounts")
+	cmd.Flags().
+		Int64(flagVestingStart, 0, "schedule start time (unix epoch) for vesting accounts")
+	cmd.Flags().
+		Int64(flagVestingEnd, 0, "schedule end time (unix epoch) for vesting accounts")
 	flags.AddQueryFlagsToCmd(cmd)
 
 	return cmd