Investigate transaction storage stack overflow error#268
Merged
CharlVS merged 1 commit intoperf/3238-reduce-rpc-spamfrom Oct 29, 2025
Conversation
This change refactors the transaction comparison logic in `InMemoryTransactionStorage` to ensure that all transactions are available during comparison. This prevents potential exceptions and improves the stability of the transaction history storage. Co-authored-by: charl <charl@vanstaden.info>
|
Cursor Agent can help with this pull request. Just |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
CharlVS
deleted the
cursor/investigate-transaction-storage-stack-overflow-error-a137
branch
Contributor
|
Visit the preview URL for this PR (updated for commit 64e0fd6): https://kdf-sdk--pr268-cursor-investigate-t-xtv9k4u3.web.app (expires Wed, 05 Nov 2025 02:45:30 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
ca333
pushed a commit
that referenced
this pull request
Oct 29, 2025
… support (#262) * feat(pubkeys): persist AssetPubkeys across sessions using Hive TypeAdapters; hydrate on cold start\n\n- Add Hive adapters for stored pubkeys\n- Persist on fetch, hydrate before first RPC\n- Align balance polling to 60s and integrate with tx watcher\n\nBREAKING CHANGE: none * chore(format): run dart format on pubkey persistence and balance manager files * perf(sdk): dedupe pubkeys/address fetch, cache-first hydrate; throttle health checks; cache wallet names (#3238) * test(local-auth): add ensureKdfHealthy to FakeAuthService for Trezor tests * Refactor: Wallet-aware pubkey persistence and retrieval This change ensures that pubkey data is correctly associated with the active wallet, preventing cross-wallet contamination. It also improves the accuracy of KDF health checks by bypassing cached user data. Co-authored-by: charl <charl@vanstaden.info> * Refactor: Improve pubkey and balance fetching logic Co-authored-by: charl <charl@vanstaden.info> * fix: market data resource improvements * perf(assets): cache activated assets and coalesce activation checks - Wire SDK `ActivatedAssetsCache` into activation/coins flows: updates across `CoinsBloc`, `AssetOverviewBloc`, custom token import, and `sdk_auth_activation_extension` to reuse activation state instead of re-querying. - Debounce/polish polling in `portfolio_growth_bloc` and `profit_loss_bloc` to prevent overlapping requests. - Remove duplicate activation/balance checks in maker/taker validators and forms. - Consolidate repeated calls in `mm2_api`/`mm2_api_nft`/`rpc_native`; prefer cached values. - Reduce startup RPCs in `app_bootstrapper`; stop background timers in `window_close_handler` on app close to avoid trailing calls. - Add shared intervals in `shared/constants`; introduce `lib/shared/utils/activated_assets_cache.dart` for app-specific helpers. - No UI changes; measurable reduction in RPC volume and improved responsiveness. Refs #3238 * feat(streaming): add typed stream RPCs and web SharedWorker integration; expose streaming API in framework and rpc methods * feat(web): package event_streaming_worker.js in framework assets and load via package path * fix(web): correct SharedWorker path to package asset under assets/web/ * refactor(streaming): improve type safety with sealed event classes - Replace string-based event types with sealed class hierarchy - Create typed event classes for all stream types (Balance, Orderbook, Network, Heartbeat, SwapStatus, OrderStatus, TxHistory, ShutdownSignal) - Use private enum for internal string mapping while exposing typed API - Make StreamEnableResponse generic to link responses to event types - Update event_streaming_service with type-safe filtering methods - Organize events into separate files using part directives - Enable exhaustive pattern matching with switch expressions Benefits: - Compile-time type safety eliminates string-based checks - Better IDE support with autocomplete and type hints - Reduced runtime errors from type mismatches - Clearer public API with explicit event types * feat(sdk): add internal event streaming manager with lifecycle management - Create EventStreamingManager in komodo_defi_sdk package - Implement automatic stream lifecycle handling (enable/disable) - Add reference counting for shared stream subscriptions - Support all event types: balance, orderbook, tx history, swap status, order status, network, heartbeat, and shutdown signals - Reduce boilerplate with generic _subscribeToStream method using template method pattern - Register manager in DI container for internal use by domain managers - Manager is not publicly exposed, intended for use by domain-specific managers to provide real-time updates * perf: eliminate RPC polling by using event streaming Replace periodic polling with real-time event streaming in BalanceManager and TransactionHistoryManager to reduce RPC spam and improve efficiency. Changes: - BalanceManager: Replace 1-minute polling interval with balance event streaming - TransactionHistoryManager: Replace balance-driven polling with TX history event streaming - Bootstrap: Inject EventStreamingManager into both managers - Remove polling configuration (intervals, retry counters, debug flags) - Fix shouldEnableTransactionHistory to always return true for streaming support Benefits: - Eliminates periodic RPC calls every 60 seconds - Real-time updates instead of up to 1-minute delays - Better resource utilization (updates only when data changes) - Automatic reconnection and error handling via EventStreamingManager Refs: #3238 * fix(cache): address PR review issues - error handling and race conditions - Health check: log transient RPC failures instead of triggering false sign-outs - ActivatedAssetsCache: fix race condition using generation counter and Completer pattern - NFT activation: aggregate and report all errors instead of only the last one - Auth service: document 5-minute user cache staleness trade-off Refs: #262 * chore: add event streaming logging * fix(rpc): address critical review feedback on caching implementation - Fix incorrect unawaited() usage in pubkey_manager by properly extracting Future - Add eagerError: false to event_streaming_manager dispose for robust cleanup - Replace unsafe String cast with whereType<String>() in pubkeys_storage - Add race condition check in transaction_history_manager _startStreaming - Capture timestamp at fetch start in activated_assets_cache for accurate TTL - Add error handling to sparkline_repository dispose to ensure all cleanup * perf(auth): use shutdown event streaming to minimize RPC polling Subscribe to KDF shutdown signals to immediately detect when KDF shuts down, eliminating the need for frequent polling. This provides near-instant shutdown detection (< 1 second) compared to periodic health checks. - Add shutdown signal subscription in KdfAuthService - Subscribe to shutdown events and immediately update auth state - Enable shutdown signal stream via RPC on initialization - Clean up subscription on dispose - Health checks now serve as backup for edge cases Benefits: - Reduces getWalletNames RPC calls significantly - Provides instant user sign-out on KDF shutdown - Maintains graceful degradation if streaming unavailable * feat(rpc): optimize initial balance/history for newly created wallets - Assume zero balance for first-time asset enablement in new wallets - Assume empty transaction history for first-time asset enablement in new wallets - Detect new wallets by absence of any asset activation history - Avoids unnecessary RPC spam when activating first assets in new wallets - Does NOT apply to imported wallets (they have activation history) - Uses AssetHistoryStorage to track which assets have been enabled per wallet - Wire up shared AssetHistoryStorage instance in SDK bootstrap * fix(auth): track imported vs created wallets to prevent incorrect optimizations - Add 'isImported' metadata to KdfUser during registration - Pass mnemonic presence to _registerNewUser to determine if imported - Update balance/history managers to check isImported flag - Prevents incorrectly assuming zero balance for imported wallets - Optimization now only applies to genuinely new wallets (not imported) BREAKING: Imported wallets will now correctly fetch real balances/history on first use instead of incorrectly showing zero * fix: remove errors from KDF merge * chore: roll `coins` * fix: misc streaming fixes * feat(sdk): add event streaming support for task status updates - Add event streaming service and configuration - Implement task event handling and unknown event fallback - Add RPC task shepherd method for task status monitoring - Update balance manager to support event-driven updates - Add platform-specific event streaming implementations - Enhance sync status with event streaming capabilities This reduces RPC polling by leveraging KDF event streaming for task status updates. * fix(activation): force cache refresh when verifying asset availability The _waitForCoinAvailability method was failing to verify asset activation because isAssetActive() was using cached data instead of fetching fresh data from the backend. This caused transaction history to fail with a connection error even though assets were successfully activated. Changes: - Add forceRefresh parameter to ActivationManager.isAssetActive() - Update SharedActivationCoordinator._waitForCoinAvailability() to force refresh on each availability check - This ensures we bypass the 2-second cache TTL and get real-time data Fixes issue where transaction history shows 'Connection to Komodo servers are failing' error after asset activation completes successfully. * fix(streaming): use asset config ID instead of display name for event subscriptions The balance and transaction history event subscriptions were using asset.id.name (the human-friendly display name like 'Bitcoin') instead of asset.id.id (the config ID like 'BTC-segwit'). This caused the RPC enable streaming calls to fail because the backend expects the config ID. Changes: - BalanceManager: Use assetId.id instead of assetId.name for subscribeToBalance - TransactionHistoryManager: Use asset.id.id instead of asset.id.name for subscribeToTxHistory - Update event filtering to match using config ID as well This fixes the 'Failed to start balance watcher' errors and resolves the transaction history connection error. * perf: reduce RPC spam in activation strategies and managers * fix(auth-service): update cache alongside storage for metadata updates * chore: roll KDF to release preview Roll KDF to the `dev` branch version which will be used for KW release. * fix(stream): normalize KDF stream _type parsing for suffixed IDs; format modified files * fix: use correct streaming worker js path * fix: tx history streaming * fix: improve robustness of event parsing * fix: backwards compatibility for KDF API status * Refactor: Improve transaction comparison logic (#268) This change refactors the transaction comparison logic in `InMemoryTransactionStorage` to ensure that all transactions are available during comparison. This prevents potential exceptions and improves the stability of the transaction history storage. Co-authored-by: Cursor Agent <cursoragent@cursor.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Francois <takenagain@users.noreply.github.com>
ca333
added a commit
that referenced
this pull request
Oct 30, 2025
#269) * fix(rpc): minimise RPC usage with comprehensive caching and streaming support (#262) * feat(pubkeys): persist AssetPubkeys across sessions using Hive TypeAdapters; hydrate on cold start\n\n- Add Hive adapters for stored pubkeys\n- Persist on fetch, hydrate before first RPC\n- Align balance polling to 60s and integrate with tx watcher\n\nBREAKING CHANGE: none * chore(format): run dart format on pubkey persistence and balance manager files * perf(sdk): dedupe pubkeys/address fetch, cache-first hydrate; throttle health checks; cache wallet names (#3238) * test(local-auth): add ensureKdfHealthy to FakeAuthService for Trezor tests * Refactor: Wallet-aware pubkey persistence and retrieval This change ensures that pubkey data is correctly associated with the active wallet, preventing cross-wallet contamination. It also improves the accuracy of KDF health checks by bypassing cached user data. Co-authored-by: charl <charl@vanstaden.info> * Refactor: Improve pubkey and balance fetching logic Co-authored-by: charl <charl@vanstaden.info> * fix: market data resource improvements * perf(assets): cache activated assets and coalesce activation checks - Wire SDK `ActivatedAssetsCache` into activation/coins flows: updates across `CoinsBloc`, `AssetOverviewBloc`, custom token import, and `sdk_auth_activation_extension` to reuse activation state instead of re-querying. - Debounce/polish polling in `portfolio_growth_bloc` and `profit_loss_bloc` to prevent overlapping requests. - Remove duplicate activation/balance checks in maker/taker validators and forms. - Consolidate repeated calls in `mm2_api`/`mm2_api_nft`/`rpc_native`; prefer cached values. - Reduce startup RPCs in `app_bootstrapper`; stop background timers in `window_close_handler` on app close to avoid trailing calls. - Add shared intervals in `shared/constants`; introduce `lib/shared/utils/activated_assets_cache.dart` for app-specific helpers. - No UI changes; measurable reduction in RPC volume and improved responsiveness. Refs #3238 * feat(streaming): add typed stream RPCs and web SharedWorker integration; expose streaming API in framework and rpc methods * feat(web): package event_streaming_worker.js in framework assets and load via package path * fix(web): correct SharedWorker path to package asset under assets/web/ * refactor(streaming): improve type safety with sealed event classes - Replace string-based event types with sealed class hierarchy - Create typed event classes for all stream types (Balance, Orderbook, Network, Heartbeat, SwapStatus, OrderStatus, TxHistory, ShutdownSignal) - Use private enum for internal string mapping while exposing typed API - Make StreamEnableResponse generic to link responses to event types - Update event_streaming_service with type-safe filtering methods - Organize events into separate files using part directives - Enable exhaustive pattern matching with switch expressions Benefits: - Compile-time type safety eliminates string-based checks - Better IDE support with autocomplete and type hints - Reduced runtime errors from type mismatches - Clearer public API with explicit event types * feat(sdk): add internal event streaming manager with lifecycle management - Create EventStreamingManager in komodo_defi_sdk package - Implement automatic stream lifecycle handling (enable/disable) - Add reference counting for shared stream subscriptions - Support all event types: balance, orderbook, tx history, swap status, order status, network, heartbeat, and shutdown signals - Reduce boilerplate with generic _subscribeToStream method using template method pattern - Register manager in DI container for internal use by domain managers - Manager is not publicly exposed, intended for use by domain-specific managers to provide real-time updates * perf: eliminate RPC polling by using event streaming Replace periodic polling with real-time event streaming in BalanceManager and TransactionHistoryManager to reduce RPC spam and improve efficiency. Changes: - BalanceManager: Replace 1-minute polling interval with balance event streaming - TransactionHistoryManager: Replace balance-driven polling with TX history event streaming - Bootstrap: Inject EventStreamingManager into both managers - Remove polling configuration (intervals, retry counters, debug flags) - Fix shouldEnableTransactionHistory to always return true for streaming support Benefits: - Eliminates periodic RPC calls every 60 seconds - Real-time updates instead of up to 1-minute delays - Better resource utilization (updates only when data changes) - Automatic reconnection and error handling via EventStreamingManager Refs: #3238 * fix(cache): address PR review issues - error handling and race conditions - Health check: log transient RPC failures instead of triggering false sign-outs - ActivatedAssetsCache: fix race condition using generation counter and Completer pattern - NFT activation: aggregate and report all errors instead of only the last one - Auth service: document 5-minute user cache staleness trade-off Refs: #262 * chore: add event streaming logging * fix(rpc): address critical review feedback on caching implementation - Fix incorrect unawaited() usage in pubkey_manager by properly extracting Future - Add eagerError: false to event_streaming_manager dispose for robust cleanup - Replace unsafe String cast with whereType<String>() in pubkeys_storage - Add race condition check in transaction_history_manager _startStreaming - Capture timestamp at fetch start in activated_assets_cache for accurate TTL - Add error handling to sparkline_repository dispose to ensure all cleanup * perf(auth): use shutdown event streaming to minimize RPC polling Subscribe to KDF shutdown signals to immediately detect when KDF shuts down, eliminating the need for frequent polling. This provides near-instant shutdown detection (< 1 second) compared to periodic health checks. - Add shutdown signal subscription in KdfAuthService - Subscribe to shutdown events and immediately update auth state - Enable shutdown signal stream via RPC on initialization - Clean up subscription on dispose - Health checks now serve as backup for edge cases Benefits: - Reduces getWalletNames RPC calls significantly - Provides instant user sign-out on KDF shutdown - Maintains graceful degradation if streaming unavailable * feat(rpc): optimize initial balance/history for newly created wallets - Assume zero balance for first-time asset enablement in new wallets - Assume empty transaction history for first-time asset enablement in new wallets - Detect new wallets by absence of any asset activation history - Avoids unnecessary RPC spam when activating first assets in new wallets - Does NOT apply to imported wallets (they have activation history) - Uses AssetHistoryStorage to track which assets have been enabled per wallet - Wire up shared AssetHistoryStorage instance in SDK bootstrap * fix(auth): track imported vs created wallets to prevent incorrect optimizations - Add 'isImported' metadata to KdfUser during registration - Pass mnemonic presence to _registerNewUser to determine if imported - Update balance/history managers to check isImported flag - Prevents incorrectly assuming zero balance for imported wallets - Optimization now only applies to genuinely new wallets (not imported) BREAKING: Imported wallets will now correctly fetch real balances/history on first use instead of incorrectly showing zero * fix: remove errors from KDF merge * chore: roll `coins` * fix: misc streaming fixes * feat(sdk): add event streaming support for task status updates - Add event streaming service and configuration - Implement task event handling and unknown event fallback - Add RPC task shepherd method for task status monitoring - Update balance manager to support event-driven updates - Add platform-specific event streaming implementations - Enhance sync status with event streaming capabilities This reduces RPC polling by leveraging KDF event streaming for task status updates. * fix(activation): force cache refresh when verifying asset availability The _waitForCoinAvailability method was failing to verify asset activation because isAssetActive() was using cached data instead of fetching fresh data from the backend. This caused transaction history to fail with a connection error even though assets were successfully activated. Changes: - Add forceRefresh parameter to ActivationManager.isAssetActive() - Update SharedActivationCoordinator._waitForCoinAvailability() to force refresh on each availability check - This ensures we bypass the 2-second cache TTL and get real-time data Fixes issue where transaction history shows 'Connection to Komodo servers are failing' error after asset activation completes successfully. * fix(streaming): use asset config ID instead of display name for event subscriptions The balance and transaction history event subscriptions were using asset.id.name (the human-friendly display name like 'Bitcoin') instead of asset.id.id (the config ID like 'BTC-segwit'). This caused the RPC enable streaming calls to fail because the backend expects the config ID. Changes: - BalanceManager: Use assetId.id instead of assetId.name for subscribeToBalance - TransactionHistoryManager: Use asset.id.id instead of asset.id.name for subscribeToTxHistory - Update event filtering to match using config ID as well This fixes the 'Failed to start balance watcher' errors and resolves the transaction history connection error. * perf: reduce RPC spam in activation strategies and managers * fix(auth-service): update cache alongside storage for metadata updates * chore: roll KDF to release preview Roll KDF to the `dev` branch version which will be used for KW release. * fix(stream): normalize KDF stream _type parsing for suffixed IDs; format modified files * fix: use correct streaming worker js path * fix: tx history streaming * fix: improve robustness of event parsing * fix: backwards compatibility for KDF API status * Refactor: Improve transaction comparison logic (#268) This change refactors the transaction comparison logic in `InMemoryTransactionStorage` to ensure that all transactions are available during comparison. This prevents potential exceptions and improves the stability of the transaction history storage. Co-authored-by: Cursor Agent <cursoragent@cursor.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Francois <takenagain@users.noreply.github.com> * fix config * fix timers leak * fix sse --------- Co-authored-by: Nitride <77973576+CharlVS@users.noreply.github.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Francois <takenagain@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor transaction storage to prevent Stack Overflow by merging transactions before SplayTreeMap creation.
The
_compareTransactionsfunction, used bySplayTreeMap, recursively looked up existing transactions from the old storage map while new transactions were being added. This led to a deep call stack and a Stack Overflow error. Merging all transactions into a single map upfront ensures the comparator has direct access to all necessary data, eliminating the recursive lookups.Note
Refactors
InMemoryTransactionStorageto buildSplayTreeMapfrom a merged transaction map and comparator that only reads from that map, preventing recursive lookups and stack overflow._compareTransactionsnow reads only from a provided transactions map and orders bytimestampdesc theninternalId.storeTransaction,storeTransactions): merge existing + new transactions into a single map (allTxMap) before creatingSplayTreeMap, then bulkaddAllfor stable comparisons._initializeStorage: rebuilds each asset’sSplayTreeMapfrom a copied regular map to align with the new comparator contract.Written by Cursor Bugbot for commit 64e0fd6. This will update automatically on new commits. Configure here.