(BTC Markets): Wrapper SubmitOrder parameter order fix & IsOrderPlaced condition correction (#394)

* corrected param order Side -> Type, also corrected condition check for IsOrderPlaced

* send open status for GetActiveOrders

* GetActiveOrder() changes to include OrderID and status matching

* BTC Markets batch order limit fixes & SplitStringSliceByLimit  method

BTC markets batch end points have limits (20 for cancel 50 for query) adds new method SplitStringSliceByLimit in common to split a slice by limit and return slice of slice

* rm line :D

* Added test for SplitStringSliceByLimit and moved to const

* ntp client reworked to not return error if no valid time servers are found but default to system

* clean up

* new line added

* use TimeMgr sublogger and wording correction on output

* Moved to DialTimeout() & Removed SetDeadline call

* removed line

* added setdeadline fix

* goimport file

* removed unused error from NTPClient as we now default to system time if no server can be reached

* Added checks for number overflows

* converted to uint as you should not be passing a negative number in

* Increased test cases for NTPClient

* Removed Helper call as no longer outputting any data from function

* removed unused param

common/common.go

@@ -340,3 +340,17 @@ func ChangePermission(directory string) error {
 		return nil
 	})
 }
+
+// SplitStringSliceByLimit splits a slice of strings into slices by input limit and returns a slice of slice of strings
+func SplitStringSliceByLimit(in []string, limit uint) [][]string {
+	var stringSlice []string
+	sliceSlice := make([][]string, 0, len(in)/int(limit)+1)
+	for len(in) >= int(limit) {
+		stringSlice, in = in[:limit], in[limit:]
+		sliceSlice = append(sliceSlice, stringSlice)
+	}
+	if len(in) > 0 {
+		sliceSlice = append(sliceSlice, in)
+	}
+	return sliceSlice
+}

common/common_test.go

@@ -7,6 +7,7 @@ import (
 	"path/filepath"
 	"reflect"
 	"runtime"
+	"strconv"
 	"strings"
 	"testing"
 )
@@ -499,3 +500,29 @@ func TestChangePermission(t *testing.T) {
 		}
 	}
 }
+
+func initStringSlice(size int) (out []string) {
+	for x := 0; x < size; x++ {
+		out = append(out, "gct-"+strconv.Itoa(x))
+	}
+	return
+}
+
+func TestSplitStringSliceByLimit(t *testing.T) {
+	slice50 := initStringSlice(50)
+	out := SplitStringSliceByLimit(slice50, 20)
+	if len(out) != 3 {
+		t.Errorf("expected len() to be 3 instead received: %v", len(out))
+	}
+	if len(out[0]) != 20 {
+		t.Errorf("expected len() to be 20 instead received: %v", len(out[0]))
+	}
+
+	out = SplitStringSliceByLimit(slice50, 50)
+	if len(out) != 1 {
+		t.Errorf("expected len() to be 3 instead received: %v", len(out))
+	}
+	if len(out[0]) != 50 {
+		t.Errorf("expected len() to be 20 instead received: %v", len(out[0]))
+	}
+}

config/config_test.go

