Add OHLC retrieval func (GetHistoricCandles) to all exchanges and expose it as a wrapper func #414

Christian-Achilli · 2020-01-14T20:49:45Z

PR Description

Added a method to the CLI to pull historic rates (candles) out of an exchange. Only implemented for Coinbasepro.
RPC layer implemented as well by adding method in rpc.proto, rigenerating protobuf files, adding method to coihnbase_wrapper.go

Fixes # (issue)
Fixed GetHistoricRates in coinbasepro.go as the arguments were of the wrong type. Tested and working against Coinbase.
Fixed the linux script to generate protobuf files as it was not workin on mac. For some reason the GOPATH was not resolved.

Type of change

Added funcionality to CLI and RPC

Please delete options that are not relevant and add an x in [] as item is complete.

[x ] New feature (non-breaking change which adds functionality)

How has this been tested

[x ] go test ./... -race
golangci-lint run
Test X

Tested end to end against coinbase

Checklist

[x ] My code follows the style guidelines of this project
I have performed a self-review of my own code
[ x] I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
[ x] My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Travis with my changes
[ x] Any dependent changes have been merged and published in downstream modules

into feature/history-trades

codecov · 2020-01-14T23:15:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8c29d11). Click here to learn what that means.
The diff coverage is 43.71%.

@@            Coverage Diff            @@
##             master     #414   +/-   ##
=========================================
  Coverage          ?   41.17%           
=========================================
  Files             ?      181           
  Lines             ?    41976           
  Branches          ?        0           
=========================================
  Hits              ?    17282           
  Misses            ?    23251           
  Partials          ?     1443

Impacted Files	Coverage Δ
engine/orders.go	`0% <ø> (ø)`
logger/logger_types.go	`100% <ø> (ø)`
config/config_types.go	`79.41% <ø> (ø)`
engine/gctscript.go	`0% <0%> (ø)`
database/models/postgres/script_execution.go	`0% <0%> (ø)`
engine/rpcserver.go	`0% <0%> (ø)`
engine/engine.go	`0% <0%> (ø)`
gctscript/wrappers/validator/validator.go	`100% <100%> (ø)`
gctscript/vm/vm_error.go	`100% <100%> (ø)`
gctscript/modules/loader/loader.go	`100% <100%> (ø)`
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c29d11...0fdf729. Read the comment docs.

thrasher-

Awesome stuff Christian and thank you for your first contribution! I have just a few changes to request

thrasher- · 2020-01-15T22:36:24Z

exchanges/binance/binance.go

@@ -72,6 +72,10 @@ type Binance struct {
 	validIntervals []TimeInterval
 }

