feat(foundation): phase 2 — foundational infrastructure - #51
Conversation
- Rust shared types (api/types.rs): all enums and structs per contracts/types.md - Storage layer: async Storage trait, SQLite backend (sqlx), IndexedDB WASM stub - NIP-59 Gift Wrap encode/decode (nostr-sdk async API) - Relay pool: Kind 38383 + Kind 1059 subscriptions, broadcast state channels - Offline message queue with exponential backoff - Flutter design system tokens (AppColors, AppSpacing, AppRadius, dark/light themes) - GoRouter scaffold with all 23 routes as stubs - App bootstrap: ProviderScope + MaterialApp.router + localisation
|
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 (4)
WalkthroughAdds app bootstrap (Riverpod-wrapped MostroApp with router, themes, and localization), GoRouter routes, design-system theming, comprehensive Rust API types, storage abstractions with SQLite and IndexedDB stub, Nostr helpers (Gift Wrap, order events, relay pool), and an offline queued-message implementation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
rust/src/nostr/relay_pool.rs (1)
132-149: Subscriptions established but event handling deferred.
subscribe_order_and_dm_feedsregisters Nostr subscriptions but doesn't process incoming events. Per the context snippet, there's no event loop callingparse_order_event. If this is intentional for Phase 2 scaffolding, consider adding a TODO comment.🤖 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 132 - 149, subscribe_order_and_dm_feeds currently only registers subscriptions (via client.subscribe with pending_orders_filter and Filter::new().kind(KIND_GIFT_WRAP).pubkeys) but never consumes incoming events; either add an event-processing loop that pulls events from the client's event stream and dispatches them (e.g., call parse_order_event for Kind 38383/order events and an appropriate handler for KIND_GIFT_WRAP DMs) after subscribing, or if this is intentional scaffolding add a clear TODO comment inside subscribe_order_and_dm_feeds referencing parse_order_event, pending_orders_filter, KIND_GIFT_WRAP, and client.subscribe so future work will implement the event loop.rust/src/api/types.rs (1)
184-201: Consider documenting the precision expectations forf64monetary fields.Using
f64forfiat_amount,fiat_amount_min,fiat_amount_max, andpremiumis acceptable for display and transmission purposes. However, adding a brief doc comment noting that these values are for display only (not for precise financial calculations) would help prevent misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/types.rs` around lines 184 - 201, Add brief doc comments to the OrderInfo struct (or directly above the fields fiat_amount, fiat_amount_min, fiat_amount_max, and premium) stating these f64 fields are for display/transmission only and must not be used for precise financial calculations; mention that precise arithmetic should use integer minor units or a decimal type (e.g., rust_decimal) instead and include the note that precision/rounding expectations are not guaranteed for these f64 values.rust/src/db/mod.rs (1)
10-11: Consider whetherSend + Syncbounds are needed givenasync fn in traitlimitations.Using
#[allow(async_fn_in_trait)]suppresses the lint, but the underlying limitation remains: futures returned by async trait methods do not inheritSendbounds automatically. If you need to call these methods from contexts that spawn tasks (e.g.,tokio::spawn), this will cause compilation errors.If cross-thread usage is required, consider using the
async-traitcrate or returningPin<Box<dyn Future<...> + Send>>explicitly. If the trait is only used within single-threaded async executors (especially relevant for WASM), the current design is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/db/mod.rs` around lines 10 - 11, The Storage trait currently uses #[allow(async_fn_in_trait)] with Send + Sync bounds but async trait methods' returned futures won't automatically be Send; decide expected concurrency and either (A) make async trait methods return explicit Send futures (e.g., change signatures to return Pin<Box<dyn Future<Output = ...> + Send>> or use a BoxFuture) or (B) switch to the async-trait crate (apply #[async_trait] to the Storage trait and its impls) so the generated futures are Send when needed, or (C) remove Send + Sync from the trait if it will only be used on a single-threaded executor; update implementations of Storage accordingly (look for the Storage trait definition and its async methods and impl blocks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/db/sqlite.rs`:
- Around line 224-236: The update_queued_message_status function updates only
the status column which leads to stale status inside the JSON `data` blob read
by list_queued_messages; modify update_queued_message_status to also update the
`data` column by loading the existing `data` JSON for the given id (or accept a
full QueuedMessage), deserialize it, set its status to the new
QueuedMessageStatus, reserialize and then execute an UPDATE that sets both
status and data (or alternatively, replace callers to use save_queued_message
instead of update_queued_message_status); locate the logic in
update_queued_message_status and implement the
fetch-deserialize-update-serialize flow or change call sites to call
save_queued_message to ensure JSON and column status stay in sync.
In `@rust/src/nostr/gift_wrap.rs`:
- Around line 14-35: The call to EventBuilder::gift_wrap_from_seal in the wrap
function is using the wrong signature and missing an await; change the
invocation to pass the signer (sender_keys) as the first argument, keep
recipient_pubkey and seal as subsequent args, and append .await before map_err
so the async method is awaited (i.e., call
EventBuilder::gift_wrap_from_seal(sender_keys, recipient_pubkey, &seal,
...).await and map the error with anyhow!("NIP-59 gift_wrap failed: {e}")).
In `@rust/src/nostr/relay_pool.rs`:
- Around line 23-24: conn_tx and relay_tx (broadcast::Sender<ConnectionState>
and broadcast::Sender<RelayInfo>) are never used so subscribers from the
subscribe_* methods never get events; send updates whenever the pool's state
changes — e.g. call conn_tx.send(updated_connection_state) wherever connection
lifecycle transitions occur (connect, disconnect, reconnect, error handling) and
call relay_tx.send(updated_relay_info) whenever relay metadata or health changes
(add/remove relay, health check results, status updates); handle or log the
Result from broadcast::Sender::send to ignore SendError (e.g. no subscribers) or
log failures and import/ use the existing logging facility so these sends don't
panic.
- Around line 106-122: Relay statuses never update because add_relay_internal
sets RelayStatus::Connecting and never listens for the Client's real status
updates; modify add_relay_internal to subscribe to the nostr_sdk Client/relay
notification stream (or its status callback), update the corresponding
RelayInfo.status (the RelayInfo entries in self.relays) to
RelayStatus::Connected/Disconnected as notifications arrive, and broadcast those
changes on the existing conn_tx and relay_tx channels (clone the senders into
the task handling notifications). Also ensure the notification task holds a copy
of the relay id/URL to locate and mutably update the right RelayInfo and send
state updates after each status change so connection_state() reflects live
statuses.
---
Nitpick comments:
In `@rust/src/api/types.rs`:
- Around line 184-201: Add brief doc comments to the OrderInfo struct (or
directly above the fields fiat_amount, fiat_amount_min, fiat_amount_max, and
premium) stating these f64 fields are for display/transmission only and must not
be used for precise financial calculations; mention that precise arithmetic
should use integer minor units or a decimal type (e.g., rust_decimal) instead
and include the note that precision/rounding expectations are not guaranteed for
these f64 values.
In `@rust/src/db/mod.rs`:
- Around line 10-11: The Storage trait currently uses
#[allow(async_fn_in_trait)] with Send + Sync bounds but async trait methods'
returned futures won't automatically be Send; decide expected concurrency and
either (A) make async trait methods return explicit Send futures (e.g., change
signatures to return Pin<Box<dyn Future<Output = ...> + Send>> or use a
BoxFuture) or (B) switch to the async-trait crate (apply #[async_trait] to the
Storage trait and its impls) so the generated futures are Send when needed, or
(C) remove Send + Sync from the trait if it will only be used on a
single-threaded executor; update implementations of Storage accordingly (look
for the Storage trait definition and its async methods and impl blocks).
In `@rust/src/nostr/relay_pool.rs`:
- Around line 132-149: subscribe_order_and_dm_feeds currently only registers
subscriptions (via client.subscribe with pending_orders_filter and
Filter::new().kind(KIND_GIFT_WRAP).pubkeys) but never consumes incoming events;
either add an event-processing loop that pulls events from the client's event
stream and dispatches them (e.g., call parse_order_event for Kind 38383/order
events and an appropriate handler for KIND_GIFT_WRAP DMs) after subscribing, or
if this is intentional scaffolding add a clear TODO comment inside
subscribe_order_and_dm_feeds referencing parse_order_event,
pending_orders_filter, KIND_GIFT_WRAP, and client.subscribe so future work will
implement the event loop.
🪄 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: 943985de-b072-40d2-9f2f-c6eb2f414103
📒 Files selected for processing (18)
lib/core/app.dartlib/core/app_routes.dartlib/core/app_theme.dartlib/main.dartrust/src/api/mod.rsrust/src/api/types.rsrust/src/db/indexeddb.rsrust/src/db/mod.rsrust/src/db/schema.rsrust/src/db/sqlite.rsrust/src/nostr/gift_wrap.rsrust/src/nostr/mod.rsrust/src/nostr/order_events.rsrust/src/nostr/relay_pool.rsrust/src/queue/mod.rsrust/src/queue/outbox.rsspecs/004-mostro-p2p-client/tasks.mdtest/widget_test.dart
| pub async fn wrap( | ||
| sender_keys: &Keys, | ||
| recipient_pubkey: &PublicKey, | ||
| content: &str, | ||
| kind: Kind, | ||
| ) -> Result<String> { | ||
| let rumor = EventBuilder::new(kind, content).build(sender_keys.public_key()); | ||
|
|
||
| let seal_builder = EventBuilder::seal(sender_keys, recipient_pubkey, rumor) | ||
| .await | ||
| .map_err(|e| anyhow!("NIP-59 seal failed: {e}"))?; | ||
|
|
||
| let seal = seal_builder | ||
| .sign_with_keys(sender_keys) | ||
| .map_err(|e| anyhow!("seal sign failed: {e}"))?; | ||
|
|
||
| let gift_wrap = | ||
| EventBuilder::gift_wrap_from_seal(recipient_pubkey, &seal, []) | ||
| .map_err(|e| anyhow!("NIP-59 gift_wrap failed: {e}"))?; | ||
|
|
||
| Ok(gift_wrap.as_json()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Cargo.toml for nostr-sdk version
rg -n 'nostr-sdk' rust/Cargo.toml
# Search for any existing usage patterns of gift_wrap_from_seal in the codebase
ast-grep --pattern 'gift_wrap_from_seal($$$)'Repository: MostroP2P/app
Length of output: 149
🌐 Web query:
nostr-sdk 0.44 gift_wrap_from_seal Event API documentation
💡 Result:
The gift_wrap_from_seal method is part of the EventBuilder API in the nostr-sdk (version 0.44+), specifically exposed for NIP-59 Gift Wrap functionality. It creates a gift wrap event (kind 1059) from an existing seal event (kind 13), which itself wraps an encrypted rumor (unsigned event). This is a two-step process in NIP-59: first seal the rumor, then gift-wrap the seal using a throwaway keypair for privacy. No official Rust documentation snippet explicitly shows the method signature in search results, but it is confirmed exposed in nostr-sdk-flutter bindings (which wrap the core nostr/nostr-sdk crates) alongside related methods like EventBuilder::seal, EventBuilder::gift_wrap, and UnwrappedGift. The core nostr crate (v0.44) provides EventBuilder for NIP-59 support via the nip59 feature flag. Usage pattern (inferred from NIP-59 Rust book example and flutter exposure): use nostr_sdk::prelude::*; // Assume 'seal' is an Event (kind 13) created via EventBuilder::seal or equivalent let gift_wrap = EventBuilder::gift_wrap_from_seal(signer, recipient_pubkey, seal).await?; Enable with nip59 feature: [dependencies] nostr-sdk = { version = "0.44", features = ["nip59"] } For full NIP-59 flow: 1. Create rumor: UnsignedEvent 2. Create seal: Encrypt rumor to recipient, sign as kind 13 3. Create gift wrap: Encrypt seal with ephemeral keys, sign as kind 1059 with p tag for recipient See Rust Nostr Book NIP-59 example using EventBuilder::gift_wrap(keys, recipient, rumor) for the standard single-step builder (likely handles seal internally). gift_wrap_from_seal allows custom seals. Primary sources: docs.rs/nostr (EventBuilder), rust-nostr.org/sdk/nips/59.html (examples), GitHub rust-nostr/nostr-sdk-flutter CHANGELOG (API exposure confirmation).
Citations:
- 1: https://rust-nostr.org/sdk/nips/59.html
- 2: https://docs.rs/crate/nostr-sdk/latest
- 3: https://github.com/rust-nostr/nostr-sdk-flutter/blob/master/CHANGELOG.md
- 4: https://docs.rs/nostr/latest/nostr/event/builder/struct.EventBuilder.html
- 5: https://docs.rs/crate/nostr-sdk/latest/source/CHANGELOG.md
- 6: http://rust-nostr.org/sdk/event/builder.html
Fix missing await and incorrect gift_wrap_from_seal signature.
The call to EventBuilder::gift_wrap_from_seal is missing the .await keyword (it is an async method in nostr-sdk 0.44) and the required signer parameter. The correct signature is gift_wrap_from_seal(signer, recipient_pubkey, seal).await?.
Update line 30-32:
let gift_wrap =
EventBuilder::gift_wrap_from_seal(sender_keys, recipient_pubkey, &seal, [])
.await
.map_err(|e| anyhow!("NIP-59 gift_wrap failed: {e}"))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/src/nostr/gift_wrap.rs` around lines 14 - 35, The call to
EventBuilder::gift_wrap_from_seal in the wrap function is using the wrong
signature and missing an await; change the invocation to pass the signer
(sender_keys) as the first argument, keep recipient_pubkey and seal as
subsequent args, and append .await before map_err so the async method is awaited
(i.e., call EventBuilder::gift_wrap_from_seal(sender_keys, recipient_pubkey,
&seal, ...).await and map the error with anyhow!("NIP-59 gift_wrap failed:
{e}")).
- sqlite: update_queued_message_status now fetches, deserializes, updates, and reserializes the full QueuedMessage so the data JSON blob stays in sync with the status column - relay_pool: broadcast relay_tx/conn_tx on add/remove relay state changes - relay_pool: spawn background polling task that maps SDK RelayStatus to internal RelayStatus every 5s and emits broadcasts on transitions - relay_pool: add TODO in subscribe_order_and_dm_feeds for event loop (Phase 3) - types: add f64 precision doc comments on fiat/premium fields of OrderInfo - db/mod: document async_fn_in_trait Send caveat on Storage trait Not changed: gift_wrap::wrap — gift_wrap_from_seal(receiver, seal, extra_tags) is a sync function; the review suggestion (adding sender_keys + .await) does not match the actual nostr-sdk 0.44 API signature.
1 participant
Summary by CodeRabbit
New Features
Tests