Feat/refactor markets tokens initialization #355
Conversation
…ent to get markets from chain instead of using Indexer endpoints. Removed deprecated support for denom INI files
WalkthroughThe pull request introduces significant updates across various modules, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
🪧 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: 4
🧹 Outside diff range and nitpick comments (30)
pyinjective/core/token.py (2)
17-19: Consider using Decimal.shift() for better performanceThe current implementation creates a new Decimal object from a string. Using
Decimal.shift()would be more efficient for this power-of-10 multiplication.@staticmethod def convert_value_to_extended_decimal_format(value: Decimal) -> Decimal: - return value * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") + return value.shift(ADDITIONAL_CHAIN_FORMAT_DECIMALS)
17-24: Consider consolidating decimal conversion methodsThe class now has three methods dealing with decimal conversions:
convert_value_to_extended_decimal_formatconvert_value_from_extended_decimal_formatchain_formatted_valueConsider creating a more cohesive API for decimal conversions, possibly with clear documentation about when to use each method and how they relate to each other.
pyinjective/ofac.py (1)
7-9: Consider using a more robust URL structure.The current URL structure includes redundant path components (
refs/heads/master). Consider simplifying it to reduce potential breakage points.-OFAC_LIST_URL = ( - "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofac.json" -) +OFAC_LIST_URL = ( + "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/master/json/wallets/ofac.json" +)This maintains the same functionality while being more resilient to repository structure changes.
tests/test_orderhash.py (2)
20-34: Add docstring to the fixture for better maintainability.Consider adding a docstring to explain the purpose and setup of this fixture, which will help other developers understand its role in the test suite.
Example addition:
@pytest.fixture def basic_composer(self, inj_usdt_spot_market, btc_usdt_perp_market, first_match_bet_market): + """ + Creates a basic Composer instance with predefined market configurations. + + Args: + inj_usdt_spot_market: Fixture providing INJ/USDT spot market + btc_usdt_perp_market: Fixture providing BTC/USDT perpetual market + first_match_bet_market: Fixture providing binary options market + + Returns: + Composer: Initialized composer instance with configured markets and tokens + """🧰 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)
48-48: Consider more robust market ID retrieval.The current approach of getting the first market ID from keys might be fragile if market order changes. Consider using the specific market ID directly from the fixture.
-spot_market_id = list(basic_composer.spot_markets.keys())[0] +spot_market_id = inj_usdt_spot_market.idpyinjective/ofac.json (1)
2-155: Consider adding documentation for OFAC list updates.Given the significant number of changes (66 additions, 6 removals), consider adding a comment block at the top of the file or in accompanying documentation to track:
- The date of the update
- The source/version of the OFAC list
- The rationale for additions/removals
[ + // Last updated: <date> + // Source: OFAC SDN List version <version> + // Changes: Added 66 addresses, removed 6 addresses "0x01e2919679362dfbc9ee1644ba9c6da6d6245bb1",tests/core/test_message_based_transaction_fee_calculator.py (2)
29-43: LGTM with a minor suggestion for readabilityThe fixture effectively centralizes the Composer setup for tests. Consider extracting the tokens dictionary to improve readability:
@pytest.fixture def basic_composer(self, inj_usdt_spot_market, btc_usdt_perp_market, first_match_bet_market): + tokens = { + inj_usdt_spot_market.base_token.symbol: inj_usdt_spot_market.base_token, + inj_usdt_spot_market.quote_token.symbol: inj_usdt_spot_market.quote_token, + btc_usdt_perp_market.quote_token.symbol: btc_usdt_perp_market.quote_token, + } composer = Composer( network=Network.devnet().string(), spot_markets={inj_usdt_spot_market.id: inj_usdt_spot_market}, derivative_markets={btc_usdt_perp_market.id: btc_usdt_perp_market}, binary_option_markets={first_match_bet_market.id: first_match_bet_market}, - tokens={ - inj_usdt_spot_market.base_token.symbol: inj_usdt_spot_market.base_token, - inj_usdt_spot_market.quote_token.symbol: inj_usdt_spot_market.quote_token, - btc_usdt_perp_market.quote_token.symbol: btc_usdt_perp_market.quote_token, - }, + tokens=tokens, ) return 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)
Line range hint
140-229: LGTM: Test refactoring improves maintainabilityThe changes effectively remove hardcoded values and standardize market setup across tests. Consider extracting the common network and client setup into a fixture to further reduce duplication:
@pytest.fixture def calculator(self, basic_composer): network = Network.testnet(node="sentry") client = AsyncClient(network=network) return MessageBasedTransactionFeeCalculator( client=client, composer=basic_composer, gas_price=5_000_000, )This would simplify the test methods and reduce duplication of the calculator setup code.
CHANGELOG.md (3)
5-5: Update placeholder release dateThe version entry uses a placeholder date (9999-99-99). Please update this to reflect the actual planned release date.
-## [1.8.0] - 9999-99-99 +## [1.8.0] - 2024-11-DD
7-8: Enhance change descriptions with more detailsThe change descriptions could benefit from more specific details:
- For the markets initialization change, consider mentioning why this change is beneficial (e.g., improved reliability, reduced latency, etc.)
- For the OFAC link change, consider including the new link format or explaining why it was changed
-The markets initialization in AsyncClient has been modified to get markets information from the chain endpoints instead of the Indexer endpoints -Changed link to the official list of OFAC filtered addresses +The markets initialization in AsyncClient has been modified to fetch markets information directly from chain endpoints instead of Indexer endpoints, improving reliability and data consistency +Updated the link format for the official OFAC filtered addresses list to align with new standards
10-11: Fix word repetition in the Removed sectionThere's unnecessary word repetition in the removal notice.
-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 denoms INI files, along with the INI files themselves from the SDK🧰 Tools
🪛 LanguageTool
[duplication] ~10-~10: Possible typo: you repeated a word
Context: ...al list of OFAC filtered addresses ### Removed - Removed the legacy deprecated markets and token...(ENGLISH_WORD_REPEAT_RULE)
tests/test_async_client.py (4)
32-33: Add docstring to the fixtureThe consolidation of exchange servicers is good, but consider adding a docstring to explain the fixture's purpose and usage.
@pytest.fixture def exchange_servicer(): + """ + Returns a configurable exchange query servicer for mocking exchange-related responses in tests. + This fixture consolidates spot, derivative, and binary options market responses. + """ return ConfigurableExchangeQueryServicer()
84-148: Consider improving token list maintainabilityWhile the token list is well-structured, consider these improvements:
- Move the token list to a separate fixture file
- Use type hints with a TokenInfo type
- Define constants for common values like "verified", "internal", token types
Example implementation:
from dataclasses import dataclass from typing import Optional @dataclass class TokenInfo: address: str denom: str symbol: str name: str decimals: int logo: str token_type: str token_verification: str is_native: bool = False coin_gecko_id: str = "" external_logo: str = "unknown.png" # In constants.py TOKEN_VERIFICATION_VERIFIED = "verified" TOKEN_VERIFICATION_INTERNAL = "internal" TOKEN_TYPE_ERC20 = "erc20" TOKEN_TYPE_NATIVE = "native" TOKEN_TYPE_TOKEN_FACTORY = "tokenFactory"
151-158: Extract market response setup into helper methodsConsider extracting the market response setup into helper methods to improve test readability and reusability.
def setup_market_responses( exchange_servicer: ConfigurableExchangeQueryServicer, spot_markets: list, derivative_markets: list, binary_markets: list ) -> None: """Helper method to set up all market responses.""" exchange_servicer.spot_markets_responses.append( exchange_query_pb.QuerySpotMarketsResponse(markets=spot_markets) ) exchange_servicer.derivative_markets_responses.append( exchange_query_pb.QueryDerivativeMarketsResponse(markets=derivative_markets) ) exchange_servicer.binary_options_markets_responses.append( exchange_query_pb.QueryBinaryMarketsResponse(markets=binary_markets) )
Line range hint
170-190: Improve test assertions readability
- Fix Yoda conditions
- Simplify market ID assertions using
anywith direct attribute access- assert 5 == len(all_tokens) + assert len(all_tokens) == 5 - assert any((btc_usdt_perp_market_meta.market.market_id == market.id for market in all_derivative_markets.values())) + assert any(market.id == btc_usdt_perp_market_meta.market.market_id for market in all_derivative_markets.values())🧰 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/core/test_gas_limit_estimator.py (1)
192-192: Fix typo in sender parameterThe sender parameter is specified as "senders" (plural) instead of "sender" (singular) in multiple test methods.
Apply this fix:
- sender="senders", + sender="sender",Also applies to: 228-228, 474-474
tests/test_composer_deprecation_warnings.py (3)
Line range hint
130-146: Improve test readability with descriptive test data.The test uses generic values (price=1, quantity=1) which could be made more meaningful with realistic test data.
Consider using more descriptive test values:
- price=Decimal(1), - quantity=Decimal(1), + price=Decimal("1000.50"), # Example BTC/USDT price + quantity=Decimal("0.15"), # Example BTC quantity
Line range hint
258-275: Consider testing edge cases in batch order creation.The test only verifies the deprecation warning with a single order. Consider adding test cases with multiple orders and edge cases.
Add test cases for:
- Empty order list
- Multiple orders
- Orders with different market IDs
orders=[order], +orders=[], # Test empty list +orders=[order, order], # Test multiple orders
Line range hint
475-486: Improve test coverage for position margin increase.The test only verifies the deprecation warning but doesn't validate the relationship between source and destination subaccounts.
Consider adding test cases for:
- Same source and destination subaccount IDs
- Invalid market ID
- Zero amount
amount=1, +amount=0, # Test zero amount case +market_id="invalid_market_id", # Test invalid market casetests/test_composer.py (3)
Line range hint
18-39: Consider adding type hints to improve code clarityThe fixtures are well-structured, but adding type hints would improve code clarity and IDE support.
@pytest.fixture - def basic_composer(self, inj_usdt_spot_market, btc_usdt_perp_market, first_match_bet_market): + def basic_composer(self, inj_usdt_spot_market: SpotMarket, btc_usdt_perp_market: DerivativeMarket, first_match_bet_market: BinaryOptionsMarket) -> Composer:
Line range hint
1007-1051: Fix duplicate test method nameThere are two test methods named
test_msg_cancel_derivative_order. This could cause confusion and potential test execution issues.Rename one of the methods to reflect its specific purpose:
- def test_msg_cancel_derivative_order(self, basic_composer): + def test_msg_cancel_binary_options_order(self, basic_composer):
Line range hint
40-1051: Consider refactoring test methods for better maintainabilityThe test methods are comprehensive but could benefit from some structural improvements:
- Extract common test data setup into helper methods
- Move magic numbers and test values to class-level constants
- Consider using parameterized tests for similar test cases
Example refactoring:
class TestComposer: # Test constants TEST_SUBACCOUNT_ID = "0x5e249f0e8cb406f41de16e1bd6f6b55e7bc75add000000000000000000000000" TEST_FEE_RECIPIENT = "inj1hkhdaj2a2clmq5jq6mspsggqs32vynpk228q3r" TEST_SENDER = "inj1apmvarl2xyv6kecx2ukkeymddw3we4zkygjyc0" def _setup_test_order_params(self): """Helper method to set up common order parameters""" return { "price": Decimal("36.1"), "quantity": Decimal("100"), "order_type": "BUY", "cid": "test_cid", "trigger_price": Decimal("43.5") }pyinjective/composer.py (2)
Line range hint
147-151: Standardize deprecation message formatWhile the deprecation warnings are correctly implemented, consider standardizing the deprecation message format across all methods. For example, some messages use "This method is deprecated. Use X instead" while others use "This method is deprecated and will be removed soon. Please use X instead".
Apply this consistent format to all deprecation warnings:
- warn("This method is deprecated. Use coin instead", DeprecationWarning, stacklevel=2) + warn("This method is deprecated and will be removed soon. Please use coin instead", DeprecationWarning, stacklevel=2)Also applies to: 156-160
Line range hint
1-1183: Consider extracting common conversion logicThe codebase has repeated patterns for converting human-readable values to chain format and validating parameters. Consider extracting these common operations into utility methods to reduce code duplication and improve maintainability.
For example:
- Create a utility method for human-readable to chain format conversion
- Create a base message composition class that handles common parameter validation
Example utility method:
def _convert_to_chain_format(self, human_readable_value: Decimal, token: Token, additional_decimals: bool = True) -> Decimal: """Convert human readable value to chain format.""" chain_value = token.chain_formatted_value(human_readable_value=human_readable_value) if additional_decimals: chain_value *= Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") return chain_valuepyinjective/async_client.py (1)
3297-3319: Consider extracting decimal conversion logic to reduce duplicationWhile the spot market initialization logic is correct, the repeated use of
Token.convert_value_from_extended_decimal_format(Decimal())creates unnecessary duplication. Consider extracting this into a helper method.def _convert_market_decimal(value: str) -> Decimal: return Token.convert_value_from_extended_decimal_format(Decimal(value)) # Usage example: maker_fee_rate=self._convert_market_decimal(market_info["makerFeeRate"])tests/rpc_fixtures/markets_fixtures.py (5)
111-127: Remove unused fixture parameters inape_usdt_spot_market_metaThe fixture
ape_usdt_spot_market_metano longer uses the parametersape_token_metaandusdt_token_meta_second_denomwithin its body. Removing unused parameters enhances code readability and maintainability.Apply this diff:
-def ape_usdt_spot_market_meta(ape_token_meta, usdt_token_meta_second_denom): +def ape_usdt_spot_market_meta():
135-150: Remove unused fixture parameters ininj_usdt_spot_market_metaThe fixture
inj_usdt_spot_market_metadoes not utilizeinj_token_metaandusdt_token_metawithin its body. Consider removing these unused parameters to simplify the function signature.Apply this diff:
-def inj_usdt_spot_market_meta(inj_token_meta, usdt_token_meta): +def inj_usdt_spot_market_meta():
158-180: Remove unused fixture parameter inbtc_usdt_perp_market_metaThe parameter
usdt_perp_token_metainbtc_usdt_perp_market_metais not used within the function body. Removing it will improve code clarity.Apply this diff:
-def btc_usdt_perp_market_meta(usdt_perp_token_meta): +def btc_usdt_perp_market_meta():
122-122: UseMarketStatusenumeration instead of integer literals forstatusThe
statusfield is currently set using the integer1. For better readability and maintainability, consider using theMarketStatusenumeration fromexchange_pb.Apply this diff:
- status=1, + status=exchange_pb.MarketStatus.MarketStatusActive,Ensure that
MarketStatusActivecorresponds to the correct status value and that you have importedMarketStatusfromexchange_pb.Also applies to: 145-145, 174-174, 230-230
127-127: UseAdminPermissionenumeration instead of integer literals foradmin_permissionsThe
admin_permissionsfield is set using the integer1. It's preferable to use theAdminPermissionenumeration for clarity.Apply this diff:
- admin_permissions=1, + admin_permissions=exchange_pb.AdminPermission.PERMISSION_ADMIN,Make sure to import
AdminPermissionfromexchange_pband verify thatPERMISSION_ADMINcorresponds to the intended permission level.Also applies to: 150-150, 179-179, 235-235
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
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/ofac.json(1 hunks)pyinjective/ofac.py(1 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)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
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~10-~10: Possible typo: you repeated a word
Context: ...al list of OFAC filtered addresses ### 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 (21)
pyinjective/core/token.py (1)
4-4: Verify the ADDITIONAL_CHAIN_FORMAT_DECIMALS constant value
Let's ensure this constant is properly defined and consistently used across the codebase.
✅ Verification successful
ADDITIONAL_CHAIN_FORMAT_DECIMALS is properly defined and consistently used across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the constant definition and its usage
# Expected: Find the constant definition and verify its consistent usage
# Find the constant definition
echo "Constant definition:"
rg -l "ADDITIONAL_CHAIN_FORMAT_DECIMALS\s*="
# Check all usages of this constant
echo -e "\nConstant usage:"
rg "ADDITIONAL_CHAIN_FORMAT_DECIMALS" -A 2
Length of output: 22975
pyinjective/ofac.py (1)
7-9: Verify the accessibility and data format of the new OFAC list URL.
Let's ensure the new URL is accessible and returns the expected data format.
✅ Verification successful
OFAC list URL is accessible and no other instances of the old URL were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the OFAC list URL accessibility and data format
# Test 1: Check if the URL is accessible and returns valid JSON
echo "Checking URL accessibility and JSON validity..."
curl -s "https://raw.githubusercontent.com/InjectiveLabs/injective-lists/refs/heads/master/json/wallets/ofac.json" | jq . >/dev/null 2>&1
if [ $? -eq 0 ]; then
echo "URL is accessible and returns valid JSON"
else
echo "Error: URL might be inaccessible or returns invalid JSON"
fi
# Test 2: Search for other potential usages of the old URL pattern
echo "Checking for other instances of similar URL patterns..."
rg -l "InjectiveLabs/injective-lists/master/wallets" || echo "No other instances found"
Length of output: 568
tests/test_orderhash.py (1)
76-77: Verify updated hash values.
The order hash values have been modified. Let's verify these changes are expected and match the current market configuration.
pyinjective/orderhash.py (1)
8-8: LGTM: Token import addition
The addition of the Token import aligns with the PR's objective of refactoring token handling and is properly utilized in the updated param_to_backend_go function.
pyinjective/ofac.json (2)
2-155: Verify Ethereum address format and checksums.
All addresses follow the standard Ethereum address format, but it's recommended to validate that these addresses are properly checksummed to prevent any potential errors in address matching.
#!/bin/bash
# Description: Verify the format of Ethereum addresses and identify any non-checksummed addresses
# Check if all addresses match the Ethereum address format
echo "Verifying Ethereum address format..."
cat pyinjective/ofac.json | jq -r '.[]' | while read addr; do
if ! echo "$addr" | grep -E "^0x[0-9a-fA-F]{40}$" > /dev/null; then
echo "Invalid address format: $addr"
fi
done
# Note: A more thorough validation would include checksum verification,
# but that requires web3 libraries which aren't available in the sandbox19-23: Verify the address replacement.
The replacement of address 0x19aa5fe80d33a56d56c78e82ea5e50e5d80b4dff with 0x19f8f2b0915daa12a3f5c9cf01df9e24d53794f7 should be cross-referenced with official OFAC updates to ensure accuracy.
tests/core/test_message_based_transaction_fee_calculator.py (1)
20-25: LGTM: Imports are correctly structured
The new fixture imports are properly annotated with noqa: F401 since they are used implicitly by pytest's fixture system.
CHANGELOG.md (1)
5-12: Verify the impact of INI files removal
The removal of legacy INI files is a breaking change that should be clearly marked as such. Additionally, consider providing migration guidance for users who might still be relying on these files.
Consider adding a "Breaking Changes" section to clearly highlight the removal of INI files and provide migration steps for users:
+### Breaking Changes
+- Removed legacy denoms INI files. Users should now rely on direct chain data retrieval. For migration assistance, please refer to the upgrade guide.🧰 Tools
🪛 LanguageTool
[duplication] ~10-~10: Possible typo: you repeated a word
Context: ...al list of OFAC filtered addresses ### Removed - Removed the legacy deprecated markets and token...
(ENGLISH_WORD_REPEAT_RULE)
tests/test_async_client.py (2)
9-9: LGTM! Import changes align with the refactoring goals
The consolidation of exchange-related imports improves code organization by using a unified exchange query protocol buffer.
Also applies to: 11-11
Line range hint 201-304: LGTM! Comprehensive test coverage for token initialization
The test methods thoroughly cover different token initialization scenarios:
- Reading from official list
- Initializing from chain denoms
- Handling empty responses
Note: The static analysis warnings about unused parameter redefinitions can be safely ignored as these parameters are actually used in the tests.
tests/core/test_gas_limit_estimator.py (2)
33-47: Well-structured fixture implementation!
The basic_composer fixture effectively reduces code duplication and improves test maintainability by providing a consistent Composer instance across tests.
🧰 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)
Line range hint 49-543: Test implementation looks good!
The test suite provides comprehensive coverage for gas limit estimation across different order types and operations. The refactoring to use the basic_composer fixture has improved code organization while maintaining thorough test coverage.
🧰 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)
tests/test_composer_deprecation_warnings.py (2)
33-39: LGTM! Clean refactoring using the fixture.
The test has been properly refactored to use the basic_composer fixture, making it more maintainable and consistent with other tests.
Line range hint 76-89: Verify the market ID retrieval logic.
While the dynamic market ID retrieval is a good practice, we should ensure it handles empty market dictionaries gracefully.
Consider adding a safety check:
-market_id = list(basic_composer.spot_markets.keys())[0]
+if not basic_composer.spot_markets:
+ pytest.skip("No spot markets available")
+market_id = next(iter(basic_composer.spot_markets))tests/test_composer.py (2)
Line range hint 1-17: Clean and well-organized imports!
The import statements are properly structured and the consolidation of fixture imports improves readability.
Line range hint 1-1051: Overall excellent test coverage and structure!
The test suite demonstrates high quality with:
- Comprehensive coverage of all composer functionality
- Proper handling of financial calculations and decimals
- Thorough validation of protobuf message structures
- Consistent test patterns and assertions
The minor issues noted above don't detract from the overall quality of the test suite.
pyinjective/composer.py (1)
138-141: Clean initialization using Python idioms!
The use of the or operator for dictionary initialization is a concise and pythonic way to provide default values. This change improves code readability while maintaining the same functionality.
pyinjective/async_client.py (4)
160-161: Initialization improvement: Using empty dictionaries instead of Optional types
The change to initialize token dictionaries with empty dictionaries instead of Optional types simplifies the code by eliminating the need for None checks.
3294-3296: LGTM: Clean token dictionary updates from official lists
The token dictionaries are properly updated with values from official lists, aligning with the PR's objective to move away from INI files.
3388-3391: LGTM: Proper null handling for settlement price
The code correctly handles the case where settlement price is empty by using a conditional expression to set it to None.
3324-3330: Verify the nested market data structure handling
The code handles a nested market data structure where market info is accessed through market_info["market"]. Please ensure this structure is consistently maintained in the API response.
| @staticmethod | ||
| def convert_value_from_extended_decimal_format(value: Decimal) -> Decimal: | ||
| return value / Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and optimize the implementation
- The method could benefit from documentation explaining how it differs from
chain_formatted_value. - Similar to the previous method, using
Decimal.shift()would be more efficient.
@staticmethod
def convert_value_from_extended_decimal_format(value: Decimal) -> Decimal:
+ """
+ Converts a value from extended decimal format by removing additional chain format decimals.
+ This differs from chain_formatted_value which handles token-specific decimal places.
+ """
- return value / Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}")
+ return value.shift(-ADDITIONAL_CHAIN_FORMAT_DECIMALS)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def convert_value_from_extended_decimal_format(value: Decimal) -> Decimal: | |
| return value / Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") | |
| @staticmethod | |
| def convert_value_from_extended_decimal_format(value: Decimal) -> Decimal: | |
| """ | |
| Converts a value from extended decimal format by removing additional chain format decimals. | |
| This differs from chain_formatted_value which handles token-specific decimal places. | |
| """ | |
| return value.shift(-ADDITIONAL_CHAIN_FORMAT_DECIMALS) |
| def param_to_backend_go(param: str) -> str: | ||
| go_param = Token.convert_value_from_extended_decimal_format(value=Decimal(param)) | ||
| return f"{go_param.normalize():.18f}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling
While the type hints and conversion logic are good improvements, this function needs additional robustness since it's used in critical order processing paths.
Consider implementing these safety checks:
def param_to_backend_go(param: str) -> str:
+ if not param:
+ raise ValueError("Parameter cannot be empty")
+ try:
+ decimal_param = Decimal(param)
+ if decimal_param < 0:
+ raise ValueError("Parameter cannot be negative")
go_param = Token.convert_value_from_extended_decimal_format(value=Decimal(param))
return f"{go_param.normalize():.18f}"
+ except (ValueError, decimal.InvalidOperation) as e:
+ raise ValueError(f"Invalid parameter format: {str(e)}")This change will:
- Prevent empty string inputs
- Validate non-negative values
- Properly handle decimal conversion errors
- Provide clear error messages
Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Add comprehensive tests for param_to_backend_go
Ensure that param_to_backend_go is thoroughly tested with various edge cases and proper error handling to maintain robust order processing.
🔗 Analysis chain
Verify order processing with edge cases
Since this function is critical for order processing, ensure comprehensive testing with various scenarios.
Run the following script to find and analyze test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for param_to_backend_go function
# and its usage in order processing
# Find test files related to order processing
echo "=== Finding relevant test files ==="
rg -l "param_to_backend_go|build_eip712_msg" tests/
# Check specific test cases for edge values
echo "=== Checking test cases for edge values ==="
rg -A 5 "def test.*order" tests/
rg -A 5 "param_to_backend_go" tests/
# Look for existing error handling tests
echo "=== Checking error handling tests ==="
rg -A 5 "(pytest.raises|assertRaises)" tests/
Length of output: 54700
| expiration_timestamp=1708099200, | ||
| settlement_timestamp=1707099200, |
There was a problem hiding this comment.
Verify settlement_timestamp is after expiration_timestamp
In first_match_bet_market_meta, the settlement_timestamp (1707099200) is earlier than the expiration_timestamp (1708099200). Typically, the settlement should occur after the expiration. Please verify and correct these timestamps if necessary.
| updated: int | ||
|
|
||
| @staticmethod | ||
| def convert_value_to_extended_decimal_format(value: Decimal) -> Decimal: |
There was a problem hiding this comment.
for me these methods names seem too long since they're commonly used across the sdk
…o feat/refactor_markets_tokens_initialization
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CHANGELOG.md (2)
5-5: Update the version date placeholderThe version date is currently set to a placeholder value (9999-99-99). Please update it to the actual release date.
9-10: Fix word repetition in the Removed sectionThere's unnecessary repetition of the word "Removed". Consider rephrasing to make it more concise.
Apply this diff to improve readability:
-### 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, including the INI files themselves🧰 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)
🔇 Additional comments (1)
CHANGELOG.md (1)
5-10: LGTM! Changelog entries accurately reflect the PR objectives
The changelog entries properly document both major changes:
- The transition from Indexer to chain endpoints for markets initialization
- The removal of deprecated INI files-based initialization
The changes are well-categorized and provide clear information about the modifications made in this version.
🧰 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)
Summary by CodeRabbit
Release Notes for Version 1.8.0
New Features
Deprecations
Bug Fixes
Documentation
Tests