all: address errcheck linter errors

Author: mfycheng

.github/workflows/test.yml

@@ -19,5 +19,8 @@ jobs:
       with:
         go-version: 1.21.x
 
+    - name: Lint
+      run: make lint
+
     - name: Test
       run: make test

.gitignore

@@ -22,7 +22,10 @@ go.work
 
 # Build output directories
 build/
+.build/
 
 # Visual Studio Code
 .vscode/
 
+# Jetbrains
+.idea/

.golangci-lint.yaml

@@ -0,0 +1,14 @@
+run:
+
+linters:
+  disable-all: true
+  enable:
+    - errcheck
+    - ineffassign
+
+issues:
+  exclude-rules:
+    - path: '(.+)_test\.go'
+      text: ".*GetPaymentHistory is deprecated: Payment history has migrated to chats"
+    - path: '(.+)testutil\.go'
+      text: ".*GetPaymentHistory is deprecated: Payment history has migrated to chats"

Makefile

@@ -1,6 +1,20 @@
-all: test
+.PHONY: all
+all: lint test
 
+.PHONY: test
 test:
 	@go test -cover ./...
 
-.PHONY: all test
+.PHONY: lint
+lint: tools.golangci-lint
+	@golangci-lint --timeout=3m --config .golangci-lint.yaml run ./...
+
+.PHONY: tools
+tools: tools.golangci-lint
+
+tools.golangci-lint: .build/markers/golangci-lint_installed
+.build/markers/golangci-lint_installed:
+	@command -v golangci-lint >/dev/null ; if [ $$? -ne 0 ]; then \
+		CGO_ENABLED=0 go install github.com/golangci/golangci-lint/cmd/[email protected]; \
+	fi
+	@mkdir -p $(shell dirname $@) && touch "$@"

pkg/cache/cache.go

@@ -1,7 +1,6 @@
 package cache
 
 import (
-	"errors"
 	"log"
 	"sync"
 )
@@ -11,7 +10,7 @@ type Cache interface {
 	SetVerbose(verbose bool)
 	GetWeight() int
 	GetBudget() int
-	Insert(key string, value interface{}, weight int) error
+	Insert(key string, value interface{}, weight int) (inserted bool)
 	Retrieve(key string) (interface{}, bool)
 	Clear()
 }
@@ -61,12 +60,12 @@ func (c *cache) GetBudget() int {
 }
 
 // Insert inserts an object into the cache
