feat(coin-config): add custom token support to coin config manager#225
feat(coin-config): add custom token support to coin config manager#225
Conversation
|
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 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. WalkthroughAdds a pluggable Hive-backed custom-token storage (ICustomTokenStorage + implementations), integrates it into coin configuration and assets update managers, removes legacy custom-asset history/notifier APIs, updates DI/bootstrap and tests, and re-exports related managers and storage types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Activation as ActivationManager
participant AssetsUM as KomodoAssetsUpdateManager
participant CCM as StrategicCoinConfigManager
participant CTS as ICustomTokenStorage (Hive)
User->>Activation: activate(asset with custom token)
Activation->>AssetsUM: assets.storeCustomToken(asset)
AssetsUM->>CCM: storeCustomToken(asset)
CCM->>CTS: storeCustomToken(asset)
CTS-->>CCM: ack
CCM->>CCM: merge token into in-memory assets (conflict handling)
CCM-->>AssetsUM: ack
AssetsUM-->>Activation: ack
sequenceDiagram
autonumber
participant Startup as StartupCoinsProvider
participant CCM as StrategicCoinConfigManager
participant CTS as ICustomTokenStorage
Startup->>CCM: new(..., customTokenStorage: NoOpCustomTokenStorage())
CCM->>CTS: getAllCustomTokens()
CTS-->>CCM: []
CCM->>CCM: load base assets, merge custom tokens
CCM-->>Startup: assets ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
|
@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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/komodo_coin_updates/lib/src/coins_config/custom_token_storage.dart (1)
125-137: Critical issue: LazyBox.containsKey requires await.In Hive LazyBox, containsKey returns a Future and must be awaited: "bool containsKey = await lazyBox.containsKey('key1')". The current code treats it as synchronous, which will cause runtime errors.
Apply this fix:
Future<bool> updateCustomToken(Asset asset) async { final box = await _openCustomTokensBox(); - final existed = box.containsKey(asset.id.id); + final existed = await box.containsKey(asset.id.id); await box.put(asset.id.id, asset); Future<bool> addCustomTokenIfNotExists(Asset asset) async { final box = await _openCustomTokensBox(); - if (box.containsKey(asset.id.id)) { + if (await box.containsKey(asset.id.id)) { _log.fine('Custom token ${asset.id.id} already exists, skipping'); return false;Also applies to: 139-150
🧹 Nitpick comments (4)
packages/komodo_coins/lib/src/asset_filter.dart (3)
15-31: Harden factory: normalize IDs and choose a safe fallbackNormalize the incoming ID to avoid case/whitespace mismatches and consider defaulting unknown IDs to a no-op strategy instead of null to prevent NPEs downstream.
Apply this diff:
- static AssetFilterStrategy? fromStrategyId(String strategyId) { - switch (strategyId) { + static AssetFilterStrategy? fromStrategyId(String strategyId) { + final id = strategyId.trim().toLowerCase(); + switch (id) { case 'none': return const NoAssetFilterStrategy(); case 'trezor': // Using default hiddenAssets - in practice, this should work for most cases return const TrezorAssetFilterStrategy(); case 'utxo': return const UtxoAssetFilterStrategy(); case 'evm': return const EvmAssetFilterStrategy(); default: - return null; + // Safer fallback to avoid null propagation + return const NoAssetFilterStrategy(); } }Optional follow-up: extract these IDs into constants to avoid stringly-typed switches.
63-67: Confirm protocol whitelist (qrc20) for Trezor supportqrc20 is allowed by isProtocolSupported. Please confirm qrc20 chains are actually supported by the Trezor flow; if not, drop it from the whitelist.
Nit: rename isProtocolSupported to isClassSupportedByTrezor for clarity.
68-71: Trim and null-safe the trezor_coin checkGuard against whitespace-only strings and avoid double indexing.
Apply this diff:
- final hasTrezorCoinField = - coinConfig['trezor_coin'] is String && - (coinConfig['trezor_coin'] as String).isNotEmpty; + final trezorCoin = (coinConfig['trezor_coin'] as String?)?.trim(); + final hasTrezorCoinField = trezorCoin?.isNotEmpty ?? false;packages/komodo_coin_updates/lib/src/coins_config/custom_token_storage.dart (1)
55-70: Erc20Protocol cast is acceptable now — add a defensive check or generic protocol flag
- Verification: repo search shows Erc20Protocol.copyWith in packages/komodo_defi_types/lib/src/protocols/erc20/erc20_protocol.dart and the only casts to Erc20Protocol are in packages/komodo_coin_updates/lib/src/coins_config/custom_token_storage.dart (lines 65, 82).
- Action: keep the cast for now, but add a runtime type-check before casting or expose isCustomToken/copyWith on the Protocol base to future-proof support for non-ERC20 protocols.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/komodo_coin_updates/lib/src/coins_config/_coins_config_index.dart(1 hunks)packages/komodo_coin_updates/lib/src/coins_config/asset_parser.dart(2 hunks)packages/komodo_coin_updates/lib/src/coins_config/coin_config_repository.dart(2 hunks)packages/komodo_coin_updates/lib/src/coins_config/custom_token_storage.dart(1 hunks)packages/komodo_coin_updates/lib/src/coins_config/no_op_custom_token_storage.dart(1 hunks)packages/komodo_coins/lib/src/asset_filter.dart(3 hunks)packages/komodo_coins/lib/src/asset_management/coin_config_manager.dart(7 hunks)packages/komodo_coins/lib/src/startup/startup_coins_provider.dart(1 hunks)packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart(9 hunks)packages/komodo_defi_types/lib/src/assets/asset.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/komodo_coins/lib/src/startup/startup_coins_provider.dart
- packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart
🔇 Additional comments (20)
packages/komodo_coin_updates/lib/src/coins_config/_coins_config_index.dart (1)
11-13: New exports support custom token storage integration.These exports enable access to the custom token storage infrastructure throughout the codebase, supporting the broader PR objective of adding custom token support.
packages/komodo_defi_types/lib/src/assets/asset.dart (1)
73-86: LGTM! Enables metadata preservation for custom token management.The copyWith method provides essential functionality for creating modified Asset instances while preserving all original metadata. This is particularly valuable for custom token workflows where assets need to be duplicated or modified while maintaining complete field integrity.
packages/komodo_coin_updates/lib/src/coins_config/coin_config_repository.dart (2)
107-109: Method documentation updated appropriately.The documentation correctly reflects the change from two-pass parsing to AssetParser-based reconstruction for parent-child relationships.
120-128: Clean refactor to use AssetParser for relationship rebuilding.The simplified approach uses AssetParser to rebuild parent-child relationships from stored assets, eliminating the previous multi-step transformation logic. This provides consistency with other parsing flows in the codebase.
packages/komodo_coin_updates/lib/src/coins_config/asset_parser.dart (2)
31-35: Method signature change: parseAssetsFromConfig now synchronous.The change from
Future<List<Asset>>toList<Asset>is appropriate since the method no longer performs asynchronous operations. This simplifies the call sites and improves performance.
146-180: Well-designed method for rebuilding asset relationships.The new
rebuildParentChildRelationshipsmethod provides a clean way to reconstruct parent-child relationships from existing assets, which is essential for storage-loaded assets. The implementation reuses existing parsing logic by converting assets back to config format and re-parsing them.packages/komodo_coin_updates/lib/src/coins_config/no_op_custom_token_storage.dart (1)
1-82: Clean no-op implementation for minimal storage scenarios.The NoOpCustomTokenStorage provides an appropriate null-object pattern implementation for cases where custom tokens should not be persisted, such as startup flows. The implementation correctly returns empty/false results for all operations while maintaining the interface contract.
packages/komodo_coins/lib/src/asset_management/coin_config_manager.dart (11)
4-4: Added import for custom token storage support.The import enables access to the custom token storage infrastructure integrated in this file.
58-62: Public API extended with custom token operations.The interface appropriately exposes custom token storage and deletion methods, maintaining consistency with the existing CoinConfigManager contract.
73-80: Factory updated to support custom token storage injection.The factory method now accepts an optional
ICustomTokenStorageparameter and defaults toCustomTokenStorage(), enabling dependency injection while providing sensible defaults.
87-91: Constructor properly stores injected custom token storage.The internal constructor correctly accepts and stores the custom token storage dependency.
98-98: Custom token storage field added.The private field stores the injected custom token storage instance for use throughout the class.
200-200: Custom token loading integrated into asset lifecycle.Custom tokens are properly loaded and merged after both initial asset loading and refresh operations, ensuring consistency across the asset lifecycle.
Also applies to: 250-250
342-359: Simple and effective custom token merging.The implementation loads custom tokens from storage and merges them directly into the assets map. Error handling is appropriate with warning-level logging that won't crash the application if custom token loading fails.
361-383: Well-designed cache update helpers.The helper methods maintain filter cache consistency when assets are added or removed. The use of
AssetFilterStrategy.fromStrategyId()provides a clean way to reconstruct strategy instances for cache updates.
385-393: Clean implementation of store operation.The store operation correctly persists to storage, updates in-memory assets, and maintains cache consistency. The simplified approach without conflict resolution is cleaner and avoids the storage/memory divergence issues identified in previous reviews.
395-402: Clean implementation of delete operation.The delete operation correctly removes from both storage and memory, maintaining consistency between persistent and in-memory state. The cache update ensures filtered views remain accurate.
415-415: Resource cleanup includes custom token storage.The dispose method properly releases custom token storage resources, preventing memory leaks.
packages/komodo_coin_updates/lib/src/coins_config/custom_token_storage.dart (2)
74-84: Same Erc20Protocol cast issue in single token retrieval.The same type assumption exists here - consider making the custom token signaling more resilient to different protocol types.
1-185: Otherwise well-implemented custom token storage.The overall implementation provides comprehensive CRUD operations for custom tokens with proper error handling, logging, and parent-child relationship reconstruction. The dependency injection support and box management are well done.
takenagain
force-pushed
the
feat/config-custom-token-support
branch
from
1509a3b to
5013348
Compare
CharlVS
left a comment
CharlVS
left a comment
There was a problem hiding this comment.
@takenagain Please create an issue to investigate if there is a way to avoid hard-coded cex-related values either from fetching info from an internal/external API or using a new/existing coins config value.
Will do so for the recently added USD stablecoin mapping in Binance. There will generally be some level of intentional hard-coded values in unit tests to mock APIs and services, but they would benefit from a refactoring round to clean up the mocks and fixtures. I haven't looked into the full functionality and comparison between |
Summary by CodeRabbit
New Features
Refactor
Tests