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

implement request_id on mostro messages #376

Merged
merged 9 commits into from
Nov 7, 2024
Merged

implement request_id on mostro messages #376

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
535ce42
testing for bilthon
arkanoider Oct 22, 2024
7e9a7f6
request_id introduced
arkanoider Oct 22, 2024
fb2499e
fix tests
arkanoider Oct 22, 2024
1bf8729
fixed cargo.toml
arkanoider Oct 22, 2024
76f65dc
updated mostro-core
arkanoider Oct 27, 2024
169a763
Use request_id from parameter
grunch Nov 6, 2024
1689ba9
Replaced last get_inner_message_kind().request_id
grunch Nov 6, 2024
18279af
Merge branch 'main' into bilthon-test
grunch Nov 7, 2024
2459b0e
Fix test
grunch Nov 7, 2024
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
1,144 changes: 594 additions & 550 deletions Cargo.lock
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ uuid = { version = "1.8.0", features = [
"serde",
] }
reqwest = { version = "0.12.1", features = ["json"] }
mostro-core = { version = "0.6.8", features = ["sqlx"] }
mostro-core = { version = "0.6.9", features = ["sqlx"] }
tracing = "0.1.40"
tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
config = "0.14.0"
Expand Down
56 changes: 36 additions & 20 deletions src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,39 +79,49 @@ pub async fn run(
match message {
Ok(msg) => {
if msg.get_inner_message_kind().verify() {
let request_id = msg.get_inner_message_kind().request_id;

Comment on lines +85 to +86
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation for request_id extraction

Consider adding proper validation and error handling for the request_id extraction. This would help prevent potential issues with invalid or missing request IDs.

- let request_id = msg.get_inner_message_kind().request_id;
+ let request_id = msg.get_inner_message_kind().request_id;
+ if !request_id.is_valid() {  // Add appropriate validation method
+     tracing::warn!("Invalid request ID received");
+     continue;
+ }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now request_id is an Option we should be safe.

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. Since request_id is now an Option, that should handle cases where it's missing or invalid. Thanks for the clarification.

✏️ Learnings added 
Learnt from: arkanoider
PR: MostroP2P/mostro#376
File: src/app.rs:82-83
Timestamp: 2024-11-07T09:44:16.848Z
Learning: In `src/app.rs`, the `request_id` field is an `Option`, ensuring safety against invalid or missing request IDs without the need for additional validation.

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.

if let Some(action) = msg.inner_action() {
match action {
Action::NewOrder => {
if let Err(e) =
order_action(msg, &event, &my_keys, &pool).await
if let Err(e) = order_action(
msg, &event, &my_keys, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::TakeSell => {
if let Err(e) =
take_sell_action(msg, &event, &my_keys, &pool).await
if let Err(e) = take_sell_action(
msg, &event, &my_keys, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::TakeBuy => {
if let Err(e) =
take_buy_action(msg, &event, &my_keys, &pool).await
if let Err(e) = take_buy_action(
msg, &event, &my_keys, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::FiatSent => {
if let Err(e) =
fiat_sent_action(msg, &event, &my_keys, &pool).await
if let Err(e) = fiat_sent_action(
msg, &event, &my_keys, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::Release => {
if let Err(e) = release_action(
msg, &event, &my_keys, &pool, ln_client,
msg, &event, &my_keys, &pool, ln_client, request_id,
)
.await
{
Expand All @@ -120,17 +130,18 @@ pub async fn run(
}
Action::Cancel => {
if let Err(e) = cancel_action(
msg, &event, &my_keys, &pool, ln_client,
msg, &event, &my_keys, &pool, ln_client, request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::AddInvoice => {
if let Err(e) =
add_invoice_action(msg, &event, &my_keys, &pool)
.await
if let Err(e) = add_invoice_action(
msg, &event, &my_keys, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
Expand All @@ -143,22 +154,25 @@ pub async fn run(
&my_keys,
&pool,
rate_list.clone(),
request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::Dispute => {
if let Err(e) =
dispute_action(msg, &event, &my_keys, &pool).await
if let Err(e) = dispute_action(
msg, &event, &my_keys, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::AdminCancel => {
if let Err(e) = admin_cancel_action(
msg, &event, &my_keys, &pool, ln_client,
msg, &event, &my_keys, &pool, ln_client, request_id,
)
.await
{
Expand All @@ -167,7 +181,7 @@ pub async fn run(
}
Action::AdminSettle => {
if let Err(e) = admin_settle_action(
msg, &event, &my_keys, &pool, ln_client,
msg, &event, &my_keys, &pool, ln_client, request_id,
)
.await
{
Expand All @@ -176,16 +190,18 @@ pub async fn run(
}
Action::AdminAddSolver => {
if let Err(e) = admin_add_solver_action(
msg, &event, &my_keys, &pool,
msg, &event, &my_keys, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
}
Action::AdminTakeDispute => {
if let Err(e) =
admin_take_dispute_action(msg, &event, &pool).await
if let Err(e) = admin_take_dispute_action(
msg, &event, &pool, request_id,
)
.await
{
warning_msg(&action, e)
}
Expand Down
35 changes: 30 additions & 5 deletions src/app/add_invoice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub async fn add_invoice_action(
event: &UnwrappedGift,
my_keys: &Keys,
pool: &Pool<Sqlite>,
request_id: u64,
) -> Result<()> {
let order_msg = msg.get_inner_message_kind();
let mut order = if let Some(order_id) = order_msg.id {
Expand Down Expand Up @@ -54,7 +55,7 @@ pub async fn add_invoice_action(
};
// Only the buyer can add an invoice
if buyer_pubkey != event.sender {
send_cant_do_msg(Some(order.id), None, &event.sender).await;
send_cant_do_msg(request_id, Some(order.id), None, &event.sender).await;
return Ok(());
}

Expand All @@ -73,8 +74,14 @@ pub async fn add_invoice_action(
{
Ok(_) => payment_request,
Err(_) => {
send_new_order_msg(Some(order.id), Action::IncorrectInvoiceAmount, None, &event.sender)
.await;
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::IncorrectInvoiceAmount,
None,
&event.sender,
)
.await;
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use the function parameter request_id instead of extracting from message.

There's an inconsistency in how the request_id is obtained. The function receives request_id as a parameter, but here it's being extracted from the message again. This could lead to confusion if they ever differ.

Apply this change for consistency:

     send_new_order_msg(
-        msg.get_inner_message_kind().request_id,
+        request_id,
         Some(order.id),
         Action::IncorrectInvoiceAmount,
         None,
         &event.sender,
     )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::IncorrectInvoiceAmount,
None,
&event.sender,
)
.await;
send_new_order_msg(
request_id,
Some(order.id),
Action::IncorrectInvoiceAmount,
None,
&event.sender,
)
.await;

Copy link
Member

Choose a reason for hiding this comment

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

Hi @arkanoider if you made this change please solve this conversation

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

return Ok(());
}
}
Expand All @@ -91,11 +98,19 @@ pub async fn add_invoice_action(
Status::SettledHoldInvoice => {
order.payment_attempts = 0;
order.clone().update(pool).await?;
send_new_order_msg(Some(order.id), Action::InvoiceUpdated, None, &buyer_pubkey).await;
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::InvoiceUpdated,
None,
&buyer_pubkey,
)
.await;
arkanoider marked this conversation as resolved.
Show resolved Hide resolved
return Ok(());
}
_ => {
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::NotAllowedByStatus,
None,
Expand Down Expand Up @@ -140,6 +155,7 @@ pub async fn add_invoice_action(

// We send a confirmation message to seller
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::BuyerTookOrder,
Some(Content::Order(order_data.clone())),
Expand All @@ -148,14 +164,23 @@ pub async fn add_invoice_action(
.await;
// We send a message to buyer saying seller paid
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::HoldInvoicePaymentAccepted,
Some(Content::Order(order_data)),
&buyer_pubkey,
)
.await;
} else {
show_hold_invoice(my_keys, None, &buyer_pubkey, &seller_pubkey, order).await?;
show_hold_invoice(
my_keys,
None,
&buyer_pubkey,
&seller_pubkey,
order,
msg.get_inner_message_kind().request_id,
)
.await?;
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use the function parameter request_id instead of extracting from message.

Final instance of the same inconsistency with request_id source.

Apply this change:

     show_hold_invoice(
         my_keys,
         None,
         &buyer_pubkey,
         &seller_pubkey,
         order,
-        msg.get_inner_message_kind().request_id,
+        request_id,
     )

Consider implementing a dedicated type for request context that encapsulates both the message and request_id. This would make the relationship between them more explicit and help prevent such inconsistencies. For example:

struct RequestContext {
    message: Message,
    request_id: u64,
}

This aligns with the PR author's suggestion about improving the implementation's organization.

Copy link
Member

Choose a reason for hiding this comment

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

same that above

Copy link
Contributor

Choose a reason for hiding this comment

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

@grunch, understood. I'll note that in this codebase, it's preferred to extract request_id directly from the message within functions.

✏️ Learnings added 
Learnt from: grunch
PR: MostroP2P/mostro#376
File: src/app/add_invoice.rs:175-183
Timestamp: 2024-11-06T15:24:54.881Z
Learning: In this codebase, prefer extracting `request_id` directly from the message within functions, instead of passing it as a function parameter.

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.

}

Ok(())
Expand Down
5 changes: 3 additions & 2 deletions src/app/admin_add_solver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub async fn admin_add_solver_action(
event: &UnwrappedGift,
my_keys: &Keys,
pool: &Pool<Sqlite>,
request_id: u64,
) -> Result<()> {
let inner_message = msg.get_inner_message_kind();
let content = if let Some(content) = &inner_message.content {
Expand All @@ -32,7 +33,7 @@ pub async fn admin_add_solver_action(
// Check if the pubkey is Mostro
if event.sender.to_string() != my_keys.public_key().to_string() {
// We create a Message
send_cant_do_msg(None, None, &event.sender).await;
send_cant_do_msg(request_id, None, None, &event.sender).await;
return Ok(());
}
let public_key = PublicKey::from_bech32(npubkey)?.to_hex();
Expand All @@ -43,7 +44,7 @@ pub async fn admin_add_solver_action(
Err(ee) => error!("Error creating solver: {:#?}", ee),
}
// We create a Message for admin
let message = Message::new_dispute(None, Action::AdminAddSolver, None);
let message = Message::new_dispute(request_id, None, Action::AdminAddSolver, None);
let message = message.as_json()?;
// Send the message
send_dm(&event.sender, message).await?;
Expand Down
12 changes: 10 additions & 2 deletions src/app/admin_cancel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub async fn admin_cancel_action(
my_keys: &Keys,
pool: &Pool<Sqlite>,
ln_client: &mut LndConnector,
request_id: u64,
) -> Result<()> {
let order_id = if let Some(order_id) = msg.get_inner_message_kind().id {
order_id
Expand All @@ -32,6 +33,7 @@ pub async fn admin_cancel_action(
match is_assigned_solver(pool, &event.sender.to_string(), order_id).await {
Ok(false) => {
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order_id),
Action::IsNotYourDispute,
None,
Expand All @@ -57,7 +59,12 @@ pub async fn admin_cancel_action(

// Was order cooperatively cancelled?
if order.status == Status::CooperativelyCanceled.to_string() {
let message = MessageKind::new(Some(order_id), Action::CooperativeCancelAccepted, None);
let message = MessageKind::new(
request_id,
Some(order_id),
Action::CooperativeCancelAccepted,
None,
);
if let Ok(message) = message.as_json() {
let _ = send_dm(&event.sender, message).await;
}
Expand All @@ -66,6 +73,7 @@ pub async fn admin_cancel_action(

if order.status != Status::Dispute.to_string() {
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::NotAllowedByStatus,
None,
Expand Down Expand Up @@ -117,7 +125,7 @@ pub async fn admin_cancel_action(
let order_updated = update_order_event(my_keys, Status::CanceledByAdmin, &order).await?;
order_updated.update(pool).await?;
// We create a Message for cancel
let message = Message::new_order(Some(order.id), Action::AdminCanceled, None);
let message = Message::new_order(request_id, Some(order.id), Action::AdminCanceled, None);
let message = message.as_json()?;
// Message to admin
send_dm(&event.sender, message.clone()).await?;
Expand Down
29 changes: 25 additions & 4 deletions src/app/admin_settle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub async fn admin_settle_action(
my_keys: &Keys,
pool: &Pool<Sqlite>,
ln_client: &mut LndConnector,
request_id: u64,
) -> Result<()> {
let order_id = if let Some(order_id) = msg.get_inner_message_kind().id {
order_id
Expand All @@ -33,6 +34,7 @@ pub async fn admin_settle_action(
match is_assigned_solver(pool, &event.sender.to_string(), order_id).await {
Ok(false) => {
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order_id),
Action::IsNotYourDispute,
None,
Expand All @@ -58,7 +60,12 @@ pub async fn admin_settle_action(

// Was orde cooperatively cancelled?
if order.status == Status::CooperativelyCanceled.to_string() {
let message = MessageKind::new(Some(order_id), Action::CooperativeCancelAccepted, None);
let message = MessageKind::new(
msg.get_inner_message_kind().request_id,
Some(order_id),
Action::CooperativeCancelAccepted,
None,
);
if let Ok(message) = message.as_json() {
let _ = send_dm(&event.sender, message).await;
}
Expand All @@ -67,6 +74,7 @@ pub async fn admin_settle_action(

if order.status != Status::Dispute.to_string() {
send_new_order_msg(
msg.get_inner_message_kind().request_id,
Some(order.id),
Action::NotAllowedByStatus,
None,
Expand All @@ -76,7 +84,15 @@ pub async fn admin_settle_action(
return Ok(());
}

settle_seller_hold_invoice(event, ln_client, Action::AdminSettled, true, &order).await?;
settle_seller_hold_invoice(
event,
ln_client,
Action::AdminSettled,
true,
&order,
request_id,
)
.await?;

let order_updated = update_order_event(my_keys, Status::SettledHoldInvoice, &order).await?;

Expand Down Expand Up @@ -110,7 +126,12 @@ pub async fn admin_settle_action(
NOSTR_CLIENT.get().unwrap().send_event(event).await?;
}
// We create a Message for settle
let message = Message::new_order(Some(order_updated.id), Action::AdminSettled, None);
let message = Message::new_order(
request_id,
Some(order_updated.id),
Action::AdminSettled,
None,
);
let message = message.as_json()?;
// Message to admin
send_dm(&event.sender, message.clone()).await?;
Expand All @@ -121,7 +142,7 @@ pub async fn admin_settle_action(
send_dm(&PublicKey::from_str(buyer_pubkey)?, message.clone()).await?;
}

let _ = do_payment(order_updated).await;
let _ = do_payment(order_updated, Some(request_id)).await;

Ok(())
}
Loading
Loading