From 8c1e571724a2157afb8457d2fc60819c5fe5547e Mon Sep 17 00:00:00 2001
From: tbjump <103955289+tbjump@users.noreply.github.com>
Date: Mon, 23 May 2022 06:05:55 -0700
Subject: [PATCH] fix getUniqueClientId to actually return unique ID (#1127)

fix getUniqueClientId to actually return unique ID
---
 node/pkg/reporter/attestation_events.go      |  8 +++--
 node/pkg/reporter/attestation_events_test.go | 36 ++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 node/pkg/reporter/attestation_events_test.go

diff --git a/node/pkg/reporter/attestation_events.go b/node/pkg/reporter/attestation_events.go
index 866db5647..4affb6c43 100644
--- a/node/pkg/reporter/attestation_events.go
+++ b/node/pkg/reporter/attestation_events.go
@@ -11,6 +11,8 @@ import (
 	"github.com/ethereum/go-ethereum/common"
 )
 
+const maxClientId = 1e6
+
 type (
 	// MessagePublication is a VAA along with a transaction identifer from the EmiterChain
 	MessagePublication struct {
@@ -47,10 +49,10 @@ func EventListener(logger *zap.Logger) *AttestationEventReporter {
 
 // getUniqueClientId loops to generate & test integers for existence as key of map. returns an int that is not a key in map.
 func (re *AttestationEventReporter) getUniqueClientId() int {
-	clientId := rand.Intn(1e6)
-	found := false
+	clientId := 0
+	found := true
 	for found {
-		clientId = rand.Intn(1e6)
+		clientId = rand.Intn(maxClientId) //#nosec G404 The clientIds don't need to be unpredictable. They just need to be unique.
 		_, found = re.subs[clientId]
 	}
 	return clientId
diff --git a/node/pkg/reporter/attestation_events_test.go b/node/pkg/reporter/attestation_events_test.go
new file mode 100644
index 000000000..afc9a70c6
--- /dev/null
+++ b/node/pkg/reporter/attestation_events_test.go
@@ -0,0 +1,36 @@
+package reporter
+
+import (
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetUniqueClientId(t *testing.T) {
+	/*
+		Rationale:
+		Pro: This test does not have false positives. It is guaranteed to fail if the magic value for the maximum client ID changes.
+		Con: It takes ca. 0.463s to run this test
+	*/
+
+	var almostFullMap = make(map[int]*lifecycleEventChannels, maxClientId)
+
+	firstExpectedValue := 0
+	secondExpectedValue := 1
+
+	// build a full map
+	for i := 0; i < maxClientId; i++ {
+		almostFullMap[i] = nil
+	}
+
+	// Test that we can find the empty slot in the map
+	delete(almostFullMap, firstExpectedValue)
+	re := AttestationEventReporter{sync.RWMutex{}, nil, almostFullMap}
+	assert.Equal(t, re.getUniqueClientId(), firstExpectedValue)
+
+	// Test that we can find a different empty slot in the map
+	almostFullMap[firstExpectedValue] = nil
+	delete(almostFullMap, secondExpectedValue)
+	assert.Equal(t, re.getUniqueClientId(), secondExpectedValue)
+}