Add LBank exchange support #327
Conversation
exchanges/lbank/README.md
Outdated
|
|
||
| ## Notes | ||
|
|
||
| + Please add notes here with any production issues |
There was a problem hiding this comment.
Please add notes here with any production issues
There was a problem hiding this comment.
what do you mean?
There was a problem hiding this comment.
For example; Websocket implementation not completed.
Codecov Report
@@ Coverage Diff @@
## master #327 +/- ##
==========================================
- Coverage 42.8% 42.24% -0.56%
==========================================
Files 124 126 +2
Lines 29410 30138 +728
==========================================
+ Hits 12588 12733 +145
- Misses 15873 16424 +551
- Partials 949 981 +32
Continue to review full report at Codecov.
|
![codelingo[bot]](https://avatars.githubusercontent.com/in/14339?s=60&v=4){kind=small}
exchanges/lbank/lbank.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (l *Lbank) sign(data string, p *rsa.PrivateKey) (string, error) { |
There was a problem hiding this comment.
As sign is a method reciver for Lbank it already has access to l.privateKey and it does not need to be passed and can just be accessed via l.privateKey below
exchanges/lbank/lbank.go
Outdated
| vals = url.Values{} | ||
| } | ||
|
|
||
| err := l.loadPrivKey() |
There was a problem hiding this comment.
loadPrivKey() can be moved to Setup() instead of SendAuthHTTPRequest() as it only needs to be set once on Setup after SetAPIKeys() you could then most likely drop the mutex lock on it
| @@ -445,3 +445,16 @@ func TestGetDepositAddress(t *testing.T) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func TestUpdateOrderbook(t *testing.T) { | |||
There was a problem hiding this comment.
Every exported function in a program should have a doc comment. The first sentence should be a summary that starts with the name (TestUpdateOrderbook) being declared.
From effective go.
exchanges/lbank/lbank_test.go
Outdated
| @@ -46,6 +41,10 @@ func TestSetup(t *testing.T) { | |||
| lbankConfig.APISecret = testAPISecret | |||
| lbankConfig.APIKey = testAPIKey | |||
| l.Setup(&lbankConfig) | |||
| if setupRan { | |||
There was a problem hiding this comment.
Good work so far a few small things that need addressing
I have not been able to complete full testing of all the auth api still waiting for a key from lbank once I get it i will review the rest as there appears to be a few other differences between what they return and how its handled
exchanges/lbank/lbank_wrapper.go
Outdated
| for a := range getOrdersRequest.Currencies { | ||
| p := exchange.FormatExchangeCurrency(l.Name, getOrdersRequest.Currencies[a]) | ||
| b := int64(1) | ||
| tempResp, err := l.QueryOrderHistory(p.String(), strconv.FormatInt(b, 10), "200") |
There was a problem hiding this comment.
b is not incremented anywhere else here and will always be 1
exchanges/lbank/lbank_wrapper.go
Outdated
| } | ||
| } | ||
| } | ||
| return resp, nil |
There was a problem hiding this comment.
resp is an empty slice still at this point
exchanges/lbank/lbank_wrapper.go
Outdated
| return resp, err | ||
| } | ||
| tempData := tempResp.PageLength | ||
| for tempData == 200 { |
There was a problem hiding this comment.
If tempData is 200 this will cause an infinite loop as it is not updated anywhere else
exchanges/lbank/lbank.go
Outdated
| } | ||
|
|
||
| // GetOpenOrders gets opening orders | ||
| func (l *Lbank) GetOpenOrders(pair string, pageNumber, pageLength int64) (OpenOrderResponse, error) { |
There was a problem hiding this comment.
There is no consistency with your data types in some places pageNumber & pageLength are strings in others they are int64
Also going by the lbank docks pageLength can only ever be 200 max which uint8 is big enough to hold instead of int64
exchanges/lbank/lbank_wrapper.go
Outdated
| return resp, err | ||
| } | ||
| tempData := tempResp.PageLength | ||
| for tempData == 200 { |
There was a problem hiding this comment.
This will cause an infinite loop if tempData is 200
exchanges/lbank/lbank_wrapper.go
Outdated
|
|
||
| for c := int64(0); c < tempData; c++ { | ||
| resp[p.String()] = append(resp[p.String()], totalOrders[c].OrderID) | ||
| b++ |
There was a problem hiding this comment.
after this has finished its execution b will be 201 it looks like you are trying to increase it by 1 after each iteration instead
exchanges/lbank/lbank_wrapper.go
Outdated
| return resp, errors.New("openorderresponse received is empty") | ||
| } | ||
|
|
||
| for c := int64(0); c < tempData; c++ { |
There was a problem hiding this comment.
if tempData is changed to a uint8 the c here will also need to be updated
exchanges/lbank/lbank_types.go
Outdated
|
|
||
| // OpenOrderResponse stores information about the opening orders | ||
| type OpenOrderResponse struct { | ||
| PageLength int64 `json:"page_length"` |
There was a problem hiding this comment.
Based on the lbank docs this will only ever be a max of 200 int64 is overkill here it will fit into a uint8
exchanges/lbank/lbank_types.go
Outdated
| type OrderHistory struct { | ||
| Result bool `json:"result,string"` | ||
| Total string `json:"total"` | ||
| PageLength int64 `json:"page_length"` |
There was a problem hiding this comment.
Based on the lbank docs this will only ever be a max of 200 int64 is overkill here it will fit into a uint8
exchanges/lbank/lbank_types.go
Outdated
| type OrderHistoryResponse struct { | ||
| Result bool `json:"result,string"` | ||
| Total string `json:"total"` | ||
| PageLength int64 `json:"page_length"` |
There was a problem hiding this comment.
Based on the lbank docs this will only ever be a max of 200 int64 is overkill here it will fit into a uint8
exchanges/lbank/lbank.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| return json.Unmarshal(intermediary, result) |
There was a problem hiding this comment.
By this point (and the same with SendAuthHTTPRequest) we have unmarshaled the data 3 times
It might be better to move ErrCapture as an anonymous struct with omitempty (that way if its not there it wont try unmarshal and the values will default to 0/false)
type TickerResponse struct {
ErrCapture `json:",omitempty"`
Symbol string `json:"symbol"`
Timestamp int64 `json:"timestamp"`
Ticker Ticker `json:"ticker"`
}
func ErrorCapture(code int) error {
msg, ok := errorCodes[code]
if !ok {
return fmt.Errorf("undefined code please check api docs for error code definition: %v", code)
}
return errors.New(msg)
}
func (l *Lbank) GetTicker(symbol string) (TickerResponse, error) {
var t TickerResponse
params := url.Values{}
params.Set("symbol", symbol)
path := fmt.Sprintf("%s/v%s/%s?%s", l.APIUrl, lbankAPIVersion, lbankTicker, params.Encode())
err := l.SendHTTPRequest(path, &t)
if err != nil {
return t, err
}
if t.Error != 0 {
return t, ErrorCapture(t.Error)
}
return t, nil
}
func (l *Lbank) SendHTTPRequest(path string, result interface{}) error {
return l.SendPayload(http.MethodGet, path, nil, nil, &result, false, false, l.Verbose, l.HTTPDebugging)
}it does add additional information to the struct (int/bool) but at the trade off of not having to do expensive unmarshaling multiple times
|
failed to parse codelingo.yaml file |
| } | ||
|
|
||
| // InfoResponse stores info | ||
| type InfoResponse struct { |
There was a problem hiding this comment.
This struct is incorrect based on the data lbank return
The format is:
{
"result": "true",
"info": {
"freeze": {
"currency": "value"
},
"asset": {
"currency": "value"
},
"free": {
"currency": "value"
}
}
}which would be
struct {
string
struct {
map[string]string
map[string]string
map[string]string
}
}| return resp, ErrorCapture(resp.Error) | ||
| } | ||
|
|
||
| return resp, nil |
There was a problem hiding this comment.
Always returns empty data due to invalid unmarshaling
(See feedback on InfoResponse struct)
| // Lbank exchange | ||
| func (l *Lbank) GetAccountInfo() (exchange.AccountInfo, error) { | ||
| var info exchange.AccountInfo | ||
| data, err := l.GetUserInfo() |
There was a problem hiding this comment.
With the changes made to GetUserInfo this will also need to be updated
exchanges/lbank/lbank.go
Outdated
| } else { | ||
| tempResp.ClosePrice = resp2[x].(float64) | ||
| } | ||
| case 4: |
There was a problem hiding this comment.
Missing the 6th value from kline response
Also this function can be simplified a lot more if we declare klineTemp as a slice of interface it will save the conversion on line 184
Along with the fact that SendHTTPRequest will unmarshal the numbers to floats we can use type assertion
It would end up looking something like:
for x := range temp {
tt := temp[x].([]interface{})
tempHolder := KlineResponseDataFixed{
TimeStamp: int64(tt[0].(float64)),
OpenPrice: tt[1].(float64),
HigestPrice: tt[2].(float64),
LowestPrice: tt[3].(float64),
ClosePrice: tt[4].(float64),
TradingVolume: tt[5].(float64),
}
k = append(k, tempHolder)
}[DEBUG]: 2019/08/07 11:58:44 Lbank exchange raw response: [[1521006900,9000.00000000,9000.00000000,9000.00000000,9000.00000000,0.01000000],[1521006960,9000.00000000,9000.00000000,9000.00000000,9000.00000000,0E-8],[1521007020,9000.00000000,9000.00000000,9000.00000000,9000.00000000,0E-8],[1521007080,9000.00000000,9000.00000000,9000.00000000,9000.00000000,0E-8],[1521007140,9000.00000000,9000.00000000,9000.00000000,9000.00000000,0E-8]]
--- PASS: TestGetKlinesFixed (1.14s)
lbank_test.go:121: [{1521006900 9000 9000 9000 9000 0.01} {1521006960 9000 9000 9000 9000 0} {1521007020 9000 9000 9000 9000 0} {1521007080 9000 9000 9000 9000 0} {1521007140 9000 9000 9000 9000 0}]
PASS
exchanges/lbank/lbank.go
Outdated
| } | ||
|
|
||
| var order OrderResponse | ||
| err = json.Unmarshal(resp.Orders, &order) |
There was a problem hiding this comment.
What is the reason for the secondary unmarshal here?
lbank have a nice feature where they return different data for Orders depending on if its empty or not it can swap between
{
"orders": ""
}and
{
"orders": {}
}Since you already have Orders as a json.RawMessage you have the ability to delay unmarshaling until you are ready
var rt OrderHistoryResponse
rt.CurrentPage = resp.CurrentPage
rt.PageLength = resp.PageLength
rt.Total = resp.Total
if len(resp.Orders) == 2 {
return rt, nil
}
err = json.Unmarshal(resp.Orders, &rt.Orders)
if err != nil {
return OrderHistoryResponse{}, err
}This should in theory catch all possible returns that their API say are possible
There was a problem hiding this comment.
Epic work @MadCozBadd and congrats on your first exchange addition to GCT 💯! Just a few nits here:
The last remaining one is to add Lbank to the exchange support table: https://github.com/thrasher-corp/gocryptotrader/blob/master/tools/documentation/root_templates/root_readme.tmpl
You can then run the tool which wil update the root README.md file.
There was a problem hiding this comment.
One last change before this can be merged in:
Please update the GCT root readme template to include LBank support. You can use the documentation tool after updating https://github.com/thrasher-corp/gocryptotrader/blob/master/tools/documentation/root_templates/root_readme.tmpl which will in turn update https://github.com/thrasher-corp/gocryptotrader/blob/master/README.md
| | frankzougc | https://github.com/frankzougc | 1 | | ||
| | starit | https://github.com/starit | 1 | | ||
| | Jimexist | https://github.com/Jimexist | 1 | | ||
| | lookfirst | https://github.com/lookfirst | 1 | | ||
| | zeldrinn | https://github.com/zeldrinn | 1 | |
There was a problem hiding this comment.
Please preserve these contributors
Description
Added go and wrapper functions for Lbank exchange, however websocket stuff hasn't been completed yet.
Fixes # (issue)
Type of change
Please delete options that are not relevant and add an
xin[]as item is complete.How Has This Been Tested?
Tested using "go test ./... race" to see if there is any issues, but all have been resolved.
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Please also consider improving test coverage whilst working on a certain package
Checklist: