diff --git a/keel/cli.py b/keel/cli.py index d1575c7d..32ab1029 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -763,13 +763,29 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N @assets_group.command("discover") @click.option("--quote", default=None, help="Settlement currency (default: config.quote_currency).") @click.option( - "--min-volume-24h", default="1000000", show_default=True, + "--min-volume-24h", default="100000", show_default=True, help="Cheap pre-filter on the venue's reported 24h quote volume. Bounds the request count; it " "is NOT a liquidity criterion -- a 24h snapshot is a different statistic from the median the " - "gate applies, so use --probe-liquidity for that. Set equal to the admission floor so " - "discovery cannot be stricter than the criterion it screens for.", + "gate applies, so use --probe-liquidity for that. Deliberately set well BELOW the admission " + "floor, not equal to it: a 24h snapshot and the gate's all-cached-history median are " + "different statistics, so an equal number does not make discovery non-stricter than the gate " + "it feeds -- a quiet trading day can push the snapshot under a floor the asset's own median " + "clears many times over.", +) +@click.option( + # Raised from 25 to 100 on 2026-08-15: lowering --min-volume-24h's floor (see that option's + # own help text) grew a typical sweep from ~35 candidates to ~130, sorted by descending 24h + # volume, so the assets that floor change exists to surface can land well past rank 25 and + # get cut off by this option before an operator ever sees them. 100 shows nearly all of a + # typical sweep at no extra cost: with NEITHER probe flag, `discover` makes exactly ONE venue + # request regardless of --limit -- filtering and sorting are local. --probe-history and + # --probe-liquidity are the ones with a per-row cost, called out below. + "--limit", default=100, show_default=True, + help="Show at most this many candidates. With neither --probe-history nor --probe-liquidity, " + "raising this costs nothing extra -- discovery still makes exactly one venue request. Each " + "probe flag adds one venue request PER CANDIDATE SHOWN (two requests per row if both are " + "given), so a large --limit combined with a probe flag multiplies the request count.", ) -@click.option("--limit", default=25, show_default=True, help="Show at most this many candidates.") @click.option( "--probe-history", is_flag=True, @@ -812,15 +828,27 @@ def assets_discover( quote_currency=quote or config.quote_currency, min_quote_24h_volume=Decimal(min_volume_24h), ) - candidates = screen_mod.discover_candidates( + result = screen_mod.discover_candidates( products, policy, exclude_assets=frozenset(config.allowlist) ) click.echo( - f"{len(products)} venue products -> {len(candidates)} candidates " + f"{len(products)} venue products -> {len(result.candidates)} candidates " f"(quote={policy.quote_currency}, 24h volume >= {policy.min_quote_24h_volume:,.0f}, " - f"excluding the current allowlist)\n" + f"excluding the current allowlist)" ) + click.echo(result.excluded.summary_line()) + # Never truncate silently: `result.candidates` above is the FULL survivor list (only the + # table loop below is cut to `limit`), so if there are more survivors than `limit` allows, + # say so explicitly -- how many exist, how many are about to be shown, and that --limit is + # the knob. A silent cap here would be exactly the defect class this command's own fix + # (the --min-volume-24h floor) exists to eliminate, just moved one step later in the pipeline. + if len(result.candidates) > limit: + click.echo( + f"showing {limit} of {len(result.candidates)} candidates -- raise --limit to see " + "the rest." + ) + click.echo("") screen_policy = screen_mod.ScreenPolicy() header = f"{'#':>3} {'product':<14} {'asset':<8} {'24h quote volume':>18}" if probe_history: @@ -832,7 +860,7 @@ def assets_discover( now_ts = int(time.time()) four_years_ago = now_ts - 4 * _DAYS_PER_YEAR * 86400 liquidity_window_start = now_ts - _LIQUIDITY_PROBE_DAYS * 86400 - for index, candidate in enumerate(candidates[:limit], start=1): + for index, candidate in enumerate(result.candidates[:limit], start=1): line = ( f"{index:>3} {candidate.product_id:<14} {candidate.asset:<8} " f"{candidate.quote_24h_volume:>18,.0f}" diff --git a/keel/commands/admission.py b/keel/commands/admission.py index 950a7b76..caa71162 100644 --- a/keel/commands/admission.py +++ b/keel/commands/admission.py @@ -35,6 +35,7 @@ from keel.commands._products import _default_sim_products from keel.compliance.screen import ( Candidate, + DiscoveryExclusions, DiscoveryPolicy, MarketFacts, ScreenResult, @@ -66,18 +67,56 @@ #: create the cycle `cli -> tui -> admission -> cli`. `test_build_discover_report_applies_ #: default_volume_floor_matching_assets_discover` reads the CLI option's own default and asserts #: it equals this constant, so the two cannot silently drift apart. -#: Discovery's 24h-volume pre-filter, pinned EQUAL to `ScreenPolicy.min_median_daily_volume` so a -#: sweep can never be stricter than the gate it feeds. It bounds how many products get probed; it -#: is not a liquidity verdict (that is `--probe-liquidity`, which computes the gate's own median). +#: +#: Discovery's 24h-volume pre-filter. It bounds how many products get probed for history; it is +#: not a liquidity verdict (that is `--probe-liquidity`, which computes the gate's own median). #: #: Was 5,000,000 until 2026-08-08. At that floor the sweep returned 9 candidates and exactly one #: unsettled survivor; at a lower floor, seven more cleared BOTH mechanical gates -- FET among them #: at $2.94M/24h, i.e. invisible to the sweep while measuring 4.8x the admission floor. The floor, #: not the market, was the binding constraint on the candidate pipeline. #: -#: `tests/commands/test_admission.py` pins this to the CLI option, to `DiscoveryPolicy`'s default -#: and to the admission floor; all four move together or the suite fails. -DEFAULT_MIN_QUOTE_24H_VOLUME = Decimal("1000000") +#: The fix that followed pinned this EQUAL to `ScreenPolicy.min_median_daily_volume` +#: (1,000,000), reasoning that a sweep pinned to the gate's own floor could never be stricter +#: than the gate it feeds. That reasoning got the INTENT right and the MECHANISM wrong: the two +#: floors measure DIFFERENT statistics -- this one is a single 24-hour venue snapshot, the +#: admission floor is the median of volume x close over ALL cached history -- so an equal number +#: does nothing to stop a quiet trading day from pushing the snapshot below a floor the asset's +#: own median clears many times over. Measured 2026-08-15: five assets (ATOM, AAVE, BCH, CRV, +#: ALGO) were silently dropped by the equal-floor sweep despite each measuring 3.1x-6.3x the +#: admission floor on the gate's own statistic; four of the five sat in an 852,133-979,000 24h +#: snapshot cluster on that single quiet day. +#: +#: The floor is now strictly BELOW the admission floor, by an order of magnitude, so a quiet-day +#: snapshot has real room before it can hide an asset the gate would admit. See +#: `keel.compliance.screen.DiscoveryPolicy.min_quote_24h_volume`, which carries the identical +#: reasoning next to the number it actually applies. +#: +#: `tests/commands/test_admission.py` pins this to the CLI option, to `DiscoveryPolicy`'s default, +#: and to being strictly less than the admission floor; all of those move together or the suite +#: fails. +DEFAULT_MIN_QUOTE_24H_VOLUME = Decimal("100000") + +#: Mirrors `keel assets discover --limit`'s own default (`keel/cli.py::assets_discover`), for the +#: same "literal here, not an import from `keel.cli`" reason `DEFAULT_MIN_QUOTE_24H_VOLUME` gives +#: above. `test_build_discover_report_applies_default_limit_matching_assets_discover` pins the two +#: together. +#: +#: Was 25, set back when `--min-volume-24h`'s floor was 1,000,000 and a sweep returned ~35 +#: candidates -- 25 showed nearly all of them. Lowering that floor to 100,000 (see +#: `DEFAULT_MIN_QUOTE_24H_VOLUME` above) grew a typical sweep to ~130 candidates, all sorted by +#: descending 24h volume, so the five assets that floor change exists to surface (ATOM, AAVE, +#: BCH, CRV, ALGO -- see that constant's docstring) landed at ranks 33-59: below the ~35 still +#: above the OLD floor, and past a limit of 25. The floor fix was real but invisible at the +#: operator's own default view. +#: +#: 100 costs nothing extra on its own: with neither `--probe-history` nor `--probe-liquidity`, +#: `assets discover` makes exactly ONE venue request (`list_products`) regardless of `--limit` -- +#: the candidate list is filtered and sorted locally. The two probe flags are the ones with a +#: per-row cost (one venue request EACH per candidate SHOWN, so two together), which is why that +#: trade-off is called out in `--limit`'s own `help=` text rather than left for an operator to +#: discover by combining the flags and watching the request count climb. +DEFAULT_DISCOVER_LIMIT = 100 # -- 2a. shortlist location (offline) ------------------------------------------------------------ @@ -380,13 +419,18 @@ class DiscoverReport: venue_product_count: int candidates: list[Candidate] min_quote_24h_volume: Decimal + #: Per-reason exclusion counts over the WHOLE sweep -- every product in `venue_product_count` + #: that did not become a candidate, not just the ones cut by `limit` below. See + #: `build_discover_report`'s docstring for why that distinction matters when reading this + #: field alongside `candidates`. + excluded: DiscoveryExclusions def build_discover_report( products: list[dict], config: Config, *, - limit: int = 25, + limit: int = DEFAULT_DISCOVER_LIMIT, min_quote_24h_volume: Decimal | None = None, ) -> DiscoverReport: """PURE over `products` -- the caller's already-fetched venue metadata. Builds no broker and @@ -396,7 +440,18 @@ def build_discover_report( Uses `screen.discover_candidates` with a `DiscoveryPolicy` built from `config.quote_currency` and the volume floor, excluding `config.allowlist` -- mirroring `assets_discover` in `keel/cli.py`. `min_quote_24h_volume` defaults to `DEFAULT_MIN_QUOTE_24H_VOLUME`, matching - `assets discover`'s own CLI default (`5000000`). + `assets discover`'s own CLI default (`100000`); `limit` defaults to `DEFAULT_DISCOVER_LIMIT`, + matching its `--limit` default (`100`). + + `DiscoverReport.candidates` is truncated to `limit`, but `DiscoverReport.excluded` is NOT -- + it is the per-reason count over the WHOLE sweep, every product in `venue_product_count` that + did not become a candidate at all. Read the two together with that in mind: `excluded.total` + plus `len(candidates)` before truncation equals `venue_product_count`, not + `len(report.candidates)` plus `excluded.total` -- the shown rows are a further cut of the + survivors, not of the population `excluded` is counted against. `render_discover_report` + relies on exactly that identity (`venue_product_count - excluded.total` recovers the + pre-truncation survivor count) to report when `limit` has cut the table, without this + dataclass needing a redundant field to carry the same number twice. """ floor = ( min_quote_24h_volume @@ -404,14 +459,15 @@ def build_discover_report( else DEFAULT_MIN_QUOTE_24H_VOLUME ) policy = DiscoveryPolicy(quote_currency=config.quote_currency, min_quote_24h_volume=floor) - candidates = discover_candidates( + result = discover_candidates( products, policy, exclude_assets=frozenset(a.upper() for a in config.allowlist) ) return DiscoverReport( quote=policy.quote_currency, venue_product_count=len(products), - candidates=candidates[:limit], + candidates=result.candidates[:limit], min_quote_24h_volume=floor, + excluded=result.excluded, ) @@ -420,14 +476,28 @@ def render_discover_report(report: DiscoverReport) -> list[str]: with the SAME loud warning `keel assets discover` prints -- these are PROPOSALS, not admissions. Deliberately omits `--probe-history`'s per-candidate marker column: that is an extra network request per candidate, out of scope for this offline module (the caller already - made the one network call this workflow needs, to fetch `products`).""" + made the one network call this workflow needs, to fetch `products`). + + Also never truncates SILENTLY: `report.candidates` is already cut to whatever `limit` + `build_discover_report` was called with, and `venue_product_count - excluded.total` recovers + how many survivors there were before that cut (see `build_discover_report`'s docstring for + the identity this leans on). When that is more than `len(report.candidates)`, a line says so + -- how many candidates exist, how many are shown, and that `--limit`/`limit=` controls it -- + so a truncated table can never read as the whole candidate set.""" + survivor_count = report.venue_product_count - report.excluded.total lines = [ - f"{report.venue_product_count} venue products -> {len(report.candidates)} candidates " + f"{report.venue_product_count} venue products -> {survivor_count} candidates " f"(quote={report.quote}, 24h volume >= {report.min_quote_24h_volume:,.0f}, excluding " "the current allowlist)", - "", - f"{'#':>3} {'product':<14} {'asset':<8} {'24h quote volume':>18} name", + report.excluded.summary_line(), ] + if survivor_count > len(report.candidates): + lines.append( + f"showing {len(report.candidates)} of {survivor_count} candidates -- raise " + "--limit to see the rest." + ) + lines.append("") + lines.append(f"{'#':>3} {'product':<14} {'asset':<8} {'24h quote volume':>18} name") for index, candidate in enumerate(report.candidates, start=1): lines.append( f"{index:>3} {candidate.product_id:<14} {candidate.asset:<8} " diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index b974c70f..5feaaa44 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -471,7 +471,22 @@ class DiscoveryPolicy: """ quote_currency: str = "USD" - min_quote_24h_volume: Decimal = Decimal("1000000") + #: DISCOVERY's floor, deliberately an order of magnitude BELOW the admission floor + #: `ScreenPolicy.min_median_daily_volume` (1,000,000), and deliberately NOT equal to it. The + #: two numbers measure different statistics -- this one is a single 24-hour venue snapshot, + #: while the admission floor is the median of volume x close over ALL cached history -- so + #: pinning them equal does not make the pre-filter non-stricter than the gate it feeds: a + #: quiet trading day can push the snapshot below a floor the asset's own median clears many + #: times over. + #: + #: Measured 2026-08-15: four of five wrongly-dropped assets (ATOM, BCH, CRV, ALGO) sat in an + #: 852,133-979,000 24h-snapshot cluster on a single quiet day, while their median-over-history + #: liquidity was 3.1x-6.3x the admission floor. 100,000 sits comfortably below that cluster, + #: with room to spare for a quieter day still. + #: + #: This bounds how many products get probed for history; it is not a liquidity verdict. + #: `--probe-liquidity` computes the gate's own statistic and is the honest estimator of that. + min_quote_24h_volume: Decimal = Decimal("100000") def median_daily_quote_volume(candles: Sequence[Any]) -> Decimal: @@ -496,43 +511,153 @@ def median_daily_quote_volume(candles: Sequence[Any]) -> Decimal: return volumes[len(volumes) // 2] +@dataclass(frozen=True) +class DiscoveryExclusions: + """Per-reason counts of every venue product `discover_candidates` dropped, in the SAME + declaration order the checks run in. + + `discover_candidates` used to drop an excluded product with a bare `continue`, so nothing + recorded WHY a product vanished or how many were lost to each reason -- an operator watching + `900 -> 40` had no way to tell whether the missing 860 were junk (wrong quote currency, + offline, disabled) or real candidates sitting just under the liquidity floor. This type is + the fix: every reason is a named field, counted exactly once per product (the FIRST check it + fails, never a later one it would also have failed), so the sweep's own output can explain + itself. + + Every field defaults to 0 rather than the type being optional or partial, because a reason + reading 0 is itself informative -- it tells the operator that reason was checked and simply + did not fire, which is different from a reason that was never evaluated at all. `counts()` + and `summary_line()` both list every reason unconditionally for the same cause: a summary + line whose shape changes depending on which reasons happened to be nonzero would make the + line itself something an operator has to learn to parse, rather than a fixed shape they can + scan every time. + """ + + wrong_quote_currency: int = 0 + not_online: int = 0 + trading_disabled: int = 0 + view_only: int = 0 + already_on_allowlist: int = 0 + unreadable_volume: int = 0 + below_volume_floor: int = 0 + + @property + def total(self) -> int: + return ( + self.wrong_quote_currency + + self.not_online + + self.trading_disabled + + self.view_only + + self.already_on_allowlist + + self.unreadable_volume + + self.below_volume_floor + ) + + def counts(self) -> tuple[tuple[str, int], ...]: + """`(human label, count)` pairs in declaration order. The single source of truth for the + label text, so `summary_line()` and any future renderer (the TUI overlay, say) cannot + drift into naming the same reason two different ways.""" + return ( + ("wrong quote currency", self.wrong_quote_currency), + ("not online", self.not_online), + ("trading disabled", self.trading_disabled), + ("view only", self.view_only), + ("already on allowlist", self.already_on_allowlist), + ("unreadable 24h volume", self.unreadable_volume), + ("below 24h volume floor", self.below_volume_floor), + ) + + def summary_line(self) -> str: + """`"excluded N: reason1 n1, reason2 n2, ..."` -- every reason named, including the zero + ones, for `counts()`'s reason above.""" + return f"excluded {self.total}: " + ", ".join(f"{label} {n}" for label, n in self.counts()) + + +@dataclass(frozen=True) +class DiscoveryResult: + """`discover_candidates`'s return value: the survivors, plus a full accounting of everyone + who did not survive and why. See `DiscoveryExclusions` for the accounting half.""" + + candidates: list[Candidate] + excluded: DiscoveryExclusions + + def discover_candidates( products: list[dict], policy: DiscoveryPolicy | None = None, exclude_assets: frozenset[str] | None = None, -) -> list[Candidate]: +) -> DiscoveryResult: """Propose candidates from venue metadata. **Proposes only — admits nothing.** §5's asymmetry: a proposal may come from anywhere, but activity may only INCREASE through the deterministic gate. Nothing here checks sector or backing, and nothing here may be read as approval — every survivor still has to clear `screen_asset`, which fails closed without a human attestation. + + Returns a `DiscoveryResult` rather than a bare `list[Candidate]` (its shape before this + accounting existed) precisely so that adding `excluded` could not become a silent trap for an + un-updated caller. A tuple `(candidates, excluded)` would have let an old call site that still + expected a list either mis-index (`result[0]` now the whole result, not the first candidate) + or iterate the wrong thing -- both wrong answers that run without complaint. A named + dataclass makes the same old call site fail LOUDLY at the attribute access + (`AttributeError: 'DiscoveryResult' object has no attribute 'asset'`) instead of silently + reading nonsense, which is the same fail-closed instinct the rest of this module applies to + admission itself. + + Each dropped product counts against the FIRST criterion order below that it fails, never a + later one it would also have failed -- so a product excluded for the wrong quote currency is + not ALSO counted as thin, even though it may be. `trading_disabled` and `is_disabled` are two + venue flags that mean the same thing to an operator and both land in the single + `trading_disabled` reason; a volume that fails to parse as a `Decimal` lands in + `unreadable_volume`, distinct from `below_volume_floor`, which is a volume that parsed fine + but did not clear `policy.min_quote_24h_volume`. + + Deliberately PURE and OFFLINE: no network call, no DB read or write, no logging. `products` + is the caller's already-fetched venue metadata, and this function's only job is to filter and + count it -- see the module-level split between computed facts and attested judgement for why + keeping side effects out of a function like this matters generally, and `keel/commands/ + admission.py`'s module docstring for why THIS function in particular has to be safely + callable with nothing but a fake product list in a test. """ policy = policy or DiscoveryPolicy() exclude = exclude_assets or frozenset() out: list[Candidate] = [] + wrong_quote_currency = 0 + not_online = 0 + trading_disabled = 0 + view_only = 0 + already_on_allowlist = 0 + unreadable_volume = 0 + below_volume_floor = 0 for product in products: product_id = product.get("product_id") or "" if (product.get("quote_currency_id") or "").upper() != policy.quote_currency.upper(): + wrong_quote_currency += 1 continue if product.get("status") != "online": + not_online += 1 continue if product.get("trading_disabled") or product.get("is_disabled"): + trading_disabled += 1 continue if product.get("view_only"): + view_only += 1 continue asset = product_id.split("-")[0] if asset in exclude: + already_on_allowlist += 1 continue raw_volume = product.get("quote_24h_volume") try: volume = Decimal(str(raw_volume)) except (TypeError, ArithmeticError, ValueError): + unreadable_volume += 1 continue if volume < policy.min_quote_24h_volume: + below_volume_floor += 1 continue out.append( @@ -544,4 +669,15 @@ def discover_candidates( ) ) - return sorted(out, key=lambda c: c.quote_24h_volume, reverse=True) + return DiscoveryResult( + candidates=sorted(out, key=lambda c: c.quote_24h_volume, reverse=True), + excluded=DiscoveryExclusions( + wrong_quote_currency=wrong_quote_currency, + not_online=not_online, + trading_disabled=trading_disabled, + view_only=view_only, + already_on_allowlist=already_on_allowlist, + unreadable_volume=unreadable_volume, + below_volume_floor=below_volume_floor, + ), + ) diff --git a/tests/commands/test_admission.py b/tests/commands/test_admission.py index 792add02..61492f20 100644 --- a/tests/commands/test_admission.py +++ b/tests/commands/test_admission.py @@ -26,6 +26,7 @@ import keel.cli as cli_module from keel.commands.admission import ( + DEFAULT_DISCOVER_LIMIT, DEFAULT_MIN_QUOTE_24H_VOLUME, DEFAULT_PROPOSALS_DIR, DiscoverReport, @@ -646,18 +647,20 @@ def test_build_discover_report_excludes_allowlist_assets(repo: Repository): def test_build_discover_report_applies_default_volume_floor_matching_assets_discover(repo): - """`keel assets discover --min-volume-24h` defaults to `5000000` -- read straight from the - CLI option's own default (by name, not position, so a decorator reorder cannot silently - break this pin) so the two can never drift apart silently.""" + """`keel assets discover --min-volume-24h`'s default and `build_discover_report`'s own + default must be the SAME number -- read straight from the CLI option's own default (by + name, not position, so a decorator reorder cannot silently break this pin) and compare it + against `DEFAULT_MIN_QUOTE_24H_VOLUME` rather than a hardcoded literal, so a future change + to either constant fails this test instead of silently drifting the two apart.""" cli_option = next( p for p in cli_module.assets_discover.params if p.name == "min_volume_24h" ) default_floor = Decimal(cli_option.default) - assert default_floor == Decimal("1000000") + assert default_floor == DEFAULT_MIN_QUOTE_24H_VOLUME config = _config(allowlist=[]) - below = _venue_product("DOGE", volume="999999") - above = _venue_product("SHIB", volume="1000001") + below = _venue_product("DOGE", volume="99999") + above = _venue_product("SHIB", volume="100001") report = build_discover_report([below, above], config) @@ -690,6 +693,55 @@ def test_build_discover_report_limit_respected(repo: Repository): assert report.candidates[1].quote_24h_volume >= report.candidates[2].quote_24h_volume +def test_build_discover_report_applies_default_limit_matching_assets_discover(): + """`keel assets discover --limit`'s default and `build_discover_report`'s own default must + be the SAME number, for the identical reason + `test_build_discover_report_applies_default_volume_floor_matching_assets_discover` pins the + volume floor: read straight from the CLI option's own default, compared against + `DEFAULT_DISCOVER_LIMIT` rather than a hardcoded literal, so a future change to either one + fails this test instead of silently drifting the two apart.""" + cli_option = next(p for p in cli_module.assets_discover.params if p.name == "limit") + assert cli_option.default == DEFAULT_DISCOVER_LIMIT + + +def test_default_discover_limit_is_100(): + """Pins the new default value itself. Raised from 25 now that a lower --min-volume-24h + surfaces ~130 candidates instead of ~35 -- 25 would hide most of a typical sweep.""" + assert DEFAULT_DISCOVER_LIMIT == 100 + + +def test_render_discover_report_states_total_and_shown_when_limit_truncates(repo: Repository): + """The load-bearing case: more candidates exist than `limit` shows. The rendered output must + say the true survivor count, the shown count, and that --limit controls the cut -- never let + a truncated table read as the whole candidate set.""" + config = _config(allowlist=[]) + products = [_venue_product(f"COIN{i}", volume=str(10_000_000 + i)) for i in range(10)] + + report = build_discover_report(products, config, limit=3) + lines = render_discover_report(report) + text = "\n".join(lines) + + assert "10 venue products -> 10 candidates" in text + assert "showing 3 of 10 candidates" in text + assert "--limit" in text + assert len(report.candidates) == 3 + + +def test_render_discover_report_no_truncation_notice_when_everything_fits(repo: Repository): + """No false alarm: when every survivor fits under `limit`, nothing should claim a cut + happened.""" + config = _config(allowlist=[]) + products = [_venue_product(f"COIN{i}", volume=str(10_000_000 + i)) for i in range(3)] + + report = build_discover_report(products, config, limit=25) + lines = render_discover_report(report) + text = "\n".join(lines) + + assert "3 venue products -> 3 candidates" in text + assert "showing" not in text + assert len(report.candidates) == 3 + + def test_render_discover_report_shows_table_and_ends_with_proposals_not_admissions_warning(): config = _config(allowlist=[]) products = [_venue_product("DOGE")] @@ -703,6 +755,42 @@ def test_render_discover_report_shows_table_and_ends_with_proposals_not_admissio assert "keel assets attest" in text +def test_build_discover_report_excluded_carries_per_reason_counts(repo: Repository): + """`DiscoverReport.excluded` is the thing that makes a `900 -> 40` sweep summary honest -- + without it, nothing distinguishes 860 products dropped for being junk (wrong quote + currency, offline, already on the allowlist) from 860 products dropped just under the + liquidity floor. One allowlisted product and one below-floor product are enough to pin that + each reason lands in its own bucket rather than a single opaque total.""" + config = _config(allowlist=["BTC"]) + below_floor = _venue_product("DOGE", volume="1") + already_allowlisted = _venue_product("BTC") + survivor = _venue_product("SOL") + + report = build_discover_report([below_floor, already_allowlisted, survivor], config) + + assert report.excluded.already_on_allowlist == 1 + assert report.excluded.below_volume_floor == 1 + assert [c.asset for c in report.candidates] == ["SOL"] + + +def test_render_discover_report_includes_the_exclusion_summary(repo: Repository): + """The rendered report must surface WHY products were dropped, not just how many candidates + survived -- otherwise an operator reading `900 -> 40` has no way to tell junk from + near-misses on the floor.""" + config = _config(allowlist=["BTC"]) + below_floor = _venue_product("DOGE", volume="1") + already_allowlisted = _venue_product("BTC") + survivor = _venue_product("SOL") + + report = build_discover_report([below_floor, already_allowlisted, survivor], config) + lines = render_discover_report(report) + text = "\n".join(lines) + + assert report.excluded.summary_line() in text + assert "already on allowlist 1" in text + assert "below 24h volume floor 1" in text + + def test_render_discover_report_never_includes_probe_history_marker(): """Out of scope by design: `--probe-history` is one extra network request per candidate, which this offline module must never make.""" @@ -717,12 +805,15 @@ def test_render_discover_report_never_includes_probe_history_marker(): def test_every_discovery_floor_default_agrees(): - """Three modules carry this default; a drift between them is silent and changes the sweep. - - `cli.assets_discover`'s option, `admission.DEFAULT_MIN_QUOTE_24H_VOLUME` and - `screen.DiscoveryPolicy` each name a floor. The first two were already pinned to each other; - `DiscoveryPolicy` was not, so a caller constructing one directly (as `cli.assets_discover` - does) could silently use a different floor from `build_discover_report`. + """Three DISCOVERY places carry this default; a drift between them is silent and changes + the sweep. `cli.assets_discover`'s `--min-volume-24h` option, `admission. + DEFAULT_MIN_QUOTE_24H_VOLUME` and `screen.DiscoveryPolicy.min_quote_24h_volume` each name + the same pre-filter floor and must move together. + + These three are deliberately NOT pinned to the admission floor + (`ScreenPolicy.min_median_daily_volume`) -- see + `test_the_discovery_floor_is_strictly_below_the_admission_liquidity_floor` for why equal + numbers there would be wrong, not merely a coincidence worth asserting. """ from keel.compliance.screen import DiscoveryPolicy @@ -732,14 +823,31 @@ def test_every_discovery_floor_default_agrees(): assert DiscoveryPolicy().min_quote_24h_volume == DEFAULT_MIN_QUOTE_24H_VOLUME -def test_the_discovery_floor_matches_the_admission_liquidity_floor(): - """Discovery should not hide assets the gate would admit. - - The sweep's floor is a 24h snapshot and the gate's is a median over history -- different - statistics (which is why `--probe-liquidity` exists). Pinning the two NUMBERS equal keeps the - pre-filter from being stricter than the criterion it screens for: at the old 5x-higher floor, - FET sat at $2.94M/24h and never appeared, while measuring 4.8x the admission floor. +def test_the_discovery_floor_is_strictly_below_the_admission_liquidity_floor(): + """Discovery's pre-filter must not be able to hide an asset the gate would admit. + + The sweep's floor (`DEFAULT_MIN_QUOTE_24H_VOLUME`) and the gate's + (`ScreenPolicy.min_median_daily_volume`) are DIFFERENT statistics: a 24h venue snapshot + versus the median of volume x close over all cached history. Pinning them to the SAME + number, as this codebase used to, does not make the pre-filter non-stricter -- a quiet + trading day can push the snapshot below a floor the asset's own median clears many times + over. + + This was tried twice and failed twice. First at 5,000,000, retired 2026-08-08: FET sat at + $2.94M/24h and never appeared in a sweep, despite measuring 4.8x the admission floor on the + gate's own statistic. Lowering the two NUMBERS to agree (1,000,000, matching the admission + floor exactly) looked like a fix but was not one -- it only shrank the gap, because equal + numbers on different statistics still let a quiet day hide an asset. Measured again on + 2026-08-15: ATOM (3.08x the admission floor on its real median), AAVE (6.32x), BCH (5.46x), + CRV (3.33x) and ALGO (3.78x) were all silently dropped, four of them with 24h snapshots + clustered between 852,133 and 979,000 on a single quiet day -- comfortably under the + 1,000,000 floor despite being multiples above the admission requirement on the statistic + that actually gates them. + + The corrected mechanism: discovery's floor must be STRICTLY LESS than the gate's, not + merely equal to it, precisely because the two sides measure different things and only a + genuine margin protects a quiet-day snapshot from undercutting a healthy median. """ from keel.compliance.screen import ScreenPolicy - assert DEFAULT_MIN_QUOTE_24H_VOLUME == ScreenPolicy().min_median_daily_volume + assert DEFAULT_MIN_QUOTE_24H_VOLUME < ScreenPolicy().min_median_daily_volume diff --git a/tests/commands/test_tui.py b/tests/commands/test_tui.py index 0f70d190..746adc49 100644 --- a/tests/commands/test_tui.py +++ b/tests/commands/test_tui.py @@ -2096,6 +2096,7 @@ def test_build_discover_overlay_with_report_is_nonempty_titled_and_headed() -> N venue_product_count=900, candidates=[candidate], min_quote_24h_volume=Decimal("5000000"), + excluded=screen_mod.DiscoveryExclusions(below_volume_floor=899), ) lines = build_discover_overlay(report) diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index a3207a1e..400e5ef8 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -419,6 +419,35 @@ def test_discover_proposes_and_says_so_loudly(tmp_path, valid_config_path, monke assert "attest" in result.output +def test_discover_reports_the_exclusion_summary(tmp_path, valid_config_path, monkeypatch): + """`discover_candidates` used to drop excluded products with a bare `continue`, so `keel + assets discover`'s output never said WHY a product vanished between the venue's product + count and the candidate table -- only the bare `900 -> 40` header line. This pins that the + CLI now echoes a per-reason summary alongside that header: one product survives, one is + excluded for the wrong quote currency, and one is excluded for sitting below the 24h + volume floor.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + venue = _FakeVenue( + [ + _venue_product("SOL-USD", "50000000"), + _venue_product("EURPAIR-EUR", "50000000", quote_currency_id="EUR"), + _venue_product("THIN-USD", "1"), + ] + ) + monkeypatch.setattr(cli_module, "_build_broker", lambda config: venue) + + result = CliRunner().invoke( + cli, ["--db", str(db_path), "--config", str(valid_config_path), "assets", "discover"] + ) + + assert result.exit_code == 0, result.output + assert "SOL" in result.output + assert "wrong quote currency 1" in result.output + assert "below 24h volume floor 1" in result.output + assert "excluded 2" in result.output + + def test_discover_excludes_the_current_allowlist(tmp_path, valid_config_path, monkeypatch): db_path = tmp_path / "t.db" _repo_at(db_path) @@ -434,6 +463,60 @@ def test_discover_excludes_the_current_allowlist(tmp_path, valid_config_path, mo assert "BTC-USD" not in result.output +def test_discover_default_limit_is_100(): + """Pins the new default. Raised from 25 now that a lower --min-volume-24h surfaces ~130 + candidates instead of ~35 -- 25 would cut off most of a typical sweep before the operator + ever sees it.""" + cli_option = next(p for p in cli_module.assets_discover.params if p.name == "limit") + assert cli_option.default == 100 + + +def test_discover_states_total_and_shown_when_limit_truncates( + tmp_path, valid_config_path, monkeypatch +): + """The load-bearing case, verified at the CLI: with more candidates than --limit shows, the + output must say the true candidate count, how many are shown, and that --limit controls it -- + never let a truncated table read as the whole candidate set.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + products = [_venue_product(f"COIN{i}-USD", str(10_000_000 + i)) for i in range(5)] + venue = _FakeVenue(products) + monkeypatch.setattr(cli_module, "_build_broker", lambda config: venue) + + result = CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "discover", "--limit", "3"], + ) + + assert result.exit_code == 0, result.output + assert "5 venue products -> 5 candidates" in result.output + assert "showing 3 of 5 candidates" in result.output + assert "--limit" in result.output + shown_rows = [ln for ln in result.output.splitlines() if "COIN" in ln and "-USD" in ln] + assert len(shown_rows) == 3 + + +def test_discover_no_truncation_notice_when_everything_fits( + tmp_path, valid_config_path, monkeypatch +): + """No false alarm: when every candidate fits under --limit, nothing should claim a cut + happened.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + products = [_venue_product(f"COIN{i}-USD", str(10_000_000 + i)) for i in range(3)] + venue = _FakeVenue(products) + monkeypatch.setattr(cli_module, "_build_broker", lambda config: venue) + + result = CliRunner().invoke( + cli, ["--db", str(db_path), "--config", str(valid_config_path), "assets", "discover"] + ) + + assert result.exit_code == 0, result.output + assert "3 venue products -> 3 candidates" in result.output + assert "showing" not in result.output + + def test_probe_history_marks_candidates_without_a_four_year_series( tmp_path, valid_config_path, monkeypatch ): diff --git a/tests/compliance/test_screen.py b/tests/compliance/test_screen.py index 86893598..3aeda027 100644 --- a/tests/compliance/test_screen.py +++ b/tests/compliance/test_screen.py @@ -543,14 +543,14 @@ def test_discovery_keeps_liquid_online_products_in_the_settlement_currency(): from keel.compliance.screen import discover_candidates found = discover_candidates([_product()]) - assert [c.asset for c in found] == ["SOL"] + assert [c.asset for c in found.candidates] == ["SOL"] def test_discovery_drops_the_wrong_quote_currency(): from keel.compliance.screen import discover_candidates - assert discover_candidates([_product(quote="USDC")]) == [] - assert discover_candidates([_product(quote="BTC")]) == [] + assert discover_candidates([_product(quote="USDC")]).candidates == [] + assert discover_candidates([_product(quote="BTC")]).candidates == [] def test_discovery_drops_untradable_products(): @@ -562,20 +562,20 @@ def test_discovery_drops_untradable_products(): {"is_disabled": True}, {"view_only": True}, ): - assert discover_candidates([_product(**kwargs)]) == [], kwargs + assert discover_candidates([_product(**kwargs)]).candidates == [], kwargs def test_discovery_drops_thin_products(): from keel.compliance.screen import discover_candidates - assert discover_candidates([_product(volume="100")]) == [] + assert discover_candidates([_product(volume="100")]).candidates == [] def test_discovery_survives_a_malformed_volume_rather_than_crashing(): from keel.compliance.screen import discover_candidates - assert discover_candidates([_product(volume=None)]) == [] - assert discover_candidates([_product(volume="n/a")]) == [] + assert discover_candidates([_product(volume=None)]).candidates == [] + assert discover_candidates([_product(volume="n/a")]).candidates == [] def test_discovery_excludes_assets_we_already_hold(): @@ -584,7 +584,7 @@ def test_discovery_excludes_assets_we_already_hold(): found = discover_candidates( [_product("BTC-USD"), _product("SOL-USD")], exclude_assets=frozenset({"BTC"}) ) - assert [c.asset for c in found] == ["SOL"] + assert [c.asset for c in found.candidates] == ["SOL"] def test_discovery_ranks_by_liquidity(): @@ -593,14 +593,14 @@ def test_discovery_ranks_by_liquidity(): found = discover_candidates( [_product("A-USD", volume="10000000"), _product("B-USD", volume="90000000")] ) - assert [c.asset for c in found] == ["B", "A"] + assert [c.asset for c in found.candidates] == ["B", "A"] def test_discovery_proposes_but_never_admits(): """A discovered candidate is still REJECTED by the screen until a human attests it.""" from keel.compliance.screen import discover_candidates - (candidate,) = discover_candidates([_product()]) + (candidate,) = discover_candidates([_product()]).candidates result = screen_asset(_facts(asset=candidate.asset), None) assert result.admitted is False @@ -611,10 +611,124 @@ def test_discovery_matches_the_quote_currency_case_insensitively(): from keel.compliance.screen import DiscoveryPolicy, discover_candidates lowercase_venue = _product(pid="SOL-USD", quote="usd") - assert discover_candidates([lowercase_venue]), "lowercase venue quote id dropped everything" + assert discover_candidates( + [lowercase_venue] + ).candidates, "lowercase venue quote id dropped everything" assert discover_candidates( [_product(pid="SOL-USD", quote="USD")], DiscoveryPolicy(quote_currency="usd") - ), "lowercase configured quote currency dropped everything" + ).candidates, "lowercase configured quote currency dropped everything" + + +def test_a_quiet_days_snapshot_no_longer_hides_an_asset_the_gate_would_admit(): + """The 2026-08-15 regression this default change fixes. + + `--min-volume-24h` used to default to `Decimal("1000000")`, pinned EQUAL to the admission + floor `ScreenPolicy.min_median_daily_volume`. That equality does not make the pre-filter + non-stricter than the gate it feeds, because the two sides measure DIFFERENT statistics: + discovery's `quote_24h_volume` is a single 24-hour venue snapshot, while the gate's + `median_daily_volume` is the median of volume x close over ALL cached history. A quiet + trading day can push the snapshot below a floor the asset clears comfortably on a typical + day, and an equal number does nothing to prevent that. + + Measured on 2026-08-15, five assets were silently dropped by discovery at the old + 1,000,000 floor despite the gate's own statistic sitting far above the admission floor: + ATOM 3,077,474 (3.08x), AAVE 6,315,463 (6.32x), BCH 5,464,940 (5.46x), CRV 3,329,753 + (3.33x) and ALGO 3,780,207 (3.78x). Four of the five -- all but AAVE -- had 24h snapshots + clustered between 852,133 and 979,000 on that single quiet day, i.e. comfortably below the + old floor while their median-over-history liquidity was multiples of the admission + requirement. + + This test uses ATOM's actual measured 24h figure, `852133`: well under the OLD floor of + 1,000,000 but above the new default of 100,000. Under the default `DiscoveryPolicy()` it + must survive the pre-filter, so the sweep can no longer hide an asset the gate would admit. + """ + from keel.compliance.screen import discover_candidates + + found = discover_candidates([_product(pid="ATOM-USD", volume="852133")]) + + assert [c.asset for c in found.candidates] == ["ATOM"] + + +def test_discovery_counts_every_exclusion_by_reason(): + """`discover_candidates` used to drop excluded products with a bare `continue`, so nothing + recorded WHY a product was excluded or how many were. That made a quiet-day floor problem + (see `test_a_quiet_days_snapshot_no_longer_hides_an_asset_the_gate_would_admit`) invisible + in the sweep's own output -- an operator watching `900 -> 40` had no way to tell whether the + missing 860 were junk (wrong quote currency, offline, disabled) or real candidates sitting + just under the floor. + + Exactly ONE product is fed per reason, first-match-wins in the declaration order of + `DiscoveryExclusions`, plus one clean survivor -- so every count below is 1, the total is the + number of reasons, and a product excluded for one reason is pinned NOT to be double-counted + against a later check it would also fail. The `trading_disabled` reason covers two venue + flags (`trading_disabled` and `is_disabled`) and is therefore fed only its first variant + here; that the second lands in the same bucket rather than a reason of its own is pinned + separately by `test_discovery_counts_both_trading_disabled_flag_variants_together`, which + keeps this test's one-product-per-reason arithmetic honest. + """ + from keel.compliance.screen import discover_candidates + + products = [ + _product("WRONGQ-USD", quote="EUR"), + _product("OFFLINE-USD", status="offline"), + _product("HALTED-USD", trading_disabled=True), + _product("VIEWONLY-USD", view_only=True), + _product("BTC-USD"), # excluded via `exclude_assets` below: already on the allowlist + _product("BADVOL-USD", volume="not-a-number"), + _product("THIN-USD", volume="1"), + _product("SOL-USD"), # the one survivor + ] + + result = discover_candidates(products, exclude_assets=frozenset({"BTC"})) + + assert result.excluded.wrong_quote_currency == 1 + assert result.excluded.not_online == 1 + assert result.excluded.trading_disabled == 1 + assert result.excluded.view_only == 1 + assert result.excluded.already_on_allowlist == 1 + assert result.excluded.unreadable_volume == 1 + assert result.excluded.below_volume_floor == 1 + assert result.excluded.total == 7 + assert [c.asset for c in result.candidates] == ["SOL"] + + +def test_discovery_counts_both_trading_disabled_flag_variants_together(): + """`trading_disabled` covers BOTH the `trading_disabled` and `is_disabled` product flags -- + feeding one of each must total 2 under the single `trading_disabled` reason, not split + across a reason that does not exist.""" + from keel.compliance.screen import discover_candidates + + products = [ + _product("HALTED-USD", trading_disabled=True), + _product("DISABLED-USD", is_disabled=True), + ] + + result = discover_candidates(products) + + assert result.excluded.trading_disabled == 2 + assert result.excluded.total == 2 + assert result.candidates == [] + + +def test_discovery_exclusions_summary_line_names_every_reason_and_the_total(): + from keel.compliance.screen import DiscoveryExclusions + + exclusions = DiscoveryExclusions( + wrong_quote_currency=1, + not_online=1, + trading_disabled=1, + view_only=1, + already_on_allowlist=1, + unreadable_volume=1, + below_volume_floor=1, + ) + + line = exclusions.summary_line() + + assert line == ( + "excluded 7: wrong quote currency 1, not online 1, trading disabled 1, view only 1, " + "already on allowlist 1, unreadable 24h volume 1, below 24h volume floor 1" + ) # -- split_failures / missing_history_lines -------------------------------------------------