+func (b *Binance) GetHistoricCandles(pair currency.Pair, rangesize int, granularity int) ([]exchange.Candle, error) {
+	return nil, common.ErrFunctionNotSupported


When an exchange API endpoint is available but is not yet implemented, we use common.ErrFunctionNotImplemented. When an exchange API endpoint isn't available, we use common.ErrFunctionNotSupported. This is used by our exchange_wrapper_coverage and exchange_wrapper_issues tools (https://github.com/thrasher-corp/gocryptotrader/tree/master/cmd/exchange_wrapper_coverage) helping us to know which exchanges support or don't have to support for a particular wrapper func.

The other thing is that we have a segregation between exchange API funcs and wrapper funcs. Wrapper funcs go into the exchanges corresponding _wrapper.go file and API non-wrapper related funcs go into the exchanges main file (binance.go)

Thanks for the explanation. Indeed it makes sense.
Question on the wrapper funcs, please. I understand GCT aims at providing some kind of unified API across all Exchanges, as far as it is possible.
So the layers are organised as follow:

rpc.proto defines the GCT Unified API layer

Then every exchange implements the IBot interface provides access to each Exchange

the exchange wrapper layer is meant to transcode the data model between the unified API and the exchange peculiar API

Is that correct? I can see there are more details than what I listed, I am keen to understand the purpose of the wrapper layer at this moment. Cheers

The GCT unified API is derived from the IBotInterface (https://github.com/thrasher-corp/gocryptotrader/blob/master/exchanges/interfaces.go#L18). All exchanges have to implement these funcs which gives the engine (and bot) the ability to interact with each exchange through its wrapper funcs. Otherwise, for individual package usage (outside of the engine usage), the application can use any func inside of the exchanges package.

The gRPC server runs as a subsystem via engine, which allows client -> server interaction to cause the engine to perform any actions you'd like on those exchanges (submitting/cancelling orders, getting info etc. all done through the exchange wrappers)

thrasher- · 2020-01-15T22:58:18Z

exchanges/coinbene/coinbene.go

@@ -25,6 +26,10 @@ type Coinbene struct {
 	WebsocketConn *wshandler.WebsocketConnection
 }

+func (c *Coinbene) GetHistoricCandles(pair currency2.Pair, rangesize int, granularity int) ([]exchange.Candle, error) {


Please place this below the const vars to keep it consistent with all exchange packages

thrasher- · 2020-01-15T22:59:14Z

exchanges/lbank/lbank.go

@@ -10,11 +10,13 @@ import (
 	"encoding/pem"
 	"errors"
 	"fmt"
+	currency2 "github.com/thrasher-corp/gocryptotrader/currency"


Can just use currency instead of currency2

thrasher- · 2020-01-15T23:01:34Z

go.mod

@@ -3,27 +3,45 @@ module github.com/thrasher-corp/gocryptotrader
 go 1.12

 require (
+	cloud.google.com/go v0.51.0 // indirect
+	cloud.google.com/go/storage v1.5.0 // indirect
+	dmitri.shuralyov.com/gpu/mtl v0.0.0-20191203043605-d42048ed14fd // indirect


It looks like doing a go get golangci-lint has updated the go.mod/go.sum files, we make this optional and not a requirement as some people would like to compile the binary without the golangci files. The grpc/protobuffs newer versions are fine however

Sorry, I am not sure what you mean. I am a newbie in Go, just starting up. Do I need to change anything?

go modules (and the go.mod file) are go handles dependency management
these additions will cause go get to pull down these package

a common thing people new to go do is run "go get" on optional tools they want to install while inside a go module enabled project

such as

go get -u github.com/golangci/golangci-lint

running this while inside the gct directory will cause it to add golangci-lint and its dependencies to gct's list

easiest thing here would most likely be to revert them both back to master

git checkout master -- go.mod go.sum

gotcha, thanks
I'll fix it

gloriousCode

Thank you for your contribution Christian! I have a few extra requested changes.

exchanges/coinbasepro/coinbasepro.go

exchanges/coinbasepro/coinbasepro_wrapper.go

exchanges/coinbasepro/coinbasepro_test.go

xtda

Great work so far thanks for the PR :D

few minor change requests from me

xtda · 2020-01-15T23:51:31Z

cmd/gctcli/commands.go

+
+	var exchangeName string
+	if !c.IsSet("exchange") {
+		exchangeName = c.Args().Get(0)


this will not properly set exchangeName if the command flag --exchange is passed

There are a few ways of handling this two examples:
Setting it manually:

var exchangeName string if c.IsSet("exchange") { exchangeName = c.String("exchange") } else { exchangeName = c.Args().First() }

Adding it as a Destination field to command value

Flags: []cli.Flag{ cli.StringFlag{ Name: "start, s", Usage: "start date to search", Value: time.Now().Add(-time.Hour).Format(timeFormat), Destination: &startTime, }, } if !c.IsSet("start") { if c.Args().Get(0) != "" { startTime = c.Args().Get(0) } }

I am using the first option. The second option, using Destination, seems clean but I don't get it so not sure how to use it :)

The Destination flag works like using the Var versions of function from the default flag package where the destination is a pointer

Its useful if you have the same flag across multiple commands (such as exchange name)

you could do

var exchangeName string Destination: &exchangeName

xtda · 2020-01-15T23:52:17Z

cmd/gctcli/commands.go

+	}
+
+	var currencyPair string
+	if !c.IsSet("pair") {


Same as above wont set pair if --pair is passed in

xtda · 2020-01-15T23:52:36Z

cmd/gctcli/commands.go

+	p := currency.NewPairDelimiter(currencyPair, pairDelimiter)
+
+	var rangesize int
+	if !c.IsSet("rangesize") {


Same as above wont set rangesize if --rangesize is passed in

xtda · 2020-01-15T23:53:00Z

cmd/gctcli/commands.go

+	}
+
+	var granularity int
+	if !c.IsSet("granularity") {


Same as above wont set granularity if --granularity is passed in

xtda · 2020-01-15T23:55:15Z

cmd/gctcli/commands.go

+	if !c.IsSet("rangesize") {
+		if c.Args().Get(1) != "" {
+			rangesizestr, err := strconv.ParseInt(c.Args().Get(2), 10, 32)
+			if err == nil {


Also need to handle if the conversion does fail here or we could send invalid characters across
(also made me notice somewhere that i forgot to do this :D)

xtda · 2020-01-15T23:55:22Z

cmd/gctcli/commands.go

+	if !c.IsSet("granularity") {
+		if c.Args().Get(2) != "" {
+			granularitystr, err := strconv.ParseInt(c.Args().Get(3), 10, 32)
+			if err == nil {


Also need to handle if the conversion does fail here or we could send invalid characters across
(also made me notice somewhere that i forgot to do this :D)

xtda · 2020-01-16T00:03:52Z

engine/rpcserver.go

+		return nil, errors.New(errCurrencyPairUnset)
+	}
+
+	candles, err := GetExchangeByName(req.Exchange).GetHistoricCandles(currency.Pair{


GetExchangeByName() can return nil which might cause this to crash if an invalid exchange name is ever passed

gocryptotrader/engine/exchange.go

Lines 74 to 81 in 4e05ad4

func GetExchangeByName(exchName string) exchange.IBotExchange {

for x := range Bot.Exchanges {

if strings.EqualFold(Bot.Exchanges[x].GetName(), exchName) {

return Bot.Exchanges[x]

}

}

return nil

}

While this should never happen due to the name check in the cli its best to also handle it just incase

this actually happened to me: I was trying to call the RPC service from my application and didn't notice I was passing a not existent exchange name. I was getting a weird nil, took me a while to figure out what the issue was as GCT was crashing during unmarshalling and was not able to get to the actual point of failure using the debugger. Eventually,
I just noticed the exchange name was wrong and fixed it

Yeah i was considering at one point changing GetExchangeByName to return an error that the exchange wasn't found as well as pointer to IBotExchange

xtda

ignore the approval firefox and github don't play nice sometimes

shazbert

Awesome PR, thanks, just a couple of nits.

shazbert · 2020-01-16T00:03:56Z

exchanges/coinbene/coinbene.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	currency2 "github.com/thrasher-corp/gocryptotrader/currency"


There are no package collisions so currency2 can be dropped and called with just currency

shazbert · 2020-01-16T00:05:24Z

exchanges/btcmarkets/btcmarkets.go

@@ -82,6 +82,10 @@ type BTCMarkets struct {
 	WebsocketConn *wshandler.WebsocketConnection
 }

+func (b *BTCMarkets) GetHistoricCandles(pair currency.Pair, rangesize int, granularity int) ([]exchange.Candle, error) {


All functions need to have a comment due to linter issues.

This still needs to be addressed across the code base please.

shazbert · 2020-01-20T22:40:26Z

exchanges/coinbasepro/coinbasepro.go

@@ -162,19 +162,28 @@ func (c *CoinbasePro) GetTrades(currencyPair string) ([]Trade, error) {

 // GetHistoricRates returns historic rates for a product. Rates are returned in
 // grouped buckets based on requested granularity.
-func (c *CoinbasePro) GetHistoricRates(currencyPair string, start, end, granularity int64) ([]History, error) {
+func (c *CoinbasePro) GetHistoricRates(currencyPair string, start, end string, granularity int64) ([]History, error) {


You can drop the string after currencyPair now

shazbert · 2020-01-20T22:41:01Z

common/common.go

@@ -354,3 +355,24 @@ func SplitStringSliceByLimit(in []string, limit uint) [][]string {
 	}
 	return sliceSlice
 }
+
+// InArray checks if _val_ belongs to _array_
+func InArray(val interface{}, array interface{}) (exists bool, index int) {


This still needs a test function in common_test.go

shazbert

Thanks for those changes! There are only basic linter issues that I have found and I have highlighted each one so you don't miss it. Also a rebase/merge is needed due to our most recent commit.

exchanges/binance/binance.go

exchanges/bitfinex/bitfinex.go

exchanges/bitflyer/bitflyer.go

exchanges/bithumb/bithumb.go

exchanges/bitmex/bitmex.go

exchanges/okcoin/okcoin.go

exchanges/okex/okex.go

exchanges/poloniex/poloniex.go

exchanges/yobit/yobit.go

exchanges/zb/zb.go

Christian-Achilli

All reviewed

thrasher-

tACK, thanks for addressing those nits!

gctcli.exe gethistoriccandles --exchange=coinbasepro --pair=BTC-USD --rangesize=10 --granularity=86400
{
 "candle": [
  {
   "time": 1579824000,
   "low": 8267,
   "high": 8400,
   "open": 8385.21,
   "close": 8302.67,
   "volume": 1314.39939822
  },
  {
   "time": 1579737600,
   "low": 8278,
   "high": 8665,
   "open": 8660.38,
   "close": 8385.22,
   "volume": 12206.24491133
  },
  {
   "time": 1579651200,
   "low": 8565,
   "high": 8791.76,
   "open": 8722.03,
   "close": 8660.38,
   "volume": 5481.65770829
  },
  {
   "time": 1579564800,
   "low": 8465,
   "high": 8774.31,
   "open": 8628.89,
   "close": 8722.03,
   "volume": 7520.65103045
  },
  {
   "time": 1579478400,
   "low": 8504.06,
   "high": 8734.84,
   "open": 8697.53,
   "close": 8628.89,
   "volume": 5605.80561385
  },
  {
   "time": 1579392000,
   "low": 8465,
   "high": 9194.99,
   "open": 8908.95,
   "close": 8697.53,
   "volume": 15218.51476182
  },
  {
   "time": 1579305600,
   "low": 8800.1,
   "high": 8980,
   "open": 8899.42,
   "close": 8909.32,
   "volume": 5600.21827844
  },
  {
   "time": 1579219200,
   "low": 8665.67,
   "high": 9013,
   "open": 8714.77,
   "close": 8899.42,
   "volume": 15447.36512517
  },
  {
   "time": 1579132800,
   "low": 8580,
   "high": 8850,
   "open": 8808.81,
   "close": 8714.76,
   "volume": 9265.76423506
  },
  {
   "time": 1579046400,
   "low": 8550,
   "high": 8900,
   "open": 8812.58,
   "close": 8808.81,
   "volume": 14977.61979023
  }
 ]
}

shazbert

Well done! Tested approved.

gloriousCode

tACK! Nice work, thanks for making those changes 🎉

xtda

Tested approved as well with latest changes

…ose it as a wrapper func (thrasher-corp#414) * initial wiring to providegethistoricalcandles * initial wiring to providegethistoricalcandles * initial wiring to providegethistoricalcandles * gethistriccandles work from cli using hard coded inputs * gethistoriccandles RPC service and CLI working fine for coinbasepro * fixed unit test * input check on grpc for gethistoriccandles * updated deps * fixed the return value when a method is not yet implemented * code review: fixed CLI input check and int32->int64 * code review: handling wrong exchange name * added check on granularity and allowing start and end being empty * code review: removed currency2 * code review: dependency reverted * improved func comment * typo in func comment * get historic values tests * unit tests for get historical rates on coinbasepro * using time format time.RFC3339 * names to camel case and improved comments * test cleanup * changed to camel case * added InArray tests * dropped not needed string time * enforced use of int64 * fixed make check * cleaned up code organisation to be consistent * fixed Travis remarks * more Travis remarks * added comments * regenerated proto files after merge * linter fix

Christian-Achilli added 9 commits January 11, 2020 22:11

initial wiring to providegethistoricalcandles

d793fdf

initial wiring to providegethistoricalcandles

41b7608

initial wiring to providegethistoricalcandles

746c53d

gethistriccandles work from cli using hard coded inputs

3c9fe69

gethistoriccandles RPC service and CLI working fine for coinbasepro

5883639

fixed unit test

e7c9d61

Merge branch 'master' of https://github.com/thrasher-corp/gocryptotrader

6a5cdc6

into feature/history-trades

Merge branch 'master' of https://github.com/thrasher-corp/gocryptotrader

14e02fd

into feature/history-trades

input check on grpc for gethistoriccandles

9d108de

updated deps

2a975df

thrasher- changed the title ~~Feature/history trades~~ Add OHLC retrieval func (GetHistoricCandles) to all exchanges and expose it as a wrapper func Jan 15, 2020

thrasher- requested changes Jan 15, 2020

View reviewed changes

thrasher- requested review from shazbert, xtda, gloriousCode and MadCozBadd January 15, 2020 23:35

thrasher- added enhancement review me This pull request is ready for review labels Jan 15, 2020

gloriousCode requested changes Jan 16, 2020

View reviewed changes

exchanges/coinbasepro/coinbasepro.go Show resolved Hide resolved

exchanges/coinbasepro/coinbasepro_wrapper.go Outdated Show resolved Hide resolved

exchanges/coinbasepro/coinbasepro_test.go Outdated Show resolved Hide resolved

xtda approved these changes Jan 16, 2020

View reviewed changes

xtda suggested changes Jan 16, 2020

View reviewed changes

shazbert requested changes Jan 16, 2020

View reviewed changes

fixed the return value when a method is not yet implemented

0b096c7

shazbert added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Jan 17, 2020

Christian-Achilli added 4 commits January 18, 2020 23:01

code review: fixed CLI input check and int32->int64

c6b0557

Merge branch 'master' into feature/history-trades

b739376

code review: handling wrong exchange name

d716c9c

added check on granularity and allowing start and end being empty

641e45f

shazbert requested changes Jan 20, 2020

View reviewed changes

shazbert added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Jan 20, 2020

Christian-Achilli added 3 commits January 20, 2020 22:49

added InArray tests

9df3780

dropped not needed string time

28c239a

enforced use of int64

1f8f27c

Christian-Achilli requested a review from shazbert January 21, 2020 09:13

Christian-Achilli and others added 4 commits January 22, 2020 21:02

fixed make check

5511d84

cleaned up code organisation to be consistent

2348a17

fixed Travis remarks

8a4ee67

more Travis remarks

814df5e

shazbert added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Jan 23, 2020

shazbert requested changes Jan 23, 2020

View reviewed changes

added comments

8c29d11

thrasher- added reconstructing Based on PR feedback, this is currently being reworked and is not to be merged and removed review me This pull request is ready for review labels Jan 23, 2020

Christian-Achilli and others added 3 commits January 23, 2020 23:14

Merge branch 'master' into feature/history-trades

65cfa80

regenerated proto files after merge

e54b785

linter fix

0fdf729

Christian-Achilli commented Jan 23, 2020

View reviewed changes

thrasher- added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Jan 24, 2020

thrasher- approved these changes Jan 24, 2020

View reviewed changes

shazbert approved these changes Jan 24, 2020

View reviewed changes

gloriousCode approved these changes Jan 24, 2020

View reviewed changes

xtda approved these changes Jan 24, 2020

View reviewed changes

thrasher- merged commit 5ac5ec8 into thrasher-corp:master Jan 24, 2020

Christian-Achilli deleted the feature/history-trades branch January 24, 2020 15:26

	func GetExchangeByName(exchName string) exchange.IBotExchange {
	for x := range Bot.Exchanges {
	if strings.EqualFold(Bot.Exchanges[x].GetName(), exchName) {
	return Bot.Exchanges[x]
	}
	}
	return nil
	}

Add OHLC retrieval func (GetHistoricCandles) to all exchanges and expose it as a wrapper func #414

Add OHLC retrieval func (GetHistoricCandles) to all exchanges and expose it as a wrapper func #414

Conversation

Christian-Achilli commented Jan 14, 2020

PR Description

Type of change

How has this been tested

Checklist

codecov bot commented Jan 14, 2020 • edited

Codecov Report

thrasher- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

xtda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtda left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

Christian-Achilli left a comment

Choose a reason for hiding this comment

thrasher- left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

xtda left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2020 •

edited