fix/add_depth_to_legacy_async_client_orderbooks_queries #396
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds changelog and bumps version to 1.11.1. Deprecates v1 AsyncClient via a DeprecationWarning in its constructor. Four v1 orderbook methods now accept an optional depth param and forward depth (default 0) to indexer calls. Adds unit tests covering AsyncClient and IndexerClient deprecation warnings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant AsyncV1 as AsyncClient (v1)
participant Indexer as IndexerClient
User->>AsyncV1: __init__(network)
Note over AsyncV1: emit DeprecationWarning ("Use AsyncClient from async_client_v2")
User->>AsyncV1: fetch_*_orderbook*_v2(..., depth=None|N)
AsyncV1->>AsyncV1: depth_to_use = depth or 0
AsyncV1->>Indexer: fetch_*_orderbook*_v2(..., depth=depth_to_use)
Indexer-->>AsyncV1: orderbook data
AsyncV1-->>User: orderbook data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes the v1 AsyncClient orderbooks queries to include the depth parameter and deprecates the v1 AsyncClient in favor of the v2 version.
Key changes:
- Added deprecation warning to v1 AsyncClient constructor directing users to v2
- Enhanced orderbook methods to accept optional depth parameter with default value
- Added comprehensive test coverage for deprecation warnings
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyinjective/async_client.py | Added deprecation warning to constructor and depth parameter to orderbook methods |
| tests/test_async_client_deprecation_warnings.py | Added test suite to verify deprecation warning functionality |
| pyproject.toml | Version bump to 1.11.1 |
| CHANGELOG.md | Documentation of changes for this release |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| self, | ||
| network: Network, | ||
| ): | ||
| warn( |
There was a problem hiding this comment.
Missing import statement for the warn function. The code uses warn() but there's no corresponding import statement visible. This will cause a NameError at runtime.
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
CHANGELOG.md (1)
5-11: Changelog accurately reflects the code changes.Entry clearly calls out the v1 AsyncClient deprecation and the orderbook depth fix. Consider minor polish: wrap AsyncClient and module paths in backticks for readability.
-### Changed -- Marked the v1 AsyncClient as deprecated +### Changed +- Marked the v1 `AsyncClient` as deprecated -### Fixed -- Fixed the Indexer orderbooks queries in the v1 AsyncClient to include the depth parameter +### Fixed +- Fixed the Indexer orderbooks queries in the v1 `AsyncClient` to include the `depth` parameterpyinjective/async_client.py (2)
1346-1351: Depth parameter forwarding looks correct; consider lightweight input validation.Forwarding
depth or 0matches the indexer’s int requirement. Optionally, guard against invalid values (e.g., negatives, non-ints) to fail early with a helpful message.- async def fetch_spot_orderbook_v2(self, market_id: str, depth: Optional[int] = None) -> Dict[str, Any]: - return await self.indexer_client.fetch_spot_orderbook_v2(market_id=market_id, depth=depth or 0) + async def fetch_spot_orderbook_v2(self, market_id: str, depth: Optional[int] = None) -> Dict[str, Any]: + depth_value = 0 if depth is None else depth + if not isinstance(depth_value, int) or depth_value < 0: + raise ValueError("depth must be a non-negative integer") + return await self.indexer_client.fetch_spot_orderbook_v2(market_id=market_id, depth=depth_value) - async def fetch_spot_orderbooks_v2(self, market_ids: List[str], depth: Optional[int] = None) -> Dict[str, Any]: - return await self.indexer_client.fetch_spot_orderbooks_v2(market_ids=market_ids, depth=depth or 0) + async def fetch_spot_orderbooks_v2(self, market_ids: List[str], depth: Optional[int] = None) -> Dict[str, Any]: + depth_value = 0 if depth is None else depth + if not isinstance(depth_value, int) or depth_value < 0: + raise ValueError("depth must be a non-negative integer") + return await self.indexer_client.fetch_spot_orderbooks_v2(market_ids=market_ids, depth=depth_value)If you prefer to keep it minimal, add docstrings documenting that depth must be a non-negative integer and that 0 means “default/full depth” (as supported by the Indexer).
1617-1619: Optional: mirror the depth validation done for spot methods.For consistency and defensive checks, apply the same depth validation here.
- async def fetch_derivative_orderbook_v2(self, market_id: str, depth: Optional[int] = None) -> Dict[str, Any]: - return await self.indexer_client.fetch_derivative_orderbook_v2(market_id=market_id, depth=depth or 0) + async def fetch_derivative_orderbook_v2(self, market_id: str, depth: Optional[int] = None) -> Dict[str, Any]: + depth_value = 0 if depth is None else depth + if not isinstance(depth_value, int) or depth_value < 0: + raise ValueError("depth must be a non-negative integer") + return await self.indexer_client.fetch_derivative_orderbook_v2(market_id=market_id, depth=depth_value)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)pyinjective/async_client.py(3 hunks)pyproject.toml(1 hunks)tests/test_async_client_deprecation_warnings.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_async_client_deprecation_warnings.py (2)
pyinjective/core/network.py (3)
Network(105-370)create_chain_grpc_channel(353-354)create_chain_stream_grpc_channel(362-363)pyinjective/async_client.py (1)
AsyncClient(56-2481)
pyinjective/async_client.py (1)
pyinjective/indexer_client.py (4)
fetch_spot_orderbook_v2(725-726)fetch_spot_orderbooks_v2(728-729)fetch_derivative_orderbook_v2(225-226)fetch_derivative_orderbooks_v2(228-229)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: There might be a mistake here.
Context: ... in this file. ## [1.11.1] - 2025-08-20 ### Changed - Marked the v1 AsyncClient as d...
(QB_NEW_EN)
[grammar] ~6-~6: There might be a mistake here.
Context: ...e. ## [1.11.1] - 2025-08-20 ### Changed - Marked the v1 AsyncClient as deprecated ...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ... v1 AsyncClient as deprecated ### Fixed - Fixed the Indexer orderbooks queries in ...
(QB_NEW_EN)
🪛 Ruff (0.12.2)
tests/test_async_client_deprecation_warnings.py
4-4: pytest imported but unused
Remove unused import: pytest
(F401)
52-52: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
🪛 GitHub Actions: pre-commit
tests/test_async_client_deprecation_warnings.py
[error] 1-1: pre-commit: trailing-whitespace failed; files were modified by the hook (tests/test_async_client_deprecation_warnings.py).
[warning] 41-41: flakeheaven: pycodestyle: line break after binary operator (W504) [pycodestyle]
[error] 4-4: flakeheaven: pyflakes: 'pytest' imported but unused (F401) [pyflakes]
[error] 1-1: black: reformatted by Black; run 'black --write' to fix code style issues.
pyinjective/async_client.py
[error] 1620-1620: flakeheaven: pycodestyle: line too long (121 > 120 characters) [pycodestyle]
[error] 1-1: black: reformatted by Black; run 'black --write' to fix code style issues.
🔇 Additional comments (2)
pyproject.toml (1)
3-3: Version bump aligned and scoped correctly.1.11.1 matches the new CHANGELOG entry and the surface-area change is limited to the version field. No concerns.
pyinjective/async_client.py (1)
61-66: Deprecation warning is correctly implemented.Clear message, correct category, and stacklevel=2 ensures it points to the caller. This aligns with the new deprecation policy.
| # Check warning message contains migration advice | ||
| assert "Please use AsyncClient from pyinjective.async_client_v2 instead" in str(warning.message) | ||
| # Check warning category | ||
| assert warning.category == DeprecationWarning |
There was a problem hiding this comment.
Use identity comparison for classes to address E721.
Compare categories with “is” instead of equality.
- assert warning.category == DeprecationWarning
+ assert warning.category is DeprecationWarning📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert warning.category == DeprecationWarning | |
| assert warning.category is DeprecationWarning |
🧰 Tools
🪛 Ruff (0.12.2)
52-52: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
🤖 Prompt for AI Agents
In tests/test_async_client_deprecation_warnings.py around line 52, the test
compares warning.category to DeprecationWarning using == which triggers E721;
change the comparison to use identity (warning.category is DeprecationWarning)
so the class object is compared by identity rather than equality, updating that
assertion accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/test_async_client_deprecation_warnings.py (1)
50-50: Use identity comparison for classes (E721).Compare the class directly with
isto satisfy Ruff/pycodestyle.- assert warning.category == DeprecationWarning + assert warning.category is DeprecationWarning
🧹 Nitpick comments (1)
tests/test_indexer_client_deprecation_warnings.py (1)
70-70: Use identity comparison for classes (E721).Match style used elsewhere and satisfy Ruff:
- assert warning.category == DeprecationWarning + assert warning.category is DeprecationWarning
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pyinjective/async_client.py(3 hunks)tests/test_async_client_deprecation_warnings.py(1 hunks)tests/test_indexer_client_deprecation_warnings.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyinjective/async_client.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/test_indexer_client_deprecation_warnings.py (4)
pyinjective/core/network.py (3)
Network(105-370)create_exchange_grpc_channel(356-357)create_explorer_grpc_channel(359-360)pyinjective/indexer_client.py (1)
IndexerClient(26-1189)pyinjective/client/indexer/grpc_stream/indexer_grpc_derivative_stream.py (1)
stream_positions(72-88)pyinjective/async_client.py (1)
listen_derivative_positions_updates(1813-1835)
tests/test_async_client_deprecation_warnings.py (2)
pyinjective/core/network.py (3)
Network(105-370)create_chain_grpc_channel(353-354)create_chain_stream_grpc_channel(362-363)pyinjective/async_client.py (1)
AsyncClient(56-2483)
🪛 Ruff (0.12.2)
tests/test_indexer_client_deprecation_warnings.py
70-70: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
tests/test_async_client_deprecation_warnings.py
50-50: Use is and is not for type comparisons, or isinstance() for isinstance checks
(E721)
🪛 GitHub Actions: run-tests
tests/test_indexer_client_deprecation_warnings.py
[error] 64-64: Test 'test_listen_derivative_positions_updates_deprecation_warning' failed: expected 1 deprecation warning containing "This method is deprecated. Use listen_derivative_positions_v2_updates instead", but none were emitted. Command: 'poetry run pytest --cov --cov-report=xml -v --full-trace'.
🔇 Additional comments (1)
tests/test_async_client_deprecation_warnings.py (1)
1-58: LGTM: solid deprecation warning coverage and isolation of side effects.Nice job stubbing network/channel creation and capturing the DeprecationWarning with stacklevel validation. No other issues spotted here.
| # Create IndexerClient instance | ||
| client = IndexerClient(network=mock_network) | ||
|
|
||
| # Mock the derivative_stream_api.stream_positions method to avoid actual streaming | ||
| client.derivative_stream_api.stream_positions = MagicMock() | ||
|
|
||
| # Capture warnings | ||
| with warnings.catch_warnings(record=True) as warning_list: | ||
| warnings.simplefilter("always") # Ensure all warnings are captured | ||
|
|
||
| # Mock callback function | ||
| mock_callback = MagicMock() | ||
|
|
||
| # Call the deprecated method - this should trigger the deprecation warning | ||
| client.listen_derivative_positions_updates( | ||
| callback=mock_callback, market_ids=["market_1"], subaccount_ids=["subaccount_1"] | ||
| ) |
There was a problem hiding this comment.
Fix: await the coroutine so the DeprecationWarning is actually emitted (matches CI failure).
IndexerClient.listen_derivative_positions_updates is async. The test currently calls it without awaiting, so the function body (and warning) never executes, causing the CI failure. Wrap the call in asyncio.run(...) (and make stream_positions an AsyncMock) to execute it.
+import asyncio
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, patch, AsyncMock
@@
- client.derivative_stream_api.stream_positions = MagicMock()
+ client.derivative_stream_api.stream_positions = AsyncMock()
@@
- client.listen_derivative_positions_updates(
- callback=mock_callback, market_ids=["market_1"], subaccount_ids=["subaccount_1"]
- )
+ asyncio.run(
+ client.listen_derivative_positions_updates(
+ callback=mock_callback, market_ids=["market_1"], subaccount_ids=["subaccount_1"]
+ )
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Create IndexerClient instance | |
| client = IndexerClient(network=mock_network) | |
| # Mock the derivative_stream_api.stream_positions method to avoid actual streaming | |
| client.derivative_stream_api.stream_positions = MagicMock() | |
| # Capture warnings | |
| with warnings.catch_warnings(record=True) as warning_list: | |
| warnings.simplefilter("always") # Ensure all warnings are captured | |
| # Mock callback function | |
| mock_callback = MagicMock() | |
| # Call the deprecated method - this should trigger the deprecation warning | |
| client.listen_derivative_positions_updates( | |
| callback=mock_callback, market_ids=["market_1"], subaccount_ids=["subaccount_1"] | |
| ) | |
| import asyncio | |
| from unittest.mock import MagicMock, patch, AsyncMock | |
| # Create IndexerClient instance | |
| client = IndexerClient(network=mock_network) | |
| # Mock the derivative_stream_api.stream_positions method to avoid actual streaming | |
| client.derivative_stream_api.stream_positions = AsyncMock() | |
| # Capture warnings | |
| with warnings.catch_warnings(record=True) as warning_list: | |
| warnings.simplefilter("always") # Ensure all warnings are captured | |
| # Mock callback function | |
| mock_callback = MagicMock() | |
| # Call the deprecated method - this should trigger the deprecation warning | |
| asyncio.run( | |
| client.listen_derivative_positions_updates( | |
| callback=mock_callback, market_ids=["market_1"], subaccount_ids=["subaccount_1"] | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In tests/test_indexer_client_deprecation_warnings.py around lines 37 to 53, the
test calls the async IndexerClient.listen_derivative_positions_updates without
awaiting so the coroutine body (and its DeprecationWarning) never runs; change
the test to await the coroutine (e.g., wrap the call in asyncio.run(...) or use
the test event loop) and make client.derivative_stream_api.stream_positions an
AsyncMock so the awaited call executes correctly and the warning is emitted and
captured.
| # Verify the underlying method was still called (functionality preserved) | ||
| client.derivative_stream_api.stream_positions.assert_called_once_with( | ||
| callback=mock_callback, | ||
| on_end_callback=None, | ||
| on_status_callback=None, | ||
| market_ids=["market_1"], | ||
| subaccount_ids=["subaccount_1"], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert the awaited call on AsyncMock for accuracy.
Since stream_positions is awaited, assert the awaited call (not just called).
- client.derivative_stream_api.stream_positions.assert_called_once_with(
+ client.derivative_stream_api.stream_positions.assert_awaited_once_with(
callback=mock_callback,
on_end_callback=None,
on_status_callback=None,
market_ids=["market_1"],
subaccount_ids=["subaccount_1"],
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Verify the underlying method was still called (functionality preserved) | |
| client.derivative_stream_api.stream_positions.assert_called_once_with( | |
| callback=mock_callback, | |
| on_end_callback=None, | |
| on_status_callback=None, | |
| market_ids=["market_1"], | |
| subaccount_ids=["subaccount_1"], | |
| ) | |
| # Verify the underlying method was still called (functionality preserved) | |
| client.derivative_stream_api.stream_positions.assert_awaited_once_with( | |
| callback=mock_callback, | |
| on_end_callback=None, | |
| on_status_callback=None, | |
| market_ids=["market_1"], | |
| subaccount_ids=["subaccount_1"], | |
| ) |
🤖 Prompt for AI Agents
In tests/test_indexer_client_deprecation_warnings.py around lines 74 to 81, the
test currently uses assert_called_once_with on an AsyncMock for stream_positions
even though the method is awaited; replace the call assertion with the
awaited-call assertion by using the AsyncMock's assert_awaited_once_with and
pass the same keyword args (callback=mock_callback, on_end_callback=None,
on_status_callback=None, market_ids=["market_1"],
subaccount_ids=["subaccount_1"]) so the test verifies the awaited invocation
rather than a plain call.
aarmoa
deleted the
fix/add_depth_to_legacy_async_client_orderbooks_queries
branch
1 participant
Summary by CodeRabbit
Changed
Bug Fixes
Tests
Chores