From e0f5cecfc5f5ce0d3703335a608e0bb8c0bf33da Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 31 Mar 2026 07:24:00 +0000 Subject: [PATCH 1/2] docs: add error handling proposal Proposes a unified ProviderError type with ErrorKind categorization, preserving provider-specific codes while enabling programmatic handling. Six-phase incremental migration plan, each phase independently buildable. https://claude.ai/code/session_0115EjMcwabSsDr869GXPvFi --- _docs/error-handling-proposal.md | 300 +++++++++++++++++++++++++++++++ 1 file changed, 300 insertions(+) create mode 100644 _docs/error-handling-proposal.md diff --git a/_docs/error-handling-proposal.md b/_docs/error-handling-proposal.md new file mode 100644 index 0000000..0d1ef6f --- /dev/null +++ b/_docs/error-handling-proposal.md @@ -0,0 +1,300 @@ +# Error Handling Proposal + +## Overview + +This document proposes a unified, ergonomic error handling strategy for `bits`. The current state uses `fmt.Errorf` strings throughout, which loses semantic information, makes programmatic handling impossible, and leaks provider internals inconsistently. + +Two goals drive this proposal: +1. **Callers can act on errors** — distinguish auth failures from rate limits from network blips without string parsing. +2. **Provider specificity is preserved** — the raw provider code/message is always accessible, never silently dropped. + +--- + +## Part 1 — Error Handling Structures + +### 1.1 Error Categories + +A single `ErrorKind` type covers all recoverable and unrecoverable conditions across every provider: + +```go +// pkg/model/errors.go + +type ErrorKind int + +const ( + ErrKindUnknown ErrorKind = iota // catch-all + ErrKindAuth // 401 / 403 / invalid API key + ErrKindRateLimit // 429 / quota exceeded + ErrKindNotFound // 404 / symbol not found + ErrKindInvalidRequest // 400 / bad parameters + ErrKindServerError // 5xx / provider-side failure + ErrKindNetwork // connection refused, timeout, DNS + ErrKindParse // unexpected response shape + ErrKindUnsupportedMarket // already exists, kept for compat + ErrKindUnsupportedFeature // already exists, kept for compat +) + +func (k ErrorKind) Retryable() bool { + return k == ErrKindRateLimit || k == ErrKindServerError || k == ErrKindNetwork +} +``` + +**Mapping rules** (applied by each provider's HTTP client): + +| Condition | Kind | +|---|---| +| HTTP 401, 403 | `ErrKindAuth` | +| HTTP 429 | `ErrKindRateLimit` | +| HTTP 404 | `ErrKindNotFound` | +| HTTP 400 | `ErrKindInvalidRequest` | +| HTTP 5xx | `ErrKindServerError` | +| `net.Error` (timeout/refused) | `ErrKindNetwork` | +| JSON unmarshal failure | `ErrKindParse` | +| API-body code ≠ success | kind inferred from code; else `ErrKindUnknown` | + +### 1.2 The `ProviderError` Type + +One concrete type is used everywhere an error crosses a provider boundary: + +```go +// pkg/model/errors.go + +type ProviderError struct { + // Normalized — callers use these without knowing which provider. + Kind ErrorKind + ProviderID string // "binance", "bitget", … + + // Raw — always preserved, never dropped. + ProviderCode string // provider's native error code ("00000", "400", …) + ProviderMessage string // provider's native message text + + // Optional HTTP layer information. + HTTPStatus int // 0 when not an HTTP error + + // Underlying cause (network error, json error, …). + Cause error +} + +func (e *ProviderError) Error() string { + if e.ProviderCode != "" { + return fmt.Sprintf("[%s] %s (code %s)", e.ProviderID, e.ProviderMessage, e.ProviderCode) + } + if e.HTTPStatus != 0 { + return fmt.Sprintf("[%s] HTTP %d: %s", e.ProviderID, e.HTTPStatus, e.ProviderMessage) + } + return fmt.Sprintf("[%s] %s", e.ProviderID, e.ProviderMessage) +} + +func (e *ProviderError) Unwrap() error { return e.Cause } +``` + +**Usage at the call site:** + +```go +res, err := client.GetPrice(ctx, "BTCUSDT", "binance") +if err != nil { + var pe *model.ProviderError + if errors.As(err, &pe) { + switch pe.Kind { + case model.ErrKindRateLimit: + time.Sleep(backoff) + // retry + case model.ErrKindAuth: + // surface config problem to user + } + // always available for logging / telemetry: + log.Printf("provider=%s code=%s http=%d", pe.ProviderID, pe.ProviderCode, pe.HTTPStatus) + } +} +``` + +**Provider-specific code access** is explicit and zero-surprise: `pe.ProviderCode` is the raw string the exchange returned (`"00000"`, `"10001"`, `"429"`, …), and `pe.ProviderMessage` is its description verbatim. + +### 1.3 Sentinel Errors (keep backward-compat) + +The two existing sentinels are promoted into `ProviderError` instances and keep their original `errors.Is` surface: + +```go +var ( + ErrUnsupportedMarket = &ProviderError{Kind: ErrKindUnsupportedMarket, ProviderMessage: "unsupported market type"} + ErrUnsupportedFeature = &ProviderError{Kind: ErrKindUnsupportedFeature, ProviderMessage: "unsupported feature"} +) +``` + +Existing `errors.Is(err, model.ErrUnsupportedMarket)` checks continue to work because `ProviderError` equality is by pointer identity for these package-level values. + +### 1.4 `ItemError` — partial failure in batch calls + +`ItemError` gains a typed `Err` so JSON serialisation is meaningful and callers can programmatically inspect per-symbol failures: + +```go +// pkg/model/response.go + +type ItemError struct { + Symbol string `json:"sym"` + Err *ProviderError `json:"err"` // typed — always a *ProviderError +} +``` + +Changing from `error` to `*ProviderError` is the only breaking change in the public model. It allows: + +```go +for _, ie := range res.Errors { + if ie.Err.Kind == model.ErrKindNotFound { + // skip missing symbols gracefully + } +} +``` + +### 1.5 Provider constructor helpers + +Each provider package exposes a small unexported helper so error construction is consistent and never duplicated: + +```go +// pkg/provider/binance/errors.go (one per provider) + +func providerErr(kind model.ErrorKind, msg string, cause error) *model.ProviderError { + return &model.ProviderError{ + Kind: kind, + ProviderID: ProviderID, // "binance" + ProviderMessage: msg, + Cause: cause, + } +} + +func httpErr(status int, body string) *model.ProviderError { + kind := httpStatusToKind(status) + return &model.ProviderError{ + Kind: kind, + ProviderID: ProviderID, + HTTPStatus: status, + ProviderMessage: body, + } +} + +func apiErr(code, msg string) *model.ProviderError { + return &model.ProviderError{ + Kind: apiCodeToKind(code), // provider-specific mapping + ProviderID: ProviderID, + ProviderCode: code, + ProviderMessage: msg, + } +} +``` + +A shared `httpStatusToKind(status int) ErrorKind` utility lives in `pkg/model/errors.go` to keep the HTTP→Kind mapping DRY across all providers. + +### 1.6 What is NOT in scope + +- Retry logic — out of scope; callers use `Retryable()` to decide. +- Circuit breaking — out of scope. +- Logging inside providers — providers return errors, they do not log them. +- Wrapping third-party library errors (e.g. `go-binance/v2`) beyond the single `Cause` field — the library's error becomes `Cause`; `ProviderMessage` is set from it. + +--- + +## Part 2 — Transition + +The transition is additive first, then breaking. It can be merged incrementally without breaking the CLI. + +### Phase 1 — Foundation (`pkg/model`) + +**Files:** `pkg/model/errors.go`, `pkg/model/response.go` + +1. Add `ErrorKind`, `ProviderError`, and `httpStatusToKind` to `errors.go`. +2. Keep the two existing sentinel vars; re-declare them as `*ProviderError` values. +3. **Do not** change `ItemError` yet — leave it `error` to avoid a cascade of compile errors. + +Result: the new types exist, nothing is broken, no providers use them yet. + +--- + +### Phase 2 — Provider HTTP clients + +**Files:** `pkg/provider/*/client.go` (all six providers) + +For each provider: + +1. Add a small `errors.go` file in the provider package with `providerErr`, `httpErr`, `apiErr` helpers. +2. Update `client.go` (or equivalent HTTP execution path) to: + - Check `resp.StatusCode` after every HTTP call (currently missing in Bitget, WhiteBit, Crypto.com). + - Return `httpErr(status, body)` on non-2xx instead of `fmt.Errorf`. + - Wrap `net.Error` / `context` errors with `providerErr(ErrKindNetwork, …, err)`. +3. Update market/exchange method files (`market.go`, `exchange.go`) to: + - Return `apiErr(code, msg)` when the response envelope signals failure (e.g. Bitget `code != "00000"`, Crypto.com `code != 0`). + +No interface changes yet — all methods still return `(model.Response[T], error)`. + +Provider-specific code→kind mappings go into each provider's `errors.go`. Examples: + +| Provider | Success signal | Auth codes | Rate-limit codes | +|---|---|---|---| +| Bitget | `code == "00000"` | `40001`, `40003` | `429xx` series | +| Crypto.com | `code == 0` | `40001` | `10006` | +| Binance | HTTP 200 | HTTP 401/403 | HTTP 429 | +| CoinGecko | HTTP 200 | HTTP 401/403 | HTTP 429 | +| MEXC | HTTP 200 | HTTP 401/403 | HTTP 429 | +| WhiteBit | `success == true` | HTTP 401/403 | HTTP 429 | + +--- + +### Phase 3 — `ItemError` migration + +**Files:** `pkg/model/response.go`, all call sites that construct `ItemError` + +Change `ItemError.Err` from `error` to `*model.ProviderError`. + +Update every construction site (fanout, provider market files) to always pass a `*ProviderError`. Any site that currently creates a raw `fmt.Errorf` must be wrapped in the provider's `providerErr` helper first. + +This is the only breaking change for library consumers. Callers that stored `ItemError.Err` as `error` continue to work via the `error` interface; callers that did type assertions need updating. + +--- + +### Phase 4 — Resolver and FanOut + +**Files:** `pkg/resolve/resolver.go`, `pkg/resolve/fanout.go` + +- `resolver.go`: replace `fmt.Errorf("no provider supports …")` with `&model.ProviderError{Kind: ErrKindUnsupportedFeature, …}`. No `ProviderID` here since the error is from the resolution layer, not a specific provider. +- `fanout.go`: no structural change needed — `ItemError` already accepts the new type from Phase 3. + +--- + +### Phase 5 — Public `pkg/bits` facade + +**Files:** `pkg/bits/client.go` + +The facade is what external consumers import, so this phase ensures the public surface is clean: + +1. `GetPrice`, `ComparePrices`, etc. already return `(model.Response[T], error)` — no signature change. +2. Document in godoc that returned errors are always `*model.ProviderError` when they originate from a provider, so callers can use `errors.As`. +3. The "no data returned" fallback path also wraps with `*model.ProviderError{Kind: ErrKindNotFound, …}` instead of `fmt.Errorf`. + +--- + +### Phase 6 — CLI layer (optional cleanup) + +**Files:** `cmd/*.go` + +The CLI commands currently print `err.Error()` on failure. After the transition, they can optionally surface the `Kind` for better UX (e.g. "authentication failed — check your API key in config") without breaking anything. + +This phase is optional and can be deferred. + +--- + +### Migration checklist + +``` +[ ] Phase 1: pkg/model — ErrorKind, ProviderError, httpStatusToKind +[ ] Phase 2a: pkg/provider/binance — errors.go + client/market updates +[ ] Phase 2b: pkg/provider/bitget — errors.go + client/market updates +[ ] Phase 2c: pkg/provider/coingecko — errors.go + client/market updates +[ ] Phase 2d: pkg/provider/cryptocom — errors.go + client/market updates +[ ] Phase 2e: pkg/provider/mexc — errors.go + client/market updates +[ ] Phase 2f: pkg/provider/whitebit — errors.go + client/market updates +[ ] Phase 3: ItemError.Err → *ProviderError +[ ] Phase 4: resolver + fanout +[ ] Phase 5: pkg/bits facade + godoc +[ ] Phase 6: (optional) CLI human-readable error messages +``` + +Each phase compiles and passes tests independently. Phases 2a–2f are fully parallel. From 85b07409f33deaa1c701892136cf22e229b8488d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 31 Mar 2026 07:30:26 +0000 Subject: [PATCH 2/2] docs: revise error handling proposal based on review - Add Is() method to ProviderError for correct errors.Is sentinel compat - Add ErrKindCanceled to distinguish context cancellation from network errors - Move httpStatusToKind out of pkg/model into per-provider errors.go - Add WrapError() escape hatch for migration safety - Note CoinGecko's different client pattern (get() vs doRequest) - Gate Phase 3 explicitly on full Phase 2 completion - Fix Retryable() note: 5xx retryable but callers must backoff/cap retries - Cover ComparePrices facade ItemError construction in Phase 5 - Add test requirement to Phase 2 checklist https://claude.ai/code/session_0115EjMcwabSsDr869GXPvFi --- _docs/error-handling-proposal.md | 211 +++++++++++++++++++++---------- 1 file changed, 145 insertions(+), 66 deletions(-) diff --git a/_docs/error-handling-proposal.md b/_docs/error-handling-proposal.md index 0d1ef6f..a514d53 100644 --- a/_docs/error-handling-proposal.md +++ b/_docs/error-handling-proposal.md @@ -27,13 +27,17 @@ const ( ErrKindRateLimit // 429 / quota exceeded ErrKindNotFound // 404 / symbol not found ErrKindInvalidRequest // 400 / bad parameters - ErrKindServerError // 5xx / provider-side failure - ErrKindNetwork // connection refused, timeout, DNS + ErrKindServerError // 502/503/504 / provider-side transient failure + ErrKindNetwork // connection refused, DNS failure + ErrKindCanceled // context.Canceled or context.DeadlineExceeded ErrKindParse // unexpected response shape ErrKindUnsupportedMarket // already exists, kept for compat ErrKindUnsupportedFeature // already exists, kept for compat ) +// Retryable reports whether errors of this kind are worth retrying with backoff. +// Callers are responsible for applying exponential backoff; this method only +// signals intent. func (k ErrorKind) Retryable() bool { return k == ErrKindRateLimit || k == ErrKindServerError || k == ErrKindNetwork } @@ -47,11 +51,17 @@ func (k ErrorKind) Retryable() bool { | HTTP 429 | `ErrKindRateLimit` | | HTTP 404 | `ErrKindNotFound` | | HTTP 400 | `ErrKindInvalidRequest` | -| HTTP 5xx | `ErrKindServerError` | -| `net.Error` (timeout/refused) | `ErrKindNetwork` | +| HTTP 502, 503, 504 | `ErrKindServerError` | +| HTTP 500 (and other 5xx) | `ErrKindServerError` | +| `net.Error` (refused, DNS) | `ErrKindNetwork` | +| `context.Canceled` / `context.DeadlineExceeded` | `ErrKindCanceled` | | JSON unmarshal failure | `ErrKindParse` | | API-body code ≠ success | kind inferred from code; else `ErrKindUnknown` | +> **Note on 5xx and retryability:** All 5xx are marked `ErrKindServerError` and `Retryable() == true`. In practice, callers should apply exponential backoff and a maximum attempt count. A persistent 500 that never clears is a provider bug; retry logic should give up eventually regardless of this flag. + +> **Note on context errors:** `ErrKindCanceled` is intentionally excluded from `Retryable()`. A canceled context means the caller no longer wants the result; retrying would be wrong. + ### 1.2 The `ProviderError` Type One concrete type is used everywhere an error crosses a provider boundary: @@ -62,7 +72,7 @@ One concrete type is used everywhere an error crosses a provider boundary: type ProviderError struct { // Normalized — callers use these without knowing which provider. Kind ErrorKind - ProviderID string // "binance", "bitget", … + ProviderID string // "binance", "bitget", … empty for resolution-layer errors // Raw — always preserved, never dropped. ProviderCode string // provider's native error code ("00000", "400", …) @@ -82,10 +92,29 @@ func (e *ProviderError) Error() string { if e.HTTPStatus != 0 { return fmt.Sprintf("[%s] HTTP %d: %s", e.ProviderID, e.HTTPStatus, e.ProviderMessage) } - return fmt.Sprintf("[%s] %s", e.ProviderID, e.ProviderMessage) + if e.ProviderID != "" { + return fmt.Sprintf("[%s] %s", e.ProviderID, e.ProviderMessage) + } + return e.ProviderMessage } func (e *ProviderError) Unwrap() error { return e.Cause } + +// Is implements errors.Is matching by Kind for sentinel comparisons. +// This ensures errors.Is(err, model.ErrUnsupportedMarket) keeps working even +// when the error is a freshly constructed *ProviderError rather than the exact +// sentinel pointer. +func (e *ProviderError) Is(target error) bool { + t, ok := target.(*ProviderError) + if !ok { + return false + } + // Match sentinels by Kind when the target has no ProviderID (i.e. is a sentinel). + if t.ProviderID == "" { + return e.Kind == t.Kind + } + return e == t +} ``` **Usage at the call site:** @@ -101,6 +130,8 @@ if err != nil { // retry case model.ErrKindAuth: // surface config problem to user + case model.ErrKindCanceled: + return // caller canceled, do not retry } // always available for logging / telemetry: log.Printf("provider=%s code=%s http=%d", pe.ProviderID, pe.ProviderCode, pe.HTTPStatus) @@ -110,9 +141,9 @@ if err != nil { **Provider-specific code access** is explicit and zero-surprise: `pe.ProviderCode` is the raw string the exchange returned (`"00000"`, `"10001"`, `"429"`, …), and `pe.ProviderMessage` is its description verbatim. -### 1.3 Sentinel Errors (keep backward-compat) +### 1.3 Sentinel Errors (backward-compat) -The two existing sentinels are promoted into `ProviderError` instances and keep their original `errors.Is` surface: +The two existing sentinels are promoted into `*ProviderError` values. The `Is` method on §1.2 ensures `errors.Is(err, model.ErrUnsupportedMarket)` keeps working for any `*ProviderError` with `Kind == ErrKindUnsupportedMarket`, not just the exact pointer — so wrapped errors and freshly constructed errors both match. ```go var ( @@ -121,22 +152,24 @@ var ( ) ``` -Existing `errors.Is(err, model.ErrUnsupportedMarket)` checks continue to work because `ProviderError` equality is by pointer identity for these package-level values. +Existing `errors.Is` call sites require no change. `errors.As` call sites that then inspect `.Kind` also work without change. ### 1.4 `ItemError` — partial failure in batch calls -`ItemError` gains a typed `Err` so JSON serialisation is meaningful and callers can programmatically inspect per-symbol failures: +`ItemError` gains a typed `Err` so JSON serialisation is meaningful and callers can programmatically inspect per-symbol failures. + +> **Pre-existing issue fixed by this change:** `ItemError.Err` is currently typed `error` (an interface), which marshals to `{}` in JSON — effectively invisible. Changing to `*ProviderError` fixes serialisation as a side effect. ```go // pkg/model/response.go type ItemError struct { Symbol string `json:"sym"` - Err *ProviderError `json:"err"` // typed — always a *ProviderError + Err *ProviderError `json:"err"` // typed — always a *ProviderError } ``` -Changing from `error` to `*ProviderError` is the only breaking change in the public model. It allows: +This is the only breaking change in the public model. It allows: ```go for _, ie := range res.Errors { @@ -148,10 +181,10 @@ for _, ie := range res.Errors { ### 1.5 Provider constructor helpers -Each provider package exposes a small unexported helper so error construction is consistent and never duplicated: +Each provider package has a small unexported `errors.go` with construction helpers. The `httpStatusToKind` mapping lives here too — **not** in `pkg/model`, which must remain HTTP-agnostic. ```go -// pkg/provider/binance/errors.go (one per provider) +// pkg/provider/binance/errors.go (pattern repeated per provider) func providerErr(kind model.ErrorKind, msg string, cause error) *model.ProviderError { return &model.ProviderError{ @@ -163,9 +196,8 @@ func providerErr(kind model.ErrorKind, msg string, cause error) *model.ProviderE } func httpErr(status int, body string) *model.ProviderError { - kind := httpStatusToKind(status) return &model.ProviderError{ - Kind: kind, + Kind: httpStatusToKind(status), ProviderID: ProviderID, HTTPStatus: status, ProviderMessage: body, @@ -180,52 +212,97 @@ func apiErr(code, msg string) *model.ProviderError { ProviderMessage: msg, } } + +// httpStatusToKind is local to each provider package (not in pkg/model). +func httpStatusToKind(status int) model.ErrorKind { + switch { + case status == 401 || status == 403: + return model.ErrKindAuth + case status == 404: + return model.ErrKindNotFound + case status == 429: + return model.ErrKindRateLimit + case status == 400: + return model.ErrKindInvalidRequest + case status >= 500: + return model.ErrKindServerError + default: + return model.ErrKindUnknown + } +} ``` -A shared `httpStatusToKind(status int) ErrorKind` utility lives in `pkg/model/errors.go` to keep the HTTP→Kind mapping DRY across all providers. +> **CoinGecko note:** CoinGecko's `client.go` uses a `get(ctx, path, &result)` helper that decodes JSON internally and returns a plain `error`. It does not follow the two-step fetch-then-unmarshal pattern of the other five providers. Its `errors.go` helpers still apply, but the integration point is inside `get()` rather than a separate `doRequest` method. No structural change to the client is needed — only the error return sites change. + +### 1.6 Migration safety valve — `WrapError` + +During the transition, some construction sites (e.g. registry failures in the facade, third-party library errors from `go-binance/v2`) may not yet produce a `*ProviderError`. A single escape-hatch function prevents compile errors and leaves an audit trail: + +```go +// pkg/model/errors.go -### 1.6 What is NOT in scope +// WrapError wraps an arbitrary error as ErrKindUnknown. Use only as a +// temporary shim during migration; replace with a typed providerErr call. +func WrapError(providerID string, err error) *ProviderError { + if err == nil { + return nil + } + // If already a ProviderError, preserve it. + var pe *ProviderError + if errors.As(err, &pe) { + return pe + } + return &ProviderError{ + Kind: ErrKindUnknown, + ProviderID: providerID, + ProviderMessage: err.Error(), + Cause: err, + } +} +``` -- Retry logic — out of scope; callers use `Retryable()` to decide. +### 1.7 What is NOT in scope + +- Retry logic — callers use `Retryable()` to decide; `bits` does not retry internally. - Circuit breaking — out of scope. - Logging inside providers — providers return errors, they do not log them. -- Wrapping third-party library errors (e.g. `go-binance/v2`) beyond the single `Cause` field — the library's error becomes `Cause`; `ProviderMessage` is set from it. +- Wrapping third-party library errors (e.g. `go-binance/v2`) beyond the `Cause` field — the library error becomes `Cause`; `ProviderMessage` is set from its `.Error()` string. --- ## Part 2 — Transition -The transition is additive first, then breaking. It can be merged incrementally without breaking the CLI. +The transition is additive first, then breaking. Each phase compiles and passes tests independently. ### Phase 1 — Foundation (`pkg/model`) **Files:** `pkg/model/errors.go`, `pkg/model/response.go` -1. Add `ErrorKind`, `ProviderError`, and `httpStatusToKind` to `errors.go`. -2. Keep the two existing sentinel vars; re-declare them as `*ProviderError` values. -3. **Do not** change `ItemError` yet — leave it `error` to avoid a cascade of compile errors. +1. Add `ErrorKind`, `ProviderError` (with `Is` and `Unwrap`), and `WrapError` to `errors.go`. +2. Re-declare the two sentinel vars as `*ProviderError` values. +3. **Do not** change `ItemError` yet — leave `Err` typed as `error` to avoid a cascade of compile errors. -Result: the new types exist, nothing is broken, no providers use them yet. +Result: new types exist, nothing broken, no providers use them yet. --- -### Phase 2 — Provider HTTP clients +### Phase 2 — Provider HTTP clients (parallel, one PR per provider) -**Files:** `pkg/provider/*/client.go` (all six providers) +**Files:** `pkg/provider/*/client.go`, `pkg/provider/*/market.go`, `pkg/provider/*/exchange.go` -For each provider: +> Phases 2a–2f are fully independent and can land in any order. -1. Add a small `errors.go` file in the provider package with `providerErr`, `httpErr`, `apiErr` helpers. -2. Update `client.go` (or equivalent HTTP execution path) to: - - Check `resp.StatusCode` after every HTTP call (currently missing in Bitget, WhiteBit, Crypto.com). - - Return `httpErr(status, body)` on non-2xx instead of `fmt.Errorf`. - - Wrap `net.Error` / `context` errors with `providerErr(ErrKindNetwork, …, err)`. -3. Update market/exchange method files (`market.go`, `exchange.go`) to: - - Return `apiErr(code, msg)` when the response envelope signals failure (e.g. Bitget `code != "00000"`, Crypto.com `code != 0`). +For each provider: -No interface changes yet — all methods still return `(model.Response[T], error)`. +1. Add `pkg/provider//errors.go` with `providerErr`, `httpErr`, `apiErr`, `httpStatusToKind`, and `apiCodeToKind`. +2. Update the HTTP execution path to check `resp.StatusCode` on every call and return `httpErr` on non-2xx. + - **Bug fix:** Bitget, WhiteBit, and Crypto.com currently do not check the HTTP status code at all — a 500 response body is silently passed to `json.Unmarshal`. This change fixes that behavior; add regression tests. +3. Wrap `net.Error` and connection errors with `providerErr(ErrKindNetwork, …, err)`. +4. Wrap `context.Canceled` / `context.DeadlineExceeded` with `providerErr(ErrKindCanceled, …, err)`. +5. Replace API-envelope error checks (`code != "00000"`, `!resp.Success`, etc.) with `apiErr(code, msg)`. +6. Use `WrapError` for any site not yet convertible (e.g. third-party library errors). -Provider-specific code→kind mappings go into each provider's `errors.go`. Examples: +Provider-specific code→kind mappings: | Provider | Success signal | Auth codes | Rate-limit codes | |---|---|---|---| @@ -236,26 +313,31 @@ Provider-specific code→kind mappings go into each provider's `errors.go`. Exam | MEXC | HTTP 200 | HTTP 401/403 | HTTP 429 | | WhiteBit | `success == true` | HTTP 401/403 | HTTP 429 | +Each provider's `errors.go` should include table-driven tests mapping HTTP statuses and API codes to expected `ErrorKind` values. + --- ### Phase 3 — `ItemError` migration -**Files:** `pkg/model/response.go`, all call sites that construct `ItemError` +> **Gate:** Phase 2 must be complete for all six providers before this phase lands. If partial, use `WrapError` to bridge un-migrated paths before opening this PR. -Change `ItemError.Err` from `error` to `*model.ProviderError`. +**Files:** `pkg/model/response.go`, all `ItemError` construction sites -Update every construction site (fanout, provider market files) to always pass a `*ProviderError`. Any site that currently creates a raw `fmt.Errorf` must be wrapped in the provider's `providerErr` helper first. +Change `ItemError.Err` from `error` to `*model.ProviderError`. Update every construction site to pass a `*ProviderError`; use `WrapError` for any remaining raw errors. This includes: -This is the only breaking change for library consumers. Callers that stored `ItemError.Err` as `error` continue to work via the `error` interface; callers that did type assertions need updating. +- `pkg/resolve/fanout.go` — wraps per-symbol errors into `ItemError` +- all provider `market.go` files that append to `Response.Errors` +- `pkg/bits/client.go` — `ComparePrices` stores errors into `ItemError{Err: err}` where `err` comes from `GetPrice`; this must be `WrapError`-guarded until Phase 5 --- -### Phase 4 — Resolver and FanOut +### Phase 4 — Resolver -**Files:** `pkg/resolve/resolver.go`, `pkg/resolve/fanout.go` +**Files:** `pkg/resolve/resolver.go` -- `resolver.go`: replace `fmt.Errorf("no provider supports …")` with `&model.ProviderError{Kind: ErrKindUnsupportedFeature, …}`. No `ProviderID` here since the error is from the resolution layer, not a specific provider. -- `fanout.go`: no structural change needed — `ItemError` already accepts the new type from Phase 3. +Replace `fmt.Errorf("no provider supports …")` and similar with `*model.ProviderError`. Use `ErrKindUnsupportedFeature` / `ErrKindUnsupportedMarket` with an empty `ProviderID` (the resolution layer has no single provider to blame). + +`fanout.go` requires no structural change after Phase 3. --- @@ -263,38 +345,35 @@ This is the only breaking change for library consumers. Callers that stored `Ite **Files:** `pkg/bits/client.go` -The facade is what external consumers import, so this phase ensures the public surface is clean: - -1. `GetPrice`, `ComparePrices`, etc. already return `(model.Response[T], error)` — no signature change. -2. Document in godoc that returned errors are always `*model.ProviderError` when they originate from a provider, so callers can use `errors.As`. -3. The "no data returned" fallback path also wraps with `*model.ProviderError{Kind: ErrKindNotFound, …}` instead of `fmt.Errorf`. +1. No signature changes — methods still return `(model.Response[T], error)`. +2. The "no data returned" path wraps with `model.WrapError("", …)` or a direct `*ProviderError{Kind: ErrKindNotFound}`. +3. `ComparePrices` / `ComparePricesWithResolution`: replace ad-hoc `ItemError{Err: err}` with `WrapError(pid, err)` to satisfy the typed field. +4. Add godoc on all public methods stating: *returned errors are always `*model.ProviderError`; use `errors.As` to inspect them.* --- -### Phase 6 — CLI layer (optional cleanup) +### Phase 6 — CLI layer (optional) **Files:** `cmd/*.go` -The CLI commands currently print `err.Error()` on failure. After the transition, they can optionally surface the `Kind` for better UX (e.g. "authentication failed — check your API key in config") without breaking anything. - -This phase is optional and can be deferred. +CLI commands can optionally inspect `*model.ProviderError.Kind` for friendlier messages (e.g. "authentication failed — check your API key in config") without breaking anything. Defer until after Phase 5 stabilises. --- ### Migration checklist ``` -[ ] Phase 1: pkg/model — ErrorKind, ProviderError, httpStatusToKind -[ ] Phase 2a: pkg/provider/binance — errors.go + client/market updates -[ ] Phase 2b: pkg/provider/bitget — errors.go + client/market updates -[ ] Phase 2c: pkg/provider/coingecko — errors.go + client/market updates -[ ] Phase 2d: pkg/provider/cryptocom — errors.go + client/market updates -[ ] Phase 2e: pkg/provider/mexc — errors.go + client/market updates -[ ] Phase 2f: pkg/provider/whitebit — errors.go + client/market updates -[ ] Phase 3: ItemError.Err → *ProviderError -[ ] Phase 4: resolver + fanout -[ ] Phase 5: pkg/bits facade + godoc -[ ] Phase 6: (optional) CLI human-readable error messages +[ ] Phase 1: pkg/model — ErrorKind, ProviderError (with Is/Unwrap), WrapError, sentinels +[ ] Phase 2a: pkg/provider/binance — errors.go + HTTP/API error wrapping + tests +[ ] Phase 2b: pkg/provider/bitget — errors.go + HTTP/API error wrapping + tests (bug fix: add HTTP status check) +[ ] Phase 2c: pkg/provider/coingecko — errors.go + HTTP/API error wrapping + tests (note: get() pattern differs) +[ ] Phase 2d: pkg/provider/cryptocom — errors.go + HTTP/API error wrapping + tests (bug fix: add HTTP status check) +[ ] Phase 2e: pkg/provider/mexc — errors.go + HTTP/API error wrapping + tests +[ ] Phase 2f: pkg/provider/whitebit — errors.go + HTTP/API error wrapping + tests (bug fix: add HTTP status check) +[ ] Phase 3: ItemError.Err → *ProviderError (gate: all Phase 2 complete) +[ ] Phase 4: resolver — ProviderError for resolution failures +[ ] Phase 5: pkg/bits facade — WrapError at remaining sites + godoc +[ ] Phase 6: (optional) CLI human-readable error messages ``` -Each phase compiles and passes tests independently. Phases 2a–2f are fully parallel. +Phases 2a–2f are fully parallel. All other phases are sequential.