Skip to content
This repository has been archived by the owner on Jun 8, 2020. It is now read-only.

[BitMex] Authenticated Channels #267

Closed
wants to merge 27 commits into from
Closed

[BitMex] Authenticated Channels #267

wants to merge 27 commits into from

Conversation

mdvx
Copy link
Contributor

@mdvx mdvx commented Jan 12, 2019

Bitmex use ExchangeSpecification to select Sandbox, ApiKey and ApiSecret

expose getStreamingService() for Bitmex and CoinbasePro

Create to resubscribe channels - this is also called manually after the socket connects (would be nice to do that automatically)
@OverRide // called by NettyStreamingService.resubscribeChannels
public String BitmexStreamingService.getAuthenticateMessage() throws IOException;

this gives access to the authenticated channels

Copy link
Collaborator

@badgerwithagun badgerwithagun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, all done from me. One question: what effect does this have on the content of the streams? Does it affect the normal market data channels (e.g. adding more information to trades), or does it make more channels available, which you catch using exchange-specific code?

The reason that I ask is because we have other PRs open (e.g. #246, #244) which overlap this heavily and at the moment only provide authenticated data through exchange-specific APIs. It may be the right time to start extending the core API to include authenticated information (such as execution reports).

pom.xml Outdated
@@ -5,7 +5,7 @@
<groupId>info.bitrich.xchange-stream</groupId>
<artifactId>xchange-stream-parent</artifactId>
<packaging>pom</packaging>
<version>4.3.13-SNAPSHOT</version>
<version>4.3.14-SNAPSHOT</version>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mdvx! Thanks for contributing, and welcome. A few review comments from me. Firstly, could you remove the version updates? They make the merge harder - we already have #268 lined up to do this next version update.

Copy link
Contributor Author

@mdvx mdvx Jan 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it affect the normal market data channels (e.g. adding more information to trades), or does it make more channels available

It make more channels available, beyond orderbook and klines, I can now subscribe to position, wallet, order and execution channels

My goal is to provide all data required for trading as streams, for every exchange that can do it.

Another avenue I have been looking at is Manifold, but I would rather we have a proper supported api (I have several streaming exchanges in the works)

Please feel free to take what you want can from this PR (I am not a git guru)

pom.xml Outdated
@@ -80,7 +80,7 @@
</repositories>

<properties>
<xchange.version>4.3.13</xchange.version>
<xchange.version>4.3.14-SNAPSHOT</xchange.version>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change this to 4.3.14? It's been released now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

@@ -65,4 +70,8 @@ public boolean isAlive() {

@Override
public void useCompressedMessages(boolean compressedMessages) { streamingService.useCompressedMessages(compressedMessages); }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than exposing the streaming service directly, perhaps it might be cleaner to add some specialised methods to BitfinexStreamingExchange to expose what you need as proper streams? I have an example in my (still unmerged PR for Binance: #246

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree, I wanted to open this up for discussuion

protected void handleMessage(JsonNode message) {
// if (message.has("info") && message.get("info").asText().startsWith("Welcome ")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this commented-out code be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes


@Override // called by NettyStreamingService.resubscribeChannels
public String getAuthenticateMessage() throws IOException {
// connect().blockingAwait();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code

new BitmexWebSocketSubscriptionMessage("authKeyExpires",
new Object[]{exchangeSpecification.getApiKey(), nonce, digestString});

//sendMessage( );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code


return objectMapper.writeValueAsString(subscribeMessage);

//streamingService.sendAuthKeyExpires(new Object[]{apiKey, nonce, digestString});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code


BitmexWebSocketSubscriptionMessage subscribeMessage =
new BitmexWebSocketSubscriptionMessage("authKeyExpires",
new Object[]{exchangeSpecification.getApiKey(), nonce, digestString});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you just use new String[] here rather than new Object[], and avoid the change to BitmexWebSocketSubscriptionMessage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i first iteration passed a digest object, but a string is suffecient now

@badgerwithagun
Copy link
Collaborator

Note: merge #268 first

@Flemingjp Flemingjp changed the title Bitmex Authenticated Channels [BitMex] Authenticated Channels Jan 14, 2019
@Flemingjp
Copy link
Collaborator

Flemingjp commented Jan 14, 2019

#268 merged. @mdvx any comments on @badgerwithagun comments?

@mdvx
Copy link
Contributor Author

mdvx commented Jan 17, 2019

I am a little stuck here, I am not sure how to pull the PR down onto a clean checkout of develop branch, so I can make the requested changes.

Are there some general instructions somewhere?

I am using git (normal, tortoise, InteliJ) on windows.

@badgerwithagun
Copy link
Collaborator

badgerwithagun commented Jan 18, 2019

@mdvx : I'm not an IntelliJ user myself, but the sequence of actions you need is:

  1. git fetch upstream - this updates your local version of the bitrich-info github change history, assuming your remote is called upstream. This is the default, but it might not be called that on your machine.
  2. git merge upstream/develop - this merges the changes in the bitrich-info github repo into the currently checked out branch in your local repo.

You can then push your changes up to your github repo.

More here: https://help.github.com/articles/syncing-a-fork/

@badgerwithagun
Copy link
Collaborator

See #274 - if this PR can be brought in line with the approach detailed there (similar to #246 and #273) that would be great. If not, I'll pick this up and unify it with #244 when I've finished work on Binance, Coinbase Pro and Bitfinex.

mdvx added 4 commits January 30, 2019 20:03
 into develop

# Conflicts:
#	pom.xml
#	service-core/pom.xml
#	service-netty/pom.xml
#	service-netty/src/main/java/info/bitrich/xchangestream/service/netty/NettyStreamingService.java
#	service-pubnub/pom.xml
#	service-pusher/pom.xml
#	service-wamp/pom.xml
#	xchange-binance/pom.xml
#	xchange-bitfinex/pom.xml
#	xchange-bitflyer/pom.xml
#	xchange-bitmex/pom.xml
#	xchange-bitstamp/pom.xml
#	xchange-cexio/pom.xml
#	xchange-coinbasepro/pom.xml
#	xchange-coinmate/pom.xml
#	xchange-gemini/pom.xml
#	xchange-hitbtc/pom.xml
#	xchange-okcoin/pom.xml
#	xchange-poloniex/pom.xml
#	xchange-poloniex2/pom.xml
#	xchange-stream-core/pom.xml
#	xchange-wex/pom.xml
@badgerwithagun badgerwithagun added the bitmex_delayed There is a backlog of BitMex issues due to a lack of reviewers. label Jun 16, 2019
@mdvx
Copy link
Contributor Author

mdvx commented Oct 18, 2019

will return to BitMEX, using trunc version

@mdvx mdvx closed this Oct 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bitmex_delayed There is a backlog of BitMex issues due to a lack of reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants