-
Notifications
You must be signed in to change notification settings - Fork 902
exchanges: Update Poloniex exchange API #2105
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
base: master
Are you sure you want to change the base?
Conversation
… into update_tif
… into update_tif
… into update_tif
… into update_tif
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, one issue left over.
exchanges/poloniex/poloniex_test.go
Outdated
| require.NoError(t, err) | ||
| assert.NotNil(t, result) | ||
|
|
||
| result, err = e.GetOrderbook(t.Context(), spotTradablePair, .1, 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scale is wrong when running live:
Poloniex attempt 1 request path: https://api.poloniex.com/markets/ROCKTRON_USDT/orderBook?limit=100&scale=0.1
Poloniex request type: GET
--- FAIL: TestGetOrderbook (0.43s)
poloniex_test.go:997:
Error Trace: C:/Users/graem/ryan/thrasher-corp/gocryptotrader/exchanges/poloniex/poloniex_test.go:997
Error: Received unexpected error:
Poloniex unsuccessful HTTP status code: 400 raw response: {"code": 24106,"message": "Invalid market depth!"}
Test: TestGetOrderbook
Lock it to BTC then it might work in your favour. Or use 0.0001.... value for scale.
Also when you do these changes please make sure you run it live.
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷
exchanges/poloniex/poloniex_test.go
Outdated
| assert.NotNil(t, result) | ||
|
|
||
| result, err = e.GetOrderbook(t.Context(), spotTradablePair, .1, 100) | ||
| result, err = e.GetOrderbook(t.Context(), spotPair, .0001, 100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't test it live 😭
--- FAIL: TestGetOrderbook (1.26s)
poloniex_test.go:999:
Error Trace: C:/Users/ohara/thrasher-corp/gocryptotrader/exchanges/poloniex/poloniex_test.go:999
Error: Received unexpected error:
Poloniex unsuccessful HTTP status code: 400 raw response: {"code": 24106,"message": "Invalid market depth!"}
Test: TestGetOrderbook
cranktakular
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This week's batch of changes I must request.
exchanges/poloniex/poloniex_test.go
Outdated
| err = e.wsHandleData(t.Context(), pressXToJSON) | ||
| if err != nil { | ||
| t.Error(err) | ||
| func TestGetCurrentOrders(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reiterating this request for change.
| poloniexOrderMove, | ||
| values, | ||
| &result) | ||
| resp, err := SendBatchValidatedAuthenticatedHTTPRequest[*OrderIDResponse](ctx, e, exchange.RestSpot, sBatchOrderEPL, http.MethodPost, "/orders/batch", nil, args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was thumbs-upped, but I'm not getting coverage for this line.
| request.Auth: request.NewRateLimitWithWeight(poloniexRateInterval, poloniexAuthRate, 1), | ||
| request.UnAuth: request.NewRateLimitWithWeight(poloniexRateInterval, poloniexUnauthRate, 1), | ||
| } | ||
| var rateLimits = request.RateLimitDefinitions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vast majority of other exchanges generate this through a GetRateLimits() function. Three others assign it to a global variable like this, while eleven use a generator function. So I think you should replace it in line with the majority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sir, adding another wrapper -- function -- in this case has no semantic or performance advantage. The reason why I did this was also because of someone's comment I don't remember who he was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was me, if we have multiple Exchange instances then the rate limits need to be static and shareable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welp, this minimal implementation can stay for now, but I would like to toss out some ideas for the future.
I wonder if we could retain that static-ness and sharability, without needing to allocate a massive map and to make dozens of function calls even when the exchange isn't used.
Perhaps the global variable could simply be an empty map, with the function being able to check whether it's been allocated, and allocating it once if it hasn't been. Although, we might then need a lock to avoid race conditions....
| Total: bal.AvailableBalance.Float64(), | ||
| Hold: bal.Hold.Float64(), | ||
| Free: bal.Available.Float64(), | ||
| AvailableWithoutBorrow: bal.AvailableBalance.Float64(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is handling the balances properly.
According to the documentation, Available represents the free balance for spot, while AvailableBalance represents that for futures, so you may need to figure out whether the account is futures or spot, and set the accounts.Balance fields based on that.
Also, I think the current implementation may be neglecting to report on the borrowed amount; I'd think positionMargin + orderMargin would constitute that. And given how there's a distinction between "Free" and "AvailableWithoutBorrow", I'd think that "Free" would have to include any margin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am using a single endpoint for this wrapper, and the documentation says nothing about the price toggling for the two account types. May be, I should look at other endpoints alternatives to re-implement this feature. I can/am look into it again if this is assumed to be vital.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's one endpoint, but as the documentation says, it returns different fields based on whether the account is spot or futures.
This seems, to me, like it is worth getting right.
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing from me then looks good to go! nice work!
| // UpdateTicker updates and returns the ticker for a currency pair | ||
| func (e *Exchange) UpdateTicker(context.Context, currency.Pair, asset.Item) (*ticker.Price, error) { | ||
| return nil, common.ErrFunctionNotSupported | ||
| func (e *Exchange) UpdateTicker(ctx context.Context, pair currency.Pair, assetType asset.Item) (*ticker.Price, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to process these tickers
if err := ticker.ProcessTicker(tickerData); err != nil {
return nil, err
}
return ticker.GetTicker(e.Name, fPair, a)
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing more from me thanks!
shazbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THings look good, live tests need to be fixed.
| case "subscribe", "unsubscribe", "error": | ||
| return conn.RequireMatchWithData("subscription", respRaw) | ||
| default: | ||
| return e.Websocket.DataHandler.Send(ctx, websocket.UnhandledMessageWarning{Message: e.Name + websocket.UnhandledMessage + string(respRaw)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: You can do what have done below (return an error) and it will be sent. Reducing a bit of code.
|
|
||
| e.setAPICredential(apiKey, apiSecret) | ||
|
|
||
| e.Websocket.DataHandler = sharedtestvalues.GetWebsocketInterfaceChannelOverride() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.\poloniex_live_test.go:29:28: cannot use sharedtestvalues.GetWebsocketInterfaceChannelOverride() (value of type chan any) as *stream.Relay value in assignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixing this will fix the mess I created on all live tests instances with this recent PR merge. ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding on specific code: I changed this line to:
e.Websocket.DataHandler = stream.NewRelay(10000)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look look soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okeh. You can update /internal/testing/exchange/exchange.go SetupWs and add w.DataHandler = stream.NewRelay(100000) and it resolved it for me
@shazbert may have a differing opinion so I shall tag him
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have the Poloniex: channel buffer is full: failed to relay <*fmt.wrapError> connection URL:[wss://ws.poloniex.com/ws/public] error: channel buffer is full: failed to relay <[]ticker.Price> on the live tests. but not as big as when we had 10000 buffer size.
gloriousCode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this. I think additional things can be bugfixes/additional PRs. This updates the API, adds extensive coverage, supports new features developed while this PR was opened and meets all my expectations while testing.
Just fix up the live testing bug @shazbert mentioned and maybe remove the lock for websocket spot trade requests (see here)

This is an extension of a PR #1351 (exchanges: Update Poloniex exchange API)
This pull request included a Poloniex exchange code implementation with REST and WebSocket capabilities. The Wrapper and Web-socket functions have all undergone testing. My code conforms to the GCT style standards used by other exchanges.
golangci-lint was used to fix a few mistakes. For future use, endpoint methods that the GCT wrapper does not support are also implemented.
The PR included implementations for REST for trading and other endpoint methods, Web-socket streaming, and Trading through the web socket stream.
Requires #1968
Fixes # (issue)
Type of change
Please delete options that are not relevant and add an
xin[]as the item is complete.How has this been tested
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 and
also consider improving test coverage whilst working on a certain feature or package.
Checklist