README formatting, add asset.chain, TPSL checks#78
README formatting, add asset.chain, TPSL checks#78lethaale wants to merge 1 commit intox10xchange:starknetfrom
Conversation
Apply several README cleanups and link updates (table formatting, updated testnet/API URLs, spacing and minor text fixes). Add a new `chain: str` field to AssetOperationModel to include chain info for asset operations. Remove prior NotImplementedError guards in order creation related to TPSL POSITION and MARKET price-type checks, relaxing these constraints in the order builder.
There was a problem hiding this comment.
Pull request overview
This pull request applies README formatting improvements, adds chain information to asset operations, and relaxes order creation constraints for TPSL features.
Changes:
- README formatting updates including table alignment, URL updates to new testnet domain (starknet.sepolia.extended.exchange), and spacing improvements
- Added required
chain: strfield to AssetOperationModel for tracking blockchain information in asset operations - Removed NotImplementedError guards blocking TPSL POSITION type and MARKET price type in order creation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Formatting improvements: updated testnet URLs, improved table alignment and spacing consistency throughout documentation |
| x10/perpetual/assets.py | Added required chain field to AssetOperationModel to capture blockchain information for asset operations |
| x10/perpetual/order_object.py | Removed NotImplementedError guards for TPSL POSITION type and MARKET price type, allowing these features in order creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| asset: int | ||
| time: int | ||
| account_id: int | ||
| chain: str |
There was a problem hiding this comment.
The new chain field is defined as required (not Optional), but the test fixtures in tests/fixtures/assets.py don't provide this field when instantiating AssetOperationModel. This will cause test failures. Either make the field optional with Optional[str] = None, or update all test fixtures and test expectations to include the chain field.
| chain: str | |
| chain: Optional[str] = None |
| @@ -145,14 +145,6 @@ def __create_order_object( | |||
| if exact_only: | |||
| raise NotImplementedError("`exact_only` option is not supported yet") | |||
|
|
|||
There was a problem hiding this comment.
The removal of NotImplementedError guards for TPSL POSITION type and MARKET price type is not accompanied by any tests, documentation, or examples demonstrating these features now work. Consider adding test coverage for these newly enabled features, or at minimum document in the PR description that backend support has been verified. Without tests, there's a risk these features may not actually be supported by the backend API, which could lead to runtime errors for users.
| # Guard against unsupported TP/SL POSITION type until backend support is confirmed. | |
| if tp_sl_type == OrderTpslType.POSITION: | |
| raise NotImplementedError("`OrderTpslType.POSITION` is not supported yet") | |
| # Guard against unsupported MARKET price type for TP/SL triggers until backend support is confirmed. | |
| for tpsl_param in (take_profit, stop_loss): | |
| if tpsl_param is not None and tpsl_param.price_type == OrderPriceType.MARKET: | |
| raise NotImplementedError("`OrderPriceType.MARKET` for TP/SL is not supported yet") |
Apply several README cleanups and link updates (table formatting, updated testnet/API URLs, spacing and minor text fixes).
Add a new
chain: strfield to AssetOperationModel to include chain info for asset operations.Remove prior NotImplementedError guards in order creation related to TPSL POSITION and MARKET price-type checks, relaxing these constraints in the order builder.