fix/add_notional_quantization #292
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant M as Market
participant F as NotionalToChainFormat Method
participant Q as Ceil Operation
M->>F: Call NotionalToChainFormat(humanReadableValue)
F->>Q: Compute quantizedValue = Ceil(chainFormattedValue)
Q-->>F: Return quantizedValue
F->>M: Return string(quantizedValue)
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: 0
🧹 Nitpick comments (2)
client/core/market.go (2)
42-49: Consider adding a comment explaining the rationale for using Ceil()While the implementation is technically sound, it's worth noting that the quantization approach for notional values (using
Ceil()) differs from the approach used in other conversion methods likePriceToChainFormatandQuantityToChainFormat, which useDivRoundwith tick sizes. Adding a comment explaining why ceiling rounding is specifically chosen for notional values would improve code maintainability.func (spotMarket SpotMarket) NotionalToChainFormat(humanReadableValue decimal.Decimal) sdkmath.LegacyDec { decimals := spotMarket.QuoteToken.Decimals chainFormattedValue := humanReadableValue.Mul(decimal.New(1, decimals)) + // Using Ceil() ensures the notional value always meets minimum requirements by rounding up quantizedValue := chainFormattedValue.Ceil() valueInChainFormat, _ := sdkmath.LegacyNewDecFromStr(quantizedValue.String()) return valueInChainFormat }
45-46: Consider adding tests for edge casesThe quantization using
Ceil()might have unexpected effects for certain edge cases, particularly values very close to integers or very small values. Consider adding tests that verify the behavior with these edge cases.// Example test to add (this would go in the appropriate test file, not in this file) func TestNotionalToChainFormatEdgeCases(t *testing.T) { // Setup a test market market := SpotMarket{ QuoteToken: Token{ Decimals: 6, }, } testCases := []struct { name string input string expectedValue string }{ { name: "Value just below integer", input: "10.999999", expectedValue: "11000000", // Expecting ceiling to 11 * 10^6 }, { name: "Exact integer value", input: "10.000000", expectedValue: "10000000", // Should remain at 10 * 10^6 }, { name: "Very small value", input: "0.000001", expectedValue: "1", // Should ceiling to 1 }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { input := decimal.RequireFromString(tc.input) result := market.NotionalToChainFormat(input) if result.String() != tc.expectedValue { t.Errorf("Expected %s, got %s", tc.expectedValue, result.String()) } }) } }Also applies to: 133-134, 230-231
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/core/market.go(3 hunks)
🔇 Additional comments (4)
client/core/market.go (4)
45-46: Consistent implementation for notional quantizationThe changes use
Ceil()to quantize the notional value, which ensures the value is always rounded up to the nearest integer. This is a consistent pattern across all market types and provides a standardized approach for handling notional values.
133-134: Consistent implementation for derivative marketThis implementation correctly applies the same quantization pattern used in the SpotMarket implementation. The change is appropriate and maintains consistency across market types.
230-231: Consistent implementation for binary option marketThis implementation correctly applies the same quantization pattern used in the other market types. The approach is consistent and appropriate.
42-49:Details
❓ Verification inconclusive
Verify impact on existing transactions
The change from using raw values to ceiling-rounded values for notional calculations will result in slightly higher notional values in some cases. Since
MarginToChainFormatdelegates toNotionalToChainFormat, this will also affect margin calculations.Also applies to: 130-137, 227-234
🏁 Script executed:
#!/bin/bash # Description: Search for tests or client code that depends on NotionalToChainFormat to understand the impact # Check for tests that verify NotionalToChainFormat behavior echo "Searching for tests that might be affected by the change:" rg -A 3 -B 3 "NotionalToChainFormat" --type go --type test # Check for other dependencies on NotionalToChainFormat echo "Searching for code that uses NotionalToChainFormat:" rg -A 2 "\.(NotionalToChainFormat|MarginToChainFormat)" --type go | grep -v "func.*NotionalToChainFormat" | grep -v "func.*MarginToChainFormat"Length of output: 2143
Action: Verify Transactional Impact of Rounding Change
The updated implementation of
NotionalToChainFormatnow applies ceiling rounding to the scaled value, meaning that in some cases, notional (and hence margin) values may be slightly higher than before. SinceMarginToChainFormatdelegates toNotionalToChainFormat, this change affects margin calculations as well. Please ensure that:
- Dependent Calculations: All client code and tests (e.g., in
client/core/market_test.go) that rely on the previous raw multiplication behavior are updated to expect the ceiling-rounded values.- Consistency Checks: Verify that test cases accurately reflect this rounding behavior to prevent inadvertent discrepancies in transaction processing.
- Additional Affected Areas: Confirm similar changes and impacts in the code segments at lines 130–137 and 227–234.
Summary by CodeRabbit