Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Websocket order handling #446

Open
wants to merge 92 commits into
base: master
from

Conversation

@gloriousCode
Copy link
Collaborator

gloriousCode commented Feb 13, 2020

Websocket order handling

ORDER
Dramatic reenactment of websocket datahandler

This is an update focused on websocket order update data handling.

  • It adds or updates exchange support for order submission and modification responses
  • It synchronises those updates with the order manager
  • It seperates websocket message handling from the wsReadData function to allow for easy websocket testing
  • It adds testing for the majority websocket responses
  • It removes order.Detail stuttering with things like order.OrderStatus being changed to order.Status
  • It adds testing to a few engine packages such as routines and orders
  • It maintains compatibility with websocket order submission via the wrapper
  • Creates a fake exchange in the engine test package

Things to consider

  • I've not live tested each exchange with order updates. This testing is relying on exchange API documentation being accurate.... Which means there's a high chance that some response handling could fail. We'll need to test this in the future with real keys and real orders to ensure that it works

Type of change

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has this been tested

  • I've written over 400 unit tests. These tests cover websocket updates, websocket data handling, order manager and other parts of the engine package
  • I've run the application to ensure no errors or panics
  • go test ./... -race
  • golangci-lint

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • 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
  • Any dependent changes have been merged and published in downstream modules
@gloriousCode gloriousCode self-assigned this Feb 13, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #446 into master will increase coverage by 4.53%.
The diff coverage is 50.65%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #446      +/-   ##
=========================================
+ Coverage   40.97%   45.5%   +4.53%     
=========================================
  Files         209     210       +1     
  Lines       47267   48700    +1433     
=========================================
+ Hits        19366   22163    +2797     
+ Misses      26008   24278    -1730     
- Partials     1893    2259     +366
Impacted Files Coverage Δ
exchanges/poloniex/poloniex_websocket.go 53.84% <ø> (+41.63%) ⬆️
exchanges/kraken/kraken_websocket.go 52.03% <ø> (+52.03%) ⬆️
exchanges/poloniex/poloniex.go 38.51% <ø> (ø) ⬆️
exchanges/request/request.go 62.31% <ø> (+0.55%) ⬆️
exchanges/yobit/yobit_wrapper.go 39.33% <ø> (ø) ⬆️
exchanges/okex/okex_wrapper.go 34.93% <ø> (+0.44%) ⬆️
exchanges/zb/zb_websocket.go 25% <ø> (+23.12%) ⬆️
gctscript/wrappers/validator/validator.go 98.5% <ø> (-1.5%) ⬇️
exchanges/zb/zb_wrapper.go 39.49% <ø> (+0.28%) ⬆️
exchanges/lakebtc/lakebtc_wrapper.go 39.39% <ø> (ø) ⬆️
... and 92 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 b26ec86...39ad97c. Read the comment docs.

Copy link
Collaborator

thrasher- left a comment

Amazing PR, great to have tests for websocket functionality! I can see a lot of attention to detail went into this, so good work. Just a few nits on my first and untested pass through

One note is that we'll need to ensure that all exchanges have a one to one mapping of supported protocol features for websocket, since this PR adds a lot of new functionality

}
}

addPassingFakeExchange()

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

Without any check to see if the fake passing exchange has already been loaded, this will append to the bot.config every time this is called and error out on exchangManager.add() after it has initially been added, which currently isn't doing any error checking

}
}

