Skip to content

Conversation

@arkanoider
Copy link
Collaborator

@arkanoider arkanoider commented Mar 6, 2025

@Catrya,

this is the possible fix for issue #460.

If you can test it it would be great, i'm in office now.

Summary by CodeRabbit

  • Refactor
    • Streamlined invoice payment processing for a smoother experience.
  • Bug Fixes
    • Restored proper network status handling to ensure reliable operations.
  • Style
    • Improved code organization and tag formatting for enhanced clarity.
  • Documentation
    • Refined documentation formatting for clearer guidance.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 6, 2025

Walkthrough

This update renames and refines the invoice processing function. The original check_order_status function in the invoice module has been renamed to pay_new_invoice and its behavior streamlined to update orders and enqueue messages without extensive status checks. Additionally, the Lightning Network status import has been reintroduced in the main module and adjusted within another module for improved code organization. Minor cosmetic changes to documentation comments in the utility module have also been applied.

Changes

File(s) Change Summary
src/app/add_invoice.rs Renamed function from check_order_status to pay_new_invoice. Removed detailed status checks and streamlined the control flow to reset attempts, update the order, and enqueue a message.
src/main.rs, src/nip33.rs Reintroduced Lightning status import: use crate::lightning::LnStatus; in main and activated the LN_STATUS import in nip33 with formatting improvements.
src/util.rs Documentation comments updated: Removed blank lines in the comments for get_tags_for_new_order and publish_order functions for cleaner formatting.

Sequence Diagram(s)

sequenceDiagram
    participant Action as add_invoice_action
    participant Invoice as pay_new_invoice
    participant DB as Database
    participant MQ as MessageQueue

    Action->>Invoice: Check order status for SettledHoldInvoice
    Invoice->>Invoice: Reset payment attempts
    Invoice->>DB: Update order record
    Invoice->>MQ: Enqueue invoice update message
    Invoice-->>Action: Return success
Loading

Suggested reviewers

  • Catrya

Poem

I'm a rabbit coding with glee,
Hopping through changes, wild and free.
Invoice flows now smooth and fine,
Lightning imports make the code shine.
With each hop, bugs run away—
Happy code, bright as day!
🐰✨


📜 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 78e1dcc and 994f220.

📒 Files selected for processing (4)
  • src/app/add_invoice.rs (3 hunks)
  • src/main.rs (1 hunks)
  • src/nip33.rs (2 hunks)
  • src/util.rs (2 hunks)
✅ Files skipped from review due to trivial changes (2)
  • src/nip33.rs
  • src/util.rs
🔇 Additional comments (3)
src/main.rs (1)

18-18: Appropriate re-addition of the LnStatus import

This import is necessary as the LnStatus type is used later in the code, specifically at line 34 for the static LN_STATUS variable and at line 97 where LnStatus::from_get_info_response is called.

src/app/add_invoice.rs (2)

15-36: Good refactoring to improve separation of concerns

The function has been renamed from check_order_status to pay_new_invoice, which now more accurately reflects its purpose. The implementation has been streamlined to focus solely on processing the payment without performing status checks, which are now handled by the calling function.


59-68: Well-structured status handling logic

The status checking logic has been appropriately moved to the add_invoice_action function, which makes the code more maintainable. The match statement properly handles different order statuses and calls pay_new_invoice only when the status is SettledHoldInvoice.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 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.

@arkanoider arkanoider requested review from Catrya and grunch March 6, 2025 11:08
Copy link
Member

@Catrya Catrya left a comment

Choose a reason for hiding this comment

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

Hi @arkanoider It is now fixed when invoice payment fails 😃
However, when invoice payment does not fail, and the exchange is successful, order event 38383 is not published with the status success, but only the settled-hold-invoice is published. In the database, the order is correctly set to success

@arkanoider
Copy link
Collaborator Author

Hi @arkanoider It is now fixed when invoice payment fails 😃 However, when invoice payment does not fail, and the exchange is successful, order event 38383 is not published with the status success, but only the settled-hold-invoice is published. In the database, the order is correctly set to success

Fix a bug --> introduce a bug --> fix a bug -->

the cycle of bugs...will check probably it's a stupid thing.

@arkanoider
Copy link
Collaborator Author

arkanoider commented Mar 7, 2025

@Catrya after the test of yesterday seems that the event is correctly sent from mostrod logs, please check if you can check your local relay because with this tool:
lightningk0ala

I can see my success trade of yesterday also buy if you put the query with our test relay:
wss://relay.mostro.network

and query:
[{"authors": ["acf3c926f37102b03a5ff8a83fa4480af59452e47d84e15513e5adfa6c2aac83"], "kinds": [38383]}]

which is my pubkey of local mostrod.

Let me know if your relay has published them.

For example you can see one with success in the image below:

image

@arkanoider arkanoider requested a review from Catrya March 7, 2025 14:48
@Catrya
Copy link
Member

Catrya commented Mar 7, 2025

@arkanoider One question, your Mostro was publishing those events in more than 1 relay at the same time?

I found that when my Mostro publishes in more than 1 relay the success event is published. ['ws://localhost:7000','wss://relay.mostro.network']

But then I tested it using only 1 relay and the last order event was settle-hold-invoice. Did it using 'wss://relay.mostro.network' you can check it in the events of that relay, order b8c6ead1-99ba-42db-8dfe-455121c0f0b3. i use nostreq --kinds 38383 --limit 300 --authors 00000018c1ae3147f9010b5d768ec2b121acf1a67f238be8e2bdd7c1f9eef705 | nostcat --stream wss://relay.mostro.network

look the mostro logs here https://pastebin.com/Uv6bbXPU

Maybe it's not serious, but it is weird.

Could you check to complete a purchase order publishing in only 1 relay please?

@arkanoider
Copy link
Collaborator Author

@arkanoider One question, is your Mostro was publishing those events more than 1 relay at the same time?

I found that when my Mostro publishes in more than 1 relay the success event is published. ['ws://localhost:7000','wss://relay.mostro.network']

But then I tested it using only 1 relay the last order event was settle-hold-invoice one. Did it using 'wss://relay.mostro.network' you can check it in the events of that relay, order b8c6ead1-99ba-42db-8dfe-455121c0f0b3. i use nostreq --kinds 38383 --limit 300 --authors 00000018c1ae3147f9010b5d768ec2b121acf1a67f238be8e2bdd7c1f9eef705 | nostcat --stream wss://relay.mostro.network

look the mostro logs here https://pastebin.com/Uv6bbXPU

Maybe it's not serious, but it is weird.

Could you check to complete a purchase order publishing in only 1 relay please?

I will check tonite at home, in my mind i am using only that relay, but I don't remember if I use also Bilthon's one.

Will check tonite and come back to you @Catrya !

Copy link
Member

@Catrya Catrya left a comment

Choose a reason for hiding this comment

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

tACK, great job @arkanoider

Copy link
Member

@grunch grunch left a comment

Choose a reason for hiding this comment

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

LGTM

@grunch grunch merged commit a9b99d7 into develop Mar 21, 2025
1 check passed
@grunch grunch deleted the pay-new-added-invoice branch March 21, 2025 18:53
This was referenced Mar 26, 2025
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>
@coderabbitai coderabbitai bot mentioned this pull request Oct 29, 2025
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.

4 participants