-
Notifications
You must be signed in to change notification settings - Fork 222
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
fix(router): frozen layout on navigation and resize #2521
Conversation
fixes non-responsive UI due to immutable MainLayout ignoring routing and layout size changes
the use of `.single` in `getSdkAsset` would result in exceptions for coins that were not in the known assets list
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 8794b15): https://walletrc--pull-2521-merge-9w50wxbr.web.app (expires Fri, 14 Feb 2025 17:43:11 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
// filter out assets that are not in the known coins list to | ||
// prevent errors when using the SDK with the `.single` property | ||
.where((coin) => sdk.assets.assetsFromTicker(coin.id).length == 1) |
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.
My intention with using the .single
property instead of .first
/.firstOrNull
was to intentionally throw an exception if there is more than one asset for a given ticker. This was possible before the SLP project was abandoned.
Although unlikely, if the coins repo has a change pushed to it where this assumption no longer holds true, then we want to error out because there are many instances where we've assumed a given ticker will have exactly 1 or 0 coins/assets, which may wreak havoc if it's not treated as a breaking change.
Unless the surrounding context makes it redundant, please change this to:
// filter out assets that are not in the known coins list to | |
// prevent errors when using the SDK with the `.single` property | |
.where((coin) => sdk.assets.assetsFromTicker(coin.id).length == 1) | |
// filter out assets that are not in the known coins list to | |
// prevent errors when using the SDK with the `.single` property | |
.where((coin) => sdk.assets.assetsFromTicker(coin.id).length >= 1) |
Also, we could add an SDK method for sdk.assets.maybeAssetsFromTicker
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.
The await cexPriceRepository.getCoinList()
function on line 34 above returns a list of coins from Binance, some of which are not in our coins repository / config. Line 38 filters out assets that are not in the “known”/available assets list, or that might have more than one asset for a given ticker.
The entire application crashes in the guest/unauthenticated Wallet page because of the .single
call in getSdkAsset throwing an exception in the UI layer (no entries in the set).
I updated the comment to clarify the purpose of the filter in 8ff69c2
Although unlikely anytime soon, if the coins repo has a change pushed to it where this assumption no longer holds true, then we want to error out because there are many instances where we've assumed a given ticker will have exactly 1 or 0 coins/assets, which may wreak havoc if it's not treated as a breaking change.
Changes
pubspec.lock
(dev
branch is using an outdated SDK release)