fix/add_notional_quantization #374
Conversation
WalkthroughThis update bumps the project to version 1.9.1. A new entry in the CHANGELOG documents a fix in the conversion of notional values to chain format. In the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant M as Market Method
participant Q as Quantizer
participant X as Multiplier
C->>M: Call notional_to_chain_format(value)
M->>Q: Quantize chain_formatted_value (ROUND_UP)
Q-->>M: Return quantized_value
M->>X: Multiply quantized_value by 10^(ADDITIONAL_CHAIN_FORMAT_DECIMALS)
X-->>M: Return extended_chain_formatted_value
M-->>C: Return final chain formatted value
Poem
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (1)
pyinjective/core/market.py (1)
124-125: Consider standardizing variable naming across similar methods.While the implementation is correct, the variable is named
quantized_notionalhere butquantized_valuein the BinaryOptionMarket class. Consider standardizing the variable names for easier maintenance.- quantized_notional = chain_formatted_value.quantize(Decimal("1"), rounding=ROUND_UP) - extended_chain_formatted_value = quantized_notional * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") + quantized_value = chain_formatted_value.quantize(Decimal("1"), rounding=ROUND_UP) + extended_chain_formatted_value = quantized_value * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)pyinjective/core/market.py(4 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run-tests (3.11, windows-latest)
- GitHub Check: run-tests (3.10, windows-latest)
- GitHub Check: run-tests (3.9, windows-latest)
🔇 Additional comments (5)
pyinjective/core/market.py (4)
2-2: LGTM! Import of ROUND_UP looks good.You've correctly imported ROUND_UP from the decimal module that will be used for the quantization implementation.
42-43: Consistent implementation of notional quantization across market types.The quantization is correctly implemented to round up notional values to the nearest whole number before applying additional chain formatting. This will ensure that notional values are consistently handled across all market types and avoid potential precision issues in chain representation.
Also applies to: 124-125, 235-236
235-236: Good implementation of quantization in BinaryOptionMarket.The notional quantization implementation for BinaryOptionMarket looks correct and follows the same pattern as the other market classes.
42-43:Details
❓ Verification inconclusive
Verify impact of ROUND_UP on boundary cases.
Using ROUND_UP will always round to the next higher integer, which is different from mathematical rounding. This could potentially increase notional values slightly compared to previous behavior.
Also applies to: 124-125, 235-236
🏁 Script executed:
#!/bin/bash # Check for existing unit tests that might be affected by the rounding change echo "Searching for unit tests related to notional_to_chain_format:" fd -t f -e py "test" | xargs grep -l "notional_to_chain_format" || echo "No direct test files found" echo "Searching for usage of notional_to_chain_format method:" rg "notional_to_chain_format" --type pyLength of output: 1582
Action Required: Confirm Rounding Behavior on Boundary Cases
The revised implementation uses
ROUND_UPwhen quantizing values, which may push results to the next higher integer on boundary cases. This behavior differs from standard mathematical rounding and could lead to slightly increased notional values. Please ensure the following:
- Review Unit Tests: Verify that the tests in
tests/core/test_market.pyfornotional_to_chain_formatadequately cover edge cases where the value is exactly at the quantization boundary.- Check Consistency Across Locations: The same rounding logic appears at lines 42-43, as well as lines 124-125 and 235-236 in
pyinjective/core/market.py. Confirm that all instances behave as expected in boundary scenarios.Once you've double-checked these areas and ensured that any potential differences in rounding behavior are intentional and correctly documented, this comment can be resolved.
CHANGELOG.md (1)
5-7: LGTM! Changelog entry correctly explains the change.The changelog entry clearly describes the addition of quantization to notional value conversion functions, which matches the code changes made in
pyinjective/core/market.py.
| quantized_balue = chain_formatted_value.quantize(Decimal("1"), rounding=ROUND_UP) | ||
| extended_chain_formatted_value = quantized_balue * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") |
There was a problem hiding this comment.
Fix the typo in variable name "quantized_balue".
There's a typo in the variable name "quantized_balue" which should be "quantized_value" to maintain consistency with the naming convention used in the other market classes.
- quantized_balue = chain_formatted_value.quantize(Decimal("1"), rounding=ROUND_UP)
- extended_chain_formatted_value = quantized_balue * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}")
+ quantized_value = chain_formatted_value.quantize(Decimal("1"), rounding=ROUND_UP)
+ extended_chain_formatted_value = quantized_value * Decimal(f"1e{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.
| quantized_balue = chain_formatted_value.quantize(Decimal("1"), rounding=ROUND_UP) | |
| extended_chain_formatted_value = quantized_balue * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") | |
| quantized_value = chain_formatted_value.quantize(Decimal("1"), rounding=ROUND_UP) | |
| extended_chain_formatted_value = quantized_value * Decimal(f"1e{ADDITIONAL_CHAIN_FORMAT_DECIMALS}") |
Summary by CodeRabbit