[TexasCoding]

Summary

fills and fills_all (sync + async) now live on PortfolioResource /
AsyncPortfolioResource, alongside settlements, deposits, and
withdrawals — the rest of the /portfolio/* family. The endpoint URL
(GET /portfolio/fills), response shape (Page[Fill]), and filters
(ticker, order_id, min_ts, max_ts, cursor, subaccount) are
identical to the previous location; only the access path changes.

OrdersResource.fills / OrdersResource.fills_all (and async counterparts)
remain in v3.0.0 as @typing_extensions.deprecated thin forwarders that
delegate to the same /portfolio/fills primitive and emit a
DeprecationWarning per call. They will be removed in a future release.

Issues closed

• Closes OrdersResource.fills / fills_all hits /portfolio/fills — belongs on PortfolioResource #351 — OrdersResource.fills / fills_all hit /portfolio/fills and belong on PortfolioResource.

Migration (BREAKING in v3.0.0)

Before (v2.x):

page = client.orders.fills(ticker="KXPRES-24-DJT")
for f in client.orders.fills_all(min_ts=1_700_000_000):
    ...

After (v3.0.0):

page = client.portfolio.fills(ticker="KXPRES-24-DJT")
for f in client.portfolio.fills_all(min_ts=1_700_000_000):
    ...

The old call sites still compile and run in v3.0.0, but each call emits a
single DeprecationWarning pointing at the new location.

Behavioral changes

• client.orders.fills(...) / client.orders.fills_all(...) now emit
  DeprecationWarning (one per call). Existing wire behavior is unchanged.
• client.portfolio.fills(...) / client.portfolio.fills_all(...) are new
  public methods with identical signatures and identical request shape.

Tests

• tests/test_portfolio.py — TestPortfolioFills, TestPortfolioFillsAll,
  TestAsyncPortfolioFills, TestAsyncPortfolioFillsAll cover the new
  location (happy path, cursor/filter parity, sync + async iterator).
• tests/test_orders.py::TestIssue351OrdersFillsDeprecated — asserts the
  old forwarders still resolve and emit exactly one DeprecationWarning
  per call.
• tests/test_async_orders.py::TestIssue351AsyncOrdersFillsDeprecated —
  same for the async forwarders.
• Pre-existing TestOrdersFills / TestAsyncOrdersFills test classes are
  preserved unchanged; they still cover the forwarder's wire behavior and
  produce informational DeprecationWarnings in test output.

Source

Round-3 independent audit closure plan, wave W4 (v3.0.0 breaking renames).

[claude]

Code Review — PR #376: Relocate fills/fills_all to PortfolioResource

Overall: The migration strategy is solid — @deprecated forwarders, clean new implementations, and good test coverage. However there are two correctness gaps that will fail CI and one structural issue worth fixing before merge.

---

Critical: Missing _contract_support.py updates

The new PortfolioResource.fills and PortfolioResource.fills_all methods are not registered in METHOD_ENDPOINT_MAP in tests/_contract_support.py. CLAUDE.md is explicit: "Drift tests hard-fail. Adding a new kwarg the spec doesn't have, or missing one the spec has, reds CI."

By comparison, every other portfolio method has entries:

# settlements, deposits, withdrawals are all registered; fills is not
sdk_method="kalshi.resources.portfolio.PortfolioResource.settlements",
sdk_method="kalshi.resources.portfolio.PortfolioResource.settlements_all",

The PR needs to add:

1. METHOD_ENDPOINT_MAP entries for PortfolioResource.fills, PortfolioResource.fills_all, and their async counterparts.
2. cursor exclusions for fills_all / AsyncPortfolioResource.fills_all — same pattern as settlements_all, deposits_all, etc.:

   ("kalshi.resources.portfolio.PortfolioResource.fills_all", "cursor"): Exclusion(reason="..."),
   ("kalshi.resources.portfolio.AsyncPortfolioResource.fills_all", "cursor"): Exclusion(reason="..."),

3. PortfolioResource.fills_all and AsyncPortfolioResource.fills_all added to _MAX_PAGES_FQNS alongside settlements_all, deposits_all, etc.

---

Structural: Duplicated _fills_params function

_fills_params is now defined identically in both kalshi/resources/orders.py and kalshi/resources/portfolio.py. The bodies are byte-for-byte the same (keyword args, _validate_limit(limit, hi=1000), and _params(...)). This is the exact duplication the _base_client.py shared helpers exist to prevent.

Options (ranked by invasiveness):

• Simplest (recommended): Import _fills_params from orders.py into portfolio.py. The dependency direction is a bit odd but avoids a shared-module change.
• Cleaner: Move _fills_params to kalshi/resources/_base.py alongside _params and _validate_limit, then import from both.

Either way, one copy should go.

---

Minor: Unnecessary inline # NOTE: comments in orders.py forwarders

The deprecated forwarders in orders.py gained # NOTE: still hits /portfolio/fills directly comments. Per CLAUDE.md: "Default to writing no comments. Only add one when the WHY is non-obvious." The docstring directly above each method already says exactly this. The inline comments don't add information and should be removed.

---

What's done well

• @deprecated from typing_extensions — correct tool for static-analysis-visible deprecation, triggers IDE warnings and mypy.
• Forwarder strategy — old call sites still compile and run in v3.0.0 with a single DeprecationWarning per call. Clean migration path.
• _fills_params in portfolio.py — correctly follows the _settlements_params pattern, including the hi=1000 limit cap.
• AsyncPortfolioResource.fills_all as def not async def — correct; _list_all returns an AsyncIterator directly, matching the established pattern on AsyncOrdersResource.
• Tests: TestPortfolioFills (wire params parity), TestPortfolioFillsAll (two-page pagination), TestIssue351*Deprecated (exactly-one-warning assertion), sync + async coverage. This is thorough.
• Docs: Both orders.md and portfolio.md updated in the same PR, and the migration note is clear and actionable.

---

Summary

	Status
_contract_support.py METHOD_ENDPOINT_MAP entries	❌ Missing
cursor / max_pages exclusions for new fills_all	❌ Missing
Duplicated _fills_params	⚠️ Should fix
Inline NOTE comments	⚠️ Remove
Tests	✅ Good coverage
Deprecation strategy	✅ Correct
Docs	✅ Updated

The first gap is a CI-hard-fail by design (the repo's own drift tests will catch it). The second is a structural debt item. Both are straightforward fixes.

[TexasCoding]

Round-2 review addressed in c490866: registered new PortfolioResource.fills/fills_all (sync + async) in METHOD_ENDPOINT_MAP and _MAX_PAGES_FQNS mirroring the existing OrdersResource entries. Old OrdersResource.fills/fills_all entries retained for the deprecated forwarders.

[claude]

Code Review — PR #376: Relocate fills/fills_all to PortfolioResource

Overall, this is a well-structured breaking change with a solid migration path. The deprecated forwarders, drift-test updates, and docs update are all handled correctly. A few things to address before merge:

---

Correctness / Bugs

_fills_params is duplicated verbatim between orders.py and portfolio.py

Both files define an identical _fills_params helper (same signature, same _validate_limit(limit, hi=1000), same _params(...) body). The PR correctly avoids importing it across resource files (which would be a weird dep direction), but the duplication means a future limit-change only gets fixed in one place. Suggest either:

• Moving it to kalshi/resources/_base.py alongside _params/_validate_limit (the natural home), or
• Leaving it duplicated with a comment noting they must stay in sync.

The current state is a silent footgun.

Auth-required tests are missing for the new portfolio.fills and portfolio.fills_all

Every other PortfolioResource method has a corresponding requires_auth guard test (e.g. test_positions_all_requires_auth on line 1131). The new TestPortfolioFills / TestPortfolioFillsAll / TestAsyncPortfolioFills / TestAsyncPortfolioFillsAll classes don't include one. Since _require_auth() is called in both methods, a test to confirm AuthRequiredError is raised without credentials would close that gap and match the existing pattern.

---

Style / Conventions (CLAUDE.md)

# NOTE: comments in deprecated forwarders are redundant

Each deprecated fills / fills_all body in orders.py now has:

# NOTE: still hits /portfolio/fills directly — the canonical
# implementation now lives on PortfolioResource (issue #351).

The docstring and @deprecated(...) message already communicate this. Per CLAUDE.md: "Only add [a comment] when the WHY is non-obvious." Here it's already explicit. These 6 comment blocks can be dropped.

Multi-line docstrings on methods slated for removal

The deprecated forwarders now carry verbose Sphinx-format docstrings. Per CLAUDE.md, docstrings that explain what (Use :meth:PortfolioResource.fills instead.) rather than why are unnecessary — especially for code that will be deleted in one release. The @deprecated(...) message on the decorator already surfaces in IDEs. The docstrings can be removed or collapsed to the single line the original had.

---

Contract Drift Tests

Cursor exclusion for AsyncPortfolioResource.fills_all is absent — intentionally?

The diff adds:

("kalshi.resources.portfolio.PortfolioResource.fills_all", "cursor"): Exclusion(...)

but no matching entry for AsyncPortfolioResource.fills_all. Looking at the existing map, async resources aren't in METHOD_ENDPOINT_MAP at all, so this is consistent with the rest of the codebase — just worth confirming it's intentional and not an oversight.

---

Tests — What's Good

• TestIssue351OrdersFillsDeprecated / TestIssue351AsyncOrdersFillsDeprecated correctly verify exactly one DeprecationWarning per call and match the deprecation message with r"OrdersResource\.fills".
• test_fills_cursor_and_filter_parity inspects raw wire params — this is the right level of rigor for a location move.
• Pagination is covered with a two-page mock in TestPortfolioFillsAll / TestAsyncPortfolioFillsAll.

---

Summary

Issue	Severity
_fills_params duplicated in orders.py and portfolio.py	Medium — silent maintenance trap
Auth-required tests missing for portfolio.fills / portfolio.fills_all	Low — coverage gap, existing forwarder tests cover the wire path
Redundant # NOTE: comments in deprecated forwarders	Nit (CLAUDE.md)
Verbose docstrings on to-be-deleted methods	Nit (CLAUDE.md)
Missing async cursor exclusion	Low — likely intentional, worth a comment in the code

[TexasCoding]

Round-2 review addressed in 0ff01cd: deduped _fills_params to kalshi/resources/_base.py; added 4 auth-required tests for new PortfolioResource fills variants; trimmed 6 redundant NOTE comments and collapsed forwarder docstrings to single-line. Deferred: missing async cursor exclusion note (consistent with rest of codebase, no async entries in METHOD_ENDPOINT_MAP).

[claude]

Code Review — PR #376: BREAKING(v3): relocate fills/fills_all to PortfolioResource

Overview

Clean, well-scoped relocation. The motivation is sound — GET /portfolio/fills belongs with the rest of the /portfolio/* family. The deprecation shim approach (thin forwarders with @typing_extensions.deprecated) is the right migration pattern. The contract map, exclusion lists, and docs are all updated in lockstep.

---

Issues

1. Multi-paragraph docstrings violate CLAUDE.md style ⚠️

kalshi/resources/portfolio.py — new fills() and fills_all() methods (sync and async) have multi-paragraph docstrings:

"""List trade fills (``GET /portfolio/fills``).

Moved from :class:`OrdersResource` in v3.0.0 (issue #351) to group
with the rest of the ``/portfolio/*`` family (``settlements``,
``deposits``, ``withdrawals``).
"""

CLAUDE.md is explicit: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max." Collapse to a single line, e.g.:

"""List trade fills (``GET /portfolio/fills``). Moved from OrdersResource in v3.0.0 (#351)."""

Same applies to the async variants.

---

2. Stray blank-line removal in portfolio.py — surgical violation

The diff removes a blank line between _settlements_params and class PortfolioResource. This line was not introduced by this PR and its removal doesn't trace to the stated goal. Per CLAUDE.md: "Don't 'improve' adjacent code, comments, or formatting."

---

3. Missing max_pages enforcement test for fills_all ⚠️

TestPortfolioFillsAll and TestAsyncPortfolioFillsAll cover the happy path and auth guard but skip max_pages cap enforcement. Compare TestPortfolioDeposits.test_all_max_pages_caps, which asserts that max_pages=1 stops after one page. The same gap exists in TestAsyncPortfolioFillsAll. All other *_all methods on PortfolioResource have this test.

def test_fills_all_max_pages_caps(self, portfolio: PortfolioResource) -> None:
    respx.get(...).mock(side_effect=[
        httpx.Response(200, json={"fills": [fill_dict(trade_id="a")], "cursor": "p2"}),
        # second page should never be fetched
    ])
    ids = [f.trade_id for f in portfolio.fills_all(max_pages=1)]
    assert ids == ["a"]

---

4. Missing filter-forwarding test for fills_all

TestPortfolioFills.test_fills_cursor_and_filter_parity verifies every query param reaches the wire for the single-page method. There's no equivalent for fills_all. Since fills_all builds params through _fills_params and then _list_all (which injects cursor internally), a test confirming ticker, order_id, min_ts, etc., are present in the first paginated request would close this gap — the same as TestPortfolioSettlements.test_settlements_all_with_all_new_filters.

---

5. Double blank line before TestIssue351OrdersFillsDeprecated in test_orders.py

Two consecutive blank lines appear before the # ── Issue #351 … comment block. PEP 8 / project style is one blank line between top-level test classes.

---

What's correct

• _fills_params correctly promoted to _base.py (single source of truth for both resources).
• @typing_extensions.deprecated correctly applied to all four forwarders (sync fills, sync fills_all, async fills, async fills_all).
• AsyncPortfolioResource.fills_all correctly returns AsyncIterator[Fill] as a non-async def — consistent with every other *_all on async resources.
• Contract map entries (sync PortfolioResource only, matching existing convention — async methods are not in METHOD_ENDPOINT_MAP).
• cursor exclusion added only for PortfolioResource.fills_all (sync), which is the only method actually in METHOD_ENDPOINT_MAP.
• AsyncPortfolioResource.fills_all added to _list_all_methods exclusion lists — correct.
• Deprecation tests assert exactly one warning per call — good.
• Docs updated in both orders.md and portfolio.md — clear and accurate.

---

Summary

Straightforward approval pending: fixing the docstring lengths (#1), restoring the blank line (#2), and adding the two missing test cases (#3, #4). The double blank line (#5) is cosmetic. Nothing blocking in logic or safety.

[claude]

Code Review — PR #376: Relocate fills/fills_all to PortfolioResource

Overview

Clean, well-scoped relocation of fills/fills_all (sync + async) from OrdersResource to PortfolioResource, where they semantically belong alongside settlements, deposits, and withdrawals. The deprecated forwarders, contract registrations, and docs all look solid. A few items worth flagging below.

---

Issues

1. Forwarders are not true delegations — they duplicate the endpoint call

OrdersResource.fills and PortfolioResource.fills both independently call self._list("/portfolio/fills", ...). They share params via _fills_params but are otherwise copies. The PR description says they "delegate to the same /portfolio/fills primitive," which is technically accurate but could mislead future contributors into thinking a change to PortfolioResource.fills propagates to the forwarder. If PortfolioResource.fills gains additional logic (auth scoping, feature flags, response post-processing), the deprecated forwarder silently diverges.

If the KalshiClient is accessible from within OrdersResource, true delegation (return self._client.portfolio.fills(...)) is preferable and removes the duplication risk. If not, this is fine as-is — just worth a note in the forwarder's docstring: """Deprecated — delegates directly to /portfolio/fills; see PortfolioResource.fills."""

2. PEP 8: blank line removed before class PortfolioResource

The diff removes the blank line between _settlements_params and class PortfolioResource:

-
 class PortfolioResource(SyncResource):

PEP 8 requires two blank lines between a module-level function and the following class. This is a one-character fix but will fail ruff if the project enforces E302.

---

Observations (non-blocking)

3. AsyncPortfolioResource.fills_all is correctly def, not async def — consistent with every other *_all method on async resources in this file. Not a bug; confirmed as the established pattern.

4. @deprecated placement on fills_all in AsyncOrdersResource — the decorator is on the def fills_all(...) which returns AsyncIterator. typing_extensions.deprecated wraps it as a regular function call, and the warning fires at call time (when the iterator is constructed), not at iteration time. This is the right behavior.

5. Contract map is consistent — AsyncPortfolioResource.fills is intentionally absent from METHOD_ENDPOINT_MAP (only sync methods are registered there, consistent with all other async resources). ✓

6. No CHANGELOG entry — intentional per project convention (updated per release). ✓

---

Tests

Test coverage is thorough:

• Happy path, filter parity, and auth guard for both fills and fills_all on both sync and async PortfolioResource. ✓
• Exactly-one-warning assertions on the deprecated forwarder paths (TestIssue351OrdersFillsDeprecated, TestIssue351AsyncOrdersFillsDeprecated). ✓
• Two-page pagination test for fills_all. ✓

One gap: there's no test asserting that calling portfolio.fills_all() does not emit a DeprecationWarning (to guard against accidentally decorating the new canonical location). Low-priority, but could catch a copy-paste error if the @deprecated decorator ever migrates wrongly.

---

Summary

Approve with the two items above addressed (especially the blank-line regression if ruff is enforced in CI). The relocation is correct, the deprecation strategy is appropriate for a breaking v3.0.0 change, and the test suite is comprehensive.