Skip to content

fix(custom-token-import): refresh asset list on import and use lowercase for custom token import#220

Merged
CharlVS merged 6 commits intodevfrom
bugfix/custom-token-import
Sep 11, 2025
Merged

fix(custom-token-import): refresh asset list on import and use lowercase for custom token import#220
CharlVS merged 6 commits intodevfrom
bugfix/custom-token-import

Conversation

@takenagain
Copy link
Copy Markdown
Contributor

@takenagain takenagain commented Sep 11, 2025

Summary by CodeRabbit

  • New Features
    • Custom tokens now refresh automatically after activation, updating asset lists without restarting.
    • Custom token lists respect current filters and update when filter settings change.
  • Bug Fixes
    • Custom token icons load reliably regardless of symbol case.
    • Improved stability when loading custom tokens only after sign-in.
    • More accurate storage of custom token history improves consistency across sessions.

@takenagain takenagain self-assigned this Sep 11, 2025
@takenagain takenagain added the bug Something isn't working label Sep 11, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 11, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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 skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds an asset refresh notifier interface and wires ActivationManager to trigger a refresh after activating custom tokens. AssetManager now implements the notifier and refactors custom-token refresh/filtering. Custom token history serialization switches to protocol config. UI icon resolver standardizes custom icon cache keys to lowercase. Bootstrap updated to pass the notifier.

Changes

Cohort / File(s) Summary
Activation completion refresh wiring
packages/komodo_defi_sdk/lib/src/activation/activation_manager.dart, packages/komodo_defi_sdk/lib/src/bootstrap.dart
ActivationManager accepts a new IAssetRefreshNotifier dependency and calls notifyCustomTokensChanged() after a group activation if any custom tokens were activated. Bootstrap updated to pass the notifier (AssetManager) to the constructor.
Notifier interface
packages/komodo_defi_sdk/lib/src/assets/asset_lookup.dart
Adds abstract interface class IAssetRefreshNotifier with void notifyCustomTokensChanged() for signaling custom-token changes.
AssetManager refresh and filtering
packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart
AssetManager now implements IAssetRefreshNotifier. Introduces _refreshCustomTokens, filter-application for custom tokens, sign-in checks, defensive iteration, unmodifiable available map, logging, and exposes notifyCustomTokensChanged() to trigger refresh.
Custom asset history serialization
packages/komodo_defi_sdk/lib/src/assets/custom_asset_history_storage.dart
Stores asset.protocol.config instead of asset.toJson() when persisting wallet assets.
UI custom icon keying
packages/komodo_ui/lib/src/defi/asset/asset_icon.dart
Caches custom icons using a lowercase configSymbol key for consistent, case-insensitive lookups; minor whitespace tweaks.

Sequence Diagram(s)

sequenceDiagram
  participant UI
  participant ActivationManager
  participant IAssetRefreshNotifier as AssetRefreshNotifier
  participant AssetManager

  Note over ActivationManager: Activation group completes
  ActivationManager->>ActivationManager: Detect activated assets
  alt Any custom tokens activated
    ActivationManager->>AssetRefreshNotifier: notifyCustomTokensChanged()
    note right of AssetRefreshNotifier: Implemented by AssetManager
    AssetRefreshNotifier-->>ActivationManager: return
    AssetRefreshNotifier->>AssetManager: refresh custom tokens
  else No custom tokens
    ActivationManager-->>ActivationManager: No notifier call
  end
Loading
sequenceDiagram
  participant AssetManager
  participant Auth
  participant WalletAPI as Wallet Assets
  participant Filter as FilterStrategy
  participant Store as In-memory Coins

  AssetManager->>AssetManager: notifyCustomTokensChanged()
  AssetManager->>Auth: isSignedIn?
  alt Not signed in
    AssetManager-->>AssetManager: Return (debug log)
  else Signed in
    AssetManager->>WalletAPI: fetch wallet assets
    WalletAPI-->>AssetManager: assets (custom tokens)
    AssetManager->>Filter: apply filter on tokens
    Filter-->>AssetManager: filtered tokens
    AssetManager->>Store: insert/update ordered coins
    Store-->>AssetManager: done
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • CharlVS
  • smk762

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title accurately summarizes the primary fixes in the changeset: it notes that the asset list is refreshed when custom tokens are imported/activated and that custom token identifiers are normalized to lowercase for import/icon lookup, which maps to the ActivationManager/AssetManager and AssetIcon changes in the diff; it is concise and focused on the main user-facing behavior.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Poem

A rabbit taps the ledger’s drum,
New tokens hop, the notifiers hum.
Assets refreshed, in tidy rows,
Lowercased icons—every nose knows.
History penned in configs tight,
We ship at dusk, we ship at night.
Hippity hop—deploying right! 🐇✨

✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix/custom-token-import

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@takenagain takenagain marked this pull request as ready for review September 11, 2025 11:06
@takenagain
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Copilot AI review requested due to automatic review settings September 11, 2025 11:06
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Sep 11, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes custom token import functionality by implementing lowercase asset ID handling and refreshing the asset list when new custom tokens are imported.

  • Implements lowercase normalization for custom token asset IDs to ensure consistent matching
  • Adds automatic asset list refresh when custom tokens are imported through activation
  • Refactors asset manager to support proper filtering and refresh of custom tokens

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/komodo_ui/lib/src/defi/asset/asset_icon.dart Adds lowercase normalization for custom token asset IDs
packages/komodo_defi_sdk/lib/src/bootstrap.dart Passes asset manager as refresh notifier to activation manager
packages/komodo_defi_sdk/lib/src/assets/custom_asset_history_storage.dart Switches to protocol config for asset storage to fix serialization issues
packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart Implements refresh notification interface and adds custom token filtering
packages/komodo_defi_sdk/lib/src/assets/asset_lookup.dart Adds asset refresh notifier interface
packages/komodo_defi_sdk/lib/src/activation/activation_manager.dart Integrates refresh notification when custom tokens are activated
Comments suppressed due to low confidence (1)

