Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors NIP-59 Gift Wrap creation in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RumorBuilder
participant Signer
participant GiftWrapHelper
participant NIP44
participant PoWMiner
participant Recipient
User->>RumorBuilder: build rumor (no .pow)
RumorBuilder->>Signer: sign rumor -> produces Seal (signed event)
Signer->>GiftWrapHelper: call with receiver, Seal, extra_tags, pow
GiftWrapHelper->>GiftWrapHelper: validate event is Seal
GiftWrapHelper->>GiftWrapHelper: generate ephemeral keypair
GiftWrapHelper->>NIP44: encrypt Seal JSON for Recipient with ephemeral pubkey
NIP44-->>GiftWrapHelper: encrypted payload
GiftWrapHelper->>GiftWrapHelper: merge extra_tags + append p tag, tweak created_at
GiftWrapHelper->>PoWMiner: mine PoW on outer Gift Wrap (pow difficulty)
PoWMiner-->>GiftWrapHelper: mined event with nonce
GiftWrapHelper->>GiftWrapHelper: sign Gift Wrap with ephemeral keys
GiftWrapHelper-->>User: return outer Gift Wrap event (signed, encrypted)
User->>Recipient: deliver Gift Wrap
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/misc.rs (1)
13-23:⚠️ Potential issue | 🟠 MajorAdd backward-compatible migration/fallback for the directory rename.
Line 13 switches storage to
~/.mcli, but there is no migration from the previous directory. Sincesrc/db.rs(Line 14) immediately derivesmcli.dbfrom this path, existing users can silently start using a new empty DB location.Consider checking the legacy directory first and migrating (or reusing) it before creating
~/.mcli.Suggested patch
pub fn get_mcli_path() -> String { let home_dir = dirs::home_dir().expect("Couldn't get home directory"); - let mcli_path = format!("{}/.mcli", home_dir.display()); + let mcli_path = format!("{}/.mcli", home_dir.display()); + let legacy_path = format!("{}/.mcliUserA", home_dir.display()); + + // Backward compatibility: if old dir exists and new dir does not, migrate. + if Path::new(&legacy_path).exists() && !Path::new(&mcli_path).exists() { + if let Err(e) = fs::rename(&legacy_path, &mcli_path) { + panic!("Couldn't migrate mostro-cli directory from legacy path: {}", e); + } + } + if !Path::new(&mcli_path).exists() { match fs::create_dir(&mcli_path) { Ok(_) => println!("Directory {} created.", mcli_path), Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { // Directory was created by another thread/process, which is fine } Err(e) => panic!("Couldn't create mostro-cli directory in HOME: {}", e), } } mcli_path }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/misc.rs` around lines 13 - 23, The code unconditionally creates ~/.mcli without checking the old location, causing existing users to get a new empty DB; update the logic around the mcli_path variable to first detect a legacy directory (e.g., the previous dot directory used before the rename, such as ~/.mostro-cli), and if that legacy path exists either set mcli_path to that legacy path or atomically migrate it by calling fs::rename(legacy_path, &mcli_path) and handle errors (AlreadyExists, NotFound) appropriately before attempting fs::create_dir(&mcli_path); keep using Path::new, match on fs::rename/fs::create_dir errors, and ensure callers that derive mcli.db from mcli_path continue to work with the migrated/reused location.
🤖 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/messaging.rs`:
- Around line 162-197: Update send_gift_wrap_dm_internal to create the
relay-visible Gift Wrap using gift_wrap_from_seal_with_pow instead of applying
PoW to the inner seal: locate send_gift_wrap_dm_internal and replace the current
flow that mines/signs the inner Seal with a call to
gift_wrap_from_seal_with_pow(receiver, &seal_event, extra_tags, pow), propagate
the pow argument (or determine appropriate pow) and handle its Result
(map/return the error) exactly like create_gift_wrap_event does, and remove any
leftover PoW/mining/ephemeral-key signing of the inner seal so only the outer
Gift Wrap is mined/signed.
---
Outside diff comments:
In `@src/util/misc.rs`:
- Around line 13-23: The code unconditionally creates ~/.mcli without checking
the old location, causing existing users to get a new empty DB; update the logic
around the mcli_path variable to first detect a legacy directory (e.g., the
previous dot directory used before the rename, such as ~/.mostro-cli), and if
that legacy path exists either set mcli_path to that legacy path or atomically
migrate it by calling fs::rename(legacy_path, &mcli_path) and handle errors
(AlreadyExists, NotFound) appropriately before attempting
fs::create_dir(&mcli_path); keep using Path::new, match on
fs::rename/fs::create_dir errors, and ensure callers that derive mcli.db from
mcli_path continue to work with the migrated/reused location.
🪄 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: 128212ca-dbff-4231-9229-c521036abf2b
📒 Files selected for processing (2)
src/util/messaging.rssrc/util/misc.rs
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
The fix is moving in the right direction — applying PoW to the Gift Wrap (kind 1059) instead of the rumor is correct per NIP-59. However there are a few issues that need to be addressed before merging.
P1: Vague error message in gift_wrap_from_seal_with_pow
return Err(anyhow::anyhow!("Invalid kind"));This gives no useful context when debugging. Please include the actual kind received:
return Err(anyhow::anyhow!(
"Expected Seal (kind {}), got kind {}",
nostr_sdk::Kind::Seal.as_u16(),
seal.kind.as_u16()
));P1: ~/.mcliUserA → ~/.mcli path change without migration or documentation
Changing the config directory path silently discards any existing data a user has in ~/.mcliUserA. If this was intentional (e.g. the old path was only used in tests), please document it explicitly in the PR description or add at least a warning in the code. If users might have real data there, a migration step is needed.
P2: No tests for gift_wrap_from_seal_with_pow
The original bug (PoW applied to the rumor instead of the wrap) had no test coverage, and the fix does not add any either. At minimum, a unit test verifying:
- the output event has kind 1059 (GiftWrap)
- the event ID satisfies the requested PoW difficulty
- a non-Seal input is correctly rejected
would prevent regressions.
Info: review signer_keys resolution in create_gift_wrap_event
The function resolves signer_keys from optional identity_keys, which can differ from trade_keys. Please confirm explicitly that the Seal is always signed with the correct resolved key in all code paths after the refactor.
This was caused by a wrong push, mcliUserA, consider .mcli the correct path. |
Yes signer of seal per nip59 of nostr is always the identity keys |
|
Mostronator review... PR #161 — Aprobado ✅ Todo lo que pedí en la review anterior fue resuelto: • Error descriptivo: el mensaje de error ya incluye el kind esperado y el recibido La corrección de fondo es correcta: el PoW se aplica al Gift Wrap externo (kind 1059) como exige NIP-59, no al rumor. Pueden mergear. Nota: la GitHub App no tiene permisos de approve en este repo, así que no pude hacer el approve formal desde GitHub. Alguien con acceso tendrá que hacerlo manualmente. |
|
Review update from Mostronator: all previously requested changes have been addressed, and from a code review perspective this PR is approved. The PoW is now correctly applied to the outer Gift Wrap (kind 1059), tests were added, CI is green, and both production paths use |
3 participants
Summary
fixed wrong creation of giftwrap with pow, we need to create giftwrap event Id with pow not rumor.
Summary by CodeRabbit
Improvements
Chores
Tests