@@ -1796,10 +1796,7 @@ func TestCheckNTPConfig(t *testing.T) {
 	c.NTPClient.AllowedDifference = nil
 
 	c.CheckNTPConfig()
-	_, err := ntpclient.NTPClient(c.NTPClient.Pool)
-	if err != nil {
-		t.Fatalf("to create ntpclient failed reason: %v", err)
-	}
+	_ = ntpclient.NTPClient(c.NTPClient.Pool)
 
 	if c.NTPClient.Pool[0] != "pool.ntp.org:123" {
 		t.Error("ntpclient with no valid pool should default to pool.ntp.org")

engine/orders.go

@@ -220,7 +220,7 @@ func (o *orderManager) Submit(exchName string, newOrder *order.Submit) (*orderSu
 		return nil, err
 	}
 
-	if result.IsOrderPlaced {
+	if !result.IsOrderPlaced {
 		return nil, errors.New("order unable to be placed")
 	}
 

engine/timekeeper.go

@@ -110,15 +110,12 @@ func (n *ntpManager) run() {
 	}
 }
 
-func (n *ntpManager) FetchNTPTime() (time.Time, error) {
+func (n *ntpManager) FetchNTPTime() time.Time {
 	return ntpclient.NTPClient(Bot.Config.NTPClient.Pool)
 }
 
 func (n *ntpManager) processTime() error {
-	NTPTime, err := n.FetchNTPTime()
-	if err != nil {
-		return err
-	}
+	NTPTime := n.FetchNTPTime()
 
 	currentTime := time.Now()
 	NTPcurrentTimeDifference := NTPTime.Sub(currentTime)

exchanges/btcmarkets/btcmarkets.go

@@ -55,7 +55,7 @@ const (
 	orderFailed             = "Failed"
 	orderPartiallyCancelled = "Partially Cancelled"
 	orderCancelled          = "Cancelled"
-	orderFullyMatched       = "FullyMatched"
+	orderFullyMatched       = "Fully Matched"
 	orderPartiallyMatched   = "Partially Matched"
 	orderPlaced             = "Placed"
 	orderAccepted           = "Accepted"

exchanges/btcmarkets/btcmarkets_wrapper.go

@@ -363,8 +363,8 @@ func (b *BTCMarkets) SubmitOrder(s *order.Submit) (order.SubmitResponse, error)
 	tempResp, err := b.NewOrder(b.FormatExchangeCurrency(s.Pair, asset.Spot).String(),
 		s.Price,
 		s.Amount,
-		s.OrderSide.String(),
 		s.OrderType.String(),
+		s.OrderSide.String(),
 		s.TriggerPrice,
 		s.TargetAmount,
 		"",
@@ -403,16 +403,20 @@ func (b *BTCMarkets) CancelAllOrders(_ *order.Cancel) (order.CancelAllResponse,
 	for x := range orders {
 		orderIDs = append(orderIDs, orders[x].OrderID)
 	}
-	tempResp, err := b.CancelBatchOrders(orderIDs)
-	if err != nil {
-		return resp, err
-	}
-	for y := range tempResp.CancelOrders {
-		tempMap[tempResp.CancelOrders[y].OrderID] = "Success"
-	}
-	for z := range tempResp.UnprocessedRequests {
-		tempMap[tempResp.UnprocessedRequests[z].RequestID] = "Cancellation Failed"
+	splitOrders := common.SplitStringSliceByLimit(orderIDs, 20)
+	for z := range splitOrders {
+		tempResp, err := b.CancelBatchOrders(splitOrders[z])
+		if err != nil {
+			return resp, err
+		}
+		for y := range tempResp.CancelOrders {
+			tempMap[tempResp.CancelOrders[y].OrderID] = "Success"
+		}
+		for z := range tempResp.UnprocessedRequests {
+			tempMap[tempResp.UnprocessedRequests[z].RequestID] = "Cancellation Failed"
+		}
 	}
+	resp.Status = tempMap
 	return resp, nil
 }
 
@@ -534,9 +538,6 @@ func (b *BTCMarkets) GetFeeByType(feeBuilder *exchange.FeeBuilder) (float64, err
 
 // GetActiveOrders retrieves any orders that are active/open
 func (b *BTCMarkets) GetActiveOrders(req *order.GetOrdersRequest) ([]order.Detail, error) {
-	var resp []order.Detail
-	var tempResp order.Detail
-	var tempData []OrderData
 	if len(req.Currencies) == 0 {
 		allPairs := b.GetEnabledPairs(asset.Spot)
 		for a := range allPairs {
@@ -544,20 +545,38 @@ func (b *BTCMarkets) GetActiveOrders(req *order.GetOrdersRequest) ([]order.Detai
 				allPairs[a])
 		}
 	}
+
+	var resp []order.Detail
 	var err error
 	for x := range req.Currencies {
-		tempData, err = b.GetOrders(b.FormatExchangeCurrency(req.Currencies[x], asset.Spot).String(), -1, -1, -1, "")
+		var tempData []OrderData
+		tempData, err = b.GetOrders(b.FormatExchangeCurrency(req.Currencies[x], asset.Spot).String(), -1, -1, -1, "open")
 		if err != nil {
 			return resp, err
 		}
 		for y := range tempData {
+			var tempResp order.Detail
 			tempResp.Exchange = b.Name
 			tempResp.CurrencyPair = req.Currencies[x]
+			tempResp.ID = tempData[y].OrderID
 			tempResp.OrderSide = order.Bid
 			if tempData[y].Side == ask {
 				tempResp.OrderSide = order.Ask
 			}
 			tempResp.OrderDate = tempData[y].CreationTime
+
+			switch tempData[y].Type {
+			case limit:
+				tempResp.OrderType = order.Limit
+			case market:
+				tempResp.OrderType = order.Market
+			default:
+				log.Errorf(log.ExchangeSys,
+					"%s unknown order type %s getting order",
+					b.Name,
+					tempData[y].Type)
+				tempResp.OrderType = order.Unknown
+			}
 			switch tempData[y].Status {
 			case orderAccepted:
 				tempResp.Status = order.Active
@@ -565,14 +584,13 @@ func (b *BTCMarkets) GetActiveOrders(req *order.GetOrdersRequest) ([]order.Detai
 				tempResp.Status = order.Active
 			case orderPartiallyMatched:
 				tempResp.Status = order.PartiallyFilled
-			case orderFullyMatched:
-				tempResp.Status = order.Filled
-			case orderCancelled:
-				tempResp.Status = order.Cancelled
-			case orderPartiallyCancelled:
-				tempResp.Status = order.PartiallyCancelled
-			case orderFailed:
-				tempResp.Status = order.Rejected
+			default:
+				log.Errorf(log.ExchangeSys,
+					"%s unexpected status %s on order %v",
+					b.Name,
+					tempData[y].Status,
+					tempData[y].OrderID)
+				tempResp.Status = order.UnknownStatus
 			}
 			tempResp.Price = tempData[y].Price
 			tempResp.Amount = tempData[y].Amount
@@ -611,37 +629,41 @@ func (b *BTCMarkets) GetOrderHistory(req *order.GetOrdersRequest) ([]order.Detai
 			tempArray = append(tempArray, orders[z].OrderID)
 		}
 	}
-	tempData, err := b.GetBatchTrades(tempArray)
-	if err != nil {
-		return resp, err
-	}
-	for c := range tempData.Orders {
-		switch tempData.Orders[c].Status {
-		case orderFailed:
-			tempResp.Status = order.Rejected
-		case orderPartiallyCancelled:
-			tempResp.Status = order.PartiallyCancelled
-		case orderCancelled:
-			tempResp.Status = order.Cancelled
-		case orderFullyMatched:
-			tempResp.Status = order.Filled
-		case orderPartiallyMatched:
-			continue
-		case orderPlaced:
-			continue
-		case orderAccepted:
-			continue
+	splitOrders := common.SplitStringSliceByLimit(tempArray, 50)
+	for x := range splitOrders {
+		tempData, err := b.GetBatchTrades(splitOrders[x])
+		if err != nil {
+			return resp, err
 		}
-		tempResp.Exchange = b.Name
-		tempResp.CurrencyPair = currency.NewPairFromString(tempData.Orders[c].MarketID)
-		tempResp.OrderSide = order.Bid
-		if tempData.Orders[c].Side == ask {
-			tempResp.OrderSide = order.Ask
+		for c := range tempData.Orders {
+			switch tempData.Orders[c].Status {
+			case orderFailed:
+				tempResp.Status = order.Rejected
+			case orderPartiallyCancelled:
+				tempResp.Status = order.PartiallyCancelled
+			case orderCancelled:
+				tempResp.Status = order.Cancelled
+			case orderFullyMatched:
+				tempResp.Status = order.Filled
+			case orderPartiallyMatched:
+				continue
+			case orderPlaced:
+				continue
+			case orderAccepted:
+				continue
+			}
+			tempResp.Exchange = b.Name
+			tempResp.CurrencyPair = currency.NewPairFromString(tempData.Orders[c].MarketID)
+			tempResp.OrderSide = order.Bid
+			if tempData.Orders[c].Side == ask {
+				tempResp.OrderSide = order.Ask
+			}
+			tempResp.ID = tempData.Orders[c].OrderID
+			tempResp.OrderDate = tempData.Orders[c].CreationTime
+			tempResp.Price = tempData.Orders[c].Price
+			tempResp.ExecutedAmount = tempData.Orders[c].Amount
+			resp = append(resp, tempResp)
 		}
-		tempResp.OrderDate = tempData.Orders[c].CreationTime
-		tempResp.Price = tempData.Orders[c].Price
-		tempResp.ExecutedAmount = tempData.Orders[c].Amount
-		resp = append(resp, tempResp)
 	}
 	return resp, nil
 }

ntpclient/ntpclient.go

@@ -2,7 +2,6 @@ package ntpclient
 
 import (
 	"encoding/binary"
-	"errors"
 	"net"
 	"time"
 
@@ -28,32 +27,39 @@ type ntppacket struct {
 }
 
 // NTPClient create's a new NTPClient and returns local based on ntp servers provided timestamp
-func NTPClient(pool []string) (time.Time, error) {
+// if no server can be reached will return local time in UTC()
+func NTPClient(pool []string) time.Time {
 	for i := range pool {
-		con, err := net.Dial("udp", pool[i])
+		con, err := net.DialTimeout("udp", pool[i], 5*time.Second)
 		if err != nil {
 			log.Warnf(log.TimeMgr, "Unable to connect to hosts %v attempting next", pool[i])
 			continue
 		}
 
-		defer con.Close()
-
-		con.SetDeadline(time.Now().Add(5 * time.Second))
+		if err := con.SetDeadline(time.Now().Add(5 * time.Second)); err != nil {
+			log.Warnf(log.TimeMgr, "Unable to SetDeadline. Error: %s\n", err)
+			con.Close()
+			continue
+		}
 
 		req := &ntppacket{Settings: 0x1B}
 		if err := binary.Write(con, binary.BigEndian, req); err != nil {
+			con.Close()
 			continue
 		}
 
 		rsp := &ntppacket{}
 		if err := binary.Read(con, binary.BigEndian, rsp); err != nil {
+			con.Close()
 			continue
 		}
 
 		secs := float64(rsp.TxTimeSec) - 2208988800
 		nanos := (int64(rsp.TxTimeFrac) * 1e9) >> 32
 
-		return time.Unix(int64(secs), nanos), nil
+		con.Close()
+		return time.Unix(int64(secs), nanos)
 	}
-	return time.Unix(0, 0), errors.New("no valid time servers")
+	log.Warnln(log.TimeMgr, "No valid NTP servers found, using current system time")
+	return time.Now().UTC()
 }

ntpclient/ntpclient_test.go

@@ -1,25 +1,26 @@
 package ntpclient
 
 import (
+	"reflect"
 	"testing"
+	"time"
 )
 
 func TestNTPClient(t *testing.T) {
 	pool := []string{"pool.ntp.org:123", "0.pool.ntp.org:123"}
-	_, err := NTPClient(pool)
-	if err != nil {
-		t.Fatalf("failed to get time %v", err)
+	v := NTPClient(pool)
+
+	if reflect.TypeOf(v) != reflect.TypeOf(time.Time{}) {
+		t.Errorf("NTPClient should return time.Time{}")
 	}
 
-	invalidpool := []string{"pool.thisisinvalid.org"}
-	_, err = NTPClient(invalidpool)
-	if err == nil {
-		t.Errorf("Expected error")
+	if v.IsZero() {
+		t.Error("NTPClient should return valid time received zero value")
 	}
 
-	firstInvalid := []string{"pool.thisisinvalid.org", "pool.ntp.org:123", "0.pool.ntp.org:123"}
-	_, err = NTPClient(firstInvalid)
-	if err != nil {
-		t.Errorf("failed to get time %v", err)
+	const timeFormat = "2006-01-02T15:04:05"
+
+	if v.UTC().Format(timeFormat) != time.Now().UTC().Format(timeFormat) {
+		t.Errorf("NTPClient returned incorrect time received: %v", v.UTC().Format(timeFormat))
 	}
 }