Conversation
WalkthroughThis PR implements a one-shot sync parameter system for ZHTLC activation. Users can now specify sync parameters through a configuration dialog that are stored temporarily (not persisted) and automatically applied during activation. Parameters are cleared upon wallet sign-out, with the system integrated into authentication state management. Changes
Sequence DiagramsequenceDiagram
participant User
participant Dialog as ZHTLC Dialog
participant LoggedInView as Logged-in View
participant ConfigSvc as Config Service
participant ActivationSvc as Activation Strategy
participant Auth as Auth State
User->>Dialog: Opens ZHTLC config
Dialog->>Dialog: Collects config & syncParams
Dialog->>LoggedInView: Returns ZhtlcConfigDialogResult
LoggedInView->>ConfigSvc: setOneShotSyncParams(asset, syncParams)
ConfigSvc->>ConfigSvc: Store in memory (non-persistent)
Note over ConfigSvc,Auth: Later, during activation...
ActivationSvc->>ConfigSvc: takeOneShotSyncParams(asset)
ConfigSvc->>ConfigSvc: Retrieve & clear from storage
ConfigSvc->>ActivationSvc: Return one-shot params
alt One-shot params present
ActivationSvc->>ActivationSvc: Apply to rpcData.syncParams
end
ActivationSvc->>ActivationSvc: Proceed with activation
Auth->>ConfigSvc: Sign-out detected
ConfigSvc->>ConfigSvc: clearOneShotSyncParamsForWallet()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the handling of ZHTLC sync parameters by introducing a one-shot mechanism that prevents unintended sync parameter persistence. The sync parameters are now applied only once per activation and are automatically cleared after use or when the user signs out.
Key changes:
- Sync parameters are no longer persisted in
ZhtlcUserConfig; they are stored temporarily inActivationConfigServiceas one-shot values - Authentication state changes trigger automatic cleanup of one-shot sync parameters
- UI dialog returns both persistent config and transient sync parameters separately
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
komodo_defi_sdk.dart |
Adds disposal of ActivationConfigService subscription in cleanup |
bootstrap.dart |
Passes auth state changes stream to ActivationConfigService constructor |
activation_config_service.dart |
Implements one-shot sync params coordinator with auth-based cleanup |
zhtlc_activation_strategy.dart |
Uses one-shot sync params instead of persisted config values |
zhtlc_config_dialog.dart |
Returns separate config and one-shot sync params in result object |
logged_in_view_widget.dart |
Calls setOneShotSyncParams before saving config when dialog provides sync params |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _disposeIfRegistered<AssetManager>((m) => m.dispose()), | ||
| _disposeIfRegistered<ActivationManager>((m) => m.dispose()), | ||
| _disposeIfRegistered<ActivationConfigService>( | ||
| (m) async => m.dispose(), |
There was a problem hiding this comment.
The dispose callback uses async => but the other dispose calls use synchronous callbacks. Since dispose() is a synchronous method (returns void not Future<void>), this should use a synchronous callback (m) => m.dispose() to maintain consistency with the other dispose calls.
| (m) async => m.dispose(), | |
| (m) => m.dispose(), |
| ); | ||
| result = ZhtlcConfigDialogResult( | ||
| config: config, | ||
| oneShotSync: syncParams, | ||
| ); |
There was a problem hiding this comment.
[nitpick] The syncParams variable may be null if syncType is 'earliest', but ZhtlcConfigDialogResult accepts nullable oneShotSync. Consider adding a comment explaining that syncParams will be null when 'earliest' is selected, as this is intentional behavior for that sync type.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/komodo_defi_sdk/example/lib/widgets/instance_manager/zhtlc_config_dialog.dart (1)
216-220: Address timezone ambiguity in date-to-timestamp conversion and UX mismatch.The review comment accurately identifies two real issues:
UX mismatch: The SnackBar says "Please select a date and time," but the code has no time picker—only a date picker. The dialog label says "Date & Time" but offers only date selection.
Timezone bug confirmed: The code uses
selectedDateTime!.millisecondsSinceEpoch ~/ 1000without calling.toUtc(), meaning the timestamp is derived from local time. This will produce different values for the same calendar date across timezones and with DST transitions.The backend documentation comment ("Start from a specific unix timestamp") doesn't explicitly specify UTC, but Unix timestamps conventionally mean seconds since epoch UTC. The current code will fail silently for users in non-UTC timezones.
Implement either:
- Option A: Add a TimeOfDay picker and compute
DateTime(selected.year, selected.month, selected.day, time.hour, time.minute).toUtc().millisecondsSinceEpoch ~/ 1000- Option B: Change label to "Date (local)" and add
.toUtc()to current calculation:selectedDateTime!.toUtc().millisecondsSinceEpoch ~/ 1000
🧹 Nitpick comments (4)
packages/komodo_defi_sdk/example/lib/widgets/instance_manager/logged_in_view_widget.dart (1)
219-235: LGTM; add lightweight error handling around async calls.Wrap setOneShotSyncParams/saveZhtlcConfig in try/catch to surface failures via SnackBar.
- if (dialogResult != null) { - if (dialogResult.oneShotSync != null) { - await sdk.activationConfigService.setOneShotSyncParams( - asset.id, - dialogResult.oneShotSync, - ); - } - await sdk.activationConfigService.saveZhtlcConfig( - asset.id, - dialogResult.config, - ); - } else { + if (dialogResult != null) { + try { + if (dialogResult.oneShotSync != null) { + await sdk.activationConfigService.setOneShotSyncParams( + asset.id, + dialogResult.oneShotSync, + ); + } + await sdk.activationConfigService.saveZhtlcConfig( + asset.id, + dialogResult.config, + ); + } catch (e) { + if (!mounted) return; + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Failed to save ZHTLC config: $e')), + ); + return; + } + } else { return; // User cancelled }packages/komodo_defi_sdk/example/lib/widgets/instance_manager/zhtlc_config_dialog.dart (2)
21-25: Make the dialog result class const and equatable-friendly.Const constructor improves immutability; optionally add ==/hashCode later if stored.
-class ZhtlcConfigDialogResult { - ZhtlcConfigDialogResult({required this.config, this.oneShotSync}); +class ZhtlcConfigDialogResult { + const ZhtlcConfigDialogResult({required this.config, this.oneShotSync}); final ZhtlcUserConfig config; final ZhtlcSyncParams? oneShotSync; }
387-397: Input validation guards are recommended for robustness.Verification confirms the main concern is already addressed: syncParams has an explicit comment "Optional, accepted for backward compatibility. Not persisted", and the toJson() method serializes only zcashParamsPath, scanBlocksPerIteration, scanIntervalMs, and taskStatusPollingIntervalMs—syncParams is excluded.
However, the suggested input guards remain valid: the current code uses fallback defaults but doesn't validate that successfully parsed user input stays within reasonable bounds (e.g., avoiding negatives). The suggested bounds checks prevent edge-case bugs if users enter negative or zero values.
packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dart (1)
259-267: LGTM: Legacy syncParams correctly converted to one-shot.The backward compatibility handling is well-implemented. When legacy code provides
syncParamsin the config, they're extracted and stored as a one-shot override before the config (withoutsyncParams) is persisted.Consider adding a log statement when converting legacy syncParams to help with debugging:
Future<void> saveZhtlcConfig(AssetId id, ZhtlcUserConfig config) async { final walletId = await _requireActiveWallet(); // If legacy callers provide syncParams in the config, convert it to // a one-shot sync override and do not persist it. if (config.syncParams != null) { + // Log or notify that syncParams are being converted to one-shot _oneShotSyncParams[_WalletAssetKey(walletId, id)] = config.syncParams; } await repo.saveConfig(walletId, id, config); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/komodo_defi_sdk/example/lib/widgets/instance_manager/logged_in_view_widget.dart(1 hunks)packages/komodo_defi_sdk/example/lib/widgets/instance_manager/zhtlc_config_dialog.dart(7 hunks)packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/zhtlc_activation_strategy.dart(2 hunks)packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dart(5 hunks)packages/komodo_defi_sdk/lib/src/bootstrap.dart(1 hunks)packages/komodo_defi_sdk/lib/src/komodo_defi_sdk.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: Flutter tests (all packages)
- GitHub Check: setup
🔇 Additional comments (9)
packages/komodo_defi_sdk/lib/src/activation/protocol_strategies/zhtlc_activation_strategy.dart (2)
1-3: Helpful doc comment.Nice clarification on one‑shot sync param usage and UI tie‑in.
85-100: Reject this review comment—ActivationRpcData and ActivationMode do not expose copyWith methods.Both
ActivationRpcDataandActivationModelackcopyWith()implementations. The current code at lines 85–100 correctly uses direct constructors to rebuild these objects. The suggested diff's runtime type checks (rpcData is dynamic && rpcData.copyWith != null) would not work and is not idiomatic Dart. When classes don't providecopyWith(), direct construction is the appropriate pattern.Likely an incorrect or invalid review comment.
packages/komodo_defi_sdk/example/lib/widgets/instance_manager/zhtlc_config_dialog.dart (3)
10-12: Import surface update — OK.Switching to SDK exports for ZhtlcUserConfig/ZhtlcSyncParams keeps UI decoupled from RPC package internals.
152-152: Public API now returns ZhtlcConfigDialogResult? — good.Matches the one‑shot workflow while preserving cancel semantics.
208-208: Private dialog API return type updated — good.Consistent with the public wrapper type.
packages/komodo_defi_sdk/lib/src/bootstrap.dart (1)
91-92: All concerns verified and properly implemented.The
ActivationConfigServicecorrectly:
- Subscribes to
authStateChangesin the constructor and stores the subscription- Clears all one-shot sync params when user signs out (
user == null, line 201)- Clears wallet-specific params when the wallet changes (line 206)
- Cancels and nullifies the subscription in
dispose()(lines 302–303)The bootstrap wiring at lines 91–92 is correct and the service lifecycle is properly managed.
packages/komodo_defi_sdk/lib/src/komodo_defi_sdk.dart (1)
411-413: Disposal implementation is correct and idempotent.The
dispose()method properly cancels the_authStateSubscriptionusing the null-coalescing operator (?.cancel()), making it safe to call multiple times. The subscription is set tonullafter cancellation, preventing any resource leaks. The added ActivationConfigService disposal in the Future.wait is properly handled.packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dart (2)
55-79: Good backward compatibility approach for syncParams deprecation.The implementation correctly handles the transition from persisted to one-shot syncParams:
- The field remains in the constructor for backward compatibility with existing code
- Clear documentation explains the non-persisted, one-shot behavior (lines 55-61)
toJsonomits the field, preventing persistencefromJsondoesn't parse it, ensuring loaded configs havesyncParams = nullThis approach allows legacy callers to continue passing
syncParamsthrough the config while the service (viasaveZhtlcConfiglines 263-265) converts them to one-shot overrides.
275-298: LGTM: One-shot sync parameter methods correctly implemented.The one-shot lifecycle is properly implemented:
setOneShotSyncParams: Stores value keyed by wallet+assettakeOneShotSyncParams: Retrieves and removes in a single operation (consume-once pattern)clearOneShotSyncParamsForWallet: Provides wallet-scoped cleanupAll methods consistently use
_requireActiveWallet()for proper wallet context, and the composite key approach prevents cross-wallet contamination.
| ActivationConfigService( | ||
| this.repo, { | ||
| required WalletIdResolver walletIdResolver, | ||
| }) : _walletIdResolver = walletIdResolver; | ||
| Stream<KdfUser?>? authStateChanges, | ||
| }) : _walletIdResolver = walletIdResolver { | ||
| // Listen to auth state changes to clear one-shot params on sign-out | ||
| _authStateSubscription = authStateChanges?.listen((user) { | ||
| if (user == null) { | ||
| // User signed out, clear all one-shot params | ||
| _oneShotSyncParams.clear(); | ||
| } else { | ||
| // User signed in or changed, clear one-shot params for previous wallet | ||
| // if it was different from the current one | ||
| if (_lastWalletId != null && _lastWalletId != user.walletId) { | ||
| clearOneShotSyncParamsForWallet(_lastWalletId!); | ||
| } | ||
| _lastWalletId = user.walletId; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify auth state stream behavior and consider initialization.
The auth state listener correctly handles sign-out and wallet switching, but there are two potential edge cases:
-
If
authStateChangesdoesn't emit the current authenticated user immediately upon subscription (e.g., not a BehaviorSubject),_lastWalletIdwill remainnulleven when a user is already authenticated. This means the first wallet switch won't trigger cleanup of the previous wallet's one-shot params (line 205 check fails). -
The
authStateChangesparameter is optional. Ifnull, one-shot params won't be automatically cleared on sign-out, potentially causing them to persist across sessions.
Consider either:
- Initializing
_lastWalletIdby querying current wallet state via_walletIdResolver()in the constructor - Documenting that
authStateChangesshould emit the current user upon subscription - Requiring
authStateChangesto be non-null for production use
Run the following script to check how authStateChanges is wired in the bootstrap:
🏁 Script executed:
#!/bin/bash
# Check how authStateChanges stream is provided to ActivationConfigService
rg -A 5 -B 5 'ActivationConfigService\(' --type=dartLength of output: 2378
---
🏁 Script executed:
```shell
#!/bin/bash
# Find KomodoDefiLocalAuth class definition and authStateChanges property
rg -n 'class KomodoDefiLocalAuth' --type=dart -A 20
Length of output: 2265
🏁 Script executed:
#!/bin/bash
# Search for authStateChanges stream definition/initialization
rg -n 'authStateChanges' --type=dart -B 2 -A 2Length of output: 17571
---
🏁 Script executed:
```shell
#!/bin/bash
# Check for any initialization or handling of _lastWalletId in ActivationConfigService
rg -n '_lastWalletId' --type=dart -B 2 -A 2 packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dart
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Check full constructor and initialization logic
sed -n '192,220p' packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dartLength of output: 1091
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for tests of ActivationConfigService auth state handling
rg -n 'ActivationConfigService' --type=dart packages/komodo_defi_sdk/test/ -B 2 -A 5 | head -100
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for dispose method in ActivationConfigService
sed -n '301,310p' packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dartLength of output: 214
Initialize _lastWalletId with current wallet to fix wallet-switch edge case.
The auth state stream (authStateChanges) is an async* generator that doesn't emit the current user on subscription—only subsequent state changes. When ActivationConfigService is created mid-session with an already-authenticated user, _lastWalletId remains null. This causes the first wallet switch to skip cleanup of the previous wallet's one-shot params (line 205 check fails).
Fix: Initialize _lastWalletId in the constructor using the _walletIdResolver:
ActivationConfigService(
this.repo, {
required WalletIdResolver walletIdResolver,
Stream<KdfUser?>? authStateChanges,
}) : _walletIdResolver = walletIdResolver {
// Initialize current wallet ID
walletIdResolver().then((walletId) {
_lastWalletId = walletId;
});
// Listen to auth state changes...
_authStateSubscription = authStateChanges?.listen((user) { ... });
}Additionally, dispose() should clear _oneShotSyncParams map to ensure params don't persist after disposal.
🤖 Prompt for AI Agents
In
packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dart
around lines 192 to 211, initialize _lastWalletId at construction so a service
created mid-session knows the current wallet and will correctly clear
previous-wallet one-shot params on the first switch: call _walletIdResolver()
(or walletIdResolver()) and set _lastWalletId from its result (use then/await as
appropriate) before subscribing to authStateChanges; also update dispose() to
clear the _oneShotSyncParams map so one-shot params do not persist after
disposal.
| /// Disposes of the service and cleans up resources. | ||
| void dispose() { | ||
| _authStateSubscription?.cancel(); | ||
| _authStateSubscription = null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Complete the cleanup by clearing one-shot params.
The dispose method cancels the auth state subscription but doesn't clear the _oneShotSyncParams map. For complete resource cleanup and to prevent potential issues if the service is accessed after disposal, the map should also be cleared.
Apply this diff:
/// Disposes of the service and cleans up resources.
void dispose() {
_authStateSubscription?.cancel();
_authStateSubscription = null;
+ _oneShotSyncParams.clear();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Disposes of the service and cleans up resources. | |
| void dispose() { | |
| _authStateSubscription?.cancel(); | |
| _authStateSubscription = null; | |
| } | |
| /// Disposes of the service and cleans up resources. | |
| void dispose() { | |
| _authStateSubscription?.cancel(); | |
| _authStateSubscription = null; | |
| _oneShotSyncParams.clear(); | |
| } |
🤖 Prompt for AI Agents
In
packages/komodo_defi_sdk/lib/src/activation_config/activation_config_service.dart
around lines 300 to 304, the dispose method cancels the auth subscription but
doesn’t clear the _oneShotSyncParams map; add a call to
_oneShotSyncParams.clear() (after cancelling and nulling the subscription) to
fully release one-shot parameters and avoid leftover state if the service is
accessed post-disposal.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (params.mode?.rpcData != null) { | ||
| final oneShotSync = await configService.takeOneShotSyncParams(asset.id); | ||
| if (oneShotSync != null) { | ||
| final rpcData = params.mode!.rpcData!; | ||
| final updatedRpcData = ActivationRpcData( | ||
| lightWalletDServers: rpcData.lightWalletDServers, | ||
| electrum: rpcData.electrum, | ||
| syncParams: oneShotSync, | ||
| ); |
There was a problem hiding this comment.
When applying one‑shot sync_params we rebuild ActivationRpcData with only lightWalletDServers and electrum, dropping other properties such as minConnected and maxConnected. For ZHTLC assets that define non‑default connection limits, supplying a one‑shot sync parameter will silently revert those settings to the defaults (maxConnected becomes 1), potentially reducing reliability. Consider cloning the existing rpcData or copying all fields so connection limits are preserved when oneShotSync is applied.
Useful? React with 👍 / 👎.
|
Visit the preview URL for this PR (updated for commit 8561d33): https://komodo-playground--pr265-dev-n21qdi8n.web.app (expires Mon, 03 Nov 2025 22:42:59 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 2bfedd77fdea45b25ba7c784416e81f177aa5c47 |
|
Visit the preview URL for this PR (updated for commit 8561d33): https://kdf-sdk--pr265-dev-5mse1dpd.web.app (expires Mon, 03 Nov 2025 22:45:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 9c1b6e6c010cf0b965c455ba7a69c4aedafa8a1d |
Summary by CodeRabbit
New Features
Refactor
Chores