fix(market-metrics): hd mode switching values shown, graph interaction, and loading speed #2558
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 WalkthroughThis pull request refactors several core components across the application. It updates bloc and repository classes to support asynchronous operations and shifts dependency injection from legacy parameters to a centralized SDK (KomodoDefiSdk). Changes include enhanced error handling, standardized logging, and modifications to serialization methods for HD wallet support. Additionally, new extension methods for wallet coin retrieval and SDK-based coin activation have been introduced, and a new logging dependency has been added. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant W as WalletPage
participant SDK as KomodoDefiSdk
participant Ext as KdfAuthMetadataExtension
U->>W: Initiate wallet load
W->>SDK: getWalletCoins()
SDK->>Ext: Retrieve wallet coin IDs and map to Coin objects
Ext-->>SDK: Return list of Coins
SDK-->>W: Send wallet coins
W->>U: Display updated wallet data
sequenceDiagram
participant Client as Client Code
participant SDK as KomodoDefiSdk
participant Ext as SdkAuthActivationExtension
Client->>SDK: waitForEnabledCoinsToPassThreshold(coins, threshold, timeout)
SDK->>Ext: Start threshold check
Ext->>SDK: Verify enabled coins via _areEnabledCoinsAboveThreshold
SDK-->>Ext: Return check result (true/false)
Ext-->>Client: Return threshold check outcome
Suggested reviewers
Poem
🪧 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 (
|
takenagain
changed the title
|
Visit the preview URL for this PR (updated for commit d2c9908): https://walletrc--pull-2558-merge-owssmdmn.web.app (expires Fri, 14 Mar 2025 15:16:02 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
switching between HD and non-HD mode for the same wallet results in the incorrect balance and market metrics graphs being shown
takenagain
force-pushed
the
bugfix/coin-profit-loss
branch
from
19b90ff to
034309b
Compare
- remove unused coinsRepo reference - move default enabled coins initialization to auth bloc - wait for enabled coins to pass threshold - load historical wallet coins instead of coins state wallet coins
coderabbitai suggestion
|
@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: 0
🔭 Outside diff range comments (3)
lib/bloc/cex_market_data/models/graph_cache.dart (2)
73-73:⚠️ Potential issueMissing HD wallet flag in toJson method
The
isHdWalletflag is missing from thetoJsonmethod, which could cause issues when persisting or serializing this object.Add the HD wallet flag to the JSON output:
def toJson() { return { 'coinId': coinId, 'fiatCoinId': fiatCoinId, 'lastUpdated': lastUpdated.toIso8601String(), 'portfolioGrowthGraphs': graph, 'graphType': graphType, 'walletId': walletId, + 'isHdWallet': isHdWallet, }; }
95-104:⚠️ Potential issueMissing isHdWallet in props list
The
isHdWalletfield is not included in thepropslist, which means it won't be considered during equality comparisons or when generating hash codes.Add the property to the props list:
@override List<Object?> get props => [ coinId, fiatCoinId, lastUpdated, graph, graphType, walletId, + isHdWallet, ];lib/bloc/app_bloc_root.dart (1)
144-148:⚠️ Potential issueSynchronous invocation of now-async method
The method
_clearCachesIfPerformanceModeChangedhas been updated to be asynchronous but is still being called synchronously without await.Update the call to use await:
- _clearCachesIfPerformanceModeChanged( + await _clearCachesIfPerformanceModeChanged( performanceMode, profitLossRepo, portfolioGrowthRepo, );
🧹 Nitpick comments (17)
lib/shared/utils/utils.dart (1)
232-232: Consider using formal deprecation annotation instead of TODO commentThe TODO comment indicates an intention to deprecate the
logfunction. For better code clarity and tooling support, consider using Dart's@Deprecatedannotation with a message suggesting the alternative.-// TODO: deprecate +@Deprecated('Use package:logging instead. See lib/services/logger/get_logger.dart for implementation details.') Future<void> log( String message, { String? path, StackTrace? trace, bool isError = false, }) async {lib/services/logger/get_logger.dart (1)
87-115: Consider adding severity-based colors to developer logsThe severity-based handling of logs is good, but consider enhancing developer logs with visual cues like colors when logging to the console for better readability.
Also, consider adding a test for these new logging functions to ensure they work as expected across different environments.
lib/views/wallet/wallet_page/wallet_main/wallet_main.dart (1)
157-166: Improved wallet data loading to handle HD mode switching.The refactoring from using potentially stale
CoinsBlocstate to directly retrieving wallet coins via the SDK is a solid improvement that addresses the core issue with HD mode switching values. This ensures data accuracy regardless of when the user signs in.The comments clearly document the rationale and future improvement plans.
Consider implementing the event-based approach mentioned in the TODO comment as soon as the SDK supports balance events, to further improve the reactivity of the UI.
lib/bloc/coins_bloc/coins_repo.dart (2)
226-237: Standardized logging in activateAssetsSyncThis change improves the log message format and properly uses the warning log level for non-critical situations. Note there's a closing curly brace typo in the log message - it should be
[$coinIdList]instead of[$coinIdList}].- 'No wallet signed in. Skipping activation of [$coinIdList}]', + 'No wallet signed in. Skipping activation of [$coinIdList]',
274-277: Same logging format issue in activateCoinsSyncThe same curly brace typo appears in this log message as well.
- 'No wallet signed in. Skipping activation of [$coinIdList}]', + 'No wallet signed in. Skipping activation of [$coinIdList]',lib/bloc/app_bloc_root.dart (1)
136-142: Consistent SDK usage for PortfolioGrowthRepositoryThe code has been updated to use the SDK parameter, but still includes the
coinsRepositoryparameter which looks redundant since we're now using the SDK.Consider removing the redundant
coinsRepositoryparameter:final portfolioGrowthRepo = PortfolioGrowthRepository.withDefaults( transactionHistoryRepo: transactionsRepo, cexRepository: binanceRepository, demoMode: performanceMode, - coinsRepository: coinsRepository, sdk: komodoDefiSdk, );lib/bloc/coins_bloc/coins_bloc.dart (1)
270-276: Missing logging in _onCoinsDeactivatedThe code logs at the beginning and end of coin deactivation, but not for any potential errors in the deactivation process.
Consider adding a try-catch block around the deactivation operations to catch and log any errors:
Future<void> _onCoinsDeactivated( CoinsDeactivated event, Emitter<CoinsState> emit, ) async { for (final coinId in event.coinIds) { final coin = state.walletCoins[coinId]!; _log.info('Disabling a ${coin.name} ($coinId)'); coin.reset(); + try { await _kdfSdk.removeActivatedCoins([coin.id.id]); await _mm2Api.disableCoin(coin.id.id); final newWalletCoins = Map<String, Coin>.of(state.walletCoins) ..remove(coin.id.id); final newCoins = Map<String, Coin>.of(state.coins); newCoins[coin.id.id]!.state = CoinState.inactive; emit(state.copyWith(walletCoins: newWalletCoins, coins: newCoins)); _log.info('${coin.name} has been disabled'); + } catch (e, s) { + _log.shout('Error disabling coin: ${coin.id.id}', e, s); + } } }lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dart (3)
89-99: Catch block for cached chart loading.
Using.catchError(...)with a fallback to an error state is a solid approach. If you need finer-grained handling for different error types, consider throwing custom exceptions in the repository and catching them here to provide more specific messaging.
102-106: Threshold-based load logic.
Waiting until 50% of coins are enabled before loading the uncached chart can prevent partial data issues. If you anticipate large coin sets or different thresholds for some users, making this configurable might be a future enhancement.
125-138: Periodic updates and error handling.
Usingemit.forEach(...)for periodic refresh is helpful, but ensure any partial or intermittent errors within the stream do not leave the user stuck if subsequent data can still load. The fallback toGrowthChartLoadFailureis fine, but additional nuance in error retries might be considered.lib/bloc/cex_market_data/profit_loss/models/profit_loss_cache.dart (1)
30-32: **Fix the minor grammar error in the doc comment **“Same [walletId] can be use for both” should read “Same [walletId] can be used for both.”
- /// Whether the wallet is an HD wallet. Same [walletId] can be use for both + /// Whether the wallet is an HD wallet. Same [walletId] can be used for bothlib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart (4)
49-92: **Robust error handling and fallback strategy **Overall, the
tryblock with cached and uncached retrieval flows is clear. A couple of considerations:
- If an exception occurs in the “uncached” section, you are silently ignoring it within the
catchErrorcall. Confirm this is always desirable.- For maintainability, consider extracting some logic into dedicated methods, reducing the length of
_onLoadPortfolioProfitLoss.Would you like a quick refactor snippet?
127-129: **_log.shoutusage might be too severe for certain errors **
_log.shoutis suitable for very critical issues. Consider whether_log.severeor_log.warningmight suffice, especially for common network or partial server errors.
159-161: **Method name tweak suggestion **
_removeUnsupportedConsmight be more accurately named_removeUnsupportedCoins. While this is minor, clarity aids readability.-future<List<Coin>> _removeUnsupportedCons(List<Coin> walletCoins, String fiatCoinId) +future<List<Coin>> _removeUnsupportedCoins(List<Coin> walletCoins, String fiatCoinId)
201-205: **Handle missing wallets more gracefully **Throwing an exception if
currentWalletis null is correct but abrupt. Consider returning an error state or logging a specific user-facing message that clarifies the absence of an authenticated wallet.lib/bloc/assets_overview/bloc/asset_overview_bloc.dart (2)
73-73: Consider Using a More Common Logging Level
_log.shoutmay be too severe in production contexts; opting for_log.severe(or_log.warning) is often more conventional for error logs.
116-116: Repeated Logging Level Suggestion
Similarly,_log.shout('Failed to load portfolio assets overview', e, s);might be replaced with_log.severe(...)for typical error logging practice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
lib/bloc/app_bloc_root.dart(4 hunks)lib/bloc/assets_overview/bloc/asset_overview_bloc.dart(7 hunks)lib/bloc/auth_bloc/auth_bloc.dart(3 hunks)lib/bloc/cex_market_data/mockup/generator.dart(2 hunks)lib/bloc/cex_market_data/mockup/mock_portfolio_growth_repository.dart(1 hunks)lib/bloc/cex_market_data/mockup/mock_transaction_history_repository.dart(1 hunks)lib/bloc/cex_market_data/models/adapters/graph_cache_adapter.dart(2 hunks)lib/bloc/cex_market_data/models/graph_cache.dart(6 hunks)lib/bloc/cex_market_data/portfolio_growth/cache_miss_exception.dart(1 hunks)lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dart(5 hunks)lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_repository.dart(14 hunks)lib/bloc/cex_market_data/profit_loss/demo_profit_loss_repository.dart(2 hunks)lib/bloc/cex_market_data/profit_loss/models/adapters/profit_loss_adapter.dart(1 hunks)lib/bloc/cex_market_data/profit_loss/models/adapters/profit_loss_cache_adapter.dart(3 hunks)lib/bloc/cex_market_data/profit_loss/models/profit_loss_cache.dart(2 hunks)lib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart(8 hunks)lib/bloc/cex_market_data/profit_loss/profit_loss_calculator.dart(1 hunks)lib/bloc/cex_market_data/profit_loss/profit_loss_repository.dart(5 hunks)lib/bloc/cex_market_data/sdk_auth_activation_extension.dart(1 hunks)lib/bloc/coins_bloc/asset_coin_extension.dart(2 hunks)lib/bloc/coins_bloc/coins_bloc.dart(20 hunks)lib/bloc/coins_bloc/coins_repo.dart(18 hunks)lib/model/kdf_auth_metadata_extension.dart(2 hunks)lib/model/wallet.dart(1 hunks)lib/services/logger/get_logger.dart(2 hunks)lib/shared/utils/utils.dart(1 hunks)lib/views/wallet/wallet_page/wallet_main/wallet_main.dart(4 hunks)pubspec.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Deploy Preview
- GitHub Check: Test web-app-macos
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Desktop (windows)
- GitHub Check: build_and_preview
- GitHub Check: Test web-app-linux-profile
- GitHub Check: validate_code_guidelines
- GitHub Check: unit_tests
- GitHub Check: Build Mobile (iOS)
🔇 Additional comments (104)
lib/bloc/cex_market_data/profit_loss/models/adapters/profit_loss_adapter.dart (1)
2-3: LGTM! Improved import organization.Changing from relative to absolute imports enhances code maintainability and makes import paths more explicit and consistent.
lib/services/logger/get_logger.dart (2)
47-48: Good logging initialization with environment-based level settingsSetting different log levels based on the environment is a good practice that ensures verbose logs in development while keeping production logs focused on important information.
51-85: Well-structured logging implementation with performance trackingThe implementation correctly replicates the functionality from the utils.dart log function while adding performance tracking and proper level-based logging. Good error handling ensures logs don't break the application.
pubspec.yaml (1)
64-64: LGTM! Added logging dependencyAdding the logging package as a direct dependency is appropriate since you're now using it for structured logging throughout the application.
lib/bloc/cex_market_data/profit_loss/profit_loss_calculator.dart (1)
2-2: Clean import organization.Adding the direct import for
komodo_defi_typesmakes dependencies more explicit rather than relying on transitive imports through other packages.lib/bloc/cex_market_data/portfolio_growth/cache_miss_exception.dart (1)
1-11: Well-structured custom exception class.The
CacheMissExceptionclass provides more specific error handling than a generic Exception, improving debugging and error reporting capabilities. The class has good documentation, a clear constructor, and a usefultoString()implementation.lib/bloc/cex_market_data/mockup/generator.dart (2)
19-19: Field declaration repositioned after constructors.Good structural improvement for better code organization.
29-30: Simplified variable declaration with type inference.Using
varwith the explicit cast injson.decodesimplifies the code while maintaining type safety.lib/bloc/auth_bloc/auth_bloc.dart (3)
7-7: Added import for app configuration.Good addition to support the new coin activation functionality.
141-142: Added default coin activation in registration flow.This ensures newly registered wallets have default coins activated immediately, addressing the issue with balances and graphs display when switching modes.
187-188: Added default coin activation in wallet restoration flow.Consistent implementation with the registration flow ensures restored wallets also have default coins activated, maintaining a uniform user experience.
lib/model/wallet.dart (1)
169-169: Good simplification of activated coins retrieval.The code now directly accesses
activated_coinsfrom metadata instead of using a removed_parseActivatedCoinsmethod. This simplifies the logic and removes conditional checks for wallet types, making the code more maintainable.lib/model/kdf_auth_metadata_extension.dart (3)
3-4: Appropriate imports added for new functionality.The new imports for
asset_coin_extension.dartandcoin.dartsupport the newly added wallet coin retrieval methods.
18-21: Good implementation of coin IDs retrieval method.This method cleanly retrieves the current user's activated coin IDs from metadata, with a sensible empty list fallback if the user is not found.
23-31:❓ Verification inconclusive
Verify coin retrieval behavior with multiple assets having the same ticker.
The implementation maintains existing behavior where the application will crash if multiple assets share the same ticker (by using
.single). This approach is explicitly noted in the comments, but consider if a more graceful fallback might be beneficial in the future.
🏁 Script executed:
#!/bin/bash # Search for any place where multiple assets might be handled for the same ticker rg -A 3 -B 3 "findAssetsByTicker" --glob "*.dart"Length of output: 2021
Action Required: Confirm Duplicate Ticker Handling Strategy
The current implementation in
lib/model/kdf_auth_metadata_extension.dartcontinues to use.singlewhen converting asset data—mirroring the approach seen inlib/coins_bloc/coins_repo.dart—which will trigger a runtime crash if more than one asset is found for a given ticker. Notably, the behavior is consistent with other parts of the codebase; for example, inlib/bloc/custom_token_import/data/custom_token_import_repository.dart, an explicit length check is performed to provide a controlled exception when duplicate entries are encountered.
- lib/model/kdf_auth_metadata_extension.dart: Uses
.singleto enforce unique asset entries.- lib/coins_bloc/coins_repo.dart: Also relies on
.singlewhich will crash on duplicates.- lib/bloc/custom_token_import/data/custom_token_import_repository.dart: Handles duplicate assets by checking the assets list length before throwing an exception.
While this behavior is intentional and explicitly documented in comments, please verify whether maintaining this strict crash behavior is the desired long-term strategy. You might consider exploring a more graceful fallback for handling cases with duplicate tickers in the future.
lib/bloc/coins_bloc/asset_coin_extension.dart (2)
21-24: Improved ProtocolData initialization with proper fallbacks.The new implementation properly handles null values and establishes a clear hierarchy of fallbacks (parent ID → platform → empty string) for the platform field.
54-55: Good addition of platform getter.This getter provides a clean way to access the platform information from protocol configuration, promoting code reuse and maintainability.
lib/bloc/cex_market_data/mockup/mock_portfolio_growth_repository.dart (3)
17-17: Appropriate SDK dependency injection.The change replaces a direct dependency on
Mm2Apiwith the SDK, aligning with modern dependency injection patterns.
22-24: Simplified constructor parameters using parent class properties.The constructor now leverages parent class properties rather than requiring separate parameters, reducing redundancy.
37-38:✅ Verification successful
Verify that this variable placement doesn't affect behavior.
The
performanceModefield declaration has been moved to the end of the class. While this shouldn't affect functionality, ensure that this placement doesn't impact initialization order or other behaviors.
🏁 Script executed:
#!/bin/bash # Search for places where performanceMode might be accessed during initialization rg -A 5 -B 5 "performanceMode" --glob "lib/bloc/cex_market_data/mockup/*.dart"Length of output: 6932
Field Declaration Order Verified – No Behavioral Impact
After reviewing the usage of
performanceModeacross the repository, it’s clear that its relocation to the end of the class does not impact the functionality. In both theMockPortfolioGrowthRepositoryand related classes,performanceModeis passed in as a constructor parameter and used appropriately. Since Dart initializes final fields using constructor parameters regardless of their declaration order, this change won’t affect initialization order or behavior.
- Verified that all references to
performanceModerely on constructor injection rather than field order.- Confirmed consistent behavior in both
MockPortfolioGrowthRepositoryandMockTransactionHistoryRepo.lib/views/wallet/wallet_page/wallet_main/wallet_main.dart (2)
7-7: Added required SDK imports for wallet coin retrieval.These imports allow the usage of the SDK for direct wallet coin retrieval and access to HD wallet extension methods.
Also applies to: 24-24
281-284: LGTM! Variable declarations reordered for better readability.The reordering of member variables in
_SliverSearchBarDelegateimproves code organization without changing functionality.lib/bloc/cex_market_data/profit_loss/demo_profit_loss_repository.dart (2)
2-2: Refactored repository to use SDK directly for better interface consistency.The transition from using
coinsRepositoryto theKomodoDefiSdkaligns with the larger codebase refactoring and provides a more consistent interface for accessing wallet data. This should help resolve issues with HD mode switching.Also applies to: 18-18, 23-23, 42-42
46-46: LGTM! Field declaration repositioned for better code organization.Moving the
performanceModefield declaration to the end of the class is a minor structural improvement.lib/bloc/cex_market_data/sdk_auth_activation_extension.dart (2)
6-52: Well-implemented extension for monitoring coin activation status.This extension provides a robust solution for waiting until a sufficient number of wallet coins are enabled before attempting to load portfolio data. The implementation includes:
- Proper error validation for timeout and delay parameters
- Configurable threshold for activation percentage
- Detailed logging for monitoring and debugging
- Clear timeout handling
This directly addresses the issue with graphs not loading during wallet login or mode switching by ensuring data is only requested when coins are ready.
54-73: Thorough threshold validation with appropriate error handling.The private helper method includes comprehensive validation:
- Checks for empty wallet coin sets
- Validates threshold range (must be between 0 and 1)
- Efficiently uses set operations to determine the intersection of enabled and wallet coins
The implementation is secure, efficient, and follows best practices.
lib/bloc/cex_market_data/models/adapters/graph_cache_adapter.dart (2)
25-26: Added backward-compatible HD wallet flag for graph caching.Excellent approach to maintaining backward compatibility by conditionally loading the
isHdWalletfield with a sensible default. This ensures that existing cached data will continue to work correctly.
33-33: Updated serialization to include HD wallet status.The serialization has been properly updated to include the new
isHdWalletfield:
- Increased the field count from 6 to 7
- Added the new field at byte index 6
This change enables the app to differentiate between HD and non-HD wallet graphs, which is essential for the requirements in this PR.
Also applies to: 46-47
lib/bloc/coins_bloc/coins_repo.dart (7)
10-13: Good addition of standardized logging packageAdding the
loggingpackage is a good step towards standardized logging across the application, replacing the previously ad-hoc logging approach.
50-51: Well-named logger instanceCreating a namespaced logger with the class name follows best practices for structured logging.
100-110: Improved error reportingChanging from error to warning log level is appropriate for this non-critical case where multiple or no results are returned.
136-149: Cleaner implementation of getEnabledCoinThe method has been refactored to use
getEnabledCoins()instead ofassetsFromTicker(), with a more streamlined check for existence. This improves readability and maintainability.
293-294: Consistent use of coin identifierRefactoring to use
coin.id.idconsistently throughout the codebase makes the code more maintainable and less error-prone.
453-457: Consistent coin identifier usage in pricing logicUsing
coin.id.idas the map key ensures consistency with other parts of the codebase and makes the code more maintainable.
497-499: Consistent cache referencingUsing
coins[i].id.idas the cache key is consistent with other changes, providing a uniform approach to coin identification across the repository.lib/bloc/cex_market_data/models/graph_cache.dart (6)
1-3: Good addition of type utilities importAdding the import for type utilities supports the new functionality for HD wallet differentiation.
17-18: Required HD wallet flag in constructorAdding the HD wallet flag as a required parameter is the right approach, ensuring it's always provided.
22-30: Backwards compatibility for HD wallet flagThe implementation correctly handles backward compatibility by defaulting
isHdWalletto false when not present in the JSON data. The explicit comment is helpful for future developers.
33-41: Updated primary key generationThe primary key now includes the HD wallet status, which is necessary to differentiate between HD and non-HD wallet caches.
60-63: Well-documented propertyThe documentation clearly explains the relationship between the wallet ID and HD status.
106-112: Updated primaryKey implementationThe primaryKey getter now correctly includes the HD wallet status to ensure unique key generation.
lib/bloc/app_bloc_root.dart (7)
22-24: Good addition of imports for mock data handlingThese imports provide the necessary classes for mock data generation and transaction history simulation.
81-102: Proper async method conversionUpdating the method signature to
Future<void>and adding theasynckeyword is the correct approach for a method that performs asynchronous operations.
120-126: Simplified repository initializationThe conditional logic for creating the transaction repository has been simplified, making it more readable and maintainable.
127-134: SDK-based repository initializationUsing the SDK directly for initialization simplifies the code and provides a more centralized approach to dependency management.
200-204: Cleaner bloc initializationPassing the repositories directly instead of creating them within the constructor makes the code cleaner and easier to understand.
207-211: Consistent ProfitLossBloc initializationUsing a consistent approach for bloc initialization improves maintainability.
213-217: Updated PortfolioGrowthBloc initializationUsing the
sdkparameter name makes the code more consistent with other components.lib/bloc/coins_bloc/coins_bloc.dart (8)
8-9: Good addition of standardized loggingAdding the logging package is consistent with the changes in other files and helps standardize the logging approach.
56-57: Well-named logger instanceCreating a namespaced logger with the class name follows best practices.
83-85: Improved documentation and early returnThe added comments explain the purpose of the early return clearly, improving code maintainability.
145-147: Consistent use of coin.id.idUsing
coin.id.idas the map key is consistent with changes in other files, providing a uniform approach to coin identification.
318-328: Proper exception handling during loginAdding a try-catch block with proper logging improves error handling for the login process.
376-378: Improved state emissionThe code now emits the activating coins state immediately instead of waiting for individual activation, providing better user feedback.
406-426: Refactored method to return stateChanging the method to return a state object instead of emitting it directly improves separation of concerns and makes the code more testable.
500-505: Improved list handling with cascade notationUsing cascade notation (
..addAll) for list operations makes the code more concise and readable.lib/bloc/cex_market_data/mockup/mock_transaction_history_repository.dart (1)
11-14: Confirm removal of unused dependencies.
It appears that the parametersapiandclienthave been removed from the constructor. Please ensure you have removed all references to them across the codebase to avoid dead code.lib/bloc/cex_market_data/profit_loss/models/adapters/profit_loss_cache_adapter.dart (3)
2-2: No issues with the import path change.
Switching to an absolute import path aligns well with improved clarity and consistency.
21-23: Good approach for backward compatibility.
Including a default value forisHdWalletensures that older records do not break when this new field is introduced.
30-30: Verify the field indexing.
The call to..writeByte(6)indicates that six fields will be written, indexed from 0 to 5. Confirm that these indices remain unique and consistent with other adapters to avoid serialization collisions.Also applies to: 40-42
lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_bloc.dart (8)
6-7: New imports look consistent.
Importingkomodo_defi_sdk.dartandlogging.dartis in line with the updated refactoring efforts to standardize logging and SDK usage.
10-10: Extension import.
Addingsdk_auth_activation_extension.dartis appropriate for enabling the newsdk.waitForEnabledCoinsToPassThreshold(...)usage.
22-22: Constructor now acceptssdk.
This change matches the refactoring toward a singleKomodoDefiSdkentry point instead of multiple repository parameters.
39-39:sdkfield introduced.
Removing the previous coin repository in favor ofKomodoDefiSdkprovides a more centralized approach for coin management.
40-40:_logfor centralized logging.
Having aLoggerinstance dedicated to this bloc improves maintainability and readability of logs.
113-118: No immediate error state after uncached load failure.
You have chosen to rely on periodic refresh attempts to recover. Ensure that end users are aware of any extended loading times if both cached and uncached attempts fail initially.
120-122: Swallowing top-level exception for cached load.
The code logs the error but does not emit an error state. Confirm that this silent failure is acceptable from a UX perspective.
197-207: New_removeInactiveCoinsmethod.
Filtering inactive coins directly viasdk.assets.getActivatedAssets()centralizes the logic for coin activation status. This helps keep the code consistent with the rest of the application’s coin lifecycle management.lib/bloc/cex_market_data/profit_loss/models/profit_loss_cache.dart (3)
16-16: **Add a constructor param to support HD wallets **Including
isHdWalletin the constructor nicely ensures that the cache can differentiate data for HD vs. non-HD wallets.
40-46: **Unique primary key leverages HD wallet distinction **Incorporating
isHdWalletinto the key is an effective way to avoid collisions when the samewalletIdis used for both HD and non-HD wallets.
48-54:✅ Verification successful
**Consider verifying usage of the new static
getPrimaryKeymethod **This method looks correct, but ensure no references still rely on the old variant.
🏁 Script executed:
#!/bin/bash # Searching codebase references to getPrimaryKey usage rg -A 5 "getPrimaryKey"Length of output: 3414
Action: Confirm usage of the new static
getPrimaryKeymethod.The verification confirms that the updated method in
lib/bloc/cex_market_data/profit_loss/models/profit_loss_cache.dartis used consistently. Notably:
- In
lib/bloc/cex_market_data/profit_loss/profit_loss_repository.dart, the call toProfitLossCache.getPrimaryKeycorrectly passescoinId,fiatCurrency,walletId, andisHdWallet.- In
lib/bloc/cex_market_data/profit_loss/models/profit_loss_cache.dart, theprimaryKeygetter wraps the new method appropriately.No remnants of the old variant were detected. You can safely proceed with these changes.
lib/bloc/cex_market_data/profit_loss/profit_loss_bloc.dart (5)
7-8: **New imports for SDK and logging **Imports of
komodo_defi_sdkandloggingalign with the established pattern in the updated codebase.Also applies to: 11-11
20-21: **Constructor update and new logger **Passing
_sdkto the bloc constructor and usingLoggerensures consistent dependency management and structured logging.Also applies to: 34-34, 36-36
101-103: **Periodic stream usage **Using
Stream.periodicis a neat way to refresh data. However, confirm you properly dispose the bloc or cancel the subscription on UI changes to prevent memory leaks or unexpected updates (e.g., when the user navigates away).Do you want a sample approach showing how to handle stream cancellation in the bloc’s
close()method or in the UI layer?
181-181: **Emit consistent states on period change **Emitting
PortfolioProfitLossChartLoadInProgressbefore dispatching a new load event is a clean approach to reflect the updated UI state.Also applies to: 189-193
245-255: **Filtering out inactive coins **Your
_removeInactiveCoinsmethod efficiently excludes non-activated assets, preventing unnecessary requests. Good job.lib/bloc/cex_market_data/profit_loss/profit_loss_repository.dart (10)
27-28: **SDK-based constructor dependency **Injecting
KomodoDefiSdkclearly centralizes your dependencies and aligns with your new approach throughout the codebase.Also applies to: 32-32
40-41: **Factory method updated to unify SDK usage **Making the SDK mandatory in
withDefaultsis consistent with the rest of the refactor.Also applies to: 58-59
62-67: **Private fields and logger **Using
finalfields with_sdkand_logensures safe references and a well-scoped logger.Also applies to: 68-68
70-75: **Hive adapters initialization **Registering these adapters here centralizes the setup needed for your profit/loss caching logic.
77-84: **Cache clearing **
clearCache()with timing logs is straightforward and helps track performance.
104-142: **Performance logging for coin support checks **Using stopwatches to time operations is a valuable approach to diagnosing performance issues.
162-205: **Expanded logging ingetProfitLoss**Capturing timing at each step (cache lookups, user fetch, etc.) provides valuable analytics and debugging insights.
210-224: **Coin support check fallback **Early exit when the coin is unsupported keeps logic crisp.
226-257: **Caching empty and transaction-based results **Caching empty sets prevents repetitive calls; good for performance.
259-293: **Final caching of computed profit/loss and measured timings **This thorough approach handles performance measurement and data storage.
lib/bloc/assets_overview/bloc/asset_overview_bloc.dart (10)
5-6: New SDK and Logging Imports
The imports forkomodo_defi_sdkandloggingare correctly added and will support the newly introduced functionality.
11-11: New Extension Import
Addingsdk_auth_activation_extensionaligns with its usage in_onLoadPortfolio(line 97).
18-22: Updated Constructor with Private Repositories and SDK
Switching to private parameters (_profitLossRepository,_investmentRepository,_sdk) enhances encapsulation. This also simplifies injecting these dependencies.
32-35: Private Fields Introduction
Defining_profitLossRepository,_investmentRepository,_sdk, and_logas private fields is a succinct way to keep the implementation encapsulated within the bloc.
45-45: Refined Usage of_profitLossRepository
Integrating_profitLossRepositoryhere is straightforward and maintains consistency with the constructor changes.
52-52: Refined Usage of_investmentRepository
Using the private repository for total investment calculation is consistent with the new design.
97-97: Waiting for Enabled Coins
await _sdk.waitForEnabledCoinsToPassThreshold(event.coins);effectively ensures all coins are recognized before proceeding.
105-105: Consistent Private Field Usage
Replacing the direct repository call with_profitLossRepositorymatches the constructor refactor and looks good.
134-134: Switch tocoin.id.id
Usingcoin.id.idinstead ofcoin.abbrensures proper referencing of the coin’s unique identifier.
143-143: Duplicate Logging Concern
This_log.shoutusage is identical to the prior suggestions regarding log level choices.lib/bloc/cex_market_data/portfolio_growth/portfolio_growth_repository.dart (8)
1-1: New Imports for Charting, SDK, Logging, and Cache-Miss Handling
These additions correctly support extended functionality such as point-based chart data, the KomodoDeFi SDK, structured logging, and the newCacheMissException.Also applies to: 6-6, 9-9, 15-15
28-33: Extended Constructor with_sdk
RequiringKomodoDefiSdk sdkand storing it in_sdkprovides a clear centralization of SDK operations within this repository.
42-42: Factory Method Updates
AcceptingKomodoDefiSdk sdkinwithDefaultsand passing it through ensures consistency and reusability across default and mock repositories.Also applies to: 49-49, 60-60
74-76: Private_sdkand Logger
Defining_sdkfor authentication checks and_logfor consistent repository-level logging aligns with the new logging approach in the codebase.
87-87: Documentation withCacheMissException
Explicitly describing the thrownCacheMissExceptionclarifies behavior and sets correct expectations for consumers of these methods.Also applies to: 100-100
112-203: RefinedgetCoinGrowthChartMethod
- Introduces multiple
Stopwatchinstances for granular performance tracking.- Validates user authentication via
_sdk.auth.currentUser.- Provides detailed
fine-level logging for cache hits/misses, transaction fetches, and empty results.- Throws
CacheMissExceptionon cache misses for clearer handling downstream.Overall, these changes strengthen observability and user-state validation.
313-453: EnhancedgetPortfolioGrowthChartMethod
- Establishes multiple
Stopwatchvariables to measure the parallel fetch phase, merging phase, and final processing.- Tracks successful vs. error fetches (
successCountanderrorCount).- Logs warnings for empty coin lists and no valid chart data.
- Appends the current USD balance point to the merged chart to ensure relevance.
- Applies domain filtering, then caches or returns the final merged chart.
This significantly boosts reliability and debug accuracy.
460-486: Enhanced_mergeTransactionsWithOhlc
- Logs performance metrics with a
Stopwatch, capturing how long merging takes.- Warns if transaction data or OHLC data is empty.
- Consolidates data into a final chart output.
This provides further insight into performance bottlenecks and strengthens error handling.
also implement some of the coderabbitai recommendations
convert to stateful widget to store tab bar view state
takenagain
requested review from
AndrewDelaney,
smk762,
CharlVS and
ShantanuSharma9873
There was a problem hiding this comment.
- The balance in the HD Wallet is higher compared to the Non-HD Wallet.
HD Wallet Balance:
Non-HD Wallet:
-
The 'Failed to load portfolio profit/loss' message is displayed on the Profit & Loss graph, for both HD & Non-HD Wallet.
-
The coin address is visible in the dropdown menu, but when clicking the RECEIVE button, the address is not available, displaying 'Address Not Found' for all coins.
Video:
https://github.com/user-attachments/assets/92317ec2-1833-4b45-bfc2-9a9270e84c64
In 2 out of 6 instances, the balance takes a significant amount of time to update at the top right corner.
@ShantanuSharma9873 that's expected since the HD wallet has a separate set of addresses.
Thanks, I will take care of these 2 issues and resolve them in a separate PR @takenagain, please have a look at the reported profit & loss error. |
|
My wallet balance is not reflecting on the graph: I have HD wallet turned off. |
Fixes the following issues
Refactoring:
utils.dartwithpackage:loggingin market-metricsTesting
NOTE: wallet requires balances in both HD and non-HD modes
Expected HD balance and graphs example:
Expected non-HD (pre-HD update) balance and graphs example:
Summary by CodeRabbit
New Features
Improvements