feat(market-data): add support for multiple market data providers #145
Conversation
|
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 WalkthroughAdds robust market data architecture: new models (AssetMarketInformation, QuoteCurrency), JSON converters, repository selection/fallback mixin, priority manager, and bootstrap. Refactors Binance/CoinGecko/Komodo repositories to typed APIs (AssetId, QuoteCurrency, Decimal), memoization, batching, and 24h change. Introduces sparkline service, SDK integration (manager rewrite), config, and extensive tests. Removes legacy CEX pair/price models. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Manager as CexMarketDataManager
participant Strat as RepositorySelectionStrategy
participant RepoA as Repo[0]
participant RepoB as Repo[1]
Client->>Manager: fiatPrice(assetId, quoteCurrency)
Manager->>Strat: selectRepository(assetId, quoteCurrency, currentPrice, repos)
Strat-->>Manager: RepoA (or null)
alt Selected repo
Manager->>RepoA: getCoinFiatPrice(assetId, quoteCurrency)
alt Success
RepoA-->>Manager: Decimal price
Manager-->>Client: price (cached)
else Failure
Manager->>RepoB: getCoinFiatPrice(...)
RepoB-->>Manager: Decimal price
Manager-->>Client: price (cached)
end
else No repo supports
Manager-->>Client: error/null (depending on API)
end
sequenceDiagram
participant UI
participant Spark as SparklineRepository
participant Cache as Hive Box
participant Repo1 as BinanceRepository
participant Repo2 as CoinGeckoRepository
UI->>Spark: fetchSparkline(assetId)
Spark->>Cache: read(symbol)
alt Fresh cache
Cache-->>Spark: data
Spark-->>UI: sparkline
else Miss/expired
alt Stablecoin
Spark-->>UI: straightline data
Spark->>Cache: write(data,timestamp)
else Non-stablecoin
Spark->>Repo1: getCoinOhlc(...7d 1d...)
alt Repo1 ok
Repo1-->>Spark: CoinOhlc
else Repo1 fails
Spark->>Repo2: getCoinOhlc(...)
Repo2-->>Spark: CoinOhlc
end
Spark-->>UI: sparkline
Spark->>Cache: write(data,timestamp)
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 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 (
|
|
Visit the preview URL for this PR (updated for commit 51861be): https://komodo-defi-sdk--pr145-bugfix-balance-watch-vj7ppbdl.web.app (expires Thu, 14 Aug 2025 18:51:28 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f9f5ac39928f333b6e8fcefb7138575e24ed347 |
takenagain
changed the title
takenagain
force-pushed
the
bugfix/balance-watcher
branch
from
d5d4872 to
41af442
Compare
Binance CexRepository supports a limited number of symbols and no longer lists KMD price history after the delisting
allows for API-specific tickers to be used on a per-repository basis
Co-authored-by: francoisvw <[email protected]>
takenagain
force-pushed
the
bugfix/balance-watcher
branch
from
41af442 to
c4a57b9
Compare
to manage streaming automatically via emit.forEach rather than manually disposing background streams. fixes the emit after close bug
takenagain
changed the title
use Decimal, use standard interface, add missing 24hr price change functionality
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (2)
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart (2)
294-297: Remove hard-coded test logic from production codeThe hard-coded check for 'test-coin' should not be in production code. Consider filtering test data at the source or using a configuration parameter.
- json.forEach((String coingeckoId, dynamic pricesData) { - if (coingeckoId == 'test-coin') { - return; - } + json.forEach((String coingeckoId, dynamic pricesData) { + // Filter out invalid or test coins if needed via configuration
273-309: Method ignores vsCurrencies parameter and always returns USD pricesThe
fetchCoinPricesmethod accepts avsCurrenciesparameter but always extracts only the 'usd' price at line 300, regardless of what currencies were requested. This will return incorrect data when non-USD currencies are requested.Either:
- Support multiple currencies properly by iterating through vsCurrencies
- Or throw an error if non-USD currencies are requested
- Or document this limitation clearly and rename the parameter
- // TODO(Francois): map to multiple currencies, or only allow 1 vs currency - final price = (pricesData as Map<String, dynamic>)['usd'] as num?; + // For now, only support single currency + if (vsCurrencies.length != 1) { + throw ArgumentError('Only single currency supported'); + } + final currency = vsCurrencies.first; + final price = (pricesData as Map<String, dynamic>)[currency] as num?;
🧹 Nitpick comments (22)
packages/komodo_cex_market_data/.gitignore (1)
70-98: Duplicate ignore rules – prune for clarity
coins_config.jsonandseed_nodes.jsonappear twice (Lines 72 & 92, 93 & 95).
While harmless to Git, the duplication clutters the file and increases merge-conflict surface.-assets/config/coins_config.json -... -assets/config/seed_nodes.jsonKeep each pattern only once.
packages/komodo_defi_sdk/example/pubspec.yaml (1)
10-10: Alignblocversion withflutter_blocto avoid transitive conflicts
flutter_bloc 9.1.1depends onbloc >=9.0.0 <10.0.0, sobloc: ^9.0.0is technically okay, but pinning to the same minor (^9.1.0) keeps the APIs in lock-step and prevents subtle mismatches when new features are used.packages/komodo_defi_sdk/example/lib/blocs/blocs.dart (1)
1-1: Maintain alphabetical export order for quick scanningPlacing
asset_market_info_bloc.dartafterauth_bloc.dartkeeps the list predictable as it grows.-export 'asset_market_info/asset_market_info_bloc.dart'; -export 'auth/auth_bloc.dart'; +export 'auth/auth_bloc.dart'; +export 'asset_market_info/asset_market_info_bloc.dart';packages/komodo_cex_market_data/test/repository_selection_strategy_test.dart (1)
22-64: Test logic is sound but could be enhanced.The test correctly verifies repository selection based on priority. Consider adding more test cases to cover edge scenarios like no supported repositories, equal priority repositories, or different request types.
The AssetId construction could be simplified for test readability:
- final asset = AssetId( - id: 'BTC', - name: 'BTC', - symbol: AssetSymbol(assetConfigId: 'BTC'), - chainId: AssetChainId(chainId: 0), - derivationPath: null, - subClass: CoinSubClass.utxo, - ); + final asset = AssetId.btc(); // If such factory exists + // or create a test helper method for common assetspackages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_provider.dart (2)
29-33: Update documentation to reflect new return type.The example in the documentation still references the old
CexPricetype, but the method now returnsAssetMarketInformation./// Example: /// ```dart - /// final Map<String, CexPrice>? prices = - /// await cexPriceProvider.getLegacyKomodoPrices(); + /// final Map<String, AssetMarketInformation> prices = + /// await komodoPriceProvider.getKomodoPrices(); /// ```
35-56: Enhance error handling for HTTP requests.The method should validate the HTTP response status code and provide more specific error messages for different failure scenarios.
@override Future<Map<String, AssetMarketInformation>> getKomodoPrices() async { final mainUri = Uri.parse(mainTickersUrl); - http.Response res; - String body; - res = await http.get(mainUri); - body = res.body; + final res = await http.get(mainUri); + + if (res.statusCode != 200) { + throw Exception('HTTP ${res.statusCode}: Failed to fetch prices from Komodo API'); + } - final json = jsonDecode(body) as Map<String, dynamic>?; + final json = jsonDecode(res.body) as Map<String, dynamic>?; if (json == null) { throw Exception('Invalid response from Komodo API: empty JSON'); } final prices = <String, AssetMarketInformation>{}; json.forEach((String priceTicker, dynamic pricesData) { prices[priceTicker] = AssetMarketInformation.fromJson( pricesData as Map<String, dynamic>, ).copyWith(ticker: priceTicker); }); return prices; }packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart (1)
112-147: Optimize repository access and improve error UX.The
CoinSparklinewidget has a few areas for improvement:
- The repository instance field creates a new instance on each widget creation
- Error messages could be more user-friendly
- Consider adding loading shimmer for better UX
class CoinSparkline extends StatelessWidget { + const CoinSparkline({required this.coinId, super.key}); + final String coinId; - final SparklineRepository repository = sparklineRepository; - CoinSparkline({required this.coinId, super.key}); @override Widget build(BuildContext context) { return FutureBuilder<List<double>?>( - future: repository.fetchSparkline(coinId), + future: sparklineRepository.fetchSparkline(coinId), builder: (context, snapshot) { if (snapshot.connectionState == ConnectionState.waiting) { - return const SizedBox.shrink(); + return const SizedBox( + width: 130, + height: 35, + child: Center(child: SizedBox( + width: 16, + height: 16, + child: CircularProgressIndicator(strokeWidth: 2), + )), + ); } else if (snapshot.hasError) { - return Text('Error: ${snapshot.error}'); + return const SizedBox( + width: 130, + height: 35, + child: Icon(Icons.show_chart, color: Colors.grey, size: 16), + ); } else if (!snapshot.hasData || (snapshot.data?.isEmpty ?? true)) { return const SizedBox.shrink(); } else {packages/komodo_cex_market_data/lib/src/models/json_converters.dart (1)
25-39: Add documentation for timestamp format assumption.The
TimestampConvertercorrectly handles the conversion, but should document that it expects timestamps in seconds (Unix epoch format).-/// Custom converter for timestamp +/// Custom converter for timestamp (Unix epoch in seconds) class TimestampConverter implements JsonConverter<DateTime?, int?> { const TimestampConverter(); + /// Converts Unix timestamp in seconds to DateTime @override DateTime? fromJson(int? json) { if (json == null) return null; return DateTime.fromMillisecondsSinceEpoch(json * 1000); } + /// Converts DateTime to Unix timestamp in seconds @override int? toJson(DateTime? dateTime) {packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart (1)
97-110: Consider parallelizing stablecoin support tests for better performanceThe sequential iteration through stablecoins could be optimized by running tests in parallel, especially when the list grows.
Consider using
Future.waitto run all support checks concurrently:- for (final stablecoin in usdStablecoins) { - final supports = await repository.supports( - assetId, - stablecoin, - PriceRequestType.currentPrice, - ); - - expect( - supports, - isTrue, - reason: - '${stablecoin.symbol} should be supported via USD mapping', - ); - } + final supportResults = await Future.wait( + usdStablecoins.map((stablecoin) => repository.supports( + assetId, + stablecoin, + PriceRequestType.currentPrice, + )), + ); + + for (var i = 0; i < usdStablecoins.length; i++) { + expect( + supportResults[i], + isTrue, + reason: + '${usdStablecoins[i].symbol} should be supported via USD mapping', + ); + }packages/komodo_cex_market_data/test/src/repository_priority_manager_test.dart (1)
32-39: Consider returning mock data instead of throwing UnimplementedErrorThe
fetch24hrTickermethod throwsUnimplementedError, which could cause test failures if accidentally called. Consider returning mock data instead.@override Future<Binance24hrTicker> fetch24hrTicker( String symbol, { String? baseUrl, }) async { - throw UnimplementedError(); + // Return mock data for testing + return Binance24hrTicker( + symbol: symbol, + priceChange: Decimal.zero, + priceChangePercent: Decimal.zero, + weightedAvgPrice: Decimal.zero, + prevClosePrice: Decimal.zero, + lastPrice: Decimal.zero, + lastQty: Decimal.zero, + bidPrice: Decimal.zero, + bidQty: Decimal.zero, + askPrice: Decimal.zero, + askQty: Decimal.zero, + openPrice: Decimal.zero, + highPrice: Decimal.zero, + lowPrice: Decimal.zero, + volume: Decimal.zero, + quoteVolume: Decimal.zero, + openTime: 0, + closeTime: 0, + firstId: 0, + lastId: 0, + count: 0, + );packages/komodo_defi_sdk/example/lib/widgets/assets/asset_market_info.dart (1)
36-41: Simplify color determination logicThe nullable comparison logic can be simplified for better readability.
- final color = - state.change24h == null - ? null - : state.change24h! >= Decimal.zero - ? Colors.green - : Colors.red; + final color = state.change24h?.let((change) => + change >= Decimal.zero ? Colors.green : Colors.red);Note: If the
letextension is not available, you can keep the current implementation or use:- final color = - state.change24h == null - ? null - : state.change24h! >= Decimal.zero - ? Colors.green - : Colors.red; + final change = state.change24h; + final color = change == null ? null : + (change >= Decimal.zero ? Colors.green : Colors.red);packages/komodo_cex_market_data/lib/src/models/asset_market_information.dart (1)
11-11: Track the TODO for potential model migration.Consider creating an issue to track the migration to
CoinMarketDataor enhancement with additional fields.Would you like me to create an issue to track this TODO item?
packages/komodo_defi_sdk/lib/src/bootstrap.dart (2)
157-176: Consider moving market data initialization to the cex_market_data package.The TODO comment correctly identifies that these repository initializations might be better encapsulated within the
komodo_cex_market_datapackage to improve separation of concerns.Would you like me to help design a factory pattern or initialization module within the market data package to handle these dependencies internally?
187-206: Proper async registration with explicit dependencies.The MarketDataManager registration correctly handles initialization and dependency declaration. Consider making the repository list configurable for better flexibility.
packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (2)
8-18: Address the TODO to eliminate global variables and improve architecture.Global variables make testing difficult and create tight coupling. Consider implementing a dependency injection pattern or factory methods as suggested in the TODO.
Would you like help designing a higher-level abstraction that avoids global state and provides better testability?
126-134: Document the rationale for USDT special handling.The special case for USDT generating constant price data should be documented to explain why stablecoins are treated differently.
Add a comment explaining the business logic:
// USDT and other USD-pegged stablecoins maintain a constant 1:1 price ratio // Generate synthetic constant price data instead of fetching from APIspackages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart (1)
52-70: Implement defensive programming for asset symbol comparison.The asset matching logic uses case-insensitive comparison, which is good. However, consider adding null safety checks and more robust symbol matching.
final supportsAsset = cache.coins.any( (c) => - c.id.toUpperCase() == - assetId.symbol.configSymbol.toUpperCase(), + c.id.trim().toUpperCase() == + assetId.symbol.configSymbol.trim().toUpperCase(), );packages/komodo_cex_market_data/lib/src/cex_repository.dart (1)
138-151: Consider enhancing documentation for new abstract methodsThe new abstract methods would benefit from more detailed documentation about expected behavior and error handling.
For example:
resolveTradingSymbol: Should document what happens if the asset can't be resolved (returns empty string, throws exception, etc.)canHandleAsset: Should clarify when this returns false vs throwing an exceptionsupports: Should document the meaning of eachPriceRequestTypevaluepackages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart (1)
61-80: Consider preventing concurrent cache refreshesMultiple concurrent calls during cache expiry could trigger redundant API fetches. Consider implementing a lock or future-based caching to prevent this.
You could use a
Futureto prevent concurrent fetches:Future<Map<String, AssetMarketInformation>>? _pendingFetch; Future<Map<String, AssetMarketInformation>> _getCachedKomodoPrices() async { // Check if a fetch is already in progress if (_pendingFetch != null) { return _pendingFetch!; } // Check if cache is valid if (_cachedPrices != null && _cacheTimestamp != null) { final now = DateTime.now(); final cacheAge = now.difference(_cacheTimestamp!); if (cacheAge.inMinutes < _cacheLifetimeMinutes) { return _cachedPrices!; } } // Start fetch and store the future _pendingFetch = _cexPriceProvider.getKomodoPrices(); try { final prices = await _pendingFetch!; _cachedPrices = prices; _cacheTimestamp = DateTime.now(); return prices; } finally { _pendingFetch = null; } }packages/komodo_cex_market_data/lib/src/models/quote_currency.dart (1)
67-68: Remove unnecessary @OverRide annotationsThe
@overrideannotations onsymbolanddisplayNamegetters are unnecessary since these properties aren't overriding anything from a base class. The freezed-generated properties are in the concrete implementation classes, not in the baseQuoteCurrencyclass.- /// Get the symbol for this currency - @override + /// Get the symbol for this currency String get symbol { - /// Get the display name for this currency - @override + /// Get the display name for this currency String get displayName {Also applies to: 78-79
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart (1)
169-171: Case-insensitive comparison may cause false positivesThe comparison between trading symbol and fiat currency uses case-insensitive matching, which might incorrectly flag valid pairs as invalid if they happen to have the same letters but different cases.
Consider using the actual resolved symbols:
- if (tradingSymbol.toUpperCase() == fiatCurrencyId.toUpperCase()) { + if (tradingSymbol == fiatCurrencyId) { throw ArgumentError('Coin and fiat coin cannot be the same'); }packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart (1)
241-247: Reduce error message duplicationThe error message is duplicated between logging and the exception.
+ final errorMsg = + 'No repository supports ${assetId.symbol.configSymbol}/$quoteCurrency for current price'; if (repo == null) { - _logger.shout( - 'No repository supports ${assetId.symbol.configSymbol}/$quoteCurrency for current price', - ); - throw StateError( - 'No repository supports ${assetId.symbol.configSymbol}/$quoteCurrency for current price', - ); + _logger.shout(errorMsg); + throw StateError(errorMsg); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/komodo_defi_sdk/example/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (53)
packages/komodo_cex_market_data/.gitignore(1 hunks)packages/komodo_cex_market_data/lib/src/binance/data/binance_provider.dart(3 hunks)packages/komodo_cex_market_data/lib/src/binance/data/binance_provider_interface.dart(3 hunks)packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart(6 hunks)packages/komodo_cex_market_data/lib/src/binance/models/binance_24hr_ticker.dart(1 hunks)packages/komodo_cex_market_data/lib/src/binance/models/binance_24hr_ticker.freezed.dart(1 hunks)packages/komodo_cex_market_data/lib/src/binance/models/binance_24hr_ticker.g.dart(1 hunks)packages/komodo_cex_market_data/lib/src/cex_repository.dart(4 hunks)packages/komodo_cex_market_data/lib/src/coingecko/coingecko.dart(0 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart(12 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart(3 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/sparkline_repository.dart(0 hunks)packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.dart(1 hunks)packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.freezed.dart(1 hunks)packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.g.dart(1 hunks)packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart(1 hunks)packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_provider.dart(3 hunks)packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart(2 hunks)packages/komodo_cex_market_data/lib/src/komodo_cex_market_data_base.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/asset_market_information.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/asset_market_information.freezed.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/asset_market_information.g.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/cex_price.dart(0 hunks)packages/komodo_cex_market_data/lib/src/models/json_converters.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/models.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/quote_currency.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/quote_currency.freezed.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/quote_currency.g.dart(1 hunks)packages/komodo_cex_market_data/lib/src/repository_priority_manager.dart(1 hunks)packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart(1 hunks)packages/komodo_cex_market_data/lib/src/sparkline_repository.dart(1 hunks)packages/komodo_cex_market_data/pubspec.yaml(1 hunks)packages/komodo_cex_market_data/pubspec_overrides.yaml(1 hunks)packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart(1 hunks)packages/komodo_cex_market_data/test/komodo/cex_price_repository_test.dart(1 hunks)packages/komodo_cex_market_data/test/komodo_price_repository_cache_test.dart(1 hunks)packages/komodo_cex_market_data/test/komodo_price_repository_test.dart(1 hunks)packages/komodo_cex_market_data/test/repository_selection_strategy_test.dart(1 hunks)packages/komodo_cex_market_data/test/src/models/quote_currency_test.dart(1 hunks)packages/komodo_cex_market_data/test/src/repository_priority_manager_test.dart(1 hunks)packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_bloc.dart(1 hunks)packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_event.dart(1 hunks)packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_state.dart(1 hunks)packages/komodo_defi_sdk/example/lib/blocs/blocs.dart(1 hunks)packages/komodo_defi_sdk/example/lib/main.dart(2 hunks)packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart(4 hunks)packages/komodo_defi_sdk/example/lib/widgets/assets/asset_market_info.dart(1 hunks)packages/komodo_defi_sdk/example/pubspec.yaml(2 hunks)packages/komodo_defi_sdk/lib/komodo_defi_sdk.dart(1 hunks)packages/komodo_defi_sdk/lib/src/bootstrap.dart(1 hunks)packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart(8 hunks)packages/komodo_defi_sdk/test/market_data_manager_test.dart(1 hunks)packages/komodo_defi_types/lib/src/assets/asset_id.dart(5 hunks)
💤 Files with no reviewable changes (3)
- packages/komodo_cex_market_data/lib/src/coingecko/coingecko.dart
- packages/komodo_cex_market_data/lib/src/coingecko/data/sparkline_repository.dart
- packages/komodo_cex_market_data/lib/src/models/cex_price.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: legacy rpc methods in the komodo defi sdk (located in `/legacy/` subdirectories) intentionally use `...
Learnt from: takenagain
PR: KomodoPlatform/komodo-defi-sdk-flutter#70
File: packages/komodo_defi_rpc_methods/lib/src/rpc_methods/swaps/legacy/cancel_order.dart:8-9
Timestamp: 2025-06-02T12:44:56.953Z
Learning: Legacy RPC methods in the Komodo DeFi SDK (located in `/legacy/` subdirectories) intentionally use `mmrpc: null` in their constructors, unlike modern RPC methods that use `mmrpc: "2.0"`. This is by design and should not be changed to maintain backward compatibility.
Applied to files:
packages/komodo_cex_market_data/pubspec_overrides.yaml
⏰ 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: Deploy Preview
- GitHub Check: build_and_preview_sdk_example_preview
🔇 Additional comments (84)
packages/komodo_cex_market_data/pubspec_overrides.yaml (1)
1-6: Verify local-path overrides won’t leak into CI/publishing pipelinesThe two
path:overrides lock the package to local, unpublished code.
That’s perfect for monorepo development, but:• CI jobs running outside the repo root (e.g.,
dart pub publish --dry-run) will fail to resolve these paths.
• If you ever publish this package, the overrides file is ignored—but forgetting that and relying on local code can mask integration issues.Confirm that:
- CI/test jobs that build this package run from the repo root so the relative paths resolve correctly, and
- A
melos clean && melos bootstrapfollowed bydart pub publish --dry-runpasses.No code change needed if both are true.
packages/komodo_cex_market_data/.gitignore (1)
19-24:.vscode/is ignored even though the template comment suggests otherwise – confirm intentThe upstream Dart/Flutter template keeps this rule commented out so teams can decide whether to version-control launch/task files.
If you actually rely on workspace settings (e.g. recommended extensions, launch configs) you may want to keep the folder; otherwise keeping it in.gitignoreis fine. Please double-check the team policy.packages/komodo_defi_sdk/example/pubspec.yaml (1)
29-30: Pre-release dependency detected – evaluate production readiness
dragon_charts_flutter: 0.1.1-dev.1is a dev/pre-release version.
If the example app is shipped to stores or CI, consider depending on a stable tag or pinning an exact commit to avoid breaking builds when the package is yanked.packages/komodo_defi_sdk/example/lib/main.dart (1)
12-12: Scoped import withshowis 👍 – no action neededLimiting the import to
sparklineRepositorykeeps the namespace clean and helps tree-shaking.packages/komodo_cex_market_data/lib/src/models/models.dart (1)
1-5: Ensure no lingering imports of removedcex_price.dartSince
cex_price.dartis no longer exported, any downstreamimport '.../cex_price.dart'orimport 'models.dart' show CexPricewill break.packages/komodo_cex_market_data/test/komodo/cex_price_repository_test.dart (4)
8-10: LGTM! Good use of dependency injection.The constructor refactoring to use dependency injection improves testability and follows good architectural patterns.
13-13: LGTM! Test group name aligns with method being tested.The rename from
'getPrices'to'getCoinList'accurately reflects the functionality being tested.
18-18: LGTM! Method call updated correctly.The change from
getKomodoPrices()togetCoinList()is consistent with the repository interface refactoring.
22-22: LGTM! Assertion correctly updated for new return type.The change from map-based assertion to list-based assertion using
.any()properly validates the presence of KMD coin in the returned list.packages/komodo_defi_sdk/lib/komodo_defi_sdk.dart (1)
8-9: LGTM! Well-structured currency type exports.The selective export of currency types (
Commodity,Cryptocurrency,FiatCurrency,QuoteCurrency,Stablecoin) provides SDK consumers with a clean interface to the strongly-typed currency system while maintaining proper encapsulation.packages/komodo_cex_market_data/test/komodo_price_repository_test.dart (6)
1-5: LGTM! Comprehensive and appropriate imports.All necessary dependencies are correctly imported for testing with decimal precision, type safety, mocking, and test framework support.
7-7: LGTM! Clean mock definition.The mock class properly implements the
IKomodoPriceProviderinterface for isolated testing.
14-17: LGTM! Proper test setup with dependency injection.The setup correctly initializes the mock provider and repository, enabling isolated testing through dependency injection.
19-26: LGTM! Well-designed test data helper.The
asset()helper function effectively simplifies test data creation with sensible defaults while maintaining flexibility for different asset IDs.
28-43: LGTM! Comprehensive positive test case.The test properly mocks the async provider response and validates that the
supportsmethod correctly identifies supported assets. The use ofStablecoin.usdtdemonstrates integration with the strongly-typed currency system.
45-61: LGTM! Essential negative test case.The test correctly validates that the
supportsmethod returns false when the requested asset (KMD) is not available in the provider's data, ensuring proper support detection logic.packages/komodo_cex_market_data/pubspec.yaml (3)
11-24: LGTM! Well-chosen dependencies for financial data handling.The added dependencies provide excellent support for:
- Precise decimal arithmetic (
decimal)- Immutable data models (
freezed_annotation)- JSON serialization (
json_annotation)- Robust async operations and logging (
async,logging)- Shared type definitions (
komodo_defi_typespath dependency)These are essential for reliable financial market data processing.
28-33: LGTM! Comprehensive development tooling.The dev dependencies provide excellent support for:
- Robust testing with mocks (
mocktail)- Code generation pipeline (
freezed,json_serializable,build_runner)- Enhanced static analysis (
very_good_analysis)These tools will ensure high code quality and maintainability.
11-33: LGTM! Appropriate version constraints.The caret version constraints allow compatible updates while preventing breaking changes. All selected versions appear to be stable releases suitable for production use.
packages/komodo_cex_market_data/lib/src/binance/models/binance_24hr_ticker.dart (4)
1-6: LGTM! Proper imports and code generation setup.The imports correctly include all necessary dependencies for decimal handling, immutable data classes, and custom converters. The part statements properly reference the generated code files.
8-11: LGTM! Well-documented immutable data class.The class declaration properly uses the freezed pattern with clear documentation describing its purpose as a Binance 24hr ticker model.
12-34: LGTM! Comprehensive and type-safe constructor.The factory constructor excellently handles all Binance 24hr ticker fields with:
- Proper use of
@DecimalConverter()for all financial values ensuring precision- Complete field coverage matching Binance API structure
- Required parameters ensuring data integrity
- Clear, descriptive field names
This provides a robust, type-safe representation of market data.
36-38: LGTM! Standard JSON deserialization support.The
fromJsonfactory method correctly follows freezed conventions for JSON deserialization, enabling seamless integration with Binance API responses.packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_event.dart (2)
3-8: LGTM!The abstract base event class follows standard BLoC patterns and properly extends Equatable with appropriate default implementation.
10-17: Well-structured event implementation.The concrete event class properly implements the Equatable pattern by including the asset parameter in props, uses immutable design with const constructor, and has clear naming.
packages/komodo_cex_market_data/test/repository_selection_strategy_test.dart (2)
1-9: LGTM!Test imports and mock class setup follow best practices with appropriate use of mocktail for null-safe Dart testing.
11-21: Good test organization.The test group setup and initialization in setUp() follows testing best practices with clear variable declarations and proper mock initialization.
packages/komodo_cex_market_data/lib/src/binance/data/binance_provider_interface.dart (1)
65-85: Excellent interface documentation and design.The new
fetch24hrTickermethod follows the established patterns in the interface with comprehensive documentation, appropriate parameter types, clear example usage, and consistent error handling approach.packages/komodo_cex_market_data/lib/src/komodo_cex_market_data_base.dart (1)
4-4: Strategic API expansion for multi-repository support.The new exports appropriately expose key abstractions (ID resolution, repository selection, priority management, sparkline access) that enable the enhanced multi-repository architecture while maintaining clean API organization.
Also applies to: 7-9
packages/komodo_cex_market_data/lib/src/binance/models/binance_24hr_ticker.g.dart (1)
1-59: Generated JSON serialization code with proper financial precision.The generated serialization code correctly uses
Decimal.fromJsonfor all financial fields ensuring precision in price and quantity data handling, which is critical for market data applications.Note: This is auto-generated code - ensure the source model annotations are maintained correctly for future regeneration.
packages/komodo_cex_market_data/lib/src/binance/data/binance_provider.dart (3)
5-5: LGTM!The import for the new
Binance24hrTickermodel is correctly added and follows the existing import structure.
39-45: Minor formatting improvements look good.The URI construction formatting has been improved for better readability while maintaining the same functionality.
97-120: Implementation follows established patterns correctly.The new
fetch24hrTickermethod is well-implemented and consistent with existing methods in terms of:
- Parameter handling and query parameter construction
- URI building with base URL fallback
- HTTP response handling and error messaging
- JSON parsing into the appropriate model
The implementation correctly handles the Binance API's
/ticker/24hrendpoint.packages/komodo_cex_market_data/lib/src/models/asset_market_information.g.dart (3)
1-7: Generated code header is properly formatted.The standard generated code header correctly indicates this file should not be manually modified.
9-28: JSON deserialization handles types and nullability correctly.The
_$AssetMarketInformationFromJsonfunction properly:
- Uses appropriate type casting for JSON fields
- Applies custom converters for
Decimal,Timestamp, andCexDataProvidertypes- Handles nullable fields with null-safe conversions
- Uses snake_case JSON keys that align with typical API conventions
30-49: JSON serialization mirrors deserialization correctly.The
_$AssetMarketInformationToJsonfunction consistently uses the same converters and key mappings as the deserialization function, ensuring proper round-trip JSON handling.packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_state.dart (3)
3-8: Well-designed immutable state class.The state class properly uses:
Decimaltypes for financial precision- Nullable fields for optional market data
- Clean constructor with named parameters
10-20: copyWith method implements correct immutability pattern.The
copyWithmethod correctly allows partial state updates while maintaining immutability. The null coalescing operator ensures existing values are preserved when not explicitly updated.
22-24: Equatable implementation is correct.The
propsoverride properly includes all fields for equality comparison, which enables efficient Bloc state change detection.packages/komodo_cex_market_data/test/komodo_price_repository_cache_test.dart (5)
7-7: Mock class properly extends the interface.The mock correctly implements
IKomodoPriceProviderfor testing purposes.
19-26: Helper function creates realistic test data.The
assethelper function creates properly structuredAssetIdinstances with all required fields, making tests more readable and maintainable.
28-59: Comprehensive cache behavior test.This test effectively verifies:
- Cache works across different method calls (
getCoinFiatPriceandgetCoin24hrPriceChange)- Provider is called only once despite multiple data requests
- Both price and change data are cached together
- Expected values are returned correctly
61-100: Cache invalidation test is thorough.The test properly verifies:
- Cache can be manually cleared
- Fresh data is fetched after cache invalidation
- Mock behavior can be changed between calls
- Provider call count is tracked correctly
102-128: Coin list caching test covers important scenario.This test ensures the coin list endpoint benefits from caching, preventing unnecessary API calls for metadata that doesn't change frequently.
packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_bloc.dart (2)
12-18: Bloc constructor and setup look correct.The Bloc properly:
- Extends the correct base class with typed parameters
- Takes required SDK dependency
- Initializes with empty state
- Registers the event handler
22-36: Price and change data fetching is well-implemented.The method correctly:
- Extracts asset from event
- Fetches both fiat price and 24h change using SDK
- Uses USD as quote currency consistently
- Updates state with fetched data
packages/komodo_defi_types/lib/src/assets/asset_id.dart (1)
21-27: LGTM! Formatting improvements enhance readability.The formatting changes consistently improve code readability by properly breaking long expressions across multiple lines and following Dart conventions. No logic changes were made.
Also applies to: 99-101, 120-127, 212-214, 229-235, 239-239
packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.g.dart (1)
1-117: LGTM! Generated serialization code follows correct patterns.The auto-generated JSON serialization code properly:
- Uses
DecimalConverterconsistently for all numeric fields- Handles
DateTimeparsing with ISO8601 format- Implements proper null safety throughout
- Follows expected
json_serializablepatternspackages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_provider.dart (1)
7-10: LGTM! Interface abstraction improves testability.The introduction of
IKomodoPriceProviderinterface enables better dependency injection and testing patterns, which aligns well with the architectural improvements in this PR.packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart (1)
75-78: LGTM! Clean integration of market data components.The addition of
CoinSparklineandAssetMarketInfowidgets enhances the asset item with valuable market visualization. The layout adjustments properly accommodate the new components.packages/komodo_cex_market_data/lib/src/models/json_converters.dart (1)
5-22: LGTM! Robust decimal conversion with proper error handling.The
DecimalConverterimplementation correctly handles null values and parsing errors gracefully. The try-catch approach prevents crashes while allowing data processing to continue.packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart (3)
1-10: LGTM!The imports and mock class declaration follow best practices for unit testing with mocktail.
16-22: LGTM!Good practice disabling memoization during tests to ensure fresh data for each test case.
251-285: LGTM!Comprehensive mapping verification tests covering all stablecoin types and the Turkish Lira special case.
packages/komodo_cex_market_data/test/src/repository_priority_manager_test.dart (2)
257-264: LGTM!Good test ensuring that the sorting method returns a new list without mutating the original, which is an important immutability guarantee.
321-350: LGTM!Comprehensive tests for priority constants, ensuring the expected values are maintained.
packages/komodo_defi_sdk/example/lib/widgets/assets/asset_market_info.dart (1)
68-72: Verify the percentage calculation logicThe function divides the value by 100, which suggests the input is already in percentage form (e.g., 5.5 for 5.5%). Please verify this matches the data format from the API.
If the API returns decimal values (e.g., 0.055 for 5.5%), the division by 100 should be removed:
String _formatChange(Decimal? value) { if (value == null) return '--'; final format = NumberFormat('+#,##0.00%;-#,##0.00%'); - return format.format(value.toDouble() / 100); + return format.format(value.toDouble()); }packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.dart (1)
1-45: LGTM!Excellent refactoring to use
freezedfor code generation andDecimalfor precise numeric handling. This reduces boilerplate, ensures immutability, and improves precision for financial calculations.packages/komodo_cex_market_data/lib/src/binance/models/binance_24hr_ticker.freezed.dart (1)
1-338: Generated code - no manual review neededThis is an auto-generated file from the freezed package. Any issues should be addressed in the source file (
binance_24hr_ticker.dart) rather than in this generated code.packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart (3)
4-17: Well-designed abstraction for platform-specific ID resolution.The interface clearly defines the contract for ID resolution strategies with appropriate method signatures and documentation.
20-47: Correct implementation of Binance ID resolution strategy.The implementation properly handles null and empty values, provides appropriate error messages, and follows the interface contract.
50-77: Consistent implementation for CoinGecko platform.The implementation maintains consistency with the Binance strategy while correctly using CoinGecko-specific fields.
packages/komodo_cex_market_data/lib/src/models/asset_market_information.dart (1)
65-79: Well-implemented JSON converter for the provider enum.The converter properly handles null values and uses the enum's built-in name property for consistent serialization.
packages/komodo_cex_market_data/test/src/models/quote_currency_test.dart (3)
5-102: Comprehensive test coverage for QuoteCurrency parsing and equality.Excellent test organization with thorough coverage of all currency types, case sensitivity, and edge cases.
104-290: Thorough testing of currency-specific behaviors and API mappings.Great attention to detail in testing platform-specific IDs (CoinGecko, Binance) and special cases like Turkish Lira.
292-403: Excellent integration tests ensuring backward compatibility.The tests thoroughly verify that all legacy enum values are supported and API mappings remain consistent.
packages/komodo_defi_sdk/test/market_data_manager_test.dart (3)
17-24: LGTM! Clean asset helper function.The asset helper function provides a clean way to create test AssetId instances with consistent default values.
26-90: Comprehensive test for repository prioritization.The test correctly verifies that the manager prefers the KomodoPriceRepository when it supports the asset, and properly mocks both repositories with different return values to ensure the correct one is called.
92-142: Excellent test coverage for fallback and caching behavior.The test effectively covers:
- Fallback to alternative repository when Komodo doesn't support the asset
- Caching behavior (verifying only one repository call for repeated requests)
- Post-disposal state handling with proper error throwing
The test logic is sound and comprehensive.
packages/komodo_cex_market_data/lib/src/repository_priority_manager.dart (4)
13-17: Well-defined default priority scheme.The priority values are logical with Komodo having highest priority (1), followed by Binance (2), and CoinGecko (3). This aligns with the typical preference for native over third-party data sources.
19-24: Appropriate sparkline-specific prioritization.Prioritizing Binance for sparkline data makes sense due to its typically higher data quality and update frequency for historical price data.
31-33: Robust fallback for unknown repository types.Using 999 as the default priority for unknown repository types ensures they have the lowest priority without breaking the sorting logic.
61-65: Clean and efficient sorting implementation.The sorting creates a new list (avoiding mutation of the original) and sorts by priority values correctly.
packages/komodo_cex_market_data/lib/src/models/asset_market_information.freezed.dart (2)
1-5: Standard generated file header.The generated code header correctly indicates this is auto-generated and should not be modified manually.
18-18: Ignore unformatted Freezed-generated gettersThe
// dart format offdirective at the top ofasset_market_information.freezed.dartintentionally disables formatting for the generated code, which is why all getters appear on one line. This is expected behavior of the Freezed generator and does not require any action.Likely an incorrect or invalid review comment.
packages/komodo_cex_market_data/lib/src/models/quote_currency.g.dart (2)
9-21: Standard JSON serialization for FiatQuoteCurrency.The serialization correctly handles the discriminated union pattern with the
runtimeTypefield and includes all necessary fields.
23-41: Proper nested object serialization.The StablecoinQuoteCurrency correctly handles nested serialization of the
underlyingFiatfield, which is aFiatQuoteCurrencyobject.packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart (3)
8-9: Well-defined request type enumeration.The enum clearly categorizes different types of price requests, which allows for request-specific repository selection logic.
29-40: Robust cache initialization with proper error handling.The cache initialization correctly handles duplicate repository processing and efficiently extracts supported fiat currencies from coin lists.
73-77: Clean cache data structure.The private cache class appropriately encapsulates the cached data with clear field definitions.
packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.freezed.dart (1)
1-4: Generated file properly markedThis freezed-generated file is correctly marked with ignore directives and should not be manually modified.
packages/komodo_cex_market_data/lib/src/models/quote_currency.freezed.dart (1)
1-4: Generated file properly markedThis freezed-generated file for the QuoteCurrency sealed class hierarchy is correctly marked with ignore directives.
packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart (1)
61-80: Well-implemented caching mechanismThe caching implementation with 5-minute lifetime is appropriate for price data, with proper null checks and timestamp validation.
packages/komodo_cex_market_data/lib/src/models/quote_currency.dart (1)
46-46: No changes needed for the Turkish Lira mappingThe CoinGecko ID
'try'is only ever used as a string literal (JSON key and return value) and cannot be parsed as Dart code. It’s covered by existing tests and poses no risk.
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart
Outdated
Show resolved
Hide resolved
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart
Outdated
Show resolved
Hide resolved
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart
Show resolved
Hide resolved
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart
Outdated
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart
Outdated
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart
Outdated
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart
Outdated
Show resolved
Hide resolved
packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (3)
11-19: Avoid global repository instancesGlobal mutable variables make testing difficult and create tight coupling. Consider using dependency injection instead.
-// Global CoinGecko repository instance for backward compatibility -final CoinGeckoRepository _coinGeckoRepository = CoinGeckoRepository( - coinGeckoProvider: CoinGeckoCexProvider(), -); -final BinanceRepository _binanceRepository = BinanceRepository( - binanceProvider: BinanceProvider(), -); - -SparklineRepository sparklineRepository = SparklineRepository(); +// Remove global instances and use dependency injection +class SparklineRepositoryFactory { + static SparklineRepository create({ + List<CexRepository>? repositories, + }) { + repositories ??= [ + BinanceRepository(binanceProvider: const BinanceProvider()), + CoinGeckoRepository(coinGeckoProvider: CoinGeckoCexProvider()), + ]; + return SparklineRepository(repositories: repositories); + } +}
43-43: Use a more specific type for the Hive boxUsing generic
Maptype reduces type safety. Define a specific model for sparkline cache data.
162-169: Critical: Hardcoded AssetId will cause incorrect support checksCreating AssetId with hardcoded
chainId: 0andsubClass: CoinSubClass.utxowill fail for any non-UTXO assets (e.g., ERC-20 tokens). This is a critical issue that will cause the repository to incorrectly report support status.Consider passing proper AssetId from the caller or implementing a symbol-to-AssetId lookup service:
- Future<bool> _supportsAsset(CexRepository repo, String symbol) async { + Future<bool> _supportsAsset( + CexRepository repo, + String symbol, + AssetId? assetId, + ) async { try { - final assetId = AssetId( - id: symbol, - name: symbol, - symbol: AssetSymbol(assetConfigId: symbol), - chainId: AssetChainId(chainId: 0), - derivationPath: null, - subClass: CoinSubClass.utxo, - ); + // Use provided assetId or implement proper lookup + assetId ??= await _assetIdResolver.resolve(symbol); + if (assetId == null) return false;packages/komodo_defi_sdk/lib/src/market_data/repository_fallback_mixin.dart (1)
175-175: Use defensive approach for asset symbol accesspackages/komodo_cex_market_data/lib/src/models/quote_currency.dart (1)
812-831: The underlyingFiat getter still has misleading fallback behaviorThe getter returns
FiatCurrency.usdfor crypto and commodity types after asserting false. In production where assertions are disabled, this will silently return USD which is incorrect - cryptocurrencies and commodities don't have an underlying fiat currency.Consider either throwing an exception or returning null:
- QuoteCurrency get underlyingFiat { + QuoteCurrency? get underlyingFiat { return when( fiat: (symbol, displayName) => this, stablecoin: (symbol, displayName, underlyingFiat) => underlyingFiat, - crypto: (symbol, displayName) { - assert( - false, - 'Cryptocurrency $symbol does not have an underlying fiat currency', - ); - return FiatCurrency.usd; - }, - commodity: (symbol, displayName) { - assert( - false, - 'Commodity $symbol does not have an underlying fiat currency', - ); - return FiatCurrency.usd; - }, + crypto: (symbol, displayName) => null, + commodity: (symbol, displayName) => null, ); }Or if a non-null return is required, throw an exception:
crypto: (symbol, displayName) { - assert( - false, - 'Cryptocurrency $symbol does not have an underlying fiat currency', - ); - return FiatCurrency.usd; + throw UnsupportedError( + 'Cryptocurrency $symbol does not have an underlying fiat currency', + ); }, commodity: (symbol, displayName) { - assert( - false, - 'Commodity $symbol does not have an underlying fiat currency', - ); - return FiatCurrency.usd; + throw UnsupportedError( + 'Commodity $symbol does not have an underlying fiat currency', + ); },packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart (1)
74-79: Consider renaming constructor parameter for consistencyThe constructor parameter
priceRepositoriescould be renamed torepositoriesfor clarity and consistency, as it's the primary repository list for the manager.CexMarketDataManager({ - required List<CexRepository> priceRepositories, + required List<CexRepository> repositories, RepositorySelectionStrategy? selectionStrategy, - }) : _priceRepositories = priceRepositories, + }) : _priceRepositories = repositories, _selectionStrategy = selectionStrategy ?? DefaultRepositorySelectionStrategy();
🧹 Nitpick comments (14)
packages/komodo_cex_market_data/test/models/json_converters_test.dart (2)
76-94: Good coverage for DecimalConverter.toJson with room for enhancement.The tests cover the essential scenarios. Consider adding tests for edge cases like very large/small numbers or numbers with many decimal places if the application handles such values.
104-121: Good basic coverage for TimestampConverter.fromJson but could be more comprehensive.The tests cover essential scenarios and correctly verify the seconds-to-milliseconds conversion. Consider adding tests for negative timestamps, invalid input types, and edge cases to match the thoroughness of the DecimalConverter tests.
packages/komodo_cex_market_data/test/coingecko/coingecko_cex_provider_test.dart (1)
133-136: Appropriate handling of flaky live API tests.Skipping these tests prevents CI instability while preserving test code. Consider creating mocked versions of these tests for reliable unit testing, or moving them to a separate integration test suite.
packages/komodo_cex_market_data/test/binance/binance_repository_test.dart (1)
24-199: Comprehensive mock data setup enables thorough testing.The extensive mock exchange info covers a wide range of trading pairs necessary for testing currency mapping logic. Consider extracting this setup to a test helper function for better maintainability.
packages/komodo_defi_sdk/test/src/market_data/repository_fallback_mixin_test.dart (3)
67-76: Consider more realistic default repository support behaviorThe current setup assumes repositories support all assets by default (lines 69-76). In production, repositories typically have specific limitations. Consider adding tests with more realistic support patterns to ensure the fallback logic handles partial support correctly.
// Setup default supports behavior for all repositories - // (assuming they support all assets unless explicitly set otherwise) + // Setup realistic support behavior - repositories typically have limitations when( () => primaryRepo.supports(any(), any(), any()), - ).thenAnswer((_) async => true); + ).thenAnswer((invocation) async { + // Simulate realistic support - e.g., primary supports most but not all + final assetId = invocation.positionalArguments[0] as AssetId; + return assetId.symbol.assetConfigId != 'UNSUPPORTED'; + });
84-90: Extract magic number for max failure countThe value
3appears to be the max failure threshold but it's hardcoded. Consider extracting this as a named constant or making it configurable to improve test maintainability.+ const maxFailureThreshold = 3; // This should match the mixin's configuration + test('repository becomes unhealthy after max failures', () { // Record failures up to max count - for (int i = 0; i < 3; i++) { + for (int i = 0; i < maxFailureThreshold; i++) { manager.recordRepositoryFailureForTest(primaryRepo); }
494-499: Clarify comment about call countThe comment on line 496 says "Called once for each test" but it's actually called twice total (once in the first test that fails, once in the second test). Consider clarifying this comment to avoid confusion.
verify( () => primaryRepo.getCoinFiatPrice(testAsset), - ).called(2); // Called once for each test + ).called(2); // Called once in first test (fails), once in second testpackages/komodo_cex_market_data/lib/src/bootstrap/market_data_bootstrap.dart (3)
14-18: Improve documentation exampleThe example code shows empty configuration parameters which isn't helpful. Provide a realistic example showing actual configuration options.
/// Example: /// ```dart /// const config = MarketDataConfig( - /// // configuration parameters + /// enableBinance: true, + /// enableCoinGecko: false, + /// customRepositories: [myCustomRepo], + /// selectionStrategy: MyCustomStrategy(), /// ); /// ```
85-89: Consider accepting providers as configuration for better testabilityCreating providers inline (e.g.,
BinanceProvider()on line 87) makes it difficult to inject mock providers for testing. Consider accepting providers through the configuration.class MarketDataConfig { const MarketDataConfig({ this.enableBinance = true, this.enableCoinGecko = true, this.enableKomodoPrice = true, this.customRepositories = const [], this.selectionStrategy, + this.binanceProvider, + this.coinGeckoProvider, + this.komodoPriceProvider, }); // ... existing fields ... + + /// Optional custom Binance provider (uses default if null) + final BinanceProvider? binanceProvider; + + /// Optional custom CoinGecko provider (uses default if null) + final ICoinGeckoProvider? coinGeckoProvider; + + /// Optional custom Komodo price provider (uses default if null) + final IKomodoPriceProvider? komodoPriceProvider; }Then update the registration:
container.registerSingletonAsync<BinanceRepository>( - () async => BinanceRepository(binanceProvider: const BinanceProvider()), + () async => BinanceRepository( + binanceProvider: config.binanceProvider ?? const BinanceProvider(), + ), );
128-139: Document the repository priority ordering rationaleThe repositories are added in a specific order (KomodoPrice → Binance → CoinGecko) but the rationale isn't documented. Add a comment explaining why this priority was chosen.
final repositories = <CexRepository>[]; - // Add repositories in priority order + // Add repositories in priority order: + // 1. KomodoPrice - preferred for [explain reason] + // 2. Binance - fallback for [explain reason] + // 3. CoinGecko - last resort due to [explain reason] if (config.enableKomodoPrice) {packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (1)
127-134: Consider making stablecoin handling more configurableThe hardcoded check for USDT should be generalized to handle all stablecoins that need constant price data.
+ // List of stablecoins that should return constant price + const stablecoinsWithConstantPrice = ['USDT', 'USDC', 'DAI', 'BUSD']; + // Handle USDT special case (constant price) - if (symbol.split('-').firstOrNull?.toUpperCase() == 'USDT') { + final baseSymbol = symbol.split('-').firstOrNull?.toUpperCase(); + if (baseSymbol != null && stablecoinsWithConstantPrice.contains(baseSymbol)) { final interval = endAt.difference(startAt).inSeconds ~/ 500; ohlcData = CoinOhlc.fromConstantPrice(packages/komodo_defi_sdk/lib/src/market_data/repository_fallback_mixin.dart (1)
44-47: Consider maintaining failure history after backoffWhen a repository becomes healthy after the backoff period, the failure count is completely reset. This means a repository that repeatedly fails after each backoff period won't be deprioritized long-term.
Consider maintaining some failure history or implementing exponential backoff periods for repositories with repeated failures.
packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart (1)
141-144: Consider removing or clarifying the mock setup for non-supporting repositoryThe mock setup for
mockCoinGeckoRepo.getCoinFiatPriceat lines 142-144 could be confusing since the comment indicates it should NOT be called. While this doesn't affect test execution (since it won't be called), consider either removing this setup or adding a comment explaining it's there to verify it's never called.- // CoinGecko should NOT be called since it doesn't support the asset - when( - () => mockCoinGeckoRepo.getCoinFiatPrice(supportedAsset), - ).thenAnswer((_) async => Decimal.parse('49999.0')); // Act + // Actpackages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart (1)
137-144: Cache key generation improved but could include protocol for complete uniquenessThe cache key now includes more identifiers (id, chainId, subClass) which significantly reduces collision risk. Consider also including the protocol if assets with identical ids exist across different protocols.
Current implementation is acceptable for most use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/komodo_defi_sdk/example/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
packages/komodo_cex_market_data/.gitignore(1 hunks)packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart(5 hunks)packages/komodo_cex_market_data/lib/src/bootstrap/market_data_bootstrap.dart(1 hunks)packages/komodo_cex_market_data/lib/src/cex_repository.dart(4 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart(12 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart(2 hunks)packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.g.dart(1 hunks)packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart(1 hunks)packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_provider.dart(2 hunks)packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart(2 hunks)packages/komodo_cex_market_data/lib/src/komodo_cex_market_data_base.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/asset_market_information.g.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/cex_coin_pair.dart(0 hunks)packages/komodo_cex_market_data/lib/src/models/json_converters.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/models.dart(1 hunks)packages/komodo_cex_market_data/lib/src/models/quote_currency.dart(1 hunks)packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart(1 hunks)packages/komodo_cex_market_data/lib/src/sparkline_repository.dart(1 hunks)packages/komodo_cex_market_data/pubspec.yaml(1 hunks)packages/komodo_cex_market_data/test/binance/binance_repository_test.dart(1 hunks)packages/komodo_cex_market_data/test/coingecko/coingecko_cex_provider_test.dart(2 hunks)packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart(1 hunks)packages/komodo_cex_market_data/test/komodo_cex_market_data_test.dart(0 hunks)packages/komodo_cex_market_data/test/models/json_converters_test.dart(1 hunks)packages/komodo_cex_market_data/test/models/quote_currency_test.dart(1 hunks)packages/komodo_cex_market_data/test/repository_priority_manager_test.dart(1 hunks)packages/komodo_cex_market_data/test/repository_selection_strategy_test.dart(1 hunks)packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_bloc.dart(1 hunks)packages/komodo_defi_sdk/example/lib/main.dart(2 hunks)packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart(4 hunks)packages/komodo_defi_sdk/example/lib/widgets/assets/asset_market_info.dart(1 hunks)packages/komodo_defi_sdk/example/pubspec.yaml(2 hunks)packages/komodo_defi_sdk/lib/src/bootstrap.dart(2 hunks)packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart(6 hunks)packages/komodo_defi_sdk/lib/src/market_data/repository_fallback_mixin.dart(1 hunks)packages/komodo_defi_sdk/lib/src/sdk/komodo_defi_sdk_config.dart(4 hunks)packages/komodo_defi_sdk/test/market_data_manager_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_selection_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/unsupported_asset_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/repository_fallback_mixin_test.dart(1 hunks)
💤 Files with no reviewable changes (2)
- packages/komodo_cex_market_data/test/komodo_cex_market_data_test.dart
- packages/komodo_cex_market_data/lib/src/models/cex_coin_pair.dart
✅ Files skipped from review due to trivial changes (2)
- packages/komodo_cex_market_data/lib/src/models/asset_market_information.g.dart
- packages/komodo_cex_market_data/lib/src/coingecko/models/coin_market_data.g.dart
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/komodo_defi_sdk/example/lib/main.dart
- packages/komodo_cex_market_data/pubspec.yaml
- packages/komodo_defi_sdk/example/pubspec.yaml
- packages/komodo_cex_market_data/test/repository_selection_strategy_test.dart
- packages/komodo_cex_market_data/lib/src/komodo_cex_market_data_base.dart
- packages/komodo_defi_sdk/example/lib/blocs/asset_market_info/asset_market_info_bloc.dart
- packages/komodo_cex_market_data/lib/src/models/models.dart
- packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart
- packages/komodo_defi_sdk/lib/src/bootstrap.dart
- packages/komodo_cex_market_data/lib/src/models/json_converters.dart
- packages/komodo_defi_sdk/test/market_data_manager_test.dart
- packages/komodo_defi_sdk/example/lib/widgets/assets/asset_market_info.dart
- packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_provider.dart
- packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart
- packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-07T11:01:44.548Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-defi-sdk-flutter#145
File: packages/komodo_cex_market_data/lib/src/models/quote_currency.dart:477-477
Timestamp: 2025-08-07T11:01:44.548Z
Learning: In freezed union classes with sealed types like QuoteCurrency, explicit type casts (e.g., `FiatCurrency.usd as FiatQuoteCurrency`) are necessary when passing static const values between union variants, even when the types should be compatible. This is due to how freezed generates the union class code and handles type inference.
Applied to files:
packages/komodo_cex_market_data/test/models/quote_currency_test.dartpackages/komodo_defi_sdk/lib/src/market_data/repository_fallback_mixin.dartpackages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dartpackages/komodo_cex_market_data/lib/src/models/quote_currency.dartpackages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart
📚 Learning: 2025-06-02T12:44:56.953Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-defi-sdk-flutter#70
File: packages/komodo_defi_rpc_methods/lib/src/rpc_methods/swaps/legacy/cancel_order.dart:8-9
Timestamp: 2025-06-02T12:44:56.953Z
Learning: Legacy RPC methods in the Komodo DeFi SDK (located in `/legacy/` subdirectories) intentionally use `mmrpc: null` in their constructors, unlike modern RPC methods that use `mmrpc: "2.0"`. This is by design and should not be changed to maintain backward compatibility.
Applied to files:
packages/komodo_defi_sdk/lib/src/sdk/komodo_defi_sdk_config.dart
📚 Learning: 2025-06-02T13:47:25.814Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-defi-sdk-flutter#70
File: packages/komodo_defi_rpc_methods/lib/src/rpc_methods/swaps/orders_history_by_filter.dart:76-79
Timestamp: 2025-06-02T13:47:25.814Z
Learning: The orders_history_by_filter API in Komodo DeFi expects float64/double format values for price and volume filter fields (from_price, to_price, from_volume, to_volume), though decimal string format could potentially work as well according to the API documentation.
Applied to files:
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dartpackages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart
⏰ 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). (1)
- GitHub Check: build_and_preview_sdk_example_preview
🔇 Additional comments (67)
packages/komodo_cex_market_data/test/models/json_converters_test.dart (5)
1-4: LGTM! Clean imports and good package structure.The imports are appropriate and follow Dart conventions. The test structure is well-organized.
5-11: LGTM! Proper test setup with good use of late initialization.The test group setup follows Dart testing best practices with appropriate use of
lateandconstconstructor.
13-74: Excellent comprehensive test coverage for DecimalConverter.fromJson.The tests cover all relevant scenarios including edge cases, type variations, and error handling. The graceful handling of invalid inputs by returning null is appropriate for a JSON converter.
97-102: LGTM! Consistent setup pattern with DecimalConverter.The setup follows the same clean pattern established for the DecimalConverter tests.
123-133: LGTM! Adequate coverage for TimestampConverter.toJson.The tests correctly verify the milliseconds-to-seconds conversion and handle null input appropriately.
packages/komodo_cex_market_data/test/coingecko/coingecko_cex_provider_test.dart (4)
38-43: LGTM! Dynamic timestamps improve test robustness.Using relative dates instead of fixed timestamps ensures tests remain valid over time and won't break due to API historical data limits.
63-94: Valuable boundary testing for large time ranges.This test effectively validates the multi-chunk functionality for large date ranges while staying within API constraints. The 343-day range is a good choice for testing splitting logic without hitting limits.
96-120: Excellent validation testing for API constraints.This test ensures proper error handling for historical data access limits, preventing runtime failures and providing clear feedback when users exceed the 365-day constraint.
122-131: Good consistency in validation across fetch methods.This test ensures both
fetchCoinMarketChartandfetchCoinOhlcconsistently enforce the 365-day historical data limit.packages/komodo_defi_sdk/lib/src/sdk/komodo_defi_sdk_config.dart (4)
2-2: Clean import addition.Properly imports the required
MarketDataConfigtype from the market data package.
12-12: Proper constructor parameter with sensible default.Using
const MarketDataConfig()as default provides good out-of-the-box configuration while allowing customization.
33-34: Well-documented field with proper immutability.The field declaration follows good practices with clear documentation and
finalmodifier.
43-43: Consistent copyWith implementation.The
copyWithmethod properly handles the new field with null-aware assignment, maintaining consistency with existing patterns.Also applies to: 56-56
packages/komodo_cex_market_data/test/repository_priority_manager_test.dart (6)
15-162: Well-structured mock implementations for test isolation.The mock classes provide comprehensive coverage of the required interfaces without external dependencies.
TestUnknownRepositoryis particularly useful for testing edge cases with unknown repository types.
164-180: Clean test setup with proper initialization.Using
latevariables withsetUp()provides clean test isolation and ensures proper initialization of repository instances for each test.
182-198: Comprehensive priority assignment testing.Tests cover all known repository types and properly verify the fallback behavior for unknown repositories (priority 999).
200-231: Good testing of specialized priority schemes.The sparkline priority tests demonstrate the flexibility of having different priority orders for different data types, with proper fallback handling.
264-343: Excellent sorting functionality testing with immutability verification.The sorting tests provide comprehensive coverage including the crucial verification that original lists remain unmodified. This ensures functional programming principles and prevents unintended side effects.
345-375: Important validation of priority constants.Testing the actual priority values in the constant maps ensures the priority system behaves as expected and provides regression protection against accidental changes.
packages/komodo_cex_market_data/.gitignore (1)
1-110: Comprehensive .gitignore following Flutter/Dart best practices.The expanded .gitignore properly covers development tools, build artifacts, platform-specific files, and CI/CD outputs. The organization with comments makes it maintainable and clear.
packages/komodo_cex_market_data/test/binance/binance_repository_test.dart (4)
1-23: Excellent test setup with proper mocking.Good use of mocktail for clean mocking, and disabling memoization in tests ensures predictable behavior and test isolation.
200-404: Thorough currency mapping logic testing.Comprehensive coverage of USD/EUR mapping, stablecoin fallbacks, and edge cases. This testing ensures the repository can handle various quote currencies by mapping to supported Binance pairs.
406-559: Effective integration testing of price fetching with mapping.Tests verify that currency mapping is properly applied in actual price fetching operations, with good verification of the correct trading pairs being used.
561-606: Valuable testing of critical internal mapping logic.Testing the internal
_mapFiatCurrencyToBinancemethod is justified here as it documents complex mapping behavior and ensures the core currency translation logic works correctly.packages/komodo_defi_sdk/test/src/market_data/repository_fallback_mixin_test.dart (2)
1-33: Test setup looks good!The mock classes and test manager are properly structured for testing the mixin functionality.
114-270: Comprehensive fallback logic testingExcellent test coverage for the fallback scenarios including primary success, fallback on failure, and handling of total failure cases.
packages/komodo_cex_market_data/test/models/quote_currency_test.dart (4)
5-102: Well-structured QuoteCurrency testsComprehensive test coverage for parsing, equality, and string representation. The design prevents symbol ambiguity across different currency types, which is good for type safety.
104-147: Thorough FiatCurrency testingGood coverage of special cases including Turkish Lira handling and USD to USDT mapping for Binance.
149-203: Excellent Stablecoin test coverageParticularly good testing of the underlying fiat mapping for all USD-pegged stablecoins.
298-386: Excellent backward compatibility testingThe comprehensive testing of all legacy enum values ensures smooth migration from the old implementation. This is critical for maintaining compatibility.
packages/komodo_cex_market_data/test/coingecko/coingecko_repository_test.dart (3)
24-158: Excellent OHLC date range handling testsComprehensive testing of the 365-day API limit including edge cases. The tests properly verify request splitting behavior.
160-386: Comprehensive currency support testingExcellent coverage of currency support scenarios including positive and negative cases. The test for unsupported underlying fiat (lines 305-339) is particularly important.
441-596: Critical mapping behavior properly testedThese tests verify the essential behavior that stablecoins always map to their underlying fiat currency for API calls, never to the stablecoin symbol itself. This is crucial for correct price data retrieval.
packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (1)
53-114: Well-implemented fallback and caching logicGood implementation with cache expiry checks, repository fallback, and optimization to cache null results to prevent repeated failed attempts.
packages/komodo_cex_market_data/lib/src/cex_repository.dart (5)
1-4: LGTM! Clean import additions support the type system improvements.The new imports for
Decimal,PriceRequestType, andAssetIdare necessary for the enhanced type safety and precision improvements in this interface refactor.
48-55: Excellent type safety improvement with strongly typed parameters.The updated method signature using
AssetIdandQuoteCurrencyinstead of string-based symbol pairs significantly improves type safety and eliminates potential string manipulation errors.
78-110: Precision and type safety improvements are well-implemented.The migration from
doubletoDecimalreturn types and strongly typed parameters (AssetId,QuoteCurrency) addresses critical precision concerns in financial calculations and improves API contract clarity.
112-136: Documentation correctly updated for abstract method.The documentation now properly states that "Subclasses must provide their own implementation" instead of incorrectly mentioning UnimplementedError behavior, which addresses the previous review feedback.
138-204: Well-designed new interface methods enhance repository capabilities.The addition of
resolveTradingSymbol,canHandleAsset, andsupportsmethods provides essential functionality for:
- Platform-specific symbol resolution
- Quick asset compatibility checks
- Comprehensive support validation including fiat currencies and request types
The method signatures and documentation are clear and consistent with the overall architecture.
packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart (6)
1-15: Excellent architectural improvements with interface adoption and inheritance.The migration to extend
CexRepositoryand use theIKomodoPriceProviderinterface significantly improves the architecture by:
- Ensuring consistent API contracts across repositories
- Enabling better dependency injection and testing
- Following interface segregation principles
17-32: Robust caching implementation with excellent concurrent request handling.The caching mechanism demonstrates several best practices:
- Reasonable 5-minute cache lifetime for market data freshness
- Concurrent fetch protection via
_pendingFetchprevents redundant API calls- Proper cache invalidation and error handling
- Clean separation of concerns with dedicated cache management methods
Also applies to: 65-93, 227-237
95-118: Excellent resolution of historical price handling concern.The implementation now correctly validates date requests and throws
UnsupportedErrorfor historical dates (older than 1 hour), which properly addresses the previous review feedback about misleading behavior when returning current prices for all historical dates.
210-225: Smart async cache population approach for synchronous method constraints.The implementation cleverly addresses the cache population concern by:
- Triggering asynchronous cache population when cache is null
- Returning false immediately (appropriate for sync method)
- Including silent error handling to prevent unhandled exceptions
- Ensuring subsequent calls benefit from populated cache
This is a reasonable compromise given the synchronous method signature constraints.
33-45: Comprehensive and well-implemented new method additions.All new method implementations demonstrate good practices:
getCoinOhlccorrectly indicates unsupported functionality with clear error messagegetCoin24hrPriceChangeincludes proper null checking and error handlingresolveTradingSymbolfollows consistent uppercase symbol patternsupportsmethod provides thorough validation of asset, fiat currency, and request type compatibilityAlso applies to: 120-144, 192-208
153-190: Consistent implementation patterns across all repository methods.The remaining methods maintain excellent consistency with:
- Proper cache utilization for performance
- Clear error messages for missing data
- Appropriate data transformations (building coin lists from prices)
- Type-safe parameter handling throughout
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart (6)
1-32: Consistent architectural improvements align with system-wide patterns.The adoption of:
ICoinGeckoProviderinterface for better dependency injectionAsyncMemoizerfor controlled caching behaviorIdResolutionStrategyfor platform-specific symbol resolution- Configurable memoization via constructor parameter
These changes maintain consistency with other repository implementations and improve testability.
50-77: Well-implemented memoization with proper cache management.The memoization approach demonstrates good practices:
- Configurable via constructor flag for testing flexibility
- Clear warning about API rate limiting without memoization
- Caches both coin list and fiat currencies for comprehensive support
- Uppercase normalization ensures consistent fiat currency lookups
80-137: Excellent enhancement handling CoinGecko API limitations with intelligent batching.The enhanced OHLC implementation addresses real-world API constraints by:
- Automatically detecting requests exceeding 365-day CoinGecko limits
- Intelligently splitting large requests into sequential batches
- Properly aggregating results while maintaining data integrity
- Clear error messages for invalid request parameters
This significantly improves usability for historical data analysis while respecting API boundaries.
166-184: Excellent fix for currency-specific price extraction.The implementation now correctly extracts prices for the requested currency by:
- Converting
currentPriceto JSON map for dynamic access- Using
mappedFiatIdas the key instead of hardcoded 'usd'- Including comprehensive error handling for missing data
- Proper
Decimalparsing for precisionThis properly addresses the previous review concern about always returning USD prices.
187-242: Sophisticated batch processing handles complex historical data requirements.The implementation demonstrates excellent handling of historical price requests:
- Input validation prevents invalid coin/fiat combinations
- Buffer days around requested range ensure data coverage
- Intelligent batching respects CoinGecko API limits
- Proper result aggregation from multiple OHLC requests
- Clean conversion from OHLC data to daily price points
140-147: New methods follow consistent patterns and include proper validation.The new method implementations demonstrate good practices:
- Strategy delegation in
resolveTradingSymbolandcanHandleAssetpromotes maintainabilitygetCoin24hrPriceChangeincludes same validation patterns as other price methodssupportsmethod efficiently uses cached data for validation- Consistent error handling and input validation across all methods
Also applies to: 244-288
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart (4)
3-40: Robust architectural improvements with excellent resilience features.The enhancements demonstrate strong engineering practices:
- Multiple API endpoints (
binanceApiEndpointarray) for fallback resilience- Interface-based dependency injection with
IBinanceProvider- Integrated logging via
Loggerfor better diagnostics- Strategy pattern for ID resolution consistency
- Configurable memoization for performance control
57-72: Excellent retry logic implementation across multiple endpoints.The retry mechanism demonstrates robust error handling:
- Systematic iteration through all available API endpoints
- Proper exception preservation and meaningful error messages
- Consistent retry pattern applied across all network-dependent methods
- Graceful degradation when primary endpoints fail
This significantly improves service reliability and user experience.
Also applies to: 114-133, 246-260
38-50: Consistent memoization implementation aligns with repository architecture.The memoization approach maintains consistency with other repositories:
AsyncMemoizerfor thread-safe caching behavior- Configurable via constructor for testing flexibility
- Comprehensive caching of both coin list and fiat currencies
- Clear documentation about API rate limiting concerns
Also applies to: 63-67
135-143: New methods demonstrate consistent patterns and robust implementation.The new method implementations excel in:
- Strategy delegation for
resolveTradingSymbolandcanHandleAsset- Consistent validation patterns in
getCoin24hrPriceChange(same coin/fiat checks)- Efficient use of cached data in
supportsmethod- Unified retry logic and error handling across all network operations
Also applies to: 230-260, 288-302
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_cex_provider.dart (5)
9-57: Excellent interface introduction promotes better architecture.The
ICoinGeckoProviderinterface provides:
- Clear contracts for all CoinGecko API operations
- Better dependency injection support for testing and modularity
- Comprehensive coverage of market data, historical data, and price operations
- Consistent method signatures across implementations
The implementation correctly adopts the interface with proper
@overrideannotations.
192-347: Sophisticated batching logic handles CoinGecko API limitations elegantly.The enhanced market chart fetching demonstrates excellent engineering:
- Automatic request splitting for date ranges exceeding 365 days
- Upfront validation of historical data access limits with clear error messages
- Intelligent result combination with duplicate data point removal
- Proper timestamp-based deduplication at chunk boundaries
- Comprehensive error handling throughout the process
This significantly improves historical data fetching capabilities while respecting API constraints.
437-466: Excellent resolution of price data validation concerns.The enhanced price parsing completely addresses the previous review feedback by:
- Explicit null/empty price validation with meaningful exceptions
- Warning logs for data quality issues using proper logging
- Comprehensive error messages including coin ID and problematic values
- No silent fallbacks to
Decimal.zerothat could mask problemsThis approach properly surfaces data quality issues for monitoring and debugging.
3-3: Good validation improvements prevent invalid API usage.The enhancements provide better API constraint handling:
- OHLC days parameter validation prevents exceeding CoinGecko's 365-day limit
- Clear error messages explain API limitations to developers
- Proper logging integration for diagnostic capabilities
Decimalimport supports precision improvements throughoutAlso applies to: 7-7, 70-70, 484-490
50-50: Return type migration aligns with unified data model approach.The change from
Map<String, CexPrice>toMap<String, AssetMarketInformation>supports:
- Unified data models across different providers
- Richer market data representation capabilities
- Consistency with the broader architectural refactor
- Better integration with the enhanced type system
Also applies to: 408-408, 462-465
packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart (3)
1-14: LGTM!Mock classes and imports are properly structured for the test suite.
48-93: LGTM!Test setup is comprehensive with proper mock initialization and test data preparation.
95-511: Excellent test coverage!The test suite provides comprehensive coverage of repository filtering edge cases including:
- Support-based filtering
- Error handling
- Unhealthy repository fallback
- Exception resilience
The tests are well-structured with clear setup, act, and assert phases.
packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart (4)
179-288: LGTM!Price fetching methods properly utilize the mixin's fallback logic with appropriate caching and error handling.
290-324: LGTM!The
priceChange24hmethod correctly implements caching and uses the mixin's fallback logic. Previous logging issues have been resolved.
326-383: LGTM!The
fiatPriceHistorymethod efficiently handles cached and missing dates separately, minimizing unnecessary API calls.
385-396: LGTM!The dispose method properly cleans up all resources including timer, caches, and mixin health data.
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_selection_test.dart
Show resolved
Hide resolved
...ages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart
Outdated
Show resolved
Hide resolved
packages/komodo_defi_sdk/test/src/market_data/edge_cases/unsupported_asset_test.dart
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the market data repositories to support multiple providers and resolves issues with unsupported coins being queried. It introduces a robust, multi-repository market data system with support for Binance, CoinGecko, and Komodo price sources, along with advanced quote currency handling and repository fallback strategies.
- Replaces single repository design with multi-repository architecture supporting Binance, CoinGecko, and Komodo
- Adds comprehensive asset cache key system and repository selection strategies
- Implements repository health tracking and fallback mechanisms with retry limits
Reviewed Changes
Copilot reviewed 79 out of 80 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/komodo_defi_types/test/asset_cache_key_test.dart | Comprehensive tests for asset cache key equality, hashing, and performance benchmarks |
| packages/komodo_defi_types/lib/src/transactions/fee_info.freezed.dart | Auto-generated code with formatting changes |
| packages/komodo_defi_types/lib/src/public_key/new_address_state.g.dart | Auto-generated JSON serialization code with improved formatting |
| packages/komodo_defi_types/lib/src/public_key/new_address_state.freezed.dart | Auto-generated freezed code with formatting optimizations |
| packages/komodo_defi_types/lib/src/public_key/confirm_address_details.g.dart | Auto-generated JSON serialization with formatting improvements |
| packages/komodo_defi_types/lib/src/public_key/confirm_address_details.freezed.dart | Auto-generated freezed code with formatting changes |
| packages/komodo_defi_types/lib/src/assets/asset_id.dart | Adds base cache key prefix extension for AssetId |
| packages/komodo_defi_types/lib/src/assets/asset_cache_key.g.dart | Auto-generated JSON serialization for AssetCacheKey |
| packages/komodo_defi_types/lib/src/assets/asset_cache_key.freezed.dart | Auto-generated freezed code for AssetCacheKey |
| packages/komodo_defi_types/lib/src/assets/asset_cache_key.dart | New AssetCacheKey model with canonical string conversion utilities |
| packages/komodo_defi_types/lib/komodo_defi_types.dart | Exports new AssetCacheKey class |
| packages/komodo_defi_sdk/test/src/market_data/edge_cases/unsupported_asset_test.dart | Tests for handling unsupported assets and repository fallback behavior |
| packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart | Tests for retry limits and anti-spam protection mechanisms |
| packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart | Tests for repository support filtering and health tracking |
| packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_selection_test.dart | Tests for repository selection strategy edge cases |
| packages/komodo_defi_sdk/test/market_data_manager_test.dart | Comprehensive tests for market data manager with fallback functionality |
| packages/komodo_defi_sdk/lib/src/sdk/komodo_defi_sdk_config.dart | Adds market data configuration support |
| packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart | Refactored to use multi-repository architecture with fallback mixin |
| packages/komodo_defi_sdk/lib/src/bootstrap.dart | Updates bootstrap to use new market data architecture |
| packages/komodo_defi_sdk/lib/komodo_defi_sdk.dart | Exports market data quote currency types |
| packages/komodo_defi_sdk/example/pubspec.yaml | Adds dependencies for market data display (bloc, dragon_charts_flutter) |
| packages/komodo_defi_sdk/example/lib/widgets/assets/asset_market_info.dart | New widget displaying asset market information |
| packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart | Enhanced asset item with sparkline charts and market info |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
packages/komodo_defi_sdk/test/src/market_data/edge_cases/unsupported_asset_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (12)
packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart (1)
130-132: Add null safety check for configSymbol.The current implementation could throw a null pointer exception if
configSymbolis null. Other strategies handle null values properly.Apply this fix:
List<String> getIdPriority(AssetId assetId) { - return [assetId.symbol.configSymbol].where((id) => id.isNotEmpty).toList(); + return [assetId.symbol.configSymbol].where((id) => id != null && id.isNotEmpty).cast<String>().toList(); }packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (2)
11-18: Avoid global mutable state for repository instancesThe global repository instances make testing difficult and can lead to state management issues. Consider using dependency injection to provide these repositories.
Move these repository instances inside a service locator or pass them through dependency injection:
-// Global CoinGecko repository instance for backward compatibility -final CoinGeckoRepository _coinGeckoRepository = CoinGeckoRepository( - coinGeckoProvider: CoinGeckoCexProvider(), -); -final BinanceRepository _binanceRepository = BinanceRepository( - binanceProvider: const BinanceProvider(), -); - -SparklineRepository sparklineRepository = SparklineRepository(); +// Consider using a service locator pattern +class MarketDataServices { + static late final SparklineRepository sparklineRepository; + + static void initialize({ + CoinGeckoRepository? coinGeckoRepo, + BinanceRepository? binanceRepo, + }) { + final defaultCoinGecko = coinGeckoRepo ?? CoinGeckoRepository( + coinGeckoProvider: CoinGeckoCexProvider(), + ); + final defaultBinance = binanceRepo ?? BinanceRepository( + binanceProvider: const BinanceProvider(), + ); + sparklineRepository = SparklineRepository( + repositories: [defaultBinance, defaultCoinGecko], + ); + } +}
55-56: Use a specific type for the Hive box to improve type safetyUsing a generic
Maptype reduces type safety and makes the code harder to maintain.Consider creating a dedicated model:
@HiveType(typeId: X) // Replace X with an appropriate type ID class SparklineCacheEntry { @HiveField(0) final List<double>? data; @HiveField(1) final DateTime timestamp; SparklineCacheEntry({required this.data, required this.timestamp}); }Then update the box type:
-Box<Map<String, dynamic>>? _box; +Box<SparklineCacheEntry>? _box;packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart (2)
95-118: Historical price limitation is properly handledThe implementation correctly validates that historical dates (more than 1 hour old) are not supported and throws an appropriate error. The current price is returned for all valid dates as documented.
216-231: canHandleAsset implementation handles cache state correctlyThe method now properly handles the case where the cache is not populated by triggering an asynchronous cache population without blocking. This addresses the previous concern about returning false when the cache is null.
packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart (2)
53-77: LGTM! Error handling properly implementedThe error handling now correctly logs failures and rethrows exceptions, addressing the previous concern about silently failing with empty lists.
280-300: Remove or replace debugger statementThe
debugger()statement at line 330-334 in the previous review is not present in this code, so this concern has been addressed.packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart (2)
149-164: Missing silent USD fallback issueThe previous concern about silently falling back to USD when the requested currency is not supported appears to have been addressed - the code now uses the
fiatCurrency.coinGeckoIdmapping without a fallback.
166-184: Price extraction correctly uses mapped fiat currencyThe
_extractPriceFromResponsemethod now correctly extracts the price for the requested currency by accessingcurrentPriceMap[mappedFiatId]instead of hardcoding USD. This properly addresses the previous concern about always returning USD prices.packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart (3)
74-78: Constructor parameter name inconsistency detected.The constructor parameter
repositoriesdiffers from the field name_priceRepositories, which could lead to confusion. Consider aligning them for better clarity.CexMarketDataManager({ - required List<CexRepository> repositories, + required List<CexRepository> priceRepositories, RepositorySelectionStrategy? selectionStrategy, -}) : _priceRepositories = repositories, +}) : _priceRepositories = priceRepositories,
141-146: Consider more unique cache key generation to avoid collisions.The cache key uses only
baseCacheKeyPrefixwhich might not be unique enough for assets with the same symbol but different protocols or chains.The current implementation already uses
assetId.baseCacheKeyPrefixwhich should include protocol information. However, verify that this prefix contains sufficient uniqueness identifiers:String _getCacheKey( AssetId assetId, { DateTime? priceDate, QuoteCurrency quoteCurrency = Stablecoin.usdt, }) { final basePrefix = assetId.baseCacheKeyPrefix; + // Ensure basePrefix includes protocol/chain for uniqueness return canonicalCacheKeyFromBasePrefix(basePrefix, { 'quote': quoteCurrency.symbol, 'kind': 'price', if (priceDate != null) 'ts': priceDate.millisecondsSinceEpoch, }); }
101-101: Critical: Missing method invocation in timer callback.The timer callback passes the method reference
_clearCachesinstead of invoking it with_clearCaches(). This prevents the cache from being cleared.- _cacheTimer = Timer.periodic(_cacheClearInterval, (_) => _clearCaches()); + _cacheTimer = Timer.periodic(_cacheClearInterval, (_) => _clearCaches());
🧹 Nitpick comments (20)
packages/komodo_cex_market_data/test/coingecko/coingecko_cex_provider_test.dart (6)
4-4: Clarify how to enable live tests: bool.fromEnvironment requires -D/--dart-define, not shell env varsUsing bool.fromEnvironment reads a compile-time define. The current skip message suggests setting a shell env var, which won’t work. Update the message to show the correct invocation.
Apply this diff to update the message (see lines 142-146):
- : 'Live API tests are skipped by default. Enable with RUN_LIVE_API_TESTS=true', + : 'Live API tests are skipped by default. Enable with -DRUN_LIVE_API_TESTS=true (dart test) ' + 'or --dart-define=RUN_LIVE_API_TESTS=true (flutter test).',Optional: If you want to also support shell env vars (runtime), switch to a runtime check (requires importing dart:io) and drop const:
Outside the selected range:
import 'dart:io'; final bool _runLiveApiTests = const bool.fromEnvironment('RUN_LIVE_API_TESTS') || (Platform.environment['RUN_LIVE_API_TESTS']?.toLowerCase() == 'true');
10-12: Remove empty setUp block to reduce noiseThe setUp adds no behavior. Drop it to keep tests concise.
- setUp(() { - // Additional setup goes here. - }); + // No additional setup required.
38-65: Solid use of bounded time window; consider minor robustness tweaksGood choice to use a recent window (7 to 3 days). To avoid rare edge-cases around data availability and network hiccups, consider:
- Asserting chronological order of returned timestamps if guaranteed.
- Using a slightly wider window (e.g., 10 to 2 days) to reduce flakiness.
113-122: Make error assertion robust for async implementationsIf fetchCoinMarketChart ever moves the validation past an await, the current synchronous closure may stop catching the error. Use an async closure so the assertion holds for both sync and async throws.
- expect( - () => provider.fetchCoinMarketChart( + expect( + () async => provider.fetchCoinMarketChart( id: 'bitcoin', vsCurrency: 'usd', fromUnixTimestamp: fromUnixTimestamp, toUnixTimestamp: toUnixTimestamp, ), throwsA(isA<ArgumentError>()), );
126-135: Use async closure for exception assertion for consistency and future-proofingSame rationale as above: make the assertion resilient to either synchronous or asynchronous validation paths.
- expect( - () => provider.fetchCoinOhlc('bitcoin', 'usd', 400), + expect( + () async => provider.fetchCoinOhlc('bitcoin', 'usd', 400), throwsA(isA<ArgumentError>()), );
137-140: Good call-out about flakiness; consider adding per-test retry for live endpointsIf flakiness continues, a lightweight retry (1–2 attempts with small delay) around the network call sites in tests can help stabilize CI without hiding real failures.
If you want, I can draft a small helper to wrap live calls with a retry for these tests.
packages/komodo_cex_market_data/test/models/json_converters_test.dart (1)
156-162: Verify the error handling behavior and consider more specific assertionsThe test verifies that invalid input types throw an error when invoked dynamically, but the assertion is quite generic. Consider testing for more specific error types if the converter is expected to throw particular exceptions.
- expect( - () => dynConverter.fromJson('1691404800'), - throwsA(isA<Error>()), - ); + expect( + () => dynConverter.fromJson('1691404800'), + throwsA(isA<TypeError>()), + );packages/komodo_cex_market_data/test/binance/binance_test_helpers.dart (1)
9-9: Consider using a fixed timestamp for deterministic tests.Using
DateTime.now().millisecondsSinceEpochintroduces non-determinism in tests, which can make debugging and reproducibility challenging.For test helpers, consider using a fixed timestamp by default:
- serverTime: serverTime ?? DateTime.now().millisecondsSinceEpoch, + serverTime: serverTime ?? 1640995200000, // Fixed timestamp: 2022-01-01 00:00:00 UTCThis ensures tests are deterministic while still allowing flexibility when a specific timestamp is needed.
Also applies to: 178-178
packages/komodo_defi_types/lib/src/assets/asset_cache_key.dart (1)
20-32: Consider validating empty keys in custom fields.The
canonicalCustomFieldsSuffixfunction correctly sorts keys and handles empty maps. However, it doesn't validate against empty string keys which could lead to malformed canonical strings like{=value|key2=value2}.Consider adding validation:
String canonicalCustomFieldsSuffix(Map<String, Object?> customFields) { if (customFields.isEmpty) { return '{}'; } final keys = customFields.keys.toList()..sort(); final parts = <String>[]; for (final key in keys) { + if (key.isEmpty) { + throw ArgumentError('Custom field keys cannot be empty'); + } parts.add('$key=${customFields[key]}'); } return '{${parts.join('|')}}'; }packages/komodo_cex_market_data/lib/komodo_cex_market_data.dart (1)
1-4: Consider updating the library documentation.The generic placeholder documentation doesn't describe the actual functionality of this market data library. Consider updating it to reflect the library's purpose.
Apply this diff to improve the documentation:
-/// Support for doing something awesome. -/// -/// More dartdocs go here. +/// Komodo CEX market data library for fetching and managing cryptocurrency market data. +/// +/// Provides support for multiple market data providers with fallback capabilities, +/// repository selection strategies, and robust error handling.packages/komodo_defi_sdk/test/market_data_manager_test.dart (1)
493-496: Verify the expected call count for primaryRepo.The comment states "Called once for each test" but verifies
.called(2). This seems to be accounting for calls from two different test runs within the same test method. Consider clarifying the comment to avoid confusion.Apply this diff to clarify the comment:
- ).called(2); // Called once for each test + ).called(2); // Called once for maxTotalAttempts=1 test and once for maxTotalAttempts=2 testpackages/komodo_cex_market_data/test/repository_fallback_mixin_test.dart (1)
443-499: Consider adding edge case tests for maxTotalAttempts.While the test covers maxTotalAttempts with values 1 and 2, consider adding tests for edge cases like 0 (should fail immediately) and a value equal to the number of repositories.
Add this test case after the existing maxTotalAttempts test:
test('handles maxTotalAttempts edge cases', () async { // Setup mocks (same as above) when( () => mockStrategy.selectRepository( assetId: any(named: 'assetId'), fiatCurrency: any(named: 'fiatCurrency'), requestType: any(named: 'requestType'), availableRepositories: any(named: 'availableRepositories'), ), ).thenAnswer((_) async => primaryRepo); when( () => primaryRepo.getCoinFiatPrice( any(), fiatCurrency: any(named: 'fiatCurrency'), ), ).thenThrow(Exception('Always fails')); // Test with maxTotalAttempts = 0 should fail immediately expect( () => manager.tryRepositoriesInOrder( testAsset, Stablecoin.usdt, PriceRequestType.currentPrice, (repo) => repo.getCoinFiatPrice(testAsset), 'test', maxTotalAttempts: 0, ), throwsA(isA<Exception>()), ); // Verify no repository was called verifyNever(() => primaryRepo.getCoinFiatPrice(any())); verifyNever(() => fallbackRepo.getCoinFiatPrice(any())); });packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart (1)
140-183: Consider adding error details for debugging.While the error handling shows a chart icon, consider logging the error for debugging purposes without exposing it to users.
Add error logging to help with debugging:
} else if (snapshot.hasError) { + // Log error for debugging but don't expose to users + debugPrint('Failed to fetch sparkline for ${widget.assetId.id}: ${snapshot.error}'); return const SizedBox( width: 130, height: 35, child: Icon(Icons.show_chart, color: Colors.grey, size: 16), );packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart (1)
76-122: Consider adding configSymbol as a fallback for CoinGecko.Unlike the Binance strategy which falls back to configSymbol, the CoinGecko strategy only uses coinGeckoId. Consider adding configSymbol as a fallback for consistency and better resilience.
Apply this diff to add configSymbol as a fallback:
List<String> getIdPriority(AssetId assetId) { final coinGeckoId = assetId.symbol.coinGeckoId; + final configSymbol = assetId.symbol.configSymbol; if (coinGeckoId == null || coinGeckoId.isEmpty) { _logger.warning( 'Missing coinGeckoId for asset ${assetId.symbol.configSymbol}, ' 'falling back to configSymbol. This may cause API issues.', ); } return [ coinGeckoId, + configSymbol, ].where((id) => id != null && id.isNotEmpty).cast<String>().toList(); }packages/komodo_defi_sdk/test/src/market_data/edge_cases/unsupported_asset_test.dart (1)
490-506: Consider adding@visibleForTestingannotationThe test helper methods expose internal mixin functionality for testing. Consider using the
@visibleForTestingannotation to make the testing intent clearer.+import 'package:meta/meta.dart'; /// Test helper class that exposes the mixin methods for testing class TestManager with RepositoryFallbackMixin { // ... existing code ... // Expose the mixin method for testing + @visibleForTesting Future<T> testTryRepositoriesInOrder<T>( // ... existing parameters ... ) { // ... existing implementation ... } // Expose the maybe version for testing + @visibleForTesting Future<T?> testTryRepositoriesInOrderMaybe<T>( // ... existing parameters ... ) { // ... existing implementation ... } }packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart (1)
86-91: Consider exact symbol matching alongside case-insensitive comparisonThe current implementation only performs case-insensitive comparison on the coin ID. Consider also checking the coin symbol field for more robust matching.
final supportsAsset = cache.coins.any( - (c) => c.id.toUpperCase() == assetId.symbol.configSymbol.toUpperCase(), + (c) => c.id.toUpperCase() == assetId.symbol.configSymbol.toUpperCase() || + c.symbol.toUpperCase() == assetId.symbol.configSymbol.toUpperCase(), );packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart (1)
193-234: Consider adding circuit breaker pattern for persistent failuresWhile the current health tracking and backoff strategy is good, consider implementing a circuit breaker pattern that completely disables a repository after repeated failures to prevent unnecessary retry attempts.
Consider enhancing the failure tracking with a circuit breaker state machine:
enum CircuitState { closed, open, halfOpen } class CircuitBreaker { CircuitState state = CircuitState.closed; int consecutiveFailures = 0; DateTime? lastFailureTime; static const int failureThreshold = 5; static const Duration cooldownPeriod = Duration(minutes: 10); bool canAttempt() { switch (state) { case CircuitState.closed: return true; case CircuitState.open: if (DateTime.now().difference(lastFailureTime!).compareTo(cooldownPeriod) > 0) { state = CircuitState.halfOpen; return true; } return false; case CircuitState.halfOpen: return true; } } void recordSuccess() { consecutiveFailures = 0; state = CircuitState.closed; } void recordFailure() { consecutiveFailures++; lastFailureTime = DateTime.now(); if (consecutiveFailures >= failureThreshold) { state = CircuitState.open; } } }This would provide more intelligent failure handling and reduce unnecessary network calls to consistently failing repositories.
packages/komodo_cex_market_data/lib/src/bootstrap/market_data_bootstrap.dart (1)
143-176: Repository priority order should be configurableThe hardcoded repository priority order (KomodoPrice → Binance → CoinGecko) may not be optimal for all use cases. Different applications might prefer different priorities based on their specific requirements (e.g., preferring Binance for its liquidity data).
Consider making the repository order configurable via MarketDataConfig:
class MarketDataConfig { const MarketDataConfig({ this.enableBinance = true, this.enableCoinGecko = true, this.enableKomodoPrice = true, this.customRepositories = const [], this.selectionStrategy, this.binanceProvider, this.coinGeckoProvider, this.komodoPriceProvider, + this.repositoryPriority = const [ + RepositoryType.komodoPrice, + RepositoryType.binance, + RepositoryType.coinGecko, + ], }); // ... existing fields ... + + /// The priority order for repository selection + final List<RepositoryType> repositoryPriority; } + +/// Enum representing available repository types +enum RepositoryType { + komodoPrice, + binance, + coinGecko, +}Then update the
buildRepositoryListmethod to respect this configuration:static Future<List<CexRepository>> buildRepositoryList( GetIt container, MarketDataConfig config, ) async { final repositories = <CexRepository>[]; + final availableRepos = <RepositoryType, CexRepository>{}; + + // Build available repositories + if (config.enableKomodoPrice) { + availableRepos[RepositoryType.komodoPrice] = + await container.getAsync<KomodoPriceRepository>(); + } + if (config.enableBinance) { + availableRepos[RepositoryType.binance] = + await container.getAsync<BinanceRepository>(); + } + if (config.enableCoinGecko) { + availableRepos[RepositoryType.coinGecko] = + await container.getAsync<CoinGeckoRepository>(); + } + + // Add repositories in configured priority order + for (final type in config.repositoryPriority) { + final repo = availableRepos[type]; + if (repo != null) { + repositories.add(repo); + } + } - // Add repositories in priority order: - // 1) KomodoPrice — preferred primary source. It is tailored to the - // Komodo ecosystem, aggregates curated pricing, and aligns IDs with our - // asset model, which typically yields the most consistent coverage. - // 2) Binance — highly reliable centralized exchange with deep liquidity - // and robust OHLC endpoints; good quality prices but narrower asset - // coverage than CoinGecko for long-tail tokens. - // 3) CoinGecko — broadest asset coverage via aggregation across venues, - // but subject to stricter rate limits and occasional data gaps; used as - // a last resort. - if (config.enableKomodoPrice) { - repositories.add(await container.getAsync<KomodoPriceRepository>()); - } - - if (config.enableBinance) { - repositories.add(await container.getAsync<BinanceRepository>()); - } - - if (config.enableCoinGecko) { - repositories.add(await container.getAsync<CoinGeckoRepository>()); - } // Add any custom repositories repositories.addAll(config.customRepositories); return repositories; }packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart (1)
170-191: Optimize by reusing cached coin listThe
getCoinListmethod rebuilds the coin list from cached prices on every call, even when the prices haven't changed. Consider caching the coin list alongside the prices.Cache the coin list when prices are fetched:
Future<Map<String, AssetMarketInformation>> _getCachedKomodoPrices() async { // ... existing cache check and fetch logic ... try { final prices = await _pendingFetch!; _cachedPrices = prices; _cacheTimestamp = DateTime.now(); + // Update coin list cache when prices are refreshed + _updateCoinListCache(prices); return prices; } finally { _pendingFetch = null; } } + void _updateCoinListCache(Map<String, AssetMarketInformation> prices) { + _cachedCoinsList = prices.values + .map( + (e) => CexCoin( + id: e.ticker, + symbol: e.ticker, + name: e.ticker, + currencies: const <String>{'USD', 'USDT'}, + source: 'komodo', + ), + ) + .toList(); + _cachedFiatCurrencies = {'USD', 'USDT'}; + } @override Future<List<CexCoin>> getCoinList() async { - if (_cachedCoinsList != null) { - return _cachedCoinsList!; + // Ensure prices are cached first + if (_cachedCoinsList == null) { + await _getCachedKomodoPrices(); } - final prices = await _getCachedKomodoPrices(); - _cachedCoinsList = - prices.values - .map( - (e) => CexCoin( - id: e.ticker, - symbol: e.ticker, - name: e.ticker, - currencies: const <String>{'USD', 'USDT'}, - source: 'komodo', - ), - ) - .toList(); - _cachedFiatCurrencies = {'USD', 'USDT'}; - return _cachedCoinsList!; + return _cachedCoinsList ?? []; }packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart (1)
10-10: Consider implementing the TODO for streaming support.The TODO comment mentions adding streaming support for price updates, which would be valuable for real-time price monitoring.
Would you like me to create an issue to track the implementation of streaming support for price updates? This could provide real-time price updates to consumers without polling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
packages/komodo_cex_market_data/lib/komodo_cex_market_data.dart(1 hunks)packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart(5 hunks)packages/komodo_cex_market_data/lib/src/bootstrap/market_data_bootstrap.dart(1 hunks)packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart(2 hunks)packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart(1 hunks)packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart(2 hunks)packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart(1 hunks)packages/komodo_cex_market_data/lib/src/repository_selection_strategy.dart(1 hunks)packages/komodo_cex_market_data/lib/src/sparkline_repository.dart(1 hunks)packages/komodo_cex_market_data/test/binance/binance_repository_test.dart(1 hunks)packages/komodo_cex_market_data/test/binance/binance_test_helpers.dart(1 hunks)packages/komodo_cex_market_data/test/coingecko/coingecko_cex_provider_test.dart(1 hunks)packages/komodo_cex_market_data/test/models/json_converters_test.dart(1 hunks)packages/komodo_cex_market_data/test/repository_fallback_mixin_test.dart(1 hunks)packages/komodo_defi_sdk/.gitignore(1 hunks)packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart(4 hunks)packages/komodo_defi_sdk/lib/src/bootstrap.dart(2 hunks)packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart(5 hunks)packages/komodo_defi_sdk/test/market_data_manager_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_selection_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart(1 hunks)packages/komodo_defi_sdk/test/src/market_data/edge_cases/unsupported_asset_test.dart(1 hunks)packages/komodo_defi_types/lib/komodo_defi_types.dart(1 hunks)packages/komodo_defi_types/lib/src/assets/asset_cache_key.dart(1 hunks)packages/komodo_defi_types/lib/src/assets/asset_cache_key.freezed.dart(1 hunks)packages/komodo_defi_types/lib/src/assets/asset_cache_key.g.dart(1 hunks)packages/komodo_defi_types/lib/src/assets/asset_id.dart(1 hunks)packages/komodo_defi_types/lib/src/public_key/confirm_address_details.freezed.dart(3 hunks)packages/komodo_defi_types/lib/src/public_key/confirm_address_details.g.dart(1 hunks)packages/komodo_defi_types/lib/src/public_key/new_address_state.freezed.dart(3 hunks)packages/komodo_defi_types/lib/src/public_key/new_address_state.g.dart(1 hunks)packages/komodo_defi_types/lib/src/transactions/fee_info.freezed.dart(8 hunks)packages/komodo_defi_types/test/asset_cache_key_test.dart(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/komodo_defi_types/lib/src/public_key/new_address_state.g.dart
- packages/komodo_defi_types/lib/src/assets/asset_cache_key.g.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/komodo_defi_types/lib/src/assets/asset_id.dart
- packages/komodo_cex_market_data/test/binance/binance_repository_test.dart
- packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart
- packages/komodo_defi_sdk/lib/src/bootstrap.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-26T21:42:19.450Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-defi-sdk-flutter#63
File: packages/komodo_defi_types/lib/src/transactions/fee_info.dart:238-246
Timestamp: 2025-05-26T21:42:19.450Z
Learning: In KDF v2.4.0-beta, there's an asymmetric protocol for Tendermint fees: the backend returns "Tendermint" format in responses but expects "CosmosGas" format in requests from clients. The FeeInfo implementation correctly handles this by using fromJson() for "Tendermint" (backend responses) and toJson() for "CosmosGas" (client requests).
Applied to files:
packages/komodo_defi_types/lib/src/transactions/fee_info.freezed.dart
⏰ 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). (3)
- GitHub Check: build_and_preview_sdk_example_preview
- GitHub Check: build_and_preview_playground_preview
- GitHub Check: Cursor Bugbot
🔇 Additional comments (68)
packages/komodo_defi_types/lib/src/public_key/confirm_address_details.g.dart (2)
10-12: Formatting-only change; FromJson remains correctNo behavioral change. Note: the strict cast
as Stringwill throw if the key is missing or null. Ensure the source model marksexpectedAddressas non-null/required (or make it nullable) to avoid runtime cast errors if upstream JSON is ever incomplete.
15-16: Formatting-only change; ToJson remains correctSerialization key remains snake_case and matches the field; output remains unchanged.
packages/komodo_cex_market_data/test/coingecko/coingecko_cex_provider_test.dart (3)
14-24: LGTM: fetchCoinList live integration assertion is minimal and validBasic shape checks and non-empty assertion are appropriate for a live integration test.
26-36: LGTM: fetchCoinMarketData live integration assertion is appropriateType and non-empty checks are sufficient here.
67-99: LGTM: Large-window within-limit test aligns with provider’s chunking strategyThis effectively validates chunked fetching within the 365-day constraint and asserts non-empty aggregates.
packages/komodo_defi_types/lib/src/public_key/confirm_address_details.freezed.dart (1)
1-149: Auto-generated Freezed file - no manual changes requiredThis is an auto-generated Freezed file that has been updated due to changes in the Freezed code generator version or configuration. The formatting and structural changes (inline annotations, CopyWith reorganization, condensed operator implementations) are all part of the standard Freezed generation output and don't require manual intervention.
The functional behavior remains unchanged - the model still correctly handles JSON serialization/deserialization for the
expected_addressfield and provides the standard Freezed utilities (copyWith, equality, hashCode, toString).packages/komodo_defi_types/lib/src/public_key/new_address_state.freezed.dart (5)
1-12: Generated code should not be modified or reviewed.This is an auto-generated Freezed file as indicated by the header comments. Any formatting or structural changes should be made to the source
.dartfile or the Freezed/build_runner configuration, not directly to this generated file. These changes will be overwritten the next time code generation runs.
19-24: Formatting changes in generated code.The getter definitions have been collapsed onto single lines, which appears to be a result of regenerating the Freezed code with different formatting settings. This is acceptable as it's generated code that maintains the same API.
30-43: Compact formatting of generated equality and string methods.The
operator ==,hashCode, andtoStringmethods have been reformatted to a more compact inline style. This maintains the same functionality while reducing line count in the generated code.
69-79: Inline formatting of copyWith implementation.The
callmethod implementation has been compacted into fewer lines with inline conditional expressions. This is a formatting change in generated code that preserves functionality.
87-107: Updated class structure maintains API compatibility.The
_NewAddressStateclass has been restructured with:
- A const constructor with required
statusfield- Final field declarations moved inline
- Updated JSON factory and serialization wiring
These changes maintain the public API while aligning with the updated Freezed code generation patterns used throughout the PR.
packages/komodo_cex_market_data/test/models/json_converters_test.dart (13)
1-11: LGTM! Well-structured test setupThe test file imports and setup are properly organized with appropriate dependencies and clear test structure using
groupandsetUp.
13-25: LGTM! Comprehensive null and valid input handlingThe tests properly verify that the converter handles null inputs, empty strings, and valid numeric strings correctly. The use of
Decimal.parsefor expected values ensures precision matching.
27-41: LGTM! Thorough numeric type coverageThe tests effectively cover different numeric input types (int, double, num) ensuring the converter handles various JSON number representations that might come from different data sources.
43-56: LGTM! Edge case handling for numeric valuesGood coverage of edge cases including negative numbers and zero values (both numeric and string representations), which are common in financial data.
58-73: LGTM! Robust error handling for invalid inputsExcellent defensive programming - the tests verify that invalid input types (invalid strings, booleans, lists, maps) are handled gracefully by returning null instead of throwing exceptions.
76-93: LGTM! Complete toJson coverageThe
toJsontests properly cover null handling, basic conversion, zero values, and negative numbers. The bidirectional conversion testing ensures data integrity.
95-104: LGTM! Excellent precision testing for large valuesThe test handles very large decimal values and includes a crucial round-trip test to ensure precision is preserved during serialization/deserialization cycles. The comment about decimal normalization is helpful.
106-112: LGTM! Comprehensive small value precision testingThis test ensures that very small decimal values with many fractional places are preserved exactly, which is critical for financial applications dealing with cryptocurrencies that may have very small unit values.
116-133: LGTM! Well-structured timestamp conversion testingThe
TimestampConvertertests properly verify null handling and basic timestamp-to-DateTime conversion with clear documentation of the expected timestamp value.
135-146: LGTM! Comprehensive edge case coverage for timestampsGood coverage of edge cases including zero timestamp (Unix epoch) and negative timestamps (pre-epoch dates), ensuring the converter handles the full range of possible timestamp values.
148-154: LGTM! Upper bound timestamp testingTesting with very large timestamps near the upper bound of valid DateTime values ensures the converter works correctly across the full range of possible dates.
165-174: LGTM! Complete toJson testing for timestampsThe timestamp
toJsontests properly cover null handling and DateTime-to-timestamp conversion, ensuring bidirectional conversion works correctly.
177-178: LGTM! Clean test file structureThe test file is well-organized and complete with proper closing of all test groups.
packages/komodo_cex_market_data/test/binance/binance_test_helpers.dart (1)
1-193: Well-structured test helpers with comprehensive coverage.The test helpers provide good coverage of different scenarios:
buildComprehensiveExchangeInfocovers multiple stablecoins and major cryptocurrenciesbuildMinimalExchangeInfoprovides a simple test case- Both functions allow optional server time override for flexibility
The symbol selection includes relevant trading pairs that would be commonly used in market data scenarios.
packages/komodo_defi_types/lib/src/transactions/fee_info.freezed.dart (9)
1-11: LGTM! Standard Freezed generated file header.The file header with generated code warnings and coverage ignores is appropriate for auto-generated Freezed files.
80-112: Consistent with Freezed generation patterns for UTXO fixed variant.The changes to
FeeInfoUtxoFixedfollow the new pattern with:
- Explicit
overrideannotations for inherited fields- Constructor ending with
: super._()- Condensed equality, hashCode, and toString implementations
The variant-specific copyWith is properly typed.
150-180: Consistent implementation for UTXO per-kilobyte variant.The
FeeInfoUtxoPerKbytevariant follows the same structural pattern asFeeInfoUtxoFixed, maintaining consistency across the codebase.
218-254: ETH gas variant correctly handles optional totalGasFee field.The
FeeInfoEthGasimplementation properly manages the optionaltotalGasFeefield with appropriate null handling in equality checks and the copyWith implementation usingfreezedsentinel value.
294-332: EIP-1559 variant properly structured with all required fields.The
FeeInfoEthGasEip1559variant correctly includes all EIP-1559 specific fields (maxFeePerGas,maxPriorityFeePerGas) and maintains consistency with the optionaltotalGasFeehandling pattern.
373-409: QRC20 gas variant maintains consistent structure.The
FeeInfoQrc20Gasimplementation follows the established pattern with appropriate field names (gasLimitinstead ofgas) and optionaltotalGasFeehandling.
449-482: CosmosGas variant correctly implements required fields.The variant properly includes all required fields for Cosmos gas calculation without optional overrides, maintaining consistency with the expected protocol requirements mentioned in the retrieved learnings.
521-554: Tendermint variant aligns with the asymmetric protocol pattern.The
FeeInfoTendermintvariant includes bothamountandgasLimitfields, which aligns with the retrieved learning about the asymmetric protocol where backend returns "Tendermint" format in responses. The structure is consistent with the expected format for backend responses.
16-44: No architectural change; Freezed mixin is expected
Themixin _$FeeInfoyou’re seeing in the generated file is the normal Freezed pattern and does not replace or weaken the sealed‐class semantics defined in yourfee_info.dart. The base-levelcopyWithreturning$FeeInfoCopyWith<FeeInfo>is by design—it delegates to variant-specific implementations under the hood.
- A search for
.when,.map, or manualswitchonFeeInfofound no usages in the codebase.You can safely ignore this concern.
Likely an incorrect or invalid review comment.
packages/komodo_defi_types/lib/komodo_defi_types.dart (1)
10-10: LGTM! Export addition aligns with the new market data architecture.The export of
asset_cache_key.dartproperly exposes the newAssetCacheKeymodel to consumers of the package, which is essential for the multi-provider market data support.packages/komodo_defi_types/lib/src/assets/asset_cache_key.dart (4)
6-18: LGTM! Well-structured data model for cache keys.The
AssetCacheKeyclass is properly designed as an immutable Freezed data class with sensible defaults forcustomFields. The JSON serialization support is appropriately included.
34-44: LGTM! Canonical key generation is deterministic and well-structured.The function properly constructs a stable canonical representation using underscore separators and delegates to
canonicalCustomFieldsSuffixfor the custom fields portion.
46-53: LGTM! Efficient canonical key construction with base prefix.The function correctly combines the pre-computed base prefix with the canonical custom fields suffix, avoiding redundant string operations.
55-64: LGTM! Clean extension providing canonical string representation.The extension method properly delegates to
canonicalCacheKeyFromPartswith all required fields, maintaining consistency with the standalone functions.packages/komodo_defi_types/lib/src/assets/asset_cache_key.freezed.dart (1)
1-166: Generated code looks correct.This is auto-generated Freezed code that properly implements the
AssetCacheKeydata class with value semantics, JSON serialization, and immutable collections forcustomFields. The implementation correctly usesDeepCollectionEqualityfor map comparison.packages/komodo_defi_types/test/asset_cache_key_test.dart (5)
6-107: LGTM! Comprehensive equality and hash code testing.The tests thoroughly validate equality semantics including:
- Identical keys with same custom fields
- Different optional fields producing distinct keys
- Order-insensitive equality for custom fields
- Proper behavior as Map keys
109-197: Excellent fuzzy testing with deterministic RNG.The fuzzy tests with a seeded Random generator provide reproducible test coverage for:
- Equal pairs and hash consistency
- Single-field mutations ensuring inequality
- Set cardinality validation between object and string representations
Using a fixed seed (1337, 4242) ensures deterministic test behavior across runs.
199-318: Well-designed micro-benchmarks with proper warm-up.The benchmark implementation is solid with:
- Conditional execution via
RUN_BENCHenvironment variable- Proper warm-up phase before timing
- Fair comparison between AssetCacheKey and String keys
- Prevention of dead code elimination
319-434: LGTM! Canonical key performance comparison is well-structured.The benchmark properly compares the performance of canonical string keys versus AssetCacheKey objects, using realistic data with AssetId instances and varying custom fields.
436-561: Excellent scaling analysis with varying custom field counts.The benchmark thoughtfully tests performance characteristics with different numbers of custom fields (0 to 32) and adjusts the sample size to maintain reasonable runtime. This provides valuable insights into how the data structure scales with complexity.
packages/komodo_defi_sdk/test/market_data_manager_test.dart (2)
16-33: LGTM! Well-structured test setup.The test setup is comprehensive with proper mock classes and fallback value registration for mocktail.
34-60: LGTM! Good test for basic functionality.The test properly verifies that the manager uses the CexRepository when available and correctly calls the expected methods.
packages/komodo_cex_market_data/test/repository_fallback_mixin_test.dart (2)
8-33: LGTM! Well-structured test harness.The test setup with mocks and the TestRepositoryFallbackManager class properly exercises the mixin functionality.
77-111: LGTM! Comprehensive health tracking tests.The health tracking tests thoroughly cover all scenarios: initial state, failure threshold, recovery, and sub-threshold failures.
packages/komodo_defi_sdk/example/lib/widgets/assets/asset_item.dart (1)
112-139: Good implementation with proper lifecycle management.The stateful widget correctly manages the Future lifecycle by caching it in initState and updating it when the assetId changes in didUpdateWidget.
packages/komodo_cex_market_data/lib/src/id_resolution_strategy.dart (1)
26-74: LGTM! Well-implemented Binance resolution strategy.The strategy properly handles null values, provides appropriate logging, and has a clear fallback mechanism.
packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_selection_test.dart (2)
353-399: LGTM! Test effectively validates repository priorityThe test properly validates that the highest priority repository (Binance) is selected over CoinGecko when both support the same asset. Using real repository instances with stub providers ensures the priority mapping from
RepositoryPriorityManageris applied correctly.
519-566: Test provides good error handling coverageThe test effectively validates that the cache initialization handles repository failures gracefully without throwing exceptions and that working repositories remain usable even when others fail.
packages/komodo_defi_sdk/test/src/market_data/edge_cases/unsupported_asset_test.dart (1)
166-210: Test correctly identifies the permissive ID resolution issueThe test demonstrates that Binance's ID resolution strategy is permissive (falling back to configSymbol) while CoinGecko is now strict (requiring a coinGeckoId). This aligns with the fix mentioned in the PR description for preventing unsupported coins from being queried.
packages/komodo_cex_market_data/lib/src/repository_fallback_mixin.dart (1)
282-292: LGTM! Well-designed testing interfaceThe testing methods are properly exposed with clear naming conventions and appropriate documentation suppression for internal methods.
packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart (2)
347-403: LGTM! Comprehensive concurrent request handling testThe test effectively validates that concurrent requests are handled correctly without exceeding the configured retry limits. Each request makes exactly one attempt as configured with
maxTotalAttempts: 1.
405-458: LGTM! Well-designed circuit breaker testThe test appropriately validates that each failed request respects the
maxTotalAttemptslimit, preventing excessive API calls even when multiple requests fail in sequence.packages/komodo_cex_market_data/lib/src/bootstrap/market_data_bootstrap.dart (1)
5-61: LGTM! Well-structured configuration classThe
MarketDataConfigclass provides a clean and flexible way to configure market data sources with sensible defaults and comprehensive documentation.packages/komodo_cex_market_data/lib/src/komodo/prices/komodo_price_repository.dart (1)
34-45: OHLC support properly removedThe method now correctly throws an
UnsupportedErrorto clearly indicate that OHLC data is not available from this repository.packages/komodo_cex_market_data/lib/src/binance/data/binance_repository.dart (2)
90-128: LGTM! Multi-endpoint retry logic properly implementedThe implementation correctly tries multiple Binance endpoints with proper error handling and fallback behavior.
222-252: LGTM! 24hr price change implementationThe method correctly fetches 24-hour price change data with multi-endpoint retry logic and proper ticker formatting.
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart (1)
212-241: LGTM! Batch processing with proper date range handlingThe implementation correctly batches large date ranges using
maxCoinGeckoDays(365 days) to avoid overwhelming the API, with proper boundary handling and result aggregation.packages/komodo_defi_sdk/lib/src/market_data/market_data_manager.dart (6)
85-107: Robust initialization with proper error handling.The initialization correctly handles per-repository failures without blocking the entire init process, logs appropriately, and sets up the cache timer properly.
169-175: Good state validation pattern.The
_assertInitializedmethod provides clear error messages when the manager is used before initialization, following defensive programming practices.
187-205: Well-structured price fetching with caching.The
_fetchAndCachePricemethod correctly encapsulates the fetch-and-cache logic with proper logging for debugging purposes.
245-258: Excellent use of mixin pattern for repository fallback.The implementation properly leverages
tryRepositoriesInOrderto handle multiple repository attempts with automatic fallback, aligning well with the multi-repository architecture.
334-390: Efficient historical price fetching with smart caching.The implementation efficiently checks cache first, fetches only missing dates, and properly converts/caches the results. The merge of cached and fetched data is handled correctly.
393-402: Comprehensive cleanup in dispose method.The dispose method properly cleans up all resources including timer cancellation, cache clearing, and mixin health data reset.
packages/komodo_cex_market_data/lib/src/coingecko/data/coingecko_repository.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart
Show resolved
Hide resolved
packages/komodo_defi_sdk/test/src/market_data/edge_cases/retry_limits_test.dart
Outdated
Show resolved
Hide resolved
Refactors the market data repositories to support multiple repositories, adds unit tests, and resolves the issue reported with unsupported coins being queried.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores