Add cryptocom exchange connector - #5330
Conversation
timmolter
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the module structure follows the XChange conventions closely (Raw/adapter split, fixture-based DTO tests, disabled integration tests, CI workflow wiring), and the signing/secret handling looks clean. I left five inline notes: two bugs that affect every private call / metadata load (error-body deserialization, perp instruments leaking into spot metadata), a likely rejection of market BUY orders, a wrong-network risk in requestDepositAddress, and a bundle of smaller correctness items.
|
|
||
| private final int code; | ||
|
|
||
| public CryptoComException(int code, String message) { |
There was a problem hiding this comment.
Jackson can't bind Crypto.com's error body ({"code":..., "message":...}) into this constructor without @JsonProperty annotations, so rescu can't instantiate this exception for non-2xx responses. Compare the convention used elsewhere in the repo, e.g. BinanceException:
public CryptoComException(
@JsonProperty("code") int code, @JsonProperty("message") String message) {This matters more than it looks: Crypto.com returns most errors (auth failures, bad params, rate limits) with a non-2xx HTTP status, which takes the rescu-exception path — not the CryptoComErrorInterceptor path, which only sees code != 0 inside HTTP-200 envelopes. As written, those errors surface as generic deserialization/HttpStatusIOException failures and the CryptoComErrorAdapter mapping (FundsExceeded/RateLimit/Nonce) rarely, if ever, fires. Consider also routing rescu-thrown CryptoComExceptions through CryptoComErrorAdapter so both paths produce the same XChange exceptions. A WireMock test for a 4xx error body (WireMock is already in the POM) would lock this down.
|
|
||
| if (instruments != null) { | ||
| for (CryptoComInstrument instrument : instruments) { | ||
| if (!"CCY_PAIR".equals(instrument.getInstType()) && instrument.getBaseCurrency() == null) { |
There was a problem hiding this comment.
This condition only skips instruments that are both non-spot and missing a base currency — should this be ||?
A PERPETUAL_SWAP like 1INCHUSD-PERP (base 1INCH, quote USD — present in this PR's own get-instruments.json fixture) passes the filter and gets registered as CurrencyPair 1INCH/USD with perp metadata (quantity_decimals: 0, qty_tick_size: 1). On the live instrument list, where the spot pair and the perp both exist, one silently clobbers the other's InstrumentMetaData depending on iteration order.
if (!"CCY_PAIR".equals(instrument.getInstType()) || instrument.getBaseCurrency() == null) {
continue;
}A unit test feeding adaptExchangeMetaData a mixed spot + perp list (the fixture already has one) would catch this.
| : CryptoComOrderSide.SELL, | ||
| CryptoComOrderType.MARKET, | ||
| null, | ||
| marketOrder.getOriginalAmount().toPlainString(), |
There was a problem hiding this comment.
Crypto.com v1 private/create-order requires notional (quote amount) for MARKET BUY orders — quantity is only accepted for SELL and limit orders — so BID market orders sent with quantity should be rejected server-side. The only test covering this path (placeMarketOrder_shouldSucceed) is doubly @Disabled, so this wouldn't show up in CI.
Suggest verifying against sandbox and, if confirmed, sending notional for MARKET BUY (e.g. derived from MarketOrder or by documenting that market BUY needs the quote amount).
| if (addresses.isEmpty()) { | ||
| throw new NotAvailableFromExchangeException("No deposit address found for " + currency); | ||
| } | ||
| return addresses.get(0).getAddress(); |
There was a problem hiding this comment.
deposit_address_list can contain one address per network (USDT: ERC20/TRC20/CRO, ...), so get(0) returns an arbitrary network's address. A user depositing to the returned address on the wrong chain loses funds — this is the riskiest spot in the PR after the error-mapping issue.
The String... args varargs is available for a network filter; suggest filtering by network when provided (and ideally only returning status == ACTIVE addresses), or at least documenting which address is returned so callers know to use the raw getCryptoComDepositAddresses for multi-network currencies.
| args != null && args.length > 0 && args[0] instanceof Integer ? (Integer) args[0] : null; | ||
| return CryptoComAdapters.adaptOrderBook( | ||
| getCryptoComOrderBook(CryptoComAdapters.toInstrumentName(instrument), depth), | ||
| (CurrencyPair) instrument); |
There was a problem hiding this comment.
Blind cast to (CurrencyPair) here (and in getTrades below) throws ClassCastException for non-pair instruments — the streaming CryptoComStreamingMarketDataService checks instanceof first; suggest making the REST side consistent.
Bundling a few related minor items:
CryptoComBaseService.buildRequestdoesn't check for missing API key/secret — the failure is a bare NPE insideCryptoComDigest.hmacSha256Hex. A guard with a clear "API credentials required" message is the usual pattern.CryptoComPrivateStreamingService: an auth failure is onlyLOG.error'd, souser.*subscribers hang forever with no error signal. Consider erroring the pending subscriptions or closing the socket so reconnect logic kicks in.toCurrencyPairreturnsnullfor derivative names likeWALUSD-PERP, sogetTickers()(which returns all tickers including derivatives) emitsTickers with a null instrument — filtering non-spot entries would be cleaner.withdrawFundsrequiresNetworkWithdrawFundsParamseven thoughnetwork_idis optional in the raw call — plainDefaultWithdrawFundsParamscould be supported.CryptoComDigest.formatValue(null)returns"", while Crypto.com's reference implementation encodes null param values as the literal string"null". No current caller passes nulls, but it's a signature-mismatch trap — worth aligning, and a known-answer test forCryptoComDigestagainst the docs' signature example would lock the algorithm down.
No description provided.