Skip to content

Fix for nostr sdk 40 issue on incoming message#465

Merged
grunch merged 4 commits into
developfrom
sdk-40-issue
Mar 24, 2025
Merged

Fix for nostr sdk 40 issue on incoming message#465
grunch merged 4 commits into
developfrom
sdk-40-issue

Conversation

@arkanoider

@arkanoider arkanoider commented Mar 23, 2025

Copy link
Copy Markdown
Collaborator

@grunch @Catrya ,

this is the fix that makes finally messages from mostro-cli parsed again from mostro.

This is the pull request of mostro-core to use for testing.

The only file involved in the fix is app.rs, the other files are just to align with nostr-sdk bumped version.

I tested message incoming from cli and now it's working, @Catrya when you will make some test chat me as usual for any doutbs.

Summary by CodeRabbit

Summary by CodeRabbit

  • Chores
    • Upgraded key dependencies to newer versions for improved underlying functionality.
  • Refactor
    • Enhanced message parsing by adopting standardized JSON formatting and offering more precise error reporting.
    • Revised how tags are created and events are dispatched to optimize performance and memory management.
    • Simplified subscription processing to streamline event handling overall.

@coderabbitai

coderabbitai Bot commented Mar 23, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

This pull request updates the dependency version in Cargo.toml and makes several refactorings across the codebase. The changes improve type safety and memory handling by parsing messages directly into a (Message, Option<Signature>) tuple, serializing messages via JSON, and passing event references instead of moving ownership. In addition, multiple functions now create tags using Tags::from_list instead of Tags::new, and the subscription API in main.rs has been simplified.

Changes

File(s) Change Summary
Cargo.toml Updated nostr-sdk dependency from 0.38.0 to 0.40.0 and mostro-core from 0.6.33 to 0.6.34; added a newline at the file end.
src/app.rs Refactored the run function to parse (Message, Option<Signature>) directly, use as_json() for serialization, and enhance error logging.
src/app/admin_cancel.rs
src/app/admin_settle.rs
src/app/admin_take_dispute.rs
Changed tag creation from Tags::new to Tags::from_list and modified client.send_event calls to pass event references.
src/app/dispute.rs
src/app/release.rs
Updated calls to client.send_event to pass event references instead of moving values.
src/main.rs Simplified the subscribe call by removing the vector wrapper around the subscription argument.
src/nip33.rs Replaced Tags::new with Tags::from_list in new_event, order_to_tags, and info_to_tags functions.
src/scheduler.rs Modified asynchronous job functions to send events as references for improved memory management.
src/util.rs Updated publish_order, send_dm, and update_order_event to pass event references, and changed tag instantiation in send_dm to use Tags::from_list.

Sequence Diagram(s)

sequenceDiagram
    participant E as Event Source
    participant A as App.run
    participant P as Parser
    participant S as Serializer
    participant C as Client
    E->>A: Receive event with rumor content
    A->>P: Parse message & signature
    P-->>A: Return (Message, Option<Signature>)
    A->>S: Serialize Message using as_json()
    S-->>A: JSON-formatted Message
    A->>C: Send event (&event)
    C-->>A: Acknowledge event sent
Loading
sequenceDiagram
    participant M as Main
    participant C as Client
    M->>C: subscribe(subscription, None)
    C-->>M: Confirm subscription
Loading

Suggested reviewers

  • Catrya

Poem

I'm a rabbit in this code patch land,
Hopping through functions with a swift command.
My tags now spring from a tidy list,
And events pass by ref—oh, what a twist!
With each bug chased and code refined,
I thump my paws for a codebase aligned! 🐇


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b76cb6 and e5d96b6.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • Cargo.toml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Cargo.toml

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/scheduler.rs (1)

214-214: Changed to pass event by reference but still cloning

Updated to use client.send_event(&ev.clone()) to align with the nostr-sdk 0.40.0 API changes, but it's still unnecessarily cloning the event before taking a reference.

Consider removing the redundant clone:

-let _ = client.send_event(&ev.clone()).await;
+let _ = client.send_event(ev).await;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9b99d7 and 5b76cb6.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • Cargo.toml (2 hunks)
  • src/app.rs (2 hunks)
  • src/app/admin_cancel.rs (2 hunks)
  • src/app/admin_settle.rs (2 hunks)
  • src/app/admin_take_dispute.rs (2 hunks)
  • src/app/dispute.rs (2 hunks)
  • src/app/release.rs (1 hunks)
  • src/main.rs (1 hunks)
  • src/nip33.rs (3 hunks)
  • src/scheduler.rs (3 hunks)
  • src/util.rs (4 hunks)
🔇 Additional comments (24)
Cargo.toml (1)

19-19:

Details

✅ Verification successful

Updated nostr-sdk dependency to version 0.40.0

