From 6f5d9dbcab0c518eab1d1b608d2a97e8c392bc43 Mon Sep 17 00:00:00 2001
From: Brendan Chou <3680392+BrendanChou@users.noreply.github.com>
Date: Thu, 12 Oct 2023 10:25:40 -0400
Subject: [PATCH] [DEC-2102] Audit lib/collections (#582)

* SliceToSet -> UniqueSliceToSet

* imports
---
 protocol/lib/collections.go                   |   7 +-
 protocol/lib/collections_test.go              | 136 +++++++++---------
 protocol/x/blocktime/keeper/keeper.go         |   2 +-
 protocol/x/bridge/keeper/keeper.go            |   2 +-
 protocol/x/clob/abci.go                       |   4 +-
 protocol/x/clob/keeper/keeper.go              |   2 +-
 .../x/clob/keeper/msg_server_cancel_orders.go |   2 +-
 .../x/clob/keeper/msg_server_place_order.go   |   4 +-
 .../keeper/untriggered_conditional_orders.go  |  11 +-
 protocol/x/delaymsg/keeper/keeper.go          |   2 +-
 protocol/x/feetiers/keeper/keeper.go          |   2 +-
 protocol/x/perpetuals/keeper/keeper.go        |   2 +-
 protocol/x/prices/keeper/keeper.go            |   2 +-
 protocol/x/rewards/keeper/keeper.go           |  11 +-
 protocol/x/sending/keeper/keeper.go           |   2 +-
 protocol/x/stats/keeper/keeper.go             |   2 +-
 protocol/x/vest/keeper/keeper.go              |   2 +-
 17 files changed, 92 insertions(+), 103 deletions(-)

diff --git a/protocol/lib/collections.go b/protocol/lib/collections.go
index 65ff422101..a00a0347df 100644
--- a/protocol/lib/collections.go
+++ b/protocol/lib/collections.go
@@ -32,14 +32,15 @@ func GetSortedKeys[R interface {
 	return keys
 }
 
-// SliceToSet converts a slice to a set. Function will panic if there are duplicate values.
-func SliceToSet[K comparable](values []K) map[K]struct{} {
+// UniqueSliceToSet converts a slice of unique values to a set.
+// The function will panic if there are duplicate values.
+func UniqueSliceToSet[K comparable](values []K) map[K]struct{} {
 	set := make(map[K]struct{}, len(values))
 	for _, sliceVal := range values {
 		if _, exists := set[sliceVal]; exists {
 			panic(
 				fmt.Sprintf(
-					"SliceToSet: duplicate value: %+v",
+					"UniqueSliceToSet: duplicate value: %+v",
 					sliceVal,
 				),
 			)
diff --git a/protocol/lib/collections_test.go b/protocol/lib/collections_test.go
index 6a22c642ab..14878e8365 100644
--- a/protocol/lib/collections_test.go
+++ b/protocol/lib/collections_test.go
@@ -5,30 +5,37 @@ import (
 	"testing"
 
 	"github.com/dydxprotocol/v4-chain/protocol/lib"
-	"github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
 
 	"github.com/stretchr/testify/require"
 )
 
 func TestContainsDuplicates(t *testing.T) {
-	// Empty case.
-	require.False(t, lib.ContainsDuplicates([]types.OrderId{}))
-
-	// Unique uint32 case.
-	allUniqueUint32s := []uint32{1, 2, 3, 4}
-	require.False(t, lib.ContainsDuplicates(allUniqueUint32s))
-
-	// Duplicate uint32 case.
-	containsDuplicateUint32 := append(allUniqueUint32s, 3)
-	require.True(t, lib.ContainsDuplicates(containsDuplicateUint32))
-
-	// Unique string case.
-	allUniqueStrings := []string{"hello", "world", "h", "w"}
-	require.False(t, lib.ContainsDuplicates(allUniqueStrings))
-
-	// Duplicate string case.
-	containsDuplicateString := append(allUniqueStrings, "world")
-	require.True(t, lib.ContainsDuplicates(containsDuplicateString))
+	tests := map[string]struct {
+		input    []uint32
+		expected bool
+	}{
+		"Nil": {
+			input:    nil,
+			expected: false,
+		},
+		"Empty": {
+			input:    []uint32{},
+			expected: false,
+		},
+		"True": {
+			input:    []uint32{1, 2, 3, 4},
+			expected: false,
+		},
+		"False": {
+			input:    []uint32{1, 2, 3, 4, 3},
+			expected: true,
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			require.Equal(t, tc.expected, lib.ContainsDuplicates(tc.input))
+		})
+	}
 }
 
 func TestGetSortedKeys(t *testing.T) {
@@ -59,6 +66,44 @@ func TestGetSortedKeys(t *testing.T) {
 	}
 }
 
+func TestUniqueSliceToSet(t *testing.T) {
+	tests := map[string]struct {
+		input     []string
+		expected  map[string]struct{}
+		panicWith string
+	}{
+		"Empty": {
+			input:    []string{},
+			expected: map[string]struct{}{},
+		},
+		"Basic": {
+			input: []string{"0", "1", "2"},
+			expected: map[string]struct{}{
+				"0": {},
+				"1": {},
+				"2": {},
+			},
+		},
+		"Duplicate": {
+			input:     []string{"one", "2", "two", "one", "4"},
+			panicWith: "UniqueSliceToSet: duplicate value: one",
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			if tc.panicWith != "" {
+				require.PanicsWithValue(
+					t,
+					tc.panicWith,
+					func() { lib.UniqueSliceToSet[string](tc.input) },
+				)
+			} else {
+				require.Equal(t, tc.expected, lib.UniqueSliceToSet[string](tc.input))
+			}
+		})
+	}
+}
+
 func TestMapSlice(t *testing.T) {
 	// Can increment all numbers in a slice by 1, and change type to `uint64`.
 	require.Equal(
@@ -171,59 +216,6 @@ func TestFilterSlice(t *testing.T) {
 	)
 }
 
-func TestSliceToSet(t *testing.T) {
-	slice := make([]int, 0)
-	for i := 0; i < 3; i++ {
-		slice = append(slice, i)
-	}
-	set := lib.SliceToSet(slice)
-	require.Equal(
-		t,
-		map[int]struct{}{
-			0: {},
-			1: {},
-			2: {},
-		},
-		set,
-	)
-	stringSlice := []string{
-		"one",
-		"two",
-	}
-	stringSet := lib.SliceToSet(stringSlice)
-	require.Equal(
-		t,
-		map[string]struct{}{
-			"one": {},
-			"two": {},
-		},
-		stringSet,
-	)
-
-	emptySlice := []types.OrderId{}
-	emptySet := lib.SliceToSet(emptySlice)
-	require.Equal(
-		t,
-		map[types.OrderId]struct{}{},
-		emptySet,
-	)
-}
-
-func TestSliceToSet_PanicOnDuplicate(t *testing.T) {
-	stringSlice := []string{
-		"one",
-		"two",
-		"one",
-	}
-	require.PanicsWithValue(
-		t,
-		"SliceToSet: duplicate value: one",
-		func() {
-			lib.SliceToSet(stringSlice)
-		},
-	)
-}
-
 func TestMergeAllMapsWithDistinctKeys(t *testing.T) {
 	tests := map[string]struct {
 		inputMaps []map[string]string
diff --git a/protocol/x/blocktime/keeper/keeper.go b/protocol/x/blocktime/keeper/keeper.go
index 5cc54a3f63..afa2e231a2 100644
--- a/protocol/x/blocktime/keeper/keeper.go
+++ b/protocol/x/blocktime/keeper/keeper.go
@@ -37,7 +37,7 @@ func NewKeeper(
 	return &Keeper{
 		cdc:         cdc,
 		storeKey:    storeKey,
-		authorities: lib.SliceToSet(authorities),
+		authorities: lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/bridge/keeper/keeper.go b/protocol/x/bridge/keeper/keeper.go
index 95f5bed49f..3b1dd12d77 100644
--- a/protocol/x/bridge/keeper/keeper.go
+++ b/protocol/x/bridge/keeper/keeper.go
@@ -43,7 +43,7 @@ func NewKeeper(
 		bridgeEventManager: bridgeEventManager,
 		bankKeeper:         bankKeeper,
 		delayMsgKeeper:     delayMsgKeeper,
-		authorities:        lib.SliceToSet(authorities),
+		authorities:        lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/clob/abci.go b/protocol/x/clob/abci.go
index afca2ad63b..a7dcef5227 100644
--- a/protocol/x/clob/abci.go
+++ b/protocol/x/clob/abci.go
@@ -86,8 +86,8 @@ func EndBlocker(
 	keeper.AddUntriggeredConditionalOrders(
 		ctx,
 		processProposerMatchesEvents.PlacedConditionalOrderIds,
-		lib.SliceToSet(processProposerMatchesEvents.GetPlacedStatefulCancellationOrderIds()),
-		lib.SliceToSet(expiredStatefulOrderIds),
+		lib.UniqueSliceToSet(processProposerMatchesEvents.GetPlacedStatefulCancellationOrderIds()),
+		lib.UniqueSliceToSet(expiredStatefulOrderIds),
 	)
 
 	// Poll out all triggered conditional orders from `UntriggeredConditionalOrders` and update state.
diff --git a/protocol/x/clob/keeper/keeper.go b/protocol/x/clob/keeper/keeper.go
index e74f298103..d127c39fea 100644
--- a/protocol/x/clob/keeper/keeper.go
+++ b/protocol/x/clob/keeper/keeper.go
@@ -87,7 +87,7 @@ func NewKeeper(
 		storeKey:                     storeKey,
 		memKey:                       memKey,
 		transientStoreKey:            liquidationsStoreKey,
-		authorities:                  lib.SliceToSet(authorities),
+		authorities:                  lib.UniqueSliceToSet(authorities),
 		MemClob:                      memClob,
 		UntriggeredConditionalOrders: make(map[types.ClobPairId]*UntriggeredConditionalOrders),
 		PerpetualIdToClobPairId:      make(map[uint32][]types.ClobPairId),
diff --git a/protocol/x/clob/keeper/msg_server_cancel_orders.go b/protocol/x/clob/keeper/msg_server_cancel_orders.go
index a7bb9115ba..949750e5b1 100644
--- a/protocol/x/clob/keeper/msg_server_cancel_orders.go
+++ b/protocol/x/clob/keeper/msg_server_cancel_orders.go
@@ -39,7 +39,7 @@ func (k msgServer) CancelOrder(
 			// TODO(CLOB-778): Prevent invalid MsgCancelOrder messages from being included in the block.
 			if errors.Is(err, types.ErrStatefulOrderDoesNotExist) {
 				processProposerMatchesEvents := k.Keeper.GetProcessProposerMatchesEvents(ctx)
-				removedOrderIds := lib.SliceToSet(processProposerMatchesEvents.RemovedStatefulOrderIds)
+				removedOrderIds := lib.UniqueSliceToSet(processProposerMatchesEvents.RemovedStatefulOrderIds)
 				if _, found := removedOrderIds[msg.GetOrderId()]; found {
 					telemetry.IncrCounterWithLabels(
 						[]string{
diff --git a/protocol/x/clob/keeper/msg_server_place_order.go b/protocol/x/clob/keeper/msg_server_place_order.go
index 479f01f5f3..d6cded8e2f 100644
--- a/protocol/x/clob/keeper/msg_server_place_order.go
+++ b/protocol/x/clob/keeper/msg_server_place_order.go
@@ -42,7 +42,7 @@ func (k msgServer) PlaceOrder(goCtx context.Context, msg *types.MsgPlaceOrder) (
 
 	// 2. Return an error if an associated cancellation or removal already exists in the current block.
 	processProposerMatchesEvents := k.Keeper.GetProcessProposerMatchesEvents(ctx)
-	cancelledOrderIds := lib.SliceToSet(processProposerMatchesEvents.PlacedStatefulCancellationOrderIds)
+	cancelledOrderIds := lib.UniqueSliceToSet(processProposerMatchesEvents.PlacedStatefulCancellationOrderIds)
 	if _, found := cancelledOrderIds[order.GetOrderId()]; found {
 		return nil, errorsmod.Wrapf(
 			types.ErrStatefulOrderPreviouslyCancelled,
@@ -50,7 +50,7 @@ func (k msgServer) PlaceOrder(goCtx context.Context, msg *types.MsgPlaceOrder) (
 			order,
 		)
 	}
-	removedOrderIds := lib.SliceToSet(processProposerMatchesEvents.RemovedStatefulOrderIds)
+	removedOrderIds := lib.UniqueSliceToSet(processProposerMatchesEvents.RemovedStatefulOrderIds)
 	if _, found := removedOrderIds[order.GetOrderId()]; found {
 		return nil, errorsmod.Wrapf(
 			types.ErrStatefulOrderPreviouslyRemoved,
diff --git a/protocol/x/clob/keeper/untriggered_conditional_orders.go b/protocol/x/clob/keeper/untriggered_conditional_orders.go
index f4d6094699..5f1cad046e 100644
--- a/protocol/x/clob/keeper/untriggered_conditional_orders.go
+++ b/protocol/x/clob/keeper/untriggered_conditional_orders.go
@@ -2,15 +2,14 @@ package keeper
 
 import (
 	"fmt"
-	satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
 	"math/big"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/dydxprotocol/v4-chain/protocol/lib"
-	"github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
-
 	indexerevents "github.com/dydxprotocol/v4-chain/protocol/indexer/events"
 	"github.com/dydxprotocol/v4-chain/protocol/indexer/indexer_manager"
+	"github.com/dydxprotocol/v4-chain/protocol/lib"
+	"github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
+	satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types"
 )
 
 // UntriggeredConditionalOrders is an in-memory struct stored on the clob Keeper.
@@ -130,7 +129,7 @@ func (k Keeper) PruneUntriggeredConditionalOrders(
 	cancelledStatefulOrderIds []types.OrderId,
 ) {
 	// Merge lists of order ids.
-	orderIdsToPrune := lib.SliceToSet(expiredStatefulOrderIds)
+	orderIdsToPrune := lib.UniqueSliceToSet(expiredStatefulOrderIds)
 	for _, orderId := range cancelledStatefulOrderIds {
 		if _, exists := orderIdsToPrune[orderId]; exists {
 			panic(
@@ -198,7 +197,7 @@ func (untriggeredOrders *UntriggeredConditionalOrders) RemoveUntriggeredConditio
 		}
 	}
 
-	orderIdsToRemoveSet := lib.SliceToSet(orderIdsToRemove)
+	orderIdsToRemoveSet := lib.UniqueSliceToSet(orderIdsToRemove)
 
 	newOrdersToTriggerWhenOraclePriceLTETriggerPrice := make([]types.Order, 0)
 	for _, order := range untriggeredOrders.OrdersToTriggerWhenOraclePriceLTETriggerPrice {
diff --git a/protocol/x/delaymsg/keeper/keeper.go b/protocol/x/delaymsg/keeper/keeper.go
index 44213a0321..9680ee7458 100644
--- a/protocol/x/delaymsg/keeper/keeper.go
+++ b/protocol/x/delaymsg/keeper/keeper.go
@@ -33,7 +33,7 @@ func NewKeeper(
 	return &Keeper{
 		cdc:         cdc,
 		storeKey:    storeKey,
-		authorities: lib.SliceToSet(authorities),
+		authorities: lib.UniqueSliceToSet(authorities),
 		router:      router,
 	}
 }
diff --git a/protocol/x/feetiers/keeper/keeper.go b/protocol/x/feetiers/keeper/keeper.go
index e05069cc96..0ab8a43fb1 100644
--- a/protocol/x/feetiers/keeper/keeper.go
+++ b/protocol/x/feetiers/keeper/keeper.go
@@ -34,7 +34,7 @@ func NewKeeper(
 		cdc:         cdc,
 		statsKeeper: statsKeeper,
 		storeKey:    storeKey,
-		authorities: lib.SliceToSet(authorities),
+		authorities: lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/perpetuals/keeper/keeper.go b/protocol/x/perpetuals/keeper/keeper.go
index 3f730baaab..ffb48554ac 100644
--- a/protocol/x/perpetuals/keeper/keeper.go
+++ b/protocol/x/perpetuals/keeper/keeper.go
@@ -40,7 +40,7 @@ func NewKeeper(
 		pricesKeeper:        pricesKeeper,
 		epochsKeeper:        epochsKeeper,
 		indexerEventManager: indexerEventsManager,
-		authorities:         lib.SliceToSet(authorities),
+		authorities:         lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/prices/keeper/keeper.go b/protocol/x/prices/keeper/keeper.go
index 054ab40edf..12c22e4a32 100644
--- a/protocol/x/prices/keeper/keeper.go
+++ b/protocol/x/prices/keeper/keeper.go
@@ -49,7 +49,7 @@ func NewKeeper(
 		timeProvider:           timeProvider,
 		indexerEventManager:    indexerEventManager,
 		marketToCreatedAt:      map[uint32]time.Time{},
-		authorities:            lib.SliceToSet(authorities),
+		authorities:            lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/rewards/keeper/keeper.go b/protocol/x/rewards/keeper/keeper.go
index 499a9de7f4..64b6eaf9c1 100644
--- a/protocol/x/rewards/keeper/keeper.go
+++ b/protocol/x/rewards/keeper/keeper.go
@@ -1,16 +1,13 @@
 package keeper
 
 import (
-	errorsmod "cosmossdk.io/errors"
 	"fmt"
 	"math/big"
 	"time"
 
-	"github.com/dydxprotocol/v4-chain/protocol/daemons/pricefeed/client/constants"
-
-	sdkmath "cosmossdk.io/math"
-
+	errorsmod "cosmossdk.io/errors"
 	sdklog "cosmossdk.io/log"
+	sdkmath "cosmossdk.io/math"
 	"github.com/cometbft/cometbft/libs/log"
 	"github.com/cosmos/cosmos-sdk/codec"
 	"github.com/cosmos/cosmos-sdk/store/prefix"
@@ -18,7 +15,7 @@ import (
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
-
+	"github.com/dydxprotocol/v4-chain/protocol/daemons/pricefeed/client/constants"
 	"github.com/dydxprotocol/v4-chain/protocol/dtypes"
 	"github.com/dydxprotocol/v4-chain/protocol/lib"
 	"github.com/dydxprotocol/v4-chain/protocol/lib/metrics"
@@ -65,7 +62,7 @@ func NewKeeper(
 		bankKeeper:        bankKeeper,
 		feeTiersKeeper:    feeTiersKeeper,
 		pricesKeeper:      pricesKeeper,
-		authorities:       lib.SliceToSet(authorities),
+		authorities:       lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/sending/keeper/keeper.go b/protocol/x/sending/keeper/keeper.go
index c4b7e80174..82cff17331 100644
--- a/protocol/x/sending/keeper/keeper.go
+++ b/protocol/x/sending/keeper/keeper.go
@@ -42,7 +42,7 @@ func NewKeeper(
 		bankKeeper:          bankKeeper,
 		subaccountsKeeper:   subaccountsKeeper,
 		indexerEventManager: indexerEventManager,
-		authorities:         lib.SliceToSet(authorities),
+		authorities:         lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/stats/keeper/keeper.go b/protocol/x/stats/keeper/keeper.go
index 392e0f4eae..93aea2384a 100644
--- a/protocol/x/stats/keeper/keeper.go
+++ b/protocol/x/stats/keeper/keeper.go
@@ -39,7 +39,7 @@ func NewKeeper(
 		epochsKeeper:      epochsKeeper,
 		storeKey:          storeKey,
 		transientStoreKey: transientStoreKey,
-		authorities:       lib.SliceToSet(authorities),
+		authorities:       lib.UniqueSliceToSet(authorities),
 	}
 }
 
diff --git a/protocol/x/vest/keeper/keeper.go b/protocol/x/vest/keeper/keeper.go
index b1938f684c..aa50fe5466 100644
--- a/protocol/x/vest/keeper/keeper.go
+++ b/protocol/x/vest/keeper/keeper.go
@@ -47,7 +47,7 @@ func NewKeeper(
 		storeKey:        storeKey,
 		bankKeeper:      bankKeeper,
 		blockTimeKeeper: blockTimeKeeper,
-		authorities:     lib.SliceToSet(authorities),
+		authorities:     lib.UniqueSliceToSet(authorities),
 	}
 }