Add Linter (golang-ci) and address fixes

.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,18 @@
+linters:
+  enable:
+    - bodyclose
+    - whitespace
+    - revive
+
+issues:
+  exclude-rules:
+    # var-naming conflicts are huge
+    - path: '(.+)\.go'
+      text: "var-naming"
+    # unused-parameter conflicts are sizeable
+    - path: '(.+)\.go'
+      text: "unused-parameter"
+    - 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,5 +1,5 @@
 .PHONY: all
-all: test
+all: lint test
 
 .PHONY: test
 test:
@@ -17,3 +17,17 @@ clean-integration-containers:
 	@docker ps | grep -E "etcd-test-[0-9a-z]{8}-[0-9]+" | awk '{print $$1}' | xargs docker rm -f 2>/dev/null || true
 	@echo Removing etcd cluster networks...
 	@docker network ls | grep -E "etcd-test-[0-9a-z]{8}-network" | awk '{print $$1}' | xargs docker network remove 2>/dev/null || true
+
+.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/antispam/airdrop.go

@@ -220,7 +220,6 @@ func (g *Guard) AllowReferralBonus(
 					recordDenialEvent(ctx, actionReferralBonus, "region restricted")
 					return false, nil
 				}
-
 			}
 
 			// Deny from mobile networks where we're currently under attack

pkg/code/antispam/guard_test.go

@@ -214,7 +214,7 @@ func TestAllowSendPayment_StaffUser(t *testing.T) {
 			ownerAccount2 := testutil.NewRandomAccount(t)
 
 			require.NoError(t, env.data.PutUser(env.ctx, &identity.Record{
-				ID: user.NewUserID(),
+				ID: user.NewID(),
 				View: &user.View{
 					PhoneNumber: &phoneNumber,
 				},
@@ -366,7 +366,7 @@ func TestAllowReceivePayments_StaffUser(t *testing.T) {
 			ownerAccount2 := testutil.NewRandomAccount(t)
 
 			require.NoError(t, env.data.PutUser(env.ctx, &identity.Record{
-				ID: user.NewUserID(),
+				ID: user.NewID(),
 				View: &user.View{
 					PhoneNumber: &phoneNumber,
 				},
@@ -375,7 +375,6 @@ func TestAllowReceivePayments_StaffUser(t *testing.T) {
 			}))
 
 			for _, ownerAccount := range []*common.Account{ownerAccount1, ownerAccount2} {
-
 				verification := &phone.Verification{
 					PhoneNumber:    phoneNumber,
 					OwnerAccount:   ownerAccount.PublicKey().ToBase58(),
@@ -499,7 +498,7 @@ func TestAllowOpenAccounts_StaffUser(t *testing.T) {
 		ownerAccount2 := testutil.NewRandomAccount(t)
 
 		require.NoError(t, env.data.PutUser(env.ctx, &identity.Record{
-			ID: user.NewUserID(),
+			ID: user.NewID(),
 			View: &user.View{
 				PhoneNumber: &phoneNumber,
 			},
@@ -508,7 +507,6 @@ func TestAllowOpenAccounts_StaffUser(t *testing.T) {
 		}))
 
 		for _, ownerAccount := range []*common.Account{ownerAccount1, ownerAccount2} {
-
 			verification := &phone.Verification{
 				PhoneNumber:    phoneNumber,
 				OwnerAccount:   ownerAccount.PublicKey().ToBase58(),
@@ -599,7 +597,7 @@ func TestAllowEstablishNewRelationship_StaffUser(t *testing.T) {
 		ownerAccount2 := testutil.NewRandomAccount(t)
 
 		require.NoError(t, env.data.PutUser(env.ctx, &identity.Record{
-			ID: user.NewUserID(),
+			ID: user.NewID(),
 			View: &user.View{
 				PhoneNumber: &phoneNumber,
 			},
@@ -692,7 +690,7 @@ func TestAllowNewPhoneVerification_StaffUser(t *testing.T) {
 	phoneNumber := "+12223334444"
 
 	require.NoError(t, env.data.PutUser(env.ctx, &identity.Record{
-		ID: user.NewUserID(),
+		ID: user.NewID(),
 		View: &user.View{
 			PhoneNumber: &phoneNumber,
 		},

pkg/code/async/account/config.go

@@ -3,7 +3,7 @@ package async_account
 // todo: setup configs
 
 const (
-	envConfigPrefix = "ACCOUNT_SERVICE_"
+	envConfigPrefix = "ACCOUNT_SERVICE_" //nolint:unused
 )
 
 type conf struct {

pkg/code/async/account/gift_card.go

@@ -41,7 +41,7 @@ func (p *service) giftCardAutoReturnWorker(serviceCtx context.Context, interval
 		func() (err error) {
 			time.Sleep(delay)
 
-			nr := serviceCtx.Value(metrics.NewRelicContextKey).(*newrelic.Application)
+			nr := serviceCtx.Value(metrics.NewRelicContextKey{}).(*newrelic.Application)
 			m := nr.StartTransaction("async__account_service__handle_gift_card_auto_return")
 			defer m.End()
 			tracedCtx := newrelic.NewContext(serviceCtx, m)
@@ -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/account/service.go

@@ -42,8 +42,6 @@ func (p *service) Start(ctx context.Context, interval time.Duration) error {
 		}
 	}()
 
-	select {
-	case <-ctx.Done():
-		return ctx.Err()
-	}
+	<-ctx.Done()
+	return ctx.Err()
 }

pkg/code/async/commitment/service.go

@@ -24,7 +24,6 @@ func New(data code_data.Provider) async.Service {
 }
 
 func (p *service) Start(ctx context.Context, interval time.Duration) error {
-
 	// Setup workers to watch for commitment state changes on the Solana side
 	for _, item := range []commitment.State{
 		commitment.StateReadyToOpen,
@@ -38,7 +37,6 @@ func (p *service) Start(ctx context.Context, interval time.Duration) error {
 			if err != nil && err != context.Canceled {
 				p.log.WithError(err).Warnf("commitment processing loop terminated unexpectedly for state %d", state)
 			}
-
 		}(item)
 	}
 
@@ -49,8 +47,6 @@ func (p *service) Start(ctx context.Context, interval time.Duration) error {
 		}
 	}()
 
-	select {
-	case <-ctx.Done():
-		return ctx.Err()
-	}
+	<-ctx.Done()
+	return ctx.Err()
 }