Binance: implement get historic trades (#588)

* Binance: implement get historic trades

* get binance trade data based on aggregate trade list
* fix small issue in rpc server: gctcli stops retrieving when there's
a gap in data

* update binance trade history availability in readme

* limit check batched aggregate requests

* add test for batched aggregated trades
* fix batch fromId query parameter
* update documentation

* send a serialised currency pair to GetAggregatedTrades

the rationale is that the API is kept generic so that callers can shoot
themselves in the foot if they want to

* allow requesting arbitrary limit of trades

* handle some error cases for batching GetAggregateTrades

* fix batch without end time

* don't return from batch too early if end time is not set
* additional check for supported limits

* don't use CheckLimits for GetAggregatedTrades

* the exchange doesn't use predefined valid limits for this request

exchanges/binance/binance.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"sort"
 	"strconv"
 	"strings"
 	"time"
@@ -161,27 +162,122 @@ func (b *Binance) GetHistoricalTrades(symbol string, limit int, fromID int64) ([
 	return nil, common.ErrFunctionNotSupported
 }
 
-// GetAggregatedTrades returns aggregated trade activity
-//
-// symbol: string of currency pair
-// limit: Optional. Default 500; max 1000.
-func (b *Binance) GetAggregatedTrades(symbol string, limit int) ([]AggregatedTrade, error) {
-	var resp []AggregatedTrade
-
-	if err := b.CheckLimit(limit); err != nil {
-		return resp, err
-	}
-
+// GetAggregatedTrades returns aggregated trade activity.
+// If more than one hour of data is requested or asked limit is not supported by exchange
+// then the trades are collected with multiple backend requests.
+// https://binance-docs.github.io/apidocs/spot/en/#compressed-aggregate-trades-list
+func (b *Binance) GetAggregatedTrades(arg *AggregatedTradeRequestParams) ([]AggregatedTrade, error) {
 	params := url.Values{}
-	params.Set("symbol", strings.ToUpper(symbol))
-	if limit > 0 {
-		params.Set("limit", strconv.Itoa(limit))
+	params.Set("symbol", arg.Symbol)
+	// if the user request is directly not supported by the exchange, we might be able to fulfill it
+	// by merging results from multiple API requests
+	needBatch := false
+	if arg.Limit > 0 {
+		if arg.Limit > 1000 {
+			// remote call doesn't support higher limits
+			needBatch = true
+		} else {
+			params.Set("limit", strconv.Itoa(arg.Limit))
+		}
+	}
+	if arg.FromID != 0 {
+		params.Set("fromId", strconv.FormatInt(arg.FromID, 10))
+	}
+	if !arg.StartTime.IsZero() {
+		params.Set("startTime", strconv.FormatInt(convert.UnixMillis(arg.StartTime), 10))
+	}
+	if !arg.EndTime.IsZero() {
+		params.Set("endTime", strconv.FormatInt(convert.UnixMillis(arg.EndTime), 10))
 	}
 
+	// startTime and endTime are set and time between startTime and endTime is more than 1 hour
+	needBatch = needBatch || (!arg.StartTime.IsZero() && !arg.EndTime.IsZero() && arg.EndTime.Sub(arg.StartTime) > time.Hour)
+	// Fall back to batch requests, if possible and necessary
+	if needBatch {
+		// fromId xor start time must be set
+		canBatch := arg.FromID == 0 != arg.StartTime.IsZero()
+		if canBatch {
+			// Split the request into multiple
+			return b.batchAggregateTrades(arg, params)
+		}
+
+		// Can't handle this request locally or remotely
+		// We would receive {"code":-1128,"msg":"Combination of optional parameters invalid."}
+		return nil, errors.New("please set StartTime or FromId, but not both")
+	}
+
+	var resp []AggregatedTrade
 	path := b.API.Endpoints.URL + aggregatedTrades + "?" + params.Encode()
 	return resp, b.SendHTTPRequest(path, limitDefault, &resp)
 }
 
+// batchAggregateTrades fetches trades in multiple requests
+// first phase, hourly requests until the first trade (or end time) is reached
+// second phase, limit requests from previous trade until end time (or limit) is reached
+func (b *Binance) batchAggregateTrades(arg *AggregatedTradeRequestParams, params url.Values) ([]AggregatedTrade, error) {
+	var resp []AggregatedTrade
+	// prepare first request with only first hour and max limit
+	if arg.Limit == 0 || arg.Limit > 1000 {
+		// Extend from the default of 500
+		params.Set("limit", "1000")
+	}
+
+	var fromID int64
+	if arg.FromID > 0 {
+		fromID = arg.FromID
+	} else {
+		for start := arg.StartTime; len(resp) == 0; start = start.Add(time.Hour) {
+			if !arg.EndTime.IsZero() && !start.Before(arg.EndTime) {
+				// All requests returned empty
+				return nil, nil
+			}
+			params.Set("startTime", strconv.FormatInt(convert.UnixMillis(start), 10))
+			params.Set("endTime", strconv.FormatInt(convert.UnixMillis(start.Add(time.Hour)), 10))
+			path := b.API.Endpoints.URL + aggregatedTrades + "?" + params.Encode()
+			err := b.SendHTTPRequest(path, limitDefault, &resp)
+			if err != nil {
+				log.Warn(log.ExchangeSys, err.Error())
+				return resp, err
+			}
+		}
+		fromID = resp[len(resp)-1].ATradeID
+	}
+
+	// other requests follow from the last aggregate trade id and have no time window
+	params.Del("startTime")
+	params.Del("endTime")
+	// while we haven't reached the limit
+	for ; arg.Limit == 0 || len(resp) < arg.Limit; fromID = resp[len(resp)-1].ATradeID {
+		// Keep requesting new data after last retrieved trade
+		params.Set("fromId", strconv.FormatInt(fromID, 10))
+		path := b.API.Endpoints.URL + aggregatedTrades + "?" + params.Encode()
+		var additionalTrades []AggregatedTrade
+		err := b.SendHTTPRequest(path, limitDefault, &additionalTrades)
+		if err != nil {
+			return resp, err
+		}
+		lastIndex := len(additionalTrades)
+		if !arg.EndTime.IsZero() {
+			// get index for truncating to end time
+			lastIndex = sort.Search(len(additionalTrades), func(i int) bool {
+				return convert.UnixMillis(arg.EndTime) < additionalTrades[i].TimeStamp
+			})
+		}
+		// don't include the first as the request was inclusive from last ATradeID
+		resp = append(resp, additionalTrades[1:lastIndex]...)
+		// If only the starting trade is returned or if we received trades after end time
+		if len(additionalTrades) == 1 || lastIndex < len(additionalTrades) {
+			// We found the end
+			break
+		}
+	}
+	// Truncate if necessary
+	if arg.Limit > 0 && len(resp) > arg.Limit {
+		resp = resp[:arg.Limit]
+	}
+	return resp, nil
+}
+
 // GetSpotKline returns kline data
 //
 // KlinesRequestParams supports 5 parameters

exchanges/binance/binance_test.go

@@ -93,7 +93,10 @@ func TestGetHistoricalTrades(t *testing.T) {
 
 func TestGetAggregatedTrades(t *testing.T) {
 	t.Parallel()
-	_, err := b.GetAggregatedTrades("BTCUSDT", 5)
+	_, err := b.GetAggregatedTrades(&AggregatedTradeRequestParams{
+		Symbol: currency.NewPair(currency.BTC, currency.USDT).String(),
+		Limit:  5,
+	})
 	if err != nil {
 		t.Error("Binance GetAggregatedTrades() error", err)
 	}
@@ -394,6 +397,178 @@ func TestNewOrderTest(t *testing.T) {
 	}
 }
 
+func TestGetHistoricTrades(t *testing.T) {
+	t.Parallel()
+	currencyPair, err := currency.NewPairFromString("BTCUSDT")
+	if err != nil {
+		t.Fatal(err)
+	}
+	start, err := time.Parse(time.RFC3339, "2020-01-02T15:04:05Z")
+	if err != nil {
+		t.Fatal(err)
+	}
+	result, err := b.GetHistoricTrades(currencyPair, asset.Spot, start, start.Add(15*time.Minute))
+	if err != nil {
+		t.Error(err)
+	}
+	var expected int
+	if mockTests {
+		expected = 5
+	} else {
+		expected = 2134
+	}
+	if len(result) != expected {
+		t.Errorf("GetHistoricTrades() expected %v entries, got %v", expected, len(result))
+	}
+}
+
+func TestGetAggregatedTradesBatched(t *testing.T) {
+	t.Parallel()
+	currencyPair, err := currency.NewPairFromString("BTCUSDT")
+	if err != nil {
+		t.Fatal(err)
+	}
+	start, err := time.Parse(time.RFC3339, "2020-01-02T15:04:05Z")
+	if err != nil {
+		t.Fatal(err)
+	}
+	mockExpectTime, err := time.Parse(time.RFC3339, "2020-01-02T16:19:04.8Z")
+	if err != nil {
+		t.Fatal(err)
+	}
+	expectTime, err := time.Parse(time.RFC3339Nano, "2020-01-02T16:19:04.831Z")
+	if err != nil {
+		t.Fatal(err)
+	}
+	tests := []struct {
+		name string
+		// mock test or live test
+		mock         bool
+		args         *AggregatedTradeRequestParams
+		numExpected  int
+		lastExpected time.Time
+	}{
+		{
+			name: "mock batch with timerange",
+			mock: true,
+			args: &AggregatedTradeRequestParams{
+				Symbol:    currencyPair.String(),
+				StartTime: start,
+				EndTime:   start.Add(75 * time.Minute),
+			},
+			numExpected:  3,
+			lastExpected: mockExpectTime,
+		},
+		{
+			name: "batch with timerange",
+			args: &AggregatedTradeRequestParams{
+				Symbol:    currencyPair.String(),
+				StartTime: start,
+				EndTime:   start.Add(75 * time.Minute),
+			},
+			numExpected:  4303,
+			lastExpected: expectTime,
+		},
+		{
+			name: "mock custom limit with start time set, no end time",
+			mock: true,
+			args: &AggregatedTradeRequestParams{
+				Symbol:    currency.NewPair(currency.BTC, currency.USDT).String(),
+				StartTime: start,
+				Limit:     1001,
+			},
+			numExpected:  4,
+			lastExpected: time.Date(2020, 1, 2, 16, 19, 5, int(200*time.Millisecond), time.UTC),
+		},
+		{
+			name: "custom limit with start time set, no end time",
+			args: &AggregatedTradeRequestParams{
+				Symbol:    currency.NewPair(currency.BTC, currency.USDT).String(),
+				StartTime: time.Date(2020, 11, 18, 12, 0, 0, 0, time.UTC),
+				Limit:     1001,
+			},
+			numExpected:  1001,
+			lastExpected: time.Date(2020, 11, 18, 13, 0, 0, int(34*time.Millisecond), time.UTC),
+		},
+		{
+			name: "mock recent trades",
+			mock: true,
+			args: &AggregatedTradeRequestParams{
+				Symbol: currency.NewPair(currency.BTC, currency.USDT).String(),
+				Limit:  3,
+			},
+			numExpected:  3,
+			lastExpected: time.Date(2020, 1, 2, 16, 19, 5, int(200*time.Millisecond), time.UTC),
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.mock != mockTests {
+				t.Skip()
+			}
+			result, err := b.GetAggregatedTrades(tt.args)
+			if err != nil {
+				t.Error(err)
+			}
+			if len(result) != tt.numExpected {
+				t.Errorf("GetAggregatedTradesBatched() expected %v entries, got %v", tt.numExpected, len(result))
+			}
+			lastTrade := result[len(result)-1]
+			lastTradeTime := time.Unix(0, lastTrade.TimeStamp*int64(time.Millisecond))
+			if !lastTradeTime.Equal(tt.lastExpected) {
+				t.Errorf("last trade expected %v, got %v", tt.lastExpected, lastTradeTime)
+			}
+		})
+	}
+}
+
+func TestGetAggregatedTradesErrors(t *testing.T) {
+	t.Parallel()
+	start, err := time.Parse(time.RFC3339, "2020-01-02T15:04:05Z")
+	if err != nil {
+		t.Fatal(err)
+	}
+	tests := []struct {
+		name string
+		args *AggregatedTradeRequestParams
+	}{
+		{
+			name: "get recent trades does not support custom limit",
+			args: &AggregatedTradeRequestParams{
+				Symbol: currency.NewPair(currency.BTC, currency.USDT).String(),
+				Limit:  1001,
+			},
+		},
+		{
+			name: "start time and fromId cannot be both set",
+			args: &AggregatedTradeRequestParams{
+				Symbol:    currency.NewPair(currency.BTC, currency.USDT).String(),
+				StartTime: start,
+				EndTime:   start.Add(75 * time.Minute),
+				FromID:    2,
+			},
+		},
+		{
+			name: "can't get most recent 5000 (more than 1000 not allowed)",
+			args: &AggregatedTradeRequestParams{
+				Symbol: currency.NewPair(currency.BTC, currency.USDT).String(),
+				Limit:  5000,
+			},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := b.GetAggregatedTrades(tt.args)
+			if err == nil {
+				t.Errorf("Binance.GetAggregatedTrades() error = %v, wantErr true", err)
+				return
+			}
+		})
+	}
+}
+
 // Any tests below this line have the ability to impact your orders on the exchange. Enable canManipulateRealOrders to run them
 // -----------------------------------------------------------------------------------------------------------------------------
 
@@ -989,15 +1164,3 @@ func TestGetRecentTrades(t *testing.T) {
 		t.Error(err)
 	}
 }
-
-func TestGetHistoricTrades(t *testing.T) {
-	t.Parallel()
-	currencyPair, err := currency.NewPairFromString("BTCUSDT")
-	if err != nil {
-		t.Fatal(err)
-	}
-	_, err = b.GetHistoricTrades(currencyPair, asset.Spot, time.Now().Add(-time.Minute*15), time.Now())
-	if err != nil && err != common.ErrFunctionNotSupported {
-		t.Error(err)
-	}
-}

exchanges/binance/binance_types.go

@@ -200,6 +200,18 @@ type HistoricalTrade struct {
 	IsBestMatch  bool    `json:"isBestMatch"`
 }
 
+// AggregatedTradeRequestParams holds request params
+type AggregatedTradeRequestParams struct {
+	Symbol string // Required field; example LTCBTC, BTCUSDT
+	// The first trade to retrieve
+	FromID int64
+	// The API seems to accept (start and end time) or FromID and no other combinations
+	StartTime time.Time
+	EndTime   time.Time
+	// Default 500; max 1000.
+	Limit int
+}
+
 // AggregatedTrade holds aggregated trade information
 type AggregatedTrade struct {
 	ATradeID       int64   `json:"a"`

exchanges/binance/binance_wrapper.go

@@ -534,8 +534,40 @@ func (b *Binance) GetRecentTrades(p currency.Pair, assetType asset.Item) ([]trad
 }
 
 // GetHistoricTrades returns historic trade data within the timeframe provided
-func (b *Binance) GetHistoricTrades(_ currency.Pair, _ asset.Item, _, _ time.Time) ([]trade.Data, error) {
-	return nil, common.ErrFunctionNotSupported
+func (b *Binance) GetHistoricTrades(p currency.Pair, a asset.Item, from, to time.Time) ([]trade.Data, error) {
+	p, err := b.FormatExchangeCurrency(p, a)
+	if err != nil {
+		return nil, err
+	}
+	req := AggregatedTradeRequestParams{
+		Symbol:    p.String(),
+		StartTime: from,
+		EndTime:   to,
+	}
+	trades, err := b.GetAggregatedTrades(&req)
+	if err != nil {
+		return nil, err
+	}
+	var result []trade.Data
+	exName := b.GetName()
+	for i := range trades {
+		t := trades[i].toTradeData(p, exName, a)
+		result = append(result, *t)
+	}
+	return result, nil
+}
+
+func (a *AggregatedTrade) toTradeData(p currency.Pair, exchange string, aType asset.Item) *trade.Data {
+	return &trade.Data{
+		CurrencyPair: p,
+		TID:          strconv.FormatInt(a.ATradeID, 10),
+		Amount:       a.Quantity,
+		Exchange:     exchange,
+		Price:        a.Price,
+		Timestamp:    time.Unix(0, a.TimeStamp*int64(time.Millisecond)),
+		AssetType:    aType,
+		Side:         order.AnySide,
+	}
 }
 
 // SubmitOrder submits a new order

exchanges/trade/README.md

@@ -60,7 +60,7 @@ _b in this context is an `IBotExchange` implemented struct_
 | Exchange | Recent Trades via REST | Live trade updates via Websocket | Trade history via REST |
 |----------|------|-----------|-----|
 | Alphapoint | No  | No        | No  |
-| Binance| Yes  | Yes        | No  |
+| Binance| Yes  | Yes        | Yes  |
 | Bitfinex | Yes  | Yes        | Yes  |
 | Bitflyer | Yes  | No      | No  |
 | Bithumb | Yes  | NA       | No  |