func TestIsOnline(t *testing.T) {
t.Skip()

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

Regression with t.Skip

@@ -17,15 +18,53 @@ import (
var (
OrderManagerDelay = time.Second * 10
ErrOrdersAlreadyExists = errors.New("order already exists")
ErrOrderFourOhFour = errors.New("order does not exist")

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

I like this, but better to be more descriptive: ErrOrderDoesNotExist for those not familiar with HTTP status codes 😆

// Adds an order to the orderStore for tracking the lifecycle
// lock is toggle-able in the event that you have already used
// orderStore's lock. Most cases should be true
func (o *orderStore) Add(order *order.Detail, lock bool) error {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

Will be error prone to have lock passed in as an func arg, the lock should be controlled internally

"Order manager: Unable to generate UUID. Err: %s",
err)
}
order.InternalOrderID = id.String()

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

else { order.InternalOrderID = id.String() } otherwise empty assignment

o.wsProcessTickers(response)
case okGroupWsTrade:
o.wsProcessTrades(response)
func stringToOrderStatus(num string) order.Status {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

After reviewing a lot of these in this PR and considering the nature of this, it would be good to add test coverage for these conversion funcs

if err == nil && errorResponse.ErrorCode > 0 {
if o.Verbose {
log.Debugf(log.ExchangeSys,
"WS Error Event: %v Message: %v for %s",

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

To keep things consistent, please place the exchange name first like the following fmt.Errorf

}
if data, ok := result.([]interface{}); ok {
if len(data) == 0 {
return errors.New("c'mon man, no data")

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

😆 in this case, we can just return nil as the exchange didn't send us any meaningful data

p.Websocket.DataHandler <- response
case "n":
var timeParse time.Time
timeParse, err = time.Parse("2006-01-02 15:04:05", notification[6].(string))

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 14, 2020

Collaborator

Since this layout is used quite a lot, can make this a const similar to how we've done it in other changes for easy usage by other devs

testdata/http_mock/binance/binance.json Show resolved Hide resolved
Copy link
Collaborator

shazbert left a comment

@gloriousCode This is an amazing update to the websocket system and order management system. Well done! Some nits are out of scope. Let me know what you think.

}
}

func TestIsOnline(t *testing.T) {
t.Skip()

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

rm

go Bot.OrderManager.orderStore.existsWithLock(o)
go Bot.OrderManager.orderStore.Add(o2, true)
go Bot.OrderManager.orderStore.existsWithLock(o)
Comment on lines 143 to 145

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

should be interacting with the return values or do this at the start and have them signal a waitgroup and make sure they finish if your just testing contention.


err = Bot.OrderManager.Cancel(&order.Cancel{})
if err == nil {
t.Error("Expected error due to nil cancel")

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

nil cancel -> empty order

})
}
}
o.CancelAllOrders(nil)

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

I think if nil we might not want to cancel any open orders. I think it might be better if we pass in an exchange config variable bool cleanupOrdersOnClose just something easy. Maybe have that toggled by grpc as well. Doesn't need to be in this PR. Just something to consider, because I don't think we should cancel anything without somebody specifically interacting with it. Whats every ones thoughts?

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Feb 14, 2020

Author Collaborator

It's definitely a lazy thing by me. Sometimes I don't like adding parameters so I'll give arguments a different use, like nil here. I agree that cancelling all orders for every exchange should probably come with an explicit instruction to do so. Or the caller just has to provide a list of all enabled exchanges so the onus is always on the caller and not an accidental nil slice oopsie doopsie we lost all our orders

var found bool
for i := range exchangeNames {
if strings.EqualFold(k, exchangeNames[i]) {
found = true
}
}
if !found {
continue
}
Comment on lines 200 to 208

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator
if !common.StringDataCompareInsensitive(exchangeNames, k) {
continue
}
return order.PartiallyFilled
case "TRIGGER_ACTIVATED":
return order.Active
case "INSUFFICIENT_BALANCE", "MARKET_UNAVAILABLE":

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

I'm thinking these might be good as order.Status because we don't get a clear reason why we get rejected and that might be important. What do you think?

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Feb 20, 2020

Author Collaborator

I've added them as order status 👍

@@ -8,6 +8,7 @@ import (
"time"

"github.com/gorilla/websocket"

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

rm line

case "activate":
return order.Rejected
Comment on lines 234 to 235

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

from the docs:

An activate message is sent when a stop order is placed

Maybe we need to add an order.Status type stopOrderActive? Or some other type.

var trade WsTrade
trade.Symbol = currencyIDMap[channelID]
dataL3 := dataL2.([]interface{})
trade.TradeID, _ = strconv.ParseInt(dataL3[1].(string), 10, 64)

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

handle error

"time"

"github.com/gorilla/websocket"

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 14, 2020

Collaborator

rm line

Copy link
Member

xtda left a comment

not much from me at the moment bunch of other people commented them already

@@ -612,3 +604,95 @@ type WithdrawResponse struct {
Msg string `json:"msg"`
ID string `json:"id"`
}

type UserAccountStream struct {

This comment has been minimized.

Copy link
@xtda

xtda Feb 14, 2020

Member

Needs a comment

od.UpdateOrderFromDetail(d)
}
case *order.Cancel:
err := Bot.OrderManager.Cancel(d)

This comment has been minimized.

Copy link
@xtda

xtda Feb 14, 2020

Member

as nothing further is being done here can just return

This can be done for a few of the returns in this function if you prefere return early style since after the switch return is always nil

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Bump, can just return Bot.OrderManager.Cancel(d)

engine/routines.go Show resolved Hide resolved
@xtda

This comment has been minimized.

Copy link
Member

xtda commented Feb 14, 2020

Also I really like FakePassingExchange can be helpful in testing other parts but not a fan of it sitting in engine/ over exhanges/

@gloriousCode

This comment has been minimized.

Copy link
Collaborator Author

gloriousCode commented Feb 14, 2020

Also I really like FakePassingExchange can be helpful in testing other parts but not a fan of it sitting in engine/ over exhanges/

Yeah, its a bit messy, but didn't want it to be included in the main application. Engine package tests made sense to me since at that area, we don't really care for exchange responses there. Happy to hear feedback on another place. I was thinking to have it in its own test file in the exchange package (and then we could add a fakefailingexchange too)

}

for y := range v {
log.Debugf(log.OrderMgr, "order manager: Cancelling order ID %v [%v]",

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 21, 2020

Collaborator

upper case o in order just to be inline with the others

Pair: newOrder.Pair,
})
if err != nil {
return nil, fmt.Errorf("order manager: Unable to add %v order %v to orderStore: %s", newOrder.Exchange, result.OrderID, err)

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 21, 2020

Collaborator

can probably drop order manager here

switch oType {
case "exchange limit", "auction-only limit", "indication-of-interest limit":
return order.Limit, nil
// block trades are conducted off order-book, so their type is market, but would be considered a hidden trade

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 21, 2020

Collaborator

maybe put this on next line

amount, _ := strconv.ParseFloat(askData[i][1], 64)
price, _ := strconv.ParseFloat(askData[i][0], 64)
Comment on lines 300 to 301

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 21, 2020

Collaborator

out of scope: Might be better to check error and return because if we append a zero value onto this slice it could skew our data dramatically.

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Great catch, yeah we should always be checking errors on parsing floats for this context

amount, _ := strconv.ParseFloat(bidData[i][1], 64)
price, _ := strconv.ParseFloat(bidData[i][0], 64)
bids = append(bids, orderbook.Item{
Comment on lines 310 to 326

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 21, 2020

Collaborator

here as well

@@ -406,7 +552,7 @@ func (b *Bitmex) GenerateDefaultSubscriptions() {
}
}

channels := []string{bitmexWSOrderbookL2, bitmexWSTrade}
channels := []string{ /*bitmexWSOrderbookL2, */ bitmexWSTrade}

This comment has been minimized.

Copy link
@shazbert

shazbert Feb 21, 2020

Collaborator

Just check this is not an oversight

Copy link
Collaborator

thrasher- left a comment

Thank you for addressing those nits! On my 2nd untested pass through, I found some more. Will test it out in depth after these have been addressed


// GetByExchangeAndID returns a specific order
func (o *orderStore) GetByExchange(exchange string) ([]*order.Detail, error) {
o.m.Lock()

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

defer'ing after locking can be more simple, as all code execution paths will unlock the lock as soon as the func exits, similar to GetByExchangeAndID

}
exch := GetExchangeByName(order.Exchange)
if exch == nil {
return errors.New("unable to get exchange by name")

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Can use built in ErrExchangeNotFound

od.UpdateOrderFromDetail(d)
}
case *order.Cancel:
err := Bot.OrderManager.Cancel(d)

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Bump, can just return Bot.OrderManager.Cancel(d)

exchanges/binance/binance_test.go Show resolved Hide resolved
t := TickerStream{}
err := json.Unmarshal(multiStreamData.Data, &t)
}
p := currency.NewPairFromString(data.Symbol)

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

This will create a pair formatting issue, need to use currency.NewPairFromFormattedPairs()

} `json:"subscription"`
}

type WsOpenOrder struct {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Needs commenterino

Data []WebsocketOrderBook `json:"data"`
}

type WebsocketOrderBook struct {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Please add commenterino

if o.Verbose {
log.Error(log.ExchangeSys, errorMessage)
// wsReadData receives and passes on websocket messages for processing
func (o *OKGroup) WsReadData(wg *sync.WaitGroup) {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

WsReadData differs to wsReadData and will throw a linter err

return asset.PerpetualSwap
case asset.Index.String():
return asset.Index
func StringToOrderStatus(num string) (order.Status, error) {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Please add comment

w.Lock()
defer w.Unlock()

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 21, 2020

Collaborator

Achieves the same result, it's easier to know your lock will get unlocked if you put a defer straight after, otherwise you run into scenarios like this: https://github.com/thrasher-corp/gocryptotrader/pull/446/files#diff-c756b319860bc00c5b63bb5399dbb40bR554

Copy link
Member

xtda left a comment

:D Awesome work only two bits from me nothing major

// Untracked websocket orders will not have internalIDs yet
if order.InternalOrderID == "" {
id, err := uuid.NewV4()
if err != nil {

This comment has been minimized.

Copy link
@xtda

xtda Feb 23, 2020

Member

At this point order.InternalOrderID wont be set to anything is there ever a case where we would need to set these to some generic known id so they are not blank later?

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Feb 26, 2020

Author Collaborator

This one is weird because its dependent on an external library. It failing will impact many things, but I don't think it should stop the logging of an order. If it ends up erroring, I'd want it to still have a GUID later for consistency. But since GUID gen is erroring, I can't really put the same code in at a later stage since its basically kicking the can down the road. I'm not sure what to do here

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 26, 2020

Collaborator

We can add our own internal GUID generator in the case that this happens, even though it should theoretically be close to nil

This comment has been minimized.

Copy link
@xtda

xtda Feb 26, 2020

Member

Yeah not sure on a clean fix

I was more just thinking if an order is added with no internal id due to uuid in the very rare case erroring GetByInternalOrderID() wont work

So in that case is it worth not adding the order? how important is it that intenralorderid is filled out correctly?

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Feb 26, 2020

Author Collaborator

Right now, its actually not very important. The order manager is not storing orders in the database, so when you reload the application, you would end up with the same order having a different GUID. So I think our own GUID handling as well as order storage will be in a future PR

@@ -89,7 +89,7 @@ func (e Exchange) QueryOrder(exch, orderID string) (*order.Detail, error) {

// SubmitOrder submit new order on exchange
func (e Exchange) SubmitOrder(exch string, submit *order.Submit) (*order.SubmitResponse, error) {

This comment has been minimized.

Copy link
@xtda

xtda Feb 23, 2020

Member

exch is unused here now and can be removed from the interface & implementations

@gloriousCode gloriousCode force-pushed the gloriousCode:broader-order-recorder branch from 7baf836 to fa492e8 Feb 24, 2020
AssetType: asset.Spot,
Pair: p,
}
case fundChange:

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 24, 2020

Collaborator

It would be nice to add tests for fundChange and orderChange

@@ -500,3 +503,14 @@ func TestGetActiveOrders(t *testing.T) {
t.Fatal(err)
}
}

func TestWsOrderNotification(t *testing.T) {

This comment has been minimized.

Copy link
@thrasher-

thrasher- Feb 24, 2020

Collaborator

Please add ws test coverage for heatrbeat, ticker, orderbook, trade plus the already existing orderChange / fundChange request

Copy link

codelingo bot left a comment

2 issues found. Ignoring 0 issues.

return false
}

// Adds an order to the orderStore for tracking the lifecycle

This comment has been minimized.

Copy link
@codelingo

codelingo bot Feb 24, 2020

Every exported function in a program should have a doc comment. The first sentence should be a summary that starts with the name (Add) being declared.
From effective go.

return nil, ErrOrderNotFound
}

// GetByExchangeAndID returns a specific order

This comment has been minimized.

Copy link
@codelingo

codelingo bot Feb 24, 2020

Every exported function in a program should have a doc comment. The first sentence should be a summary that starts with the name (GetByExchange) being declared.
From effective go.

@thrasher- thrasher- added reconstructing and removed review me labels Feb 24, 2020
@gloriousCode gloriousCode force-pushed the gloriousCode:broader-order-recorder branch from 0c73b28 to 512590d Feb 26, 2020
@thrasher-

This comment has been minimized.

Copy link
Collaborator

thrasher- commented Feb 26, 2020

Please merge/rebase

gloriousCode added 3 commits Jan 5, 2020
…pointer struct. Adds case to ws routines
…er in routines.go
…rs. fixes ordersides
gloriousCode added 20 commits Feb 13, 2020
…mes DisplayQty to DisplayQuantity. Removes verbose. Adds some websocket properties. Updates bitmex asset type in test
…kline switch cases. Updates some websocket capabilities.
…pareInsensitive
…if found, processes via wrapper method, else, goes through wsHandleData method. Removes weird locking system from wrapper/websocket data race. Updates bitfinex to properly handle websocket order requests and notifications
…mentations for trades. Botches a new error type
…eparately
…version. Even in order trades and some wrapper functions
…inance and bitfinex issues with auth endpoint use, map casting. Expands more order.ClassificationError usage. Fixes some more generic websocket response errors
…g to accomodate. Adds shiny new time type. Expands wrapper websocket functionality definitions
… currency conversion
…k to use update instead
@gloriousCode gloriousCode force-pushed the gloriousCode:broader-order-recorder branch from 6fb996d to fd072d7 Feb 26, 2020
…e a currency pair and asset type based on a string. It will iterate over enabled pairs and compare them to formatted pairs and then return that pair if found.
…ly bad if it wasn't there
Copy link

codelingo bot left a comment

LGTM

Copy link

codelingo bot left a comment

LGTM

Copy link

codelingo bot left a comment

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.