<p></p>
<p><b>@cbeams</b> requested changes on this pull request.</p>
<p>NACK. I just had a few minutes to run through these changes right now, and so have added some mostly style-related change requests. I'd like to actually run and test everything myself as well, but will have to get to that later.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441341564">pricenode/src/main/java/bisq/price/spot/ExchangeRateProvider.java</a>:</p>
<pre style='color:#555'>> @@ -60,4 +75,82 @@ protected void onRefresh() {
.filter(e -> "USD".equals(e.getCurrency()) || "LTC".equals(e.getCurrency()))
.forEach(e -> log.info("BTC/{}: {}", e.getCurrency(), e.getPrice()));
}
+
+ /**
+ * @param exchangeClass Class of the {@link Exchange} for which the rates should be polled
+ * @return Exchange rates for Bisq-supported fiat currencies and altcoins in the specified {@link Exchange}
</pre>
<p>Please wrap comments at 90 characters per <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="305884324" data-permission-text="Title is private" data-url="https://github.com/bisq-network/style/issues/5" data-hovercard-type="issue" data-hovercard-url="/bisq-network/style/issues/5/hovercard" href="https://github.com/bisq-network/style/issues/5">bisq-network/style#5</a>.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441341961">pricenode/src/main/java/bisq/price/spot/ExchangeRateProvider.java</a>:</p>
<pre style='color:#555'>> + // Filter the supported altcoins (currency pair format is ALT-BTC)
+ currencyPairs.stream()
+ .filter(cp -> cp.counter.equals(Currency.BTC))
+ .filter(cp -> supportedCryptoCurrencies.contains(cp.base.getCurrencyCode()))
+ .forEach(cp -> {
+ try {
+ Ticker t = marketDataService.getTicker(new CurrencyPair(cp.base, cp.counter));
+
+ result.add(new ExchangeRate(
+ cp.base.getCurrencyCode(),
+ t.getLast(),
+ // Some exchanges do not provide timestamps
+ t.getTimestamp() == null ? new Date() : t.getTimestamp(),
+ this.getName()
+ ));
+ } catch (CurrencyPairNotValidException cpnve) {
</pre>
<p>Use <code>ex</code> for exception variable names. (<code>e</code> is also used widely throughout the codebase; <code>ex</code> is preferred).</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441342593">pricenode/src/main/java/bisq/price/spot/ExchangeRateProvider.java</a>:</p>
<pre style='color:#555'>> + Ticker t = marketDataService.getTicker(new CurrencyPair(cp.base, cp.counter));
+
+ result.add(new ExchangeRate(
+ cp.counter.getCurrencyCode(),
+ t.getLast(),
+ // Some exchanges do not provide timestamps
+ t.getTimestamp() == null ? new Date() : t.getTimestamp(),
+ this.getName()
+ ));
+ } catch (CurrencyPairNotValidException cpnve) {
+ // Some exchanges support certain currency pairs for other services but not for spot markets
+ // In that case, trying to retrieve the market ticker for that pair may fail with this specific type of exception
+ log.info("Currency pair " + cp + " not supported in Spot Markets: " + cpnve.getMessage());
+ } catch (Exception e) {
+ // Catch any other type of generic exception (IO, network level, rate limit reached, etc)
+ log.info("Exception encountered while retrieving rate for currency pair " + cp + ": " + e.getMessage());
</pre>
<p>Please wrap code at 120 chars per <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="305847830" data-permission-text="Title is private" data-url="https://github.com/bisq-network/style/issues/3" data-hovercard-type="issue" data-hovercard-url="/bisq-network/style/issues/3/hovercard" href="https://github.com/bisq-network/style/issues/3">bisq-network/style#3</a>.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441344741">pricenode/src/main/java/bisq/price/spot/ExchangeRateService.java</a>:</p>
<pre style='color:#555'>> + else {
+ // If the map was built incorrectly and this currency points to an empty list of rates, skip it
+ return;
+ }
</pre>
<p>Prefer modelling this as guard logic, e.g.:</p>
<div class="highlight highlight-source-java"><pre><span class="pl-k">if</span> (exchangeRateList<span class="pl-k">.</span>isEmpty())
<span class="pl-c"><span class="pl-c">//</span> If the map was built incorrectly and this currency points to an empty list of rates, skip it</span>
<span class="pl-k">return</span>;
<span class="pl-k">if</span> (exchangeRateList<span class="pl-k">.</span>size() <span class="pl-k">==</span> <span class="pl-c1">1</span>) {
<span class="pl-c"><span class="pl-c">//</span> If a single provider has rates for this currency, then aggregate = rate from that provider</span>
aggregateExchangeRate <span class="pl-k">=</span> exchangeRateList<span class="pl-k">.</span>get(<span class="pl-c1">0</span>);
}
<span class="pl-k">else</span> {
<span class="pl-c"><span class="pl-c">//</span> If multiple providers have rates for this currency, then aggregate = average of the rates</span>
<span class="pl-c"><span class="pl-c">//</span> ...</span>
}
aggregateExchangeRates<span class="pl-k">.</span>put(aggregateExchangeRate<span class="pl-k">.</span>getCurrency(), aggregateExchangeRate);</pre></div>
<p>See also <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="329785694" data-permission-text="Title is private" data-url="https://github.com/bisq-network/style/issues/12" data-hovercard-type="issue" data-hovercard-url="/bisq-network/style/issues/12/hovercard" href="https://github.com/bisq-network/style/issues/12">bisq-network/style#12</a></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441346629">pricenode/src/main/java/bisq/price/spot/providers/Binance.java</a>:</p>
<pre style='color:#555'>> +package bisq.price.spot.providers;
+
+import bisq.price.spot.ExchangeRate;
+import bisq.price.spot.ExchangeRateProvider;
+
+import org.knowm.xchange.binance.BinanceExchange;
+
+import org.springframework.core.annotation.Order;
+import org.springframework.stereotype.Component;
+
+import java.time.Duration;
+
+import java.util.Set;
+
+@Component
+@Order(5)
</pre>
<p>Now that we are always querying all <code>ExchangeRateProviders</code> and averaging the results, the <code>@Order</code> annotation is no longer needed. As per the Javadoc in <code>ExchangeRateProvider</code>, this annotation was used to ensure precedence, but that becomes moot now. I'd recommend removing it, and updating the aforementioned Javadoc to avoid confusion.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441348909">pricenode/src/main/java/bisq/price/spot/providers/Poloniex.java</a>:</p>
<pre style='color:#555'>>
@Component
@Order(4)
-class Poloniex extends ExchangeRateProvider {
-
- private final RestTemplate restTemplate = new RestTemplate();
+public class Poloniex extends ExchangeRateProvider {
</pre>
<p>Prefer package-private visibility whenever possible, i.e. please remove the <code>public</code> modifier here and in all other <code>ExchangeRateProvider</code> implementations. The <code>bisq.spot.providers</code> package is expressly designed such that all member classes are package-private; they need not and should not be referenced from any other package.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441349939">pricenode/src/test/java/bisq/price/ExchangeTestBase.java</a>:</p>
<pre style='color:#555'>> +
+import bisq.core.locale.CurrencyUtil;
+import bisq.core.locale.TradeCurrency;
+
+import com.google.common.collect.Sets;
+
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+
+import lombok.extern.slf4j.Slf4j;
+
+import static org.junit.Assert.assertTrue;
+
+@Slf4j
+public class ExchangeTestBase {
</pre>
<p>Prefer <code>AbstractExchangeTest</code> for naming consistency and please mark the class as <code>abstract</code> to better signal that it is not in fact a concrete / executable test.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/4315#discussion_r441351017">pricenode/src/test/java/bisq/price/ExchangeTestBase.java</a>:</p>
<pre style='color:#555'>> +
+import bisq.core.locale.CurrencyUtil;
+import bisq.core.locale.TradeCurrency;
+
+import com.google.common.collect.Sets;
+
+import java.util.Set;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+
+import lombok.extern.slf4j.Slf4j;
+
+import static org.junit.Assert.assertTrue;
+
+@Slf4j
+public class ExchangeTestBase {
</pre>
<p>Actually, prefer <code>AbstractExchangeRateProviderTest</code> for clarity, as what is being tested are indeed <code>ExchangeRateProvider</code> implementations.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/4315#pullrequestreview-432133722">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNU4EE43JKN6KWWL5BLRXBZCFANCNFSM4N75VIBA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNXOYHN3FLUJOHRIVH3RXBZCFA5CNFSM4N75VIBKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGODHA5MWQ.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/4315#pullrequestreview-432133722",
"url": "https://github.com/bisq-network/bisq/pull/4315#pullrequestreview-432133722",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>