fix: correct KIS fill parsing and OpenClaw Telegram mirroring#169
fix: correct KIS fill parsing and OpenClaw Telegram mirroring#169robin-watcha wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThese changes enhance the KIS WebSocket's execution parsing with separate handlers for overseas and domestic markets, simplify the OpenClawClient API by removing callback_url and request_analysis, adjust notification flows to guarantee Telegram forwarding on OpenClaw failures, and integrate a trade notifier into the WebSocket monitor with proper lifecycle management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as KIS Client
participant KIS as KIS WebSocket
participant Parser as Execution Parser
participant Result as Execution Result
Client->>KIS: Connect & Listen
KIS->>KIS: Receive execution message
KIS->>KIS: Check if execution event (fill_yn="2")
alt Is Execution Event
KIS->>Parser: Delegate to market-specific parser
alt Overseas Market
Parser->>Parser: Parse by OVERSEAS_FILL_FIELDS indices
Parser->>Parser: Map side via OVERSEAS_SIDE_MAP
Parser->>Parser: Validate symbol, side, price, qty, timestamps
else Domestic Market
Parser->>Parser: Parse by DOMESTIC_FILL_FIELDS indices
Parser->>Parser: Validate symbol, side, price, qty, timestamps
end
Parser-->>Result: Return execution dict
KIS-->>Client: Deliver execution
else Not Execution Event
KIS-->>Client: Skip message
end
sequenceDiagram
participant Notifier as Fill Event Source
participant OpenClaw as OpenClaw Service
participant Telegram as Telegram API
participant Fallback as Telegram Notifier
Notifier->>OpenClaw: Send fill notification
alt OpenClaw Succeeds
OpenClaw-->>Notifier: Return request_id
Notifier-->>Notifier: In finally: Log success
else OpenClaw Fails
OpenClaw-->>Notifier: Raise exception
Notifier-->>Fallback: Forward to Telegram notifier
Fallback->>Telegram: Send formatted message
Telegram-->>Fallback: Message sent
Fallback-->>Notifier: Return result
Notifier-->>Notifier: Return None (not delivered to OpenClaw)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
|
PR Review: KIS Fill Parsing and OpenClaw Telegram MirroringOverviewThis PR introduces critical fixes for KIS WebSocket execution parsing and improves the OpenClaw integration with Telegram fallback. The changes are well-structured and address real production issues. ✅ Strengths1. Market-Specific Parsing LogicThe separation of overseas vs domestic execution parsing is a clean architectural improvement:
2. Correct Field Index MappingThe fix for OVERSEAS_FILL_FIELDS = {
"side": 4, # Critical fix here
"symbol": 7,
"filled_qty": 8,
"filled_price": 9,
"filled_at": 10,
"fill_yn": 12,
}3. Proper Fill Event FilteringThe if tr_code in OVERSEAS_EXECUTION_TR_CODES:
return str(data.get("fill_yn", "")).strip() == "2"4. Resilient Telegram MirroringThe try:
# OpenClaw delivery attempts
...
finally:
await self._forward_to_telegram(message, alert_type)This is excellent for reliability. 5. Comprehensive Test Coverage
🔍 Issues & Suggestions1. Critical:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
app/services/kis_websocket.py (2)
864-896:order_idhardcoded at index 2 is not inOVERSEAS_FILL_FIELDS.All other field indices are looked up from
OVERSEAS_FILL_FIELDS, butorder_iduses a literalfields[2]on Line 880. For consistency and maintainability, consider adding"order_id": 2to the constant.Proposed change
OVERSEAS_FILL_FIELDS = { + "order_id": 2, "side": 4, "symbol": 7, "filled_qty": 8, "filled_price": 9, "filled_at": 10, "fill_yn": 12, }Then on Line 880:
- order_id = fields[2].strip() if len(fields) > 2 else "" + order_id = fields[OVERSEAS_FILL_FIELDS["order_id"]].strip() if len(fields) > OVERSEAS_FILL_FIELDS["order_id"] else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/kis_websocket.py` around lines 864 - 896, In _parse_overseas_execution, the order_id is currently read using a hardcoded fields[2]; add "order_id": 2 to the OVERSEAS_FILL_FIELDS constant and replace the literal index with fields[OVERSEAS_FILL_FIELDS["order_id"]].strip() (keeping the existing fallback to None when empty) so all field indices are consistently sourced from OVERSEAS_FILL_FIELDS.
864-922: Missing docstrings on_parse_overseas_executionand_parse_domestic_execution.Both methods contain non-trivial field extraction logic tied to KIS API payload formats. As per coding guidelines: "Document complex analysis logic and API integration details in docstrings using Google style format."
Example docstrings
def _parse_overseas_execution(self, fields: list[str]) -> dict[str, Any] | None: + """Parse overseas (US) execution payload fields into a structured dict. + + Uses index-based extraction per KIS H0GSCNI0 TR payload layout. + Returns None if the payload has insufficient fields or invalid price/qty. + + Args: + fields: Split payload tokens from the WebSocket message. + + Returns: + Parsed execution dict with symbol, side, price, qty, amount, + fill_yn, and timestamp; or None on validation failure. + """def _parse_domestic_execution(self, fields: list[str]) -> dict[str, Any] | None: + """Parse domestic (KR) execution payload fields into a structured dict. + + Uses index-based extraction per KIS H0STCNI0 TR payload layout. + Returns None if the payload has insufficient fields or invalid price/qty. + + Args: + fields: Split payload tokens from the WebSocket message. + + Returns: + Parsed execution dict with symbol, side, price, qty, amount, + and timestamp; or None on validation failure. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/kis_websocket.py` around lines 864 - 922, Add Google-style docstrings to both _parse_overseas_execution and _parse_domestic_execution describing their purpose (parsing KIS execution/fill payloads), parameters (fields: list[str]) and return type (dict[str, Any] | None), plus the validation rules (required indices, non-empty symbol, positive filled_price/filled_qty, fill_yn handling for overseas) and mapping behavior (use of OVERSEAS_FILL_FIELDS, DOMESTIC_FILL_FIELDS, OVERSEAS_SIDE_MAP, _SIDE_MAP). Mention the helper functions used (_to_float, _extract_timestamp) and which fields they apply to, include a short example input payload snippet and example return value, and note that None is returned on invalid/insufficient data; keep docstrings concise and in Google style.app/services/openclaw_client.py (1)
88-106: Thebreakon Line 106 is redundant with tenacity'sAsyncRetrying.After a successful pass through
with attempt:(no exception raised), tenacity'sasync forloop naturally terminates. The explicitbreakdoesn't cause harm but is unnecessary.Proposed cleanup
logger.info( "OpenClaw fill notification sent: request_id=%s symbol=%s account=%s", request_id, normalized_order.symbol, normalized_order.account, ) delivered_to_openclaw = True - break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/openclaw_client.py` around lines 88 - 106, The explicit break inside the AsyncRetrying loop is redundant; remove the break statement in the async for attempt in AsyncRetrying(...) block and let tenacity naturally exit the loop on success, but keep the delivered_to_openclaw = True assignment (and the logger.info call) so the success state is preserved; locate these in the same block that uses httpx.AsyncClient, res.raise_for_status(), and the with attempt: context to make the change.tests/test_websocket_monitor.py (1)
283-321: Missing pytest category marker on the new test.The existing tests in this class don't have explicit markers either, but the coding guidelines require
@pytest.mark.unit,@pytest.mark.integration, or@pytest.mark.slowon all tests. Consider adding@pytest.mark.unitto the class or to this test. As per coding guidelines: "Use pytest markers (@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow) to categorize tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_websocket_monitor.py` around lines 283 - 321, The new async test test_main_configures_and_shuts_down_trade_notifier is missing a pytest category marker; add `@pytest.mark.unit` (or place `@pytest.mark.unit` on the containing test class) directly above the test coroutine definition to satisfy the project's test categorization guideline and ensure pytest recognizes its category; keep the existing pytest.mark.asyncio decorator and ensure pytest is imported/available where the test is defined.websocket_monitor.py (1)
306-310: Shutdown always attempts notifier cleanup regardless of whether it was configured — verify this is intentional.The
finallyblock unconditionally callsget_trade_notifier().shutdown(), even when telegram settings are absent andconfigure()was never called. From theshutdown()implementation this is safe (it's a no-op when_http_client is None), but worth confirming the singleton factory never raises on first access here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@websocket_monitor.py` around lines 306 - 310, The shutdown path should not unconditionally invoke get_trade_notifier() because the factory may raise or the notifier may never have been configured; change the finally block to safely obtain and validate the notifier before calling shutdown: attempt to call get_trade_notifier() inside a guarded try/except, and only await trade_notifier.shutdown() if the call succeeded and the returned object indicates it was configured (e.g., getattr(trade_notifier, "_http_client", None) is not None or a new is_trade_notifier_configured() helper returns True); log a debug/warning if the factory call fails and skip shutdown instead of assuming a valid notifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/services/kis_websocket.py`:
- Around line 864-896: In _parse_overseas_execution, the order_id is currently
read using a hardcoded fields[2]; add "order_id": 2 to the OVERSEAS_FILL_FIELDS
constant and replace the literal index with
fields[OVERSEAS_FILL_FIELDS["order_id"]].strip() (keeping the existing fallback
to None when empty) so all field indices are consistently sourced from
OVERSEAS_FILL_FIELDS.
- Around line 864-922: Add Google-style docstrings to both
_parse_overseas_execution and _parse_domestic_execution describing their purpose
(parsing KIS execution/fill payloads), parameters (fields: list[str]) and return
type (dict[str, Any] | None), plus the validation rules (required indices,
non-empty symbol, positive filled_price/filled_qty, fill_yn handling for
overseas) and mapping behavior (use of OVERSEAS_FILL_FIELDS,
DOMESTIC_FILL_FIELDS, OVERSEAS_SIDE_MAP, _SIDE_MAP). Mention the helper
functions used (_to_float, _extract_timestamp) and which fields they apply to,
include a short example input payload snippet and example return value, and note
that None is returned on invalid/insufficient data; keep docstrings concise and
in Google style.
In `@app/services/openclaw_client.py`:
- Around line 88-106: The explicit break inside the AsyncRetrying loop is
redundant; remove the break statement in the async for attempt in
AsyncRetrying(...) block and let tenacity naturally exit the loop on success,
but keep the delivered_to_openclaw = True assignment (and the logger.info call)
so the success state is preserved; locate these in the same block that uses
httpx.AsyncClient, res.raise_for_status(), and the with attempt: context to make
the change.
In `@tests/test_websocket_monitor.py`:
- Around line 283-321: The new async test
test_main_configures_and_shuts_down_trade_notifier is missing a pytest category
marker; add `@pytest.mark.unit` (or place `@pytest.mark.unit` on the containing test
class) directly above the test coroutine definition to satisfy the project's
test categorization guideline and ensure pytest recognizes its category; keep
the existing pytest.mark.asyncio decorator and ensure pytest is
imported/available where the test is defined.
In `@websocket_monitor.py`:
- Around line 306-310: The shutdown path should not unconditionally invoke
get_trade_notifier() because the factory may raise or the notifier may never
have been configured; change the finally block to safely obtain and validate the
notifier before calling shutdown: attempt to call get_trade_notifier() inside a
guarded try/except, and only await trade_notifier.shutdown() if the call
succeeded and the returned object indicates it was configured (e.g.,
getattr(trade_notifier, "_http_client", None) is not None or a new
is_trade_notifier_configured() helper returns True); log a debug/warning if the
factory call fails and skip shutdown instead of assuming a valid notifier.
1 participant
Summary
fill_yn == "2"filled_amount = price * qtyrequest_analysispath and keep alert-style senders (fill/scan/watch)Verification
uv run pytest --no-cov tests/test_kis_websocket.py -q -k "overseas or domestic or fill_event"uv run pytest --no-cov tests/test_openclaw_client.py -quv run pytest --no-cov tests/test_websocket_monitor.py -quv run ruff check app/services/kis_websocket.py app/services/openclaw_client.py websocket_monitor.py tests/test_kis_websocket.py tests/test_openclaw_client.py tests/test_websocket_monitor.pyuv run pyright app/services/kis_websocket.py app/services/openclaw_client.py websocket_monitor.pySummary by CodeRabbit
New Features
Improvements