Skip to content

feat: add yfinance-backed paper trading mode - #2

Merged
riroan merged 7 commits into
riroan:mainfrom
jhojin7:main
Jul 31, 2026
Merged

riroan merged 7 commits into
riroan:mainfrom
jhojin7:main

Conversation

@jhojin7

@jhojin7 jhojin7 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds a yfinance-backed paper trading mode so the bot can run without KIS credentials.

Changes include:

  • Add broker selection via BROKER=kis|yfinance
  • Add trading mode selection via TRADING_MODE=paper|live
  • Add YFinanceBroker for paper trading only
  • Persist paper trading state under data/yfinance_paper_state.json
  • Update .env.example and README with yfinance paper mode setup
  • Add tests for broker factory/config/yfinance broker behavior

Motivation

Currently, running the project requires KIS credentials. This makes initial testing harder for users who want to explore the bot logic without setting up a live brokerage account.

With this change, users can run the bot in paper mode using yfinance market data first, then switch to KIS later when ready.

Notes

  • yfinance broker supports paper trading only.
  • Live trading still requires KIS.
  • yfinance data can be delayed, incomplete, or unavailable, so it should not be used for live trading decisions.
  • Paper trading state is stored locally and ignored by git.

Draft status

Opening as draft first for review. I would like feedback on the broker abstraction, config naming, and whether the scheduler behavior should stay as-is or be made configurable.

@MartinDog

Copy link
Copy Markdown

LGTM

@jhojin7
jhojin7 marked this pull request as ready for review July 9, 2026 03:58
@jhojin7

jhojin7 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@riroan 컨플릭트 해결되었습니다 확인부탁드립니다~

@riroan
riroan marked this pull request as draft July 9, 2026 05:27
@riroan
riroan marked this pull request as ready for review July 9, 2026 05:27
@riroan
riroan self-requested a review as a code owner July 9, 2026 05:27
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

🤖 AI PR Review

🔴 High

  1. YFinanceBroker.submit_order()가 주문 동시성에 취약합니다.

    • 현재 _cash, _positions, _orders, _fills를 전역 mutable 상태로 두고 있는데, 락 없이 수정합니다.
    • 동일 브로커 인스턴스에서 동시에 매수/매도 주문이 들어오면 잔고 중복 차감, 포지션 수량 꼬임, state 파일 덮어쓰기 경쟁이 발생할 수 있습니다.
    • 특히 connect() 이후 paper 모드에서 주문이 즉시 체결되므로, 이벤트 루프 상 병렬 주문 시 데이터 일관성 문제가 큽니다.
  2. YFinanceBroker의 상태 파일 저장이 원자적이지만 로드 시 예외 처리가 없습니다.

    • _load_state()에서 JSON 파싱 실패, 파일 손상, 부분 기록, 권한 오류가 발생하면 브로커 초기화 자체가 실패합니다.
    • 재시작 후 state 파일이 깨졌을 때 봇 전체가 올라오지 못할 수 있어, 예외 처리 및 복구 전략이 필요합니다.

🟡 Medium

  1. Config.get_trading_mode()가 TRADING_MODE 값 검증을 하지 않습니다.

    • "paper"/"live" 외의 값이 들어와도 그대로 반환되며, 이후 create_broker()에서 BROKER=yfinance 조합만 제한됩니다.
    • BROKER=kis인 경우 잘못된 TRADING_MODE가 들어와도 paper_trading 판정이 기대와 다르게 동작할 수 있습니다.
    • 잘못된 설정은 조기 실패시키는 편이 안전합니다.
  2. YFinanceBroker.get_current_price() / _history()는 외부 데이터 소스 실패에 대한 재시도나 타임아웃 제어가 없습니다.

    • yfinance는 네트워크 지연/실패/빈 응답이 흔할 수 있는데, 현재는 호출 시마다 직접 의존합니다.
    • 주문 처리 경로에서 실패하면 fill_price = order.price or 0으로 떨어져 0원 체결 같은 비정상 동작 가능성이 있습니다.
    • 최소한 주문 체결 경로에서는 0 체결을 막아야 합니다.
  3. submit_order()의 실패 시 fallback 가격이 0이면 잘못된 체결이 발생할 수 있습니다.

    • get_current_price() 실패 후 order.price가 없으면 Decimal("0")으로 체결합니다.
    • 시장가 주문인데 가격을 못 구했다고 0원 체결되는 것은 치명적인 버그입니다.
    • 데이터 조회 실패 시 주문을 rejected 처리하는 것이 맞습니다.
  4. YFinanceBroker.cancel_order()는 취소만 하고 이미 체결된 주문/부분체결 개념과 맞지 않습니다.

    • 현재 구현상 주문이 즉시 FILLED되므로 cancel 가능성이 거의 없지만, 상태 파일에 저장된 order를 취소 가능하게 바꾸면 실제 자산/현금 롤백이 없습니다.
    • 향후 확장 시 정합성 문제가 생기기 쉽습니다.
  5. OrderManager._calculate_buy_quantity()의 현금 계산 변경이 종목/마켓 혼합 포트폴리오를 충분히 반영하지 못할 수 있습니다.

    • KRW 종목과 USD 종목을 동시에 운용하는 경우, 시장별 cash만 보게 되면서 실제 전체 가용 자금 기준과 괴리가 생길 수 있습니다.
    • 기존 로직을 의도적으로 바꾼 것이라면 문서화/테스트가 더 필요합니다.
  6. yfinance paper state 파일에 대한 테스트는 있으나 손상/미존재/권한 오류 케이스 테스트가 없습니다.

    • 현재는 정상 케이스만 확인합니다.
    • 상태 복구 안정성 측면에서 예외 케이스 테스트가 부족합니다.

