[bisq-network/bisq] [WIP] PriceNode: Add support for multiple ExchangeRateProviders (#4315)

[Chris Beams]

@cbeams requested changes on this pull request.

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.

> @@ -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}

Please wrap comments at 90 characters per https://github.com/bisq-network/style/issues/5.

> +        // 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) {

Use `ex` for exception variable names. (`e` is also used widely throughout the codebase; `ex` is preferred).

> +                        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());

Please wrap code at 120 chars per https://github.com/bisq-network/style/issues/3.

> +            else {
+                // If the map was built incorrectly and this currency points to an empty list of rates, skip it
+               return;
+            }

Prefer modelling this as guard logic, e.g.:

```java
if (exchangeRateList.isEmpty())
    // If the map was built incorrectly and this currency points to an empty list of rates, skip it
    return;

if (exchangeRateList.size() == 1) {
    // If a single provider has rates for this currency, then aggregate = rate from that provider
    aggregateExchangeRate = exchangeRateList.get(0);
}
else {
    // If multiple providers have rates for this currency, then aggregate = average of the rates
    // ...
}
aggregateExchangeRates.put(aggregateExchangeRate.getCurrency(), aggregateExchangeRate);
```

See also https://github.com/bisq-network/style/issues/12

> +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;
+
+ at Component
+ at Order(5)

Now that we are always querying all `ExchangeRateProviders` and averaging the results, the `@Order` annotation is no longer needed. As per the Javadoc in `ExchangeRateProvider`, 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.

>  
 @Component
 @Order(4)
-class Poloniex extends ExchangeRateProvider {
-
-    private final RestTemplate restTemplate = new RestTemplate();
+public class Poloniex extends ExchangeRateProvider {

Prefer package-private visibility whenever possible, i.e. please remove the `public` modifier here and in all other `ExchangeRateProvider` implementations. The `bisq.spot.providers` package is expressly designed such that all member classes are package-private; they need not and should not be referenced from any other package.

> +
+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;
+
+ at Slf4j
+public class ExchangeTestBase {

Prefer `AbstractExchangeTest` for naming consistency and please mark the class as `abstract` to better signal that it is not in fact a concrete / executable test.

> +
+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;
+
+ at Slf4j
+public class ExchangeTestBase {

Actually, prefer `AbstractExchangeRateProviderTest` for clarity, as what is being tested are indeed `ExchangeRateProvider` implementations.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4315#pullrequestreview-432133722
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200617/5cfedafb/attachment.html>