fix(nft): implement parent coin activation for NFTs#3010
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 WalkthroughThe changes refactor NFT-related repository and API logic to improve error handling, logging, and control flow. The Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant NftsRepo
participant CoinsRepo
participant MM2ApiNft
UI->>NftsRepo: updateNft/getNfts(chains)
NftsRepo->>CoinsRepo: getKnownCoins()
NftsRepo->>CoinsRepo: activateCoin(parentCoin) (for each chain)
NftsRepo->>MM2ApiNft: updateNftList/getNftList(chains)
MM2ApiNft->>MM2ApiNft: enable inactive chains with retry (per chain)
MM2ApiNft->>MM2ApiNft: fetch active NFT chains
MM2ApiNft->>MM2ApiNft: update/fetch NFTs (per chain)
MM2ApiNft-->>NftsRepo: aggregated result or error map
NftsRepo-->>UI: result or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements parent coin activation for NFT operations and improves error handling resilience. The main goal is to ensure parent coins are activated before performing NFT operations and to make NFT fetching more robust by handling per-chain failures gracefully.
- Adds automatic parent coin activation before NFT operations
- Refactors
updateNftListandgetNftListto handle individual chain failures without stopping the entire operation - Improves error messages and logging for better debugging
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/bloc/nfts/nft_main_repo.dart | Adds parent coin activation logic and integrates it into NFT update/fetch operations |
| lib/mm2/mm2_api/mm2_api_nft.dart | Refactors NFT API methods to process chains individually and handle failures gracefully |
|
Visit the preview URL for this PR (updated for commit 6cee8f2): https://walletrc--pull-3010-merge-zm0yuh3l.web.app (expires Tue, 05 Aug 2025 20:33:36 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
takenagain
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
circumvent "NoSuchCoin" error when re-enabling the coin in the same session.
…activate-parent-coins-for-nfts
|
@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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/bloc/coins_bloc/coins_repo.dart(2 hunks)lib/bloc/nfts/nft_main_bloc.dart(5 hunks)lib/bloc/nfts/nft_main_repo.dart(3 hunks)lib/mm2/mm2_api/mm2_api_nft.dart(2 hunks)packages/komodo_ui_kit/lib/src/skeleton_loaders/skeleton_loader_list_tile.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/bloc/nfts/nft_main_repo.dart
- lib/mm2/mm2_api/mm2_api_nft.dart
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: takenagain
PR: KomodoPlatform/komodo-wallet#2976
File: lib/bloc/coins_manager/coins_manager_bloc.dart:82-83
Timestamp: 2025-07-23T09:31:17.738Z
Learning: In the CoinsManagerBloc, `state.selectedCoins` is used separately from `state.coins` to indicate whether a coin is enabled or not in the UI. The order of Set deduplication when merging coin lists doesn't affect UI behavior because selection state is tracked independently from the coin list used for filtering and sorting.
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.
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.
lib/bloc/coins_bloc/coins_repo.dart (3)
Learnt from: takenagain
PR: #2976
File: lib/bloc/coins_manager/coins_manager_bloc.dart:82-83
Timestamp: 2025-07-23T09:31:17.738Z
Learning: In the CoinsManagerBloc, state.selectedCoins is used separately from state.coins to indicate whether a coin is enabled or not in the UI. The order of Set deduplication when merging coin lists doesn't affect UI behavior because selection state is tracked independently from the coin list used for filtering and sorting.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
lib/bloc/nfts/nft_main_bloc.dart (6)
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #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.
Learnt from: takenagain
PR: #2608
File: lib/bloc/fiat/fiat_onramp_form/fiat_form_bloc.dart:2-3
Timestamp: 2025-05-01T21:00:36.970Z
Learning: It's acceptable to use unconditional dart:io imports in the Komodo wallet codebase when the usage is guarded by !kIsWeb conditional checks that prevent the platform-specific code from being executed in web environments.
Learnt from: takenagain
PR: #2611
File: test_units/tests/system_health/system_clock_repository_test.dart:7-8
Timestamp: 2025-05-08T19:07:13.442Z
Learning: In the Komodo Wallet project, test files are structured to define test functions that are called from a central test runner in test_units/main.dart, rather than each test file having its own main() function.
Learnt from: takenagain
PR: #2611
File: test_units/tests/system_health/system_clock_repository_test.dart:7-8
Timestamp: 2025-05-08T19:07:13.442Z
Learning: In the Komodo Wallet project, test files are structured to define test functions that are called from a central test runner in test_units/main.dart, rather than each test file having its own main() function.
Learnt from: takenagain
PR: #2611
File: test_units/tests/system_health/http_head_time_provider_test.dart:8-24
Timestamp: 2025-05-08T19:05:13.083Z
Learning: In the Komodo Wallet project, test functions are defined in individual files under test_units/tests/ but are executed through the central entry point in test_units/main.dart, so individual test files don't need their own main() function.
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
lib/bloc/coins_bloc/coins_repo.dart
[warning] 495-495: The declaration '_disableCoin' isn't referenced.
[warning] 495-495: The declaration '_disableCoin' isn't referenced.
🔇 Additional comments (8)
packages/komodo_ui_kit/lib/src/skeleton_loaders/skeleton_loader_list_tile.dart (2)
4-9: LGTM! Clean parameterization of height.The addition of the optional
heightparameter with a sensible default value of 122 maintains backward compatibility while providing flexibility for different use cases.
57-57: LGTM! Correct usage of height parameter.The height parameter is properly utilized in the Container widget, completing the parameterization cleanly.
lib/bloc/coins_bloc/coins_repo.dart (1)
435-438: LGTM! Clear documentation of the temporary workaround.The updated documentation clearly explains that only balance watchers are cancelled to avoid "NoSuchCoin" errors during reactivation, which aligns with the PR's goal of fixing coin reactivation issues.
lib/bloc/nfts/nft_main_bloc.dart (5)
4-4: LGTM! Appropriate imports for enhanced functionality.The added imports support the new retry mechanism and concurrency control features. The specific import of
retryandExponentialBackofffromkomodo_defi_type_utilsaligns with the implementation in_getAllNfts.Also applies to: 7-8
26-27: Good improvement: Restartable transformer prevents concurrent updates.Using
restartable()transformer forNftMainChainUpdateRequestedevents ensures that if a new update request comes in while one is already processing, the old one gets cancelled and the new one starts fresh. This is particularly valuable for NFT updates which can be time-consuming operations.
56-57: Enhanced observability with appropriate warning log.The warning log provides clear diagnostic information when NFT tab changes are attempted in invalid states, helping with debugging the loading issues mentioned in the PR objectives.
89-89: Consistent logging pattern for unauthorized access.The warning log follows the same pattern as the one in
_onTabChanged, providing consistent diagnostic information across the bloc.
185-194: Excellent retry mechanism implementation with proper error handling.The retry logic addresses the core issue described in the PR objectives where NFT operations fail due to parent coins not being activated. Key strengths:
- Uses exponential backoff with 3 attempts, providing reasonable resilience
- Properly catches and logs severe errors when all retries fail
- The
_repo.updateNft(chains)call likely includes the parent coin activation logic mentioned in the AI summary- Error logging includes both the exception and stack trace for debugging
This implementation ensures NFT updates are robust against transient failures while still proceeding with
getNfts()even if updates fail completely.
| // Skip the deactivation step for now, as it results in "NoSuchCoin" errors | ||
| // when trying to re-enable the coin later in the same session. | ||
| // TODO: Revisit this and create an issue on KDF to track the problem. | ||
| // final deactivationTasks = [ | ||
| // ...coins.map((coin) async { | ||
| // await _disableCoin(coin.id.id); | ||
| // if (notify) _broadcastAsset(coin.copyWith(state: CoinState.inactive)); | ||
| // }), | ||
| // ...allChildCoins.map((child) async { | ||
| // await _disableCoin(child.id.id); | ||
| // if (notify) { | ||
| // _broadcastAsset(child.copyWith(state: CoinState.inactive)); | ||
| // } | ||
| // }), | ||
| // ]; | ||
| // await Future.wait(deactivationTasks); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address the unused _disableCoin method and track the TODO.
The commented-out deactivation logic creates technical debt. The _disableCoin method is now unused (flagged by pipeline), and the TODO should be properly tracked.
Consider these actions:
- Remove the unused
_disableCoinmethod since it's no longer called - Create a proper issue to track the TODO comment about revisiting coin deactivation
- Add a reference to the created issue in the TODO comment
🏁 Script executed:
#!/bin/bash
# Verify that _disableCoin method is indeed unused throughout the codebase
rg -A 5 "_disableCoin" --type dartLength of output: 1188
Remove unused deactivation logic and properly track the TODO
The private method _disableCoin is only referenced by the commented‐out block and is never invoked at runtime, and leaving both the dead code and the TODO in place creates unnecessary technical debt. Please:
- In lib/bloc/coins_bloc/coins_repo.dart:
- Remove the entire commented-out deactivation tasks block (around lines 475–490).
- Delete the unused
_disableCoin(String coinId)method definition (just below the commented block).
- Create a new issue (e.g. KDF-XXXX) to investigate and resolve the “NoSuchCoin” errors when re-enabling coins in the same session.
- Update the existing TODO comment to reference the newly created issue, for example:
// TODO(KDF-XXXX): revisit coin deactivation and re-enable path
🤖 Prompt for AI Agents
In lib/bloc/coins_bloc/coins_repo.dart around lines 475 to 490, remove the
entire commented-out block containing the deactivation tasks since it is unused
and adds technical debt. Also delete the private method _disableCoin(String
coinId) defined just below this block as it is no longer called. Create a new
issue (e.g., KDF-XXXX) to track the “NoSuchCoin” errors when re-enabling coins,
and update the existing TODO comment to reference this issue with the format: //
TODO(KDF-XXXX): revisit coin deactivation and re-enable path.
|
Init on a heavily spammed wallet took a few mins, but got there. Caught a stream of errors in console (brave) pretty much exactly when the NFTs became visible - but only briefly as then the tab crashed. Console error logmain.dart.js:7031 Uncaught Error This was not replicated in chrome with a different seed, so it may be specific to the NFTs in wallet. Console error logkdflib.js:1918 Uncaught Error: closure invoked recursively or after being dropped |
|
Confirmed that despite previously mentioned errors, NFT pages did load in app via web (chrome), desktop (linux) and mobile (android). |
|
Related logs for same wallet running on linux desktop: |
…activate-parent-coins-for-nfts
|
@smk762 I split the image loading improvements into #3020 to avoid bloating and blocking this one. I merged The changes to the NFT image widget in #3020 are significant, so it might be safer to merge this PR and postpone the image loading improvements to the next release (assuming that the image loading errors are not significant enough to be a blocker). |
* feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* feat(nft): tolerate activation failures * fix(nft): skip missing parents in firstWhere Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * refactor(nft-main-repo): fix parent coin filter and use logging package * fix(nft): don't return an error if there are no NFTs returned * fix(nft): revert map-based error responses in favor of manual activation * fix(skeleton-list-tile): add height constraint to fix renderflex assert * fix(coin): temporarily skip calling deactivation RPC circumvent "NoSuchCoin" error when re-enabling the coin in the same session. * fix(activation): use more complete coin conversion function --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fixes:
disable_coinRPC call for now, leaving the coin enabled in the background. Encountered with background NFT refresh tasks that try to activate the required parent and NFT coins before making requests.Summary by CodeRabbit
Bug Fixes
New Features
Refactor