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

Jump to bottom

POST /api/order cancel should include current status #1311

Open
jerryfletcher21 opened this issue Jun 1, 2024 · 7 comments · May be fixed by #1326
Open

POST /api/order cancel should include current status #1311

jerryfletcher21 opened this issue Jun 1, 2024 · 7 comments · May be fixed by #1326
Labels
⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin enhancement 🆙 New feature or request good first issue Good for newcomers

Comments

@jerryfletcher21
Copy link
Contributor

Describe the bug
It is not a bug, it is a security improvement (for the client).
When a POST requests is make to /api/order with action cancel, the order is cancelled. Depending on the current status of the order, a penalty may be inflicted on the canceller. The point is that the client can not be 100% percent sure that the current status of the order is the one it think it is. A public order may for example have been taken after the last request of the client to get the current status.

By adding a new parameter in the requests, (current_status for example), the client specifes the status in which it wants to cancel the offer. The server then proceeds in cancelling the offer just if the current status is the one specified in the request, if not it simply returns an error saying that current status is not the correct one.

To Reproduce
Make a POST requsts at /api/order with action cancel

Expected behavior
Prevent the client from cancelling an offer in the wrong status
@Reckless-Satoshi Reckless-Satoshi added enhancement 🆙 New feature or request good first issue Good for newcomers ⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin labels Jun 2, 2024
@Reckless-Satoshi
Copy link
Collaborator

This is a great solution to the problem 👍 It is rare, but it can happen in our official client that a maker cancels the order while still public and by the time the request hits, a taker has shown up, therefore making the maker lose his bond without any previous notification and no recourse.

Implementing this fix on the backend and client is rewarded with ⚡ 100K Sats ⚡ (60K Sats for a PR with a backend only fix, and 30K Sats for a PR with client only fix)

@aftermath2
Copy link
Contributor

@Reckless-Satoshi I think I can start working on this, could you assign me?

@jerryfletcher21 jerryfletcher21 mentioned this issue Jun 15, 2024
Closed
1 task
@jerryfletcher21
Copy link
Contributor Author

Hello @aftermath2, I have made a PR that implements this fix on the backend. I am not familiar with frontend sutff, you can work on the frontend if you want.

@aftermath2 aftermath2 linked a pull request Jun 15, 2024 that will close this issue
Open
1 task
@aftermath2
Copy link
Contributor

@jerryfletcher21 I've already made the changes to the backend and frontend (PR), sorry I didn't know you were working on it.

@jerryfletcher21
Copy link
Contributor Author

jerryfletcher21 commented Jun 16, 2024 •
edited
Loading

@aftermath2 ah that is a bit unfortunate that we both worked at the backed. Let's wait for @Reckless-Satoshi's feedbacks and then we may do a a single pull requests.

@Reckless-Satoshi
Copy link
Collaborator

Haha, okay this is a failure guys! 😄 🤣

To not over-complicate things, we'll merge #1326 but there is a few things from #1325 that make sense to incorporate. More details on #1325 #1326 reviews.

@aftermath2
Copy link
Contributor

@Reckless-Satoshi Just rebased to main to pull the latest changes, resolved a conflict and ran the tests. I think the issue is ready to review, let me know if there are further changes needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
⚡Eligible for Sats ⚡ This issue or pull request rewards bitcoin enhancement 🆙 New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add cancel_status param to the cancel order action aftermath2/robosats
3 participants
@Reckless-Satoshi @aftermath2 @jerryfletcher21