refactor: Make ADMIN_NSEC optional and rename from NSEC_PRIVKEY #149
Conversation
|
Warning Rate limit exceeded@AndreaDiazCorreia has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 11 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughContext.context_keys was made optional (Option); ADMIN_NSEC is loaded only for admin commands. Admin-related call sites now guard for missing admin keys and use as_ref() where needed. README and .env-sample were updated with env var guidance and formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Init
participant Checker as is_admin_command()
participant Env as Env (ADMIN_NSEC)
participant Context as Context{context_keys}
participant Handler as Admin Handler
CLI->>Checker: Is command admin?
alt admin
Checker-->>CLI: true
CLI->>Env: read ADMIN_NSEC
Env-->>Context: Some(Keys)
else non-admin
Checker-->>CLI: false
CLI->>Context: None
end
Context->>Handler: execute command with Optional Keys
alt Keys present
Handler->>Handler: use admin_keys (as_ref)
Handler-->>CLI: success
else Keys missing
Handler-->>CLI: error ("admin keys not available")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (1)
src/cli/take_dispute.rs (1)
66-68: Minor: Inconsistent logging label.Line 68 uses "Admin PubKey" while lines 102 and 139 use "Admin Keys". Consider using consistent terminology across all admin functions for better UX.
Apply this diff for consistency:
- println!("🔑 Admin PubKey: {}", admin_keys.public_key); + println!("🔑 Admin Keys: {}", admin_keys.public_key);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.env-sample(1 hunks)README.md(1 hunks)src/cli.rs(3 hunks)src/cli/adm_send_dm.rs(2 hunks)src/cli/take_dispute.rs(4 hunks)src/util/events.rs(1 hunks)src/util/storage.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli.rs:301-306
Timestamp: 2025-09-12T20:02:14.291Z
Learning: The mostro-cli is designed as a test client that allows users to test both regular user operations and admin operations from the terminal. Therefore, MOSTRO_PUBKEY should always be required regardless of whether NSEC_PRIVKEY is present, as both user and admin operations need to know which Mostro instance to interact with.
📚 Learning: 2025-09-12T20:02:14.291Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli.rs:301-306
Timestamp: 2025-09-12T20:02:14.291Z
Learning: The mostro-cli is designed as a test client that allows users to test both regular user operations and admin operations from the terminal. Therefore, MOSTRO_PUBKEY should always be required regardless of whether NSEC_PRIVKEY is present, as both user and admin operations need to know which Mostro instance to interact with.
Applied to files:
.env-sampleREADME.mdsrc/cli.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/take_dispute.rssrc/cli/adm_send_dm.rssrc/util/storage.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/take_dispute.rssrc/cli/adm_send_dm.rssrc/util/storage.rs
📚 Learning: 2025-10-18T11:48:11.944Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 146
File: src/cli/dm_to_user.rs:25-32
Timestamp: 2025-10-18T11:48:11.944Z
Learning: In mostro-cli's DM functionality (dm_to_user.rs and similar), message content consists of ephemeral public keys that change every time and contains no sensitive personal data. Logging message content to stdout is acceptable and not a privacy concern.
Applied to files:
src/cli/take_dispute.rssrc/cli/adm_send_dm.rssrc/util/storage.rs
📚 Learning: 2025-09-13T10:31:42.313Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/get_dm.rs:24-29
Timestamp: 2025-09-13T10:31:42.313Z
Learning: In mostro-cli's get_dm.rs, the message filtering is intentional: non-admin users should only receive DirectMessage from users, while messages from Mostro are only fetched when the admin flag is true. Don't include GiftWrap messages for non-admin users.
Applied to files:
src/cli/take_dispute.rssrc/cli/adm_send_dm.rssrc/util/events.rssrc/util/storage.rs
📚 Learning: 2025-10-18T09:38:00.449Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 146
File: src/cli/restore.rs:61-66
Timestamp: 2025-10-18T09:38:00.449Z
Learning: In src/cli/restore.rs, the restore session request to Mostro will only receive GiftWrap messages as responses, not PrivateDirectMessage. The wait_for_dm subscription filtering for GiftWrap only is intentional and correct for this endpoint.
Applied to files:
src/cli/take_dispute.rssrc/cli/adm_send_dm.rs
📚 Learning: 2025-09-09T19:07:29.834Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/add_invoice.rs:57-79
Timestamp: 2025-09-09T19:07:29.834Z
Learning: In Nostr direct message flows, explicit subscriptions before sending DMs are not needed because the server will route responses using the correct key, and wait_for_dm likely handles subscription logic internally.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter through manual filtering after fetching events: it uses a hardcoded 2880-minute window in the Nostr filter, then manually filters results with `if event.created_at.as_u64() < since as u64 { continue; }`.
Applied to files:
src/util/events.rs
📚 Learning: 2025-09-13T20:40:51.992Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/get_dm_user.rs:38-40
Timestamp: 2025-09-13T20:40:51.992Z
Learning: The get_direct_messages_from_trade_keys function in src/util.rs already handles the `since` parameter properly by creating filters with `since(Timestamp::from(since))`, so no additional local filtering is needed in CLI functions that call it.
Applied to files:
src/util/events.rs
📚 Learning: 2025-09-13T10:31:42.313Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/get_dm.rs:24-29
Timestamp: 2025-09-13T10:31:42.313Z
Learning: In mostro-cli, index 0 represents the master key and should be skipped when iterating through trade keys. The loop should start from index 1 when fetching user trade keys in get_dm.rs.
Applied to files:
src/cli.rs
🧬 Code graph analysis (5)
src/cli/take_dispute.rs (2)
src/util/messaging.rs (2)
send_dm(186-238)wait_for_dm(69-112)src/parser/dms.rs (1)
parse_dm_events(744-839)
src/cli/adm_send_dm.rs (2)
src/parser/common.rs (3)
create_standard_table(49-55)create_field_value_header(58-67)create_emoji_field_row(94-99)src/util/messaging.rs (1)
send_admin_gift_wrap_dm(14-21)
src/util/events.rs (1)
src/parser/dms.rs (1)
parse_dm_events(744-839)
src/cli.rs (1)
src/util/messaging.rs (2)
var(199-201)None(51-51)
src/util/storage.rs (1)
src/util/messaging.rs (1)
send_dm(186-238)
🔇 Additional comments (15)
.env-sample (1)
5-8: LGTM! Clear documentation for optional admin key.The comment clearly indicates that ADMIN_NSEC is only required for admin commands, which aligns with the implementation changes in the codebase.
README.md (2)
23-23: LGTM! Documentation accurately reflects the changes.The documentation now clearly states that the admin private key is only needed for admin commands, which matches the implementation.
29-30: LGTM! Clear setup instructions.The instructions clearly guide users to set ADMIN_NSEC only for admin commands, making the optional nature of the admin key explicit.
src/cli.rs (3)
57-57: LGTM! Type change enables conditional admin key loading.Changing
context_keysfromKeystoOption<Keys>is the correct approach for making admin credentials optional. This aligns with the PR objective of requiring admin keys only for admin-specific commands.
394-405: LGTM! Clear and correct conditional loading logic.The conditional loading of admin keys is well-implemented:
- Only loads ADMIN_NSEC when
is_admin_commandreturns true- Provides clear error message indicating the requirement for admin commands
- Returns None for non-admin commands, allowing regular users to run the CLI without admin credentials
427-438: Function is complete; no missing admin commands.Verification confirms all admin command variants from the Commands enum (7 total: AdmCancel, AdmSettle, AdmListDisputes, AdmAddSolver, AdmTakeDispute, AdmSendDm, and GetAdminDm) are correctly included in the
is_admin_commandfunction. The code is accurate and complete.src/util/storage.rs (2)
47-48: LGTM! Proper guard for admin key availability.The admin key retrieval correctly:
- Uses
as_ref()to avoid moving the value- Provides a clear error message when admin keys are not available
- Aligns with the pattern used throughout the codebase
51-51: LGTM! Correct passing of admin keys.The change from
Some(&ctx.context_keys)toSome(admin_keys)correctly adapts to the new optional admin key structure.src/cli/adm_send_dm.rs (3)
10-11: LGTM! Consistent admin key guard.The admin key retrieval follows the same pattern used throughout the codebase, with a clear error message.
20-20: LGTM! Correct usage of admin keys.The change from
ctx.context_keys.public_key()toadmin_keys.public_key()correctly uses the locally retrieved admin keys.
31-31: LGTM! Correct parameter passing.The change correctly passes
admin_keys(which is&Keys) to the function, adapting to the new optional admin key structure.src/util/events.rs (2)
90-92: LGTM! Correct admin key retrieval for admin DM path.The admin key retrieval is correctly scoped to the
DirectMessagesAdminpath and uses a consistent error message.
98-98: LGTM! Correct parameter passing.The change correctly passes
admin_keystoparse_dm_events, adapting to the new optional admin key structure.src/cli/take_dispute.rs (2)
100-102: LGTM! Proper admin key guard.The admin key retrieval is consistent with other functions and provides clear error messaging.
137-156: LGTM! Correct admin key usage throughout the dispute flow.The changes correctly:
- Retrieve admin keys with proper error handling
- Pass admin keys to
send_dm,wait_for_dm, andparse_dm_events- Maintain the expected dispute handling flow
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/cli/take_dispute.rs (1)
12-41: Add consistent admin keys validation for better error messages.Unlike the other admin commands in this file (
execute_admin_cancel_dispute,execute_admin_settle_dispute,execute_take_dispute), this function doesn't explicitly validate that admin keys are available before proceeding. Whileadmin_send_dmwill eventually fail if admin keys are missing, adding an explicit check here would provide a clearer error message and maintain consistency across all admin commands.Apply this diff to add the admin keys check:
pub async fn execute_admin_add_solver(npubkey: &str, ctx: &Context) -> Result<()> { println!("👑 Admin Add Solver"); println!("═══════════════════════════════════════"); let mut table = create_standard_table(); table.set_header(create_field_value_header()); table.add_row(create_emoji_field_row("🔑 ", "Solver PubKey", npubkey)); table.add_row(create_emoji_field_row( "🎯 ", "Mostro PubKey", &ctx.mostro_pubkey.to_string(), )); println!("{table}"); println!("💡 Adding new solver to Mostro...\n"); + + let admin_keys = ctx.context_keys.as_ref() + .ok_or_else(|| anyhow::anyhow!("Admin keys not available. ADMIN_NSEC must be set for admin commands."))?; + println!("🔑 Admin Keys: {}", admin_keys.public_key()); + // Create takebuy message let take_dispute_message = Message::new_dispute(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/take_dispute.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli.rs:301-306
Timestamp: 2025-09-12T20:02:14.291Z
Learning: The mostro-cli is designed as a test client that allows users to test both regular user operations and admin operations from the terminal. Therefore, MOSTRO_PUBKEY should always be required regardless of whether NSEC_PRIVKEY is present, as both user and admin operations need to know which Mostro instance to interact with.
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-10-18T11:48:11.944Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 146
File: src/cli/dm_to_user.rs:25-32
Timestamp: 2025-10-18T11:48:11.944Z
Learning: In mostro-cli's DM functionality (dm_to_user.rs and similar), message content consists of ephemeral public keys that change every time and contains no sensitive personal data. Logging message content to stdout is acceptable and not a privacy concern.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-09-13T10:31:42.313Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/get_dm.rs:24-29
Timestamp: 2025-09-13T10:31:42.313Z
Learning: In mostro-cli's get_dm.rs, the message filtering is intentional: non-admin users should only receive DirectMessage from users, while messages from Mostro are only fetched when the admin flag is true. Don't include GiftWrap messages for non-admin users.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-10-18T09:38:00.449Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 146
File: src/cli/restore.rs:61-66
Timestamp: 2025-10-18T09:38:00.449Z
Learning: In src/cli/restore.rs, the restore session request to Mostro will only receive GiftWrap messages as responses, not PrivateDirectMessage. The wait_for_dm subscription filtering for GiftWrap only is intentional and correct for this endpoint.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-09-09T19:07:29.834Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/add_invoice.rs:57-79
Timestamp: 2025-09-09T19:07:29.834Z
Learning: In Nostr direct message flows, explicit subscriptions before sending DMs are not needed because the server will route responses using the correct key, and wait_for_dm likely handles subscription logic internally.
Applied to files:
src/cli/take_dispute.rs
🧬 Code graph analysis (1)
src/cli/take_dispute.rs (2)
src/util/messaging.rs (2)
send_dm(186-238)wait_for_dm(69-112)src/parser/dms.rs (1)
parse_dm_events(744-839)
🔇 Additional comments (4)
src/cli/take_dispute.rs (4)
144-144: LGTM: Correct propagation of admin keys through DM operations.The admin keys are correctly passed to the DM operation chain:
send_dmreceivesSome(admin_keys)for theidentity_keysparameterwait_for_dmreceivesSome(admin_keys)for theorder_trade_keysparameterparse_dm_eventsreceivesadmin_keysfor thepubkeyparameterAll parameter types match their function signatures (
Option<&Keys>or&Keysas expected).Also applies to: 153-153, 156-156
137-139: Fix method call syntax forpublic_key().Line 139 is missing parentheses when calling the
public_key()method. In the nostr_sdk crate,public_key()is a method, not a field, so it requires parentheses.Apply this diff to fix the syntax error:
let admin_keys = ctx.context_keys.as_ref() .ok_or_else(|| anyhow::anyhow!("Admin keys not available. ADMIN_NSEC must be set for admin commands."))?; - println!("🔑 Admin Keys: {}", admin_keys.public_key); + println!("🔑 Admin Keys: {}", admin_keys.public_key());⛔ Skipped due to learnings
Learnt from: arkanoider Repo: MostroP2P/mostro-cli PR: 135 File: src/util.rs:614-619 Timestamp: 2025-09-14T14:01:01.728Z Learning: In the mostro-cli codebase, the user arkanoider indicated that accessing `public_key` without parentheses (as a field) is valid, even though most of the codebase uses `public_key()` method calls. This suggests there may be `Deref` implementation or other Rust features that allow both syntaxes to work.
66-68: Fix method call syntax forpublic_key().Line 68 is missing parentheses when calling the
public_key()method. In the nostr_sdk crate,public_key()is a method, not a field, so it requires parentheses.Apply this diff to fix the syntax error:
let admin_keys = ctx.context_keys.as_ref() .ok_or_else(|| anyhow::anyhow!("Admin keys not available. ADMIN_NSEC must be set for admin commands."))?; - println!("🔑 Admin Keys: {}", admin_keys.public_key); + println!("🔑 Admin Keys: {}", admin_keys.public_key());⛔ Skipped due to learnings
Learnt from: arkanoider Repo: MostroP2P/mostro-cli PR: 135 File: src/util.rs:614-619 Timestamp: 2025-09-14T14:01:01.728Z Learning: In the mostro-cli codebase, the user arkanoider indicated that accessing `public_key` without parentheses (as a field) is valid, even though most of the codebase uses `public_key()` method calls. This suggests there may be `Deref` implementation or other Rust features that allow both syntaxes to work.
100-102: Fix method call syntax forpublic_key().Line 102 is missing parentheses when calling the
public_key()method. In the nostr_sdk crate,public_key()is a method, not a field, so it requires parentheses.Apply this diff to fix the syntax error:
let admin_keys = ctx.context_keys.as_ref() .ok_or_else(|| anyhow::anyhow!("Admin keys not available. ADMIN_NSEC must be set for admin commands."))?; - println!("🔑 Admin Keys: {}", admin_keys.public_key); + println!("🔑 Admin Keys: {}", admin_keys.public_key()); admin_send_dm(ctx, take_dispute_message).await?;⛔ Skipped due to learnings
Learnt from: arkanoider Repo: MostroP2P/mostro-cli PR: 135 File: src/util.rs:614-619 Timestamp: 2025-09-14T14:01:01.728Z Learning: In the mostro-cli codebase, the user arkanoider indicated that accessing `public_key` without parentheses (as a field) is valid, even though most of the codebase uses `public_key()` method calls. This suggests there may be `Deref` implementation or other Rust features that allow both syntaxes to work.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/cli/take_dispute.rs (2)
26-28: Consider extracting the admin key retrieval pattern to reduce duplication.The same pattern for retrieving admin keys and logging the public key is repeated across all four admin command functions. Consider extracting this into a helper function to improve maintainability.
Example:
fn get_admin_keys(ctx: &Context) -> Result<&Keys> { let admin_keys = ctx.context_keys.as_ref() .ok_or_else(|| anyhow::anyhow!("Admin keys not available. ADMIN_NSEC must be set for admin commands."))?; println!("🔑 Admin Keys: {}", admin_keys.public_key); Ok(admin_keys) }Then replace the duplicated blocks with:
let admin_keys = get_admin_keys(ctx)?;Also applies to: 71-73, 105-107, 142-144
28-28: Consider if logging admin public keys is necessary.All admin command functions now log the admin public key to stdout. While public keys are not secret, this additional logging might be unnecessary noise in production usage. Consider either removing these logs or gating them behind a verbose/debug flag.
Also applies to: 73-73, 107-107, 144-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/take_dispute.rs(5 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli.rs:301-306
Timestamp: 2025-09-12T20:02:14.291Z
Learning: The mostro-cli is designed as a test client that allows users to test both regular user operations and admin operations from the terminal. Therefore, MOSTRO_PUBKEY should always be required regardless of whether NSEC_PRIVKEY is present, as both user and admin operations need to know which Mostro instance to interact with.
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-10-18T11:48:11.944Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 146
File: src/cli/dm_to_user.rs:25-32
Timestamp: 2025-10-18T11:48:11.944Z
Learning: In mostro-cli's DM functionality (dm_to_user.rs and similar), message content consists of ephemeral public keys that change every time and contains no sensitive personal data. Logging message content to stdout is acceptable and not a privacy concern.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-09-13T10:31:42.313Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/get_dm.rs:24-29
Timestamp: 2025-09-13T10:31:42.313Z
Learning: In mostro-cli's get_dm.rs, the message filtering is intentional: non-admin users should only receive DirectMessage from users, while messages from Mostro are only fetched when the admin flag is true. Don't include GiftWrap messages for non-admin users.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-10-18T09:38:00.449Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 146
File: src/cli/restore.rs:61-66
Timestamp: 2025-10-18T09:38:00.449Z
Learning: In src/cli/restore.rs, the restore session request to Mostro will only receive GiftWrap messages as responses, not PrivateDirectMessage. The wait_for_dm subscription filtering for GiftWrap only is intentional and correct for this endpoint.
Applied to files:
src/cli/take_dispute.rs
📚 Learning: 2025-09-09T19:07:29.834Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/add_invoice.rs:57-79
Timestamp: 2025-09-09T19:07:29.834Z
Learning: In Nostr direct message flows, explicit subscriptions before sending DMs are not needed because the server will route responses using the correct key, and wait_for_dm likely handles subscription logic internally.
Applied to files:
src/cli/take_dispute.rs
🧬 Code graph analysis (1)
src/cli/take_dispute.rs (2)
src/util/messaging.rs (2)
send_dm(186-238)wait_for_dm(69-112)src/parser/dms.rs (1)
parse_dm_events(744-839)
🔇 Additional comments (2)
src/cli/take_dispute.rs (2)
142-161: LGTM! Admin key handling inexecute_take_disputeis correct.The explicit passing of admin_keys to
send_dm,wait_for_dm, andparse_dm_eventsis type-correct and properly handles the optional admin keys pattern. The changes ensure admin messages are sent and processed with the correct admin credentials.
41-41: ****The original review comment is based on a false premise. The
admin_send_dmfunction (src/util/storage.rs:46-56) properly handles optional admin keys by internally extractingctx.context_keysand validating availability with the same error handling as the calling functions. This is intentional design—the wrapper function abstracts key extraction from the context, allowing callers to pass only the context. The early validation in calling functions provides user feedback before attempting the operation, whileadmin_send_dmperforms its own validation. This pattern is consistent and correct.Likely an incorrect or invalid review comment.
|
@AndreaDiazCorreia I like it! Var name is more meaningful and logic is correct! If you can I would only use your nice helper: /// Helper function to retrieve and validate admin keys from context
fn get_admin_keys(ctx: &Context) -> Result<&Keys> {
let admin_keys = ctx.context_keys.as_ref()
.ok_or_else(|| anyhow::anyhow!("Admin keys not available. ADMIN_NSEC must be set for admin commands."))?;
// Only log admin public key in verbose mode
if std::env::var("RUST_LOG").is_ok() {
println!("🔑 Admin Keys: {}", admin_keys.public_key);
}
Ok(admin_keys)
}in all the other files that are using that check, maybe we can move the helper in |
|
Another point, probably |
|
LGTM! |
2 participants
This PR fixes an issue where the NSEC_PRIVKEY environment variable was required for all users, even when executing regular trading commands. The changes make admin credentials optional and only load them when admin commands are executed. Additionally, the variable has been renamed from NSEC_PRIVKEY to ADMIN_NSEC to better reflect its purpose.
Changes include:
context_keysoptional in the Context structSummary by CodeRabbit
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.