-
Notifications
You must be signed in to change notification settings - Fork 219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(add-coins): improve "Add Assets" coin list loading speed #2522
Conversation
legacy fee formatting was causing exceptions when attempting to parse the already formatted string as double
- use sdk to get enabled assets list rather than - reduce the number of coins manager bloc refreshes (calls to asset->coin conversion functions)
ease transition to SDK by allowing supporting code to transition to using AssetId rather than Coin or coin ticker / abbreviation
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 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 0aaad21): https://walletrc--pull-2522-merge-rxbgrnpl.web.app (expires Tue, 18 Feb 2025 08:28:21 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, and another big step to bringing the codebase to the standard we've been pushing for.
on<CoinsManagerCoinsUpdate>(_onCoinsUpdate); | ||
on<CoinsManagerCoinsListReset>(_onResetCoinsList); | ||
on<CoinsManagerCoinTypeSelect>(_onCoinTypeSelect); | ||
on<CoinsManagerCoinsSwitch>(_onCoinsSwitch); | ||
on<CoinsManagerCoinSelect>(_onCoinSelect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: avoid using irregular verbs for bloc event names, or at least use a compound verb (with the final verb being regular) since it keeps the naming unambiguous and consistent with the recommended conventions and prevents confusion. This way, readers can more easily predict and understand event behaviour, aligning with the Bloc library's best practices.
However, no changes are needed in this context even if the intended meaning follows the conventions since it would be consistent with the existing code (albeit not following.
on<CoinsManagerCoinsUpdate>(_onCoinsUpdate); | ||
on<CoinsManagerCoinsListReset>(_onResetCoinsList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the conventions and the rest of the handlers, the handler function name should follow _on{Noun (optional)}{Verb (event)}
on<CoinsManagerCoinsListReset>(_onResetCoinsList); | |
on<CoinsManagerCoinsListReset>(_onCoinsListReset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed along with the formatting changes in 0aaad21
@takenagain, also, please enable the |
Reduces the number of calls to RPC-heavy conversion functions in
CoinsManagerBloc
andCoinsRepo
. Also cherry-picked from the custom token import PR to add theAssetId
to theCoin
model to ease the migration from string tickers.Before:
Screen.Recording.2025-02-09.at.23.25.59.mov
After:
Screen.Recording.2025-02-09.at.23.17.45.mov