fix(cex-market-data): coingecko ohlc parsing#203
Conversation
coingecko ohlc format differs to the binance format, so use a freezed union type and convert all values to decimal to standardise
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a union-based OHLC model with Decimal fields and source tagging, updates Binance/CoinGecko providers to pass OhlcSource to CoinOhlc.fromJson, and adjusts repositories/sparkline and tests to use closeTimeMs/closeDecimal and Ohlc.binance constructors. Adds generated Freezed and JSON serialization files for the new model. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Repo as Repository (Binance/CoinGecko)
participant Provider as HTTP Provider
participant Model as CoinOhlc/Ohlc
App->>Repo: fetch OHLC / prices
Repo->>Provider: request klines/ohlc
Provider->>Provider: decode JSON
Provider->>Model: CoinOhlc.fromJson(data, source: binance|coingecko)
Model->>Model: Ohlc.fromKlineArray(..., source)
Model-->>Provider: CoinOhlc{ ohlc: List<Ohlc.variant> }
Provider-->>Repo: CoinOhlc
Repo->>Repo: use closeTimeMs, closeDecimal
Repo-->>App: computed prices/map
sequenceDiagram
autonumber
actor App
participant Spark as SparklineRepository
participant Model as Ohlc entries
App->>Spark: fetchSparkline(...)
Spark->>Model: iterate ohlc list
Note over Spark,Model: Extract closeDecimal<br/>convert to double
Spark-->>App: List<double> sparkline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
Refactors OHLC data handling to use a freezed union type that accommodates different data formats between CoinGecko and Binance APIs, standardizing all price values as Decimal types for improved precision.
- Replaces simple Ohlc class with a freezed union type supporting both CoinGecko and Binance formats
- Converts all numeric price values from double to Decimal for better precision
- Adds source hints and getter extensions to abstract format differences
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/komodo_cex_market_data/lib/src/models/coin_ohlc.dart |
Converts Ohlc from simple class to freezed union type with CoinGecko and Binance variants, adds Decimal support |
packages/komodo_cex_market_data/lib/src/models/coin_ohlc.freezed.dart |
Generated freezed code for the union type |
packages/komodo_cex_market_data/lib/src/models/coin_ohlc.g.dart |
Generated JSON serialization code |
packages/komodo_cex_market_data/lib/src/sparkline_repository.dart |
Updates to use new closeDecimal getter instead of close property |
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart |
Updates to use new closeTimeMs and closeDecimal getters |
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart |
Adds CoinGecko source hint when parsing OHLC data |
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart |
Updates to use new closeTimeMs and closeDecimal getters |
packages/komodo_cex_market_data/lib/src/binance/data/binance_provider.dart |
Adds Binance source hint when parsing OHLC data |
packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart |
Updates test data to use Ohlc.binance constructor with Decimal values |
packages/komodo_cex_market_data/test/binance/binance_repository_test.dart |
Updates test data to use Ohlc.binance constructor with Decimal values |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Visit the preview URL for this PR (updated for commit e07b176): https://komodo-defi-sdk--pr203-bugfix-coingecko-ohl-okznr5tf.web.app (expires Fri, 29 Aug 2025 08:10:07 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f9f5ac39928f333b6e8fcefb7138575e24ed347 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart (1)
153-154: Bug: wrong query parameter key for precisionThe precision argument is being placed under price_change_percentage, which breaks both parameters.
- if (precision != null) 'price_change_percentage': precision, + if (precision != null) 'precision': precision,
🧹 Nitpick comments (10)
packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (2)
138-141: Avoid string round-trip when converting Decimal to doubleUse Decimal.toDouble() to reduce allocations and parse overhead.
- final data = - ohlcData.ohlc - .map((e) => double.parse(e.closeDecimal.toString())) - .toList(); + final data = ohlcData.ohlc + .map((e) => e.closeDecimal.toDouble()) + .toList();
188-191: Same optimization for constant-price sparkline conversionApply the same Decimal.toDouble() change here for consistency and efficiency.
- final constantData = - ohlcData.ohlc - .map((e) => double.parse(e.closeDecimal.toString())) - .toList(); + final constantData = ohlcData.ohlc + .map((e) => e.closeDecimal.toDouble()) + .toList();packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart (1)
164-165: Select the most recent candle; don’t assume.firstis “latest”.If
getCoinOhlcreturns candles in ascending time (Binance typically does),.firstwill yield the oldest item in the range, not the current price you want. Prefer.last, or pick by maxcloseTimeMsto be robust to ordering.Apply this minimal fix:
- return ohlcData.ohlc.first.closeDecimal; + // Use the most recent candle in the returned range. + return ohlcData.ohlc.last.closeDecimal;If ordering isn’t guaranteed by providers, sort by
closeTimeMsfirst.packages/komodo_cex_market_data/lib/src/models/coin_ohlc.g.dart (1)
12-16: Decimal parsing path differs from@DecimalConverter; verify JSON shape.Generated code uses
Decimal.fromJson(json['open'] as String)for OHLC prices, bypassing@DecimalConverter. This requires JSON strings, not numbers. If any serialization/deserialization path emits numeric JSON for these fields, it will break.No change requested in generated code; if needed, force the converter at the source by annotating fields with an explicit
@JsonKey(fromJson: DecimalConverter().fromJson, toJson: DecimalConverter().toJson)incoin_ohlc.dartso the generator doesn’t default toDecimal.fromJson.packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart (3)
30-37: Use the CoinGecko variant in CoinGecko tests for clarity.You’re stubbing
Ohlc.binanceinside a CoinGecko repository test. Functionally OK (thanks to the getters), but confusing. PreferOhlc.coingeckoto match the subject under test.- Ohlc.binance( - open: Decimal.fromInt(100), - high: Decimal.fromInt(110), - low: Decimal.fromInt(90), - close: Decimal.fromInt(105), - openTime: startAt.millisecondsSinceEpoch, - closeTime: endAt.millisecondsSinceEpoch, - ), + Ohlc.coingecko( + timestamp: endAt.millisecondsSinceEpoch, + open: Decimal.fromInt(100), + high: Decimal.fromInt(110), + low: Decimal.fromInt(90), + close: Decimal.fromInt(105), + ),
74-81: Same here: preferOhlc.coingeckoin CoinGecko tests.- Ohlc.binance( + Ohlc.coingecko( + timestamp: endAt.millisecondsSinceEpoch, open: Decimal.fromInt(100), high: Decimal.fromInt(110), low: Decimal.fromInt(90), close: Decimal.fromInt(105), - openTime: startAt.millisecondsSinceEpoch, - closeTime: endAt.millisecondsSinceEpoch, ),
121-128: Consistency nit: use CoinGecko OHLC in CoinGecko tests.- Ohlc.binance( + Ohlc.coingecko( + timestamp: endAt.millisecondsSinceEpoch, open: Decimal.fromInt(100), high: Decimal.fromInt(110), low: Decimal.fromInt(90), close: Decimal.fromInt(105), - openTime: startAt.millisecondsSinceEpoch, - closeTime: endAt.millisecondsSinceEpoch, ),packages/komodo_cex_market_data/lib/src/models/coin_ohlc.dart (3)
39-47: UTC timestamps for synthetic data; also consider Decimal parameter.
- For synthetic candles, use UTC epoch millis (
time.toUtc().millisecondsSinceEpoch) to align with exchange data and downstream UTC grouping.- Optional: accept
Decimal constantValuedirectly to avoid parse/format churn.- final time = startAt.add(Duration(seconds: index * intervalSeconds)); + final time = startAt.add(Duration(seconds: index * intervalSeconds)).toUtc(); ... - openTime: time.millisecondsSinceEpoch, - closeTime: time.millisecondsSinceEpoch, + openTime: time.millisecondsSinceEpoch, + closeTime: time.millisecondsSinceEpoch,If you want to switch to a Decimal param:
- factory CoinOhlc.fromConstantPrice({ + factory CoinOhlc.fromConstantPrice({ required DateTime startAt, required DateTime endAt, required int intervalSeconds, - double constantValue = 1.0, + Decimal constantValue = Decimal.one, }) { ... - high: Decimal.parse(constantValue.toString()), + high: constantValue, ...Also applies to: 53-61
116-172: Robust array parsing with clear erroring; one minor enhancement.
- Conversion helpers are defensive and produce actionable errors.
- Source hint + length heuristic is pragmatic.
Minor enhancement: some Binance-like arrays can be length 12–13+; you already accept
>= 11which is fine. Consider documenting index expectations above the factory (link to API docs) to aid future maintenance.
178-190: Document “closeTimeMs” semantics for CoinGecko variant.
closeTimeMsmaps to the same timestamp asopenTimeMsfor CoinGecko, since their array only carries one timestamp. It’s fine, but deserves a comment to avoid confusion when someone expects distinct open/close times.Optionally add helpers:
extension OhlcDateHelpers on Ohlc { DateTime get closeAtUtc => DateTime.fromMillisecondsSinceEpoch(closeTimeMs, isUtc: true); DateTime get openAtUtc => DateTime.fromMillisecondsSinceEpoch(openTimeMs, isUtc: true); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
packages/komodo_cex_market_data/lib/src/binance/data/binance_provider.dart(2 hunks)packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart(2 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart(1 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/coin_ohlc.dart(4 hunks)packages/komodo_cex_market_data/lib/src/models/coin_ohlc.freezed.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/coin_ohlc.g.dart(1 hunks)packages/komodo_cex_market_data/lib/src/sparkline_repository.dart(2 hunks)packages/komodo_cex_market_data/test/binance/binance_repository_test.dart(1 hunks)packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_and_preview_playground_preview
- GitHub Check: build_and_preview_sdk_example_preview
🔇 Additional comments (8)
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart (1)
507-507: Correct: propagate source to CoinOhlc.fromJsonPassing source: OhlcSource.coingecko aligns with the new union model and prevents mis-parsing.
packages/komodo_cex_market_data/test/binance/binance_repository_test.dart (1)
258-268: Ohlc.binance usage matches current factory signatureI’ve confirmed that the test’s named parameters (
openTime,open,high,low,close,closeTime) exactly align with theconst factory Ohlc.binance({ required int openTime, required Decimal open, required Decimal high, required Decimal low, required Decimal close, required int closeTime, … })signature incoin_ohlc.dart. The optional fields (volume,quoteAssetVolume, etc.) are not passed in tests, which is valid since they’re nullable. No changes are needed here.packages/komodo_cex_market_data/lib/src/binance/data/binance_provider.dart (3)
45-49: Correct: tag Binance OHLC JSON with its sourcefromJson(..., source: OhlcSource.binance) is required with the new union. Good catch.
51-53: Minor consistency tweak acknowledgedError message reformatting is fine; preserves content while improving readability.
119-121: Minor consistency tweak acknowledgedSame here for the 24hr ticker path; no functional change.
packages/komodo_cex_market_data/lib/src/models/coin_ohlc.dart (2)
14-24: Parsing by source + heuristic looks good.Factory now accepts a source hint and gracefully falls back to length-heuristics. This should unblock mixed-provider inputs while keeping strict erroring on invalid shapes.
87-114: Freezed union for OHLC is a solid move.Clear separation of CoinGecko vs Binance payloads, with snake_case JSON, and Decimal fields for precision. This should simplify downstream logic and serialization.
packages/komodo_cex_market_data/lib/src/models/coin_ohlc.freezed.dart (1)
12-36: Generated Freezed union looks consistent with the source.Runtime type switching, equality, copyWith, and JSON glue match the declared variants. No manual changes needed.
There was a problem hiding this comment.
Please retain (and update if needed) the public member documentation in this file and (if applicable) all other files touched by this PR.
Otherwise, looks good, thanks.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on August 22, 2025
Learn more in the Cursor dashboard.
| ids: ['bitcoin'], | ||
| vsCurrency: 'usd', // Should fall back to 'usd', never 'usdt' | ||
| ), | ||
| () => mockProvider.fetchCoinMarketData(ids: ['bitcoin']), |
There was a problem hiding this comment.
Bug: Test Coverage Loss for Stablecoin Mapping
The getCoin24hrPriceChange tests stopped verifying the vsCurrency parameter passed to the provider. This removes coverage for the critical logic that maps stablecoins (like USDT) to their underlying fiat currencies (like USD) and handles fallbacks, meaning a future regression in this mapping might go undetected.
Fixes CoinGecko OHLC parsing with a freezed union type. The Binance and CoinGecko OHLC responses require different structures and parsing, which prevented the CoinGecko API from filling in the gaps.
Before:
After:
Summary by CodeRabbit
New Features
Refactor
Improvements
Style
Tests