Conversation
… message. Added unit tests and script examples
WalkthroughThis update significantly enhances the capabilities of the platform by integrating comprehensive support for IBC Transfer module queries, alongside the existing support for the 'tendermint' module. It streamlines the cookies management logic for gRPC calls and introduces a suite of functionalities for handling token transfers, querying denomination traces, hashes, and escrow information within the Injective Protocol environment. Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (13)
Additional comments not posted (17)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 8
Actionable comments outside the diff hunks (8)
CHANGELOG.md (2)
Line range hint
52-52: Use hyphenated compound adjectives for clarity.In the changelog entry for version 1.0.1, the phrase "low level API components" could be clearer if hyphenated to indicate that "low-level" is a compound adjective modifying "API components".
- - Added low level API components for all modules + - Added low-level API components for all modules
Line range hint
68-68: Address potential missing preposition in the changelog entry.The entry for version 1.0 about removing support for
sentrynodes might be missing a preposition that clarifies the context. Consider rephrasing for better readability.- - Remove support for `sentry` nodes in mainnet network. + - Removed support for `sentry` nodes from the mainnet network.tests/test_composer.py (6)
Line range hint
24-24: Remove duplicate import ofinj_usdt_spot_marketto improve code clarity.
Line range hint
24-24: Remove duplicate import ofbtc_usdt_perp_marketto improve code clarity.
Line range hint
24-24: Remove duplicate import offirst_match_bet_marketto improve code clarity.
Line range hint
42-42: Remove duplicate import ofinj_tokento improve code clarity.
Line range hint
1191-1191: Ensure that the redefinition oftest_msg_cancel_derivative_orderis intentional. If both versions are needed, consider renaming one to avoid overshadowing.
Line range hint
1355-1355: Ensure that the redefinition oftest_msg_external_transferis intentional. Consider renaming one of the methods to ensure both are executed and serve their intended purpose.
| from pyinjective.proto.ibc.applications.transfer.v1 import tx_pb2 as ibc_transfer_tx_pb | ||
| from pyinjective.proto.ibc.core.client.v1 import client_pb2 as ibc_core_client_pb |
There was a problem hiding this comment.
Consider grouping imports from the same module.
- from pyinjective.proto.ibc.applications.transfer.v1 import tx_pb2 as ibc_transfer_tx_pb
- from pyinjective.proto.ibc.core.client.v1 import client_pb2 as ibc_core_client_pb
+ from pyinjective.proto.ibc.applications.transfer.v1 import tx_pb2 as ibc_transfer_tx_pb
+ from pyinjective.proto.ibc.core.client.v1 import client_pb2 as ibc_core_client_pbCommittable 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.
| from pyinjective.proto.ibc.applications.transfer.v1 import tx_pb2 as ibc_transfer_tx_pb | |
| from pyinjective.proto.ibc.core.client.v1 import client_pb2 as ibc_core_client_pb | |
| from pyinjective.proto.ibc.applications.transfer.v1 import tx_pb2 as ibc_transfer_tx_pb | |
| from pyinjective.proto.ibc.core.client.v1 import client_pb2 as ibc_core_client_pb |
| return base_coin_pb.Coin(amount=str(amount), denom=denom) | ||
|
|
||
| def coin(self, amount: int, denom: str): | ||
| def coin(self, amount: int, denom: str) -> base_coin_pb.Coin: |
There was a problem hiding this comment.
Ensure return type annotation consistency.
The coin method lacks a return type annotation, which is present in other newly added methods like create_coin_amount. For consistency and clarity, consider adding a return type annotation.
- def coin(self, amount: int, denom: str):
+ def coin(self, amount: int, denom: str) -> base_coin_pb.Coin: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.
| def coin(self, amount: int, denom: str) -> base_coin_pb.Coin: | |
| def coin(self, amount: int, denom: str) -> base_coin_pb.Coin: |
| def msg_ibc_transfer( | ||
| self, | ||
| source_port: str, | ||
| source_channel: str, | ||
| token_amount: base_coin_pb.Coin, | ||
| sender: str, | ||
| receiver: str, | ||
| timeout_height: Optional[int] = None, | ||
| timeout_timestamp: Optional[int] = None, | ||
| memo: Optional[str] = None, | ||
| ) -> ibc_transfer_tx_pb.MsgTransfer: | ||
| if timeout_height is None and timeout_timestamp is None: | ||
| raise ValueError("IBC Transfer error: Either timeout_height or timeout_timestamp must be provided") | ||
| parsed_timeout_height = None | ||
| if timeout_height: | ||
| parsed_timeout_height = ibc_core_client_pb.Height( | ||
| revision_number=timeout_height, revision_height=timeout_height | ||
| ) | ||
| return ibc_transfer_tx_pb.MsgTransfer( | ||
| source_port=source_port, | ||
| source_channel=source_channel, | ||
| token=token_amount, | ||
| sender=sender, | ||
| receiver=receiver, | ||
| timeout_height=parsed_timeout_height, | ||
| timeout_timestamp=timeout_timestamp, | ||
| memo=memo, | ||
| ) |
There was a problem hiding this comment.
Validate timeout_height and timeout_timestamp parameters in msg_ibc_transfer.
The method msg_ibc_transfer checks if both timeout_height and timeout_timestamp are None but does not validate the scenario where both are provided, which might not be intended or supported by the underlying IBC protocol. Consider adding a validation to ensure only one of them is provided, or clearly document the expected behavior if both are provided.
+ if timeout_height is not None and timeout_timestamp is not None:
+ raise ValueError("IBC Transfer error: Only one of timeout_height or timeout_timestamp should be provided")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.
| def msg_ibc_transfer( | |
| self, | |
| source_port: str, | |
| source_channel: str, | |
| token_amount: base_coin_pb.Coin, | |
| sender: str, | |
| receiver: str, | |
| timeout_height: Optional[int] = None, | |
| timeout_timestamp: Optional[int] = None, | |
| memo: Optional[str] = None, | |
| ) -> ibc_transfer_tx_pb.MsgTransfer: | |
| if timeout_height is None and timeout_timestamp is None: | |
| raise ValueError("IBC Transfer error: Either timeout_height or timeout_timestamp must be provided") | |
| parsed_timeout_height = None | |
| if timeout_height: | |
| parsed_timeout_height = ibc_core_client_pb.Height( | |
| revision_number=timeout_height, revision_height=timeout_height | |
| ) | |
| return ibc_transfer_tx_pb.MsgTransfer( | |
| source_port=source_port, | |
| source_channel=source_channel, | |
| token=token_amount, | |
| sender=sender, | |
| receiver=receiver, | |
| timeout_height=parsed_timeout_height, | |
| timeout_timestamp=timeout_timestamp, | |
| memo=memo, | |
| ) | |
| def msg_ibc_transfer( | |
| self, | |
| source_port: str, | |
| source_channel: str, | |
| token_amount: base_coin_pb.Coin, | |
| sender: str, | |
| receiver: str, | |
| timeout_height: Optional[int] = None, | |
| timeout_timestamp: Optional[int] = None, | |
| memo: Optional[str] = None, | |
| ) -> ibc_transfer_tx_pb.MsgTransfer: | |
| if timeout_height is None and timeout_timestamp is None: | |
| raise ValueError("IBC Transfer error: Either timeout_height or timeout_timestamp must be provided") | |
| if timeout_height is not None and timeout_timestamp is not None: | |
| raise ValueError("IBC Transfer error: Only one of timeout_height or timeout_timestamp should be provided") | |
| parsed_timeout_height = None | |
| if timeout_height: | |
| parsed_timeout_height = ibc_core_client_pb.Height( | |
| revision_number=timeout_height, revision_height=timeout_height | |
| ) | |
| return ibc_transfer_tx_pb.MsgTransfer( | |
| source_port=source_port, | |
| source_channel=source_channel, | |
| token=token_amount, | |
| sender=sender, | |
| receiver=receiver, | |
| timeout_height=parsed_timeout_height, | |
| timeout_timestamp=timeout_timestamp, | |
| memo=memo, | |
| ) |
| async def test_fetch_params( | ||
| self, | ||
| ibc_transfer_servicer, | ||
| ): | ||
| params = ibc_transfer.Params( | ||
| send_enabled=True, | ||
| receive_enabled=False, | ||
| ) | ||
|
|
||
| ibc_transfer_servicer.params_responses.append( | ||
| ibc_transfer_query.QueryParamsResponse( | ||
| params=params, | ||
| ) | ||
| ) | ||
|
|
||
| api = self._api_instance(servicer=ibc_transfer_servicer) | ||
|
|
||
| result_params = await api.fetch_params() | ||
| expected_params = {"params": {"sendEnabled": params.send_enabled, "receiveEnabled": params.receive_enabled}} | ||
|
|
||
| assert result_params == expected_params |
There was a problem hiding this comment.
Ensure consistent use of single quotes for strings.
Throughout the file, there's an inconsistent use of single and double quotes for strings. It's a good practice to stick to one style for consistency. The Python community often prefers single quotes unless the string contains a single quote character.
- expected_params = {"params": {"sendEnabled": params.send_enabled, "receiveEnabled": params.receive_enabled}}
+ expected_params = {'params': {'sendEnabled': params.send_enabled, 'receiveEnabled': params.receive_enabled}}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.
| async def test_fetch_params( | |
| self, | |
| ibc_transfer_servicer, | |
| ): | |
| params = ibc_transfer.Params( | |
| send_enabled=True, | |
| receive_enabled=False, | |
| ) | |
| ibc_transfer_servicer.params_responses.append( | |
| ibc_transfer_query.QueryParamsResponse( | |
| params=params, | |
| ) | |
| ) | |
| api = self._api_instance(servicer=ibc_transfer_servicer) | |
| result_params = await api.fetch_params() | |
| expected_params = {"params": {"sendEnabled": params.send_enabled, "receiveEnabled": params.receive_enabled}} | |
| assert result_params == expected_params | |
| async def test_fetch_params( | |
| self, | |
| ibc_transfer_servicer, | |
| ): | |
| params = ibc_transfer.Params( | |
| send_enabled=True, | |
| receive_enabled=False, | |
| ) | |
| ibc_transfer_servicer.params_responses.append( | |
| ibc_transfer_query.QueryParamsResponse( | |
| params=params, | |
| ) | |
| ) | |
| api = self._api_instance(servicer=ibc_transfer_servicer) | |
| result_params = await api.fetch_params() | |
| expected_params = {'params': {'sendEnabled': params.send_enabled, 'receiveEnabled': params.receive_enabled}} | |
| assert result_params == expected_params |
| @pytest.mark.asyncio | ||
| async def test_fetch_denom_hash( | ||
| self, | ||
| ibc_transfer_servicer, | ||
| ): | ||
| denom_hash = "97498452BF27CC90656FD7D6EFDA287FA2BFFFF3E84691C84CB9E0451F6DF0A4" | ||
|
|
||
| ibc_transfer_servicer.denom_hash_responses.append( | ||
| ibc_transfer_query.QueryDenomHashResponse( | ||
| hash=denom_hash, | ||
| ) | ||
| ) | ||
|
|
||
| api = self._api_instance(servicer=ibc_transfer_servicer) | ||
|
|
||
| result_hash = await api.fetch_denom_hash(trace="transfer/channel-126/uluna") | ||
| expected_hash = {"hash": denom_hash} | ||
|
|
||
| assert result_hash == expected_hash |
There was a problem hiding this comment.
Consider adding more test cases for test_fetch_denom_hash.
The current test case for test_fetch_denom_hash covers a basic scenario. It might be beneficial to add more test cases covering edge cases, such as invalid hash formats or empty strings, to ensure robust error handling and validation.
Would you like me to suggest some additional test cases?
| @pytest.mark.asyncio | ||
| async def test_fetch_total_escrow_for_denom( | ||
| self, | ||
| ibc_transfer_servicer, | ||
| ): | ||
| coin = coin_pb.Coin(denom="inj", amount="988987297011197594664") | ||
|
|
||
| ibc_transfer_servicer.total_escrow_for_denom_responses.append( | ||
| ibc_transfer_query.QueryTotalEscrowForDenomResponse( | ||
| amount=coin, | ||
| ) | ||
| ) | ||
|
|
||
| api = self._api_instance(servicer=ibc_transfer_servicer) | ||
|
|
||
| result_escrow = await api.fetch_total_escrow_for_denom(denom="inj") | ||
| expected_escrow = { | ||
| "amount": { | ||
| "denom": coin.denom, | ||
| "amount": coin.amount, | ||
| } | ||
| } | ||
|
|
||
| assert result_escrow == expected_escrow |
There was a problem hiding this comment.
Enhance test coverage for edge cases in test_fetch_total_escrow_for_denom.
The test test_fetch_total_escrow_for_denom focuses on a successful scenario. Consider adding tests for edge cases, such as when the denom does not exist or the API returns an error. This would ensure the application gracefully handles such situations.
Would you like assistance in drafting these additional test scenarios?
| ## [1.5.0] - 9999-99-99 | ||
| ### Added | ||
| - Added support for all queries in the chain 'tendermint' module | ||
| - Added support for all queries in the IBC Transfer module | ||
|
|
||
| ### Changed | ||
| - Refactored cookies management logic to use all gRPC calls' responses to update the current cookies |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-7]
Clarify the release date for version 1.5.0.
The release date for version 1.5.0 is marked as 9999-99-99, which seems to be a placeholder. Please update this with the actual release date to maintain an accurate and informative changelog.
| ## [1.5.0] - 9999-99-99 | ||
| ### Added | ||
| - Added support for all queries in the chain 'tendermint' module | ||
| - Added support for all queries in the IBC Transfer module |
There was a problem hiding this comment.
Remove duplicate word in the changelog entry.
There's a minor typographical issue with the repeated word "Added" in the entry for the IBC Transfer module support. Consider revising for clarity and conciseness.
- - Added support for all queries in the IBC Transfer module
+ - Support for all queries in the IBC Transfer moduleCommittable 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.
| - Added support for all queries in the IBC Transfer module | |
| - Support for all queries in the IBC Transfer module |
Solves CHAIN-74
Summary by CodeRabbit
New Features
Refactor
Tests