Release/v1.8.0 #358
Release/v1.8.0 #358
Conversation
…v1_6_1 feat/sync_dec_with_master_v1_6_1
Implement OFAC list address check
…o fix/sync_dev_after_v170
fix/sync_dev_after_v170
…iveLabs/sdk-python into fix/sync_dev_after_v1_7_1
fix/sync_dev_after_v1_7_1
…ent to get markets from chain instead of using Indexer endpoints. Removed deprecated support for denom INI files
…into feat/sync_dev_with_v1_7_2
Feat/sync dev with v1.7.2
…o feat/refactor_markets_tokens_initialization
…ns_initialization Feat/refactor markets tokens initialization
WalkthroughThis pull request introduces significant changes to the market initialization and management within the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (21)
pyinjective/core/token.py (1)
17-24: Add docstrings to the new methodsThe methods are well-implemented and mathematically sound. The use of
Decimaltype ensures precision in financial calculations.Consider adding docstrings to clarify the purpose and usage:
@staticmethod def convert_value_to_extended_decimal_format(value: Decimal) -> Decimal: + """ + Converts a decimal value to an extended chain format by multiplying with a power of 10. + + Args: + value (Decimal): The original decimal value + + Returns: + Decimal: The value in extended chain format + """ return value * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") @staticmethod def convert_value_from_extended_decimal_format(value: Decimal) -> Decimal: + """ + Converts a decimal value from extended chain format back to original format. + + Args: + value (Decimal): The value in extended chain format + + Returns: + Decimal: The original decimal value + """ return value / Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}")tests/test_orderhash.py (1)
20-34: Consider adding validation for market fixtures.While the fixture is well-structured, consider adding validation to ensure the market fixtures are properly initialized before using them. This would prevent potential null reference issues if the fixture dependencies fail.
@pytest.fixture def basic_composer(self, inj_usdt_spot_market, btc_usdt_perp_market, first_match_bet_market): + # Validate market fixtures + if not all([inj_usdt_spot_market, btc_usdt_perp_market, first_match_bet_market]): + pytest.fail("Required market fixtures not properly initialized") + composer = Composer( network=Network.devnet().string(), spot_markets={inj_usdt_spot_market.id: inj_usdt_spot_market},🧰 Tools
🪛 Ruff
21-21: Redefinition of unused
inj_usdt_spot_marketfrom line 13(F811)
21-21: Redefinition of unused
btc_usdt_perp_marketfrom line 10(F811)
21-21: Redefinition of unused
first_match_bet_marketfrom line 11(F811)
pyinjective/orderhash.py (1)
99-101: Consider providing migration guidanceGiven that this is a breaking change in value handling, consider:
- Adding deprecation warnings for the old integer-based approach
- Providing migration documentation for users
- Including examples of correct value formatting in docstrings
tests/core/test_message_based_transaction_fee_calculator.py (2)
29-43: Add docstring to document fixture's purpose and parametersThe fixture implementation looks good, but would benefit from documentation.
Consider adding a docstring:
@pytest.fixture def basic_composer(self, inj_usdt_spot_market, btc_usdt_perp_market, first_match_bet_market): + """Create a Composer instance with predefined market configurations. + + Args: + inj_usdt_spot_market: Spot market fixture for INJ/USDT + btc_usdt_perp_market: Perpetual market fixture for BTC/USDT + first_match_bet_market: Binary option market fixture + + Returns: + Composer: Configured composer instance with markets and tokens + """ composer = Composer(🧰 Tools
🪛 Ruff
30-30: Redefinition of unused
inj_usdt_spot_marketfrom line 23(F811)
30-30: Redefinition of unused
btc_usdt_perp_marketfrom line 20(F811)
30-30: Redefinition of unused
first_match_bet_marketfrom line 21(F811)
149-149: Consider moving market_id extraction to the fixtureThe pattern
market_id = list(basic_composer.spot_markets.keys())[0]is repeated across multiple tests. Consider moving this to the fixture to improve maintainability.Update the fixture to return both the composer and commonly used market IDs:
@pytest.fixture def basic_composer(self, inj_usdt_spot_market, btc_usdt_perp_market, first_match_bet_market): composer = Composer(...) - return composer + return { + 'composer': composer, + 'spot_market_id': list(composer.spot_markets.keys())[0], + 'perp_market_id': list(composer.derivative_markets.keys())[0], + 'binary_market_id': list(composer.binary_option_markets.keys())[0] + }Then update the tests to use these values directly:
market_id = basic_composer['spot_market_id']Also applies to: 180-180, 216-216
CHANGELOG.md (1)
9-10: Remove duplicate word "Removed"The word "Removed" appears twice in the sentence, which affects readability.
Apply this diff to fix the duplication:
-### Removed -- Removed the legacy deprecated markets and tokens initialization using the denoms INI files in the SDK. Removed also the INI files from the SDK +### Removed +- Legacy deprecated markets and tokens initialization using the denoms INI files in the SDK. Removed also the INI files from the SDK🧰 Tools
🪛 LanguageTool
[duplication] ~9-~9: Possible typo: you repeated a word
Context: ...s instead of the Indexer endpoints ### Removed - Removed the legacy deprecated markets and token...(ENGLISH_WORD_REPEAT_RULE)
tests/test_async_client.py (2)
151-158: Consider adding negative test casesWhile the happy path is well tested, consider adding test cases for:
- Invalid market responses
- Empty market responses
- Malformed market data
Would you like me to help generate these additional test cases?
Also applies to: 165-165, 188-190
Line range hint
1-304: Consider adding integration testsWhile the unit tests are comprehensive, consider adding integration tests to verify:
- Interaction between AsyncClient and actual network endpoints
- Error handling for network failures
- Rate limiting scenarios
This would help ensure the refactored initialization logic works correctly in production scenarios.
🧰 Tools
🪛 Ruff
77-77: Redefinition of unused
inj_usdt_spot_market_metafrom line 18(F811)
78-78: Redefinition of unused
ape_usdt_spot_market_metafrom line 14(F811)
79-79: Redefinition of unused
btc_usdt_perp_market_metafrom line 15(F811)
80-80: Redefinition of unused
first_match_bet_market_metafrom line 16(F811)
170-170: Yoda condition detected
Rewrite as
len(all_tokens) == 5(SIM300)
tests/test_composer_deprecation_warnings.py (3)
Line range hint
76-84: Consider parameterizing similar test cases.The spot and derivative order tests follow very similar patterns. Consider using pytest's parameterize feature to reduce code duplication and make it easier to add new test cases.
Example refactor:
@pytest.mark.parametrize( "deprecated_method,new_method,market_type", [ ("SpotOrder", "spot_order", "spot_markets"), ("DerivativeOrder", "derivative_order", "derivative_markets"), ], ) def test_order_deprecation_warnings(self, basic_composer, deprecated_method, new_method, market_type): market_id = list(getattr(basic_composer, market_type).keys())[0] with warnings.catch_warnings(record=True) as all_warnings: getattr(basic_composer, deprecated_method)( market_id=market_id, subaccount_id="subaccount id", fee_recipient="fee recipient", price=1, quantity=1, is_buy=True, cid="cid", ) deprecation_warnings = [w for w in all_warnings if issubclass(w.category, DeprecationWarning)] assert len(deprecation_warnings) == 1 assert str(deprecation_warnings[0].message) == f"This method is deprecated. Use {new_method} instead"
Line range hint
237-251: Consider adding edge cases for derivative order tests.While the basic functionality is well-tested, consider adding test cases for:
- Invalid leverage values
- Market orders with different order types (SELL/BUY)
- Boundary conditions for price and quantity
Also applies to: 258-273, 283-297
475-485: Consider validating amount parameter in position margin test.While the test verifies the deprecation warning, it might be valuable to add assertions for invalid amount values (e.g., negative or zero) to ensure proper validation in the new implementation.
Example additional test:
def test_msg_increase_position_margin_validation(self, basic_composer): market_id = list(basic_composer.derivative_markets.keys())[0] with pytest.raises(ValueError, match="Amount must be positive"): basic_composer.msg_increase_position_margin( sender="sender", source_subaccount_id="source_subaccount id", destination_subaccount_id="destination_subaccount id", market_id=market_id, amount=-1, )tests/test_composer.py (4)
Line range hint
1-17: Add a module docstring to describe the test suite's purpose.Consider adding a descriptive docstring at the module level to explain the purpose and scope of these tests.
+""" +Unit tests for the Composer class, which handles message composition for various +blockchain operations including spot markets, derivative markets, and binary options. +""" import json from decimal import Decimal
20-20: Add a class docstring to TestComposer.Add a docstring to describe the test class's purpose and the types of tests it contains.
class TestComposer: + """ + Test suite for the Composer class, verifying message composition functionality + for various blockchain operations including market orders, transfers, and + administrative actions. + """
Line range hint
32-38: Add docstrings to test methods.Consider adding docstrings to test methods to explain the test scenario, inputs, and expected outcomes. For example:
def test_msg_create_denom(self, basic_composer: Composer): + """ + Test creation of a denom message with valid parameters. + + Verifies that the message is correctly composed with the specified sender, + subdenom, name, symbol, and decimals. + """ sender = "inj1apmvarl2xyv6kecx2ukkeymddw3we4zkygjyc0"
Line range hint
32-65: Consider adding edge case tests.The test coverage is good, but consider adding tests for edge cases such as:
- Invalid/boundary values for decimals
- Empty or special characters in subdenom/name/symbol
- Maximum length values for strings
Example additional test:
def test_msg_create_denom_edge_cases(self, basic_composer: Composer): """Test denom creation with edge case inputs.""" # Test with maximum allowed decimals with pytest.raises(ValueError, match="Decimals must be <= 18"): basic_composer.msg_create_denom( sender="inj1apmvarl2xyv6kecx2ukkeymddw3we4zkygjyc0", subdenom="test", name="Test", symbol="TEST", decimals=19 ) # Test with special characters in subdenom with pytest.raises(ValueError, match="Invalid subdenom"): basic_composer.msg_create_denom( sender="inj1apmvarl2xyv6kecx2ukkeymddw3we4zkygjyc0", subdenom="test@#$", name="Test", symbol="TEST", decimals=18 )pyinjective/composer.py (1)
Line range hint
1-1441: Consider additional improvements for maintainabilityWhile the code is well-structured, consider these enhancements:
- Extract common validation logic into utility functions
- Add detailed docstrings to new methods matching the style of deprecated ones
- Ensure consistent type hints across all methods
Example pattern for docstrings in new methods:
def msg_deposit(self, sender: str, subaccount_id: str, amount: Decimal, denom: str): """Create a deposit message for the exchange module. Args: sender (str): The address of the message sender subaccount_id (str): The subaccount ID for the deposit amount (Decimal): The amount to deposit in human readable format denom (str): The denomination of the token to deposit Returns: MsgDeposit: The composed deposit message """tests/rpc_fixtures/markets_fixtures.py (5)
122-122: Use Enum forstatusFieldThe
statusfield is set to the integer1. For better readability and maintainability, consider using the appropriate enum fromexchange_pb.MarketStatus, such asexchange_pb.MarketStatus.MARKET_STATUS_ACTIVE.Apply this change:
- status=1, + status=exchange_pb.MarketStatus.MARKET_STATUS_ACTIVE,
145-145: Use Enum forstatusFieldSimilar to the previous fixture, replace the integer
1with the appropriate enum value for thestatusfield.Apply this change:
- status=1, + status=exchange_pb.MarketStatus.MARKET_STATUS_ACTIVE,
158-158: Update Imports for ConsistencyIn the
btc_usdt_perp_market_metafixture, ensure that the imports are correctly aligned with the proto definitions used.Consider organizing imports for clarity:
-from pyinjective.proto.injective.exchange.v1beta1 import exchange_pb2 as exchange_pb, query_pb2 as exchange_query_pb +from pyinjective.proto.injective.exchange.v1beta1 import ( + exchange_pb2 as exchange_pb, + query_pb2 as exchange_query_pb, +)
164-164: Use Enum fororacle_typeFieldThe
oracle_typeis set to the integer10. Replace it with the corresponding enum value from the oracle types to enhance readability.Apply this change:
- oracle_type=10, + oracle_type=exchange_pb.OracleType.ORACLE_TYPE_BANDIBC,
230-230: Use Enum forstatusFieldReplace the integer
1with the appropriate enum value for thestatusfield in the binary options market.Apply this change:
- status=1, + status=exchange_pb.MarketStatus.MARKET_STATUS_ACTIVE,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
CHANGELOG.md(1 hunks)pyinjective/async_client.py(3 hunks)pyinjective/composer.py(1 hunks)pyinjective/constant.py(0 hunks)pyinjective/core/token.py(2 hunks)pyinjective/denoms_devnet.ini(0 hunks)pyinjective/orderhash.py(2 hunks)pyinjective/utils/denom.py(0 hunks)pyinjective/utils/fetch_metadata.py(0 hunks)pyinjective/utils/metadata_validation.py(0 hunks)pyproject.toml(1 hunks)tests/core/test_gas_limit_estimator.py(14 hunks)tests/core/test_message_based_transaction_fee_calculator.py(4 hunks)tests/rpc_fixtures/markets_fixtures.py(1 hunks)tests/test_async_client.py(8 hunks)tests/test_composer.py(1 hunks)tests/test_composer_deprecation_warnings.py(14 hunks)tests/test_orderhash.py(3 hunks)
💤 Files with no reviewable changes (5)
- pyinjective/constant.py
- pyinjective/denoms_devnet.ini
- pyinjective/utils/denom.py
- pyinjective/utils/fetch_metadata.py
- pyinjective/utils/metadata_validation.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~9-~9: Possible typo: you repeated a word
Context: ...s instead of the Indexer endpoints ### Removed - Removed the legacy deprecated markets and token...
(ENGLISH_WORD_REPEAT_RULE)
🪛 Ruff
tests/core/test_gas_limit_estimator.py
34-34: Redefinition of unused inj_usdt_spot_market from line 27
(F811)
34-34: Redefinition of unused btc_usdt_perp_market from line 24
(F811)
34-34: Redefinition of unused first_match_bet_market from line 25
(F811)
tests/core/test_message_based_transaction_fee_calculator.py
30-30: Redefinition of unused inj_usdt_spot_market from line 23
(F811)
30-30: Redefinition of unused btc_usdt_perp_market from line 20
(F811)
30-30: Redefinition of unused first_match_bet_market from line 21
(F811)
tests/test_async_client.py
77-77: Redefinition of unused inj_usdt_spot_market_meta from line 18
(F811)
78-78: Redefinition of unused ape_usdt_spot_market_meta from line 14
(F811)
79-79: Redefinition of unused btc_usdt_perp_market_meta from line 15
(F811)
80-80: Redefinition of unused first_match_bet_market_meta from line 16
(F811)
170-170: Yoda condition detected
Rewrite as len(all_tokens) == 5
(SIM300)
tests/test_orderhash.py
21-21: Redefinition of unused inj_usdt_spot_market from line 13
(F811)
21-21: Redefinition of unused btc_usdt_perp_market from line 10
(F811)
21-21: Redefinition of unused first_match_bet_market from line 11
(F811)
🔇 Additional comments (24)
pyinjective/core/token.py (2)
25-26: Clarify the relationship between formatting methods
The class now has two different approaches for decimal formatting:
- Instance method
chain_formatted_valueusingself.decimals - Static methods using
ADDITIONAL_CHAIN_FORMAT_DECIMALS
Let's verify their distinct use cases and ensure they're not redundant.
#!/bin/bash
# Description: Check the usage patterns of both formatting approaches
# Expected: Different contexts for each method's usage
# Check chain_formatted_value usage
echo "Checking chain_formatted_value usage:"
rg -p "chain_formatted_value" --type python -A 2 -B 2
# Check new static methods usage
echo -e "\nChecking static methods usage:"
rg -p "convert_value_(to|from)_extended_decimal_format" --type python -A 2 -B 24-4: Verify the ADDITIONAL_CHAIN_FORMAT_DECIMALS constant value
Let's verify the constant's value and its usage across the codebase to ensure consistency.
✅ Verification successful
ADDITIONAL_CHAIN_FORMAT_DECIMALS constant is correctly defined and used
The constant is properly defined in pyinjective/constant.py with a value of 18 and is consistently used across the codebase for chain format decimal conversions. The import in token.py is valid and aligns with its usage in the token formatting methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the constant definition and its usage
# Expected: The constant should be consistently used for chain format conversions
# Check constant definition
echo "Checking constant definition:"
rg -p "ADDITIONAL_CHAIN_FORMAT_DECIMALS\s*=\s*" --type python
# Check all usages
echo -e "\nChecking all usages:"
rg "ADDITIONAL_CHAIN_FORMAT_DECIMALS" --type python
Length of output: 359
Script:
#!/bin/bash
# Let's try without the --type python flag since it's not recognized
# First, check the constant definition
echo "Checking constant definition:"
rg "ADDITIONAL_CHAIN_FORMAT_DECIMALS\s*=\s*"
# Then check its usage
echo -e "\nChecking all usages:"
rg "ADDITIONAL_CHAIN_FORMAT_DECIMALS"
# Also check the files where it's imported
echo -e "\nChecking import statements:"
rg "from.*ADDITIONAL_CHAIN_FORMAT_DECIMALS"
Length of output: 14642
tests/test_orderhash.py (2)
9-16: LGTM! Well-organized fixture imports.
The market fixtures are properly imported and will be used by the test cases. The F811 warnings can be safely ignored as they are false positives - this is standard pytest fixture behavior.
Line range hint 36-77: Enhance test coverage and documentation.
The test could benefit from:
- Documentation explaining how the expected hash values were derived
- Additional test cases covering edge cases
- Consider parameterizing the test to make it more maintainable
def test_spot_order_hash(self, requests_mock, basic_composer):
+ """
+ Test spot order hash computation.
+ Expected hashes were generated using reference implementation at:
+ <add reference implementation link>
+ """Also, verify that the hardcoded hash values are still valid after the market initialization refactor.
✅ Verification successful
Let me gather more information about the test implementation and hash computation to make a more informed decision.
Let me check the implementation of order hash computation to understand how these hashes are generated.
Let me check the test file and its dependencies to understand how the order hashes are computed.
Let me check the implementation of the hash computation and the market fixtures to understand how the hashes are generated.
Based on the code analysis, I can now generate the final response.
The order hash values are valid and should not be changed
The test verifies the deterministic computation of order hashes which are critical for order matching and execution. The hashes are generated using:
- EIP712 message structure with fixed parameters (name, version, chainId, etc.)
- Standardized order parameters conversion (using
param_to_backend_go) - Deterministic nonce management through
OrderHashManager
The market initialization refactor does not affect the hash computation because:
- The market fixture (
inj_usdt_spot_market) remains unchanged - The order parameters used in the test are explicit and not derived from market configuration
- The hash computation depends on the order parameters, not the market configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other tests using these hash values to ensure consistency
rg -l "0x79f259a10d813f11c9582b94950b5a0922180c1b2785638d8aefa04e0c8ae4aa|0x96c2c4a5d38a20457e8f3319b9aac194ad03b2ce80d0acd718190487bf12e434"
Length of output: 167
Script:
#!/bin/bash
# Check the implementation of order hash computation
ast-grep --pattern 'def compute_order_hashes'
# Look for any documentation or comments about hash computation
rg -B2 -A2 "compute.*hash" --type py
# Check for any test fixtures or setup related to basic_composer
rg -l "basic_composer" --type py
# Look for any constants or configurations related to spot orders
rg "spot.*order" --type py
Length of output: 215671
Script:
#!/bin/bash
# Look for order hash computation implementation
ast-grep --pattern 'def compute_order_hashes'
# Check for any test fixtures related to basic_composer
rg -A 10 "def basic_composer" --type py
# Look for any test setup or configuration that might affect the hash computation
rg -A 5 "market_initialization" --type py
Length of output: 5843
Script:
#!/bin/bash
# Check the test file content
cat tests/test_orderhash.py
# Look for any market fixtures used in the test
rg -l "inj_usdt_spot_market" --type py
# Look for any order hash computation implementation
rg -A 10 "compute_order_hashes" --type py
Length of output: 7515
Script:
#!/bin/bash
# Check the orderhash implementation
cat pyinjective/orderhash.py
# Check the market fixtures
cat tests/model_fixtures/markets_fixtures.py
# Look for any EIP712 message building implementation
rg -A 10 "build_eip712_msg" --type py
Length of output: 10565
pyinjective/orderhash.py (2)
8-8: LGTM: Clean import addition
The Token import is properly placed and necessary for the new value conversion logic.
99-101:
Verify precision handling in the new conversion logic
The function has undergone significant changes:
- Parameter type changed from
inttostr - Now uses
Token.convert_value_from_extended_decimal_formatinstead of direct division - Returns normalized string with exactly 18 decimal places
This is a breaking change that requires careful verification:
Let's verify the conversion logic and its usage:
Please ensure:
- All callers have been updated to pass string values
- The precision matches the blockchain's requirements
- Test cases exist to validate the conversion accuracy
Consider adding a validation check for the input parameter:
def param_to_backend_go(param: str) -> str:
+ if not isinstance(param, (str, Decimal)):
+ raise TypeError(f"Parameter must be string or Decimal, got {type(param)}")
go_param = Token.convert_value_from_extended_decimal_format(value=Decimal(param))
return f"{go_param.normalize():.18f}"✅ Verification successful
Based on the shell script results, I can now generate the final response:
Function change is safe and maintains precision
The change to param_to_backend_go is well-designed and maintains precision:
- The function now accepts strings which are converted to Decimal for exact precision
- Uses
Token.convert_value_from_extended_decimal_formatwhich properly handles decimal scaling (divides by 1e18) - Returns normalized 18 decimal place string format which matches the blockchain's requirements
- The function is used consistently throughout the codebase for converting various decimal values
The change from int to str input is actually an improvement as it:
- Prevents potential integer overflow issues
- Allows for more precise decimal values to be passed
- Maintains compatibility with blockchain decimal requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the impact of the new conversion logic
# Check for any direct integer parameters being passed
rg -A 2 "param_to_backend_go\([0-9]+"
# Check for any existing calls that might break with the new str parameter
ast-grep --pattern 'param_to_backend_go($_)'
# Look for test cases validating the precision
rg -B 2 -A 2 "param_to_backend_go.*test"
Length of output: 786
Script:
#!/bin/bash
# Let's check the order_info class to understand the types of price, quantity, etc.
rg -B 2 -A 2 "class.*Order"
# Check the Token class and its conversion method
ast-grep --pattern 'convert_value_from_extended_decimal_format'
# Look for test files related to orderhash
fd test.*py --exec rg -l "orderhash|param_to_backend"
# Check if there are any direct integer usages in test files
rg "order.*price.*=.*[0-9]+"
rg "order.*quantity.*=.*[0-9]+"
Length of output: 312317
Script:
#!/bin/bash
# Let's check the Token class implementation and its usage
rg -B 2 -A 2 "class Token"
rg -B 2 -A 2 "convert_value_from_extended_decimal_format"
# Check test files for param_to_backend_go usage
fd test.*py --exec rg -B 2 -A 2 "param_to_backend_go"
# Check for any direct numeric values being passed to param_to_backend_go
rg "param_to_backend_go\(.*[0-9]+"
Length of output: 6424
tests/core/test_message_based_transaction_fee_calculator.py (2)
20-25: LGTM: Fixture imports are properly configured
The market fixture imports are correctly annotated with noqa: F401 as they are used as pytest fixtures.
Line range hint 140-244: LGTM: Gas limit calculations are correct
The gas limit calculations properly account for:
- Base transaction gas
- Message-specific gas limits
- Combined gas limits for multiple messages
tests/test_async_client.py (3)
9-11: LGTM: Clean consolidation of exchange servicers
The consolidation of separate spot and derivative servicers into a single exchange servicer improves maintainability while maintaining the same functionality.
Also applies to: 32-34
84-148: LGTM: Comprehensive token list with real-world data structure
The token list provides excellent test coverage with various token types (ERC20, native, tokenFactory) and complete metadata, making the tests more realistic and maintainable.
Line range hint 200-304: LGTM: Good test coverage for token initialization paths
The tests thoroughly cover both initialization paths:
- Tokens from official list
- Tokens from chain denoms
This ensures reliability of the token initialization process after the refactor.
tests/core/test_gas_limit_estimator.py (2)
33-47: Well-structured fixture implementation!
The basic_composer fixture effectively centralizes the test setup, reducing code duplication and improving maintainability. The initialization includes all necessary components (spot markets, derivative markets, binary option markets, and tokens) in a clean and organized manner.
🧰 Tools
🪛 Ruff
34-34: Redefinition of unused inj_usdt_spot_market from line 27
(F811)
34-34: Redefinition of unused btc_usdt_perp_market from line 24
(F811)
34-34: Redefinition of unused first_match_bet_market from line 25
(F811)
49-57: Test methods effectively refactored to use the basic_composer fixture!
The changes demonstrate:
- Consistent usage of the fixture across all test methods
- Dynamic market ID retrieval from the composer
- Clear and well-structured gas limit calculations and assertions
Also applies to: 58-85, 114-142, 172-203, 206-239, 462-491, 531-544
tests/test_composer_deprecation_warnings.py (3)
Line range hint 33-47: LGTM! Well-structured basic message tests.
The tests for MsgDeposit and MsgWithdraw follow a consistent pattern and effectively verify deprecation warnings.
Line range hint 130-142: Good use of the basic_composer fixture for order creation.
The test effectively demonstrates the transition from deprecated to new methods by using the new spot_order method to create the test order.
Line range hint 433-456: LGTM! Consistent transfer test implementation.
The transfer-related tests maintain consistency with other test patterns and effectively verify deprecation warnings.
tests/test_composer.py (1)
Line range hint 1-1161: Overall, the test suite is well-structured and comprehensive.
The test file demonstrates good practices with:
- Comprehensive test coverage of the Composer class
- Clear and consistent test patterns
- Good use of fixtures and assertions
- Thorough validation of message composition
The suggested improvements (docstrings and edge cases) would further enhance the already solid test suite.
pyinjective/composer.py (3)
138-141: Clean and efficient dictionary initialization!
The use of the or operator for default empty dictionaries is a clean and idiomatic Python approach. This change:
- Simplifies the initialization logic
- Removes external file dependencies
- Maintains backward compatibility
Line range hint 147-151: Well-structured deprecation implementation!
The deprecation of methods follows best practices:
- Uses
warnings.warn()with proper stacklevel - Provides clear migration paths to new methods
- Maintains backward compatibility
- Includes informative deprecation messages
Also applies to: 169-182, 217-257, 306-379, 380-433, 434-487
Line range hint 1-1441: Verify input validation for financial operations
Given the critical nature of blockchain transactions and financial operations, consider:
- Adding input range validation for financial values
- Implementing comprehensive error handling for edge cases
- Adding logging for debugging and monitoring
✅ Verification successful
Let me analyze the code further to verify the validation concerns.
Based on the analysis of the code, I can now provide a final response regarding the validation concerns.
Input validation and error handling are adequately implemented
The codebase shows appropriate validation and error handling for financial operations:
-
All Decimal conversions are handled through dedicated methods with proper validation:
create_coin_amount()uses token-specific validation throughchain_formatted_value()calculate_margin()has proper boundary checks for reduce-only orders- Financial operations use dedicated conversion methods with built-in validation
-
Error handling is implemented through:
- Try-catch blocks for RPC operations
- Proper error propagation in critical paths
- Input validation through type hints and dedicated conversion methods
-
The code uses:
- Strong typing with Decimal for financial values
- Dedicated token and market classes for conversions
- Standardized chain format conversions with validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential validation gaps in financial operations
# Look for decimal operations without try-catch
rg -A 2 "Decimal\("
# Look for financial calculations without validation
rg -A 2 "amount.*=.*Decimal"
Length of output: 149198
Script:
#!/bin/bash
# Look for validation checks around Decimal conversions and financial calculations
# Search for try-catch or validation patterns
rg -A 3 "try:|if.*Decimal|assert.*Decimal|raise.*Decimal"
# Look for input validation in key financial methods
rg -A 5 "def (create_coin_amount|calculate_margin|msg_deposit|msg_withdraw)"
Length of output: 108322
tests/rpc_fixtures/markets_fixtures.py (4)
111-111: Update Fixture Function Signature
The ape_usdt_spot_market_meta fixture no longer accepts parameters. Please ensure that any tests or code that use this fixture are updated to reflect the new signature to prevent unexpected errors.
135-135: Update Fixture Function Signature
The inj_usdt_spot_market_meta fixture still accepts parameters, while similar fixtures have removed them. Verify if the parameters inj_token_meta and usdt_token_meta are necessary. If not, consider removing them for consistency.
173-173: Verify Field Naming Convention for isPerpetual
The field isPerpetual may not match the expected naming convention in Python Protobuf classes, which typically use snake_case. Confirm whether the field should be is_perpetual to avoid potential attribute errors.
223-223:
Correct settlement_timestamp and expiration_timestamp Values
The settlement_timestamp (1707099200) is earlier than the expiration_timestamp (1708099200). Typically, the settlement should occur after expiration. Please ensure these timestamps are accurate.
Adjust the timestamps appropriately:
- expiration_timestamp=1708099200,
- settlement_timestamp=1707099200,
+ expiration_timestamp=1707099200,
+ settlement_timestamp=1708099200,Likely invalid or redundant comment.
CHANGELOG.md
Outdated
| ## [1.8.0] | ||
| ### Changed | ||
| - The markets initialization in AsyncClient has been modified to get markets information from the chain endpoints instead of the Indexer endpoints | ||
|
|
||
| ### Removed | ||
| - Removed the legacy deprecated markets and tokens initialization using the denoms INI files in the SDK. Removed also the INI files from the SDK |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add migration guidance for breaking changes
The changelog indicates significant changes in how markets and tokens are initialized, moving from Indexer endpoints to chain endpoints and removing INI files. However, it lacks guidance for users who need to migrate from the old implementation.
Consider adding a migration section that:
- Explains how to migrate from Indexer endpoints to chain endpoints
- Details any code changes required after removing INI files
- Provides example snippets showing the new initialization approach
Example addition:
### Removed
- Legacy deprecated markets and tokens initialization using the denoms INI files in the SDK. Removed also the INI files from the SDK
+
+### Migration Guide
+- Replace Indexer endpoint initialization with chain endpoints:
+ ```python
+ # Old approach
+ # client.get_markets_from_indexer()
+
+ # New approach
+ # client.get_markets_from_chain()
+ ```
+- Update market initialization code to remove INI file dependencies🧰 Tools
🪛 LanguageTool
[duplication] ~9-~9: Possible typo: you repeated a word
Context: ...s instead of the Indexer endpoints ### Removed - Removed the legacy deprecated markets and token...
(ENGLISH_WORD_REPEAT_RULE)
| min_quantity_tick_size="1", | ||
| min_notional="5", | ||
| admin="inj1knhahceyp57j5x7xh69p7utegnnnfgxavmahjr", | ||
| admin_permissions=1, |
There was a problem hiding this comment.
Ensure Correct Type for admin_permissions
As with the ape_usdt_spot_market_meta fixture, ensure that admin_permissions uses the correct type.
Apply this change if an enum is expected:
- admin_permissions=1,
+ admin_permissions=exchange_pb.AdminPermission.PERMIT_ADMIN_TRANSFER,Committable suggestion skipped: line range outside the PR's diff.
| min_quantity_tick_size="1", | ||
| min_notional="5", | ||
| admin="inj1knhahceyp57j5x7xh69p7utegnnnfgxavmahjr", | ||
| admin_permissions=1, |
There was a problem hiding this comment.
Ensure Correct Type for admin_permissions
Again, verify that admin_permissions is using the correct type as per the proto definition.
Apply this change if an enum is expected:
- admin_permissions=1,
+ admin_permissions=exchange_pb.AdminPermission.PERMIT_ADMIN_TRANSFER,Committable suggestion skipped: line range outside the PR's diff.
| admin="inj1knhahceyp57j5x7xh69p7utegnnnfgxavmahjr", | ||
| admin_permissions=1, |
There was a problem hiding this comment.
Ensure Correct Type for admin_permissions
The admin_permissions field is set to the integer 1. If admin_permissions expects a specific permission object or enum, such as exchange_pb.AdminPermissions, ensure that you are using the correct type. This prevents potential issues with permission handling.
Apply this change if an enum is expected:
- admin_permissions=1,
+ admin_permissions=exchange_pb.AdminPermission.PERMIT_ADMIN_TRANSFER,Committable suggestion skipped: line range outside the PR's diff.
| min_notional="0", | ||
| settlement_price="1", | ||
| min_notional="1", | ||
| admin_permissions=1, |
There was a problem hiding this comment.
Ensure Correct Type for admin_permissions
Check the admin_permissions field for the correct type.
Apply this change if an enum is expected:
- admin_permissions=1,
+ admin_permissions=exchange_pb.AdminPermission.PERMIT_ADMIN_TRANSFER,Committable suggestion skipped: line range outside the PR's diff.
| base_token = self._tokens_by_denom.get(market_info["baseDenom"]) | ||
| quote_token = self._tokens_by_denom.get(market_info["quoteDenom"]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Potential None Values for Tokens in Market Initialization
At lines 3299-3300, base_token and quote_token may be None if the tokens are not found in self._tokens_by_denom. Passing None to the SpotMarket constructor could lead to errors or unexpected behavior.
Add checks to ensure tokens are found before proceeding:
base_token = self._tokens_by_denom.get(market_info["baseDenom"])
quote_token = self._tokens_by_denom.get(market_info["quoteDenom"])
if base_token is None or quote_token is None:
# Handle missing tokens, e.g., log a warning or skip this market
continue # or raise an exception
market = SpotMarket(
id=market_info["marketId"],
status=market_info["status"],
ticker=market_info["ticker"],
base_token=base_token,
quote_token=quote_token,
# ... rest of the initialization
)| quote_token = self._tokens_by_denom.get(market["quoteDenom"]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure quote_token is Valid in Derivative Market Initialization
In lines 3329-3330, quote_token may be None if not present in self._tokens_by_denom. This could cause issues when initializing DerivativeMarket.
Include a check for quote_token:
quote_token = self._tokens_by_denom.get(market["quoteDenom"])
if quote_token is None:
# Handle missing token
continue # or raise an exception
derivative_market = DerivativeMarket(
id=market["marketId"],
status=market["status"],
ticker=market["ticker"],
# ... rest of the initialization
quote_token=quote_token,
# ... rest of the initialization
)| markets_info = (await self.fetch_chain_binary_options_markets(status="Active"))["markets"] | ||
| for market_info in markets_info: | ||
| quote_token = tokens_by_denom.get(market_info["quoteDenom"], None) | ||
| quote_token = self._tokens_by_denom.get(market_info["quoteDenom"]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate quote_token in Binary Option Market Initialization
At line 3364, quote_token might be None if the token is not found. This needs handling to prevent errors during market initialization.
Add a check before initializing the market:
quote_token = self._tokens_by_denom.get(market_info["quoteDenom"])
if quote_token is None:
# Handle missing token
continue # or raise an exception
market = BinaryOptionMarket(
id=market_info["marketId"],
status=market_info["status"],
ticker=market_info["ticker"],
# ... rest of the initialization
quote_token=quote_token,
# ... rest of the initialization
)| self._tokens_by_denom = dict() | ||
| self._tokens_by_symbol = dict() |
There was a problem hiding this comment.
Variable Initialization May Prevent Tokens and Markets from Initializing Properly
By initializing _tokens_by_denom and _tokens_by_symbol to empty dictionaries at lines 160-161, the conditional checks for None in methods like all_tokens, all_spot_markets, all_derivative_markets, and all_binary_option_markets will never trigger. This means _initialize_tokens_and_markets may not be called, leading to uninitialized tokens and markets.
To fix this issue, update the conditions to check if the dictionaries are empty instead of None.
Apply this diff:
@@ -195,7 +195,7 @@ class AsyncClient:
async def all_tokens(self) -> Dict[str, Token]:
- if self._tokens_by_symbol is None:
+ if not self._tokens_by_symbol:
async with self._tokens_and_markets_initialization_lock:
- if self._tokens_by_symbol is None:
+ if not self._tokens_by_symbol:
await self._initialize_tokens_and_markets()
return deepcopy(self._tokens_by_symbol)Repeat similar changes for the other methods:
@@ -202,7 +202,7 @@ class AsyncClient:
async def all_spot_markets(self) -> Dict[str, SpotMarket]:
- if self._spot_markets is None:
+ if not self._spot_markets:
async with self._tokens_and_markets_initialization_lock:
- if self._spot_markets is None:
+ if not self._spot_markets:
await self._initialize_tokens_and_markets()
return deepcopy(self._spot_markets)Committable suggestion skipped: line range outside the PR's diff.
CHANGELOG.md
Outdated
|
|
||
| All notable changes to this project will be documented in this file. | ||
|
|
||
| ## [1.8.0] |
There was a problem hiding this comment.
Please add the date besides the version, as in the previous entries
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
9-10: Fix word repetition in removal noticeThe word "Removed" is repeated unnecessarily.
### Removed -- Removed the legacy deprecated markets and tokens initialization using the denoms INI files in the SDK. Removed also the INI files from the SDK ++ Legacy deprecated markets and tokens initialization using the denoms INI files in the SDK. Removed the INI files from the SDK🧰 Tools
🪛 LanguageTool
[duplication] ~9-~9: Possible typo: you repeated a word
Context: ...s instead of the Indexer endpoints ### Removed - Removed the legacy deprecated markets and token...(ENGLISH_WORD_REPEAT_RULE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~9-~9: Possible typo: you repeated a word
Context: ...s instead of the Indexer endpoints ### Removed - Removed the legacy deprecated markets and token...
(ENGLISH_WORD_REPEAT_RULE)
| ## [1.8.0] - 2024-11-14 | ||
| ### Changed | ||
| - The markets initialization in AsyncClient has been modified to get markets information from the chain endpoints instead of the Indexer endpoints | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance changelog entry with migration guidance
The changelog indicates significant changes in the markets initialization process, but lacks guidance for users who need to migrate their code. Consider adding a migration section that explains:
- The differences between chain endpoints and Indexer endpoints
- Impact on existing applications
- Code examples showing the old vs new approach
Example addition:
### Changed
- The markets initialization in AsyncClient has been modified to get markets information from the chain endpoints instead of the Indexer endpoints
+
+### Migration Guide
+#### Markets Initialization
+```python
+# Before (using Indexer endpoints)
+markets = await client.get_markets_from_indexer()
+
+# After (using chain endpoints)
+markets = await client.get_markets_from_chain()
+```
+
+#### Impact
+- Performance: Chain endpoints provide real-time market data
+- Consistency: Direct access to on-chain market state
+- Breaking Change: Applications using Indexer endpoints need to be updated
Summary by CodeRabbit
Release Notes for Version 1.8.0
New Features
Deprecations
Bug Fixes
Chores
Tests