feat(pubkey-manager): add pubkey watch function similar to balance watch#178
feat(pubkey-manager): add pubkey watch function similar to balance watch#178
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit 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 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 introduces a major refactor of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PubkeyManager
participant ApiClient
participant AuthState
Client->>PubkeyManager: watchPubkeys(asset)
alt Cached pubkeys available
PubkeyManager-->>Client: Emit cached pubkeys
end
loop Every 30 seconds
PubkeyManager->>ApiClient: Fetch latest pubkeys for asset
ApiClient-->>PubkeyManager: Return pubkeys
PubkeyManager-->>Client: Emit updated pubkeys
end
AuthState-->>PubkeyManager: On wallet change
PubkeyManager-->>Client: Reset cache, cancel watchers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit f678d1b): https://komodo-defi-sdk--pr178-feat-pubkey-watcher-cwknam3g.web.app (expires Tue, 19 Aug 2025 12:16:42 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 7f9f5ac39928f333b6e8fcefb7138575e24ed347 |
|
@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: 1
♻️ Duplicate comments (2)
packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart (2)
70-79: Same activation-result check needed forcreateNewPubkey.Discarding the
ActivationResultrisks attempting address creation on an inactive asset – likely to fail. Apply the guard here too.
83-93: Ditto forcreateNewPubkeyStream.Stream should early-error if activation doesn’t succeed; otherwise consumers see an empty stream.
🧹 Nitpick comments (1)
packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart (1)
197-214: Silent catch hides useful diagnostics.Swallowing all exceptions without at least a
debugPrintmakes production debugging painful.- } catch (_) { - // Swallow transient errors; continue with last known state + } catch (e, st) { + debugPrint('watchPubkeys polling error for ${asset.id.name}: $e\n$st');Even a low-level log is preferable to complete silence.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/komodo_defi_sdk/lib/src/balances/balance_manager.dart(1 hunks)packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart(4 hunks)packages/komodo_defi_sdk/pubspec.yaml(1 hunks)packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart(1 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). (2)
- GitHub Check: build_and_preview_sdk_example_preview
- GitHub Check: build_and_preview_playground_preview
🔇 Additional comments (3)
packages/komodo_defi_sdk/lib/src/balances/balance_manager.dart (1)
2-2: No functional change – looks good.
The added blank line keeps import grouping tidy without altering behaviour.packages/komodo_defi_sdk/pubspec.yaml (1)
48-49: Dev-dependency addition approved, but double-check transitive impact.
fake_asyncis appropriate for the new tests; just ensure no version-range conflicts with the SDK (³.7) or with other packages at resolution time.packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart (1)
214-248: Nice use offake_async– test clearly asserts the 30 s poll.
Coverage is thorough and readable.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a reactive watchPubkeys function to the PubkeyManager class, providing streaming updates for public keys similar to the existing balance watching functionality. This addresses the need for real-time pubkey monitoring in wallet applications.
- Introduces streaming pubkey watching with periodic updates and caching functionality
- Adds proper lifecycle management for authentication state changes and resource cleanup
- Updates the example app to demonstrate the new streaming pubkey functionality
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart |
Core implementation of streaming pubkey watching with caching, auth handling, and dispose pattern |
packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart |
Comprehensive test suite covering streaming behavior, auth changes, error handling, and edge cases |
packages/komodo_defi_sdk/pubspec.yaml |
Adds fake_async dependency for testing time-dependent behavior |
packages/komodo_defi_sdk/example/lib/screens/asset_page.dart |
Updates example to use streaming pubkey watching instead of one-time fetch |
packages/komodo_defi_sdk/lib/src/balances/balance_manager.dart |
Minor formatting change adding blank line |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart (1)
246-247: Await subscription cancellation in tests
Usingunawaited(sub.cancel())may let the stream keep running after the test finishes, causing flakiness.
Simplyawait sub.cancel();as previously suggested.
🧹 Nitpick comments (3)
packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart (2)
61-62: Make polling interval injectableHard-coding the 30 s value makes unit-tests depend on fake-time tricks and prevents product owners from tuning network load.
Pass aDuration pollingInterval(defaulting to 30 s) to the constructor or towatchPubkeys.
298-307: Skip restart when nobody is listeningInside
_resetState()you restart watchers for every non-closed controller, even if it currently has zero listeners (!controller.hasListener).
CheckhasListenerto avoid spinning idle polling tasks.packages/komodo_defi_sdk/example/lib/screens/asset_page.dart (1)
82-86: Avoid mutating the cached list in place
_pubkeys?.keys.add(newPubkey);mutates the existing list that may be shared with other widgets or caches, breaking value equality and change-detection.Create a new instance instead:
- _pubkeys?.keys.add(newPubkey); + if (_pubkeys != null) { + _pubkeys = _pubkeys!.copyWith( + keys: [..._pubkeys!.keys, newPubkey], + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/komodo_defi_sdk/example/lib/screens/asset_page.dart(4 hunks)packages/komodo_defi_sdk/lib/src/pubkeys/pubkey_manager.dart(4 hunks)packages/komodo_defi_sdk/test/pubkeys/pubkey_manager_test.dart(1 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). (2)
- GitHub Check: build_and_preview_sdk_example_preview
- GitHub Check: build_and_preview_playground_preview
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
CharlVS
left a comment
CharlVS
left a comment
There was a problem hiding this comment.
Thank you for your contributions.
| /// Pre-caches pubkeys for an asset to warm the cache and notify listeners | ||
| Future<void> preCachePubkeys(Asset asset); |
There was a problem hiding this comment.
Nit: "precache" is one word, so the name should be precachePubkeys
| if (_isDisposed) return; | ||
| _isDisposed = true; | ||
|
|
||
| await _authSubscription?.cancel(); | ||
| _authSubscription = null; | ||
|
|
||
| for (final subscription in _activeWatchers.values) { | ||
| await subscription.cancel(); | ||
| } | ||
| _activeWatchers.clear(); | ||
|
|
||
| for (final controller in _pubkeysControllers.values) { | ||
| await controller.close(); | ||
| } | ||
| _pubkeysControllers.clear(); | ||
|
|
||
| _pubkeysCache.clear(); | ||
| _watchedAssets.clear(); | ||
| _currentWalletId = null; |
There was a problem hiding this comment.
Improve the error handling to make it more resilient to errors being thrown early in the function. If it won't cause potential race conditions, consider putting all async operations into a Future.await([...]) since it lets all futures run and also runs them concurrently.
| /// Streamed version of [createNewPubkey] | ||
| Stream<NewAddressState> createNewPubkeyStream(Asset asset); |
There was a problem hiding this comment.
Nit: createNewPubkeyStream may be unclear since (without reading comments for additional context) it suggests its function is to create a stream for new pubkeys.
In line with the convention to add watch- for creating streamed versions of methods, it can be named watchCreateNewPubkey
There was a problem hiding this comment.
Is there a specific reason you opted to make the methods require the Asset type instead of assetId? Some of the behaviour may depend on the Asset's properties, but since this is a publicly exposed SDK method, it places a significant responsibility on projects consuming the SDK, as they must retain a reference to the entire Asset across all states to use these methods.
Also, it may cause unexpected behaviour if we're using the Asset values passed since the consuming app could instantiate their own Asset object to alter the method's behaviour.
There was a problem hiding this comment.
To align with the behaviour of existing PubkeyManager methods in terms of performing automatic activation of assets. The Asset type is required for activation using SharedActivationCoordinator.
Introduces a Pubkey watch function similar to the one in
BalanceManagerto address the comment in #3049.Adds
fake_asyncas a direct dev dependency for unit testing.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
fake_asyncpackage as a development dependency.