From 0832518a973d369cba6098b87b713cb109c77816 Mon Sep 17 00:00:00 2001 From: Stephane Janel Date: Sun, 22 Oct 2023 11:08:37 +0200 Subject: [PATCH] More strict exchange name parsing for better error diagnosis in options parser --- alpine.Dockerfile | 2 +- src/api-objects/src/tradedamounts.cpp | 3 +- src/api/exchanges/test/commonapi_test.hpp | 5 ++- src/engine/include/queryresulttypes.hpp | 30 +++++++++--------- src/engine/include/stringoptionparser.hpp | 3 +- src/engine/src/coincenter.cpp | 23 +++++++++----- src/engine/src/coincentercommands.cpp | 31 +++---------------- src/engine/src/stringoptionparser.cpp | 25 ++++++++++----- src/engine/test/stringoptionparser_test.cpp | 8 +++-- .../include/abstractmetricgateway.hpp | 2 +- .../include/prometheusmetricgateway.hpp | 4 +-- .../src/prometheusmetricgateway.cpp | 5 ++- src/objects/src/exchangename.cpp | 6 +++- 13 files changed, 75 insertions(+), 72 deletions(-) diff --git a/alpine.Dockerfile b/alpine.Dockerfile index 9e1bd1f5..ddf0bb7e 100644 --- a/alpine.Dockerfile +++ b/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 diff --git a/src/api-objects/src/tradedamounts.cpp b/src/api-objects/src/tradedamounts.cpp index a79b3366..7cb1b91b 100644 --- a/src/api-objects/src/tradedamounts.cpp +++ b/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; diff --git a/src/api/exchanges/test/commonapi_test.hpp b/src/api/exchanges/test/commonapi_test.hpp index 3f471c63..0e585c89 100644 --- a/src/api/exchanges/test/commonapi_test.hpp +++ b/src/api/exchanges/test/commonapi_test.hpp @@ -19,7 +19,7 @@ namespace cct::api { template class TestAPI { public: - TestAPI() { createPrivateExchangeIfKeyPresent(exchangePublic, coincenterInfo, apiKeysProvider); } + TestAPI() { createPrivateExchangeIfKeyPresent(); } static MarketSet ComputeMarketSetSample(const MarketSet &markets, const CurrencyExchangeFlatSet ¤cies) { 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); diff --git a/src/engine/include/queryresulttypes.hpp b/src/engine/include/queryresulttypes.hpp index 2e3f1c27..94651010 100644 --- a/src/engine/include/queryresulttypes.hpp +++ b/src/engine/include/queryresulttypes.hpp @@ -21,39 +21,41 @@ namespace cct { class Exchange; +template +using ExchangeWith = std::pair; + using MarketOrderBookConversionRate = std::tuple>; using MarketOrderBookConversionRates = FixedCapacityVector; -using MarketsPerExchange = FixedCapacityVector, kNbSupportedExchanges>; +using MarketsPerExchange = FixedCapacityVector, kNbSupportedExchanges>; -using MonetaryAmountPerExchange = - FixedCapacityVector, kNbSupportedExchanges>; +using MonetaryAmountPerExchange = FixedCapacityVector, kNbSupportedExchanges>; -using LastTradesPerExchange = FixedCapacityVector, kNbSupportedExchanges>; +using LastTradesPerExchange = FixedCapacityVector, kNbSupportedExchanges>; -using TradedAmountsPerExchange = SmallVector, kTypicalNbPrivateAccounts>; +using TradedAmountsPerExchange = SmallVector, kTypicalNbPrivateAccounts>; using TradedAmountsVectorWithFinalAmountPerExchange = - SmallVector, kTypicalNbPrivateAccounts>; + SmallVector, kTypicalNbPrivateAccounts>; -using ExchangeHealthCheckStatus = FixedCapacityVector, kNbSupportedExchanges>; +using ExchangeHealthCheckStatus = FixedCapacityVector, kNbSupportedExchanges>; -using ExchangeTickerMaps = FixedCapacityVector, kNbSupportedExchanges>; +using ExchangeTickerMaps = FixedCapacityVector, kNbSupportedExchanges>; using BalancePerExchange = SmallVector, kTypicalNbPrivateAccounts>; -using WalletPerExchange = SmallVector, kTypicalNbPrivateAccounts>; +using WalletPerExchange = SmallVector, kTypicalNbPrivateAccounts>; -using OpenedOrdersPerExchange = SmallVector, kTypicalNbPrivateAccounts>; +using OpenedOrdersPerExchange = SmallVector, kTypicalNbPrivateAccounts>; -using DepositsPerExchange = SmallVector, kTypicalNbPrivateAccounts>; +using DepositsPerExchange = SmallVector, kTypicalNbPrivateAccounts>; -using WithdrawsPerExchange = SmallVector, kTypicalNbPrivateAccounts>; +using WithdrawsPerExchange = SmallVector, kTypicalNbPrivateAccounts>; using DeliveredWithdrawInfoWithExchanges = std::pair, DeliveredWithdrawInfo>; -using NbCancelledOrdersPerExchange = SmallVector, kTypicalNbPrivateAccounts>; +using NbCancelledOrdersPerExchange = SmallVector, kTypicalNbPrivateAccounts>; -using ConversionPathPerExchange = FixedCapacityVector, kNbSupportedExchanges>; +using ConversionPathPerExchange = FixedCapacityVector, kNbSupportedExchanges>; } // namespace cct \ No newline at end of file diff --git a/src/engine/include/stringoptionparser.hpp b/src/engine/include/stringoptionparser.hpp index 04e09c9e..92fb54a2 100644 --- a/src/engine/include/stringoptionparser.hpp +++ b/src/engine/include/stringoptionparser.hpp @@ -3,6 +3,7 @@ #include #include +#include "cct_string.hpp" #include "cct_vector.hpp" #include "currencycode.hpp" #include "exchangename.hpp" @@ -51,7 +52,7 @@ class StringOptionParser { CurrenciesPublicExchanges getCurrenciesPublicExchanges() const; - vector getCSVValues() const; + vector getCSVValues() const; protected: std::size_t getNextCommaPos(std::size_t startPos = 0, bool throwIfNone = true) const; diff --git a/src/engine/src/coincenter.cpp b/src/engine/src/coincenter.cpp index cabb7327..2c3fce5a 100644 --- a/src/engine/src/coincenter.cpp +++ b/src/engine/src/coincenter.cpp @@ -1,16 +1,26 @@ #include "coincenter.hpp" #include -#include -#include +#include +#include #include -#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 diff --git a/src/engine/src/coincentercommands.cpp b/src/engine/src/coincentercommands.cpp index c9abd070..26c49408 100644 --- a/src/engine/src/coincentercommands.cpp +++ b/src/engine/src/coincentercommands.cpp @@ -76,12 +76,7 @@ namespace { std::pair ParseOrderRequest(const CoincenterCmdLineOptions &cmdLineOptions, std::string_view orderRequestStr) { auto currenciesPrivateExchangesTuple = StringOptionParser(orderRequestStr).getCurrenciesPrivateExchanges(false); - auto orderIdViewVector = StringOptionParser(cmdLineOptions.ids).getCSVValues(); - vector 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(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 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(cmdLineOptions.minAge), std::chrono::duration_cast(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 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(cmdLineOptions.minAge), std::chrono::duration_cast(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)); } diff --git a/src/engine/src/stringoptionparser.cpp b/src/engine/src/stringoptionparser.cpp index 14717f2a..1d3cddf7 100644 --- a/src/engine/src/stringoptionparser.cpp +++ b/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 +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 StringOptionParser::getCSVValues() const { +vector StringOptionParser::getCSVValues() const { std::size_t pos = 0; - vector ret; + vector ret; if (!_opt.empty()) { do { std::size_t nextCommaPos = getNextCommaPos(pos, false); diff --git a/src/engine/test/stringoptionparser_test.cpp b/src/engine/test/stringoptionparser_test.cpp index 31fbaf8c..0d88fbc0 100644 --- a/src/engine/test/stringoptionparser_test.cpp +++ b/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()); - EXPECT_EQ(StringOptionParser("val1,").getCSVValues(), vector{{"val1"}}); - EXPECT_EQ(StringOptionParser("val1,value").getCSVValues(), vector({{"val1"}, {"value"}})); + EXPECT_EQ(StringOptionParser("").getCSVValues(), vector()); + EXPECT_EQ(StringOptionParser("val1,").getCSVValues(), vector{{"val1"}}); + EXPECT_EQ(StringOptionParser("val1,value").getCSVValues(), vector({{"val1"}, {"value"}})); } } // namespace cct \ No newline at end of file diff --git a/src/monitoring/include/abstractmetricgateway.hpp b/src/monitoring/include/abstractmetricgateway.hpp index e5eefa6e..3c0a841d 100644 --- a/src/monitoring/include/abstractmetricgateway.hpp +++ b/src/monitoring/include/abstractmetricgateway.hpp @@ -11,7 +11,7 @@ class AbstractMetricGateway { public: using BucketBoundaries = std::span; - virtual ~AbstractMetricGateway() {} + virtual ~AbstractMetricGateway() = default; /// Register some numeric value virtual void add(MetricType metricType, MetricOperation op, const MetricKey& key, double v = 0) = 0; diff --git a/src/monitoring/include/prometheusmetricgateway.hpp b/src/monitoring/include/prometheusmetricgateway.hpp index c7267d56..6be53b15 100644 --- a/src/monitoring/include/prometheusmetricgateway.hpp +++ b/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 _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 diff --git a/src/monitoring/src/prometheusmetricgateway.cpp b/src/monitoring/src/prometheusmetricgateway.cpp index 08fcefa5..81771c0a 100644 --- a/src/monitoring/src/prometheusmetricgateway.cpp +++ b/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()), - _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(Clock::now() - nowTime).count()); + std::chrono::duration_cast(Clock::now() - nowTime).count()); } else { log::error("Unable to push metrics to Prometheus instance - Bad return code {}", returnCode); } diff --git a/src/objects/src/exchangename.cpp b/src/objects/src/exchangename.cpp index 6379e6bd..df17a265 100644 --- a/src/objects/src/exchangename.cpp +++ b/src/objects/src/exchangename.cpp @@ -4,11 +4,15 @@ #include #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('_');