Backfill prices from pro cmc api - #5038
Open
tspenov wants to merge 2 commits into
Open
Conversation
tspenov
marked this pull request as ready for review
March 6, 2026 11:44
Contributor
📝 WalkthroughWalkthroughIntroduces a complete CoinMarketCap Pro backfill system for historical price data gaps. Includes configuration, rate limiting setup, two database tables with schemas and migrations, gap detection logic, run orchestration with state transitions, Oban workers for job processing, API client integration, and operational runbook documentation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ProBackfill
participant Verification
participant Run
participant RunSeederWorker
participant AssetWorker
participant ProApiClient
participant Database
User->>ProBackfill: start_run(scope, interval, ...)
ProBackfill->>Verification: gap_check(opts)
Verification->>Database: Fetch projects & price data
Verification-->>ProBackfill: Gap analysis (assets with gaps)
ProBackfill->>Run: create_run_with_assets(...)
Run->>Database: Insert Run record
Run->>Database: Insert Asset records (pending)
Run-->>ProBackfill: Run created
ProBackfill->>RunSeederWorker: Enqueue job
ProBackfill-->>User: {:ok, run_status}
RunSeederWorker->>Run: Fetch run by id
RunSeederWorker->>Database: List pending assets
RunSeederWorker->>AssetWorker: Enqueue job per asset
loop Per Asset
AssetWorker->>AssetWorker: Mark asset running
AssetWorker->>ProApiClient: fetch_range(cmc_id, from, to)
ProApiClient->>ProApiClient: Rate limit check
ProApiClient-->>AssetWorker: Price points + usage
AssetWorker->>Database: Export results (if not dry_run)
AssetWorker->>Database: Mark asset completed
AssetWorker->>Run: increment_stats(...)
end
Run->>Run: maybe_mark_completed()
Run->>Database: Update run status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (8)
priv/repo/migrations/20260306173000_create_coinmarketcap_pro_backfill_tables.exs (1)
31-57: LGTM with minor consideration.The
coinmarketcap_pro_backfill_assetstable is well-designed. The cascade delete onrun_idensures cleanup when runs are removed. The unique constraint on(run_id, project_id)prevents duplicate assets per run, and the composite indexes support efficient status/rank queries within a run.Consider whether an index on
cmc_integer_idis needed if you query assets by their CMC identifier directly (e.g., for lookups during API responses). If not queried independently, the current indexes are sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@priv/repo/migrations/20260306173000_create_coinmarketcap_pro_backfill_tables.exs` around lines 31 - 57, Single-line: Add an index on cmc_integer_id if you query assets by CMC id. The migration for the coinmarketcap_pro_backfill_assets table currently lacks an index on the cmc_integer_id column; if you perform lookups by cmc_integer_id (e.g., during API responses), add an index by inserting a create(index(:coinmarketcap_pro_backfill_assets, [:cmc_integer_id])) after the table creation so queries on cmc_integer_id benefit from the index and avoid full table scans.lib/sanbase/external_services/coinmarketcap/pro_backfill/run.ex (2)
106-112: Potential race condition in completion check.
maybe_mark_completed/1reads counters from the passedrunstruct, but these values may be stale if concurrentincrement_statscalls have updated the database. Consider refreshing the run from the database or using a single atomic query.Alternative approach
def maybe_mark_completed(%Run{} = run) do - if run.total_assets > 0 and run.done_assets + run.failed_assets >= run.total_assets do - update_run(run, %{status: "completed", finished_at: DateTime.utc_now()}) - else - {:ok, run} - end + # Refresh from DB to get latest counters + run = get!(run.id) + if run.total_assets > 0 and run.done_assets + run.failed_assets >= run.total_assets do + update_run(run, %{status: "completed", finished_at: DateTime.utc_now()}) + else + {:ok, run} + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/run.ex` around lines 106 - 112, maybe_mark_completed/1 currently checks the passed %Run{} counters (done_assets, failed_assets, total_assets) which can be stale due to concurrent increment_stats updates; modify maybe_mark_completed (or create a helper) to re-fetch the latest Run row from the DB by id (or perform a single atomic UPDATE/WHERE query that sets status="completed" and finished_at=utc_now() only when done_assets + failed_assets >= total_assets) instead of using the in-memory struct, and call update_run (or the repo update) against the fresh DB state to avoid the race; reference maybe_mark_completed/1, increment_stats, update_run and the Run struct when making the change.
69-112: Add@specand@docfor public functions.Per coding guidelines, public functions should have typespecs and documentation. Consider adding specs for
create/1,get/1,get!/1,list/1,update_run/2,mark_running/1,mark_paused/1,mark_canceled/1,mark_failed/2,maybe_mark_completed/1, andincrement_stats/2.Example for key functions
+ `@doc` "Creates a new backfill run." + `@spec` create(map()) :: {:ok, t()} | {:error, Ecto.Changeset.t()} def create(attrs) do %Run{} |> changeset(attrs) |> Repo.insert() end + `@doc` "Fetches a run by ID, returns nil if not found." + `@spec` get(integer()) :: t() | nil def get(id), do: Repo.get(Run, id) + `@doc` "Marks the run as completed if all assets are processed." + `@spec` maybe_mark_completed(t()) :: {:ok, t()} | {:error, Ecto.Changeset.t()} def maybe_mark_completed(%Run{} = run) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/run.ex` around lines 69 - 112, Add `@spec` and `@doc` for each public function to satisfy the coding guideline: document purpose, args and return values and add typespecs referencing the Run struct and Repo return shapes. For functions create/1, get/1, get!/1, list/1, update_run/2, mark_running/1, mark_paused/1, mark_canceled/1, mark_failed/2, maybe_mark_completed/1 (and increment_stats/2 if present elsewhere) add a one-line `@doc` describing the behaviour and expected side effects and an `@spec` such as create(attrs :: map()) :: {:ok, Run.t()} | {:error, Ecto.Changeset.t()}, get(id :: integer() | binary()) :: Run.t() | nil, get!(id) :: Run.t(), list(opts :: keyword()) :: [Run.t()], update_run(run :: Run.t(), attrs :: map()) :: {:ok, Run.t()} | {:error, Ecto.Changeset.t()}, mark_running/1, mark_paused/1, mark_canceled/1, mark_failed/2, maybe_mark_completed/1 :: {:ok, Run.t()} | {:error, Ecto.Changeset.t()} (or Run.t() for get!/1), and for increment_stats/2 use appropriate arg/return types; place these annotations directly above each function (e.g. above create/1, get/1, etc.) and import or alias Run.t() if needed.lib/sanbase/external_services/coinmarketcap/pro_backfill/asset.ex (2)
34-57: Consider removingrun_idandproject_idfromcast/3.Per coding guidelines, fields set programmatically (like foreign keys) should not be listed in
castcalls for security purposes. Instead, set them explicitly when creating the struct.If the changeset is only used internally, this is lower risk, but separating the pattern improves consistency.
Proposed refactor
def changeset(%Asset{} = asset, attrs) do asset |> cast(attrs, [ - :run_id, - :project_id, :slug, :cmc_integer_id, :rank, ... ]) - |> validate_required([:run_id, :project_id, :slug, :cmc_integer_id, :status]) + |> validate_required([:slug, :cmc_integer_id, :status]) ... end + def changeset_with_associations(%Asset{} = asset, attrs, run_id, project_id) do + asset + |> changeset(attrs) + |> put_change(:run_id, run_id) + |> put_change(:project_id, project_id) + |> validate_required([:run_id, :project_id]) + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/asset.ex` around lines 34 - 57, The changeset function (changeset/2 in Asset) currently includes programmatically-set foreign keys :run_id and :project_id in cast/3; remove those keys from the cast list and stop accepting them from external params, then ensure callers set them explicitly (e.g., build the struct with %Asset{} |> put_change(:run_id, run_id) |> put_change(:project_id, project_id) or set them on the params before Repo.insert) so unique_constraint([:run_id, :project_id]) still applies but the keys are not mass-assignable via cast/3.
59-127: Add@specand@docfor public functions.Per coding guidelines, public functions should have typespecs and documentation. Consider adding specs for all public functions:
get!/1,get/1,list_by_run/1,list_pending_by_run/1,list_failed_by_run/2,update_asset/2,mark_running/1,mark_completed/2,mark_failed/3,mark_canceled/1, andinsert_many/1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/asset.ex` around lines 59 - 127, Add missing `@spec` and `@doc` annotations for all public functions in this module: get!/1, get/1, list_by_run/1, list_pending_by_run/1, list_failed_by_run/2, update_asset/2, mark_running/1, mark_completed/2, mark_failed/3, mark_canceled/1, and insert_many/1; for each function add a one-line `@doc` describing purpose and behavior and an `@spec` declaring argument and return types (use Asset.t() for returned structs, {:ok, Asset.t()} | {:error, Ecto.Changeset.t()} for update functions, list(Asset.t()) for list functions, Repo.insert_all return shape for insert_many, and appropriate param types such as integer() | binary() for ids and map() for attrs) so the compiler and callers have clear contracts; attach specs immediately above each function (use the existing function names like update_asset/2, mark_failed/3, etc.) and ensure module imports/aliases (Asset, Repo) are referenced consistently for types.lib/sanbase/external_services/coinmarketcap/pro_backfill/pro_api_client.ex (1)
9-37: Add@specand@docfor the public function.Per coding guidelines, public functions should have typespecs and documentation with examples.
Proposed addition
+ `@doc` """ + Fetches historical price data for a CMC asset within the specified time range. + + ## Options + * `:interval` - Data interval (default: "5m") + * `:convert` - Currencies to convert to (default: "USD,BTC") + + ## Examples + + iex> fetch_range(1, 1609459200, 1609545600) + {:ok, [%PricePoint{...}], %{api_credits_used: 1.0, ...}} + """ + `@spec` fetch_range(integer(), integer(), integer(), keyword()) :: + {:ok, [PricePoint.t()], map()} + | {:rate_limited, integer(), map()} + | {:error, String.t()} def fetch_range(cmc_integer_id, from_unix, to_unix, opts \\ []) do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/pro_api_client.ex` around lines 9 - 37, Add a `@spec` and `@doc` for the public function fetch_range/4: declare `@spec` fetch_range(integer(), integer(), integer(), keyword()) :: {:ok, list(), map()} | {:rate_limited, non_neg_integer(), map()} | {:error, String.t()}; add `@doc` describing purpose, arguments (cmc_integer_id, from_unix, to_unix as unix seconds, opts with :interval and :convert), return values, and one short usage example showing a successful call and a rate-limited call (e.g. fetch_range(1, 1_640_995_200, 1_640_998_800) and fetch_range(1, from, to, interval: "1h")). Ensure the `@doc` is above fetch_range/4 and includes types consistent with the `@spec`.lib/sanbase/external_services/coinmarketcap/pro_backfill.ex (1)
20-110: Document and spec the public commands.This module adds several public entry points without
@doc, examples, or@specs.As per coding guidelines, "Add typespecs (
@spec) to all public functions", "Add '@doc' documentation to all public functions", and "Include examples in documentation for public functions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/external_services/coinmarketcap/pro_backfill.ex` around lines 20 - 110, Add `@doc`, `@spec` and example doctests for each public function (gap_check/1, start_run/1, pause_run/1, resume_run/1, cancel_run/1, list_runs/1, status/1): add a short `@doc` describing purpose, args and return values and one-line usage example; add `@spec` signatures using appropriate types (e.g. opts :: Keyword.t(), run_id :: integer() | binary(), returns {:ok, map()} | {:error, String.t()} or Run.t()/Asset.t. aliases like Run.t() when appropriate); for start_run/1 and status/1 document the expected map shape in `@spec/`@doc (use map() or a typed map), and include minimal doctest examples showing success and error cases; update module imports/aliases if needed so types like Run.t() and Asset.t. resolve.lib/sanbase/external_services/coinmarketcap/pro_backfill/verification.ex (1)
9-129: Either privatize or document/spec these helpers.
gap_check/1,gap_check_project/3,expected_timestamps/2,timestamps_to_ranges/1, andsplit_fillable_ranges/1are public in a new module, but none of them have@doc, examples, or@specs. If the helpers are internal, make themdefp; otherwise document the public contract.As per coding guidelines, "Add typespecs (
@spec) to all public functions", "Add '@doc' documentation to all public functions", and "Include examples in documentation for public functions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/verification.ex` around lines 9 - 129, The listed helper functions (gap_check/1, gap_check_project/3, expected_timestamps/2, timestamps_to_ranges/1, split_fillable_ranges/1) are public but undocumented and untyped; either make them internal by changing their definitions to defp or add proper `@spec` and `@doc` (including short examples) for each to satisfy the project guidelines — update gap_check/1, gap_check_project/3, expected_timestamps/2, timestamps_to_ranges/1 and split_fillable_ranges/1 accordingly (if kept public add `@specs` for input/output types and `@doc` blocks with usage examples; if internal change def -> defp and remove public contract requirements).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ops/cmc_pro_backfill_runbook.md`:
- Around line 34-38: The docs call uses the wrong module path
Sanbase.ExternalServices.Coinmarketcap.ProBackfill.gap_check; replace that with
ProBackfill.Verification.gap_check/1 (keeping the same argument map: %{scope:
:single, slug: slug, interval: "5m"}) so the example matches the actual
implementation and naming used by the codebase.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill.ex`:
- Around line 22-39: The start_run/1 function calls Verification.gap_check/1
which may synthesize time_start and time_end (e.g., for :single), but the code
still passes the original opts into create_run_with_assets/3 causing later
Keyword.fetch! calls to fail; update the call to create_run_with_assets to
include the resolved discovery window from result (use result.time_start and
result.time_end) instead of the original opts so create_run_with_assets (and any
subsequent Keyword.fetch!/2) receive the synthesized time_start/time_end values.
- Around line 43-57: The pause_run/1 and resume_run/1 functions currently call
Oban.pause_queue/2 and Oban.resume_queue/2 which pauses the entire shared queue;
remove those queue-wide calls so pausing is implemented per-run: in pause_run/1
(which uses Run.get/1 and Run.mark_paused/1) delete the
Oban.pause_queue(`@oban_conf_name`, queue: `@worker_queue`) call and simply return
:ok after marking the run paused, and in resume_run/1 (which uses Run.get/1 and
Run.update_run/2) delete the Oban.resume_queue(`@oban_conf_name`, queue:
`@worker_queue`) call and simply return :ok after updating the run status; keep
the Run.* calls and existing error handling intact so workers (e.g.,
AssetWorker) continue to rely on run.status checks/snooze behavior.
- Around line 129-176: The transaction currently ignores tagged-tuple returns
and can leave stale state; refactor the Repo.transaction block to use a with
chain that matches {:ok, run} <- Run.create(attrs), {:ok, _} <-
Asset.insert_many(rows) (or wrap insert_many to return {:ok, _}), {:ok,
run_after_update} <- Run.update_run(run, %{total_assets: length(rows),
pending_assets: length(rows)}), and when enqueuing, {:ok, _job} <-
Oban.insert(`@oban_conf_name`, RunSeederWorker.new(%{"run_id" =>
run_after_update.id})) (or handle the else branch to update run status and
return {:ok, run_after_update}); ensure every call (Run.create,
Asset.insert_many, Run.update_run, Oban.insert) is pattern-matched so any
{:error, _} bubbles out of the transaction to trigger rollback and return the
updated run struct (run_after_update) rather than the original run.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/asset_worker.ex`:
- Around line 62-64: Guard against Run.get(run.id) returning nil before calling
Run.maybe_mark_completed: replace the direct pipe of Run.get(run.id) |>
Run.maybe_mark_completed() with a nil-safe check (e.g., case/if) that only calls
Run.maybe_mark_completed(run) when Run.get(run.id) returns a non-nil run struct,
and otherwise does nothing/returns :ok; apply the same nil-guarding pattern for
both occurrences currently calling Run.get(...) |> Run.maybe_mark_completed() so
you avoid FunctionClauseError.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/audit_report.ex`:
- Around line 4-31: Add a `@spec` and `@doc` for the public function run_report/1:
define a brief `@doc` describing that run_report/1 fetches the ProBackfill status
for a given run_id and returns {:ok, summary_map} on success or passes through
{:error, reason} from ProBackfill.status/1; add an `@spec` such as
run_report(term()) :: {:ok, map()} | {:error, term()} placed immediately above
the run_report/1 function to satisfy the project coding guidelines and reference
the function that builds and returns the summary.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/pro_api_client.ex`:
- Around line 20-24: The Req.get call that performs the external CoinMarketCap
request (the call using base_url(), `@path`, headers(), params in
pro_api_client.ex) lacks a timeout and can hang; update that Req.get invocation
to include appropriate request timeouts (e.g., recv/connect/overall timeout
options) so the call fails fast on unresponsive CMC API, and ensure the error
path still returns a proper error tuple that callers of body_to_price_points and
usage_from_body can handle.
- Around line 26-30: The 429 handler assumes header_value(headers,
"retry-after") is present and passes it to Sanbase.Math.to_integer/1 which may
crash on nil; update the logic in the clause that matches {:ok, %{status: 429,
headers: headers}} to defensively handle a missing header by converting
header_value(headers, "retry-after") with a safe fallback (e.g. case or pattern
match: if nil use 1, otherwise call Sanbase.Math.to_integer/1) before applying
max(1), then call Server.wait_until(`@rate_limiting_server`, wait_until) and
return the same {:rate_limited, wait_seconds, ...} tuple; reference the
functions header_value, Sanbase.Math.to_integer, Server.wait_until and the
module attribute `@rate_limiting_server` when making the change.
In
`@lib/sanbase/external_services/coinmarketcap/pro_backfill/run_seeder_worker.ex`:
- Around line 12-41: The status check for cancellation is done after calling
Run.mark_running, so run.status will be "running" and the check always fails;
modify perform/1 to fetch the run with Run.get(run_id), check run.status ==
"canceled" immediately and return :ok if canceled, and only then call
Run.mark_running(run) to transition to running; update the control flow around
Run.get, the pre-check, and the subsequent Run.mark_running/1 call (symbols:
perform, Run.get, Run.mark_running, run.status) so canceled runs are skipped
before marking them running.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/verification.ex`:
- Around line 141-160: fetch_interval/2 currently returns arbitrary DateTimes
which can be off the 5-minute grid expected by expected_timestamps/2 and may
silently accept descending windows; update fetch_interval/2 to (1) snap the
returned from down to the nearest 5-minute boundary and the returned to up to
the nearest 5-minute boundary (use DateTime.to_unix/1, integer math with 300s,
and DateTime.from_unix!/1) for all {:ok, from, to} paths including the discovery
default branch, and (2) validate that from <= to after snapping and return
{:error, "time_start must be before or equal to time_end"} when the window is
descending; keep the same return shapes ({:ok, from, to} or {:error, _}) so
callers like expected_timestamps/2 get an ascending, 5-minute-aligned range.
- Around line 48-59: The code currently collapses every non-{:ok, points} result
of Price.timeseries_metric_data(...) into MapSet.new(), treating read failures
as confirmed gaps; change the match in verification.ex to explicitly handle
{:error, reason} (or any non-{:ok, points} tuple) instead of returning an empty
MapSet — return or propagate an error result (e.g., {:error, reason}) or
otherwise mark the call as failed so callers won't treat transient failures as
missing points; specifically update the block around
Price.timeseries_metric_data/6 (the actual variable assignment) to pattern-match
{:ok, points} -> build MapSet, {:error, reason} -> propagate {:error, reason}
(or a distinct failure value) and only treat MapSet.empty as a true gap when you
have an explicit successful response.
- Around line 167-186: The projects_for_scope/2 function currently calls
Project.by_slug/1 and silently drops unknown or non-CMC projects; update both
the :single and :list branches to validate slugs up front using the
source-filtering helper (Project.List.projects_with_source/2) so only projects
that exist and have the CoinMarketCap source are returned. For :single, wrap the
incoming slug in a list, call Project.List.projects_with_source([slug],
:coinmarketcap) and return the single-element list or [] if none; for :list,
call Project.List.projects_with_source(slugs, :coinmarketcap) (after
List.wrap/1) and return that filtered list instead of mapping Project.by_slug/1.
This prevents nil entries that later cause projects_map[gap.project_id] to be
nil in create_run_with_assets/3.
In `@priv/repo/structure.sql`:
- Around line 1004-1010: The DB schema allows arbitrary strings for workflow
columns (asset.status, asset.usage_precision and run.scope, run.status,
run.usage_precision) but the app enforces finite states via validate_inclusion;
add matching DB-level safeguards by creating ENUM types or CHECK constraints for
each column that exactly mirror the allowed values in the Ecto changesets,
ensure the current default values are included in those sets, add a migration
that first finds/fixes any existing rows with invalid values before applying the
constraint, and include meaningful constraint names (e.g., asset_status_check,
run_usage_precision_check) so future migrations/reference can locate them.
- Around line 997-1001: The coinmarketcap_pro_backfill_assets table defines
project_id as integer but public.project.id is bigint; change the column type of
project_id to bigint in the migration for coinmarketcap_pro_backfill_assets (and
the other affected blocks around 9925-9930), and add a foreign key constraint
referencing public.project(id) (e.g., ALTER TABLE ... ADD CONSTRAINT ... FOREIGN
KEY (project_id) REFERENCES public.project(id)); after modifying the migration,
regenerate structure.sql so the schema file and constraints are consistent.
---
Nitpick comments:
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill.ex`:
- Around line 20-110: Add `@doc`, `@spec` and example doctests for each public
function (gap_check/1, start_run/1, pause_run/1, resume_run/1, cancel_run/1,
list_runs/1, status/1): add a short `@doc` describing purpose, args and return
values and one-line usage example; add `@spec` signatures using appropriate types
(e.g. opts :: Keyword.t(), run_id :: integer() | binary(), returns {:ok, map()}
| {:error, String.t()} or Run.t()/Asset.t. aliases like Run.t() when
appropriate); for start_run/1 and status/1 document the expected map shape in
`@spec/`@doc (use map() or a typed map), and include minimal doctest examples
showing success and error cases; update module imports/aliases if needed so
types like Run.t() and Asset.t. resolve.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/asset.ex`:
- Around line 34-57: The changeset function (changeset/2 in Asset) currently
includes programmatically-set foreign keys :run_id and :project_id in cast/3;
remove those keys from the cast list and stop accepting them from external
params, then ensure callers set them explicitly (e.g., build the struct with
%Asset{} |> put_change(:run_id, run_id) |> put_change(:project_id, project_id)
or set them on the params before Repo.insert) so unique_constraint([:run_id,
:project_id]) still applies but the keys are not mass-assignable via cast/3.
- Around line 59-127: Add missing `@spec` and `@doc` annotations for all public
functions in this module: get!/1, get/1, list_by_run/1, list_pending_by_run/1,
list_failed_by_run/2, update_asset/2, mark_running/1, mark_completed/2,
mark_failed/3, mark_canceled/1, and insert_many/1; for each function add a
one-line `@doc` describing purpose and behavior and an `@spec` declaring argument
and return types (use Asset.t() for returned structs, {:ok, Asset.t()} |
{:error, Ecto.Changeset.t()} for update functions, list(Asset.t()) for list
functions, Repo.insert_all return shape for insert_many, and appropriate param
types such as integer() | binary() for ids and map() for attrs) so the compiler
and callers have clear contracts; attach specs immediately above each function
(use the existing function names like update_asset/2, mark_failed/3, etc.) and
ensure module imports/aliases (Asset, Repo) are referenced consistently for
types.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/pro_api_client.ex`:
- Around line 9-37: Add a `@spec` and `@doc` for the public function fetch_range/4:
declare `@spec` fetch_range(integer(), integer(), integer(), keyword()) :: {:ok,
list(), map()} | {:rate_limited, non_neg_integer(), map()} | {:error,
String.t()}; add `@doc` describing purpose, arguments (cmc_integer_id, from_unix,
to_unix as unix seconds, opts with :interval and :convert), return values, and
one short usage example showing a successful call and a rate-limited call (e.g.
fetch_range(1, 1_640_995_200, 1_640_998_800) and fetch_range(1, from, to,
interval: "1h")). Ensure the `@doc` is above fetch_range/4 and includes types
consistent with the `@spec`.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/run.ex`:
- Around line 106-112: maybe_mark_completed/1 currently checks the passed %Run{}
counters (done_assets, failed_assets, total_assets) which can be stale due to
concurrent increment_stats updates; modify maybe_mark_completed (or create a
helper) to re-fetch the latest Run row from the DB by id (or perform a single
atomic UPDATE/WHERE query that sets status="completed" and finished_at=utc_now()
only when done_assets + failed_assets >= total_assets) instead of using the
in-memory struct, and call update_run (or the repo update) against the fresh DB
state to avoid the race; reference maybe_mark_completed/1, increment_stats,
update_run and the Run struct when making the change.
- Around line 69-112: Add `@spec` and `@doc` for each public function to satisfy the
coding guideline: document purpose, args and return values and add typespecs
referencing the Run struct and Repo return shapes. For functions create/1,
get/1, get!/1, list/1, update_run/2, mark_running/1, mark_paused/1,
mark_canceled/1, mark_failed/2, maybe_mark_completed/1 (and increment_stats/2 if
present elsewhere) add a one-line `@doc` describing the behaviour and expected
side effects and an `@spec` such as create(attrs :: map()) :: {:ok, Run.t()} |
{:error, Ecto.Changeset.t()}, get(id :: integer() | binary()) :: Run.t() | nil,
get!(id) :: Run.t(), list(opts :: keyword()) :: [Run.t()], update_run(run ::
Run.t(), attrs :: map()) :: {:ok, Run.t()} | {:error, Ecto.Changeset.t()},
mark_running/1, mark_paused/1, mark_canceled/1, mark_failed/2,
maybe_mark_completed/1 :: {:ok, Run.t()} | {:error, Ecto.Changeset.t()} (or
Run.t() for get!/1), and for increment_stats/2 use appropriate arg/return types;
place these annotations directly above each function (e.g. above create/1,
get/1, etc.) and import or alias Run.t() if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/verification.ex`:
- Around line 9-129: The listed helper functions (gap_check/1,
gap_check_project/3, expected_timestamps/2, timestamps_to_ranges/1,
split_fillable_ranges/1) are public but undocumented and untyped; either make
them internal by changing their definitions to defp or add proper `@spec` and `@doc`
(including short examples) for each to satisfy the project guidelines — update
gap_check/1, gap_check_project/3, expected_timestamps/2, timestamps_to_ranges/1
and split_fillable_ranges/1 accordingly (if kept public add `@specs` for
input/output types and `@doc` blocks with usage examples; if internal change def
-> defp and remove public contract requirements).
In
`@priv/repo/migrations/20260306173000_create_coinmarketcap_pro_backfill_tables.exs`:
- Around line 31-57: Single-line: Add an index on cmc_integer_id if you query
assets by CMC id. The migration for the coinmarketcap_pro_backfill_assets table
currently lacks an index on the cmc_integer_id column; if you perform lookups by
cmc_integer_id (e.g., during API responses), add an index by inserting a
create(index(:coinmarketcap_pro_backfill_assets, [:cmc_integer_id])) after the
table creation so queries on cmc_integer_id benefit from the index and avoid
full table scans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d65863ef-039f-4a7f-9e62-b0a21bc048c3
📒 Files selected for processing (13)
config/scrapers_config.exsdocs/ops/cmc_pro_backfill_runbook.mdlib/sanbase/application/scrapers.exlib/sanbase/external_services/coinmarketcap/pro_backfill.exlib/sanbase/external_services/coinmarketcap/pro_backfill/asset.exlib/sanbase/external_services/coinmarketcap/pro_backfill/asset_worker.exlib/sanbase/external_services/coinmarketcap/pro_backfill/audit_report.exlib/sanbase/external_services/coinmarketcap/pro_backfill/pro_api_client.exlib/sanbase/external_services/coinmarketcap/pro_backfill/run.exlib/sanbase/external_services/coinmarketcap/pro_backfill/run_seeder_worker.exlib/sanbase/external_services/coinmarketcap/pro_backfill/verification.expriv/repo/migrations/20260306173000_create_coinmarketcap_pro_backfill_tables.exspriv/repo/structure.sql
Comment on lines
+34
to
+38
| Sanbase.ExternalServices.Coinmarketcap.ProBackfill.gap_check( | ||
| scope: :single, | ||
| slug: slug, | ||
| interval: "5m" | ||
| ) |
Contributor
There was a problem hiding this comment.
Same incorrect module path for gap_check.
This call should also use ProBackfill.Verification.gap_check/1 for consistency with the actual implementation.
📝 Proposed fix
-Sanbase.ExternalServices.Coinmarketcap.ProBackfill.gap_check(
+Sanbase.ExternalServices.Coinmarketcap.ProBackfill.Verification.gap_check(
scope: :single,
slug: slug,
interval: "5m"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Sanbase.ExternalServices.Coinmarketcap.ProBackfill.gap_check( | |
| scope: :single, | |
| slug: slug, | |
| interval: "5m" | |
| ) | |
| Sanbase.ExternalServices.Coinmarketcap.ProBackfill.Verification.gap_check( | |
| scope: :single, | |
| slug: slug, | |
| interval: "5m" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ops/cmc_pro_backfill_runbook.md` around lines 34 - 38, The docs call
uses the wrong module path
Sanbase.ExternalServices.Coinmarketcap.ProBackfill.gap_check; replace that with
ProBackfill.Verification.gap_check/1 (keeping the same argument map: %{scope:
:single, slug: slug, interval: "5m"}) so the example matches the actual
implementation and naming used by the codebase.
Comment on lines
+22
to
+39
| def start_run(opts) do | ||
| with {:ok, interval} <- fetch_interval(Keyword.get(opts, :interval, "5m")), | ||
| {:ok, result} <- Verification.gap_check(Keyword.put(opts, :interval, interval)) do | ||
| assets_with_gap = | ||
| result.assets | ||
| |> Enum.filter(&(length(&1.fillable_missing_ranges) > 0)) | ||
|
|
||
| if assets_with_gap == [] do | ||
| {:ok, | ||
| %{ | ||
| status: "no_gap", | ||
| total_assets: result.total_assets, | ||
| fillable_now_assets: result.fillable_now_assets, | ||
| deferred_assets: result.deferred_assets | ||
| }} | ||
| else | ||
| create_run_with_assets(opts, interval, assets_with_gap) | ||
| end |
Contributor
There was a problem hiding this comment.
Propagate the resolved discovery window into the run.
Verification.gap_check/1 can synthesize time_start and time_end for :single, but this branch still passes the original opts into create_run_with_assets/3. When gaps are found, Keyword.fetch!/2 at Line 120 and Line 121 raises because those keys were never present. Use result.time_start and result.time_end when creating the run.
Proposed fix
- create_run_with_assets(opts, interval, assets_with_gap)
+ create_run_with_assets(
+ Keyword.merge(opts,
+ time_start: result.time_start,
+ time_end: result.time_end
+ ),
+ interval,
+ assets_with_gap
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill.ex` around lines 22
- 39, The start_run/1 function calls Verification.gap_check/1 which may
synthesize time_start and time_end (e.g., for :single), but the code still
passes the original opts into create_run_with_assets/3 causing later
Keyword.fetch! calls to fail; update the call to create_run_with_assets to
include the resolved discovery window from result (use result.time_start and
result.time_end) instead of the original opts so create_run_with_assets (and any
subsequent Keyword.fetch!/2) receive the synthesized time_start/time_end values.
Comment on lines
+43
to
+57
| def pause_run(run_id) do | ||
| with %Run{} = run <- Run.get(run_id), | ||
| {:ok, _} <- Run.mark_paused(run) do | ||
| Oban.pause_queue(@oban_conf_name, queue: @worker_queue) | ||
| :ok | ||
| else | ||
| _ -> {:error, "Run not found"} | ||
| end | ||
| end | ||
|
|
||
| def resume_run(run_id) do | ||
| with %Run{} = run <- Run.get(run_id), | ||
| {:ok, _} <- Run.update_run(run, %{status: "running"}) do | ||
| Oban.resume_queue(@oban_conf_name, queue: @worker_queue) | ||
| :ok |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n lib/sanbase/external_services/coinmarketcap/pro_backfill.ex | head -100Repository: santiment/sanbase2
Length of output: 3625
🏁 Script executed:
rg "pause_run|resume_run" -A 3 -B 1 lib/sanbase/external_services/coinmarketcap/Repository: santiment/sanbase2
Length of output: 1041
🏁 Script executed:
rg "@worker_queue|@oban_conf_name" lib/sanbase/external_services/coinmarketcap/pro_backfill.exRepository: santiment/sanbase2
Length of output: 386
🏁 Script executed:
fd "run_seeder" lib/sanbase/external_services/coinmarketcap/ -type fRepository: santiment/sanbase2
Length of output: 232
🏁 Script executed:
rg "RunSeederWorker|def perform" lib/sanbase/external_services/coinmarketcap/ -A 10Repository: santiment/sanbase2
Length of output: 5402
🏁 Script executed:
rg "run_id.*active|status.*paused" lib/sanbase/external_services/coinmarketcap/ -B 2 -A 2Repository: santiment/sanbase2
Length of output: 1479
🏁 Script executed:
rg "insert.*@oban_conf_name" lib/sanbase/external_services/coinmarketcap/pro_backfill.ex -B 5 -A 5Repository: santiment/sanbase2
Length of output: 418
Remove queue-wide pause/resume operations; rely on existing per-run pause checks in workers.
pause_run/1 and resume_run/1 pause the entire shared queue, blocking all other backfill runs on that queue. However, AssetWorker already checks run.status and snoozes when paused ({:snooze, 30}), making the queue pause redundant. Remove Oban.pause_queue() and Oban.resume_queue() calls—the per-run status checks in workers provide the needed pause behavior without blocking concurrent runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill.ex` around lines 43
- 57, The pause_run/1 and resume_run/1 functions currently call
Oban.pause_queue/2 and Oban.resume_queue/2 which pauses the entire shared queue;
remove those queue-wide calls so pausing is implemented per-run: in pause_run/1
(which uses Run.get/1 and Run.mark_paused/1) delete the
Oban.pause_queue(`@oban_conf_name`, queue: `@worker_queue`) call and simply return
:ok after marking the run paused, and in resume_run/1 (which uses Run.get/1 and
Run.update_run/2) delete the Oban.resume_queue(`@oban_conf_name`, queue:
`@worker_queue`) call and simply return :ok after updating the run status; keep
the Run.* calls and existing error handling intact so workers (e.g.,
AssetWorker) continue to rely on run.status checks/snooze behavior.
Comment on lines
+129
to
+176
| Repo.transaction(fn -> | ||
| {:ok, run} = | ||
| Run.create(attrs) | ||
|
|
||
| projects_map = | ||
| Project.List.projects_with_source("coinmarketcap", | ||
| include_hidden: true, | ||
| order_by_rank: true | ||
| ) | ||
| |> Map.new(&{&1.id, &1}) | ||
|
|
||
| rows = | ||
| assets_with_gap | ||
| |> Enum.map(fn gap -> | ||
| project = projects_map[gap.project_id] | ||
| cmc_data = LatestCoinmarketcapData.latest_coinmarketcap_data(project) | ||
| cmc_integer_id = if(cmc_data, do: cmc_data.coinmarketcap_integer_id, else: nil) | ||
| rank = if(cmc_data, do: cmc_data.rank, else: nil) | ||
| ranges = %{"ranges" => gap.fillable_missing_ranges} | ||
|
|
||
| {rank || 9_999_999, -(gap.missing_points_count || 0), | ||
| %{ | ||
| run_id: run.id, | ||
| project_id: project.id, | ||
| slug: project.slug, | ||
| cmc_integer_id: cmc_integer_id, | ||
| rank: rank, | ||
| status: "pending", | ||
| missing_ranges: ranges, | ||
| inserted_at: now, | ||
| updated_at: now | ||
| }} | ||
| end) | ||
| |> Enum.sort_by( | ||
| fn {rank, missing_points_count, _row} -> {rank, missing_points_count} end, | ||
| :asc | ||
| ) | ||
| |> Enum.map(&elem(&1, 2)) | ||
| |> Enum.reject(&is_nil(&1.cmc_integer_id)) | ||
|
|
||
| Asset.insert_many(rows) | ||
| Run.update_run(run, %{total_assets: length(rows), pending_assets: length(rows)}) | ||
|
|
||
| if rows != [] do | ||
| job = RunSeederWorker.new(%{"run_id" => run.id}) | ||
| Oban.insert(@oban_conf_name, job) | ||
| else | ||
| Run.update_run(run, %{status: "completed", finished_at: DateTime.utc_now()}) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the file and understand its structure
cd /repo
find . -name "pro_backfill.ex" -type fRepository: santiment/sanbase2
Length of output: 196
🏁 Script executed:
#!/bin/bash
# Read the file to see the actual implementation
cat -n lib/sanbase/external_services/coinmarketcap/pro_backfill.ex | head -200Repository: santiment/sanbase2
Length of output: 7444
🏁 Script executed:
#!/bin/bash
# Search for Run.create, Run.update_run implementations
fd -e ex -e exs | xargs rg "def create\(" | grep -i runRepository: santiment/sanbase2
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Search for the Run module
fd -e ex -e exs | xargs rg "defmodule.*Run" | grep -v test | head -20Repository: santiment/sanbase2
Length of output: 1386
🏁 Script executed:
#!/antml:bsh
# Look for return types and error handling patterns
rg "def (create|update_run)" -A 5 lib/sanbase/repo.exRepository: santiment/sanbase2
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Get the Run module implementation
cat -n lib/sanbase/external_services/coinmarketcap/pro_backfill/run.ex | head -100Repository: santiment/sanbase2
Length of output: 3474
🏁 Script executed:
#!/bin/bash
# Check Oban.insert return type
rg "def insert" lib/sanbase/ -A 3 | head -50Repository: santiment/sanbase2
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Look for Asset.insert_many implementation
fd -e ex -e exs | xargs rg "def insert_many" | head -10Repository: santiment/sanbase2
Length of output: 173
🏁 Script executed:
#!/antml:bsh
# Check the broader transaction context and error handling
cat -n lib/sanbase/external_services/coinmarketcap/pro_backfill.ex | sed -n '112,181p'Repository: santiment/sanbase2
Length of output: 2699
🏁 Script executed:
#!/bin/bash
# Verify Oban.insert behavior and if it's inside transaction context
rg "Oban.insert" lib/sanbase/external_services/coinmarketcap/pro_backfill.ex -B 5 -A 2Repository: santiment/sanbase2
Length of output: 397
Use with to handle tagged tuple returns and ensure transaction rollback on failure.
Run.update_run/2 and Oban.insert/2 return {:ok, _} or {:error, _} tuples at lines 170, 174, and 176, but their results are ignored. The bare match on Run.create/1 at line 130 raises an exception instead of graceful error handling. If the enqueue at line 174 fails, assets remain in "pending" status indefinitely with no job scheduled. If Run.update_run/2 fails at lines 170 or 176, run metadata is never persisted. Additionally, line 179 returns the original run struct before line 170's update, yielding stale counts. Chain operations with with for consistent error propagation and transaction rollback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill.ex` around lines 129
- 176, The transaction currently ignores tagged-tuple returns and can leave
stale state; refactor the Repo.transaction block to use a with chain that
matches {:ok, run} <- Run.create(attrs), {:ok, _} <- Asset.insert_many(rows) (or
wrap insert_many to return {:ok, _}), {:ok, run_after_update} <-
Run.update_run(run, %{total_assets: length(rows), pending_assets:
length(rows)}), and when enqueuing, {:ok, _job} <- Oban.insert(`@oban_conf_name`,
RunSeederWorker.new(%{"run_id" => run_after_update.id})) (or handle the else
branch to update run status and return {:ok, run_after_update}); ensure every
call (Run.create, Asset.insert_many, Run.update_run, Oban.insert) is
pattern-matched so any {:error, _} bubbles out of the transaction to trigger
rollback and return the updated run struct (run_after_update) rather than the
original run.
Comment on lines
+62
to
+64
| Run.get(run.id) | ||
| |> Run.maybe_mark_completed() | ||
|
|
Contributor
There was a problem hiding this comment.
Guard against nil from Run.get/1 before maybe_mark_completed.
Run.get(run.id) can return nil if the run was deleted. Piping nil to Run.maybe_mark_completed/1 would cause a FunctionClauseError.
Proposed fix
- Run.get(run.id)
- |> Run.maybe_mark_completed()
+ case Run.get(run.id) do
+ %Run{} = refreshed_run -> Run.maybe_mark_completed(refreshed_run)
+ nil -> :ok
+ endApply the same pattern at lines 90-91.
Also applies to: 90-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/asset_worker.ex`
around lines 62 - 64, Guard against Run.get(run.id) returning nil before calling
Run.maybe_mark_completed: replace the direct pipe of Run.get(run.id) |>
Run.maybe_mark_completed() with a nil-safe check (e.g., case/if) that only calls
Run.maybe_mark_completed(run) when Run.get(run.id) returns a non-nil run struct,
and otherwise does nothing/returns :ok; apply the same nil-guarding pattern for
both occurrences currently calling Run.get(...) |> Run.maybe_mark_completed() so
you avoid FunctionClauseError.
Comment on lines
+48
to
+59
| actual = | ||
| case Price.timeseries_metric_data(slug, "price_usd", from, to, @max_fillable_interval, | ||
| source: "coinmarketcap" | ||
| ) do | ||
| {:ok, points} -> | ||
| points | ||
| |> Enum.map(&DateTime.to_unix(&1.datetime)) | ||
| |> MapSet.new() | ||
|
|
||
| _ -> | ||
| MapSet.new() | ||
| end |
Contributor
There was a problem hiding this comment.
Don't treat read failures as confirmed gaps.
lib/sanbase/prices/price.ex already distinguishes {:ok, points} from {:error, reason} for Price.timeseries_metric_data/6. Collapsing every non-{:ok, points} result into MapSet.new() makes transient query/source failures look like “all points missing”, which can enqueue unnecessary backfill work and burn CMC credits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/verification.ex`
around lines 48 - 59, The code currently collapses every non-{:ok, points}
result of Price.timeseries_metric_data(...) into MapSet.new(), treating read
failures as confirmed gaps; change the match in verification.ex to explicitly
handle {:error, reason} (or any non-{:ok, points} tuple) instead of returning an
empty MapSet — return or propagate an error result (e.g., {:error, reason}) or
otherwise mark the call as failed so callers won't treat transient failures as
missing points; specifically update the block around
Price.timeseries_metric_data/6 (the actual variable assignment) to pattern-match
{:ok, points} -> build MapSet, {:error, reason} -> propagate {:error, reason}
(or a distinct failure value) and only treat MapSet.empty as a true gap when you
have an explicit successful response.
Comment on lines
+141
to
+160
| defp fetch_interval(opts, scope) do | ||
| from = Keyword.get(opts, :time_start) | ||
| to = Keyword.get(opts, :time_end) | ||
|
|
||
| case {from, to} do | ||
| {%DateTime{} = from, %DateTime{} = to} -> | ||
| {:ok, from, to} | ||
|
|
||
| {nil, nil} when scope == :single -> | ||
| discovery_to_unix = fillable_to_unix() | ||
| discovery_from_unix = discovery_to_unix - @default_discovery_days * 86_400 | ||
|
|
||
| {:ok, DateTime.from_unix!(discovery_from_unix), DateTime.from_unix!(discovery_to_unix)} | ||
|
|
||
| {nil, nil} -> | ||
| {:error, "Both :time_start and :time_end are required for :all and :list scopes"} | ||
|
|
||
| _ -> | ||
| {:error, "Both :time_start and :time_end must be DateTime"} | ||
| end |
Contributor
There was a problem hiding this comment.
Validate and align the requested time window.
expected_timestamps/2 assumes an ascending 5-minute-aligned range, but fetch_interval/2 accepts arbitrary DateTimes. Off-grid windows like 00:02..01:02 generate timestamps the storage layer will never return, and time_start > time_end quietly reports no_gap. Snap the bounds to 5-minute edges and reject descending windows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/verification.ex`
around lines 141 - 160, fetch_interval/2 currently returns arbitrary DateTimes
which can be off the 5-minute grid expected by expected_timestamps/2 and may
silently accept descending windows; update fetch_interval/2 to (1) snap the
returned from down to the nearest 5-minute boundary and the returned to up to
the nearest 5-minute boundary (use DateTime.to_unix/1, integer math with 300s,
and DateTime.from_unix!/1) for all {:ok, from, to} paths including the discovery
default branch, and (2) validate that from <= to after snapping and return
{:error, "time_start must be before or equal to time_end"} when the window is
descending; keep the same return shapes ({:ok, from, to} or {:error, _}) so
callers like expected_timestamps/2 get an ascending, 5-minute-aligned range.
Comment on lines
+167
to
+186
| defp projects_for_scope(:single, opts) do | ||
| case Keyword.get(opts, :slug) do | ||
| slug when is_binary(slug) -> | ||
| case Project.by_slug(slug) do | ||
| %Project{} = project -> [project] | ||
| nil -> [] | ||
| end | ||
|
|
||
| _ -> | ||
| [] | ||
| end | ||
| end | ||
|
|
||
| defp projects_for_scope(:list, opts) do | ||
| slugs = Keyword.get(opts, :slugs, []) |> List.wrap() | ||
|
|
||
| slugs | ||
| |> Enum.map(&Project.by_slug/1) | ||
| |> Enum.reject(&is_nil/1) | ||
| end |
Contributor
There was a problem hiding this comment.
Reject unknown or non-CMC slugs up front.
Unlike the source-filtered Project.List.projects_with_source/2 flow in lib/sanbase/project/list/project_list.ex Lines 194-205, the :single and :list branches take whatever Project.by_slug/1 returns and silently drop unknown slugs. That means bad input can return total_assets: 0, and a non-CoinMarketCap project can reach create_run_with_assets/3, where projects_map[gap.project_id] is nil and later dereferenced at Line 143 and Line 152 of lib/sanbase/external_services/coinmarketcap/pro_backfill.ex.
As per coding guidelines, "Use NimbleOptions to validate inputs at context boundaries".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/sanbase/external_services/coinmarketcap/pro_backfill/verification.ex`
around lines 167 - 186, The projects_for_scope/2 function currently calls
Project.by_slug/1 and silently drops unknown or non-CMC projects; update both
the :single and :list branches to validate slugs up front using the
source-filtering helper (Project.List.projects_with_source/2) so only projects
that exist and have the CoinMarketCap source are returned. For :single, wrap the
incoming slug in a list, call Project.List.projects_with_source([slug],
:coinmarketcap) and return the single-element list or [] if none; for :list,
call Project.List.projects_with_source(slugs, :coinmarketcap) (after
List.wrap/1) and return that filtered list instead of mapping Project.by_slug/1.
This prevents nil entries that later cause projects_map[gap.project_id] to be
nil in create_run_with_assets/3.
Comment on lines
+997
to
+1001
| CREATE TABLE public.coinmarketcap_pro_backfill_assets ( | ||
| id bigint NOT NULL, | ||
| run_id bigint NOT NULL, | ||
| project_id integer NOT NULL, | ||
| slug character varying(255) NOT NULL, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== backfill assets column definition ==\n'
sed -n '997,1003p' priv/repo/structure.sql
printf '\n== project primary key definition ==\n'
sed -n '3457,3459p' priv/repo/structure.sql
printf '\n== asset schema association ==\n'
sed -n '24,40p' lib/sanbase/external_services/coinmarketcap/pro_backfill/asset.ex
printf '\n== migration source ==\n'
fd 20260306173000_create_coinmarketcap_pro_backfill_tables.exs priv/repo/migrations -x sed -n '1,220p' {}Repository: santiment/sanbase2
Length of output: 3490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== FK constraints around line 9925-9930 ==\n'
sed -n '9920,9935p' priv/repo/structure.sql
printf '\n== All FKs for coinmarketcap_pro_backfill tables ==\n'
rg 'coinmarketcap_pro_backfill.*_fkey' priv/repo/structure.sql
printf '\n== CHECK constraints for coinmarketcap_pro_backfill ==\n'
rg -A 2 'CHECK.*coinmarketcap_pro_backfill|coinmarketcap_pro_backfill.*CHECK' priv/repo/structure.sql || printf 'No CHECK constraints found\n'Repository: santiment/sanbase2
Length of output: 1177
Change project_id to bigint and add foreign key constraint to project.
Line 1000 stores project_id as integer, but public.project.id is bigint (Line 3458). This type mismatch prevents adding a foreign key constraint to project, allowing orphaned asset rows to be inserted and causing the column to overflow once project IDs exceed 32-bit range. Update the migration to use bigint for project_id and add the foreign key constraint, then regenerate structure.sql.
Suggested fix
CREATE TABLE public.coinmarketcap_pro_backfill_assets (
id bigint NOT NULL,
run_id bigint NOT NULL,
- project_id integer NOT NULL,
+ project_id bigint NOT NULL,
slug character varying(255) NOT NULL,
cmc_integer_id integer NOT NULL, ALTER TABLE ONLY public.coinmarketcap_pro_backfill_assets
ADD CONSTRAINT coinmarketcap_pro_backfill_assets_run_id_fkey FOREIGN KEY (run_id) REFERENCES public.coinmarketcap_pro_backfill_runs(id) ON DELETE CASCADE;
+
+ALTER TABLE ONLY public.coinmarketcap_pro_backfill_assets
+ ADD CONSTRAINT coinmarketcap_pro_backfill_assets_project_id_fkey FOREIGN KEY (project_id) REFERENCES public.project(id);Also applies to: 9925-9930
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@priv/repo/structure.sql` around lines 997 - 1001, The
coinmarketcap_pro_backfill_assets table defines project_id as integer but
public.project.id is bigint; change the column type of project_id to bigint in
the migration for coinmarketcap_pro_backfill_assets (and the other affected
blocks around 9925-9930), and add a foreign key constraint referencing
public.project(id) (e.g., ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY
(project_id) REFERENCES public.project(id)); after modifying the migration,
regenerate structure.sql so the schema file and constraints are consistent.
Comment on lines
+1004
to
+1010
| status character varying(255) DEFAULT 'pending'::character varying NOT NULL, | ||
| missing_ranges jsonb DEFAULT '{}'::jsonb NOT NULL, | ||
| points_exported integer DEFAULT 0 NOT NULL, | ||
| api_credits_used double precision DEFAULT 0.0 NOT NULL, | ||
| api_calls_total integer DEFAULT 0 NOT NULL, | ||
| rate_limited_calls_total integer DEFAULT 0 NOT NULL, | ||
| usage_precision character varying(255) DEFAULT 'exact'::character varying NOT NULL, |
Contributor
There was a problem hiding this comment.
Mirror the state-machine validations in the database.
These workflow columns are plain varchars with no DB checks: asset status/usage_precision and run scope/status/usage_precision. The Ecto changesets validate them, but any raw SQL, backfill repair script, or manual update can still persist invalid states and wedge the run orchestration. Please add CHECK constraints (or enums) that match the schema-level validate_inclusion/2 rules.
Also applies to: 1045-1058
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@priv/repo/structure.sql` around lines 1004 - 1010, The DB schema allows
arbitrary strings for workflow columns (asset.status, asset.usage_precision and
run.scope, run.status, run.usage_precision) but the app enforces finite states
via validate_inclusion; add matching DB-level safeguards by creating ENUM types
or CHECK constraints for each column that exactly mirror the allowed values in
the Ecto changesets, ensure the current default values are included in those
sets, add a migration that first finds/fixes any existing rows with invalid
values before applying the constraint, and include meaningful constraint names
(e.g., asset_status_check, run_usage_precision_check) so future
migrations/reference can locate them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Ticket
Checklist:
Summary by CodeRabbit