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

[Binance] Support for execution reports / UserTrades / fills #246

Merged
merged 12 commits into from
Jan 29, 2019

Conversation

badgerwithagun
Copy link
Collaborator

This follows the same pattern as #160 did for GDAX. Execution reports are now fetched from Binance if you connect with an API key. That makes the following available:

L2 (Generic) API

StreamingMarketDataService.getTrades(...): now returns both UserTrade and Trade instances. As with the existing solution for GDAX, these are duplicated (see #230

L1 API

The following are available if you cast your StreamingMarketDataService to BinanceStreamingMarketDataService:

  • BinanceStreamingMarketDataService.getRawExecutionReports(): full raw details of all trades on all currencies, order creations and order fills. This is the raw Binance API (and is thus subject to change if the Binance API itself changes).
  • BinanceStreamingMarketDataService.getUserTrades(): Execution reports filtered down to just trades and converted to UserTrade.
  • BinanceStreamingMarketDataService.getUserTrades(CurrencyPair): As above, but filtered to a specific currency.

I think there's an argument for supporting the latter two as part of the generic API. Thoughts?

Keepalive not handled yet, some debug code still in place and the user trades currently duplicate non-user trades AND ignore currency filters.
@badgerwithagun
Copy link
Collaborator Author

I've been running with this for some time, with no problems.

Anyone got a review in them?

@badgerwithagun
Copy link
Collaborator Author

Still no reviews for this?

@badgerwithagun
Copy link
Collaborator Author

Anyone?

@badgerwithagun
Copy link
Collaborator Author

@Flemingjp, I want to get started on the next phase of this: unifying execution report support for the exchanges where we already provide it to create an L1 API. There's not much here that's controversial. Can we merge it on the basis of a simple code review?

@badgerwithagun
Copy link
Collaborator Author

@Flemingjp , I can't wait any longer and am now turning this into generic support across Binance, Bitfinex and Coinbase Pro, with more to come. That code is now ready to push but I don't want to complicate this PR it I don't have to; I'd rather submit the next bit as a separate PR.

If this isn't merged soon it's going to become another monster PR with useful features that never gets merged because it's too big for anyone to review, and we're going to be frozen regarding Binance in the same way as we are with Bitmex.

I am probably going to start looking at Bitmex next (#267, #244 and ensuring consistency with this), but I can't do anything about that either until this is merged.

I suggest either you and I make a decision on this now, or we call upon a couple of other regular contributors to agree on it, because if not, I can't afford to keep spending the time balancing two divergent code sets.

@badgerwithagun
Copy link
Collaborator Author

See #274 for details of the generic API I'm working on which builds on this.

@Flemingjp
Copy link
Collaborator

@badgerwithagun Apologies for the delay - are you still wanting this merged to help support your L2 api additions?

@badgerwithagun
Copy link
Collaborator Author

Yes please! Once this and the Bitfinex one are in, I'll be able to publicise my dev branch so I can get feedback on progress.

@Flemingjp Flemingjp merged commit b442730 into bitrich-info:develop Jan 29, 2019
@badgerwithagun
Copy link
Collaborator Author

Thanks @Flemingjp .

@badgerwithagun badgerwithagun deleted the binance-user-channel branch February 4, 2019 12:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants