More strict exchange name parsing for better error diagnosis in options parser

alpine.Dockerfile

@@ -1,5 +1,5 @@
 # Multi stage build to separate docker build image from executable (to make the latter smaller)
-FROM alpine:3.18.3 AS build
+FROM alpine:3.18.4 AS build
 
 # Install base & build dependencies, needed certificates for curl to work with https
 RUN apk add --update --upgrade --no-cache g++ libc-dev curl-dev cmake ninja git ca-certificates

src/api-objects/src/tradedamounts.cpp

@@ -7,8 +7,7 @@
 namespace cct {
 
 string TradedAmounts::str() const {
-  string ret;
-  ret.append(tradedFrom.str());
+  string ret = tradedFrom.str();
   ret.append(" -> ");
   ret.append(tradedTo.str());
   return ret;

src/api/exchanges/test/commonapi_test.hpp

@@ -19,7 +19,7 @@ namespace cct::api {
 template <class PublicExchangeT, class PrivateExchangeT>
 class TestAPI {
  public:
-  TestAPI() { createPrivateExchangeIfKeyPresent(exchangePublic, coincenterInfo, apiKeysProvider); }
+  TestAPI() { createPrivateExchangeIfKeyPresent(); }
 
   static MarketSet ComputeMarketSetSample(const MarketSet &markets, const CurrencyExchangeFlatSet &currencies) {
     static constexpr int kNbSamples = 1;
@@ -287,8 +287,7 @@ class TestAPI {
   }
 
  private:
-  void createPrivateExchangeIfKeyPresent(PublicExchangeT &exchangePublic, const CoincenterInfo &coincenterInfo,
-                                         const APIKeysProvider &apiKeysProvider) {
+  void createPrivateExchangeIfKeyPresent() {
     std::string_view publicExchangeName = exchangePublic.name();
     if (!apiKeysProvider.contains(publicExchangeName)) {
       log::warn("Skip {} private API test as cannot find associated private key", publicExchangeName);

src/engine/include/queryresulttypes.hpp

@@ -21,39 +21,41 @@
 namespace cct {
 class Exchange;
 
+template <class T>
+using ExchangeWith = std::pair<const Exchange *, T>;
+
 using MarketOrderBookConversionRate = std::tuple<std::string_view, MarketOrderBook, std::optional<MonetaryAmount>>;
 
 using MarketOrderBookConversionRates = FixedCapacityVector<MarketOrderBookConversionRate, kNbSupportedExchanges>;
 
-using MarketsPerExchange = FixedCapacityVector<std::pair<const Exchange *, MarketSet>, kNbSupportedExchanges>;
+using MarketsPerExchange = FixedCapacityVector<ExchangeWith<MarketSet>, kNbSupportedExchanges>;
 
-using MonetaryAmountPerExchange =
-    FixedCapacityVector<std::pair<const Exchange *, MonetaryAmount>, kNbSupportedExchanges>;
+using MonetaryAmountPerExchange = FixedCapacityVector<ExchangeWith<MonetaryAmount>, kNbSupportedExchanges>;
 
-using LastTradesPerExchange = FixedCapacityVector<std::pair<const Exchange *, LastTradesVector>, kNbSupportedExchanges>;
+using LastTradesPerExchange = FixedCapacityVector<ExchangeWith<LastTradesVector>, kNbSupportedExchanges>;
 
-using TradedAmountsPerExchange = SmallVector<std::pair<const Exchange *, TradedAmounts>, kTypicalNbPrivateAccounts>;
+using TradedAmountsPerExchange = SmallVector<ExchangeWith<TradedAmounts>, kTypicalNbPrivateAccounts>;
 
 using TradedAmountsVectorWithFinalAmountPerExchange =
-    SmallVector<std::pair<const Exchange *, TradedAmountsVectorWithFinalAmount>, kTypicalNbPrivateAccounts>;
+    SmallVector<ExchangeWith<TradedAmountsVectorWithFinalAmount>, kTypicalNbPrivateAccounts>;
 
-using ExchangeHealthCheckStatus = FixedCapacityVector<std::pair<const Exchange *, bool>, kNbSupportedExchanges>;
+using ExchangeHealthCheckStatus = FixedCapacityVector<ExchangeWith<bool>, kNbSupportedExchanges>;
 
-using ExchangeTickerMaps = FixedCapacityVector<std::pair<const Exchange *, MarketOrderBookMap>, kNbSupportedExchanges>;
+using ExchangeTickerMaps = FixedCapacityVector<ExchangeWith<MarketOrderBookMap>, kNbSupportedExchanges>;
 
 using BalancePerExchange = SmallVector<std::pair<Exchange *, BalancePortfolio>, kTypicalNbPrivateAccounts>;
 
-using WalletPerExchange = SmallVector<std::pair<const Exchange *, Wallet>, kTypicalNbPrivateAccounts>;
+using WalletPerExchange = SmallVector<ExchangeWith<Wallet>, kTypicalNbPrivateAccounts>;
 
-using OpenedOrdersPerExchange = SmallVector<std::pair<const Exchange *, OrdersSet>, kTypicalNbPrivateAccounts>;
+using OpenedOrdersPerExchange = SmallVector<ExchangeWith<OrdersSet>, kTypicalNbPrivateAccounts>;
 
-using DepositsPerExchange = SmallVector<std::pair<const Exchange *, DepositsSet>, kTypicalNbPrivateAccounts>;
+using DepositsPerExchange = SmallVector<ExchangeWith<DepositsSet>, kTypicalNbPrivateAccounts>;
 
-using WithdrawsPerExchange = SmallVector<std::pair<const Exchange *, WithdrawsSet>, kTypicalNbPrivateAccounts>;
+using WithdrawsPerExchange = SmallVector<ExchangeWith<WithdrawsSet>, kTypicalNbPrivateAccounts>;
 
 using DeliveredWithdrawInfoWithExchanges = std::pair<std::array<const Exchange *, 2>, DeliveredWithdrawInfo>;
 
-using NbCancelledOrdersPerExchange = SmallVector<std::pair<const Exchange *, int>, kTypicalNbPrivateAccounts>;
+using NbCancelledOrdersPerExchange = SmallVector<ExchangeWith<int>, kTypicalNbPrivateAccounts>;
 
-using ConversionPathPerExchange = FixedCapacityVector<std::pair<const Exchange *, MarketsPath>, kNbSupportedExchanges>;
+using ConversionPathPerExchange = FixedCapacityVector<ExchangeWith<MarketsPath>, kNbSupportedExchanges>;
 }  // namespace cct

src/engine/include/stringoptionparser.hpp

@@ -3,6 +3,7 @@
 #include <string_view>
 #include <utility>
 
+#include "cct_string.hpp"
 #include "cct_vector.hpp"
 #include "currencycode.hpp"
 #include "exchangename.hpp"
@@ -51,7 +52,7 @@ class StringOptionParser {
 
   CurrenciesPublicExchanges getCurrenciesPublicExchanges() const;
 
-  vector<std::string_view> getCSVValues() const;
+  vector<string> getCSVValues() const;
 
  protected:
   std::size_t getNextCommaPos(std::size_t startPos = 0, bool throwIfNone = true) const;

src/engine/src/coincenter.cpp

@@ -1,16 +1,26 @@
 #include "coincenter.hpp"
 
 #include <algorithm>
-#include <chrono>
-#include <iostream>
+#include <optional>
+#include <span>
 #include <thread>
 
-#include "abstractmetricgateway.hpp"
+#include "balanceoptions.hpp"
+#include "cct_exception.hpp"
 #include "coincentercommands.hpp"
 #include "coincentercommandtype.hpp"
-#include "coincenteroptions.hpp"
+#include "coincenterinfo.hpp"
+#include "currencycode.hpp"
+#include "depositsconstraints.hpp"
+#include "exchangename.hpp"
+#include "exchangeretriever.hpp"
+#include "exchangesecretsinfo.hpp"
+#include "market.hpp"
+#include "monetaryamount.hpp"
+#include "ordersconstraints.hpp"
 #include "queryresultprinter.hpp"
-#include "stringoptionparser.hpp"
+#include "queryresulttypes.hpp"
+#include "withdrawsconstraints.hpp"
 
 namespace cct {
 using UniquePublicSelectedExchanges = ExchangeRetriever::UniquePublicSelectedExchanges;
@@ -317,8 +327,7 @@ void Coincenter::updateFileCaches() const {
   log::debug("Store all cache files");
   _commonAPI.updateCacheFile();
   _fiatConverter.updateCacheFile();
-  auto exchanges = _exchangePool.exchanges();
-  std::for_each(exchanges.begin(), exchanges.end(), [](const Exchange &exchange) { exchange.updateCacheFile(); });
+  std::ranges::for_each(_exchangePool.exchanges(), [](const Exchange &exchange) { exchange.updateCacheFile(); });
 }
 
 }  // namespace cct

src/engine/src/coincentercommands.cpp

@@ -76,12 +76,7 @@ namespace {
 std::pair<OrdersConstraints, ExchangeNames> ParseOrderRequest(const CoincenterCmdLineOptions &cmdLineOptions,
                                                               std::string_view orderRequestStr) {
   auto currenciesPrivateExchangesTuple = StringOptionParser(orderRequestStr).getCurrenciesPrivateExchanges(false);
-  auto orderIdViewVector = StringOptionParser(cmdLineOptions.ids).getCSVValues();
-  vector<OrderId> orderIds;
-  orderIds.reserve(orderIdViewVector.size());
-  for (std::string_view orderIdView : orderIdViewVector) {
-    orderIds.emplace_back(orderIdView);
-  }
+  auto orderIds = StringOptionParser(cmdLineOptions.ids).getCSVValues();
   return std::make_pair(
       OrdersConstraints(std::get<0>(currenciesPrivateExchangesTuple), std::get<1>(currenciesPrivateExchangesTuple),
                         std::chrono::duration_cast<Duration>(cmdLineOptions.minAge),
@@ -306,12 +301,7 @@ bool CoincenterCommands::addOption(const CoincenterCmdLineOptions &cmdLineOption
   if (cmdLineOptions.recentDepositsInfo) {
     auto [currencyCode, exchanges] = StringOptionParser(*cmdLineOptions.recentDepositsInfo)
                                          .getCurrencyPrivateExchanges(StringOptionParser::CurrencyIs::kOptional);
-    auto depositIdViewVector = StringOptionParser(cmdLineOptions.ids).getCSVValues();
-    vector<string> depositIds;
-    depositIds.reserve(depositIdViewVector.size());
-    for (std::string_view depositIdView : depositIdViewVector) {
-      depositIds.emplace_back(depositIdView);
-    }
+    auto depositIds = StringOptionParser(cmdLineOptions.ids).getCSVValues();
     DepositsConstraints depositConstraints(currencyCode, std::chrono::duration_cast<Duration>(cmdLineOptions.minAge),
                                            std::chrono::duration_cast<Duration>(cmdLineOptions.maxAge),
                                            DepositsConstraints::IdSet(std::move(depositIds)));
@@ -323,12 +313,7 @@ bool CoincenterCommands::addOption(const CoincenterCmdLineOptions &cmdLineOption
   if (cmdLineOptions.recentWithdrawsInfo) {
     auto [currencyCode, exchanges] = StringOptionParser(*cmdLineOptions.recentWithdrawsInfo)
                                          .getCurrencyPrivateExchanges(StringOptionParser::CurrencyIs::kOptional);
-    auto withdrawIdViewVector = StringOptionParser(cmdLineOptions.ids).getCSVValues();
-    vector<string> withdrawIds;
-    withdrawIds.reserve(withdrawIdViewVector.size());
-    for (std::string_view withdrawIdView : withdrawIdViewVector) {
-      withdrawIds.emplace_back(withdrawIdView);
-    }
+    auto withdrawIds = StringOptionParser(cmdLineOptions.ids).getCSVValues();
     WithdrawsConstraints withdrawConstraints(currencyCode, std::chrono::duration_cast<Duration>(cmdLineOptions.minAge),
                                              std::chrono::duration_cast<Duration>(cmdLineOptions.maxAge),
                                              WithdrawsConstraints::IdSet(std::move(withdrawIds)));
@@ -341,26 +326,20 @@ bool CoincenterCommands::addOption(const CoincenterCmdLineOptions &cmdLineOption
     StringOptionParser anyParser(cmdLineOptions.withdraw);
     auto [amountToWithdraw, isPercentageWithdraw, fromExchange, toExchange] =
         anyParser.getMonetaryAmountFromToPrivateExchange();
-    ExchangeNames exchanges;
-    exchanges.push_back(std::move(fromExchange));
-    exchanges.push_back(std::move(toExchange));
     _commands.emplace_back(CoincenterCommandType::kWithdraw)
         .setAmount(amountToWithdraw)
         .setPercentageAmount(isPercentageWithdraw)
-        .setExchangeNames(std::move(exchanges))
+        .setExchangeNames(ExchangeNames{std::move(fromExchange), std::move(toExchange)})
         .setWithdrawOptions(ComputeWithdrawOptions(cmdLineOptions));
   }
 
   if (!cmdLineOptions.withdrawAll.empty()) {
     StringOptionParser anyParser(cmdLineOptions.withdrawAll);
     auto [curToWithdraw, fromExchange, toExchange] = anyParser.getCurrencyFromToPrivateExchange();
-    ExchangeNames exchanges;
-    exchanges.push_back(std::move(fromExchange));
-    exchanges.push_back(std::move(toExchange));
     _commands.emplace_back(CoincenterCommandType::kWithdraw)
         .setAmount(MonetaryAmount(100, curToWithdraw))
         .setPercentageAmount(true)
-        .setExchangeNames(std::move(exchanges))
+        .setExchangeNames(ExchangeNames{std::move(fromExchange), std::move(toExchange)})
         .setWithdrawOptions(ComputeWithdrawOptions(cmdLineOptions));
   }
 

src/engine/src/stringoptionparser.cpp

@@ -64,7 +64,7 @@ std::string_view GetNextStr(std::string_view opt, CharOrStringType sep, std::siz
 }
 
 auto GetNextPercentageAmount(std::string_view opt, std::string_view sepWithPercentageAtLast, std::size_t &pos) {
-  std::string_view amountStr = GetNextStr(opt, sepWithPercentageAtLast, pos);
+  auto amountStr = GetNextStr(opt, sepWithPercentageAtLast, pos);
 
   if (amountStr.empty()) {
     if (pos == opt.size()) {
@@ -86,6 +86,16 @@ auto GetNextPercentageAmount(std::string_view opt, std::string_view sepWithPerce
   }
   return std::make_pair(std::move(startAmount), isPercentage);
 }
+
+template <class CharOrStringType>
+auto GetNextExchangeName(std::string_view opt, CharOrStringType sep, std::size_t &pos) {
+  auto nextStr = GetNextStr(opt, sep, pos);
+  if (nextStr.empty()) {
+    throw invalid_argument("Expected an exchange identifier in '{}'", opt);
+  }
+  return ExchangeName(nextStr);
+}
+
 }  // namespace
 
 ExchangeNames StringOptionParser::getExchanges() const { return GetExchanges(_opt); }
@@ -173,19 +183,18 @@ StringOptionParser::getMonetaryAmountCurrencyPrivateExchanges(bool withCurrency)
 StringOptionParser::CurrencyFromToPrivateExchange StringOptionParser::getCurrencyFromToPrivateExchange() const {
   std::size_t pos = 0;
   CurrencyCode cur(GetNextStr(_opt, ',', pos));
-  ExchangeName from(GetNextStr(_opt, '-', pos));
+  ExchangeName from = GetNextExchangeName(_opt, '-', pos);
   // Warning: in C++, order of evaluation of parameters is unspecified. Because GetNextStr has side
   // effects (it modifies 'pos') we need temporary variables here
-  return std::make_tuple(std::move(cur), std::move(from), ExchangeName(GetNextStr(_opt, '-', pos)));
+  return std::make_tuple(std::move(cur), std::move(from), GetNextExchangeName(_opt, '-', pos));
 }
 
 StringOptionParser::MonetaryAmountFromToPrivateExchange StringOptionParser::getMonetaryAmountFromToPrivateExchange()
     const {
   std::size_t pos = 0;
   auto [startAmount, isPercentage] = GetNextPercentageAmount(_opt, ",%", pos);
-  ExchangeName from(GetNextStr(_opt, '-', pos));
-  return std::make_tuple(std::move(startAmount), isPercentage, std::move(from),
-                         ExchangeName(GetNextStr(_opt, '-', pos)));
+  ExchangeName from = GetNextExchangeName(_opt, '-', pos);
+  return std::make_tuple(std::move(startAmount), isPercentage, std::move(from), GetNextExchangeName(_opt, '-', pos));
 }
 
 std::size_t StringOptionParser::getNextCommaPos(std::size_t startPos, bool throwIfNone) const {
@@ -226,9 +235,9 @@ StringOptionParser::CurrenciesPublicExchanges StringOptionParser::getCurrenciesP
   return ret;
 }
 
-vector<std::string_view> StringOptionParser::getCSVValues() const {
+vector<string> StringOptionParser::getCSVValues() const {
   std::size_t pos = 0;
-  vector<std::string_view> ret;
+  vector<string> ret;
   if (!_opt.empty()) {
     do {
       std::size_t nextCommaPos = getNextCommaPos(pos, false);

src/engine/test/stringoptionparser_test.cpp

@@ -120,6 +120,8 @@ TEST(StringOptionParserTest, GetMonetaryAmountFromToPrivateExchange) {
   EXPECT_EQ(StringOptionParser("4.106eth,kraken_user2-huobi_user3").getMonetaryAmountFromToPrivateExchange(),
             std::make_tuple(MonetaryAmount("4.106ETH"), false, ExchangeName("kraken", "user2"),
                             ExchangeName("huobi", "user3")));
+
+  EXPECT_THROW(StringOptionParser("test").getMonetaryAmountFromToPrivateExchange(), invalid_argument);
 }
 
 TEST(StringOptionParserTest, GetMonetaryAmountPercentageFromToPrivateExchange) {
@@ -191,9 +193,9 @@ TEST(StringOptionParserTest, GetCurrenciesPrivateExchangesWithCurrencies) {
 }
 
 TEST(StringOptionParserTest, CSVValues) {
-  EXPECT_EQ(StringOptionParser("").getCSVValues(), vector<std::string_view>());
-  EXPECT_EQ(StringOptionParser("val1,").getCSVValues(), vector<std::string_view>{{"val1"}});
-  EXPECT_EQ(StringOptionParser("val1,value").getCSVValues(), vector<std::string_view>({{"val1"}, {"value"}}));
+  EXPECT_EQ(StringOptionParser("").getCSVValues(), vector<string>());
+  EXPECT_EQ(StringOptionParser("val1,").getCSVValues(), vector<string>{{"val1"}});
+  EXPECT_EQ(StringOptionParser("val1,value").getCSVValues(), vector<string>({{"val1"}, {"value"}}));
 }
 
 }  // namespace cct

src/monitoring/include/abstractmetricgateway.hpp

@@ -11,7 +11,7 @@ class AbstractMetricGateway {
  public:
   using BucketBoundaries = std::span<const double>;
 
-  virtual ~AbstractMetricGateway() {}
+  virtual ~AbstractMetricGateway() = default;
 
   /// Register some numeric value
   virtual void add(MetricType metricType, MetricOperation op, const MetricKey& key, double v = 0) = 0;

src/monitoring/include/prometheusmetricgateway.hpp

@@ -28,7 +28,7 @@ class PrometheusMetricGateway : public AbstractMetricGateway {
   PrometheusMetricGateway &operator=(const PrometheusMetricGateway &) = delete;
   PrometheusMetricGateway &operator=(PrometheusMetricGateway &&) = delete;
 
-  ~PrometheusMetricGateway();
+  ~PrometheusMetricGateway() override;
 
   void add(MetricType metricType, MetricOperation op, const MetricKey &key, double val = 0) override;
 
@@ -45,6 +45,6 @@ class PrometheusMetricGateway : public AbstractMetricGateway {
   std::unordered_map<MetricKey, void *> _familiesMap;
   std::mutex _familiesMapMutex;
   TimePoint _lastFlushedTime;
-  int _checkFlushCounter;  // To decrease number of times flush check is done
+  int _checkFlushCounter{};  // To decrease number of times flush check is done
 };
 }  // namespace cct

src/monitoring/src/prometheusmetricgateway.cpp

@@ -53,8 +53,7 @@ PrometheusMetricGateway::PrometheusMetricGateway(const MonitoringInfo& monitorin
                std::string(monitoringInfo.jobName()), prometheus::Gateway::GetInstanceLabel(GetHostName()),
                std::string(monitoringInfo.username()), std::string(monitoringInfo.password())),
       _registry(std::make_shared<Registry>()),
-      _lastFlushedTime(Clock::now()),
-      _checkFlushCounter(0) {
+      _lastFlushedTime(Clock::now()) {
   // create a push gateway
   _gateway.RegisterCollectable(_registry);
 }
@@ -229,7 +228,7 @@ void PrometheusMetricGateway::flush() {
   int returnCode = _gateway.Push();
   if (returnCode == kHTTPSuccessReturnCode) {
     log::info("Flushed data to Prometheus in {} ms",
-              std::chrono::duration_cast<std::chrono::milliseconds>(Clock::now() - nowTime).count());
+              std::chrono::duration_cast<TimeInMs>(Clock::now() - nowTime).count());
   } else {
     log::error("Unable to push metrics to Prometheus instance - Bad return code {}", returnCode);
   }

src/objects/src/exchangename.cpp

@@ -4,11 +4,15 @@
 #include <string_view>
 
 #include "cct_exception.hpp"
+#include "cct_invalid_argument_exception.hpp"
 #include "cct_string.hpp"
 #include "toupperlower.hpp"
 
 namespace cct {
 ExchangeName::ExchangeName(std::string_view globalExchangeName) : _nameWithKey(globalExchangeName) {
+  if (globalExchangeName.empty()) {
+    throw invalid_argument("Exchange name cannot be empty");
+  }
   for (std::size_t charPos = 0, sz = globalExchangeName.size(); charPos < sz && _nameWithKey[charPos] != '_';
        ++charPos) {
     _nameWithKey[charPos] = tolower(_nameWithKey[charPos]);
@@ -18,7 +22,7 @@ ExchangeName::ExchangeName(std::string_view globalExchangeName) : _nameWithKey(g
 ExchangeName::ExchangeName(std::string_view exchangeName, std::string_view keyName)
     : _nameWithKey(ToLower(exchangeName)) {
   if (_nameWithKey.find('_') != string::npos) {
-    throw exception("Invalid exchange name {}", _nameWithKey);
+    throw invalid_argument("Invalid exchange name {}", _nameWithKey);
   }
   if (!keyName.empty()) {
     _nameWithKey.push_back('_');