fix(custom-token-import): check platform in deduplication and update fields#3129
fix(custom-token-import): check platform in deduplication and update fields#3129
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. WalkthroughUpdates coin activation retry defaults; refactors custom token import to use injected private dependencies and an AssetId-driven flow with network API mapping, token propagation wait, and temporary deactivation. Adds protocol-copy helper for ERC-20 construction, logging changes, and updates SDK submodule pointer. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI
participant BLoC as CustomTokenImportBloc
participant Repo as CustomTokenImportRepository
participant SDK as SDK (type utils)
participant Coins as CoinsRepo
User->>UI: Enter address + select network
UI->>BLoC: FetchCustomToken(address, networkAssetId)
BLoC->>Repo: fetchCustomToken(networkAssetId, address)
Repo-->>BLoC: Asset (existing or newly constructed)
note over BLoC: Activate token for visibility (maxRetryAttempts: 5)
BLoC->>Coins: activateAssetsSync([asset], maxRetryAttempts=5)
BLoC->>SDK: pollUntil(() => token visible)
alt token appears before timeout
SDK-->>BLoC: token visible
else timeout
SDK-->>BLoC: TimeoutException
note over BLoC: Log warning, continue
end
BLoC->>Coins: deactivateAssetsSync([asset]) %% temporary to avoid confusion
BLoC-->>UI: Show fetched token data (not yet imported)
sequenceDiagram
autonumber
actor User
participant UI as UI
participant BLoC as CustomTokenImportBloc
participant Repo as CustomTokenImportRepository
participant Analytics as AnalyticsBloc
participant SDK as SDK Auth
User->>UI: Submit Import
UI->>BLoC: SubmitImportCustomToken(asset)
BLoC->>Repo: importCustomToken(asset)
Repo-->>BLoC: Import result
BLoC->>Analytics: logImport(walletType=SDK.auth.walletType, token)
BLoC-->>UI: Import success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks (3 passed)✅ 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 |
|
Visit the preview URL for this PR (updated for commit c7fd226): https://walletrc--pull-3129-merge-j4fx6f14.web.app (expires Fri, 26 Sep 2025 16:01:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
…fields - add subclass check in deduplication step - use ticker instead of formatted for request - add chain_id field to protocol object
ERC20 and ETH have wildly different expected protocol object structures
takenagain
force-pushed
the
bugfix/custom-token-address-case
branch
from
231c3ec to
dd1c436
Compare
takenagain
force-pushed
the
bugfix/custom-token-address-case
branch
from
dd1c436 to
9bf6034
Compare
|
@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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/bloc/custom_token_import/data/custom_token_import_repository.dart (1)
151-158: Harden HTTP call: add timeout, check status code, and typed decode.Avoid decoding non-200 and handle timeouts to reduce flakiness.
- try { - final response = await http.get(url); - final data = jsonDecode(response.body); - return data; + try { + final response = + await http.get(url).timeout(const Duration(seconds: 10)); + if (response.statusCode != 200) { + _log.warning('Token API $url returned ${response.statusCode}'); + return null; + } + final data = jsonDecode(response.body) as Map<String, dynamic>; + return data; } catch (e, s) { _log.severe('Error fetching token data from $url', e, s); return null; }
🧹 Nitpick comments (10)
sdk (2)
1-1: Ensure CI checks out submodules recursively.The unresolved URI errors for example packages are consistent with submodules not being fetched in CI.
Apply to your workflow using actions/checkout:
- - uses: actions/checkout@v4 + - uses: actions/checkout@v4 + with: + submodules: recursive + fetch-depth: 0 - - name: Flutter pub get + - name: Init submodules + run: git submodule update --init --recursive + - name: Flutter pub get run: flutter pub get
1-1: Prefer pinning submodule to a tag or add provenance.For reproducibility, consider updating to an annotated tag (e.g., sdk vX.Y.Z) or reference the upstream commit link/changelog in the PR description.
lib/bloc/coins_bloc/coins_repo.dart (5)
245-256: Doc default vs. code default mismatch (30 → 15 attempts).The docstring still says 30 attempts, but the default is now 15. Update the docs to avoid confusion.
- /// - Default: 500ms → 1s → 2s → 4s → 8s → 10s → 10s... (30 attempts ≈ 3 minutes) + /// - Default: 500ms → 1s → 2s → 4s → 8s → 10s → 10s... (15 attempts ≈ 1.8 minutes) @@ - /// - [maxRetryAttempts]: Maximum number of retry attempts (default: 30) + /// - [maxRetryAttempts]: Maximum number of retry attempts (default: 15)Also applies to: 253-256, 270-273
417-420: Should activateCoinsSync also default to 15 attempts?Assets path defaults to 15 while coins path remains at 30. If unintentional, align them; if intentional, add a short note in the docs to explain why they differ.
- int maxRetryAttempts = 30, + int maxRetryAttempts = 15,
83-97: Remove unused parameter from _subscribeToBalanceUpdates.The second parameter is unused; simplify the API and call sites.
- void _subscribeToBalanceUpdates(Asset asset, Coin coin) { + void _subscribeToBalanceUpdates(Asset asset) { @@ - _balanceWatchers[asset.id] = _kdfSdk.balances.watchBalance(asset.id).listen( + _balanceWatchers[asset.id] = _kdfSdk.balances.watchBalance(asset.id).listen( @@ - _subscribeToBalanceUpdates(asset, coin); + _subscribeToBalanceUpdates(asset); @@ - _subscribeToBalanceUpdates(parentAsset, coin); + _subscribeToBalanceUpdates(parentAsset);Also applies to: 332-339
281-287: Fix typo: _addAssetsToWalletMetdata → _addAssetsToWalletMetadata.Minor readability nit; rename method and call sites.
- await _addAssetsToWalletMetdata(assets.map((asset) => asset.id)); + await _addAssetsToWalletMetadata(assets.map((asset) => asset.id)); @@ - Future<void> _addAssetsToWalletMetdata(Iterable<AssetId> assets) async { + Future<void> _addAssetsToWalletMetadata(Iterable<AssetId> assets) async {Also applies to: 369-381
468-471: Capture errors from removeActivatedCoins.Keep it fire-and-forget but log failures.
- unawaited(_kdfSdk.removeActivatedCoins(allCoinIds.toList())); + unawaited( + _kdfSdk.removeActivatedCoins(allCoinIds.toList()).catchError( + (e, s) => _log.warning('removeActivatedCoins failed', e, s), + ), + );lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart (3)
141-159: Fix typo: Propogation → Propagation.Rename the method and its call site.
- await _waitForCustomTokenPropogation(tokenData); + await _waitForCustomTokenPropagation(tokenData); @@ - Future<void> _waitForCustomTokenPropogation( + Future<void> _waitForCustomTokenPropagation(Also applies to: 102-104
146-154: Polling: avoid manual delay inside the poll task.If poll supports an interval parameter, use it; otherwise keep as-is.
- await poll<bool>( - () async { - await Future.delayed(const Duration(seconds: 1)); - return _sdk.assets.available.containsKey(tokenData.id); - }, - isComplete: (assetExists) => assetExists, - maxDuration: timeout, - ); + await poll<bool>( + () async => _sdk.assets.available.containsKey(tokenData.id), + isComplete: (assetExists) => assetExists, + maxDuration: timeout, + // interval: const Duration(seconds: 1), // if supported + );
88-97: Network asset resolution via ticker can be ambiguous.getCoin(ticker) returns null if multiple assets share the ticker. If this occurs in production for some networks, consider selecting by subclass or AssetId instead of ticker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/bloc/coins_bloc/coins_repo.dart(1 hunks)lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart(6 hunks)lib/bloc/custom_token_import/data/custom_token_import_repository.dart(3 hunks)sdk(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: In the Komodo Wallet project, part files share imports with their parent files. The import for `app_config.dart` in `coins_bloc.dart` is necessary because the part file `coins_state.dart` uses `excludedAssetList` from that package.
Applied to files:
lib/bloc/coins_bloc/coins_repo.dartlib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart
📚 Learning: 2025-04-01T15:51:37.060Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2566
File: lib/bloc/coins_bloc/coins_bloc.dart:10-10
Timestamp: 2025-04-01T15:51:37.060Z
Learning: The `excludedAssetList` from `app_config.dart` is used in the `_filterExcludedAssets` method in `coins_state.dart`. Since `coins_state.dart` is a part file of `coins_bloc.dart`, the import needs to be in the parent file even though it's not directly used there. In Dart, part files share the namespace and imports of their parent files.
Applied to files:
lib/bloc/coins_bloc/coins_repo.dart
📚 Learning: 2025-08-31T19:24:53.317Z
Learnt from: CR
PR: KomodoPlatform/komodo-wallet#0
File: AGENTS.md:0-0
Timestamp: 2025-08-31T19:24:53.317Z
Learning: For Komodo DeFi Flutter SDK repository, reference `/KDF_API_DOCUMENTATION.md` and applicable RPCs, data structures, and notes when implementing RPC-related features
Applied to files:
lib/bloc/coins_bloc/coins_repo.dart
📚 Learning: 2025-09-02T15:57:12.405Z
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#3125
File: lib/views/fiat/fiat_inputs.dart:61-62
Timestamp: 2025-09-02T15:57:12.405Z
Learning: The Debouncer class is available from the 'package:komodo_ui/komodo_ui.dart' import in the Komodo Wallet codebase.
Applied to files:
lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
sdk
[error] 6-6: Target of URI doesn't exist: 'package:mocktail/mocktail.dart'
[error] 11-11: Missing concrete implementations of 'IBinanceProvider.fetch24hrTicker', 'IBinanceProvider.fetchExchangeInfo', 'IBinanceProvider.fetchExchangeInfoReduced', and 'IBinanceProvider.fetchKlines'
[error] 11-11: Classes can only extend other classes
[error] 31-31: The function 'when' isn't defined
[error] 33-33: The function 'any' isn't defined
[error] 4-4: Target of URI doesn't exist: 'package:mocktail/mocktail.dart'
[error] 7-7: Missing concrete implementations of 'abstract class ICoinGeckoProvider.fetchCoinHistoricalMarketData', 'abstract class ICoinGeckoProvider.fetchCoinList', 'abstract class ICoinGeckoProvider.fetchCoinMarketChart', 'abstract class ICoinGeckoProvider.fetchCoinMarketData', and 3 more
[error] 7-7: Classes can only extend other classes
[error] 39-39: The function 'when' isn't defined
[error] 6-6: Target of URI doesn't exist: 'package:kdf_sdk_example/main.dart'
[error] 4-4: Target of URI doesn't exist: 'package:kdf_sdk_example/blocs/auth/auth_bloc.dart'
[error] 1-1: SeedDialog reference or undefined; ensure SeedDialog widget is defined and imported correctly
🔇 Additional comments (6)
sdk (2)
1-1: Resolve “Target of URI doesn’t exist: package:kdf_sdk_example/…”Tests or examples importing the example app as a package will fail unless that package is part of the workspace or the submodule is present.
- If needed at runtime/tests: add the example to your workspace (melos) and path dependencies.
- Otherwise: replace those imports with relative paths or remove the test files.
Minimal melos adjustment:
# melos.yaml packages: - packages/** + - sdk/examples/**Then add path dependency in the appropriate pubspec or drop the import if unused.
1-1: Submodule bump — verify SDK API surface & unblock CI.
- CI is configured to checkout submodules recursively, and .gitmodules + git submodule status show sdk present, but sdk/ is not populated in this checkout — I could not inspect the SDK itself.
- I did not find any mocktail imports/dev_dependencies, IBinanceProvider/ICoinGeckoProvider subclasses, or package:kdf_sdk_example/ imports in the main repo.
- Seed dialog exists as customSeedDialog (lib/views/wallets_manager/widgets/custom_seed_dialog.dart) and tests reference its keys (test_integration/helpers/restore_wallet.dart, test_integration/tests/wallets_manager_tests/*).
- Action: populate/checkout the sdk submodule (git submodule update --init --recursive), then diff the SDK provider interfaces vs previous commit and update consumers/tests accordingly (adjust mocks, add mocktail to dev_dependencies if tests import it, and fix any example-package import paths revealed by the SDK changes).
lib/bloc/custom_token_import/data/custom_token_import_repository.dart (2)
100-105: Verify coinPaprikaId assignment.You're setting coinPaprikaId to the CoinGecko id. Confirm the schema expects Paprika's id; otherwise leave it null or map correctly.
47-51: Dedup condition looks good.Matching both contract address and subclass prevents cross-network collisions.
lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart (2)
146-151: Key equality check: confirm AssetId uses value-based ==/hashCode.containsKey(tokenData.id) assumes value equality for AssetId keys; otherwise check by value (e.g., values.any((a) => a.id == tokenData.id)).
98-106: Activation flow looks solid.Pre-activating the network, fetching the token, short activation with retries, then balance check is a good UX compromise.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the custom token import functionality by adding platform validation during deduplication, improving network API handling, and updating asset field management. The changes ensure better token metadata resolution and more accurate asset configuration.
- Add platform/subclass validation to prevent duplicate tokens across different networks
- Switch from formatted to ticker-based field usage for better consistency
- Implement proper chain ID handling and protocol data configuration
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/bloc/custom_token_import/data/custom_token_import_repository.dart | Enhanced token deduplication with platform checks, improved asset creation with proper chain ID and protocol configuration |
| lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart | Added token propagation waiting logic, improved error handling with structured logging, and filtered networks by API support |
| lib/bloc/coins_bloc/coins_repo.dart | Reduced default retry attempts for asset activation from 30 to 15 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
lib/bloc/custom_token_import/data/custom_token_import_repository.dart
Outdated
Show resolved
Hide resolved
lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart
Outdated
Show resolved
Hide resolved
lib/bloc/custom_token_import/bloc/custom_token_import_bloc.dart
Outdated
Show resolved
Hide resolved
|
Confirmed import successful for Banano on polygon network, while using non-mixed case contract address.
Curiously though, after import, transaction history was shown which does not exist This transaction history actually belongs to MATIC (the parent coin). 🚳 Entering the page for a different non-imported token, with actual history, showed "no transactions available" 🚳 Entering the page for a different non-imported token, without actual history, showed "no transactions available" (as expected) 👍 Note: GLEECBTC/komodo-defi-sdk-flutter#217 is under review which may resolve this, though the inconsistency of error between non-imported and imported tokens indicates there may be something extra to be handled. |
|
Upon closing tab, reopening and logging in again, the imported token is no longer present. It was assumed to be auto enabled, yet it was not, and was also not present in the coin activation menu. Above testing performed on Brave browser. Below testing on chrome for confirmation: On both browsers, during the next session / login after import, all tokens (imported and existing) fail to activate, and show the following message in page 👺 
In Chrome, the imported token did appear after 2nd login. After retrying in Brave, the issue does not appear to be browser related - it is due to how the user exits the app.
|
|
Linux desktop fails to import tokens at all 
Logs are nothing but persistent activation errors 
Looking at the SDK, I noticed the inclusion of in custom token activation requests. Cross-checking in postman, I can confirm that the lowercase ETH contract_address, which fails checksum, will result in a similar error.
Mixed case contract_address input activated fine in postman. The errors I was encountering in console logs screenshot are not custom tokens (i.e. should not include the |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Via linux desktop:
-
Confirmed successful import, to correct network, with token type suffix in ticker
-
Confirmed transaction history display for tokens is present.
-
Confirmed transaction history display for parent coins is present.
-
Confirmed import accepts non-mixed case contract addresses
-
Imported token does not persist when exiting the app without logging out first. Console logs showed error post import which may be related:
AppBlocObserver -> onError
CoinsBloc: Bad state: No element
Trace: #0 SetBase.single (dart:collection/set.dart:127)
#1 KdfAuthMetadataExtension.getWalletAssets.<anonymous closure> (package:web_dex/model/kdf_auth_metadata_extension.dart:64)
#2 MappedIterable.elementAt (dart:_internal/iterable.dart:402)
#3 ListIterator.moveNext (dart:_internal/iterable.dart:371)
#4 new _GrowableList._ofEfficientLengthIterable (dart:core-patch/growable_array.dart:194)
#5 new _GrowableList.of (dart:core-patch/growable_array.dart:154)
#6 new List.of (dart:core-patch/array_patch.dart:39)
#7 SetBase.toList (dart:collection/set.dart:119)
#8 KdfAuthMetadataExtension.getWalletAssets (package:web_dex/model/kdf_auth_metadata_extension.dart:65)
<asynchronous suspension>
#9 CoinsRepo.fetchCurrentPrices (package:web_dex/bloc/coins_bloc/coins_repo.dart:555)
<asynchronous suspension>
#10 CoinsBloc._onPricesUpdated (package:web_dex/bloc/coins_bloc/coins_bloc.dart:272)
<asynchronous suspension>
#11 Bloc.on.<anonymous closure>.handleEvent (package:bloc/src/bloc.dart:226)
<asynchronous suspension>
[ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: Bad state: No element
#0 SetBase.single (dart:collection/set.dart:127)
#1 KdfAuthMetadataExtension.getWalletAssets.<anonymous closure> (package:web_dex/model/kdf_auth_metadata_extension.dart:64)
#2 MappedIterable.elementAt (dart:_internal/iterable.dart:402)
#3 ListIterator.moveNext (dart:_internal/iterable.dart:371)
#4 new _GrowableList._ofEfficientLengthIterable (dart:core-patch/growable_array.dart:194)
#5 new _GrowableList.of (dart:core-patch/growable_array.dart:154)
#6 new List.of (dart:core-patch/array_patch.dart:39)
#7 SetBase.toList (dart:collection/set.dart:119)
#8 KdfAuthMetadataExtension.getWalletAssets (package:web_dex/model/kdf_auth_metadata_extension.dart:65)
<asynchronous suspension>
#9 CoinsRepo.fetchCurrentPrices (package:web_dex/bloc/coins_bloc/coins_repo.dart:555)
<asynchronous suspension>
#10 CoinsBloc._onPricesUpdated (package:web_dex/bloc/coins_bloc/coins_bloc.dart:272)
<asynchronous suspension>
#11 Bloc.on.<anonymous closure>.handleEvent (package:bloc/src/bloc.dart:226)
<asynchronous suspension>
This error would post to console periodically.
Additionally, after import the following error was sighted:
Error loading custom icon for AUDE-PLG20: Invalid argument(s): No host specified in URI file:///home/smk/GITHUB/KP/komodo-wallet/assets/coin_icons/png/aude.png
Error loading custom icon for aude-plg20: Invalid argument(s): No host specified in URI file:///home/smk/GITHUB/KP/komodo-wallet/assets/coin_icons/png/aude.png
Error loading custom icon for aude-plg20: Invalid argument(s): No host specified in URI file:///home/smk/GITHUB/KP/komodo-wallet/assets/coin_icons/png/aude.png
Error loading custom icon for aude-plg20: Invalid argument(s): No host specified in URI file:///home/smk/GITHUB/KP/komodo-wallet/assets/coin_icons/png/aude.png
Unsure if this is token specific - I intentionally chose something inactive - though a logo is shown at https://polygonscan.com/address/0x78ced9279aaa76108cd8fffb95d30e0206b10f90
For context:
- Initial import of BANANO token in a different wallet login with no prior activity was successful, including the image.
On Web:
|
|
Confirm ARB removed from list.
|
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
Confirmed import successful for all networks on web, linux and windows.
Unable to test MacOS at this time, unless binaries are available for download.
Summary by CodeRabbit