feat(coin-updates): integrate komodo_coin_updates into komodo_coins#190
feat(coin-updates): integrate komodo_coin_updates into komodo_coins#190
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 WalkthroughRefactors coin configuration handling across packages: introduces provider/repository architecture using Asset models and Hive CE, removes legacy Coin/CoinConfig models/adapters, adds generated Hive registrar, new runtime config loader, new asset/update managers and strategies in komodo_coins, integrates into SDK, enhances CEX market data caching with typed Hive, updates CI workflows/docs, and adjusts build transformer/CDN handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant SDK as KomodoAssetsUpdateManager
participant Cfg as AssetRuntimeUpdateConfigRepository
participant Factory as CoinConfigDataFactory
participant Repo as CoinConfigRepository
participant ProvG as GithubCoinConfigProvider
participant ProvA as LocalAssetCoinConfigProvider
participant Hive as Hive (assets/settings)
App->>SDK: init()
SDK->>Cfg: load()
Cfg-->>SDK: AssetRuntimeUpdateConfig
SDK->>Factory: createRepository(config, transformer)
Factory-->>SDK: Repo
SDK->>Factory: createLocalProvider(config)
Factory-->>SDK: ProvA
SDK->>Repo: updatedAssetStorageExists()
alt storage empty
SDK->>ProvA: getAssets()
ProvA-->>SDK: [Asset]
SDK->>Hive: upsertAssets + commit (bundled)
else storage present
SDK->>ProvG: getLatestCommit()
ProvG-->>SDK: latest SHA
SDK->>Repo: isLatestCommit()
alt outdated
SDK->>Repo: updateCoinConfig()
Repo->>ProvG: getAssetsForCommit(latest)
ProvG-->>Repo: [Asset]
Repo->>Hive: upsertAssets + commit (latest)
else up-to-date
SDK-->>App: init complete
end
end
SDK-->>App: assets available
sequenceDiagram
autonumber
actor App
participant Upd as StrategicCoinUpdateManager
participant Strat as UpdateStrategy
participant Repo as CoinConfigRepository
participant ProvG as CoinConfigProvider (fallback)
App->>Upd: startBackgroundUpdates()
loop every updateInterval
Upd->>Strat: shouldUpdate(requestType=background, repo, times, commits)
Strat-->>Upd: true/false
alt true
Upd->>Repo: updateCoinConfig()
Repo-->>Upd: done
Upd->>Repo: getCurrentCommit()
Repo-->>Upd: new SHA
Upd-->>App: emit UpdateResult(success, counts, hashes)
else false
Upd-->>App: no-op
end
end
App->>Upd: getLatestCommitHash()
Upd->>Repo: isLatestCommit()
alt error or null
Upd->>ProvG: getLatestCommit()
ProvG-->>Upd: latest SHA
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ogging removed package level build.yaml
takenagain
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fromJson expects flatter coins_config.json format, whereas the toJson methods return more nested custom structures. This causes fromJson parsing to fail if the toJson methods are used.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the coin management system to replace the legacy komodo_coins implementation with komodo_coin_updates, introducing a comprehensive asset runtime update system. The primary goal is to enable runtime coin configuration updates while maintaining backward compatibility and improving test coverage.
- Integrates komodo_coin_updates package for runtime asset management with GitHub/CDN providers
- Replaces hard-coded coin configuration with dynamic build configuration reading
- Adds comprehensive unit test coverage with asset config builders and mock implementations
- Migrates to hive_ce for persistent storage with automatic adapter registration
Reviewed Changes
Copilot reviewed 154 out of 155 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/komodo_defi_types/test/utils/asset_config_builders.dart | Test utility builders for creating asset configurations in tests |
| packages/komodo_defi_types/test/assets/asset_roundtrip_test.dart | JSON serialization roundtrip tests using asset config builders |
| packages/komodo_coins/test/*.dart | Comprehensive test suite for coin management strategies and fallback behavior |
| packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart | Integration of new KomodoAssetsUpdateManager with commit hash exposure |
| packages/komodo_defi_framework/lib/src/config/kdf_startup_config.dart | Updated to use StartupCoinsProvider instead of legacy KomodoCoins |
Comments suppressed due to low confidence (1)
packages/komodo_defi_types/lib/src/types.dart:35
- The removal of 'protocols/base/protocol_class.dart' export could be a breaking change if other packages depend on this export. Consider adding a deprecation notice before removing public exports.
export 'protocols/erc20/erc20_protocol.dart';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 38
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/komodo_coin_updates/docs/testing.md (1)
42-42: Remove stray trailing “42”.This appears to be an accidental artifact at EOF.
-42packages/komodo_coin_updates/lib/src/coins_config/config_transform.dart (1)
172-191: Deep-copy entries and filter by protocol, not presence of ws_url
- Today you mutate each element map in-place and use
ws_urlas the filter sentinel. If an incoming non-WSS entry happens to carry aws_url, it will be incorrectly filtered.- Clone each entry map before normalization to avoid aliasing originals, then filter using the
protocolfield.- JsonList filterElectrums( + JsonList filterElectrums( JsonList electrums, { required ElectrumServerType serverType, }) { - final electrumsCopy = JsonList.of(electrums); - - for (final e in electrumsCopy) { - if (e['protocol'] == 'WSS') { - e['ws_url'] = e['url']; - } - } - - return electrumsCopy..removeWhere( - (JsonMap e) => - serverType == ElectrumServerType.wssOnly - ? e['ws_url'] == null - : e['ws_url'] != null, - ); + // Clone each entry map to avoid mutating shared references. + final electrumsCopy = JsonList.of( + electrums.map<JsonMap>((e) => JsonMap.of(e)).toList(), + ); + + for (final e in electrumsCopy) { + if ((e['protocol'] as String?) == 'WSS') { + e['ws_url'] = e['url']; + } else { + e.remove('ws_url'); // keep normalized shape + } + } + + // Filter using protocol for correctness. + return electrumsCopy + ..removeWhere((JsonMap e) => serverType == ElectrumServerType.wssOnly + ? e['protocol'] != 'WSS' + : e['protocol'] == 'WSS'); }packages/komodo_coin_updates/docs/providers.md (1)
51-55: Fix minor grammar/whitespace; add a security note on tokens.Tighten the sentence and remind readers not to hardcode tokens.
-## Testing providers +## Testing providers @@ -- Inject `http.Client` in GitHub provider and `AssetBundle` in local provider to - supply fakes/mocks in tests. +- Inject `http.Client` in the GitHub provider and `AssetBundle` in the local provider to supply fakes/mocks in tests. + +> Note: Never hardcode a personal access token in source code or assets. Prefer environment variables or encrypted CI secrets.
♻️ Duplicate comments (4)
packages/komodo_defi_sdk/example/lib/blocs/coins_commit/coins_commit_cubit.dart (1)
18-24: Null-safety in truncated getters: avoid!on nullable valuesWhile
?.short-circuits, thecurrent!.length/latest!.lengthreads are still brittle and flagged by analyzers. Use a local variable to compute safely.- String? get currentTruncated => - current?.substring(0, current!.length >= 7 ? 7 : current!.length); + String? get currentTruncated { + final value = current; + if (value == null) return null; + final end = value.length >= 7 ? 7 : value.length; + return value.substring(0, end); + } /// Returns the latest commit hash truncated to 7 characters - String? get latestTruncated => - latest?.substring(0, latest!.length >= 7 ? 7 : latest!.length); + String? get latestTruncated { + final value = latest; + if (value == null) return null; + final end = value.length >= 7 ? 7 : value.length; + return value.substring(0, end); + }packages/komodo_cex_market_data/lib/src/hive_adapters.dart (1)
1-2: Uses Hive CE API — ensure runtime dependency alignsThis file imports package:hive_ce/hive.dart. Per my comment in pubspec.yaml, add a runtime dependency on hive_ce and remove hive to avoid mixed APIs.
packages/komodo_defi_framework/app_build/build_config.json (1)
68-69: Raw GitHub as primary content origin: validate rate limits and add commit-pinned fetchesSwitching
coins_repo_content_urltoraw.githubusercontent.comtrades CDN caching for direct origin. Ensure:
- The provider builds content URLs using the pinned commit (when available) instead of branch paths to avoid TOCTOU and reduce cache churn.
- There is robust fallback to the CDN mirrors already listed in
cdn_branch_mirrors.Run to confirm URL construction and fallback ordering:
#!/bin/bash set -euo pipefail # 1) Find where coins_repo_content_url is consumed rg -n --pretty --glob 'packages/**' 'coins_repo_content_url|coinsGithubContentUrl|GithubCoinConfigProvider|fromConfig' -C3 # 2) Inspect URL assembly for commit vs branch use rg -n --pretty --glob 'packages/**' -C3 \ -e 'raw\.githubusercontent\.com' \ -e 'coins_repo_branch' \ -e 'bundled_coins_repo_commit' \ -e 'update_commit_on_build' # 3) Verify any fallback ordering that prefers CDN mirrors rg -n --pretty --glob 'packages/**' -C3 'cdn_branch_mirrors'Note: This echoes concerns previously raised about CDN vs raw GitHub reliability. Validating commit-pinned usage would address most of the risk.
packages/komodo_defi_types/lib/src/assets/asset.dart (1)
28-28: Clarification on past concernThe earlier suggestion about a separate factory for compatibility isn’t necessary here: adding an optional named parameter in Dart does not break existing call sites.
also fix broken tests
also improve concurrent requests handling in sparkline repository
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/komodo_defi_sdk/test/src/market_data/edge_cases/repository_supports_filtering_test.dart (1)
566-568: Fix call to non-existent method.The test calls
recordRepositoryFailureForTestmethod which doesn't exist on theTestSupportFilteringManager. This should use the newtriggerRepositoryFailuresmethod instead.// Make Binance unhealthy; since health is keyed by runtimeType, all mocks become unhealthy - for (int i = 0; i < 3; i++) { - testManager.recordRepositoryFailureForTest(mockBinanceRepo); - } + when( + () => mockBinanceRepo.getCoinFiatPrice(any()), + ).thenThrow(Exception('Binance failed')); + await testManager.triggerRepositoryFailures(mockBinanceRepo, 4);packages/komodo_wallet_build_transformer/lib/src/steps/fetch_coin_assets_build_step.dart (2)
121-144: Fix broken URL in user-facing error messageThe repository link is misspelled (
KomodPlatformmissing “o”). Broken link in a build-stop error message is frustrating for users following remediation steps.Apply:
- https://github.com/KomodPlatform/flutter + https://github.com/KomodoPlatform/flutter
209-221: Prefix mapped file and folder paths withartifactOutputDirectoryThe current implementation passes raw keys from
config.mappedFilesandconfig.mappedFolders(e.g."assets/icons") directly to_getFilesInFolderandrevertOrDeleteGitFiles. Those keys are relative to the configured artifact directory, so Git operations will target the wrong paths (or delete unintended project files) instead of the downloaded assets.Apply this patch to ensure both file and folder paths are rooted at
artifactOutputDirectory:--- a/packages/komodo_wallet_build_transformer/lib/src/steps/fetch_coin_assets_build_step.dart +++ b/packages/komodo_wallet_build_transformer/lib/src/steps/fetch_coin_assets_build_step.dart @@ -209,8 +209,15 @@ _log.info( 'Reverting fetch coin assets build step. ' 'Reverting or deleting downloaded files.', ); - final mappedFilePaths = config.mappedFiles.keys.toList(); - final mappedFolderPaths = config.mappedFolders.keys.toList(); + final mappedFilePaths = config.mappedFiles.keys + .map((p) => path.join(artifactOutputDirectory, p)) + .toList(); + final mappedFolderPaths = config.mappedFolders.keys + .map((p) => path.join(artifactOutputDirectory, p)) + .toList(); final mappedFolderFilePaths = mappedFolderPaths.map(_getFilesInFolder); final allFiles = mappedFilePaths + mappedFolderFilePaths.expand((List<String> x) => x).toList(); await GitHubFileDownloader.revertOrDeleteGitFiles(allFiles);
- This ensures
revertOrDeleteGitFilesoperates on the actual locations where assets were written.- No further changes to
_getFilesInFolderor import statements are required, aspath.joinis already available.packages/komodo_coin_updates/lib/src/coins_config/config_transform.dart (1)
33-50: Re-evaluate needsTransform after each transform (avoid pre-filtering pitfall)Filtering the transforms once against the original
configcan skip later transforms whose preconditions only become true after an earlier transform. EvaluateneedsTransformagainst the evolving config in sequence.- /// Applies all necessary transforms to the given coin configuration. - JsonMap apply(JsonMap config) { - final neededTransforms = _transforms.where((t) => t.needsTransform(config)); - - if (neededTransforms.isEmpty) { - return config; - } - - return neededTransforms.fold( - config, - // Instantiating a new map for each transform is not ideal, given the - // large size of the config file. However, it is necessary to avoid - // mutating the original map and for making the transforms idempotent. - // Use sparingly and ideally only once. - (config, transform) => transform.transform(JsonMap.of(config)), - ); - } + /// Applies all necessary transforms to the given coin configuration. + JsonMap apply(JsonMap config) { + var out = config; + for (final t in _transforms) { + if (t.needsTransform(out)) { + // Copy to preserve idempotency and avoid mutating caller state + out = t.transform(JsonMap.of(out)); + } + } + return out; + }
♻️ Duplicate comments (12)
.github/workflows/firebase-hosting-pull-request.yml (2)
58-60: Nice: token parity for Playground dry/build steps.This addresses prior rate-limit failures by ensuring the same token context as the SDK example build.
89-91: LGTM: consistent token usage for the SDK example build.Parity achieved; this should prevent intermittent GitHub API rate-limit issues during asset generation.
packages/komodo_coins/lib/src/asset_management/coin_config_manager.dart (1)
1-9: Add missing import forfirstOrNull(will not compile otherwise).This file uses
Iterable.firstOrNull(Lines 292–296) but doesn’t import the extension provider. Unless a higher-level library re-exports it, this won’t compile. Add thecollectionimport here. Also verify sibling usage in loading_strategy.dart if present.Apply this diff:
import 'dart:async'; import 'dart:collection'; +import 'package:collection/collection.dart'; import 'package:komodo_coins/src/asset_filter.dart'; import 'package:komodo_coins/src/asset_management/coin_config_fallback_mixin.dart'; import 'package:komodo_coins/src/asset_management/loading_strategy.dart';Run to confirm whether a re-export already exists (if so, this import is redundant but harmless):
#!/bin/bash # Check for re-exports that might already expose Iterable.firstOrNull rg -nP "export\s+['\"]package:collection/collection.dart['\"]" -C2 # Confirm all files that use firstOrNull have an import or are covered by a re-export rg -nP --type=dart '\bfirstOrNull\b' -C2packages/komodo_defi_sdk/example/lib/blocs/coins_commit/coins_commit_cubit.dart (1)
18-24: Avoid forced null checks in substring args; make truncation null-safe and DRY- /// Returns the current commit hash truncated to 7 characters - String? get currentTruncated => - current?.substring(0, current!.length >= 7 ? 7 : current!.length); + /// Returns the current commit hash truncated to 7 characters + String? get currentTruncated { + final s = current; + if (s == null) return null; + final len = s.length; + return s.substring(0, len >= 7 ? 7 : len); + } - /// Returns the latest commit hash truncated to 7 characters - String? get latestTruncated => - latest?.substring(0, latest!.length >= 7 ? 7 : latest!.length); + /// Returns the latest commit hash truncated to 7 characters + String? get latestTruncated { + final s = latest; + if (s == null) return null; + final len = s.length; + return s.substring(0, len >= 7 ? 7 : len); + }Optional: factor the logic into a private helper to remove duplication:
String? _truncate7(String? s) { if (s == null) return null; final len = s.length; return s.substring(0, len >= 7 ? 7 : len); }packages/komodo_coin_updates/README.md (1)
16-29: Installation section conflicts with unpublished package status; show path/git dependency instead ofdart pub add.This repo sets
publish_to: none, sodart pub addand a versionedpubspec.yamlentry are misleading. Replace with monorepo path and external git usage examples.Apply this update:
-## Installation - -Preferred (adds the latest compatible version): - -```sh -dart pub add komodo_coin_updates -``` - -Or manually in `pubspec.yaml`: - -```yaml -dependencies: - komodo_coin_updates: ^latest -``` +## Installation + +Add the dependency and import the library. + +If you are in this monorepo: + +```yaml +dependencies: + komodo_coin_updates: + path: packages/komodo_coin_updates +``` + +If you consume from another repo (unpublished): + +```yaml +dependencies: + komodo_coin_updates: + git: + url: https://github.com/KomodoPlatform/komodo-defi-sdk-flutter.git + path: packages/komodo_coin_updates + ref: dev +```packages/komodo_cex_market_data/lib/src/sparkline_repository.dart (2)
147-151: Cache check implementation looks correct.The current implementation properly checks the typed cache through
_getCachedSparkline, which handles both positive and negative cache entries as noted in past review comments.
272-305: Typed cache implementation is correct.The
_getCachedSparklinemethod properly reads from the typedBox<SparklineData>and handles:
- Type checking with cleanup of unexpected types
- Expiry checking with automatic deletion
- Returning immutable copies of cached data
- Proper error handling
This addresses the issues noted in past review comments about cache misses and negative cache handling.
packages/komodo_coins/lib/src/update_management/coin_update_manager.dart (1)
195-221: Serialize updates to prevent overlapping immediate/background runs
updateNow()can overlap with the periodic background timer’s_performBackgroundUpdate(), causing duplicate work and possible repository races. Introduce an in-flight future and route both paths through it to serialize updates. This also deduplicates concurrent immediate calls.Apply (excerpt):
@@ DateTime? _lastUpdateTime; + Future<UpdateResult>? _inFlightUpdate; @@ Future<UpdateResult> updateNow() async { @@ - final result = await retry( - () => _performUpdate(UpdateRequestType.immediateUpdate), + final result = await retry( + () => _runSerialized(UpdateRequestType.immediateUpdate), maxAttempts: 3, @@ return result; } + + // Ensures only one update runs at a time. Background requests are skipped + // if an update is already in flight; immediate requests will await it. + Future<UpdateResult> _runSerialized(UpdateRequestType requestType) async { + final inflight = _inFlightUpdate; + if (inflight != null) { + if (requestType == UpdateRequestType.backgroundUpdate) { + _logger.finer('Update in flight; skipping background request.'); + return const UpdateResult(success: true, updatedAssetCount: 0); + } + try { + await inflight; + } catch (_) { + // ignore; we'll attempt our own update + } + } + final completer = Completer<UpdateResult>(); + _inFlightUpdate = completer.future; + try { + final result = await _performUpdate(requestType); + completer.complete(result); + return result; + } catch (e, s) { + completer.completeError(e, s); + rethrow; + } finally { + _inFlightUpdate = null; + } + }And in the background path:
- final result = await _performUpdate(UpdateRequestType.backgroundUpdate); + final result = await _runSerialized(UpdateRequestType.backgroundUpdate);Check if a similar serializer already exists elsewhere:
#!/bin/bash rg -nP '\b_inFlightUpdate\b|_runSerialized\b|serialize[d]?\b' packages rg -nP '(updateNow\(\)|_performBackgroundUpdate\(\))' packages/komodo_coins/lib/src/update_management/coin_update_manager.dart -n -C3packages/komodo_coins/test/strategic_coin_update_manager_test.dart (2)
441-459: Nice: deterministic stream test using expectLater/emitsThis removes flakiness from arbitrary delays and validates the emission precisely.
489-507: Nice: deterministic stream test for fresh managerSame improvement here — reliable, delay-free stream assertions.
packages/komodo_coin_updates/test/seed_node_updater_test.dart (1)
37-44: Good fix: use const SeedNode and realistic contact emailSwitching to const constructors and replacing empty-string email with a realistic test address improves immutability and clarity. This resolves the prior nit.
Also applies to: 45-52
packages/komodo_coin_updates/docs/getting-started.md (1)
26-28: Fix example imports: add komodo_defi_types for AssetRuntimeUpdateConfigThe sample uses AssetRuntimeUpdateConfig but only imports komodo_coin_updates; the snippet won’t compile without importing the type from komodo_defi_types.
Apply this diff to the snippet:
```dart -import 'package:komodo_coin_updates/komodo_coin_updates.dart'; +import 'package:komodo_coin_updates/komodo_coin_updates.dart'; +import 'package:komodo_defi_types/komodo_defi_types.dart' + show AssetRuntimeUpdateConfig;</blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: francoisvw <francoisvw@protonmail.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on August 22, 2025
Learn more in the Cursor dashboard.
…t blocking (#207) Co-authored-by: Cursor Agent <cursoragent@cursor.com>
komodo_coin_updatesto useAssetfor storage and retrieval, removing most of the legacy models and adapters in the process.komodo_coin_updatestohive_ce.build_config.jsontokomodo_coin_updatesto read the coin configuration settings.komodo_coin_updatesandkomodo_coinsto validate the desired functionality works as expected.komodo_coin_updatespackage.komodo_coin_updatesintokomodo_coins, replacing the hard-coded URL bypassing any local coin config files.Screen.Recording.2025-08-22.at.17.02.07.mov
Local testing
Update
packages/komodo_defi_framework/assets/configwith the contents of coins_config.json from23b16a0.Modify the following values in
packages/komodo_defi_framework/app_build/build_config.jsonwith the following values. This will prevent thecoins_config.jsonfrom being overridden with an updated version at build time.Register a new account.
Search for "NYC" and "DIMI". Confirm that "NYC" does not have a UTXO/Smart coin and "DIMI" cannot be activated due to missing derivation path.
Refresh page and sign back in.
Search for "NYC" and "DIMI" again and confirm that NYC now has a UTXO coin and "DIMI" can now be activated.
Summary by CodeRabbit