💡 Suggestion

  1. YFinanceBroker에 주문/상태 변경용 asyncio.Lock를 두는 것을 권장합니다.

    • _save_state(), _load_state() 제외하고도 submit_order(), reset_state(), cancel_order()는 하나의 임계구역으로 묶는 것이 안전합니다.
  2. _load_state() 실패 시 안전하게 초기 상태로 복구하고 로그를 남기세요.

    • JSONDecodeError, OSError를 잡아 “state 파일 손상” 경고 후 초기화하는 방식이 운영에 더 적합합니다.
  3. 주문 체결 가격 확보 실패 시 0원 체결 대신 rejected 처리하세요.

    • 특히 MARKET 주문에서는 데이터 부재를 명시적 실패로 다루는 편이 안전합니다.
  4. TRADING_MODE 값 검증 테스트를 추가하세요.

    • 잘못된 환경변수 입력이 있을 때 조기 실패하는지 확인하는 테스트가 있으면 설정 오류를 줄일 수 있습니다.
  5. YFinanceBroker의 파일 저장/복구 경로 테스트를 추가하세요.

    • 손상된 JSON, 읽기/쓰기 권한 오류, state 파일 미존재 케이스가 있으면 운영 안정성을 더 높일 수 있습니다.

Comment thread src/utils/config.py Outdated
Comment thread .env.example
@jhojin7

jhojin7 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

AI 리뷰 확인했습니다. 지적된 부분 중 아래 항목은 이번 PR에서 반영하겠습니다.

  • YFinanceBroker.submit_order() 상태 변경 구간에 asyncio.Lock 추가
  • state 파일 _load_state()에서 JSON 손상/읽기 오류 발생 시 초기 상태로 안전 복구 + warning 로그
  • 시장가 주문 가격 조회 실패 시 0 체결하지 않고 주문을 REJECTED 처리
  • TRADING_MODE 값을 paper|live로 검증하고 잘못된 값은 조기 실패
  • 위 케이스들에 대한 테스트 추가

cancel_order()는 현재 yfinance paper broker가 즉시 체결 모델이라 실제로 취소 가능한 구간이 거의 없습니다. 그래서 이번에는 FILLED 주문 취소 불가를 명확히 유지하고, 향후 부분체결/지연체결 모델을 넣을 때 롤백/정합성 처리를 별도 설계하는 방향으로 두겠습니다.

OrderManager._calculate_buy_quantity()의 시장별 cash 계산은 KRX/KRW와 US/USD 포트폴리오를 분리 운용하려는 의도였는데, 이 의도가 코드만으로는 충분히 드러나지 않아서 테스트/주석을 보강하겠습니다.

@riroan riroan left a comment •

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Squash 머지 해주세요

@jhojin7

jhojin7 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author
  • Fork에서 upstream으로 PR을 올린 상황에서 AI Review 워크플로가 실패했습니다.
  • Fork PR에는 보안상 secret이 노출되지 않아 OPENAI_API_KEY가 빈 값으로 전달된 것이 원인으로 보입니다.
  • Upstream에서 PR 번호로 AI Review 워크플로를 수동 실행하거나, 해당 체크를 제외하고 squash merge해 주시면 됩니다.

@riroan
riroan merged commit 1ab5e3c into riroan:main Jul 31, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants