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

feat: handles cancel request for already cancelled order & removes redundant check #393

Closed
wants to merge 1 commit into from
Closed

feat: handles cancel request for already cancelled order & removes redundant check #393

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
18 changes: 16 additions & 2 deletions src/app/cancel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,21 @@ pub async fn cancel_action(
}
};

if order.status == Status::Canceled.to_string()
|| order.status == Status::CooperativelyCanceled.to_string()
|| order.status == Status::CanceledByAdmin.to_string()
{
error!("Order {} is already canceled", order_id);
send_cant_do_msg(
request_id,
Some(order_id),
Some("Order already canceled".to_string()),
Copy link
Collaborator

@Catrya Catrya Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the idea is to send messages with just the action, without a text string inside, so that clients can interpret that action and show a message to users with the text they want and in the language they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, I somehow missed this.

Yes, the idea is to replace custom messages with static codes that clients can map to localized, human-readable messages. However, I don’t think we have those static codes implemented yet, so for now, I decided to stick with the legacy method of using hard-coded, human-readable error messages.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually there are, there are no longer any actions with text strings in the code, all the ones that had them were replaced by one of these: https://github.com/MostroP2P/mostro-core/blob/c0cd7bbd263a303e7aab46588362839e836f0ac0/src/message.rs#L37C1-L84C2

Perhaps in this case just send the cant do msg, but if is needit something more descriptive, create it as a new action
But if don't do it for now, at least keep it pending to don't forget later, because since there are no more actions with text string we could forget that was added one recently and not change it later. My 2 cents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. The thing is, we can have many many different reasons for errors like CantDo. Expanding this set for every new specific error we want to convey to clients would result in an unbounded number of actions, making CantDo itself too vague in the process.

Instead I'd suggest keeping the action list concise and grouping all specific error messages into a new enum, which we could call ErrorMessage. What do you say @grunch?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this response, yes I think it makes sense @bilthon, if you want you want to do it please go first to mostro-core and then change it here

Copy link
Member

Choose a reason for hiding this comment

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

Here the proposal for CantDoReason error handling

MostroP2P/mostro-core#69

And here the merged PR

MostroP2P/mostro-core#70

It is already working on mostro-core 0.6.15

&event.sender,
)
.await;
return Ok(());
}

if order.status == Status::Pending.to_string() {
// Validates if this user is the order creator
if user_pubkey != order.creator_pubkey {
Expand Down Expand Up @@ -73,8 +88,7 @@ pub async fn cancel_action(
}

if order.kind == OrderKind::Sell.to_string()
&& (order.status == Status::WaitingBuyerInvoice.to_string()
|| order.status == Status::WaitingBuyerInvoice.to_string())
&& (order.status == Status::WaitingBuyerInvoice.to_string())
{
cancel_add_invoice(ln_client, &mut order, event, pool, my_keys, request_id).await?;
}
Expand Down
Loading