Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed wrong request_id in messages #388

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Removed wrong request_id in messages #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/app/admin_take_dispute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,14 +119,14 @@ pub async fn admin_take_dispute_action(
// to them know who will assist them on the dispute
let solver_pubkey = Peer::new(event.sender.to_hex());
let msg_to_buyer = Message::new_order(
request_id,
None,
Some(order.id),
Action::AdminTookDispute,
Some(Content::Peer(solver_pubkey.clone())),
);

let msg_to_seller = Message::new_order(
request_id,
None,
Some(order.id),
Action::AdminTookDispute,
Some(Content::Peer(solver_pubkey)),
Expand Down
2 changes: 1 addition & 1 deletion src/app/fiat_sent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ pub async fn fiat_sent_action(

// We a message to the seller
send_new_order_msg(
request_id,
None,
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Codebase verification

Pattern of request_id handling is inconsistent across the codebase

Based on the search results, there are inconsistencies in how request_id is handled for notifications:

  • In src/app/fiat_sent.rs, request_id is correctly set to None for seller notifications
  • However, similar notification scenarios in other files handle it differently:
    • src/app/cancel.rs: Some notifications use None while others pass the original request_id
    • src/app/add_invoice.rs: Buyer notifications inconsistently handle request_id
    • src/app/release.rs: Mixed usage of None and original request_id for similar notification scenarios
🔗 Analysis chain

LGTM! Correct handling of request_id for notifications.

The change to set request_id to None for the seller's notification is appropriate since the seller didn't make a request and is just being notified of the buyer's action. The original request_id is correctly preserved for the buyer's response.

Let's verify this pattern is consistently applied across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other instances where notifications might need similar request_id handling
# Look for patterns where send_new_order_msg is called with request_id

# Search for all send_new_order_msg calls to review request_id handling
rg -A 5 'send_new_order_msg\(' 

# Search for notification-like scenarios in message sending
ast-grep --pattern 'send_new_order_msg($$$request_id$$$)'

Length of output: 15279

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is normal Rabbit! Not always the use is the same...

Copy link
Contributor

Choose a reason for hiding this comment

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

@arkanoider, understood! If the varying use of request_id is intentional, then no changes are needed. Thank you for clarifying.

✏️ Learnings added 
Learnt from: arkanoider
PR: MostroP2P/mostro#388
File: src/app/fiat_sent.rs:69-69
Timestamp: 2024-11-08T20:37:58.724Z
Learning: In the MostroP2P codebase, variations in `request_id` handling for notifications are acceptable and may vary depending on the specific context and requirements.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Some(order.id),
Action::FiatSentOk,
Some(Content::Peer(peer)),
Expand Down
Loading