fix(dex): crash on buy coin dropdown selection#2624
Conversation
KDF returns ARRR in the best orders response, which is currently excluded from the state which caused the exception to be thrown in grouped list view
|
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 update coin filtering logic across several modules to exclude unsupported coins such as ARRR and NFT v2 assets. Filtering is applied when fetching best orders in bridge and taker forms. Type specificity is improved in a grouped list view widget, and minor variable declaration improvements are made in the wallets repository. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant BridgeBloc
participant AppConfig
participant Repository
UI->>BridgeBloc: Request best orders
BridgeBloc->>Repository: Fetch best orders
Repository-->>BridgeBloc: Return best orders
BridgeBloc->>AppConfig: Access excludedAssetList
BridgeBloc->>BridgeBloc: Filter out excluded coins from best orders
BridgeBloc-->>UI: Emit filtered best orders
sequenceDiagram
participant UI
participant TakerBloc
participant AppConfig
participant Repository
UI->>TakerBloc: Request best orders
TakerBloc->>Repository: Fetch best orders
Repository-->>TakerBloc: Return best orders
TakerBloc->>AppConfig: Access excludedAssetList
TakerBloc->>TakerBloc: Filter out excluded coins from best orders
TakerBloc-->>UI: Emit filtered best orders
Suggested labels
Suggested reviewers
Poem
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 (
|
|
Visit the preview URL for this PR (updated for commit 3cb8de2): https://walletrc--pull-2624-merge-4h2sveol.web.app (expires Wed, 21 May 2025 14:53:42 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@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: 2
🔭 Outside diff range comments (2)
lib/bloc/bridge_form/bridge_bloc.dart (1)
99-99: 💡 Verification agent🧩 Analysis chain
Address linting warning for deprecated method.
The pipeline reports that 'getCoin' is deprecated. Consider updating to the recommended alternatives: KomodoDefiSdk assets or getCoinFromId.
Also applies to: 589-589
🏁 Script executed:
#!/bin/bash # Check for usage of the recommended alternatives in the codebase echo "Checking for usage of the recommended alternatives..." rg "getCoinFromId|KomodoDefiSdk.*assets" --type=dartLength of output: 623
Replace deprecated
getCoinwithgetCoinFromId
ThegetCoincall at these locations is deprecated. Please switch to the existinggetCoinFromIdmethod in your repository, which already returns a nullableCoin?. For example:// Before final Coin? defaultTickerCoin = _coinsRepository.getCoin(event.ticker); // After final Coin? defaultTickerCoin = _coinsRepository.getCoinFromId(event.ticker);This change applies to:
- lib/bloc/bridge_form/bridge_bloc.dart (line 99)
- lib/bloc/bridge_form/bridge_bloc.dart (line 589)
🧰 Tools
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
[info] 99-99: 'getCoin' is deprecated and shouldn't be used. Use KomodoDefiSdk assets or getCoinFromId instead
lib/bloc/taker_form/taker_bloc.dart (1)
242-242: 💡 Verification agent🧩 Analysis chain
Address linting warning for deprecated method.
Similar to BridgeBloc, the pipeline reports that 'getCoin' is deprecated here. Consider updating to the recommended alternatives: KomodoDefiSdk assets or getCoinFromId.
🏁 Script executed:
#!/bin/bash # Check for examples of the recommended alternatives in this file echo "Checking for examples of the recommended methods in this file..." rg "getCoinFromId|KomodoDefiSdk.*assets" lib/bloc/taker_form/taker_bloc.dartLength of output: 217
Let’s locate the
getCoindefinition, its deprecation, and available alternatives across the repo:
🏁 Script executed:
#!/bin/bash set -e echo "1) Usages of getCoin:" rg --context 2 "getCoin\(" -n . echo echo "2) Definitions or mentions of getCoinFromId:" rg --context 2 "getCoinFromId" -n . echo echo "3) References to KomodoDefiSdk (assets API):" rg --context 2 "KomodoDefiSdk" -n . echo echo "4) Any @Deprecated annotations:" rg --context 2 "@Deprecated" -n . echo echo "5) Locate CoinsRepo class to inspect getCoin:" rg --context 2 "class .*CoinsRepo" -n .Length of output: 65903
Running additional verification to inspect
TakerBlocanddefaultDexCointypes:
🏁 Script executed:
#!/bin/bash set -e echo "1) TakerBloc class and constructor:" rg -C10 "class TakerBloc" -n lib/bloc/taker_form/taker_bloc.dart echo echo "2) Declaration and usage of defaultDexCoin:" rg -C5 "defaultDexCoin" -n lib/bloc/taker_form/taker_bloc.dartLength of output: 1597
Replace deprecated
getCoinwithgetCoinFromIdin TakerBlocLine 242 of lib/bloc/taker_form/taker_bloc.dart uses the deprecated
final Coin? defaultCoin = _coinsRepo.getCoin(defaultDexCoin);Replace it with the new ID-based lookup:
- final Coin? defaultCoin = _coinsRepo.getCoin(defaultDexCoin); + final Coin? defaultCoin = _coinsRepo.getCoinFromId(defaultDexCoin);• If
defaultDexCoinis aString, wrap it in anAssetId:final Coin? defaultCoin = _coinsRepo.getCoinFromId(AssetId(defaultDexCoin));• Alternatively, pull directly from the SDK’s assets bag via
_kdfSdk.assets.available.
This silences the lint warning and drops the deprecated API.
🧹 Nitpick comments (1)
lib/blocs/wallets_repository.dart (1)
46-47: Best practice: Using immutable variable declaration.Changed from
vartofinalto ensure immutability, which is a good Dart practice. This prevents unintended reassignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/app_config/app_config.dart(1 hunks)lib/bloc/bridge_form/bridge_bloc.dart(2 hunks)lib/bloc/taker_form/taker_bloc.dart(1 hunks)lib/blocs/wallets_repository.dart(2 hunks)lib/views/dex/simple/form/tables/orders_table/grouped_list_view.dart(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
lib/bloc/bridge_form/bridge_bloc.dart (1)
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.
🪛 GitHub Actions: takenagain is validating code guidelines 🚀
lib/bloc/bridge_form/bridge_bloc.dart
[info] 99-99: 'getCoin' is deprecated and shouldn't be used. Use KomodoDefiSdk assets or getCoinFromId instead
[info] 589-589: 'getCoin' is deprecated and shouldn't be used. Use KomodoDefiSdk assets or getCoinFromId instead
🔇 Additional comments (9)
lib/app_config/app_config.dart (1)
76-77: Documentation improvement for ARRR exclusion.The added comment clearly explains why ARRR is excluded from the list of supported coins, helping other developers understand the reason behind this decision.
lib/blocs/wallets_repository.dart (1)
118-118: Improved exception message formatting.The change simplifies exception handling by directly using string interpolation (
$e) instead of explicitly callingtoString(). This preserves the original exception information better.lib/bloc/bridge_form/bridge_bloc.dart (1)
7-7: Added necessary import for excludedAssetList.Added import for app_config.dart to access the excludedAssetList constant used in the fix.
lib/views/dex/simple/form/tables/orders_table/grouped_list_view.dart (6)
18-18: Improved constructor parameter order.Repositioning
super.keyafter required parameters aligns with Dart best practices and improves code readability.
22-22: Enhanced type safety with specific callback signature.Changing from generic
Function(T)to explicitvoid Function(T)improves type safety and makes the intention clearer that this callback doesn't return a value.
38-38: Better use of predefined constants.Using
EdgeInsets.zeroinstead of creating a new instance withconst EdgeInsets.all(0)is more idiomatic and slightly more efficient.
86-87: Consistent parameter typing and default value.The method signature now correctly specifies the return type for the callback and uses the standard
EdgeInsets.zerofor default padding, improving consistency across the codebase.
109-109: Improved immutability with final variable.Making the
groupedmapfinalprevents accidental reassignment of the variable reference while still allowing the map's contents to be modified - a good practice for maintaining code clarity.
13-128: Overall these changes help prevent the dropdown selection crash.These type safety improvements and better constant usage, while subtle, contribute to the stability of the component that displays filtered coin data. This works in conjunction with the backend filtering changes mentioned in the summary that exclude unsupported coins like ARRR and NFT v2 assets.
|
|
||
| /// Unsupported coins like ARRR cause downstream errors, so we need to | ||
| /// remove them from the list here | ||
| bestOrders.result | ||
| ?.removeWhere((coinId, _) => excludedAssetList.contains(coinId)); | ||
|
|
There was a problem hiding this comment.
Critical bugfix: Filter out unsupported coins to prevent crashes.
This change prevents downstream errors by filtering out coins like 'ARRR' that lack proper activation parameters, addressing the crash mentioned in the PR title.
The implementation correctly uses removeWhere to filter out all coins listed in the excludedAssetList before updating the state with the best orders.
| /// Unsupported coins like ARRR cause downstream errors, so we need to | ||
| /// remove them from the list here | ||
| bestOrders.result | ||
| ?.removeWhere((coinId, _) => excludedAssetList.contains(coinId)); | ||
|
|
There was a problem hiding this comment.
Critical bugfix: Filter out unsupported coins to prevent crashes.
This change applies the same fix as in BridgeBloc to filter out unsupported coins from the best orders list, ensuring consistency between the two implementations and preventing crashes during coin selection.
The implementation correctly uses removeWhere to filter out all coins listed in the excludedAssetList before updating the state with the best orders.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash on the buy coin dropdown by filtering out unsupported coins (like ARRR) and removing NFT_* coins from the default enabled list. Key changes include adjustments to type safety in callbacks, proper exclusion of unsupported coins in the best orders logic in both taker and bridge blocs, and minor refactoring in the wallets repository to improve code clarity.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/views/dex/simple/form/tables/orders_table/grouped_list_view.dart | Adjusted parameter ordering and improved type safety using void callbacks, and replaced EdgeInsets.all(0) with EdgeInsets.zero. |
| lib/blocs/wallets_repository.dart | Replaced mutable variable with a final variable for clearer intent and simplified exception string interpolation. |
| lib/bloc/taker_form/taker_bloc.dart | Added filtering of unsupported coins from best orders to prevent downstream errors. |
| lib/bloc/bridge_form/bridge_bloc.dart | Introduced app_config import and similar filtering of unsupported coins to prevent errors. |
| lib/app_config/app_config.dart | Removed NFT_* coins from the enabled-by-default coins list and added explanatory comments. |
takenagain
requested review from
AndrewDelaney,
CharlVS,
ShantanuSharma9873 and
smk762
smk762
left a comment
smk762
left a comment
There was a problem hiding this comment.
- No crash on buy coin selection ✔️
- No NFT related activation error in log stream ✔️
LGTM, thanks!
Changes
best_ordersresponse, coins list, and fiat onramp to prevent downstream errors from unsupported coins. ARRR is returned in thebest_ordersresponse, despite not being supported (not yet implemented - activation failes), which caused the buy coin dropdown to crash.NFT_*coins from the enabled-by-default coins list to fix the failed activation log spam in console. The NFT page handles required coin activations, so inclusion in this list is unnecessary. The auto-activation does not provide the required parameters for activation, resulting in failed activation and subsequent periodic reactivation attempts.Testing
Local debug build is required to access the Swap page to confirm that the buy coin selection dropdown does not cause the page/application to crash as reported by @DeckerSU
Summary by CodeRabbit