feat/add unique token symbol #386
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as TokensFileLoader
participant Token as Token
participant Client as AsyncClient/AsyncClientV2
Loader->>Token: Create Token (unique_symbol: "")
Client->>Token: Create Token (unique_symbol: computed value)
Client->>Client: Store Token by unique_symbol in dictionaries
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new unique_symbol property on Token and propagates its value through model loaders, async clients, fixtures, and tests.
- Added
unique_symbolfield to theTokenmodel and initialized it in the JSON loader and both async clients. - Updated tests and fixtures to include assertions and values for
unique_symbol. - Bumped the package version and updated the CI matrix to Python 3.12 (removed Windows).
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/model_fixtures/markets_v2_fixtures.py | Added unique_symbol values to market fixtures |
| tests/core/test_tokens_file_loader.py | Added assertions for default unique_symbol |
| pyproject.toml | Bumped version to 1.11.0-rc5 |
| pyinjective/core/tokens_file_loader.py | Initialized unique_symbol in load_json |
| pyinjective/core/token.py | Added unique_symbol field to Token model |
| pyinjective/async_client_v2.py | Populated unique_symbol in initialize_tokens_from_chain_denoms, _token_representation, and _tokens_from_official_lists |
| pyinjective/async_client.py | Same updates as above for sync async client |
| .github/workflows/run-tests.yml | Added Python 3.12; removed Windows from OS matrix |
Comments suppressed due to low confidence (2)
pyinjective/async_client_v2.py:1287
- There are no tests verifying that
unique_symbolis correctly computed and assigned ininitialize_tokens_from_chain_denoms. Consider adding unit tests to cover this new behavior.
unique_symbol=unique_symbol,
.github/workflows/run-tests.yml:12
- [nitpick] Windows has been removed from the test matrix. If cross-platform support includes Windows, re-add
windows-latestto ensure test coverage on that OS.
os: [ubuntu-latest, macos-latest]
| decimals=token_info["decimals"], | ||
| logo=token_info["logo"], | ||
| updated=-1, | ||
| unique_symbol="", |
There was a problem hiding this comment.
The loader currently hardcodes unique_symbol to an empty string. To preserve any unique_symbol provided in the source JSON, use something like unique_symbol=token_info.get('unique_symbol', '').
| unique_symbol="", | |
| unique_symbol=token_info.get("unique_symbol", ""), |
|
|
||
| tokens_by_denom[token.denom] = token | ||
| tokens_by_symbol[unique_symbol] = token | ||
| new_token = Token( |
There was a problem hiding this comment.
[nitpick] The token-instantiation block is duplicated in both clients (async_client.py and async_client_v2.py). Consider extracting a shared factory or helper method to reduce duplication.
| new_token = Token( | |
| new_token = self._create_token( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
pyinjective/async_client.py (1)
2262-2266: Excellent implementation of unique_symbol feature with opportunity for refactoring.The unique_symbol calculation logic is consistently implemented across all three token initialization methods. The fallback strategy (denom → symbol → name) ensures uniqueness while providing meaningful identifiers.
The unique_symbol calculation logic is duplicated in three places. Extract this into a private helper method to improve maintainability:
+ def _calculate_unique_symbol( + self, + denom: str, + symbol: str, + name: str, + tokens_by_symbol: Dict[str, Token] + ) -> str: + """Calculate a unique symbol for the token, using fallback logic.""" + unique_symbol = denom + for symbol_candidate in [symbol, name]: + if symbol_candidate not in tokens_by_symbol: + unique_symbol = symbol_candidate + break + return unique_symbolThen replace the duplicated logic in all three methods with calls to this helper method.
Also applies to: 2401-2405, 2435-2439
🧹 Nitpick comments (2)
pyinjective/async_client.py (2)
2268-2280: LGTM: Token creation with unique_symbol is correctly implemented.The unique_symbol calculation logic and Token constructor call are properly implemented. The fallback logic from denom → symbol → name ensures uniqueness.
Consider extracting the unique_symbol calculation logic into a helper method since it's duplicated across multiple methods:
+ def _calculate_unique_symbol(self, denom: str, symbol: str, name: str, tokens_by_symbol: Dict[str, Token]) -> str: + unique_symbol = denom + for symbol_candidate in [symbol, name]: + if symbol_candidate not in tokens_by_symbol: + unique_symbol = symbol_candidate + break + return unique_symbol
2407-2420: LGTM: Consistent implementation of unique_symbol logic.The Token creation with unique_symbol parameter is correctly implemented and follows the same pattern as other methods.
This method duplicates the unique_symbol calculation logic from
initialize_tokens_from_chain_denoms. Consider using the helper method suggested earlier to eliminate this duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/run-tests.yml(1 hunks)pyinjective/async_client.py(3 hunks)pyinjective/async_client_v2.py(3 hunks)pyinjective/core/token.py(1 hunks)pyinjective/core/tokens_file_loader.py(1 hunks)pyproject.toml(1 hunks)tests/core/test_tokens_file_loader.py(2 hunks)tests/model_fixtures/markets_v2_fixtures.py(3 hunks)
🔇 Additional comments (11)
pyinjective/core/token.py (1)
16-16: Breaking change handled consistently across codebase.Adding a required field to the dataclass is a breaking change, but based on the AI summary, this appears to be properly coordinated with updates to token initialization throughout the codebase.
.github/workflows/run-tests.yml (1)
11-12: Good addition of Python 3.12 support.Adding Python 3.12 to the test matrix ensures compatibility with the latest Python version. Note that Windows testing has been removed - ensure this aligns with your platform support strategy.
pyproject.toml (1)
3-3: Appropriate version bump for new feature.The increment from rc4 to rc5 is suitable for the addition of the unique_symbol feature.
pyinjective/core/tokens_file_loader.py (1)
22-22: Proper initialization of new required field.Initializing
unique_symbolto an empty string is appropriate since the JSON data doesn't contain this field. This suggests the unique symbol will be computed/assigned elsewhere in the token processing pipeline.tests/core/test_tokens_file_loader.py (2)
50-50: Good test coverage for new field.The assertion properly verifies that
unique_symbolis initialized to an empty string as expected from the loader.
100-100: Consistent test coverage maintained.The test properly verifies the new field behavior in the async loading scenario as well.
tests/model_fixtures/markets_v2_fixtures.py (1)
19-19: LGTM! Test fixtures correctly updated for the new unique_symbol field.The fixtures properly demonstrate the unique_symbol usage pattern - using the symbol when it's unique (
"INJ","USDT") and falling back to the denom for disambiguation (usdt_perp_tokenuses the denom to differentiate from the regular USDT token).Also applies to: 35-35, 51-51
pyinjective/async_client_v2.py (3)
1273-1278: Well-implemented unique symbol determination logic.The logic correctly ensures uniqueness by checking symbol availability first, then name, and finally falling back to denom. This approach prioritizes human-readable identifiers while guaranteeing uniqueness in the tokens_by_symbol dictionary.
Also applies to: 1287-1287
1412-1416: Consistent unique symbol logic in _token_representation method.The implementation mirrors the logic in
initialize_tokens_from_chain_denoms, maintaining consistency across different token creation paths.Also applies to: 1426-1426
1446-1450: Proper unique symbol handling for tokens from official lists.The method correctly determines unique symbols and creates new Token instances with the additional unique_symbol field. The logic is consistent with other token initialization methods.
Also applies to: 1452-1464
pyinjective/async_client.py (1)
2441-2453: LGTM: Comprehensive Token creation and storage implementation.The Token creation with all parameters including unique_symbol is correctly implemented. The dictionary storage uses appropriate keys (denom for
tokens_by_denomand unique_symbol fortokens_by_symbol).The variable naming with
new_tokenis clear and the implementation maintains consistency with the established patterns.
Summary by CodeRabbit
New Features
unique_symbol, to tokens for improved identification and handling across the platform.Tests
unique_symbolattribute in tokens.Chores