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

#446 Bitstamp v2 not getting trades - fixed #456

Conversation

pchertalev
Copy link
Contributor

No description provided.

@jurepetrovic
Copy link

Hello,

@pchertalev
No, I don't mind better changes, just so it works smooth and I can use it in production ;-)

I see you used existing JSON mapping class.
I didn't want to do it, because it's already used in v1 of the protocol.
info.bitrich.xchangestream.bitstamp.BitstampStreamingMarketDataService, line 64.

If this doesn't break compatibility I don't mind.
Or do you want to throw v1 support completely out?

@pchertalev
Copy link
Contributor Author

pchertalev commented Nov 28, 2019

There is only one version of BitstampAdapters class in XChange 4.4, there is no v2.BitstampAdapters, and there is no other method BitstampAdapters.adaptTrade for v2 version. So I don't see any other way - we must use BitstampTransaction class because it is used as parameter BitstampAdapters.adaptTrade. BitstampWebSocketTransaction is just wrapper for adapting input json fields to BitstampTransaction fileds.
As you see I used BitstampOrderBook class without wrapper as well because websocket json format is completely match and it is required for BitstampAdapters.adaptOrderBook method. BitstampOrderBook has all required Jackson annotations for parsing - this allow us to avoid other classes in code.
About cutting out v1 - I have checked Bitstamp V1 pusher service still works and it can be used by someone. So I don't see reason to remove it so far.
I don't know why v1 version contains special empty channel prefix for BTC_USD, but I checked - it works :) Actually it works with one as well but I did not touch this :)

private String getChannelPostfix(CurrencyPair currencyPair) {
    if (currencyPair.equals(CurrencyPair.BTC_USD)) { 
        return "";
    }
    return "_" + currencyPair.base.toString().toLowerCase() + currencyPair.counter.toString().toLowerCase();
}

@jurepetrovic
Copy link

Ok, if BitstampWebTransaction works for v1 and v2 trades, it looks perfect to me ;)

You need something from me to merge?

As soon as you guys put it in develop branch of main repo,
I'll go after it and use it in production.

Thanks,
Jure

@pchertalev
Copy link
Contributor Author

@Flemingjp @badgerwithagun Hi guys, if you have a time, please look at this PR

@mdvx
Copy link
Contributor

mdvx commented Nov 29, 2019

#446 (comment)

image

I see the side calculation is in BitstampAdapters.adaptTrade

So I guess Bitstamp's own website is displaying it wrong.

Copy link
Contributor

@mdvx mdvx left a comment

Choose a reason for hiding this comment

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

oops duplicate

Copy link
Contributor

@mdvx mdvx left a comment

Choose a reason for hiding this comment

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

looks good to me

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.

One thing spotted with the changed code.

@badgerwithagun badgerwithagun added the awaiting_fixes PR is awaiting submitter to respond to a review label Dec 6, 2019
@badgerwithagun badgerwithagun merged commit 5f46bfc into bitrich-info:develop Dec 9, 2019
@pchertalev pchertalev deleted the 446-bitstamp-not-getting-public-trades branch February 20, 2020 10:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting_fixes PR is awaiting submitter to respond to a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants