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

.github/workflows/run-tests.yml

@@ -1,4 +1,4 @@
-name: Run tests
+Name: Run tests
 
 on:
   push:
@@ -13,6 +13,11 @@ env:
   GKE_CLUSTER: protocol-us-central1
   GKE_ZONE: us-central1
 
+permissions:
+  contents: read
+  pull-requests: read
+  checks: write
+
 jobs:
   go-test:
     runs-on: ubuntu-latest
@@ -44,8 +49,44 @@ 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
+
+      # 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
+          skip-cache: 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
+          skip-cache: true
 
       - 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 && gci --version
+	go install mvdan.cc/gofumpt@latest && gofumpt --version
 
 ########################
 ### 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 gofumpt 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 --timeout=15m --build-tags=e2e,test,integration
+
+.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

docs/static/openapi.yml

@@ -79107,8 +79107,6 @@ definitions:
         description: params holds all the parameters of this module.
         type: object
     description: QueryParamsResponse is response type for the Query/Params RPC method.
-  poktroll.service.MsgAddServiceResponse:
-    type: object
   poktroll.tokenomics.Params:
     type: object
     properties:

e2e/tests/init_test.go

@@ -16,14 +16,15 @@ import (
 
 	tmcli "github.com/cometbft/cometbft/libs/cli"
 	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/regen-network/gocuke"
+	"github.com/stretchr/testify/require"
+
 	"github.com/pokt-network/poktroll/app"
 	"github.com/pokt-network/poktroll/testutil/testclient"
 	apptypes "github.com/pokt-network/poktroll/x/application/types"
 	sessiontypes "github.com/pokt-network/poktroll/x/session/types"
 	sharedtypes "github.com/pokt-network/poktroll/x/shared/types"
 	suppliertypes "github.com/pokt-network/poktroll/x/supplier/types"
-	"github.com/regen-network/gocuke"
-	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -37,14 +38,20 @@ var (
 
 	flagFeaturesPath string
 	keyRingFlag      = "--keyring-backend=test"
-	appGateServerUrl = "http://localhost:42069" // Keeping localhost by default because that is how we run the tests on our machines locally
+	// Keeping localhost by default because that is how we run the tests on our machines locally
+	appGateServerUrl = "http://localhost:42069"
 )
 
 func init() {
 	addrRe = regexp.MustCompile(`address:\s+(\S+)\s+name:\s+(\S+)`)
 	amountRe = regexp.MustCompile(`amount:\s+"(.+?)"\s+denom:\s+upokt`)
 
-	flag.StringVar(&flagFeaturesPath, "features-path", "*.feature", "Specifies glob paths for the runner to look up .feature files")
+	flag.StringVar(
+		&flagFeaturesPath,
+		"features-path",
+		"*.feature",
+		"Specifies glob paths for the runner to look up .feature files",
+	)
 
 	// If "APPGATE_SERVER_URL" envar is present, use it for appGateServerUrl
 	if url := os.Getenv("APPGATE_SERVER_URL"); url != "" {
@@ -255,7 +262,7 @@ func (s *suite) TheSupplierIsStakedForService(supplierName string, serviceId str
 	s.Fatalf("supplier %s is not staked for service %s", supplierName, serviceId)
 }
 
-func (s *suite) TheSessionForApplicationAndServiceContainsTheSupplier(appName string, serviceId string, supplierName string) {
+func (s *suite) TheSessionForApplicationAndServiceContainsTheSupplier(appName, serviceId, supplierName string) {
 	app, found := accNameToAppMap[appName]
 	if !found {
 		s.Fatalf("application %s not found", appName)
@@ -287,10 +294,15 @@ func (s *suite) TheSessionForApplicationAndServiceContainsTheSupplier(appName st
 	s.Fatalf("session for app %s and service %s does not contain supplier %s", appName, serviceId, supplierName)
 }
 
-func (s *suite) TheApplicationSendsTheSupplierARequestForServiceWithData(appName, supplierName, serviceId, requestData string) {
+func (s *suite) TheApplicationSendsTheSupplierARequestForServiceWithData(
+	appName, supplierName, serviceId, requestData string,
+) {
 	res, err := s.pocketd.RunCurl(appGateServerUrl, serviceId, requestData)
 	if err != nil {
-		s.Fatalf("error sending relay request from app %s to supplier %s for service %s: %v", appName, supplierName, serviceId, err)
+		s.Fatalf(
+			"error sending relay request from app %s to supplier %s for service %s: %v",
+			appName, supplierName, serviceId, err,
+		)
 	}
 
 	relayKey := relayReferenceKey(appName, supplierName)