Conversation
클라이언트 측 사전 검증 실패 시 사용할 ValidationError를 UpbeatError 직속 서브클래스로 추가한다. API 응답 없이 발생하므로 APIStatusError 계열과 분리하며, market/price/min_total 속성을 포함한다. Closes #43 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Upbeat(validate_min_order=True) 설정 시 orders.create()에서 매수 주문(side="bid")의 금액이 market.bid.min_total 미만이면 API 호출 없이 ValidationError를 발생시킨다. get_chance()로 최소금액을 조회 하며, 매도 주문과 기본 동작은 변경 없음. Closes #43 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
interruping
left a comment
There was a problem hiding this comment.
Review Summary
전체적으로 잘 설계된 PR입니다. opt-in 방식(validate_min_order=False 기본값)으로 기존 동작에 영향 없이 기능을 추가한 점, ValidationError를 UpbeatError 직속 서브클래스로 분리해 API 응답 기반 에러와 명확히 구분한 점이 좋습니다. 테스트 커버리지도 8개 시나리오로 충분합니다.
Highlights
_compute_bid_total을 순수 함수로 분리 — 테스트/가독성 우수with_options()에서 플래그 올바르게 전파- 기본값
False로 breaking change 없음 - sync/async 양쪽 동일하게 적용
Concerns
ValidationError.price속성명이 실제 저장하는 값(computed total)과 불일치 (라인 코멘트 참고)get_chance()호출이 검증할 때마다 발생 — 캐싱 고려 필요 (라인 코멘트 참고)cancel_and_new()에 검증 미적용 (아래 참고)
cancel_and_new() 검증 누락
cancel_and_new()도 new_price/new_volume/new_ord_type를 받으므로, validate_min_order=True 사용자는 교체 주문에도 검증이 적용될 것으로 기대할 수 있습니다. 의도적 제외라면 docstring에 한 줄 남겨두면 좋겠고, 아니라면 _check_min_order 추가를 고려해주세요. (단, side/market 파라미터가 없어 추가 설계 필요)
🤖 Reviewed by Claude Code (claude-opus-4-6)
src/upbeat/_errors.py
Outdated
| """Raised when client-side validation catches an invalid order before sending.""" | ||
|
|
||
| market: str | ||
| price: str |
There was a problem hiding this comment.
price 속성명이 혼동을 줄 수 있습니다
_check_min_order에서 이 필드에 str(total)을 전달합니다. 시장가 주문에서는 price == total이라 문제없지만, **지정가 주문에서는 total = price * volume**이므로 실제 단가가 아닌 계산된 총액이 들어갑니다.
total(또는 order_total)로 이름을 바꾸면 의미가 명확해질 것 같습니다.
class ValidationError(UpbeatError):
market: str
total: str # 계산된 주문 총액
min_total: str🤖 Claude Code
There was a problem hiding this comment.
동의합니다. price → total로 rename했습니다. e07bb53에서 ValidationError의 필드명과 관련 호출부를 모두 변경했습니다.
There was a problem hiding this comment.
e07bb53 확인했습니다. price → total rename이 _errors.py, orders.py (sync/async 양쪽), test_orders.py assertion까지 빠짐없이 반영되었습니다. 깔끔합니다. ✅
🤖 Claude Code
| total = _compute_bid_total(price, volume, ord_type) | ||
| if total is None: | ||
| return | ||
| chance = self.get_chance(market=market) |
There was a problem hiding this comment.
매 주문마다 get_chance() API 호출이 발생합니다
validate_min_order=True일 때 create() 호출마다 /v1/orders/chance 요청이 추가로 나갑니다. 동일 마켓에 반복 주문하는 시나리오(예: DCA 전략)에서는 레이트 리밋 소진이 우려됩니다.
제안: 짧은 TTL의 per-market 캐시를 두면 불필요한 호출을 줄일 수 있습니다.
# 예시: 간단한 TTL 캐시
def _get_min_total(self, market: str) -> Decimal | None:
now = time.monotonic()
cached = self._min_total_cache.get(market)
if cached and now - cached[1] < 60:
return cached[0]
chance = self.get_chance(market=market)
...당장은 opt-in이라 급하지 않지만, strategies/ 쪽에서 활용할 때 병목이 될 수 있습니다.
🤖 Claude Code
There was a problem hiding this comment.
이슈 #43 범위 외로 명시된 사항이라 이번 PR에서는 반영하지 않겠습니다. DCA 등 반복 주문 시나리오에서 병목이 될 수 있다는 점은 동의하므로, per-market TTL 캐시는 별도 이슈로 추적하면 좋겠습니다.
There was a problem hiding this comment.
네, 이해합니다. 범위를 명확히 구분하는 게 맞습니다. 캐싱은 별도 이슈로 추적하면 충분할 것 같습니다. 👍
🤖 Claude Code
src/upbeat/api/orders.py
Outdated
| raise ValidationError( | ||
| f"Order total {total} is below minimum {min_total} for {market}", | ||
| market=market, | ||
| price=str(total), |
There was a problem hiding this comment.
price=str(total) — 위 _errors.py 코멘트와 연결됩니다. 여기서 계산된 total 값을 price라는 이름의 인자로 전달하고 있어 혼동의 원인이 됩니다. 필드명을 total로 바꾸면 이 라인도 total=str(total)이 되어 자연스러워집니다.
🤖 Claude Code
There was a problem hiding this comment.
e07bb53에서 total=str(total)로 변경했습니다.
There was a problem hiding this comment.
확인했습니다. total=str(total)로 자연스러워졌습니다. ✅
🤖 Claude Code
| if price is None: | ||
| return None | ||
| if ord_type == "limit": | ||
| return Decimal(price) * Decimal(volume) if volume is not None else None |
There was a problem hiding this comment.
👍 volume is not None 가드가 잘 되어있어 Decimal(None) 호출이 방지됩니다. price에 잘못된 문자열이 올 경우 Decimal의 InvalidOperation이 발생하는데, 이는 호출자 입력 오류이므로 현재 동작(예외 전파)이 합리적입니다.
🤖 Claude Code
There was a problem hiding this comment.
감사합니다. 말씀대로 잘못된 문자열 입력 시 InvalidOperation 예외 전파가 호출자 책임이라 별도 처리 없이 두었습니다.
There was a problem hiding this comment.
동의합니다. 👍
🤖 Claude Code
지정가 주문에서 price * volume 계산 결과가 들어가므로 price라는 이름이 혼동을 준다는 리뷰 반영. 필드명을 total로 변경하여 계산된 주문 총액임을 명확히 한다. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
ValidationError클래스를UpbeatError직속 서브클래스로 추가 (API 응답 없이 클라이언트 측에서 발생)Upbeat(validate_min_order=True)설정 시orders.create()에서 매수 주문(side="bid")의 금액이market.bid.min_total미만이면 API 호출 없이ValidationError발생create_test()에도 동일 적용,with_options()에서 플래그 전파Test plan
get_chance호출 안 함 확인ValidationError발생 확인price * volume기준 검증 확인side="ask") 검증 건너뜀 확인price=None시 검증 건너뜀 확인create_test()검증 동작 확인Closes #43
🤖 Generated with Claude Code