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 all 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 @@ -82,39 +82,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 @@ -123,17 +133,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 @@ -146,22 +157,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 @@ -170,7 +184,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 @@ -179,16 +193,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
26 changes: 23 additions & 3 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 @@ -74,6 +75,7 @@ pub async fn add_invoice_action(
Ok(_) => payment_request,
Err(_) => {
send_new_order_msg(
request_id,
Some(order.id),
Action::IncorrectInvoiceAmount,
None,
Expand All @@ -96,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(
request_id,
Some(order.id),
Action::InvoiceUpdated,
None,
&buyer_pubkey,
)
.await;
return Ok(());
}
_ => {
send_new_order_msg(
request_id,
Some(order.id),
Action::NotAllowedByStatus,
None,
Expand Down Expand Up @@ -145,6 +155,7 @@ pub async fn add_invoice_action(

// We send a confirmation message to seller
send_new_order_msg(
request_id,
Some(order.id),
Action::BuyerTookOrder,
Some(Content::Order(order_data.clone())),
Expand All @@ -153,14 +164,23 @@ pub async fn add_invoice_action(
.await;
// We send a message to buyer saying seller paid
send_new_order_msg(
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,
request_id,
)
.await?;
}

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
let sender_keys = crate::util::get_keys().unwrap();
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 @@ -21,6 +21,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 @@ -31,6 +32,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 @@ -56,7 +58,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 sender_keys = crate::util::get_keys().unwrap();
let _ = send_dm(&event.sender, sender_keys, 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 @@ -124,7 +132,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
let sender_keys = crate::util::get_keys().unwrap();
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 @@ -24,6 +24,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 @@ -34,6 +35,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 @@ -59,7 +61,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 sender_keys = crate::util::get_keys().unwrap();
let _ = send_dm(&event.sender, sender_keys, message).await;
Expand All @@ -69,6 +76,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 @@ -78,7 +86,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 @@ -121,7 +137,12 @@ pub async fn admin_settle_action(
}
}
// 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
let sender_keys = crate::util::get_keys().unwrap();
Expand All @@ -143,7 +164,7 @@ pub async fn admin_settle_action(
.await?;
}

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

Ok(())
}
Loading
Loading