fix stream binance bug due to homeCurrencies #360

2021-03-07T23:40:09-08:00

thisiscam commented

2021-03-07 23:40:09 -08:00

(Migrated from github.com)

This fixes #357 .

Quote from @scionaltera, the cause is basically:

Another possibility is that we're putting the wrong CurrencyPairs in the ProductSubscription in StreamingTickerStrategy#getTickers() due to the homeCurrency setting. The code in the BinanceStreamingMarketDataService checks to see if the ticker you're asking for is in the original list created when the connection was first connected. If, for example, we put "BTC/USD" in at that point and "BTC/USDT" in later, or the reverse, that could cause this problem too.

Also upgraded to 5.0.6 so might be better merged after #354 .

This fixes #357 . Quote from @scionaltera, the cause is basically: > Another possibility is that we're putting the wrong CurrencyPairs in the ProductSubscription in StreamingTickerStrategy#getTickers() due to the homeCurrency setting. The code in the BinanceStreamingMarketDataService checks to see if the ticker you're asking for is in the original list created when the connection was first connected. If, for example, we put "BTC/USD" in at that point and "BTC/USDT" in later, or the reverse, that could cause this problem too. Also upgraded to 5.0.6 so might be better merged after #354 .

scionaltera (Migrated from github.com) requested changes

2021-03-08 10:23:23 -08:00

src/main/java/com/r307/arbitrader/service/ticker/StreamingTickerStrategy.java

					
				@ -54,3 +54,3 @@

				            currencyPairs.forEach(builder::addTicker);

				            currencyPairs.forEach(pair -> { builder.addTicker(exchangeService.convertExchangePair(exchange, pair)); });

This is a good find. Unfortunately I can't merge this PR until after we're ready to go to 5.0.6 and I won't be able to merge with the extra snapshot repository enabled. If you can reduce the PR to just this one change I could merge it today.

thisiscam (Migrated from github.com) reviewed

2021-03-08 10:25:20 -08:00

src/main/java/com/r307/arbitrader/service/ticker/StreamingTickerStrategy.java

					
				@ -54,3 +54,3 @@

				            currencyPairs.forEach(builder::addTicker);

				            currencyPairs.forEach(pair -> { builder.addTicker(exchangeService.convertExchangePair(exchange, pair)); });

Ok thanks. Just let you know that without 5.0.6 at least Binance doesn't work for me.
But I guess this line is separate from that issue.

Ok thanks. Just let you know that without 5.0.6 at least Binance doesn't work for me. But I guess this line is separate from that issue.

thisiscam (Migrated from github.com) reviewed

2021-03-08 10:30:26 -08:00

src/main/java/com/r307/arbitrader/service/ticker/StreamingTickerStrategy.java

					
				@ -54,3 +54,3 @@

				            currencyPairs.forEach(builder::addTicker);

				            currencyPairs.forEach(pair -> { builder.addTicker(exchangeService.convertExchangePair(exchange, pair)); });

@scionaltera done!

scionaltera (Migrated from github.com) reviewed

2021-03-08 10:36:13 -08:00

src/main/java/com/r307/arbitrader/service/ticker/StreamingTickerStrategy.java

					
				@ -54,3 +54,3 @@

				            currencyPairs.forEach(builder::addTicker);

				            currencyPairs.forEach(pair -> { builder.addTicker(exchangeService.convertExchangePair(exchange, pair)); });

The line you fixed will still fix the bug in the code as it is now, and you're still free to use 5.0.6 in your own branch. We just haven't had a chance to test the upgrade to 5.0.6 by itself much yet, and I keep merging things on top of #326 and #359 which is slowing testing on those big PRs. I'd like to get them out the door, bump to our next version and unblock everything else.

The line you fixed will still fix the bug in the code as it is now, and you're still free to use 5.0.6 in your own branch. We just haven't had a chance to test the upgrade to 5.0.6 _by itself_ much yet, and I keep merging things on top of #326 and #359 which is slowing testing on those big PRs. I'd like to get them out the door, bump to our next version and unblock everything else.