The version bump of nostr-sdk from 0.38.0 to 0.40.0 is directly related to fixing the issue with parsing messages from mostro-cli as mentioned in the PR objectives.


🏁 Script executed:

#!/bin/bash
# Verify if the nostr-sdk API changes between versions 0.38.0 and 0.40.0 affect message parsing
rg -A 3 -B 3 "client\.send_event|client\.subscribe" --type rust

Length of output: 3482


Verified Dependency Update: nostr-sdk v0.40.0 Consistently Fixes Message Parsing
The targeted API methods (client.send_event and client.subscribe) are used consistently across multiple modules, and there’s no evidence of regression or misalignment with the new version. This bump correctly addresses the message parsing issues observed with mostro-cli.

src/app/release.rs (1)

116-116: Changed to pass event by reference

The modification changes client.send_event(event) to client.send_event(&event) to match the updated API in nostr-sdk 0.40.0. This passes a reference instead of transferring ownership of the event.

src/scheduler.rs (2)

126-126: Changed to pass relay_ev by reference

Updated to use client.send_event(&relay_ev) instead of moving ownership, aligning with the nostr-sdk 0.40.0 API changes.


157-157: Changed to pass info_ev by reference

Updated to use client.send_event(&info_ev) instead of moving ownership, aligning with the nostr-sdk 0.40.0 API changes.

src/main.rs (1)

93-93: Updated subscribe call to match new API

Changed from client.subscribe(vec![subscription], None) to client.subscribe(subscription, None) to match the updated API in nostr-sdk 0.40.0, which now expects a single Filter object rather than a vector.

src/app/dispute.rs (2)

31-47: Consistent tag creation approach applied

The change from Tags::new to Tags::from_list aligns with similar changes throughout the codebase, creating a more consistent approach to tag creation.


58-58: Improved memory handling by passing event reference

Passing the event by reference (&event) instead of by value prevents unnecessary ownership transfer, which is more efficient and follows Rust best practices.

src/app/admin_take_dispute.rs (2)

251-264: Consistent tag creation approach applied

The change from Tags::new to Tags::from_list maintains consistency with similar changes across the codebase, improving code standardization.


286-293: Improved memory handling by passing event reference

Passing the event by reference (&event) instead of by value prevents unnecessary cloning and follows Rust ownership best practices.

src/app/admin_cancel.rs (2)

86-99: Consistent tag creation approach applied

The change from Tags::new to Tags::from_list continues the pattern seen in other files, creating a more uniform API usage throughout the codebase.


106-108: Improved memory handling with reference passing

Changed to pass the event as a reference (&event) instead of by value, which is more efficient and consistent with other send_event calls in the PR.

src/app/admin_settle.rs (3)

86-99: Consistent tag creation approach applied

The change from Tags::new to Tags::from_list maintains the consistent pattern applied across all modified files in this PR.


107-109: Improved memory handling by passing event reference

The event is now passed by reference (&event) instead of by value, preventing unnecessary ownership transfer and keeping consistent with other similar calls.


1-157: Overall implementation successfully addresses the nostr sdk issue

The changes consistently update event handling across all dispute management functions, effectively addressing the issue with parsing messages from mostro-cli. The approach of passing events by reference and standardizing tag creation is applied uniformly across all affected files.

src/nip33.rs (3)

34-34: Method update: Tags::new to Tags::from_list

The change from Tags::new to Tags::from_list appears to be part of a consistent update across the codebase to fix message parsing issues between mostro-cli and mostro.


145-145: Method update: Tags::new to Tags::from_list

This change is consistent with the same pattern applied throughout the codebase.


157-157: Method update: Tags::new to Tags::from_list

This change is consistent with the same pattern applied throughout the codebase.

src/app.rs (3)

298-306: Fixed message parsing to properly handle mostro-cli messages

This change improves type safety by directly parsing the message into a structured (Message, Option<Signature>) tuple rather than using a more generic approach. This appears to be the core fix for the issue with parsing messages from mostro-cli.


308-315: Added proper message serialization for signature verification

The message is now properly serialized to JSON format before signature verification, which ensures consistent handling of messages regardless of their source.


321-321: Updated signature verification to use the properly serialized message

Now using the correctly serialized message JSON for signature verification, which ensures compatibility with messages from mostro-cli.

src/util.rs (4)

345-345: Memory optimization: Passing event reference instead of moving ownership

Changed to pass a reference to the event (&event) rather than moving ownership, which improves memory efficiency. This change is consistent with similar changes throughout the codebase.


434-435: Method update: Tags::new to Tags::from_list

This change is consistent with the pattern applied throughout the codebase, likely to fix message parsing issues.


446-446: Memory optimization: Passing event reference instead of moving ownership

