Merged
Conversation
…ster Release/release v140 to master
…ods_from_examples Fix/remove deprecated methods from examples
Contributor
WalkthroughThe update enhances the 'tendermint' module in version 1.5.0, expanding the async client's capabilities for interacting with the Tendermint blockchain. It introduces features for querying node information, syncing status, latest blocks, blocks by height, and validator sets. These enhancements are distributed across Python scripts, the core library, and test suites, bolstering support for Tendermint queries within the Injective Protocol ecosystem. Changes
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #313 +/- ##
==========================================
+ Coverage 76.16% 76.40% +0.24%
==========================================
Files 50 51 +1
Lines 4111 4170 +59
Branches 351 353 +2
==========================================
+ Hits 3131 3186 +55
- Misses 875 878 +3
- Partials 105 106 +1 ☔ View full report in Codecov by Sentry. |
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Configuration used: .coderabbit.yaml
Files selected for processing (13)
- CHANGELOG.md (1 hunks)
- examples/chain_client/tendermint/query/1_GetNodeInfo.py (1 hunks)
- examples/chain_client/tendermint/query/2_GetSyncing.py (1 hunks)
- examples/chain_client/tendermint/query/3_GetLatestBlock.py (1 hunks)
- examples/chain_client/tendermint/query/4_GetBlockByHeight.py (1 hunks)
- examples/chain_client/tendermint/query/5_GetLatestValidatorSet.py (1 hunks)
- examples/chain_client/tendermint/query/6_GetValidatorSetByHeight.py (1 hunks)
- pyinjective/async_client.py (6 hunks)
- pyinjective/core/tx/grpc/tenderming_grpc_api.py (1 hunks)
- pyproject.toml (1 hunks)
- tests/core/tx/grpc/configurable_tendermint_query_servicer.py (1 hunks)
- tests/core/tx/grpc/test_tendermint_grpc_api.py (1 hunks)
- tests/test_async_client_deprecation_warnings.py (4 hunks)
Check Runs (1)
codecov/patch completed (9)
- pyinjective/async_client.py: 391-391: Added line #L391 was not covered by tests
- pyinjective/async_client.py: 508-509: Added lines #L508 - L509 were not covered by tests
- pyinjective/async_client.py: 1236-1236: Added line #L1236 was not covered by tests
- pyinjective/async_client.py: 1239-1239: Added line #L1239 was not covered by tests
- pyinjective/async_client.py: 1253-1253: Added line #L1253 was not covered by tests
- pyinjective/async_client.py: 1256-1256: Added line #L1256 was not covered by tests
- pyinjective/async_client.py: 1261-1261: Added line #L1261 was not covered by tests
- pyinjective/async_client.py: 1266-1266: Added line #L1266 was not covered by tests
- pyinjective/core/tx/grpc/tenderming_grpc_api.py: 52-52: Added line #L52 was not covered by tests
Files skipped from review due to trivial changes (1)
- pyproject.toml
Additional comments: 12
examples/chain_client/tendermint/query/5_GetLatestValidatorSet.py (1)
- 1-19: The script
5_GetLatestValidatorSet.pycorrectly demonstrates fetching the latest validator set. However, the importfrom google.protobuf import symbol_databaseand its instantiationsymbol_db = symbol_database.Default()are not used within the script. Unless there's a side-effect or requirement not visible in this context, consider removing these to clean up the code.examples/chain_client/tendermint/query/6_GetValidatorSetByHeight.py (1)
- 1-22: The script
6_GetValidatorSetByHeight.pyeffectively demonstrates fetching a validator set by height with pagination. As in the previous script, thegoogle.protobuf.symbol_databaseimport and instantiation appear unused. If these are not required for the script's functionality, removing them would streamline the code.tests/core/tx/grpc/configurable_tendermint_query_servicer.py (1)
- 1-43: The
ConfigurableTendermintQueryServicerclass is a well-designed component for setting up mock responses in tests, enhancing the flexibility and robustness of testing tendermint queries. This approach allows for comprehensive testing of various scenarios without the need for a live network connection. Good job on implementing this testing utility.tests/core/tx/grpc/test_tendermint_grpc_api.py (4)
- 16-18: Creating a fixture for
ConfigurableTendermintQueryServiceris a good practice as it promotes test reusability and maintainability.- 21-385: The test methods within
TestTxGrpcApiare well-structured and follow asynchronous testing patterns withpytest.mark.asyncio, which is appropriate for testing async functions.However, consider adding more comprehensive assertions to validate the structure and data types of the responses, ensuring they match expected formats beyond just value equality.
- 72-73: Using a dummy metadata provider function (
_dummy_metadata_provider) is a clever way to isolate tests from external dependencies. However, ensure that this approach does not skip important integration points that should be tested, especially regarding metadata handling.- 386-387: The
_dummy_metadata_providermethod is defined but not used within the test suite. If it's intended for future use, consider adding a comment explaining its purpose. Otherwise, it might be better to remove it to keep the code clean.tests/test_async_client_deprecation_warnings.py (4)
- 9-9: Adding the import for
tendermint_queryis necessary for the new Tendermint-related tests. This change aligns with the PR's objective to enhance Tendermint module support.- 37-37: The addition of
ConfigurableTendermintQueryServiceras a fixture is crucial for testing the new Tendermint functionalities. It's good to see thorough testing being considered for the new features.- 111-113: The
tendermint_servicerfixture is well-defined and follows the pattern of other service fixtures in the file. This consistency is important for maintainability.- 1711-1727: The new test method
test_get_latest_block_deprecation_warningcorrectly tests for a deprecation warning when usingget_latest_block, which is being replaced byfetch_latest_block. This change is consistent with the PR's goal to update and deprecate older methods in favor of new implementations. However, it's essential to ensure that all references to the deprecated method are updated across the codebase.Verification successful
The verification process confirms that the deprecated method
get_latest_blockis only referenced in its definition and the test designed to check for its deprecation warning. This is consistent with the goal of deprecating older methods in favor of new implementations, as mentioned in the review comment. Therefore, the review comment is accurate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the deprecated `get_latest_block` method. rg --type py 'get_latest_block\('Length of output: 236
pyinjective/async_client.py (1)
- 197-202: Initialization of
self.tendermint_apiwithTendermintGrpcApiis correctly done, ensuring that the Tendermint API is available for use within theAsyncClientclass.
|
|
||
| from pyinjective.client.model.pagination import PaginationOption | ||
| from pyinjective.core.network import Network | ||
| from pyinjective.core.tx.grpc.tenderming_grpc_api import TendermintGrpcApi |
Contributor
There was a problem hiding this comment.
The import path for TendermintGrpcApi seems to contain a typo: tenderming_grpc_api should likely be tendermint_grpc_api.
- from pyinjective.core.tx.grpc.tenderming_grpc_api import TendermintGrpcApi
+ from pyinjective.core.tx.grpc.tendermint_grpc_api import TendermintGrpcApiCommittable 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.
Suggested change
| from pyinjective.core.tx.grpc.tenderming_grpc_api import TendermintGrpcApi | |
| from pyinjective.core.tx.grpc.tendermint_grpc_api import TendermintGrpcApi |
Contributor
There was a problem hiding this comment.
@keithbaum, the verification confirms that the correct filename is indeed tendermint_grpc_api.py, and the initial suggestion to correct the typo was accurate. Thank you for pointing out the typo, and the suggested correction is as follows:
- from pyinjective.core.tx.grpc.tenderming_grpc_api import TendermintGrpcApi
+ from pyinjective.core.tx.grpc.tendermint_grpc_api import TendermintGrpcApiThis correction aligns with the actual filename in the repository, ensuring the import statement is correct.
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the correct filename in the repository to verify the typo correction.
fd 'tendermint_grpc_api.py'
Length of output: 74
| from pyinjective.core.market import BinaryOptionMarket, DerivativeMarket, SpotMarket | ||
| from pyinjective.core.network import Network | ||
| from pyinjective.core.token import Token | ||
| from pyinjective.core.tx.grpc.tenderming_grpc_api import TendermintGrpcApi |
Contributor
There was a problem hiding this comment.
The import path for TendermintGrpcApi contains a typo: tenderming_grpc_api should be tendermint_grpc_api.
- from pyinjective.core.tx.grpc.tenderming_grpc_api import TendermintGrpcApi
+ from pyinjective.core.tx.grpc.tendermint_grpc_api import TendermintGrpcApiCommittable 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.
Suggested change
| from pyinjective.core.tx.grpc.tenderming_grpc_api import TendermintGrpcApi | |
| from pyinjective.core.tx.grpc.tendermint_grpc_api import TendermintGrpcApi |
Comment on lines
+1
to
+16
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
|
|
||
| syncing = await client.fetch_syncing() | ||
| print(syncing) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.get_event_loop().run_until_complete(main()) |
Contributor
There was a problem hiding this comment.
The script 2_GetSyncing.py is well-structured and follows good practices for asynchronous programming in Python. It correctly initializes the AsyncClient and fetches the syncing status. However, consider adding error handling around the await client.fetch_syncing() call to gracefully handle potential exceptions that could arise from network issues or API changes.
Comment on lines
+1
to
+16
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
|
|
||
| node_info = await client.fetch_node_info() | ||
| print(node_info) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.get_event_loop().run_until_complete(main()) |
Contributor
There was a problem hiding this comment.
The script 1_GetNodeInfo.py is concise and follows asynchronous programming conventions. Like the previous script, adding error handling for the await client.fetch_node_info() call would improve the robustness of the script by managing exceptions that might occur during the API call.
Comment on lines
+1
to
+16
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
|
|
||
| latest_block = await client.fetch_latest_block() | ||
| print(latest_block) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.get_event_loop().run_until_complete(main()) |
Contributor
There was a problem hiding this comment.
The script 3_GetLatestBlock.py effectively demonstrates how to retrieve the latest block information asynchronously. Consistent with previous scripts, incorporating error handling around the await client.fetch_latest_block() call would enhance the script's reliability in handling potential runtime exceptions.
Comment on lines
+1
to
+16
| import asyncio | ||
|
|
||
| from pyinjective.async_client import AsyncClient | ||
| from pyinjective.core.network import Network | ||
|
|
||
|
|
||
| async def main() -> None: | ||
| network = Network.testnet() | ||
| client = AsyncClient(network) | ||
|
|
||
| block = await client.fetch_block_by_height(height=15793860) | ||
| print(block) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.get_event_loop().run_until_complete(main()) |
Contributor
There was a problem hiding this comment.
The script 4_GetBlockByHeight.py is clear and demonstrates fetching a block by height. For improved usability, consider adding a comment to explain that the height value is a placeholder and should be replaced with the desired block height by the user. Additionally, implementing error handling for the await client.fetch_block_by_height(height=15793860) call would make the script more robust.
Comment on lines
+1
to
+69
| from typing import Any, Callable, Dict, Optional | ||
|
|
||
| from grpc.aio import Channel | ||
|
|
||
| from pyinjective.client.model.pagination import PaginationOption | ||
| from pyinjective.proto.cosmos.base.tendermint.v1beta1 import ( | ||
| query_pb2 as tendermint_query, | ||
| query_pb2_grpc as tendermint_query_grpc, | ||
| ) | ||
| from pyinjective.utils.grpc_api_request_assistant import GrpcApiRequestAssistant | ||
|
|
||
|
|
||
| class TendermintGrpcApi: | ||
| def __init__(self, channel: Channel, metadata_provider: Callable): | ||
| self._stub = tendermint_query_grpc.ServiceStub(channel) | ||
| self._assistant = GrpcApiRequestAssistant(metadata_provider=metadata_provider) | ||
|
|
||
| async def fetch_node_info(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetNodeInfoRequest() | ||
| response = await self._execute_call(call=self._stub.GetNodeInfo, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_syncing(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetSyncingRequest() | ||
| response = await self._execute_call(call=self._stub.GetSyncing, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_latest_block(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetLatestBlockRequest() | ||
| response = await self._execute_call(call=self._stub.GetLatestBlock, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_block_by_height(self, height: int) -> Dict[str, Any]: | ||
| request = tendermint_query.GetBlockByHeightRequest(height=height) | ||
| response = await self._execute_call(call=self._stub.GetBlockByHeight, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_latest_validator_set(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetLatestValidatorSetRequest() | ||
| response = await self._execute_call(call=self._stub.GetLatestValidatorSet, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_validator_set_by_height( | ||
| self, height: int, pagination: Optional[PaginationOption] = None | ||
| ) -> Dict[str, Any]: | ||
| if pagination is None: | ||
| pagination = PaginationOption() | ||
| request = tendermint_query.GetValidatorSetByHeightRequest( | ||
| height=height, pagination=pagination.create_pagination_request() | ||
| ) | ||
| response = await self._execute_call(call=self._stub.GetValidatorSetByHeight, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def abci_query( | ||
| self, path: str, data: Optional[bytes] = None, height: Optional[int] = None, prove: bool = False | ||
| ) -> Dict[str, Any]: | ||
| request = tendermint_query.ABCIQueryRequest(path=path, data=data, height=height, prove=prove) | ||
| response = await self._execute_call(call=self._stub.ABCIQuery, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def _execute_call(self, call: Callable, request) -> Dict[str, Any]: | ||
| return await self._assistant.execute_call(call=call, request=request) |
Contributor
There was a problem hiding this comment.
The TendermintGrpcApi class is comprehensive and provides a wide range of functionalities for interacting with the Tendermint gRPC API. It's well-structured and follows best practices for asynchronous programming and gRPC interactions. However, it appears that the default pagination logic introduced in line 52 might not be covered by tests. Ensuring coverage for this logic is important for validating its behavior and handling edge cases.
Comment on lines
+13
to
+68
| class TendermintGrpcApi: | ||
| def __init__(self, channel: Channel, metadata_provider: Callable): | ||
| self._stub = tendermint_query_grpc.ServiceStub(channel) | ||
| self._assistant = GrpcApiRequestAssistant(metadata_provider=metadata_provider) | ||
|
|
||
| async def fetch_node_info(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetNodeInfoRequest() | ||
| response = await self._execute_call(call=self._stub.GetNodeInfo, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_syncing(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetSyncingRequest() | ||
| response = await self._execute_call(call=self._stub.GetSyncing, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_latest_block(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetLatestBlockRequest() | ||
| response = await self._execute_call(call=self._stub.GetLatestBlock, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_block_by_height(self, height: int) -> Dict[str, Any]: | ||
| request = tendermint_query.GetBlockByHeightRequest(height=height) | ||
| response = await self._execute_call(call=self._stub.GetBlockByHeight, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_latest_validator_set(self) -> Dict[str, Any]: | ||
| request = tendermint_query.GetLatestValidatorSetRequest() | ||
| response = await self._execute_call(call=self._stub.GetLatestValidatorSet, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def fetch_validator_set_by_height( | ||
| self, height: int, pagination: Optional[PaginationOption] = None | ||
| ) -> Dict[str, Any]: | ||
| if pagination is None: | ||
| pagination = PaginationOption() | ||
| request = tendermint_query.GetValidatorSetByHeightRequest( | ||
| height=height, pagination=pagination.create_pagination_request() | ||
| ) | ||
| response = await self._execute_call(call=self._stub.GetValidatorSetByHeight, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def abci_query( | ||
| self, path: str, data: Optional[bytes] = None, height: Optional[int] = None, prove: bool = False | ||
| ) -> Dict[str, Any]: | ||
| request = tendermint_query.ABCIQueryRequest(path=path, data=data, height=height, prove=prove) | ||
| response = await self._execute_call(call=self._stub.ABCIQuery, request=request) | ||
|
|
||
| return response | ||
|
|
||
| async def _execute_call(self, call: Callable, request) -> Dict[str, Any]: |
Contributor
There was a problem hiding this comment.
The TendermintGrpcApi class is well-structured and provides a comprehensive set of methods for interacting with the Tendermint gRPC API. Each method is designed to fetch specific information, such as node info, syncing status, latest block, block by height, latest validator set, validator set by height, and perform an ABCI query. The use of asynchronous programming ensures that these operations do not block the main thread, which is crucial for performance in I/O-bound tasks like network requests.
A few points to consider for further refinement:
- Error Handling: It's important to ensure robust error handling around the gRPC calls. Consider catching exceptions specific to gRPC calls and handling them appropriately, possibly logging the errors or retrying the calls under certain conditions.
- Type Annotations: The return type of each method is annotated as
Dict[str, Any], which is quite generic. If possible, providing more specific type annotations would enhance code readability and maintainability by making the expected structure of the responses clearer. - Documentation: Adding docstrings to each method would greatly improve the understandability of the code. These should describe the purpose of the method, its parameters, its return type, and any exceptions it might raise.
- Pagination Handling: For methods that support pagination, such as
fetch_validator_set_by_height, consider documenting how pagination is handled and what the caller should expect in terms of response structure, especially how to retrieve subsequent pages.
Overall, the implementation aligns well with the objectives of enhancing the software by adding comprehensive support for Tendermint module queries. These suggestions aim to further improve the code's robustness, clarity, and maintainability.
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yaml
Files selected for processing (2)
- pyinjective/async_client.py (6 hunks)
- tests/core/tx/grpc/test_tendermint_grpc_api.py (1 hunks)
Check Runs (1)
codecov/patch completed (9)
- pyinjective/async_client.py: 391-391: Added line #L391 was not covered by tests
- pyinjective/async_client.py: 508-509: Added lines #L508 - L509 were not covered by tests
- pyinjective/async_client.py: 1236-1236: Added line #L1236 was not covered by tests
- pyinjective/async_client.py: 1239-1239: Added line #L1239 was not covered by tests
- pyinjective/async_client.py: 1253-1253: Added line #L1253 was not covered by tests
- pyinjective/async_client.py: 1256-1256: Added line #L1256 was not covered by tests
- pyinjective/async_client.py: 1261-1261: Added line #L1261 was not covered by tests
- pyinjective/async_client.py: 1266-1266: Added line #L1266 was not covered by tests
Files skipped from review as they are similar to previous changes (1)
- tests/core/tx/grpc/test_tendermint_grpc_api.py
Additional comments: 3
pyinjective/async_client.py (3)
- 42-42: The import of
TendermintGrpcApiis correctly added to support the new Tendermint module functionalities.- 53-53: The import of
ed25519_keysis added, presumably for handling validator set responses. Ensure that this import is utilized within the code to justify its presence.- 197-202: Initialization of
self.tendermint_apiwithTendermintGrpcApiis correctly done, aligning with the PR's objective to enhance Tendermint module support.
Comment on lines
+390
to
+391
| block = await self.fetch_latest_block() | ||
| self.timeout_height = int(block["block"]["header"]["height"]) + DEFAULT_TIMEOUTHEIGHT |
Contributor
There was a problem hiding this comment.
The method sync_timeout_height is not covered by tests. It's crucial to ensure that the functionality for syncing the timeout height is correctly implemented and does not introduce any unexpected behavior.
Would you like me to help generate tests for this method or open a GitHub issue to track this task?
Comment on lines
+508
to
+509
| latest_block = await self.fetch_latest_block() | ||
| return latest_block["block"]["header"]["chainId"] |
Contributor
There was a problem hiding this comment.
The method get_chain_id is not covered by tests. Considering this method fetches the chain ID from the latest block, it's essential to verify its correctness and reliability through testing.
Would you like assistance in creating tests for this method or should I open a GitHub issue to ensure it's tracked?
Comment on lines
+1234
to
+1268
| # region Tendermint module | ||
| async def fetch_node_info(self) -> Dict[str, Any]: | ||
| return await self.tendermint_api.fetch_node_info() | ||
|
|
||
| async def fetch_syncing(self) -> Dict[str, Any]: | ||
| return await self.tendermint_api.fetch_syncing() | ||
|
|
||
| async def get_latest_block(self) -> tendermint_query.GetLatestBlockResponse: | ||
| """ | ||
| This method is deprecated and will be removed soon. Please use `fetch_latest_block` instead | ||
| """ | ||
| warn("This method is deprecated. Use fetch_latest_block instead", DeprecationWarning, stacklevel=2) | ||
| req = tendermint_query.GetLatestBlockRequest() | ||
| return await self.stubCosmosTendermint.GetLatestBlock(req) | ||
|
|
||
| async def fetch_latest_block(self) -> Dict[str, Any]: | ||
| return await self.tendermint_api.fetch_latest_block() | ||
|
|
||
| async def fetch_block_by_height(self, height: int) -> Dict[str, Any]: | ||
| return await self.tendermint_api.fetch_block_by_height(height=height) | ||
|
|
||
| async def fetch_latest_validator_set(self) -> Dict[str, Any]: | ||
| return await self.tendermint_api.fetch_latest_validator_set() | ||
|
|
||
| async def fetch_validator_set_by_height( | ||
| self, height: int, pagination: Optional[PaginationOption] = None | ||
| ) -> Dict[str, Any]: | ||
| return await self.tendermint_api.fetch_validator_set_by_height(height=height, pagination=pagination) | ||
|
|
||
| async def abci_query( | ||
| self, path: str, data: Optional[bytes] = None, height: Optional[int] = None, prove: bool = False | ||
| ) -> Dict[str, Any]: | ||
| return await self.tendermint_api.abci_query(path=path, data=data, height=height, prove=prove) | ||
|
|
||
| # endregion |
Contributor
There was a problem hiding this comment.
The newly added methods in the Tendermint module (fetch_node_info, fetch_syncing, fetch_latest_block, fetch_block_by_height, fetch_latest_validator_set, fetch_validator_set_by_height, and abci_query) are crucial for interacting with the Tendermint gRPC API. However, several of these methods are not covered by tests, which is essential for ensuring their correct functionality and integration.
Would you like help in creating tests for these Tendermint module methods, or should I open a GitHub issue to track this task?
nicolasbaum
approved these changes
Mar 14, 2024
nicolasbaum
approved these changes
Mar 14, 2024
nicolasbaum
approved these changes
Mar 14, 2024
added 5 commits
March 14, 2024 17:05
Contributor
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yaml
Files selected for processing (11)
- CHANGELOG.md (1 hunks)
- examples/chain_client/3_MessageBroadcaster.py (1 hunks)
- examples/chain_client/5_MessageBroadcasterWithoutSimulation.py (1 hunks)
- examples/chain_client/exchange/8_MsgCancelSpotOrder.py (1 hunks)
- examples/chain_client/exchange/9_MsgBatchUpdateOrders.py (1 hunks)
- pyinjective/denoms_devnet.ini (4 hunks)
- pyinjective/denoms_mainnet.ini (9 hunks)
- pyinjective/denoms_testnet.ini (5 hunks)
- tests/core/test_gas_limit_estimator.py (10 hunks)
- tests/test_async_client_deprecation_warnings.py (5 hunks)
- tests/test_composer_deprecation_warnings.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_async_client_deprecation_warnings.py
Additional comments: 41
examples/chain_client/3_MessageBroadcaster.py (1)
- 59-59: The function call has been updated from
composer.MsgBatchUpdateOrderstocomposer.msg_batch_update_orders. This change aligns with Python's naming conventions for methods, promoting readability and consistency. Ensure that all references to this function throughout the codebase have been updated accordingly.examples/chain_client/5_MessageBroadcasterWithoutSimulation.py (1)
- 59-59: The function call has been updated from
composer.MsgBatchUpdateOrderstocomposer.msg_batch_update_orders. This change is consistent with Python's naming conventions and improves readability. Ensure that all references to this function throughout the codebase have been updated accordingly.examples/chain_client/exchange/8_MsgCancelSpotOrder.py (1)
- 38-38: The function call has been updated from
composer.MsgCancelSpotOrdertocomposer.msg_cancel_spot_order. This change adheres to Python's naming conventions, enhancing readability and consistency. Verify that all references to this function throughout the codebase have been updated accordingly.examples/chain_client/exchange/9_MsgBatchUpdateOrders.py (1)
- 121-121: The function call has been renamed from
composer.MsgBatchUpdateOrderstocomposer.msg_batch_update_orders. This change is in line with Python's naming conventions, improving readability and consistency. Ensure that all references to this function throughout the codebase have been updated accordingly.CHANGELOG.md (1)
- 9-11: The update for version 1.4.1 mentions updates to example scripts that were still using deprecated methods. Ensure that all example scripts have been reviewed and updated accordingly to prevent any potential issues with deprecated methods.
pyinjective/denoms_devnet.ini (5)
- 167-170: The tick sizes and quantities for the
Devnet Spot PROJ/INJpair have been updated. Ensure these values align with the intended precision and liquidity requirements for this trading pair.- 181-188: New trading pair
Devnet Spot QUNT/INJadded with specific configurations. Verify that themin_price_tick_sizeandmin_quantity_tick_sizeare appropriate for the market dynamics of this pair.- 190-197: New trading pair
Devnet Spot HDRO/INJadded. It's crucial to ensure that the largemin_price_tick_sizeandmin_quantity_tick_sizevalues are suitable for the expected trading volume and liquidity.- 317-320: The
HDROtoken configuration has been added. Confirm that thepeggy_denomanddecimalsare correctly set up to match the token's specifications on the blockchain.- 341-344: The
QUNTtoken configuration has been introduced. It's important to verify the accuracy of thepeggy_denomanddecimalsto ensure seamless integration with the blockchain.pyinjective/denoms_testnet.ini (9)
- 145-152: New trading pair
Testnet Spot TEST2/INJadded. Ensure themin_price_tick_sizeandmin_quantity_tick_sizeare set correctly to facilitate accurate and efficient trading.- 154-161: New trading pair
Testnet Spot TEST2/USDTadded. It's important to verify that the tick sizes and quantities are appropriate for the market conditions of this pair.- 226-233: The configuration for
Testnet Spot Injhas been updated. Double-check that themin_price_tick_sizeandmin_quantity_tick_sizeadjustments are in line with the desired trading precision and liquidity.- 235-242: New trading pair
Testnet Spot MT/INJadded. Confirm that themin_price_tick_sizeandmin_quantity_tick_sizevalues are suitable for the expected trading volume and liquidity of this pair.- 253-260: New trading pair
Testnet Spot HDRO/INJadded. It's crucial to ensure that themin_price_tick_sizeandmin_quantity_tick_sizevalues are appropriate for the market dynamics of this pair.- 468-471: The
HDROtoken configuration has been added. Verify that thepeggy_denomanddecimalsare correctly set up to match the token's specifications on the blockchain.- 476-479: The
MTtoken configuration has been updated. Ensure that thepeggy_denomanddecimalsaccurately reflect the token's properties for seamless integration with the blockchain.- 492-499: New token configurations for
TEST1andTEST2have been introduced. Confirm the accuracy ofpeggy_denomanddecimalsto ensure correct handling of these tokens on the testnet.- 528-531: The
hINJtoken configuration has been added. It's important to verify thepeggy_denomanddecimalsto ensure they align with the token's blockchain specifications.tests/core/test_gas_limit_estimator.py (10)
- 160-160: The change from
composer.MsgBatchUpdateOrderstocomposer.msg_batch_update_ordersin the method call for creating a batch update order message for spot orders is consistent with the PR's objective to refactor and enhance the codebase. However, ensure that thecomposerclass's method names are consistently used throughout the codebase to maintain readability and prevent confusion.- 197-197: The modification to use
composer.msg_batch_update_ordersfor creating derivative orders aligns with the PR's goal of code enhancement. It's important to verify that all references to the old method name have been updated across the entire project to ensure consistency and avoid runtime errors.- 252-252: Switching to
composer.msg_batch_update_ordersfor creating binary options orders is a positive change that supports the PR's objectives. Double-check that the new method is implemented correctly in thecomposerclass and that its usage is consistent across the project.- 287-287: The update to use
composer.msg_batch_update_ordersfor canceling spot orders is in line with the PR's aim to refactor the code. It's crucial to ensure that the method's implementation in thecomposerclass handles all order types correctly and that the change is reflected wherever necessary in the project.- 321-321: Changing to
composer.msg_batch_update_ordersfor canceling derivative orders reflects the PR's intention to improve the code structure. Confirm that thecomposerclass's method is thoroughly tested for handling derivative orders and that the project consistently uses the new method name.- 355-355: The transition to
composer.msg_batch_update_ordersfor canceling binary options orders supports the PR's goal of code enhancement. Ensure that the method in thecomposerclass is correctly implemented to handle binary options orders and that the change is consistently applied across the project.- 374-374: Using
composer.msg_batch_update_ordersto cancel all orders for a spot market is a good practice that aligns with the PR's objectives. Verify that thecomposerclass's method adequately supports this functionality and that the change is uniformly applied throughout the project.- 394-394: The modification to use
composer.msg_batch_update_ordersfor canceling all orders in a derivative market is consistent with the PR's aim. It's essential to ensure that the method in thecomposerclass is fully functional for this purpose and that the project consistently adopts the new method name.- 414-414: Switching to
composer.msg_batch_update_ordersfor canceling all orders in a binary options market supports the PR's objectives. Confirm that thecomposerclass's method is correctly implemented for this functionality and that the change is consistently applied across the project.- 443-443: The change to use
composer.msg_batch_update_orderswithin anexecmessage for creating spot orders is in line with the PR's goal of code enhancement. Ensure that thecomposerclass's method is properly tested in this context and that the change is uniformly adopted in the project.tests/test_composer_deprecation_warnings.py (3)
- 53-64: The test
test_coin_deprecation_warningcorrectly checks for a deprecation warning when using theCoinmethod. However, it's important to ensure that the new methodcoinexists and is properly documented to guide users towards the updated usage.- 66-66: The renaming of
teste_order_data_deprecation_warningtotest_order_data_deprecation_warningcorrects a typographical error in the method name, improving readability and consistency in naming conventions.- 537-550: The addition of
test_msg_withdraw_delegator_reward_deprecation_warningextends the test coverage to include deprecation warnings for theMsgWithdrawDelegatorRewardmethod. Similar to the previous test, ensure that the new methodmsg_withdraw_delegator_rewardis available and documented for users.pyinjective/denoms_mainnet.ini (9)
- 685-692: The configuration for
Mainnet Spot DOJO/INJhas been added correctly, following the established pattern for such configurations. Ensure that the base and quote values align with the expected precision for these tokens.- 712-719: The configuration for
Mainnet Spot ANDR/INJintroduces a significantmin_price_tick_sizeof10000000and amin_display_price_tick_sizeof0.00001, which seems unusually high compared to other configurations. Verify that these values are intended and consistent with the trading strategy for this pair.- 721-728: The configuration for
Mainnet Spot hINJ/INJis consistent with the pattern for spot pairs. Themin_price_tick_sizeandmin_display_price_tick_sizeare set to0.0001, which is standard for pairs involving INJ.- 739-746: The configuration for
Mainnet Spot PHUC/INJuses amin_price_tick_sizeof10000and amin_display_price_tick_sizeof0.00000001, which is consistent with other configurations involving INJ. Ensure themin_quantity_tick_sizeandmin_display_quantity_tick_sizeare appropriate for the expected trading volume.- 757-764: The configuration for
Mainnet Spot WHALE/INJintroduces amin_price_tick_sizeandmin_display_price_tick_sizethat are consistent with high-value tokens. This configuration should be verified for accuracy given the unique nature of the WHALE token.- 766-773: The configuration for
Mainnet Spot HDRO/INJfollows the established pattern. Given the novelty of the HDRO token, ensure that the tick sizes and quantities are aligned with the market expectations for this token.- 775-782: The configuration for
Mainnet Spot PYTH/INJhas a highermin_price_tick_sizeandmin_display_price_tick_sizecompared to other INJ pairs. Confirm that these values are intended to reflect the trading characteristics of the PYTH token.- 982-989: The configuration for
Mainnet Derivative W/USDT PERPis correctly added, following the pattern for derivative pairs. Ensure that themin_price_tick_sizeandmin_display_price_tick_sizeare appropriate for the volatility and trading strategy of this derivative.- 1000-1007: The configuration for
Mainnet Derivative DOGE/USDT PERPuses amin_price_tick_sizeandmin_display_price_tick_sizethat are consistent with the expected volatility of DOGE. However, themin_quantity_tick_sizeandmin_display_quantity_tick_sizeare set to10, which might be high for a derivative. Confirm these values are correct.
Comment on lines
+5
to
+7
| ## [1.5.0] - 9999-99-99 | ||
| ### Added | ||
| - Added support for all queries in the chain 'tendermint' module |
Contributor
There was a problem hiding this comment.
The entry for version 1.5.0 uses a placeholder date 9999-99-99. It's important to replace this with the actual release date before the changelog is finalized.
- ## [1.5.0] - 9999-99-99
+ ## [1.5.0] - <actual release date>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.
Suggested change
| ## [1.5.0] - 9999-99-99 | |
| ### Added | |
| - Added support for all queries in the chain 'tendermint' module | |
| ## [1.5.0] - <actual release date> | |
| ### Added | |
| - Added support for all queries in the chain 'tendermint' module |
Comment on lines
+856
to
+863
| [0x3b7fb1d9351f7fa2e6e0e5a11b3639ee5e0486c33a6a74f629c3fc3c3043efd5] | ||
| description = 'Mainnet Derivative BONK/USDT PERP' | ||
| base = 0 | ||
| quote = 6 | ||
| min_price_tick_size = 0.01 | ||
| min_display_price_tick_size = 0.00000001 | ||
| min_quantity_tick_size = 10000 | ||
| min_display_quantity_tick_size = 10000 |
Contributor
There was a problem hiding this comment.
The configuration for Mainnet Derivative BONK/USDT PERP introduces a min_price_tick_size of 0.01 and a min_display_price_tick_size of 0.00000001, which seems to be an error given the mismatch in scale. The min_quantity_tick_size and min_display_quantity_tick_size also appear to be unusually high. This configuration should be reviewed for potential errors.
- min_display_price_tick_size = 0.00000001
+ min_display_price_tick_size = 0.01
- min_display_quantity_tick_size = 10000
+ min_display_quantity_tick_size = 0.01Committable 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.
Suggested change
| [0x3b7fb1d9351f7fa2e6e0e5a11b3639ee5e0486c33a6a74f629c3fc3c3043efd5] | |
| description = 'Mainnet Derivative BONK/USDT PERP' | |
| base = 0 | |
| quote = 6 | |
| min_price_tick_size = 0.01 | |
| min_display_price_tick_size = 0.00000001 | |
| min_quantity_tick_size = 10000 | |
| min_display_quantity_tick_size = 10000 | |
| [0x3b7fb1d9351f7fa2e6e0e5a11b3639ee5e0486c33a6a74f629c3fc3c3043efd5] | |
| description = 'Mainnet Derivative BONK/USDT PERP' | |
| base = 0 | |
| quote = 6 | |
| min_price_tick_size = 0.01 | |
| min_display_price_tick_size = 0.01 | |
| min_quantity_tick_size = 10000 | |
| min_display_quantity_tick_size = 0.01 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
pyproject.tomlfrom "1.4.0-pre" to "1.5.0-pre".