-func (c *cache) Insert(key string, value interface{}, weight int) error {
+func (c *cache) Insert(key string, value interface{}, weight int) (inserted bool) {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()
 
 	if _, found := c.lookup[key]; found {
-		return errors.New("key already exists in cache")
+		return false
 	}
 
 	node := &cacheNode{
@@ -106,7 +105,7 @@ func (c *cache) Insert(key string, value interface{}, weight int) error {
 		delete(c.lookup, c.tail.key)
 	}
 
-	return nil
+	return true
 }
 
 // Retrieve gets an object out of the cache

pkg/cache/cache_test.go

@@ -2,24 +2,18 @@ package cache
 
 import (
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestCacheInsert(t *testing.T) {
 	cache := NewCache(1)
-	insertError := cache.Insert("A", "", 1)
-
-	if insertError != nil {
-		t.Fatalf("Cache insert resulted in unexpected error: %s", insertError)
-	}
+	require.True(t, cache.Insert("A", "", 1))
 }
 
 func TestCacheInsertWithinBudget(t *testing.T) {
 	cache := NewCache(1)
-	insertError := cache.Insert("A", "", 2)
-
-	if insertError != nil {
-		t.Fatalf("Cache insert resulted in unexpected error: %s", insertError)
-	}
+	require.True(t, cache.Insert("A", "", 2))
 }
 
 func TestCacheInsertUpdatesWeight(t *testing.T) {
@@ -28,19 +22,13 @@ func TestCacheInsertUpdatesWeight(t *testing.T) {
 	_ = cache.Insert("B", "", 1)
 	_ = cache.Insert("budget_exceeded", "", 1)
 
-	if cache.GetWeight() != 2 {
-		t.Fatal("Cache with budget 2 did not correctly set weight after evicting one of three nodes")
-	}
+	require.Equal(t, 2, cache.GetWeight())
 }
 
 func TestCacheInsertDuplicateRejected(t *testing.T) {
 	cache := NewCache(2)
-	_ = cache.Insert("dupe", "", 1)
-	dupeError := cache.Insert("dupe", "", 1)
-
-	if dupeError == nil {
-		t.Fatal("Cache insert of duplicate key did not result in any err")
-	}
+	require.True(t, cache.Insert("dupe", "", 1))
+	require.False(t, cache.Insert("dupe", "", 1))
 }
 
 func TestCacheInsertEvictsLeastRecentlyUsed(t *testing.T) {
@@ -51,43 +39,34 @@ func TestCacheInsertEvictsLeastRecentlyUsed(t *testing.T) {
 	_ = cache.Insert("B", "", 1)
 
 	_, foundEvicted := cache.Retrieve("evicted")
-	if foundEvicted {
-		t.Fatal("Cache insert did not trigger eviction after weight exceedance")
-	}
+	require.False(t, foundEvicted)
 
 	// double check that only 1 one was evicted and not any extra
 	_, foundA := cache.Retrieve("A")
+	require.True(t, foundA)
 	_, foundB := cache.Retrieve("B")
-
-	if !foundA || !foundB {
-		t.Fatal("Cache insert evicted more than necessary")
-	}
+	require.True(t, foundB)
 }
 
 func TestCacheInsertEvictsLeastRecentlyRetrieved(t *testing.T) {
 	cache := NewCache(2)
 	_ = cache.Insert("A", "", 1)
 	_ = cache.Insert("evicted", "", 1)
 
-	// retrieve the oldest node, promoting it head so it is not evicted
+	// retrieve the oldest node, promoting it head, so it is not evicted
 	cache.Retrieve("A")
 
 	// insert once more, exceeding weight capacity
 	_ = cache.Insert("B", "", 1)
 	// now the least recently used key should be evicted
 	_, foundEvicted := cache.Retrieve("evicted")
-	if foundEvicted {
-		t.Fatal("Cache insert did not evict least recently used after weight exceedance")
-	}
+	require.False(t, foundEvicted)
 }
 
 func TestClear(t *testing.T) {
 	cache := NewCache(1)
 	_ = cache.Insert("cleared", "", 1)
 	cache.Clear()
 	_, found := cache.Retrieve("cleared")
-
-	if found {
-		t.Fatal("Still able to retrieve nodes after cache was cleared")
-	}
+	require.False(t, found)
 }

pkg/code/async/account/gift_card.go

@@ -198,12 +198,18 @@ func (p *service) initiateProcessToAutoReturnGiftCard(ctx context.Context, giftC
 	}
 
 	// Finally, update the user by best-effort sending them a push
-	go push.SendGiftCardReturnedPushNotification(
-		ctx,
-		p.data,
-		p.pusher,
-		giftCardVaultAccount,
-	)
+	go func() {
+		err := push.SendGiftCardReturnedPushNotification(
+			ctx,
+			p.data,
+			p.pusher,
+			giftCardVaultAccount,
+		)
+		if err != nil {
+			p.log.WithError(err).Warn("failed to send gift card return push notification (best effort)")
+		}
+	}()
+
 	return nil
 }
 

pkg/code/async/commitment/worker.go

@@ -4,18 +4,15 @@ import (
 	"context"
 	"database/sql"
 	"errors"
+	"fmt"
 	"math"
 	"sync"
 	"time"
 
 	"github.com/mr-tron/base58"
 	"github.com/newrelic/go-agent/v3/newrelic"
+	"github.com/sirupsen/logrus"
 
-	"github.com/code-payments/code-server/pkg/database/query"
-	"github.com/code-payments/code-server/pkg/metrics"
-	"github.com/code-payments/code-server/pkg/pointer"
-	"github.com/code-payments/code-server/pkg/retry"
-	"github.com/code-payments/code-server/pkg/solana"
 	"github.com/code-payments/code-server/pkg/code/common"
 	"github.com/code-payments/code-server/pkg/code/data/action"
 	"github.com/code-payments/code-server/pkg/code/data/commitment"
@@ -24,6 +21,11 @@ import (
 	"github.com/code-payments/code-server/pkg/code/data/nonce"
 	"github.com/code-payments/code-server/pkg/code/data/treasury"
 	"github.com/code-payments/code-server/pkg/code/transaction"
+	"github.com/code-payments/code-server/pkg/database/query"
+	"github.com/code-payments/code-server/pkg/metrics"
+	"github.com/code-payments/code-server/pkg/pointer"
+	"github.com/code-payments/code-server/pkg/retry"
+	"github.com/code-payments/code-server/pkg/solana"
 )
 
 //
@@ -424,16 +426,39 @@ func (p *service) injectCommitmentVaultManagementFulfillments(ctx context.Contex
 		if err != nil {
 			return err
 		}
+
+		// note: defer() will only run when the outer function returns, and therefore
+		// all of the defer()'s in this loop will be run all at once at the end, rather
+		// than at the end of each iteration.
+		//
+		// Since we are not committing (and therefore consuming) the nonce's until the
+		// end of the function, this is desirable. If we released at the end of each
+		// iteration, we could potentially acquire the same nonce multiple times for
+		// different transactions, which would fail.
 		defer func() {
-			selectedNonce.ReleaseIfNotReserved()
+			if err := selectedNonce.ReleaseIfNotReserved(); err != nil {
+				p.log.
+					WithFields(logrus.Fields{
+						"method":        "injectCommitmentVaultManagementFulfillments",
+						"nonce_account": selectedNonce.Account.PublicKey().ToBase58(),
+						"blockhash":     selectedNonce.Blockhash.ToBase58(),
+					}).
+					WithError(err).
+					Warn("failed to release nonce")
+			}
+
+			// This is idempotent regardless of whether the above
 			selectedNonce.Unlock()
 		}()
 
 		txn, err := transaction.MakeNoncedTransaction(selectedNonce.Account, selectedNonce.Blockhash, txnToMake.ixns...)
 		if err != nil {
 			return err
 		}
-		txn.Sign(common.GetSubsidizer().PrivateKey().ToBytes())
+
+		if err := txn.Sign(common.GetSubsidizer().PrivateKey().ToBytes()); err != nil {
+			return fmt.Errorf("failed to sign transaction: %w", err)
+		}
 
 		intentOrderingIndex := uint64(0)
 		fulfillmentOrderingIndex := uint32(i)