Changed to pass a reference to the event (&event) rather than moving ownership, which improves memory efficiency. This change is consistent with similar changes throughout the codebase.


527-527: Memory optimization: Passing event reference instead of moving ownership

Changed to pass a reference to the event (&event) rather than moving ownership, which improves memory efficiency. This change is consistent with similar changes throughout the codebase.

@grunch grunch requested a review from Catrya March 24, 2025 14:09
@grunch

grunch commented Mar 24, 2025

Copy link
Copy Markdown
Member

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Mar 24, 2025

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

@grunch grunch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@grunch grunch merged commit 9c06e48 into develop Mar 24, 2025
@grunch grunch deleted the sdk-40-issue branch March 24, 2025 17:14
arkanoider added a commit that referenced this pull request Apr 30, 2025
* 📝 Add docstrings to `feature-full-privacy-mode-checks` (#455)

* add feature to check correctly if order is full privacy or normal

* 📝 Add docstrings to `feature-full-privacy-mode-checks`

Docstrings generation was requested by @arkanoider.

* #454 (comment)

The following files were modified:

* `src/app/order.rs`
* `src/util.rs`

---------

Co-authored-by: arkanoider <github.913zc@simplelogin.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: arkanoider <113362043+arkanoider@users.noreply.github.com>

* Fix network in order event (#452)

Now in orers event 38383 the network that comes out is the correct one

* Feature anyhow removal (#459)

* remove of anyhow dependency start

* removed anyhow dependecy

* remove patch from cargo.toml

* Update mostro-core version

---------

Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>

* Feature disputes (#463)

* Feature: dispute fix for new logic

* fix for coop cancel case

* Add logic for info message about dispute for solver

* first working dispute info message for admin after taking disputes

* added fields to dispute info message for solver

* removed some probably useless and refactored a bit admin-take-dispute function

* clippy suggestion

* other cosmetics to admin take file

* some other cosmetics on admin-take-dispute file

* fix on admin-take

* Update admin_take_dispute.rs

* fixed a wrong search of order id in admin take file@

* Fix: added correct check to add solver from admin cli

* fix: wrong check on admin cancel

* Fix initiator pubkey: now is the trade key of initiator not identity

* quick fix for full privacy orders

* new logic with mixed full privacy - regular reputation mode

* Update cargo toml to compile with version 0.6.32 of mostro-core - this is in preparation for migrating to nostr-sdk 0.40

* rollback sdk to 0.38 version to merge disputes - we will come back to 0.40 when we will fix signature issue

* Update mostro-core dependency

---------

Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>

* added correction for the case of buyer adding back a new invoice after payment failure (#462)

* Fix for nostr sdk 40 issue on incoming message (#465)

* testing sdk 40

* fix for sdk 40

* cleaned cargo.toml

* Bump mostro core version

---------

Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>

* fix: sends order with an updated 'status' field as the reply to add-invoice when there is a preimage (#464)

* Feature-nip69-order-status (#467)

* feature: align to nip69 order status in nostr event

* Improved in progress nip69 logic

* refined event states of the order with nip69 request

* add another check to avoid multiple events with in-progress state

* Privacy range order fix (#468)

* add optional field for users in full privacy inside solver message

* To be tested - privacy range child order fix

* fix cargo.toml

* bumped mostro-core version to 6.35

* New user or privacy order send same user info with zeros (#471)

* new user or privacy order send same user info with zeros

* fix days field and removed some println! macro with nicer tracing messages

* bumped mostro-core version to 6.36 and added requested fix to compile

* Add previous order state in database for solver message (#472)

* add previous state in database for solver message

* bumped mostro-core version to 6.36

* Taker info message to maker.

I think that could be modifies in a more generic UserInfo.
 (#473)

* added logic to send infos to maker when an order is taken

* Add takers info message for maker

* fix cargo.toml

* set peer pubkey to empty string when notifying taker reputation to maker

* moved user info message to the right point of the order flow

* fix cargo.toml

* changed message struct from UserDisputeInfo to UserInfo

* fix: get master keys for user infos

* Bumped mostro-core to version 6.38

* fix for buy order flow with message to maker (#479)

* Rabbit fixes

---------

Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Catrya <140891948+Catrya@users.noreply.github.com>
Co-authored-by: Bilthon <bilthon@gmail.com>
arkanoider added a commit that referenced this pull request May 18, 2025
* testing sdk 40

* fix for sdk 40

* cleaned cargo.toml

* Bump mostro core version

---------

Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>
grunch added a commit that referenced this pull request May 29, 2025
* 📝 Add docstrings to `feature-full-privacy-mode-checks` (#455)

* add feature to check correctly if order is full privacy or normal

* 📝 Add docstrings to `feature-full-privacy-mode-checks`

Docstrings generation was requested by @arkanoider.

* #454 (comment)

The following files were modified:

* `src/app/order.rs`
* `src/util.rs`

---------

Co-authored-by: arkanoider <github.913zc@simplelogin.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: arkanoider <113362043+arkanoider@users.noreply.github.com>

* Feature disputes (#463)

* Feature: dispute fix for new logic

* fix for coop cancel case

* Add logic for info message about dispute for solver

* first working dispute info message for admin after taking disputes

* added fields to dispute info message for solver

* removed some probably useless and refactored a bit admin-take-dispute function

* clippy suggestion

* other cosmetics to admin take file

* some other cosmetics on admin-take-dispute file

* fix on admin-take

* Update admin_take_dispute.rs

* fixed a wrong search of order id in admin take file@

* Fix: added correct check to add solver from admin cli

* fix: wrong check on admin cancel

* Fix initiator pubkey: now is the trade key of initiator not identity

* quick fix for full privacy orders

* new logic with mixed full privacy - regular reputation mode

* Update cargo toml to compile with version 0.6.32 of mostro-core - this is in preparation for migrating to nostr-sdk 0.40

* rollback sdk to 0.38 version to merge disputes - we will come back to 0.40 when we will fix signature issue

* Update mostro-core dependency

---------

Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>

* Fix for nostr sdk 40 issue on incoming message (#465)

* testing sdk 40

* fix for sdk 40

* cleaned cargo.toml

* Bump mostro core version

---------

Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>

* Feature-nip69-order-status (#467)

* feature: align to nip69 order status in nostr event

* Improved in progress nip69 logic

* refined event states of the order with nip69 request

* add another check to avoid multiple events with in-progress state

* Taker info message to maker.

I think that could be modifies in a more generic UserInfo.
 (#473)

* added logic to send infos to maker when an order is taken

* Add takers info message for maker

* fix cargo.toml

* set peer pubkey to empty string when notifying taker reputation to maker

* moved user info message to the right point of the order flow

* fix cargo.toml

* changed message struct from UserDisputeInfo to UserInfo

* fix: get master keys for user infos

* Bumped mostro-core to version 6.38

* first commit for database encryption

* Moving on with encryption work

* Implemented base64 on cipher

* Takebuy and takesell pending orders check now working with encryption

* Removed unwraps

* Finishing code: changed password management and completing checks

* Finishing code: used hashset for search

* added unit test for evaluate timings

* first order cycle completed - to be fixed empty password case

* Added logic for testing a caching idea using hashmap to speed up search of identity keys when encrypted

* ready to start review process

* Cleaned test

* removed patch from cargo.toml

* Rabbit fixes

* some improvements to have also rating working with encryption

* Removed useless call in db.rs

* fix for tests

* Cosmetics on release.rs - improved a bit a function in more ribust way

* fix: wrong check on full privacy orders in disputes

* fix: more meaningful variable name in dispute setup

* fix: wrong commented lines reactivated

* fix: some fixes in privacy orders with encryption

* fix: some fixes following coderabbit advices

Introduced a random delay on password entering to avoid timing attacks.

Removed awaiting from store_encrypted function that does not require async.

* refactor: removed many useless connection opening to database with direct use of borrowed pool variable

* chore: more meaningful text for user in db.rs

* chore: improved encryption unit test

now fix salt for testing works - fix salt and password are used to test timings with decryption using key caching, to test it do:

cargo test test_fetch_string_column_scalar -- --nocapture

* refactor: some improvements in db.rs

improved password error management

reduce code duplication for pending orders checks

* chore(rebase-on-main): commit cargo.lock for rebasing on top of main encrytpion

* feat: completing refactor and rebase of encryption pr

* refactor: introduced new refactoring of encryption-decryption function

- cargo compiles now, we will have to create the new mostro-core after some testings.

- bumped nostr-sdk to 0.41 release, no breaking changes

* chore: temporary added git branch of mostro-core to compile

* chore(test): fix tests

* chore: bumped mostro-core version in cargo.toml

* fix: wrong database path string creation

* chore: fix tests in db.rs

* fix: child order after a trade with a full privacy user have wrong master keys

* fix: typo on check password hash function

* fix: child order event after a complete order does not send user information also if order is normal (not full privacy)

* fix: if a child order come from a privacy order user rating are sent in the event wit all zeroes

* fix: fixed bug found by Catrya

- when I recreate the child order master keys were saved without encryption
  so when a takesell or takebuy comes in the search for pending user orders generate a decrytpion error.

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Francisco Calderón <fjcalderon@gmail.com>
@coderabbitai coderabbitai Bot mentioned this pull request Oct 3, 2025
@coderabbitai coderabbitai Bot mentioned this pull request Oct 13, 2025
@coderabbitai coderabbitai Bot mentioned this pull request Mar 25, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants