From 35023acd3efbd7e52fadba34cfd1ff22ce4d677b Mon Sep 17 00:00:00 2001
From: Tsachi Herman <24438559+tsachiherman@users.noreply.github.com>
Date: Fri, 23 Sep 2022 14:19:09 -0400
Subject: [PATCH] clients/horizonclient: fix theoretical bug in
 currentServerTime (#4596)

address the case where the host name provided to currentServerTime isn't contained in the global ServerTimeMap.
---
 clients/horizonclient/internal.go      |  4 +--
 clients/horizonclient/internal_test.go | 44 ++++++++++++++++++++++++++
 clients/horizonclient/main_test.go     |  8 ++++-
 3 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 clients/horizonclient/internal_test.go

diff --git a/clients/horizonclient/internal.go b/clients/horizonclient/internal.go
index 2133125550..123788caa9 100644
--- a/clients/horizonclient/internal.go
+++ b/clients/horizonclient/internal.go
@@ -140,9 +140,9 @@ func setCurrentServerTime(host string, serverDate []string, clock *clock.Clock)
 // currentServerTime returns the current server time for a given horizon server
 func currentServerTime(host string, currentTimeUTC int64) int64 {
 	serverTimeMapMutex.Lock()
-	st := ServerTimeMap[host]
+	st, has := ServerTimeMap[host]
 	serverTimeMapMutex.Unlock()
-	if &st == nil {
+	if !has {
 		return 0
 	}
 
diff --git a/clients/horizonclient/internal_test.go b/clients/horizonclient/internal_test.go
new file mode 100644
index 0000000000..80391c1d0c
--- /dev/null
+++ b/clients/horizonclient/internal_test.go
@@ -0,0 +1,44 @@
+package horizonclient
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestCurrentServerTime(t *testing.T) {
+	t.Run("non-existing-host-name", func(t *testing.T) {
+		currentTime := currentServerTime("non-existing-host-name", 60)
+		require.Zerof(t, currentTime, "server time for non-existing time is expected to be zero, but was %d instead", currentTime)
+	})
+
+	t.Run("server-behind", func(t *testing.T) {
+		serverTimeMapMutex.Lock()
+		ServerTimeMap["TestCurrentServerTime-server-behind"] = ServerTimeRecord{ServerTime: 27, LocalTimeRecorded: 23}
+		serverTimeMapMutex.Unlock()
+
+		currentTime := currentServerTime("TestCurrentServerTime-server-behind", 500)
+		defer func() {
+			serverTimeMapMutex.Lock()
+			delete(ServerTimeMap, "TestCurrentServerTime-server-behind")
+			serverTimeMapMutex.Unlock()
+		}()
+
+		require.Zerof(t, currentTime, "server time is too old and the method should have returned 0; instead, %d was returned", currentTime)
+	})
+
+	t.Run("normal", func(t *testing.T) {
+		serverTimeMapMutex.Lock()
+		ServerTimeMap["TestCurrentServerTime-server"] = ServerTimeRecord{ServerTime: 27, LocalTimeRecorded: 23}
+		serverTimeMapMutex.Unlock()
+
+		defer func() {
+			serverTimeMapMutex.Lock()
+			delete(ServerTimeMap, "TestCurrentServerTime-server")
+			serverTimeMapMutex.Unlock()
+		}()
+
+		currentTime := currentServerTime("TestCurrentServerTime-server", 37)
+		require.Equalf(t, currentTime, int64(41), "currentServerTime should have returned %d, but returned %d instead", 41, currentTime)
+	})
+}
diff --git a/clients/horizonclient/main_test.go b/clients/horizonclient/main_test.go
index d42fab46e5..2149bbbf29 100644
--- a/clients/horizonclient/main_test.go
+++ b/clients/horizonclient/main_test.go
@@ -58,7 +58,7 @@ func TestCheckMemoRequired(t *testing.T) {
 		Asset:       txnbuild.NativeAsset{},
 	}
 
-	asset := txnbuild.CreditAsset{"ABCD", kp.Address()}
+	asset := txnbuild.CreditAsset{Code: "ABCD", Issuer: kp.Address()}
 	pathPaymentStrictSend := txnbuild.PathPaymentStrictSend{
 		SendAsset:   asset,
 		SendAmount:  "10",
@@ -1455,7 +1455,9 @@ func TestFetchTimebounds(t *testing.T) {
 	// When no saved server time, return local time
 	st, err := client.FetchTimebounds(100)
 	if assert.NoError(t, err) {
+		serverTimeMapMutex.Lock()
 		assert.IsType(t, ServerTimeMap["localhost"], ServerTimeRecord{})
+		serverTimeMapMutex.Unlock()
 		assert.Equal(t, st.MinTime, int64(0))
 	}
 
@@ -1472,7 +1474,9 @@ func TestFetchTimebounds(t *testing.T) {
 	// get saved server time
 	st, err = client.FetchTimebounds(100)
 	if assert.NoError(t, err) {
+		serverTimeMapMutex.Lock()
 		assert.IsType(t, ServerTimeMap["localhost"], ServerTimeRecord{})
+		serverTimeMapMutex.Unlock()
 		assert.Equal(t, st.MinTime, int64(0))
 		// serverTime + 100seconds
 		assert.Equal(t, st.MaxTime, int64(1560947196))
@@ -1480,7 +1484,9 @@ func TestFetchTimebounds(t *testing.T) {
 
 	// mock server time
 	newRecord := ServerTimeRecord{ServerTime: 100, LocalTimeRecorded: 1560947096}
+	serverTimeMapMutex.Lock()
 	ServerTimeMap["localhost"] = newRecord
+	serverTimeMapMutex.Unlock()
 	st, err = client.FetchTimebounds(100)
 	assert.NoError(t, err)
 	assert.IsType(t, st, txnbuild.TimeBounds{})