[bug] Refactor suggestion to prevent panics in CMC symbol CSV function

[Unique-Divine]

🛠️ Refactor suggestion

⚠️ Potential issue

Panic on empty input set in coinmarketcapSymbolCsv.

coinmarketcapSymbolCsv builds a CSV by appending a trailing comma and slicing s[:len(s)-1]. If symbols is empty, this slices s[: -1] and panics.

Suggested fix (handle empty set and avoid trailing comma):

// Add: import "strings"

func coinmarketcapSymbolCsv(symbols set.Set[types.Symbol]) string {
  if len(symbols) == 0 {
    return ""
  }
  var b strings.Builder
  first := true
  for symbol := range symbols {
    if !first {
      b.WriteByte(',')
    }
    b.WriteString(string(symbol))
    first = false
  }
  return b.String()
}

Optionally, fail-fast in CoinmarketcapPriceUpdate when no symbols are provided:

if len(symbols) == 0 {
  metrics.PriceSourceCounter.WithLabelValues(CoinMarketCap, "false").Inc()
  return nil, fmt.Errorf("no symbols provided")
}

🤖 Prompt for AI Agents

In feeder/priceprovider/sources/coinmarketcap.go around line 11,
coinmarketcapSymbolCsv currently appends a trailing comma and slices off the
last byte which panics when the input set is empty; change the function to
handle len(symbols)==0 by returning an empty string, use strings.Builder to
build the CSV without a trailing comma (write commas only between elements) and
add the "strings" import; additionally consider adding a fail-fast check in
CoinmarketcapPriceUpdate to return an error (and update metrics) if
len(symbols)==0 so the caller fails early.

---

🛠️ Refactor suggestion

Add HTTP timeout, status-code checks, API key validation, and improve error/log handling.

Currently:

• http.Client has no timeout → potential hangs.
• No status code checks → 4xx/5xx treated as success until parsing fails.
• API key not validated → silent empty header.
• buildReq error is not wrapped and has style issues; avoid newlines and capitalize.
• Logging uses logger.Err(err) with err potentially nil in getPricesFromResponse when a symbol is missing.

Recommendations:

• Use a client timeout (align with types.PriceTimeout).

client := &http.Client{Timeout: types.PriceTimeout}

• Validate API key once:

if c == nil || c.ApiKey == "" {
  return nil, fmt.Errorf("coinmarketcap api key is required")
}

• Check HTTP status codes:

if res.StatusCode != http.StatusOK {
  body, _ := io.ReadAll(res.Body)
  metrics.PriceSourceCounter.WithLabelValues(CoinMarketCap, "false").Inc()
  return nil, fmt.Errorf("coinmarketcap non-200 status: %d body=%s", res.StatusCode, string(body))
}

• Wrap and style buildReq errors:

return nil, fmt.Errorf("cannot create request to %s: %w", link, err)

• Replace logger.Err(err) on missing symbol with an explicit log without a nil error:

logger.Warn().
  Str("source", CoinMarketCap).
  Str("symbol", string(symbol)).
  Msg("missing price for symbol in CoinMarketCap response")

🤖 Prompt for AI Agents

In feeder/priceprovider/sources/coinmarketcap.go around line 11, the HTTP
client, request building, API-key handling, response status handling, and
logging need hardening: set the http.Client timeout to types.PriceTimeout;
validate c != nil and c.ApiKey != "" early and return a clear error if missing;
after performing the request check res.StatusCode == http.StatusOK, read the
body for context and increment metrics.PriceSourceCounter on non-200 responses
before returning a wrapped error; wrap buildReq errors with a single-line
fmt.Errorf("cannot create request to %s: %w", link, err) (capitalize and avoid
newlines); and replace logger.Err(err) when a symbol is missing with an explicit
logger.Warn() message that does not pass a nil error and includes source and
symbol fields.

Originally posted by @coderabbitai[bot] in #67 (comment)