Add API specification and behavioral test suite#8
Conversation
There was a problem hiding this comment.
Pull request overview
Adds formal API specification documents and a new test suite (unit spec-compliance + optional live testnet integration) to validate the SDK’s REST/WebSocket contracts, alongside a small MarketTrade model update to match observed server payloads.
Changes:
- Added REST/WebSocket/API-wide specification documents under
docs/. - Added REST + WebSocket spec-compliance behavioral tests (mocked/unit) and optional live testnet integration tests.
- Updated
MarketTrademodel fields to better align with REST/WS payload differences.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/api_resources/conftest.py | Adds shared fixtures + a mock httpx transport for request/response assertions. |
| tests/api_resources/test_rest_spec_compliance.py | Unit tests for REST reader request shapes, auth header behavior, error parsing, and Pydantic validation negative cases. |
| tests/api_resources/test_websocket_spec_compliance.py | Unit tests for WS subscribe/unsubscribe message format, topic construction, WS payload model parsing, and bigint parsing. |
| tests/api_resources/test_testnet_integration.py | Live testnet integration tests (auto-skipped without DECIBEL_API_KEY) for REST endpoints + WS subscription smoke tests. |
| src/decibel/read/_market_trades.py | Extends MarketTrade fields and relaxes some fields to be optional to accommodate REST/WS differences. |
| docs/SPEC.md | Adds overall SDK/API contract specification and cross-links to REST/WS specs. |
| docs/SPEC-REST.md | Adds detailed REST endpoint + schema specification derived from OpenAPI. |
| docs/SPEC-WEBSOCKET.md | Adds detailed WebSocket protocol/topic/schema specification derived from AsyncAPI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "is_buy": False, | ||
| "is_reduce_only": False, | ||
| "is_tpsl": False, | ||
| "details": "", | ||
| "tp_order_id": None, | ||
| "tp_trigger_price": None, |
There was a problem hiding this comment.
The WebSocket spec includes subscribe/unsubscribe response messages like {"success": true, "message": "Subscribed ..."} that do not contain a "topic" field (see docs/SPEC-WEBSOCKET.md §2.2). This test currently asserts those messages should raise, which would encode spec-noncompliant behavior and encourages treating normal server responses as errors. Update the expectation to accept/ignore these non-topic response messages (and adjust DecibelWsSubscription._parse_message accordingly).
| @pytest.fixture(scope="module") | ||
| def first_market_addr(read: DecibelReadDex) -> str: | ||
| """Get a valid market address from testnet for parameterized tests.""" | ||
| markets = asyncio.get_event_loop().run_until_complete(read.markets.get_all()) | ||
| assert len(markets) > 0, "Testnet should have at least one market" | ||
| return markets[0].market_addr |
There was a problem hiding this comment.
This fixture calls asyncio.get_event_loop().run_until_complete(), which is fragile under pytest-asyncio (asyncio_mode=auto) and can break when an event loop is already running (or create loop mismatch warnings). Prefer making this an async fixture and awaiting read.markets.get_all() directly.
| now_ms = int(time.time() * 1000) | ||
| candles = await read.candlesticks.get_by_name( | ||
| # Use market name from first market | ||
| market_name=(await read.markets.get_all())[0].market_name, | ||
| interval=CandlestickInterval.ONE_DAY, | ||
| start_time=now_ms - 86400000 * 30, # 30 days ago | ||
| end_time=now_ms, | ||
| ) | ||
| # May be empty if no recent trades, but should not error | ||
| assert isinstance(candles, list) | ||
|
|
||
| async def test_candlestick_fields(self, read: DecibelReadDex) -> None: | ||
| """Each candlestick SHALL have OHLCV fields per spec.""" | ||
| from decibel.read._candlesticks import CandlestickInterval |
There was a problem hiding this comment.
first_market_addr is injected into this test but never used; the test fetches markets again and uses markets[0] instead. This adds an extra live-network call and makes failures harder to diagnose. Either remove the unused fixture parameter or use it to supply the market address/name for the request.
|
|
||
| class TestAuthentication: | ||
| """SPEC.md Section 4.1: REST API Authentication headers.""" | ||
|
|
||
| async def test_bearer_token_included_when_key_set(self) -> None: | ||
| """SHALL include Authorization: Bearer <KEY> header.""" | ||
| transport = MockTransport() | ||
| transport.set_response([]) | ||
|
|
||
| async with httpx.AsyncClient(transport=transport) as client: | ||
| from decibel.read._market_prices import _MarketPriceList | ||
|
|
||
| await get_request( | ||
| model=_MarketPriceList, | ||
| url="https://test/api/v1/prices", | ||
| api_key="my-api-key-xyz", | ||
| client=client, | ||
| ) | ||
|
|
||
| req = transport.captured_requests[0] | ||
| assert req.headers["authorization"] == "Bearer my-api-key-xyz" | ||
|
|
||
| async def test_no_auth_header_when_key_is_none(self) -> None: | ||
| """SHALL NOT include Authorization header when api_key is None.""" | ||
| transport = MockTransport() | ||
| transport.set_response([]) | ||
|
|
||
| async with httpx.AsyncClient(transport=transport) as client: | ||
| from decibel.read._market_prices import _MarketPriceList | ||
|
|
||
| await get_request( | ||
| model=_MarketPriceList, | ||
| url="https://test/api/v1/prices", | ||
| api_key=None, | ||
| client=client, | ||
| ) | ||
|
|
||
| req = transport.captured_requests[0] | ||
| assert "authorization" not in req.headers |
There was a problem hiding this comment.
PR description and docs/SPEC-REST.md state auth header behavior is validated and that Origin is a required header, but these tests only assert Authorization and never check Origin. Either (a) add Origin header support in get_request/_base_request_async and assert it here, or (b) update the spec docs to reflect what the SDK actually sends.
| ### 8.1 Language & Runtime | ||
| - Python 3.10+ (for union type syntax and modern typing) | ||
| - Async-first design using `asyncio` with synchronous wrappers | ||
| - `httpx` for HTTP client (async + sync support) | ||
| - `websockets` for WebSocket connections | ||
| - `pydantic` for data validation and serialization |
There was a problem hiding this comment.
The spec says Python 3.10+ here, but pyproject.toml sets requires-python = ">=3.11" (and ruff/pyright target 3.11). Please update this section to avoid publishing a minimum version that the package doesn't actually support.
| account: str | ||
| market: str | ||
| action: str | ||
| trade_id: str | int | None = None | ||
| size: float | ||
| price: float | ||
| is_profit: bool | ||
| realized_pnl_amount: float | ||
| is_funding_positive: bool | ||
| is_funding_positive: bool | None = None | ||
| realized_funding_amount: float | ||
| is_rebate: bool | ||
| fee_amount: float | ||
| order_id: str | None = None | ||
| client_order_id: str | None = None | ||
| source: str | None = None | ||
| transaction_unix_ms: int |
There was a problem hiding this comment.
Several fields that appear to be required in both REST and WS trade payloads (e.g., trade_id, order_id, client_order_id) are declared optional with default None. This weakens validation and conflicts with the new spec docs that treat these as required fields (docs/SPEC-REST.md §11.4; docs/SPEC-WEBSOCKET.md §3.4). Prefer keeping always-present fields required (no default), and if REST/WS differ, use separate Pydantic models (or a discriminated wrapper) so each surface remains strict without leaking None into the public API.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class UserTradesResponse(BaseModel): | ||
| items: list[UserTrade] | ||
| total_count: int | ||
| total_count: int = 0 |
There was a problem hiding this comment.
total_count is defined with a default (= 0), which makes the field optional during validation. For REST trade_history responses this can silently accept malformed/incomplete server payloads and return an incorrect total_count (0) to callers. Prefer keeping total_count required (no default) or making it explicitly optional (int | None) and handling the fallback in the reader layer when appropriate.
| total_count: int = 0 | |
| total_count: int |
|
|
||
| items: list[UserOrder] | ||
| total_count: int | ||
| total_count: int = 0 |
There was a problem hiding this comment.
total_count is currently defaulted to 0, which makes it optional during model validation. Since UserOrderHistoryReader.get_by_addr() is parsing a paginated REST response, this risks masking missing/invalid total_count values and returning incorrect metadata to SDK consumers. Consider keeping total_count required (no default) or making it optional and computing a fallback outside the Pydantic model only when the API truly omits it.
| total_count: int = 0 | |
| total_count: int |
| @pytest.fixture(scope="module") | ||
| def eth_market(read_client): | ||
| """Get ETH market info for order tests.""" | ||
| markets = asyncio.get_event_loop().run_until_complete(read_client.markets.get_all()) | ||
| eth = next((m for m in markets if "ETH" in m.market_name), None) | ||
| if eth is None: | ||
| pytest.skip("No ETH market found on testnet") | ||
| return eth |
There was a problem hiding this comment.
This fixture uses asyncio.get_event_loop().run_until_complete(...) to call async SDK methods. Under Python 3.12+ (and with pytest-asyncio managing loops) this can raise RuntimeError when no loop is set, or interfere with the test event loop. Prefer converting the fixture to async and await read_client.markets.get_all() (or otherwise avoid run_until_complete).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class UserTwapHistoryResponse(BaseModel): | ||
| items: list[UserActiveTwap] | ||
| total_count: int | ||
| total_count: int = 0 |
There was a problem hiding this comment.
total_count defaulting to 0 can silently change the meaning of responses when the server omits the field (e.g., items may be non-empty but total_count becomes 0). Consider making this field int | None = None (unknown) or keeping it required and handling the server omission at the request/parsing layer so pagination logic can’t misinterpret a missing count as an empty result set.
| total_count: int = 0 | |
| total_count: int | None = None |
|
|
||
| class UserTradesResponse(BaseModel): | ||
| items: list[UserTrade] | ||
| total_count: int | ||
| total_count: int = 0 | ||
|
|
There was a problem hiding this comment.
Same concern as other paginated responses: defaulting total_count to 0 conflates “missing/unknown total” with “zero total”. This can break pagination logic downstream. Prefer int | None = None (and document it) or enforce presence of total_count and fail fast when the API response is malformed.
|
|
||
| items: list[UserOrder] | ||
| total_count: int | ||
| total_count: int = 0 |
There was a problem hiding this comment.
Defaulting total_count to 0 risks misrepresenting responses when the field is omitted (unknown vs zero). If this is intended to tolerate missing server fields, consider using total_count: int | None = None and/or adding a validator that sets it to len(items) only when that’s the desired fallback.
| total_count: int = 0 | |
| total_count: int | None = None |
| class UserFundingHistoryResponse(BaseModel): | ||
| items: list[UserFunding] | ||
| total_count: int | ||
| total_count: int = 0 |
There was a problem hiding this comment.
total_count: int = 0 has ambiguous semantics for a paginated response: missing/unknown total gets treated as zero. Consider changing it to int | None = None (unknown) or ensuring the SDK always receives total_count so callers relying on it don’t silently get incorrect pagination info.
| total_count: int = 0 | |
| total_count: int | None = None |
| assert "prices" in data | ||
|
|
||
| def test_non_topic_message_raises(self) -> None: | ||
| """Messages without 'topic' field SHALL raise ValueError.""" | ||
| ws = DecibelWsSubscription(TESTNET_CONFIG, api_key="test") | ||
| raw = json.dumps({"success": True, "message": "Subscribed"}) | ||
| with pytest.raises(ValueError, match="missing topic field"): | ||
| ws._parse_message(raw) | ||
|
|
There was a problem hiding this comment.
This test asserts that any message without a topic must raise. However, SPEC-WEBSOCKET.md in this PR defines subscribe/unsubscribe responses as {success, message} without topic. To keep the spec, docs, and tests consistent (and to avoid treating normal ACK frames as errors), consider updating the expected behavior here to treat {success, message} frames as non-data messages that are ignored (e.g., return None) rather than raising.
| with patch.object(ws, "_open", new_callable=AsyncMock): | ||
| unsub = ws.subscribe("t", MagicMock, MagicMock()) | ||
|
|
||
| unsub() | ||
| # unsubscribe message sent via create_task | ||
| await asyncio.sleep(0) | ||
| mock_conn.send.assert_awaited() | ||
|
|
There was a problem hiding this comment.
This unsubscribe will schedule _delayed_close() since it removes the last topic, which can leave a pending task (0.5s sleep) at test teardown. To avoid leaked tasks, consider cancelling/awaiting ws._close_timer_task (or patching _delayed_close/asyncio.sleep) before the test exits.
| **Required Headers:** | ||
|
|
||
| | Header | Value | Required | | ||
| |--------|-------|----------| | ||
| | `Authorization` | `Bearer <API_KEY>` | YES | | ||
| | `Origin` | Application origin URL (e.g., `https://app.decibel.trade`) | YES | | ||
|
|
There was a problem hiding this comment.
The spec here marks Origin as a required REST header, but the SDK’s request helpers (decibel._utils._base_request_async/_base_request_sync) only add Authorization and (for PATCH/POST) Content-Type. Either update the spec to clarify Origin is browser/CORS-specific (not required for this SDK), or implement adding a configurable Origin header in the SDK so the docs and implementation match.
| **Required Headers:** | |
| | Header | Value | Required | | |
| |--------|-------|----------| | |
| | `Authorization` | `Bearer <API_KEY>` | YES | | |
| | `Origin` | Application origin URL (e.g., `https://app.decibel.trade`) | YES | | |
| **Headers:** | |
| | Header | Value | Required | | |
| |--------|-------|----------| | |
| | `Authorization` | `Bearer <API_KEY>` | YES | | |
| | `Origin` | Application origin URL (e.g., `https://app.decibel.trade`) | Browser/CORS-specific | | |
| The `Origin` header is not required for requests made by this Python SDK. It is a browser-managed header used in CORS scenarios and may be sent automatically by browser-based clients. |
| ### 2.2 Subscribe Response (Success) | ||
|
|
||
| ```json | ||
| { | ||
| "success": true, | ||
| "message": "Subscribed to user_open_orders:0x1234..." | ||
| } | ||
| ``` |
There was a problem hiding this comment.
The subscribe/unsubscribe response examples omit topic, but the rest of the spec and SDK parsing logic typically use topic to distinguish data frames. If ACK frames truly have no topic (as shown), the SDK/tests should treat {success, message} frames as ignorable control messages. Consider clarifying this section (e.g., explicitly stating ACK frames have no topic and must be ignored by clients) so it’s unambiguous for implementers.
| ### 4.2 User Positions | ||
|
|
||
| **Topic:** `user_positions:{userAddr}` | ||
|
|
||
| **Message Schema:** | ||
| ```json | ||
| { | ||
| "topic": "user_positions:0x1234...", | ||
| "positions": [ | ||
| { | ||
| "market": "0x...", | ||
| "user": "0x1234...", | ||
| "size": 2.5, | ||
| "user_leverage": 10, | ||
| "max_allowed_leverage": 20, | ||
| "entry_price": 49800.0, | ||
| "is_isolated": false, | ||
| "is_deleted": false, | ||
| "unrealized_funding": -25.5, | ||
| "event_uid": 123456789012345678901234567890123456, | ||
| "estimated_liquidation_price": 45000.0, | ||
| "transaction_version": 12345681, | ||
| "tp_order_id": "tp_001", | ||
| "tp_trigger_price": 52000, | ||
| "tp_limit_price": 51900, | ||
| "sl_order_id": "sl_001", | ||
| "sl_trigger_price": 48000, | ||
| "sl_limit_price": null, | ||
| "has_fixed_sized_tpsls": false | ||
| } | ||
| ] | ||
| } | ||
| ``` | ||
|
|
||
| **Payload Type:** `UserPositionsResponse` | ||
|
|
||
| | Field | Type | Required | | ||
| |-------|------|----------| | ||
| | `topic` | string | Yes | | ||
| | `positions` | PositionDto[] | Yes | | ||
|
|
||
| **WebSocket-specific PositionDto fields (not in REST):** | ||
| - `max_allowed_leverage` (uint32, required) | ||
| - `event_uid` (u128, required) | ||
|
|
||
| **WebSocket TP/SL price fields** are `int64` (chain units), unlike REST which uses `float64`. | ||
|
|
||
| --- | ||
|
|
||
| ### 4.3 User Open Orders | ||
|
|
||
| **Topic:** `user_open_orders:{userAddr}` | ||
|
|
There was a problem hiding this comment.
Topic names here (user_positions:{userAddr}, user_open_orders:{userAddr}, etc.) don’t match the current SDK reader topics (e.g., UserPositionsReader.subscribe_by_addr() uses account_positions:{sub_addr} in src/decibel/read/_user_positions.py). Either update this spec to reflect the actual server/SKD topic strings, or update the SDK (and compliance tests) so the documented contract and implementation agree.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### 4.2 User Positions | ||
|
|
||
| **Topic:** `user_positions:{userAddr}` | ||
|
|
||
| **Message Schema:** | ||
| ```json | ||
| { | ||
| "topic": "user_positions:0x1234...", | ||
| "positions": [ |
There was a problem hiding this comment.
The SDK’s UserPositionsReader.subscribe_by_addr() uses the topic account_positions:{sub_addr} (see src/decibel/read/_user_positions.py:83), but this spec section documents user_positions:{userAddr}. Please update the documented topic/message examples (or update the SDK) so the spec and implementation match.
| ### 4.3 User Open Orders | ||
|
|
||
| **Topic:** `user_open_orders:{userAddr}` | ||
|
|
||
| **Message Schema:** | ||
| ```json | ||
| { | ||
| "topic": "user_open_orders:0x1234...", | ||
| "orders": [ |
There was a problem hiding this comment.
The SDK’s UserOpenOrdersReader.subscribe_by_addr() uses the topic account_open_orders:{sub_addr} (see src/decibel/read/_user_open_orders.py:90), but this spec section documents user_open_orders:{userAddr}. Please align the spec with the implemented topic (or vice versa).
| | Market Candlestick | `market_candlestick:{marketAddr}:{interval}` | market address, interval | | ||
| | Account Overview | `account_overview:{userAddr}` | subaccount address | | ||
| | User Positions | `user_positions:{userAddr}` | subaccount address | | ||
| | User Open Orders | `user_open_orders:{userAddr}` | subaccount address | | ||
| | User Order History | `user_order_history:{userAddr}` | subaccount address | | ||
| | User Trade History | `user_trade_history:{userAddr}` | subaccount address | | ||
| | User Trades | `user_trades:{userAddr}` | subaccount address | | ||
| | User Funding History | `user_funding_rate_history:{userAddr}` | subaccount address | |
There was a problem hiding this comment.
In the Topic String Format table, the documented patterns for User Positions and User Open Orders (user_positions:*, user_open_orders:*) don’t match the SDK’s current subscription topics (account_positions:*, account_open_orders:*). This table should be updated to reflect the actual topic names used by the SDK (or the SDK updated to match the spec).
| class UserTwapHistoryResponse(BaseModel): | ||
| items: list[UserActiveTwap] | ||
| total_count: int | ||
| total_count: int | None = None |
There was a problem hiding this comment.
total_count was changed from required to optional here. This diverges from the spec docs in this PR that describe paginated responses as always including total_count. Either document the omission explicitly (so callers handle None) or keep the field required to catch API regressions.
| total_count: int | None = None | |
| total_count: int |
| def from_env(cls) -> BotConfig: | ||
| private_key = os.environ.get("PRIVATE_KEY", "") | ||
| api_key = os.environ.get("APTOS_NODE_API_KEY", "") | ||
| if not private_key or not api_key: | ||
| print("Error: PRIVATE_KEY and APTOS_NODE_API_KEY must be set") | ||
| print("See docstring at top of this file for setup instructions") | ||
| sys.exit(1) |
There was a problem hiding this comment.
This example reads the API key from APTOS_NODE_API_KEY and passes it as DecibelReadDex(..., api_key=...), but the rest of the repo (including the integration tests) refers to this as DECIBEL_API_KEY. If these are different credentials (Decibel trading API key vs Aptos node API key), the example should accept them separately to avoid confusing users and misconfiguring auth headers.
| if self.state.phase == BotPhase.WAITING_FOR_PRICE: | ||
| # Schedule the buy order placement (can't await in a sync callback) | ||
| asyncio.get_event_loop().call_soon( | ||
| lambda: asyncio.ensure_future(self._place_buy_order()) | ||
| ) |
There was a problem hiding this comment.
Using asyncio.get_event_loop() + asyncio.ensure_future() inside callbacks can be brittle and may emit deprecation warnings on newer Python versions. Since these callbacks should already be running on the event loop, prefer asyncio.get_running_loop().create_task(...) (or asyncio.create_task(...)) to schedule _place_buy_order() / _place_sell_order().
| for sig in (signal.SIGINT, signal.SIGTERM): | ||
| loop.add_signal_handler(sig, bot.stop) |
There was a problem hiding this comment.
loop.add_signal_handler(...) is not supported on some platforms (notably Windows) and will raise NotImplementedError. Since this is an example intended to be runnable, consider guarding this with a try/except (or platform check) so the script still works without signal handlers.
| for sig in (signal.SIGINT, signal.SIGTERM): | |
| loop.add_signal_handler(sig, bot.stop) | |
| try: | |
| for sig in (signal.SIGINT, signal.SIGTERM): | |
| loop.add_signal_handler(sig, bot.stop) | |
| except NotImplementedError: | |
| log.warning("Asyncio signal handlers are not supported on this platform; continuing without them") |
docs/SPEC-WEBSOCKET.md
Outdated
| ### 4.4 User Order History | ||
|
|
||
| **Topic:** `user_order_history:{userAddr}` | ||
|
|
||
| **Message Schema:** | ||
| ```json | ||
| { | ||
| "topic": "user_order_history:0x1234...", | ||
| "orders": [ ... ] | ||
| } | ||
| ``` | ||
|
|
||
| **Payload Type:** `UserOrderHistoryResponse` (same shape as UserOpenOrdersResponse) | ||
|
|
||
| --- | ||
|
|
||
| ### 4.5 User Trade History | ||
|
|
||
| **Topic:** `user_trade_history:{userAddr}` | ||
|
|
There was a problem hiding this comment.
The SDK does not subscribe to user_order_history:{addr} or user_trade_history:{addr} topics (order updates use order_updates:{addr} and trades use user_trades:{addr} in the readers). Either update these documented WebSocket topics to the ones the SDK actually uses, or implement the missing topics so the spec and behavior are consistent.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gas_station_url=None, | ||
| gas_station_api_key=None, | ||
| deployment=_DEPLOYMENT, | ||
| chain_id=1, |
There was a problem hiding this comment.
The testnet config fixture sets chain_id=1, but Network.TESTNET uses chain id 2 (see src/decibel/_constants.py:132-141). Using the wrong chain id in shared fixtures can hide bugs in any code paths that rely on config.chain_id.
Suggested fix: set chain_id=2 in this fixture (or reuse TESTNET_CONFIG as the base and override only the URLs you need).
| chain_id=1, | |
| chain_id=2, |
|
|
||
| 3. Run: | ||
|
|
||
| uv run python examples/buy_low_sell_high_bot.py |
There was a problem hiding this comment.
The docstring run command uses the wrong path. This file lives under examples/bots/, but the instructions say uv run python examples/buy_low_sell_high_bot.py, which will fail.
Suggested fix: update the command to uv run python examples/bots/buy_low_sell_high_bot.py.
| uv run python examples/buy_low_sell_high_bot.py | |
| uv run python examples/bots/buy_low_sell_high_bot.py |
| Paginated responses SHALL return: | ||
|
|
||
| ```json | ||
| { | ||
| "items": [ ... ], | ||
| "total_count": 42 | ||
| } | ||
| ``` |
There was a problem hiding this comment.
This section states that paginated responses SHALL always include total_count, but several SDK response models in this PR were relaxed to total_count: int | None = None (e.g. src/decibel/read/_user_order_history.py, _user_trade_history.py, _user_funding_history.py, _user_twap_history.py).
Suggested fix: either (a) keep total_count required in the SDK models and treat missing total_count as a server/spec violation, or (b) update the spec docs here (and in SPEC-REST) to document that total_count may be omitted/null for specific endpoints.
| ### 11.8 PaginatedResponse\<T\> | ||
|
|
||
| ```json | ||
| { | ||
| "items": [ T, ... ], | ||
| "total_count": 42 | ||
| } | ||
| ``` | ||
|
|
||
| All paginated responses SHALL include `items` (array) and `total_count` (int32). | ||
|
|
There was a problem hiding this comment.
PaginatedResponse<T> is documented here as always including a non-null total_count, but some SDK paginated response models were changed in this PR to allow missing total_count (set to int | None = None).
Suggested fix: align the spec with actual SDK/server behavior by either making total_count optional in this schema (and noting which endpoints are affected), or keep it required and update the SDK to fail validation when it’s missing.
docs/SPEC-WEBSOCKET.md
Outdated
| **Topic:** `user_positions:{userAddr}` | ||
|
|
||
| **Message Schema:** | ||
| ```json | ||
| { | ||
| "topic": "user_positions:0x1234...", |
There was a problem hiding this comment.
The WebSocket topic names documented here (e.g. user_positions:{userAddr} / user_open_orders:{userAddr}) don’t match the SDK’s actual subscription topics (e.g. account_positions:{sub_addr} and account_open_orders:{sub_addr} in src/decibel/read/_user_positions.py:83 and _user_open_orders.py:90).
Suggested fix: update this section’s topic strings to match the SDK/server (or, if the spec is authoritative, update the SDK topic construction + compliance tests accordingly).
| **Topic:** `user_positions:{userAddr}` | |
| **Message Schema:** | |
| ```json | |
| { | |
| "topic": "user_positions:0x1234...", | |
| **Topic:** `account_positions:{sub_addr}` | |
| **Message Schema:** | |
| ```json | |
| { | |
| "topic": "account_positions:0x1234...", |
| | Market Candlestick | `market_candlestick:{marketAddr}:{interval}` | market address, interval | | ||
| | Account Overview | `account_overview:{userAddr}` | subaccount address | | ||
| | User Positions | `user_positions:{userAddr}` | subaccount address | | ||
| | User Open Orders | `user_open_orders:{userAddr}` | subaccount address | | ||
| | User Order History | `user_order_history:{userAddr}` | subaccount address | | ||
| | User Trade History | `user_trade_history:{userAddr}` | subaccount address | | ||
| | User Trades | `user_trades:{userAddr}` | subaccount address | | ||
| | User Funding History | `user_funding_rate_history:{userAddr}` | subaccount address | | ||
| | Order Updates | `order_updates:{userAddr}` | subaccount address | |
There was a problem hiding this comment.
The topic string table lists user_positions, user_open_orders, and user_order_history, but the SDK currently uses account_positions, account_open_orders, and does not construct a user_order_history:{addr} topic (order updates use order_updates:{addr} instead).
Suggested fix: reconcile this table with the SDK’s actual topics (or update the SDK/readers + tests if the spec is intended to be authoritative).
| @@ -34,6 +60,8 @@ class MarketTrade(BaseModel): | |||
| realized_funding_amount: float | |||
| is_rebate: bool | |||
| fee_amount: float | |||
| order_id: str | |||
| client_order_id: str | |||
| transaction_unix_ms: int | |||
| transaction_version: int | |||
|
|
|||
| @@ -46,7 +74,7 @@ class MarketTradesResponse(BaseModel): | |||
| class MarketTradeWsMessage(BaseModel): | |||
| model_config = ConfigDict(populate_by_name=True) | |||
|
|
|||
| trades: list[MarketTrade] | |||
| trades: list[_WsTradeItem] | |||
|
|
|||
There was a problem hiding this comment.
MarketTradeWsMessage is part of the public API (decibel.read exports it), but its trades field is typed as list[_WsTradeItem] where _WsTradeItem is a private, non-exported class. This makes it awkward for SDK consumers to type-annotate WS trade callbacks without importing a private symbol.
Suggested fix: rename _WsTradeItem to a public model (e.g. MarketTradeWsItem) and export it (or type trades as list[MarketTrade] if you intentionally want one unified public type).
- Add docs/SPEC.md, SPEC-REST.md, SPEC-WEBSOCKET.md documenting all REST and WebSocket API contracts from the official OpenAPI/AsyncAPI specs - Add spec compliance tests (unit) verifying model validation, auth headers, error handling, reader topic construction, and negative cases - Add testnet integration tests verifying SDK against live API (auto-skip without DECIBEL_API_KEY env var) - Fix MarketTrade.is_funding_positive: make optional (REST API omits it, WebSocket-only field per spec) - Add missing MarketTrade fields: trade_id, order_id, client_order_id, source
- Add CLAUDE.md with pre-commit checklist and project conventions - Run ruff format on test files to pass CI format check
- Add test_testnet_write_integration.py covering: mint, subaccount, deposit/withdraw, order place/query/cancel, and authenticated read endpoints (positions, orders, trades, funds, delegations) - Fix UserTradesResponse.total_count: default to 0 (API omits it) - Fix UserOrders.total_count: default to 0 (API omits it) - Fix UserTrade.is_funding_positive: make optional (REST omits it)
124 new unit tests covering: - _pagination: construct_known_query_params (97%) - _order_status: status parsing and helper methods (95%) - _exceptions: TxnSubmitError/TxnConfirmError init (100%) - _utils: prettify_validation_error, sync request wrappers, address generators, FetchError edge cases (88%) - _gas_price_manager: auth headers, fetch/set gas price, async and sync variants (66%) - read/_ws: subscribe/unsubscribe, ready_state, reset, reconnect scheduling (59%) Total coverage: 53% -> 70% (with integration tests)
New integration tests for previously untested endpoints: - market_prices.get_by_name (single market) - portfolio_chart.get_by_addr - user_funding_history.get_by_addr - user_twap_history.get_by_addr - user_bulk_orders.get_by_addr - vaults.get_user_owned_vaults - vaults.get_user_performances_on_vaults SDK fixes found by live testing: - UserFundingHistoryResponse.total_count: default to 0 (API omits) - UserTwapHistoryResponse.total_count: default to 0 (API omits) All 28 read integration tests + 16 write integration tests pass against live testnet with zero JSON mismatches.
New integration tests: - markets.list_market_addresses() — on-chain view - markets.market_name_by_address() — on-chain view - markets.get_by_name() — documents known bug (wrong resource type) - vaults.get_vault_share_price() — on-chain view - trading_points.get_by_owner() — REST endpoint SDK fixes: - UserFundingHistoryResponse.total_count: default to 0 (API omits) - UserTwapHistoryResponse.total_count: default to 0 (API omits) Every SDK reader method is now tested against live testnet: 26 REST endpoints + 4 on-chain views + 2 WebSocket channels = 33 tests
1. Fix _parse_message to return None for subscribe/unsubscribe responses instead of raising ValueError (spec §2.2 compliance) 2. Remove fragile get_event_loop().run_until_complete() from fixtures, use asyncio.run() instead 3. Remove unused first_market_addr fixture and parameter 4. Update SPEC docs: remove Origin as required header (SDK doesn't send it), fix Python version 3.10+ -> 3.11+ 5. Split MarketTrade into strict REST model (trade_id, order_id, client_order_id, source all required) and separate _WsTradeItem (has is_funding_positive, no source) 6. Change total_count from default=0 to int|None on UserOrders, UserTradesResponse, UserFundingHistoryResponse, UserTwapHistoryResponse to avoid silently masking missing API data
End-to-end trading bot demonstrating the full SDK surface: - REST API: market info, account overview - WebSocket: real-time price feed, order update stream - Write SDK: place_order, cancel lifecycle - State machine: WAITING -> BUY_PLACED -> SELL_PLACED -> repeat Configurable via env vars: MARKET, BUY_SPREAD_PCT, SELL_SPREAD_PCT, ORDER_SIZE_USD, NETWORK. Includes setup instructions in docstring.
- Add asyncio.Lock to prevent race condition where multiple price ticks fire concurrent buy order placements - Transition phase inside the lock BEFORE the async order call - Round order size down to nearest lot_size multiple (fixes ESIZE_NOT_RESPECTING_LOT_SIZE on-chain error) - Remove debug print of private key
- Move buy_low_sell_high_bot.py into examples/bots/ - Add .env.example with all config vars documented (no real creds)
Cover the new ConnectTimeout/ReadTimeout/ConnectError retry paths added in main (PR #9). Tests verify: - Retries on each error type then succeeds - Times out with TxnConfirmError after retries exhausted
gregnazario
force-pushed
the
make-python-spec
branch
from
2db8553 to
489aae8
Compare
Spec docs: - Fix WS topic names to match SDK: user_positions -> account_positions, user_open_orders -> account_open_orders - Remove user_order_history/user_trade_history WS sections (SDK uses order_updates and user_trades topics instead) - Add note clarifying ACK frames have no topic field - Add note explaining AsyncAPI vs actual server topic divergence Bot example: - Replace asyncio.get_event_loop() with get_running_loop().create_task() - Guard add_signal_handler with try/except NotImplementedError for Windows - Accept DECIBEL_API_KEY in addition to APTOS_NODE_API_KEY - Update .env.example to use DECIBEL_API_KEY Tests: - Patch _delayed_close in WS unsubscribe tests to prevent leaked tasks
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| | Param | Type | Required | Description | | ||
| |-------|------|----------|-------------| | ||
| | `account` | string | Yes | Subaccount address | |
There was a problem hiding this comment.
This section documents account as the query parameter for /api/v1/open_orders, but the current SDK uses user (see src/decibel/read/_user_open_orders.py:69). Please align the spec with the SDK (or update the SDK to match the OpenAPI contract) so users and the spec compliance tests don’t diverge.
| | `account` | string | Yes | Subaccount address | | |
| | `user` | string | Yes | Subaccount address | |
| | Param | Type | Required | Description | | ||
| |-------|------|----------|-------------| | ||
| | `account` | string | Yes | Subaccount address | | ||
| | `limit` | int32 | Yes | Max results | | ||
| | `offset` | int32 | Yes | Pagination offset | |
There was a problem hiding this comment.
This section documents account as the query parameter for /api/v1/order_history, but the SDK currently sends user (see src/decibel/read/_user_order_history.py:80). Please reconcile the spec vs implementation and consider adding a compliance test to lock the expected param name.
| **WebSocket TP/SL price fields** are `int64` (chain units), unlike REST which uses `float64`. | ||
|
|
||
| --- | ||
|
|
||
| ### 4.3 User Open Orders | ||
|
|
There was a problem hiding this comment.
The spec says the open orders topic is user_open_orders:{userAddr}, but the SDK currently subscribes with account_open_orders:{sub_addr} (see src/decibel/read/_user_open_orders.py:90). Please either update the spec to match the implementation or fix the SDK topic string so it matches the AsyncAPI contract.
| # SPEC Section 9.3 — Topic strings constructed by actual readers | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestReaderTopicConstruction: | ||
| """Verify actual readers construct correct topic strings per spec Section 9.3.""" | ||
|
|
There was a problem hiding this comment.
The topic-construction suite doesn’t currently cover UserOpenOrdersReader.subscribe_by_addr(). Given the spec in this PR uses user_open_orders:{addr} while the SDK uses account_open_orders:{addr} (src/decibel/read/_user_open_orders.py:90), please add an explicit test here to catch topic drift and force alignment with the spec.
| # Required: API key from Geomi (https://geomi.dev) | ||
| # Either DECIBEL_API_KEY or APTOS_NODE_API_KEY is accepted | ||
| DECIBEL_API_KEY="aptoslabs_..." | ||
|
|
There was a problem hiding this comment.
The variable name APTOS_NODE_API_KEY suggests an Aptos fullnode API key, but the comment says it’s the Decibel/Geomi API key. Since the SDK uses different keys for trading REST/WS auth vs fullnode rate limits, consider splitting this into two env vars (e.g., DECIBEL_API_KEY for DecibelReadDex(api_key=...) and APTOS_NODE_API_KEY for BaseSDKOptions(node_api_key=...)) to avoid misconfiguration.
| # Required: API key from Geomi (https://geomi.dev) | |
| # Either DECIBEL_API_KEY or APTOS_NODE_API_KEY is accepted | |
| DECIBEL_API_KEY="aptoslabs_..." | |
| # Required: Decibel/Geomi trading API key (https://geomi.dev) | |
| DECIBEL_API_KEY="aptoslabs_..." | |
| # Optional: Aptos fullnode API key for node rate limits, if your node provider requires one | |
| APTOS_NODE_API_KEY="" |
| api_key: str | ||
| network: str = "testnet" | ||
| market: str = "ETH/USD" | ||
| buy_spread_pct: float = 1.0 # buy this % below oracle price | ||
| sell_spread_pct: float = 1.0 # sell this % above entry price | ||
| order_size_usd: float = 100.0 # notional order size in USD | ||
|
|
||
| @classmethod | ||
| def from_env(cls) -> BotConfig: | ||
| private_key = os.environ.get("PRIVATE_KEY", "") | ||
| # Accept either DECIBEL_API_KEY or APTOS_NODE_API_KEY | ||
| api_key = os.environ.get("DECIBEL_API_KEY") or os.environ.get("APTOS_NODE_API_KEY", "") | ||
| if not private_key or not api_key: | ||
| print("Error: PRIVATE_KEY and DECIBEL_API_KEY (or APTOS_NODE_API_KEY) must be set") | ||
| print("See docstring at top of this file for setup instructions") | ||
| sys.exit(1) | ||
|
|
||
| return cls( | ||
| private_key=private_key, | ||
| api_key=api_key, |
There was a problem hiding this comment.
BotConfig.from_env() reads APTOS_NODE_API_KEY into api_key, and later uses it both for DecibelReadDex(api_key=...) (trading REST/WS auth) and BaseSDKOptions(node_api_key=...) (Aptos fullnode header). These are not necessarily the same credential; consider using separate config fields / env vars to avoid accidentally sending the wrong key to either service.
| api_key: str | |
| network: str = "testnet" | |
| market: str = "ETH/USD" | |
| buy_spread_pct: float = 1.0 # buy this % below oracle price | |
| sell_spread_pct: float = 1.0 # sell this % above entry price | |
| order_size_usd: float = 100.0 # notional order size in USD | |
| @classmethod | |
| def from_env(cls) -> BotConfig: | |
| private_key = os.environ.get("PRIVATE_KEY", "") | |
| # Accept either DECIBEL_API_KEY or APTOS_NODE_API_KEY | |
| api_key = os.environ.get("DECIBEL_API_KEY") or os.environ.get("APTOS_NODE_API_KEY", "") | |
| if not private_key or not api_key: | |
| print("Error: PRIVATE_KEY and DECIBEL_API_KEY (or APTOS_NODE_API_KEY) must be set") | |
| print("See docstring at top of this file for setup instructions") | |
| sys.exit(1) | |
| return cls( | |
| private_key=private_key, | |
| api_key=api_key, | |
| decibel_api_key: str | |
| aptos_node_api_key: str = "" | |
| network: str = "testnet" | |
| market: str = "ETH/USD" | |
| buy_spread_pct: float = 1.0 # buy this % below oracle price | |
| sell_spread_pct: float = 1.0 # sell this % above entry price | |
| order_size_usd: float = 100.0 # notional order size in USD | |
| @property | |
| def api_key(self) -> str: | |
| """Backward-compatible alias for the Decibel trading API key.""" | |
| return self.decibel_api_key | |
| @classmethod | |
| def from_env(cls) -> BotConfig: | |
| private_key = os.environ.get("PRIVATE_KEY", "") | |
| decibel_api_key = os.environ.get("DECIBEL_API_KEY", "") | |
| aptos_node_api_key = os.environ.get("APTOS_NODE_API_KEY", "") | |
| if not private_key or not decibel_api_key: | |
| print("Error: PRIVATE_KEY and DECIBEL_API_KEY must be set") | |
| print("See docstring at top of this file for setup instructions") | |
| sys.exit(1) | |
| return cls( | |
| private_key=private_key, | |
| decibel_api_key=decibel_api_key, | |
| aptos_node_api_key=aptos_node_api_key, |
| Requires two env vars: | ||
| DECIBEL_API_KEY - API key for testnet | ||
| DECIBEL_PRIVATE_KEY - Private key of a funded testnet account |
There was a problem hiding this comment.
This integration test treats DECIBEL_API_KEY as both the trading API key (used for DecibelReadDex(api_key=...)) and the Aptos fullnode API key (passed as BaseSDKOptions(node_api_key=...)). If those are distinct credentials in practice, consider requiring a separate env var for the node API key (or leaving node_api_key unset) and update the module docstring accordingly to prevent false-negative setup failures.
| from decibel._utils import FetchError, get_request | ||
| from decibel.read._account_overview import AccountOverview | ||
| from decibel.read._candlesticks import Candlestick | ||
| from decibel.read._market_prices import MarketPrice, MarketPricesReader | ||
| from decibel.read._markets import PerpMarket | ||
| from decibel.read._user_open_orders import UserOpenOrder | ||
| from decibel.read._user_positions import UserPosition |
There was a problem hiding this comment.
This suite claims to verify that readers send correct query parameters, but it currently doesn’t include reader-level tests for /api/v1/open_orders and /api/v1/order_history (only the UserOpenOrder/UserPosition models are referenced). Adding transport-based tests for those readers would help catch spec/implementation drift (e.g., whether the query key is account vs user).
Summary
docs/SPEC.md,SPEC-REST.md,SPEC-WEBSOCKET.md) documenting the full REST and WebSocket API contracts derived from the official OpenAPI 3.1.0 and AsyncAPI 3.0.0 specs at https://docs.decibel.tradeMarketTrade.is_funding_positivemade optional (REST API omits it; it's WebSocket-only per spec Section 9.1)MarketTradefields (trade_id,order_id,client_order_id,source) that the REST API returnsTest plan
uv run pytest tests/ -k "not testnet"— 101 unit/spec tests pass (no network needed)DECIBEL_API_KEY=<key> uv run pytest tests/api_resources/test_testnet_integration.py -v— 21 integration tests pass against live testnetDECIBEL_API_KEYuv run ruff check— all lint checks pass