[GDAX] UserTrades and Trades duplicate each other when using authenticated stream

[badgerwithagun]

As discussed in review of #160, if a user trade executes and the stream is authenticated, we return a UserTrade for that trade (from the authenticated user channel), but we also return the public Trade (from the public fills channel).

Do we consider this a bug? The only way I can think of to de-duplicate the two, giving preference to the UserTrade, is to delay the fills stream by just enough that we can assume that the UserTrade will arrive first, then skip any IDs we've seen recently.

Example implementation of GDAXStreamingMarketDataService.getTrades():

// To avoid duplication of user and public trades, we delay the public trades by one second
// and skip any trade ids which have recently been streamed as user trades. This isn't
// perfect, of course...
Cache<String, ? super Trade> recentTrades = CacheBuilder.newBuilder().maximumSize(50).build();
return publicTrades
  .concatMap(t -> Observable.just(t).delay(1, TimeUnit.SECONDS))
  .skipWhile(t -> recentTrades.asMap().containsKey(t.getId()))
  .mergeWith(
    getUserTrades()
      .filter(t -> t.getCurrencyPair().equals(currencyPair))
      .doOnNext(t -> recentTrades.put(t.getId(), t))
  );

[Flemingjp]

It is odd behaviour. My gut reaction would be to avoid any delays and return the current duplicated stream - this is something the user can resolve but in reality its a problem that every user will have to solve, and will cause much confusion.

Is there any way to check if authenticated and then just use one stream of trades or the other - avoid merging them at all?

[badgerwithagun]

It certainly wouldnt work for me - I require both public and private trades. Some exchanges really don't like you opening multiple sockets; their rate limiting can be quite aggressive in that regard.

I think it'd be stretching the contract of the current API too.

One argument is that maybe backing away from mixing authenticated and unauthenticated is the right solution.

[pchertalev]

FYI: GDAX was removed from xchange because:
The gdax.com domain and all its endpoints are being deprecated. Consider this module dead and add new feature requests to xchange-coinbasepro

dsx-tech/XChange@0e5b9c3

[badgerwithagun]

@pchertalev: see discussion on #205

[bryantharris]

Is this the result of the implementation or a result of the way gdax sends events based on the various types you've subscribed to?

If gdax sends the trade out twice for two different subscription types then I'm more inclined to say the bot/client should expect this behavior.

[badgerwithagun]

This may be true, but if it is, I think it would make more sense to split the streams and have a new getUserTrades() method in the API. Otherwise, real-world use of getTrades() will pretty much always need a filter() on type to avoid this

[fynnfluegge]

Is there a reason why coinbaspro was not updated to 4.3.14?

[badgerwithagun]

That looks like a mistake on my part, @fynnfluegge. I'll correct it now.

[badgerwithagun]

This will be fixed under the plan outlined on #274.