From a727beeb15bc437a0c693b24ff5772cead25390d Mon Sep 17 00:00:00 2001 From: Andrew Date: Mon, 16 Dec 2019 14:47:54 +1100 Subject: [PATCH] (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 | 14 +++ common/common_test.go | 27 +++++ config/config_test.go | 5 +- engine/orders.go | 2 +- engine/timekeeper.go | 7 +- exchanges/btcmarkets/btcmarkets.go | 2 +- exchanges/btcmarkets/btcmarkets_wrapper.go | 124 ++++++++++++--------- ntpclient/ntpclient.go | 22 ++-- ntpclient/ntpclient_test.go | 23 ++-- 9 files changed, 145 insertions(+), 81 deletions(-) diff --git a/common/common.go b/common/common.go index ecccac193ad..93086851b54 100644 --- a/common/common.go +++ b/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 +} diff --git a/common/common_test.go b/common/common_test.go index 7f892fbf907..5413564af44 100644 --- a/common/common_test.go +++ b/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])) + } +} diff --git a/config/config_test.go b/config/config_test.go index 45a0409a70b..21a1a3869e5 100644 --- a/config/config_test.go +++ b/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") diff --git a/engine/orders.go b/engine/orders.go index aa50406ebe4..c177678dd9d 100644 --- a/engine/orders.go +++ b/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") } diff --git a/engine/timekeeper.go b/engine/timekeeper.go index 8956ff6d135..cbec0ccaa5e 100644 --- a/engine/timekeeper.go +++ b/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) diff --git a/exchanges/btcmarkets/btcmarkets.go b/exchanges/btcmarkets/btcmarkets.go index a477f483695..637e32ae147 100644 --- a/exchanges/btcmarkets/btcmarkets.go +++ b/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" diff --git a/exchanges/btcmarkets/btcmarkets_wrapper.go b/exchanges/btcmarkets/btcmarkets_wrapper.go index eed1560b4d8..060db5f975e 100644 --- a/exchanges/btcmarkets/btcmarkets_wrapper.go +++ b/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 } diff --git a/ntpclient/ntpclient.go b/ntpclient/ntpclient.go index 2f239f7a111..9c9314a251e 100644 --- a/ntpclient/ntpclient.go +++ b/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() } diff --git a/ntpclient/ntpclient_test.go b/ntpclient/ntpclient_test.go index 6fe5ee5ea1f..5693430dda4 100644 --- a/ntpclient/ntpclient_test.go +++ b/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)) } }