packages/komodo_defi_sdk/lib/src/bootstrap.dart:1

  • The same assetManager instance is passed twice as consecutive parameters. This appears to be a duplicate parameter that should be reviewed to ensure the correct dependencies are being injected.
// ignore_for_file: cascade_invocations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
packages/komodo_defi_sdk/lib/src/assets/custom_asset_history_storage.dart (1)

24-31: Consider stronger de-duplication on insert

Manual ID check avoids duplicates, but Set still depends on equality elsewhere. Optionally normalize by ID and re-store to ensure a canonical set.

packages/komodo_defi_sdk/lib/src/bootstrap.dart (1)

105-113: Constructor wiring looks correct; prefer named args for clarity

Passing assetManager twice (as lookup and notifier) is valid, but easy to misorder. Consider switching ActivationManager to named parameters to prevent future mix-ups.

packages/komodo_defi_sdk/lib/src/activation/activation_manager.dart (2)

44-46: Timeout behavior is fine

Throwing inside onTimeout is acceptable; alternatively rely on the default TimeoutException to simplify.


78-82: Parent resolution hints grouping fragility

Needing to resolve parentAsset here suggests _groupByPrimary can produce a child as primary. Consider fixing grouping to always use the top-level parent as primary to simplify downstream logic.

packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart (1)

266-274: LGTM: async refresh with error logging

Consider debouncing if high-frequency notifications become an issue.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa95fe8 and 9418d06.

📒 Files selected for processing (6)
  • packages/komodo_defi_sdk/lib/src/activation/activation_manager.dart (6 hunks)
  • packages/komodo_defi_sdk/lib/src/assets/asset_lookup.dart (1 hunks)
  • packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart (9 hunks)
  • packages/komodo_defi_sdk/lib/src/assets/custom_asset_history_storage.dart (1 hunks)
  • packages/komodo_defi_sdk/lib/src/bootstrap.dart (1 hunks)
  • packages/komodo_ui/lib/src/defi/asset/asset_icon.dart (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Cursor Bugbot
  • GitHub Check: Cursor Bugbot
  • GitHub Check: build_and_preview_playground_preview
  • GitHub Check: Flutter tests (all packages)
🔇 Additional comments (16)
packages/komodo_ui/lib/src/defi/asset/asset_icon.dart (2)

55-64: LGTM: minor layout cleanup

No functional change here.


241-272: LGTM: retrieval path matches canonical lowercase key

Build-time lookup uses _sanitizedId, consistent with the write-key change.

packages/komodo_defi_sdk/lib/src/assets/asset_lookup.dart (1)

29-34: Simple, clear notifier interface

Good separation of concerns and minimal surface.

packages/komodo_defi_sdk/lib/src/bootstrap.dart (1)

65-81: Cycle avoidance check

Using ValueGetter<ActivationManager> breaks the hard cycle—nice. Verify no early calls to _activationManager() happen during AssetManager.init().

packages/komodo_defi_sdk/lib/src/activation/activation_manager.dart (4)

22-23: LGTM: notifier dependency added

Constructor and field addition are coherent with the new notifier flow.


137-144: LGTM: enabled-coins resolution via config IDs

Mapping RPC tickers to assets through findAssetsByConfigId is consistent with the new strategy-based filtering.


209-212: Good: notify refresh only when custom tokens were activated

Prevents unnecessary refresh churn.


238-245: LGTM: active assets aggregation

Matches the status check logic above.

packages/komodo_defi_sdk/lib/src/assets/asset_manager.dart (8)

1-4: LGTM: targeted imports

Bringing in unawaited and debugPrint is appropriate.


46-46: LGTM: implements notifier on the manager

Right place to centralize refresh behavior.


118-123: LGTM: strategy change also refreshes custom tokens

Good to keep custom tokens aligned with current filter.


170-175: LGTM: defensive error on use-before-init

Clear message aids callers.


181-182: LGTM: unmodifiable view

Prevents external mutation.


215-218: LGTM: defensive copy to avoid concurrent modification

Appropriate safeguard.


231-238: LGTM: child lookup uses defensive copy

Consistent with the above; avoids iteration invalidation.


281-288: LGTM: strategy-based filtering for custom tokens

Using token.protocol.config aligns with the rest of the PR.

@takenagain takenagain requested a review from CharlVS September 11, 2025 11:28
Copy link
Copy Markdown
Collaborator

@CharlVS CharlVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYSM.

Please use .of instead of .from

@takenagain
Copy link
Copy Markdown
Contributor Author

TYSM.

Please use .of instead of .from

Done in fad851b

Enum values are a mix of ticker, formatted, and token suffix values,
@CharlVS CharlVS merged commit b0a6f1e into dev Sep 11, 2025
5 of 9 checks passed
@CharlVS CharlVS deleted the bugfix/custom-token-import branch September 11, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants