Improve price aggregation with multiple sources #58
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhanced price aggregation functionality in the Changes
Sequence DiagramsequenceDiagram
participant AP as AggregatePriceProvider
participant PP1 as PriceProvider1
participant PP2 as PriceProvider2
participant PP3 as PriceProvider3
AP->>PP1: GetPrice()
AP->>PP2: GetPrice()
AP->>PP3: GetPrice()
AP->>AP: computeConsolidatedPrice()
AP->>AP: removeOutliers()
AP->>AP: median()
AP-->>Caller: Return Consolidated Price
Suggested reviewers
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![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)
feeder/priceprovider/aggregateprovider.go (2)
67-71: Consider logging the number of valid prices.
Although the code logs a warning when no valid prices are found, logging the count of valid prices used for consolidation could help with debugging and monitoring.
116-130: Outlier removal threshold might need tuning.
Using1*stddevis an arbitrary cutoff that may be too strict or too lenient depending on market volatility. You might consider making it configurable or adaptive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feeder/priceprovider/aggregateprovider.go(3 hunks)feeder/priceprovider/aggregateprovider_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
feeder/priceprovider/aggregateprovider.go (6)
5-6: Imports look good.
These additional imports (mathandsort) are appropriate for performing calculations such as outlier checks and median computations.
57-63: Good approach for collecting valid prices.
Storing valid prices in a dedicatedallPricesslice improves clarity and modularity.
88-91: Well-documented function purpose.
Having clear doc comments about removing outliers and computing the median clarifies the new aggregation logic.
92-114: Mixed aggregation strategy is consistent with PR objectives.
- Single and two-price cases are handled by straightforward returns/averages.
- For three or more prices, outliers are removed and a median is taken.
This design aligns with the stated goal of stabilizing reported prices.
132-141: Efficient and correct median calculation.
Sorting first and then taking the middle elements is straightforward and mathematically correct for this small dataset scenario.
142-156: Sample variance vs. population variance.
Usinglen(prices) - 1is a standard approach for sample variance. Ensure that this is the intended statistical treatment for your data source (sample-based or population-based).feeder/priceprovider/aggregateprovider_test.go (7)
1-10: Imports are concise and appropriate.
They cover necessary packages (testing,require, etc.) for robust test development.
12-16: Mock provider is straightforward.
This makes the tests more deterministic by controlling returned prices.
17-23: Robust default for non-existent prices.
Returning{Price: -1, Valid: false}cleanly signals invalid data in the tests.
25-34: Tests invalid price scenario.
This scenario validates correct fallback behavior when no valid prices are available.
35-50: Single price test covers minimal valid data flow.
Properly confirms the aggregator returns the single valid price without modification.
52-69: Correct averaging logic for two inputs.
Verifies that the aggregator computes a simple average of two valid prices.
71-115: Thorough outlier removal and median testing.
These tests validate multiple scenarios, including removing a large outlier among three or four providers, confirming the intended behavior.
The current aggregator implementation suffers from a flaw that it uses "random" prices. This leads to high fluctuations in reported prices from the same validator, depending on which source reported the price last and is used in the aggregator priceprovider.
This pull request introduces a consolidation of all prices for the same pair from all sources. It removes outliers (if sources > 2) and uses the median price afterwards.
On <= 2 sources it uses the mean price.
This should lead to better price reporting especially in volatile situations with multiple sources.