feat: added correct pow management#51
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 34 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoved the static Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/key_handler/user_handlers.rs (1)
53-62:⚠️ Potential issue | 🟡 MinorFix clippy error: too many arguments (8/7).
The CI pipeline is failing because
execute_take_order_actionnow has 8 parameters, exceeding clippy's default limit of 7. Since this function needs all these parameters and follows the established pattern (similar functions likespawn_take_order_taskinasync_tasks.rsalready use#[allow(clippy::too_many_arguments)]), add the attribute to suppress this warning.🔧 Proposed fix
/// Execute taking an order. /// /// This avoids code duplication between Enter key and 'y' key handlers. /// Validates the take_state, sets the UI mode to waiting, and spawns an async task to take the order. +#[allow(clippy::too_many_arguments)] pub(crate) fn execute_take_order_action( app: &mut AppState, take_state: TakeOrderState,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/user_handlers.rs` around lines 53 - 62, Add a clippy allow attribute to suppress the "too many arguments" lint for the execute_take_order_action function: place #[allow(clippy::too_many_arguments)] immediately above the pub(crate) fn execute_take_order_action(...) definition (consistent with how spawn_take_order_task in async_tasks.rs is handled) so the function can keep its eight parameters without triggering the lint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/util/dm_utils/dm_helpers.rs`:
- Around line 16-44: The function gift_wrap_from_seal_with_pow has formatting
inconsistencies (signature/braces, chained call layout, and multi-line result
formatting) that CI flags; fix by running rustfmt (cargo fmt --all) and ensure
the EventBuilder chain
(EventBuilder::new(...).tags(...).custom_created_at(...).pow(pow).sign_with_keys(...).map_err(...))
is formatted as a single consistent chain and the function signature/closing
brace align with project style, leaving the added .pow(pow) and the error
mapping in place.
In `@src/util/dm_utils/mod.rs`:
- Line 33: The module fails cargo fmt --check; run rustfmt to fix formatting for
src/util/dm_utils/mod.rs (the touched import line referencing
nostr_pow_from_instance and MostroInstanceInfo). Run cargo fmt --all (or
rustfmt) and commit the reformatted file so the use statement and module
formatting pass CI; ensure the import line in mod.rs is updated by the formatter
before merging.
In `@src/util/mostro_info.rs`:
- Around line 317-323: The test currently constructs a MostroInstanceInfo with
MostroInstanceInfo::default() and then reassigns the pow field, which triggers
clippy::field_reassign_with_default; instead, create each instance with an
explicit struct literal setting pow (e.g. MostroInstanceInfo { pow: Some(0),
..Default::default() }) for the zero, eight, and u32::MAX cases (or create three
separate instances like info0, info8, info_max) and call
nostr_pow_from_instance(Some(&infoX)) asserting the expected values (use u8::MAX
expectation for the u32::MAX case) to avoid reassigning fields after default
construction.
---
Outside diff comments:
In `@src/ui/key_handler/user_handlers.rs`:
- Around line 53-62: Add a clippy allow attribute to suppress the "too many
arguments" lint for the execute_take_order_action function: place
#[allow(clippy::too_many_arguments)] immediately above the pub(crate) fn
execute_take_order_action(...) definition (consistent with how
spawn_take_order_task in async_tasks.rs is handled) so the function can keep its
eight parameters without triggering the lint.
🪄 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: 25190c44-ac94-4578-ad26-3d3a9157bc92
📒 Files selected for processing (26)
README.mddocs/STARTUP_AND_CONFIG.mdsettings.tomlsrc/settings.rssrc/ui/key_handler/admin_handlers.rssrc/ui/key_handler/async_tasks.rssrc/ui/key_handler/confirmation.rssrc/ui/key_handler/enter_handlers.rssrc/ui/key_handler/input_helpers.rssrc/ui/key_handler/message_handlers.rssrc/ui/key_handler/mod.rssrc/ui/key_handler/user_handlers.rssrc/util/chat_utils.rssrc/util/dm_utils/dm_helpers.rssrc/util/dm_utils/mod.rssrc/util/mod.rssrc/util/mostro_info.rssrc/util/order_utils/execute_add_invoice.rssrc/util/order_utils/execute_admin_add_solver.rssrc/util/order_utils/execute_admin_cancel.rssrc/util/order_utils/execute_admin_settle.rssrc/util/order_utils/execute_finalize_dispute.rssrc/util/order_utils/execute_send_msg.rssrc/util/order_utils/execute_take_dispute.rssrc/util/order_utils/send_new_order.rssrc/util/order_utils/take_order.rs
💤 Files with no reviewable changes (3)
- settings.toml
- README.md
- src/settings.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/ui/key_handler/user_handlers.rs (1)
53-63: Consider bundling infrastructure dependencies into a context struct.The 8-parameter signature warrants the
#[allow(clippy::too_many_arguments)]suppression, but the infrastructure-related arguments (pool, client, mostro_pubkey, order_result_tx, dm_subscription_tx, mostro_info) are functionally cohesive and could be consolidated into a single context struct. This would eliminate the suppression, simplify both the function signature and its two call sites, and reduce coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/key_handler/user_handlers.rs` around lines 53 - 63, The function execute_take_order_action has too many infra arguments; create a single context struct (e.g., TakeOrderContext or ExecuteTakeOrderCtx) that contains: pool: SqlitePool, client: Client, mostro_pubkey: nostr_sdk::PublicKey, order_result_tx: UnboundedSender<crate::ui::OperationResult>, dm_subscription_tx: UnboundedSender<crate::util::OrderDmSubscriptionCmd>, and mostro_info: Option<crate::util::MostroInstanceInfo>; replace the multiple parameters in execute_take_order_action with one ctx: TakeOrderContext (keep app: &mut AppState and take_state: TakeOrderState), update all call sites to construct and pass the context, ensure the context derives/uses Clone or is wrapped in Arc if needed for ownership, and then remove the #[allow(clippy::too_many_arguments)] on execute_take_order_action.src/util/dm_utils/dm_helpers.rs (1)
16-45: Remove the review-note comments before merge.Line 16, Line 23, Line 25, and Line 43 read like reviewer guidance rather than source-facing docs. They add noise without clarifying the helper’s contract. A short doc comment explaining why this wrapper exists would be cleaner.
✂️ Suggested cleanup
-// nip59::RANGE_RANDOM_TIMESTAMP_TWEAK — you already use `nip59::` elsewhere in the project +/// Mirrors the SDK gift-wrap path, but applies PoW to the outer Gift Wrap +/// before signing so relays that require NIP-13 accept the published event. fn gift_wrap_from_seal_with_pow( receiver: &PublicKey, seal: &Event, extra_tags: impl IntoIterator<Item = Tag>, pow: u8, ) -> Result<Event> { - // or map to anyhow in create_gift_wrap_event if seal.kind != nostr_sdk::Kind::Seal { - // same validation as upstream, or map_err to anyhow return Err(anyhow::anyhow!("Invalid kind")); } @@ EventBuilder::new(nostr_sdk::Kind::GiftWrap, content) .tags(tags) .custom_created_at(Timestamp::tweaked(nip59::RANGE_RANDOM_TIMESTAMP_TWEAK)) - .pow(pow) // <-- this is what the SDK’s gift_wrap path does NOT do + .pow(pow) .sign_with_keys(&ephem) .map_err(|e| anyhow::anyhow!("Failed to sign gift wrap: {e}")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/dm_utils/dm_helpers.rs` around lines 16 - 45, Remove the inline reviewer-note comments inside the gift_wrap_from_seal_with_pow helper and replace them with a concise doc comment that explains the purpose and contract of the function (why this wrapper exists, what it validates e.g., ensures seal.kind == nostr_sdk::Kind::Seal, and that it encrypts and signs using an ephemeral key), while leaving the implementation (use of nip59::RANGE_RANDOM_TIMESTAMP_TWEAK, constructing content via nip44::encrypt, pushing Tag::public_key(*receiver), setting custom_created_at on EventBuilder, and signing with ephem and .pow(pow)) unchanged; specifically delete the reviewer guidance lines referencing nip59, mapping errors/validation, and the SDK note, and add a short /// doc comment above gift_wrap_from_seal_with_pow describing intent and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/key_handler/user_handlers.rs`:
- Around line 53-63: The function execute_take_order_action has too many infra
arguments; create a single context struct (e.g., TakeOrderContext or
ExecuteTakeOrderCtx) that contains: pool: SqlitePool, client: Client,
mostro_pubkey: nostr_sdk::PublicKey, order_result_tx:
UnboundedSender<crate::ui::OperationResult>, dm_subscription_tx:
UnboundedSender<crate::util::OrderDmSubscriptionCmd>, and mostro_info:
Option<crate::util::MostroInstanceInfo>; replace the multiple parameters in
execute_take_order_action with one ctx: TakeOrderContext (keep app: &mut
AppState and take_state: TakeOrderState), update all call sites to construct and
pass the context, ensure the context derives/uses Clone or is wrapped in Arc if
needed for ownership, and then remove the #[allow(clippy::too_many_arguments)]
on execute_take_order_action.
In `@src/util/dm_utils/dm_helpers.rs`:
- Around line 16-45: Remove the inline reviewer-note comments inside the
gift_wrap_from_seal_with_pow helper and replace them with a concise doc comment
that explains the purpose and contract of the function (why this wrapper exists,
what it validates e.g., ensures seal.kind == nostr_sdk::Kind::Seal, and that it
encrypts and signs using an ephemeral key), while leaving the implementation
(use of nip59::RANGE_RANDOM_TIMESTAMP_TWEAK, constructing content via
nip44::encrypt, pushing Tag::public_key(*receiver), setting custom_created_at on
EventBuilder, and signing with ephem and .pow(pow)) unchanged; specifically
delete the reviewer guidance lines referencing nip59, mapping errors/validation,
and the SDK note, and add a short /// doc comment above
gift_wrap_from_seal_with_pow describing intent and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c68f9528-9f0b-4b4e-970c-ecb9f81588cc
📒 Files selected for processing (8)
docs/MESSAGE_FLOW_AND_PROTOCOL.mddocs/POW_AND_OUTBOUND_EVENTS.mddocs/README.mddocs/SETTINGS_ANALYSIS.mdsrc/ui/key_handler/user_handlers.rssrc/util/dm_utils/dm_helpers.rssrc/util/dm_utils/mod.rssrc/util/mostro_info.rs
✅ Files skipped from review due to trivial changes (4)
- docs/README.md
- docs/SETTINGS_ANALYSIS.md
- docs/MESSAGE_FLOW_AND_PROTOCOL.md
- docs/POW_AND_OUTBOUND_EVENTS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/util/dm_utils/mod.rs
- src/util/mostro_info.rs
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review completed on latest head (9af9daa).
I validated the architectural change from static settings pow to dynamic PoW resolution from Mostro instance info (kind 38385), including full propagation through user/admin order flows and admin shared-key chat wrapping.
I also verified the custom gift-wrap path now applies .pow(pow) on the outer GiftWrap event and keeps NIP-59 timestamp tweaking intact, which fixes the key protocol gap this PR targets.
Local verification passed on this branch: cargo fmt --all -- --check, cargo clippy --all-targets --all-features -- -D warnings, and cargo test.
No blocking issues found.
1 participant
Summary
powfromsettings.toml/Settings. NIP-13 difficulty for published events comes from the Mostro instance status event (kind 38385, tagpow), vianostr_pow_from_instance(src/util/mostro_info.rs).AppState.mostro_infois passed throughEnterKeyContextand intosend_dm, all order/dispute/admin paths that publish to Mostro, and admin dispute chat (chat_utils/input_helpers).EventBuilder::gift_wrapin rust-nostr does not apply PoW to the outer kind 1059 event. We build the seal withEventBuilder::seal+sign, then wrap with a localgift_wrap_from_seal_with_powthat mirrors the SDK’sgift_wrap_from_sealbut adds.pow(pow)before signing the published Gift Wrap so instances that require PoW accept the event.Breaking / migration
powremoved from settings: Existingsettings.tomlfiles may still containpow = …; serde ignores unknown keys. Users should rely on instance info (Mostro Info tab / startup fetch).Docs
docs/STARTUP_AND_CONFIG.mdupdated to describe PoW from 38385, not settings.Summary by CodeRabbit
Configuration
powsetting was removed from settings.toml and UI-generated defaults.Behavior
Documentation