feat: implement all wired TODOs across Rust and Dart layers - #82
Merged
Conversation
Rust: - orders.rs: remove stale Phase 7 doc comment; expose pub(crate) trade_key_for_order and publish_event for cross-module use - actions.rs: add rate_user() building a RateUser NIP-59 gift wrap - reputation.rs: wire submit_rating to dispatch RateUser via the daemon; replace set_privacy_mode TODO with best-effort identity check Dart: - about_screen: async-load app version via getAppVersion() bridge - privacy_mode_provider: init from getPrivacyMode(), propagate changes to setPrivacyMode() on every toggle - rate_counterpart_screen: call submitRating() instead of delay stub - connect_wallet_screen: call connectWallet() and populate state from returned NwcWalletInfo - wallet_settings_screen: call disconnectWallet() before clearing state - relay_management_card: load relays from getRelays() on init; wire addRelay() and removeRelay() bridge calls
- orders.rs: add subscribe_gift_wraps() spawned per maker order; decrypts Kind 1059 events with trade key, calls resolve_maker_order() on Action::NewOrder so daemon UUID is known before K38383 arrives - orders.rs: add process_gift_wrap_rumor() for dispatching inner message - orders.rs: call subscribe_gift_wraps() from create_order() after key derivation - actions.rs: add dispute() action builder using Action::Dispute - disputes.rs: wire open_dispute() to dispatch Action::Dispute via NIP-59 using the stored trade key index; logs warn when no trade key found - relay_pool.rs: remove Phase 3 TODO, note K1059 is handled per-trade Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
WalkthroughIntegrates Flutter UI with Rust bridge APIs (reputation, nwc, nostr), replaces stubbed flows with real bridge calls, adds NIP‑59 Mostro action builders and per‑trade gift‑wrap subscription/processing, and adds relay-management localization and provider wiring. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
- orders.rs: fetch trade keys before subscribing to prevent orphan relay subscription if key derivation fails - privacy_mode_provider.dart: await setPrivacyMode and roll back state on failure instead of fire-and-forget ignore() - about_screen.dart: set _appVersion = 'unknown' on getAppVersion failure instead of silently swallowing the error - relay_management_card.dart: guard _loadRelays with _loading flag to prevent concurrent load races; make _removeRelay async with rollback + SnackBar on failure; rollback optimistic add + show SnackBar on failure - l10n: add relayAddFailed / relayRemoveFailed keys in all 5 languages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/src/nostr/relay_pool.rs (1)
143-162:⚠️ Potential issue | 🟠 MajorReconnect still cannot rebuild the per-trade gift-wrap workers.
This method only gets
trade_pubkeysand re-applies a raw Kind 1059 filter. The actual worker incrate::api::orders::subscribe_gift_wraps()also needs the trade index to load recipient keys, so after restart/reconnect existing maker orders still have no decrypt/routing path for gift-wrap ACK/cancel events. Rehydrate(trade_pubkey, trade_index)pairs and respawn those workers here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/nostr/relay_pool.rs` around lines 143 - 162, subscribe_order_and_dm_feeds currently re-subscribes a raw Kind 1059 filter using only trade_pubkeys, which doesn't rehydrate per-trade gift-wrap workers because subscribe_gift_wraps (and create_order) require the trade_index to load recipient keys; fix by querying stored maker orders (e.g. using pending_orders_filter or DB/state that holds orders) to rebuild a Vec of (trade_pubkey, trade_index) pairs, then for each pair call the same code path used by crate::api::orders::subscribe_gift_wraps() (or factor out its worker-spawn logic into a helper) to respawn the per-trade gift-wrap worker so decrypt/routing keys are restored for KIND_GIFT_WRAP events.rust/src/api/reputation.rs (1)
128-173:⚠️ Potential issue | 🔴 CriticalMake rating submission atomic with the outbound send.
The
RateUserevent is built/published before the one-shot local insert. Two concurrent callers can therefore emit multiple rating events, and a missing trade key or transient build/publish failure still lets the later insert consume the only rating slot, blocking any retry. Reserve a pending rating under the same lock, then finalize or roll back based on the send result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/reputation.rs` around lines 128 - 173, Reserve the rating slot before any outbound send by calling store.try_insert_mine(...) with a provisional RatingInfo (e.g. a "pending" marker or is_mine=true but clearly reserved) for trade_id, then perform the trade key lookup and call crate::mostro::actions::rate_user and crate::api::orders::publish_event; if the send succeeds leave/finalize the stored RatingInfo, but if any step fails remove or roll back the reservation (implement and call a store.remove_mine/delete_mine(trade_id) or an update-to-failed state) so a transient publish/build error doesn't consume the only rating slot—use the existing symbols RatingInfo, store.try_insert_mine, crate::mostro::actions::rate_user, and crate::api::orders::publish_event to locate and implement this change.
🧹 Nitpick comments (2)
lib/features/settings/screens/wallet_settings_screen.dart (1)
43-55: Consider only showing success feedback when disconnect actually succeeds.The current implementation shows "Wallet disconnected" snackbar and clears local state even when the bridge call fails. While the Rust side's only documented failure mode is
NoWalletConnected(which makes the unconditionalsetDisconnected()safe), showing a success message after a failure could mislead users.♻️ Suggested approach to differentiate success/failure feedback
Future<void> _disconnect(BuildContext context, WidgetRef ref) async { + bool success = false; try { await nwc_api.disconnectWallet(); + success = true; } catch (e) { debugPrint('[WalletSettings] disconnect failed: $e'); } ref.read(nwcProvider.notifier).setDisconnected(); if (context.mounted) { ScaffoldMessenger.of(context).showSnackBar( - const SnackBar(content: Text('Wallet disconnected')), + SnackBar( + content: Text(success + ? 'Wallet disconnected' + : 'Wallet cleared locally'), + ), ); } }lib/features/about/screens/about_screen.dart (1)
14-30: Use Riverpod state management instead of StatefulWidget for the app version.This async state is managed with
StatefulWidget/setState, but the codebase uses Riverpod extensively (21+ files across lib/). Refactor to aFutureProviderandConsumerWidgetfor consistency.♻️ Refactor sketch
+import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:mostro/src/rust/api.dart' as rust_api; +final appVersionProvider = FutureProvider<String>( + (ref) => rust_api.getAppVersion(), +); + -class AboutScreen extends StatefulWidget { +class AboutScreen extends ConsumerWidget { const AboutScreen({super.key}); `@override` - State<AboutScreen> createState() => _AboutScreenState(); -} - -class _AboutScreenState extends State<AboutScreen> { - String _appVersion = '…'; - - `@override` - void initState() { - super.initState(); - rust_api.getAppVersion().then((v) { - if (mounted) setState(() => _appVersion = v); - }).catchError((_) {}); - } + Widget build(BuildContext context, WidgetRef ref) { + final version = ref.watch(appVersionProvider); + final appVersionText = version.when( + data: (v) => 'v$v', + loading: () => 'v…', + error: (_, __) => 'vunknown', + ); + // existing UI... + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/about/screens/about_screen.dart` around lines 14 - 30, The AboutScreen currently uses StatefulWidget with _AboutScreenState and a _appVersion field populated via rust_api.getAppVersion() in initState; replace this with a Riverpod FutureProvider (e.g., appVersionProvider) that calls rust_api.getAppVersion(), remove the StatefulWidget and _appVersion/_AboutScreenState, and convert AboutScreen into a ConsumerWidget that reads ref.watch(appVersionProvider) and handles AsyncValue.when (loading, error, data) to render the version UI; ensure you import flutter_riverpod and update any references to use the provider instead of setState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/about/screens/about_screen.dart`:
- Line 29: The code is silently swallowing errors in the catchError on the
version-fetch future in about_screen.dart; update the catchError handler (the
chain that ends with .catchError((_) {})) to set a sensible fallback version
string (e.g. "unknown" or "N/A") into the same state variable used to display
the version and log the error once (use your app logger or debugPrint) including
the exception details; ensure you call setState (or the appropriate state
updater) so the UI leaves the loading "…" state and shows the fallback version.
In `@lib/features/account/providers/privacy_mode_provider.dart`:
- Around line 26-28: In setPrivacyMode (privacy_mode_provider.dart) the call
reputation_api.setPrivacyMode(...) uses .ignore() but dart:async isn't imported
and the function returns Future<void>; fix by either importing dart:async so
.ignore() is available, or replace the .ignore() usage with
unawaited(reputation_api.setPrivacyMode(enabled: enabled)) and import
package:flutter/foundation.dart if needed, or alternatively make setPrivacyMode
async and await reputation_api.setPrivacyMode(...); update imports accordingly
and ensure the call matches the chosen approach.
In `@lib/features/settings/widgets/relay_management_card.dart`:
- Around line 75-81: The _removeRelay method currently removes the item from the
_relays list before calling nostr_api.removeRelay, which can desync UI if the
API fails; change the logic to perform the API call first or perform a rollback
on failure: capture the removed entry (e.g., final removed = _relays[index]),
call nostr_api.removeRelay(url: removed.url).then((_) => setState(() =>
_relays.removeAt(index))).catchError((e) { setState(() => _relays.insert(index,
removed)); debugPrint(...); }); — this uses the existing _removeRelay, _relays,
and nostr_api.removeRelay symbols to ensure the UI is only updated on success
and restored on error.
- Around line 145-147: The optimistic UI adds the relay before the backend call
and never rolls back on failure; update the logic around nostr_api.addRelay(url:
url) to mirror the removal case: either await the addRelay call before updating
the UI, or keep the optimistic insertion but in the onError callback remove the
relay from the in-memory list/state and trigger a UI refresh (and surface an
error message). Specifically, update the code path that performs the optimistic
add (the code invoking nostr_api.addRelay) so that onError undoes the prior
state change (remove the relay entry) and logs/shows the error, matching the
rollback behavior used for removeRelay.
In `@rust/src/api/disputes.rs`:
- Around line 139-176: The code currently inserts the dispute via
try_insert_if_absent_or_resolved() before attempting the Gift Wrap dispatch,
making failures non-retryable; change the flow so the dispute is not marked
fully "open" until the remote dispatch succeeds: either (A) attempt the full
dispatch sequence first (use trade_key_for_order, get_active_trade_keys,
crate::mostro::actions::dispute, and publish_event) and only call
try_insert_if_absent_or_resolved() after publish_event returns Ok, or (B)
introduce a distinct retryable state (e.g., "pending" or "dispatch_failed") in
the dispute store and call try_insert_if_absent_or_resolved_pending() initially,
then update to "open" only after publish_event succeeds; ensure error cases from
get_active_trade_keys, dispute build (crate::mostro::actions::dispute), and
publish_event bubble up or set the pending state rather than logging and
returning Ok(dispute).
In `@rust/src/api/orders.rs`:
- Around line 757-790: The match currently treats Ok(Err(_)) from rx.recv() as
terminal and breaks the per-trade listener; change it to inspect the RecvError
so that RecvError::Lagged(_) results in continue (recoverable) while only
RecvError::Closed (or other fatal cases) causes break—apply this to the match
handling rx.recv() (the arm that now reads Ok(Err(_)) => break) so the
RelayPoolNotification event loop (where gift-wrap events are deserialized and
process_gift_wrap_rumor is called) mirrors the recover-on-lagged behavior used
elsewhere (~lines 1092–1095).
---
Outside diff comments:
In `@rust/src/api/reputation.rs`:
- Around line 128-173: Reserve the rating slot before any outbound send by
calling store.try_insert_mine(...) with a provisional RatingInfo (e.g. a
"pending" marker or is_mine=true but clearly reserved) for trade_id, then
perform the trade key lookup and call crate::mostro::actions::rate_user and
crate::api::orders::publish_event; if the send succeeds leave/finalize the
stored RatingInfo, but if any step fails remove or roll back the reservation
(implement and call a store.remove_mine/delete_mine(trade_id) or an
update-to-failed state) so a transient publish/build error doesn't consume the
only rating slot—use the existing symbols RatingInfo, store.try_insert_mine,
crate::mostro::actions::rate_user, and crate::api::orders::publish_event to
locate and implement this change.
In `@rust/src/nostr/relay_pool.rs`:
- Around line 143-162: subscribe_order_and_dm_feeds currently re-subscribes a
raw Kind 1059 filter using only trade_pubkeys, which doesn't rehydrate per-trade
gift-wrap workers because subscribe_gift_wraps (and create_order) require the
trade_index to load recipient keys; fix by querying stored maker orders (e.g.
using pending_orders_filter or DB/state that holds orders) to rebuild a Vec of
(trade_pubkey, trade_index) pairs, then for each pair call the same code path
used by crate::api::orders::subscribe_gift_wraps() (or factor out its
worker-spawn logic into a helper) to respawn the per-trade gift-wrap worker so
decrypt/routing keys are restored for KIND_GIFT_WRAP events.
---
Nitpick comments:
In `@lib/features/about/screens/about_screen.dart`:
- Around line 14-30: The AboutScreen currently uses StatefulWidget with
_AboutScreenState and a _appVersion field populated via rust_api.getAppVersion()
in initState; replace this with a Riverpod FutureProvider (e.g.,
appVersionProvider) that calls rust_api.getAppVersion(), remove the
StatefulWidget and _appVersion/_AboutScreenState, and convert AboutScreen into a
ConsumerWidget that reads ref.watch(appVersionProvider) and handles
AsyncValue.when (loading, error, data) to render the version UI; ensure you
import flutter_riverpod and update any references to use the provider instead of
setState.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b5dc636-8d6f-4634-947e-74311e965dfa
📒 Files selected for processing (11)
lib/features/about/screens/about_screen.dartlib/features/account/providers/privacy_mode_provider.dartlib/features/rate/screens/rate_counterpart_screen.dartlib/features/settings/screens/connect_wallet_screen.dartlib/features/settings/screens/wallet_settings_screen.dartlib/features/settings/widgets/relay_management_card.dartrust/src/api/disputes.rsrust/src/api/orders.rsrust/src/api/reputation.rsrust/src/mostro/actions.rsrust/src/nostr/relay_pool.rs
Comment on lines
+14
to
+23
| PrivacyModeNotifier() : super(false) { | ||
| _init(); | ||
| } | ||
|
|
||
| Future<void> _init() async { | ||
| try { | ||
| final current = await reputation_api.getPrivacyMode(); | ||
| if (mounted) state = current; | ||
| } catch (_) {} | ||
| } |
Contributor
There was a problem hiding this comment.
This provider fails open while the Rust value is still loading.
super(false) is observable until _init() completes. Consumers already use this provider synchronously to choose the selected privacy option and to hide/show reputation metadata, so a stored true value briefly renders as "privacy off", and the empty catch leaves that unsafe fallback in place forever on bridge errors. Model this as loading/nullable state or default closed until the bridge value arrives. See lib/features/account/screens/account_screen.dart:245-265 and lib/features/order/screens/take_order_screen.dart:289-299.
orders.rs: distinguish RecvError::Lagged (continue) from Closed (break) in gift-wrap loop so a slow subscriber recovers instead of terminating. disputes.rs: dispatch Action::Dispute to Mostro before persisting the local record so failed publishes leave a retryable clean slate; update tests to use seed_dispute() helper that bypasses dispatch. reputation.rs: reserve the rating slot via try_insert_mine() before attempting publish_event so concurrent submit_rating calls can't both reach the wire; add remove_mine() rollback called on dispatch failure. relay_pool.rs: change subscribe_order_and_dm_feeds() signature from Vec<PublicKey> to Vec<(PublicKey, u32)> and spawn a subscribe_gift_wraps worker per trade key so per-trade decrypt/routing is restored on reconnect. about_screen.dart: replace StatefulWidget + initState pattern with a FutureProvider<String> (appVersionProvider) and ConsumerWidget so version loading is handled declaratively with AsyncValue.when(loading/data/error).
This was referenced Apr 3, 2026
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
1 participant
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.
Rust:
trade_key_for_order and publish_event for cross-module use
daemon; replace set_privacy_mode TODO with best-effort identity check
Dart:
to setPrivacyMode() on every toggle
returned NwcWalletInfo
addRelay() and removeRelay() bridge calls
decrypts Kind 1059 events with trade key, calls resolve_maker_order()
on Action::NewOrder so daemon UUID is known before K38383 arrives
using the stored trade key index; logs warn when no trade key found
Summary by CodeRabbit
New Features
Bug Fixes