Skip to content

Add missing order fields - #126

Open
alexex10 wants to merge 6 commits into
starknetfrom
task/add-missing-order-fields
Open

Add missing order fields#126
alexex10 wants to merge 6 commits into
starknetfrom
task/add-missing-order-fields

Conversation

@alexex10

@alexex10 alexex10 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Add missing fields for RFQ and CHASE orders

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to add missing order-related fields (notably rfq_start_price) across the SDK’s order models and order-creation/signing flow, and updates the MCP “place order” tool to derive a market price from top-of-book when a MARKET order omits price.

Changes:

  • Add rfq_start_price parameters to REST/blocking client order placement and introduce RFQ-specific validation in the signing/order object builder.
  • Extend order models to include rfq_start_price and add chase_order to OpenOrderModel.
  • Update MCP order placement to compute a “best market price” from the orderbook and bump SDK version to 2.6.0.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
x10/tools/mcp/place_order_tool.py Adjusts MARKET price derivation from top-of-book and adds RFQ gating/validation for MCP order placement.
x10/signing/order_object.py Adds rfq_start_price argument and validation to order-object creation/signing flow.
x10/models/order.py Adds rfq_start_price to order models and introduces chase_order on open orders.
x10/clients/rest/rest_api_client.py Threads rfq_start_price through the async REST order placement API.
x10/clients/blocking/blocking_trading_client.py Threads rfq_start_price through the blocking trading client order placement API.
pyproject.toml Bumps package version from 2.5.0 to 2.6.0.
Suppressed comments (2)

x10/tools/mcp/place_order_tool.py:188

  • This check rejects TPSL orders because TPSL sets price = Decimal(0) and Decimal(0) is falsy, so not order_price raises even though price=0 is required for TPSL orders.
            if not order_price:
                raise ValidationError("`order_price` is required")

x10/signing/order_object.py:200

  • rfq_start_price validation uses truthiness checks, so a provided value like Decimal(0) won’t be validated (it will be treated as “not provided”). Use is not None to detect whether the caller supplied the parameter.
        if rfq_start_price and not market.is_rfq:
            raise ValidationError("`rfq_start_price` must not be provided for non-RFQ markets")

        if rfq_start_price and order_type != OrderType.MARKET:
            raise ValidationError("`rfq_start_price` must not be provided for non-MARKET orders")

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread x10/tools/mcp/place_order_tool.py
Comment thread x10/signing/order_object.py Outdated
Comment thread x10/signing/order_object.py

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

x10/signing/order_object.py:59

  • rfq_start_price is a new request field with new validation rules (RFQ markets + MARKET orders only), but there are already comprehensive create_order_object tests under tests/signing/order_object/ and none cover this new behavior. Add tests for: (1) rejects rfq_start_price on non-RFQ markets, (2) rejects it for non-MARKET orders, (3) includes rfqStartPrice in to_api_request_json() when set for a valid RFQ market.
def create_order_object(
    *,
    account: StarkPerpetualAccount,
    market: MarketModel,
    amount_of_synthetic: Decimal,
    price: Decimal,
    rfq_start_price: Optional[Decimal] = None,
    side: OrderSide,
    starknet_domain: StarknetDomain,
    order_type: OrderType = OrderType.LIMIT,

x10/signing/order_object.py:198

  • rfq_start_price validation uses truthiness checks, so a valid Decimal(0) (or other falsy values) will bypass validation and could be sent on non-RFQ / non-MARKET orders. Use explicit is not None checks so the constraints are enforced consistently.
    def validate_rfq_start_price():
        if rfq_start_price and not market.is_rfq:
            raise ValidationError("`rfq_start_price` must not be provided for non-RFQ markets")

        if rfq_start_price and order_type != OrderType.MARKET:
            raise ValidationError("`rfq_start_price` must not be provided for non-MARKET orders")

x10/tools/mcp/place_order_tool.py:188

  • place_order now only checks order_price is None, which allows price=Decimal(0) for LIMIT orders (previously rejected by not price). A zero/negative limit price is invalid for normal markets and will create nonsensical orders. Add a strict validation for LIMIT orders (and keep TPSL’s price=0 special-case).
            order_price = (
                await _get_best_market_price(client=client, market=market, side=side)
                if order_type == OrderType.MARKET and price is None
                else price
            )

x10/models/order.py:168

  • Adding rfq_start_price to NewOrderModel changes the output of to_api_request_json() when called with the default exclude_none=False (it will now include rfqStartPrice: None). Existing signing tests that assert the full dict (e.g., tests/signing/order_object/test_*) will need their expected payloads updated accordingly (or updated to dump with exclude_none=True).
    qty: Decimal
    price: Decimal
    rfq_start_price: Optional[Decimal] = None
    reduce_only: bool = False

Comment thread x10/models/order.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Suppressed comments (3)

x10/signing/order_object.py:198

  • rfq_start_price validation uses truthiness checks (if rfq_start_price ...), so Decimal(0) (or other falsy numeric values) bypass the constraints and can be sent for non-RFQ markets / non-MARKET orders. Use explicit is not None checks so validation is based on presence rather than truthiness.
        if rfq_start_price and not market.is_rfq:
            raise ValidationError("`rfq_start_price` must not be provided for non-RFQ markets")

        if rfq_start_price and order_type != OrderType.MARKET:
            raise ValidationError("`rfq_start_price` must not be provided for non-MARKET orders")

x10/models/order.py:168

  • Adding rfq_start_price to NewOrderModel changes serialization output (e.g., to_api_request_json() / model_dump) and will break existing snapshot-style tests and any callers comparing the serialized dict unless they’re updated (or use exclude_none=True). Please update the affected tests/fixtures accordingly.
    qty: Decimal
    price: Decimal
    rfq_start_price: Optional[Decimal] = None
    reduce_only: bool = False

x10/tools/mcp/place_order_tool.py:180

  • This rejects RFQ markets, but the function still has an RFQ-specific placement path later (place_rfq_order(...) if market.is_rfq else ...). With the early raise, the RFQ branch becomes unreachable and the tool can never place RFQ orders. Either remove this guard (and optionally add RFQ-specific params like rfq_start_price), or remove the RFQ placement branch entirely if MCP truly shouldn’t support RFQ.
            if market.is_rfq:
                raise ValidationError("RFQ markets are not supported by MCP")

@alexex10
alexex10 marked this pull request as ready for review August 20, 2026 13:35
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.

2 participants